Re: [dpdk-dev] [PATCH 1/2] ethdev: announce flow API action PORT_ID changes

[Ori Kam <orika@nvidia.com>]

Hi Andrew,

I think before we can change the API we must agree on the meaning of representor.

PSB more comments

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Sunday, August 1, 2021 3:04 PM
> To: Eli Britstein <elibr@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Ori Kam
> <orika@nvidia.com>
> Cc: dev@dpdk.org; Ilya Maximets <i.maximets@ovn.org>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Matan Azrad <matan@nvidia.com>; Ivan
> Malov <ivan.malov@oktetlabs.ru>; Viacheslav Galaktionov
> <viacheslav.galaktionov@oktetlabs.ru>
> Subject: Re: [PATCH 1/2] ethdev: announce flow API action PORT_ID changes
> 
> On 8/1/21 1:57 PM, Eli Britstein wrote:
> >
> > On 8/1/2021 1:22 PM, Andrew Rybchenko wrote:
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> By its very name, action PORT_ID means that packets hit an ethdev
> >> with the given DPDK port ID. At least the current comments don't
> >> state the opposite.
> >> That said, since port representors had been adopted, applications
> >> like OvS have been misusing the action. They misread its purpose as
> >> sending packets to the opposite end of the "wire" plugged to the
> >> given ethdev, for example, redirecting packets to the VF itself
> >> rather than to its representor ethdev.
> >> Another example: OvS relies on this action with the admin PF's ethdev
> >> port ID specified in it in order to send offloaded packets to the
> >> physical port.
> >>
> >> Since there might be applications which use this action in its valid
> >> sense, one can't just change the documentation to greenlight the
> >> opposite meaning.
> >>
> >> The documentation must be clarified and rte_flow_action_port_id
> >> structure should be extended to support both meanings.
> >
> > I think the only clarification needed is that PORT_ID acts as if
> > rte_eth_tx_burst is called with the specified port-id.
> 
> Sorry, but I still think that it is opposite meaning to the current
> documentation which says "Directs matching traffic to a given DPDK port ID."
> Since it happens on switching level (transfer rule) "to a given DPDK port"
> means that it will be received on a given DPDK port.
> 
> Anyway, the goal of the deprecation notice is to highlight that it must be
> fixed and ensure that we can choose right decision even if it breaks API/ABI.
> 
Agree, it is good that you created the announcement.
I think we should continue our discussion on what is a representor.
I think for current implementation the doc should say "direct / matches
traffic to / from the switch port which the selected DPDK representor port
is connected to or to DPDK port if this port is not a representor."
If we go this way there is no need to change the API only the doc.

> > Regarding representors, it's not different. When using TX on a
> > representor port, the packets appear as RX on its represented port.
> >
> > Please elaborate if there is a use case for the PORT_ID~ in which the
> > app can get the packets using rte_eth_rx_burst on the specified port-id.
> 
> Multi-home host with a NIC with two physical ports and two PFs used by
> DPDK app with layer 3 (IP addresses). Different cores used to handle traffic
> from different ports plus routing in DPDK app. If traffic to port #0 IP address
> is received on phys port #1, it is useful to redirect traffic to port ID 0 directly
> to have these packets on correct CPU cores from the very beginning to avoid
> SW mechanisms to pass from port #1 CPU cores to port #0 CPU cores.
> 
To make sure I understand you are talking about a DPDK application that
is connected to number of ports and it is Eswitch manager, but it doesn't use
representors but the actual ports, right?
I think the definition I wrote above also works for this case.


Best,
Ori

> >>
> >> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> ---
> >>   doc/guides/rel_notes/deprecation.rst | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >>
> >> diff --git a/doc/guides/rel_notes/deprecation.rst
> >> b/doc/guides/rel_notes/deprecation.rst
> >> index d9c0e65921..6e6413c89f 100644
> >> --- a/doc/guides/rel_notes/deprecation.rst
> >> +++ b/doc/guides/rel_notes/deprecation.rst
> >> @@ -158,3 +158,8 @@ Deprecation Notices
> >>   * security: The functions ``rte_security_set_pkt_metadata`` and
> >>     ``rte_security_get_userdata`` will be made inline functions and
> >> additional
> >>     flags will be added in structure ``rte_security_ctx`` in DPDK 21.11.
> >> +
> >> +* ethdev: Definition of the flow API action PORT_ID is ambiguous and
> >> needs
> >> +  clarification. Structure rte_flow_action_port_id will be extended
> >> +to
> >> +  specify traffic direction to represented entity or ethdev port
> >> itself in
> >> +  DPDK 21.11.
> >> --
> >> 2.30.2
> >>