>-----Original Message-----
>From: David Marchand <david.march...@redhat.com>
>Sent: Wednesday, 11 January 2023 9:59
>To: Ivan Malov <ivan.ma...@oktetlabs.ru>
>Cc: Ilya Maximets <i.maxim...@ovn.org>; Eli Britstein <el...@nvidia.com>;
>d...@openvswitch.org; Stephen Hemminger
><step...@networkplumber.org>; Ori Kam <or...@nvidia.com>; Maxime
>Coquelin <maxime.coque...@redhat.com>; Ian Stokes
><ian.sto...@intel.com>; Andrew Rybchenko
><andrew.rybche...@oktetlabs.ru>
>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 <ivan.ma...@oktetlabs.ru> 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 <ivan.ma...@oktetlabs.ru>
>> 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 <jun.y...@nxp.com>
>> Date: Wed Oct 6 22:31:23 2021 +0530
>>
>> net/dpaa2: support Tx flow redirection action
>>
>> commit 00ea15e7a39617903ad39a9b89280a028ec7783a
>> Author: Satheesh Paul <psathe...@marvell.com>
>> 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,
>
>Thanks!
>
>--
>David Marchand
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
