From patchwork Fri Sep 3 07:46:10 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Rybchenko X-Patchwork-Id: 97875 X-Patchwork-Delegate: ferruh.yigit@amd.com Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 72E83A0C53; Fri, 3 Sep 2021 09:46:42 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F3ED840DDE; Fri, 3 Sep 2021 09:46:41 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 41754406B4 for ; Fri, 3 Sep 2021 09:46:40 +0200 (CEST) Received: by shelob.oktetlabs.ru (Postfix, from userid 122) id 9DF147F6D6; Fri, 3 Sep 2021 10:46:39 +0300 (MSK) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on shelob.oktetlabs.ru X-Spam-Level: X-Spam-Status: No, score=0.8 required=5.0 tests=ALL_TRUSTED, DKIM_ADSP_DISCARD, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from aros.oktetlabs.ru (aros.oktetlabs.ru [192.168.38.17]) by shelob.oktetlabs.ru (Postfix) with ESMTP id A43147F52A; Fri, 3 Sep 2021 10:46:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru A43147F52A Authentication-Results: shelob.oktetlabs.ru/A43147F52A; dkim=none; dkim-atps=neutral From: Andrew Rybchenko To: Thomas Monjalon , Ferruh Yigit , Ori Kam Cc: dev@dpdk.org, Eli Britstein , Ilya Maximets , Ajit Khaparde , Ivan Malov , John Daley , Hyong Youb Kim , Jerin Jacob , Nithin Dabilpuram , Kiran Kumar K , Somnath Kotur Date: Fri, 3 Sep 2021 10:46:10 +0300 Message-Id: <20210903074610.313622-1-andrew.rybchenko@oktetlabs.ru> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20210601111420.5549-1-ivan.malov@oktetlabs.ru> References: <20210601111420.5549-1-ivan.malov@oktetlabs.ru> MIME-Version: 1.0 Subject: [dpdk-dev] [PATCH v1] ethdev: clarify flow action PORT ID semantics X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" From: Ivan Malov Definition of action PORT_ID is ambiguous. Documentation says "Directs matching traffic to a given DPDK port ID." It suggests that an application will receive corresponding traffic on the port using ethdev Rx burst API. Some network PMDs implement PORT_ID action this way. However, OvS+DPDK uses PORT_ID action a way which is natural it to bypass OvS and redirect corresponding traffic to wire if the DPDK port is a physical function or to a virtual function if the DPDK port is a VF representor. Anyway corresponding packets will not be received using ethdev Rx burst API by the OvS+DPDK. Other network PMDs implement the PORT_ID action following the semantics. The latter semantics may be explained to match the above definition taking port representor definition into account which says that port representor is a ghost of the real port and redirecting a traffic to it means redirecting the traffic to the corresponding real port. However, representors are not the only use case, and solution should be extended anyway to support both options. Since these interpretations of the PORT_ID action semantics are basically opposite and both have reasons behind, it is bad to silently break some applications changing default meaning. So make it backward incompatible to ensure that all applications are updated to use it in a right way. Stick to ingress/egress terminology to specify direction since it is well defined from DPDK application point of view: - ingress to be received via the specified port; - egress as if it would-be transmitted via the specified port. Signed-off-by: Ivan Malov Signed-off-by: Andrew Rybchenko --- The patch is a follow up of the long discussion of the RFC [1]. It is just an attempt to summarize some results of the discussion. Small quote from the discussion: On June 3, 2021, 11:05 a.m. UTC, Ilya Maximets wrote: > On June 3, 2021, 10:33 a.m. UTC, Andrew Rybchenko wrote: > > > > A. Add "ingress" bit with "egress" as unset meaning. > > Yes, that's what is current behaviour assumed and > > used by OvS and implemented in some PMDs. > > My problem with it that it is, IMHO, inconsistent > > default value (as explained above). > > > > B. Add "egress" bit with "ingress" as unset meaning. > > Basically it is what is suggested in the RFC, but > > the problem of the suggestion is the silent breakage > > of existing users (let's put it a side if it is > > correct usage or misuse). It is still the fact. > > > > C. Encode above in ethdev port ID MSB. > > The problem of the solution is that encoding > > makes sense for representors, but the problem > > exists for non-representor ports as well. > > I have no good ideas on terminology in the case > > if we try to solve it for non-representors. > > > > D. Break API and ABI and add enum with unset(default)/ > > ingress/egress members to enforce application to > > specify direction. > > > > It is unclear what we'll do in the case of A, B and D > > if we encode representor in port ID MSB in any case. > > My opinion: > > - Option D is the best choice for rte_flow. No defaults, users forced > to explicitly choose the direction in HW-independent way. > > - I agree that option C somewhat conflicts with the 'ingress/egress' > flag idea and it is more hardware-specific. Therefore if option C > is going to be implemented it should be implemented in concept of > option A, i.e. 'egress' is default option if port ID MSB is not set. If the solution is accepted, testpmd must be updated to require action direction specification and drivers which implement the action must be updated to handle direction properly and reject unspecified value with appropriate error message. If the patch does not address all concerns and there is still significant disagreement on the topic, it would be good to schedule call to discuss it. [1] https://patches.dpdk.org/project/dpdk/patch/20210601111420.5549-1-ivan.malov@oktetlabs.ru/ doc/guides/prog_guide/rte_flow.rst | 7 ++++++- doc/guides/rel_notes/deprecation.rst | 6 ------ doc/guides/rel_notes/release_21_11.rst | 4 ++++ lib/ethdev/rte_flow.h | 25 ++++++++++++++++++++++++- 4 files changed, 34 insertions(+), 8 deletions(-) diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst index 2b42d5ec8c..89404b38af 100644 --- a/doc/guides/prog_guide/rte_flow.rst +++ b/doc/guides/prog_guide/rte_flow.rst @@ -1919,7 +1919,10 @@ See `Item: PHY_PORT`_. Action: ``PORT_ID`` ^^^^^^^^^^^^^^^^^^^ -Directs matching traffic to a given DPDK port ID. + +Directs matching traffic to the specified DPDK port (ingress) or to +the would-be destination as if the application itself sent this traffic +from the said DPDK port (egress). See `Item: PORT_ID`_. @@ -1934,6 +1937,8 @@ See `Item: PORT_ID`_. +--------------+---------------------------------------+ | ``id`` | DPDK port ID | +--------------+---------------------------------------+ + | ``dir`` | egress or ingress | + +--------------+---------------------------------------+ Action: ``METER`` ^^^^^^^^^^^^^^^^^ diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst index 76a4abfd6b..4ec6927c86 100644 --- a/doc/guides/rel_notes/deprecation.rst +++ b/doc/guides/rel_notes/deprecation.rst @@ -128,12 +128,6 @@ Deprecation Notices is deprecated and will be removed in DPDK 21.11. Shared counters should be managed using shared actions API (``rte_flow_shared_action_create`` etc). -* ethdev: Definition of the flow API action ``RTE_FLOW_ACTION_TYPE_PORT_ID`` - is ambiguous and needs clarification. - Structure ``rte_flow_action_port_id`` will be extended to specify - traffic direction to the represented entity or ethdev port itself - in DPDK 21.11. - * ethdev: Flow API documentation is unclear if ethdev port used to create a flow rule adds any implicit match criteria in the case of transfer rules. The semantics will be clarified in DPDK 21.11 and it will require fixes in diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst index d707a554ef..b7aa175a32 100644 --- a/doc/guides/rel_notes/release_21_11.rst +++ b/doc/guides/rel_notes/release_21_11.rst @@ -84,6 +84,10 @@ API Changes Also, make sure to start the actual text at the margin. ======================================================= +* ethdev: ``struct rte_flow_action_port_id`` is extended with the direction + field with unspecified default, so all ``PORT_ID`` flow API action users + must be updated to make correct choice. + ABI Changes ----------- diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index 70f455d47d..3b83ed7d3a 100644 --- a/lib/ethdev/rte_flow.h +++ b/lib/ethdev/rte_flow.h @@ -2632,10 +2632,32 @@ struct rte_flow_action_phy_port { uint32_t index; /**< Physical port index. */ }; +/** Traffic direction from application point of view. */ +enum rte_flow_direction { + /** + * Invalid value which should not be used and must be + * rejected by drivers. + */ + RTE_FLOW_DIRECTION_UNSPECIFIED = 0, + /** As if the traffic was sent by the application. */ + RTE_FLOW_EGRESS, + /** To be received by the application. */ + RTE_FLOW_INGRESS, +}; + /** * RTE_FLOW_ACTION_TYPE_PORT_ID * - * Directs matching traffic to a given DPDK port ID. + * Directs matching traffic to an ethdev with the given DPDK port ID or + * to the upstream port (the peer side of the wire) corresponding to it. + * + * It's assumed that it's the PMD (typically, its instance at the admin + * PF) which controls the binding between a (representor) ethdev and an + * upstream port. Typical bindings: VF rep. <=> VF, PF <=> network port. + * If the PMD instance is unaware of the binding between the ethdev and + * its upstream port (or can't control it), it should reject the action + * with the egress direction specified and log an appropriate error + * message. * * @see RTE_FLOW_ITEM_TYPE_PORT_ID */ @@ -2643,6 +2665,7 @@ struct rte_flow_action_port_id { uint32_t original:1; /**< Use original DPDK port ID if possible. */ uint32_t reserved:31; /**< Reserved, must be zero. */ uint32_t id; /**< DPDK port ID. */ + enum rte_flow_direction dir; /**< Direction to route traffic to. */ }; /**