Hi Andrew, > -----Original Message----- > From: Andrew Rybchenko <andrew.rybche...@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.rybche...@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.rybche...@oktetlabs.ru> > >>>> Sent: Sunday, August 1, 2021 3:04 PM > >>>> To: Eli Britstein <el...@nvidia.com>; NBU-Contact-Thomas Monjalon > >>>> <tho...@monjalon.net>; Ferruh Yigit <ferruh.yi...@intel.com>; Ori > >>>> Kam <or...@nvidia.com> > >>>> Cc: dev@dpdk.org; Ilya Maximets <i.maxim...@ovn.org>; Ajit > Khaparde > >>>> <ajit.khapa...@broadcom.com>; Matan Azrad <ma...@nvidia.com>; > >> Ivan > >>>> Malov <ivan.ma...@oktetlabs.ru>; Viacheslav Galaktionov > >>>> <viacheslav.galaktio...@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.ma...@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.rybche...@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 > >>>>>> > >>> > >
