On Wed, Jan 11, 2023 at 9:16 AM Eli Britstein <[email protected]> wrote: > > > > >-----Original Message----- > >From: David Marchand <[email protected]> > >Sent: Wednesday, 11 January 2023 9:59 > >To: Ivan Malov <[email protected]> > >Cc: Ilya Maximets <[email protected]>; Eli Britstein <[email protected]>; > >[email protected]; Stephen Hemminger > ><[email protected]>; Ori Kam <[email protected]>; Maxime > >Coquelin <[email protected]>; Ian Stokes > ><[email protected]>; Andrew Rybchenko > ><[email protected]> > >Subject: Re: [PATCH v3 0/3] Rework the usage of DPDK transfer flow offloads > > > >External email: Use caution opening links or attachments > > > > > >Hi Ivan, > > > >On Fri, Jul 22, 2022 at 12:02 PM Ivan Malov <[email protected]> wrote: > >> > >> Hi Ilya, Eli, > >> > >> Many thanks for your feedback. Please see below. > >> > >> On Wed, 20 Jul 2022, Ilya Maximets wrote: > >> > >> > On 7/20/22 14:18, Ivan Malov wrote: > >> >> DPDK has got support for offloads involving assignment of > >> >> per-packet Rx metadata (flag, mark, tunnel ID and the likes). > >> >> However, delivery of such metadata from the NIC to the PMD might > >> >> need to be negotiated in advance. API [1] addresses that problem. Make > >OvS invoke this API. > >> >> > >> >> Another problem is how flow rules with attribute "transfer" refer > >> >> to embedded switch ports by means of their representors. Action > >> >> PORT_ID has proved ambiguous: it refers to a DPDK ethdev, but in > >> >> fact steers packets to the entity represented by the ethdev, in example, > >to a VF. > >> >> The problem is addressed by [2]. Use the solution in OvS accordingly. > >> >> > >> >> In addition, [2] and [3] address filtering traffic by input ports > >> >> of the embedded switch. In the suggested approach, "transfer" rules > >> >> are managed via the only ethdev with sufficient privileges, whilst > >> >> match criteria include an explicit item to indicate the desired input > >> >> port. > >> >> Revisit OvS support for "transfer" rules to follow the said approach. > >> >> > >> >> The following tests have been considered so far: > >> >> - build check with the current dpdk-next-net; > >> >> - running "make check" for every patch; > >> >> - tunnel offload demo with net/sfc PMD. > >> >> > >> >> [1] > >> >> > >https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fma > >> >> ils.dpdk.org%2Farchives%2Fdev%2F2021- > >October%2F224291.html&data=05% > >> >> > >7C01%7Celibr%40nvidia.com%7C9f047907e3d54640a11f08daf3a9b52a%7C4308 > >> >> > >3d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638090207473941055%7CUnkn > >own > >> >> > >%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW > >wi > >> >> > >LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=OwaqM%2BGfklo73Lb%2BJIp > >5vcH0DH > >> >> F%2FL6PZCbrvEgupwWc%3D&reserved=0 [2] > >> >> > >https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fma > >> >> ils.dpdk.org%2Farchives%2Fdev%2F2021- > >October%2F224620.html&data=05% > >> >> > >7C01%7Celibr%40nvidia.com%7C9f047907e3d54640a11f08daf3a9b52a%7C4308 > >> >> > >3d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638090207473941055%7CUnkn > >own > >> >> > >%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW > >wi > >> >> > >LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=WF%2FsuI3pSCjMcHetlKbJze5 > >xFon8 > >> >> cnj%2F4A4YCd9QwWg%3D&reserved=0 [3] > >> >> > >https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fma > >> >> ils.dpdk.org%2Farchives%2Fdev%2F2021- > >October%2F225081.html&data=05% > >> >> > >7C01%7Celibr%40nvidia.com%7C9f047907e3d54640a11f08daf3a9b52a%7C4308 > >> >> > >3d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638090207473941055%7CUnkn > >own > >> >> > >%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW > >wi > >> >> > >LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=AP07kpKyl3DPp01Rui7G7AIq > >%2B4SH > >> >> YrnlOAU30lNP%2BXg%3D&reserved=0 > >> >> --- > >> >> v1 -> v2: amendments to care about proxy detach and port ID > >> >> logging > >> >> v2 -> v3: a minor adjustment in the cover letter's subject line > >> >> > >> >> Ivan Malov (3): > >> >> netdev-dpdk: negotiate delivery of per-packet Rx metadata > >> >> netdev-offload-dpdk: replace action PORT_ID with REPRESENTED_PORT > >> >> netdev-offload-dpdk: use flow transfer proxy mechanism > >> >> > >> >> lib/netdev-dpdk.c | 141 +++++++++++++++++++++++++++++++++- > >---- > >> >> lib/netdev-dpdk.h | 4 +- > >> >> lib/netdev-offload-dpdk.c | 91 +++++++++++++++++++++--- > >> >> 3 files changed, 209 insertions(+), 27 deletions(-) > >> >> > >> > > >> > Hi. Since this patch set contains use of DPDK's experimental API, > >> > it needs to be based on dpdk-latest branch and have 'dpdk-latest' > >> > in the subject prefix. OVS doesn't normally use experimental API. > >> > The tunnel offload support was an exception, because it required > >> > significant re-work of the generic code that wasn't feasible to > >> > carry in the dpdk-latest branch. > >> > >> Very useful information indeed. > >> > >> > > >> > CC: Ian, dpdk-latest branch might need some rebase. > >> > > >> > Also, please, don't send new versions in reply to previous one. > >> > It's only required for dpdk-dev list for some reason, but prohibited > >> > or frown upon in every other list that I know of. It's very > >> > inconvenient to work this way. > >> > >> Thanks for the hint. I should've known. > >> > >> > > >> > For the patch set itself, a brief look over current state of DPDK > >> > shows that not all drivers that support PORT_ID support the > >> > REPRESENTED_PORT action as well. I see at least dpaa2 and cnxk that > >> > doesn't. So, this patch set essentially breaks offloading for these > >> > NICs, IIUC. Yet another rte_flow inconsistency. > >> > Any plans for addressing that? > >> > > >> > Best regards, Ilya Maximets. > >> > > >> > >> This observation of yours is correct. The REPRESENTED_PORT commit is: > >> > >> commit 88caad251c8de3a84e353b0b2a27014bc303df87 > >> Author: Ivan Malov <[email protected]> > >> Date: Wed Oct 13 20:34:40 2021 +0300 > >> > >> ethdev: add represented port action to flow API > >> > >> While the PORT_ID commits in dpaa2 and cnxk, respectively, are: > >> > >> commit 028d1dfd183043db03eb3d726579b18219c02624 > >> Author: Jun Yang <[email protected]> > >> Date: Wed Oct 6 22:31:23 2021 +0530 > >> > >> net/dpaa2: support Tx flow redirection action > >> > >> commit 00ea15e7a39617903ad39a9b89280a028ec7783a > >> Author: Satheesh Paul <[email protected]> > >> Date: Thu Oct 21 10:41:15 2021 +0530 > >> > >> net/cnxk: support port ID flow action > >> > >> Indeed, I might've lost at least one driver from my view when sending > >> the REPRESENTED_PORT series updating the capable PMDs. > >> And in the case of cnxk, perhaps maintainers were busy enough and thus > >> did not spot the PORT_ID deprecation being under way. > >> > >> So yes, the problem exists, and many thanks for noticing that. > >> > >> Looking into it, I see that we've had a deprecation notice in DPDK for > >> almost a year already: > >> > >> * ethdev: Items and actions ``PF``, ``VF``, ``PHY_PORT``, ``PORT_ID`` are > >> deprecated as hard-to-use / ambiguous and will be removed in DPDK > >22.11. > >> > >> * ethdev: The use of attributes ``ingress`` / ``egress`` in "transfer" > >> flows > >> is deprecated as ambiguous with respect to the embedded switch. The use > >of > >> these attributes will become invalid starting from DPDK 22.11. > >> > >> So, based on that, I devise the following plan: > >> > >> 1) I will send a patch to make the new items / actions non-experimental. > >> 2) I will send two patches to add REPRESENTED_PORT to dpaa2 and cnxk. > >> 3) I will dispose of the deprecated items / actions and the notice. > >> Doing so will also remove "experimental" from transfer proxy > >> as well as from Rx metadata configuration API. > >> 4) When (1-3) have been done, I will re-send the OvS patch series, but > >> this time without ALLOW_EXPERIMENTAL guards. The idea is that OvS, > >> in its next release, should pick DPDK 22.11 stable release, that > >> is, the use of Rx metadata, flow proxy and new items / actions > >> should not be experimental by that time any longer. > >> > >> However, if the patch sets (like this one) can be applied earlier in > >> OvS, then it might be handy to preserve ALLOW_EXPERIMENTAL tags. Then > >> they can be removed later on, when OvS is switched to DPDK 22.11. > >> But don't have a strong opinion on that. > > > >- You can rebsae this series now, since experimental tag checks can be > >removed. > > > >- I was doing a quick check on dpdk offload in OVS. > >Unless I missed something, full offloading is broken because of the use of > >mixed ingress and transfer attributes. > >Unfortunately, the issue on ingress attribute is still present with this > >series > >applied. > >Do you have a fix for this too? > @@ -2242,7 +2242,7 @@ > struct nlattr *nl_actions, > size_t actions_len) > { > - const struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1 }; > + const struct rte_flow_attr flow_attr = { .transfer = 1 }; > struct flow_actions actions = { > .actions = NULL, > .cnt = 0,
Well, that's what I did as a hack, and it seemed enough. Should this simple fix go as is, do we need Ivan series as well? -- David Marchand _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
