[dpdk-dev] [PATCH v1] ethdev: clarify flow action PORT ID semantics

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

From: Ivan Malov <ivan.malov@oktetlabs.ru>

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 <ivan.malov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
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. */
 };
 
 /**
-- 
2.30.2