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

[Ori Kam <orika@nvidia.com>]

Hi  Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Sunday, August 1, 2021 4:24 PM
> Subject: Re: [PATCH 1/2] ethdev: announce flow API action PORT_ID changes
> 
> On 8/1/21 3:56 PM, Ori Kam wrote:
> > Hi Andrew,
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Sunday, August 1, 2021 3:44 PM
> >> Subject: Re: [PATCH 1/2] ethdev: announce flow API action PORT_ID
> >> changes
> >>
> >> Hi Ori,
> >>
> >> On 8/1/21 3:23 PM, Ori Kam wrote:
> >>> Hi Andrew,
> >>>
> >>> I think before we can change the API we must agree on the meaning of
> >> representor.
> >>
> >> The question is not directly related to a representor definition.
> >> Just indirectly. PORT_ID action makes sense for non-representor ports
> >> as well.
> >>
> >>> 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.
> >>
> >> Hopefully you agree that the area requires clarification and must be
> >> improved. I think so hot discussions really prove it.
> >>
> > +1
> >
> >>> I think we should continue our discussion on what is a representor.
> >>
> >> Yes, but it is a hard topic. I'd like to unbind PORT_ID action from
> >> the discussion, since the action makes sense for non-representors as well.
> >>
> > If this can be done great, I'm for it, but I'm not sure it can be, but let's try.
> >
> >>> 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."
> >>
> >> IMHO it is better to keep the definition of the action simple and do
> >> not have any representor specifics in it. Representor is an ethdev
> >> port. If we direct traffic to an ethdev port, it should be received
> >> on the ethdev port regardless if it is a representor or not.
> >> It is better to avoid exceptions and special cases.
> >>
> >
> > Lets see if I understand correctly, you suggest that port  action /
> > item will be for DPDK port, unless they are marked with some bit which
> > means that the traffic should be routed to the switch port which the
> > DPDK port represent am I correct?
> 
> Here I'm talking about PORT_ID action only. As for details, I've tried to keep it
> out-of-scope of the deprecation notice.
> 
+1 but we need to check if we need it at all or just change doc.

> However, since we are going to break something here, it is better to break
> hard to be sure that every since usage is updated. So, I tend to to solution
> suggested by Ilya [1] which is similar to Linux kernel.
> I.e. add an enum with invalid zero value and two members to specify
> direction.
> 
> [1]
> https://patches.dpdk.org/project/dpdk/patch/20210601111420.5549-1-
> ivan.malov@oktetlabs.ru/#133431
> 
> as for PORT_ID pattern item, I think ingress/egress attributes define
> direction. If it is an ingress flow rule, PORT_ID item should match traffic
> coming from represented entity in the case of port representor and
> associated network port in the case of ethdev port associated with it. In
> egress case it otherwise matches traffic sent using Tx burst via corresponding
> ethdev port.
> 
I think that Ingress egress has only meaning when talking about NIC steering
and not E-Switch steering.
I think that we can just use original bit to mark if we want to send traffic
to DPDK port or to other port.

In any case I will be happy if we could have a meeting to discuss this
approach before sending your patch. 
I think this can save a lot of time.

Best,
Ori


> >>> 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.
> >>
> >> Other possible request is to direct traffic from phys port #0 to phys
> >> port #1 directly and say it in terms of PORT_ID action.
> >>
> > But we are talking using the switch layer(transfer mode) right?
> 
> Yes.
> 
> > Best,
> > Ori
> >> Thanks,
> >> Andrew.
> >>
> >>> 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
> >>>>>>
> >>>
> >