[ovs-dev] [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken ethernet matching HWOL for XL710NIC.
Finn, Emma
emma.finn at intel.com
Mon Aug 17 12:50:00 UTC 2020
> -----Original Message-----
> From: Eli Britstein <elibr at nvidia.com>
> Sent: Monday 17 August 2020 12:50
> To: Ilya Maximets <i.maximets at ovn.org>; Stokes, Ian
> <ian.stokes at intel.com>; dev at openvswitch.org; Finn, Emma
> <emma.finn at intel.com>
> Subject: RE: [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken ethernet
> matching HWOL for XL710NIC.
>
>
>
> >-----Original Message-----
> >From: Ilya Maximets <i.maximets at ovn.org>
> >Sent: Monday, August 17, 2020 2:41 PM
> >To: Stokes, Ian <ian.stokes at intel.com>; Ilya Maximets
> ><i.maximets at ovn.org>; Eli Britstein <elibr at nvidia.com>;
> >dev at openvswitch.org; Finn, Emma <emma.finn at intel.com>
> >Subject: Re: [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken
> >ethernet matching HWOL for XL710NIC.
> >
> >
> >On 8/17/20 1:30 PM, Stokes, Ian wrote:
> >>>>> -----Original Message-----
> >>>>> From: Ilya Maximets <i.maximets at ovn.org>
> >>>>> Sent: Monday, August 17, 2020 12:59 PM
> >>>>> To: Ian Stokes <ian.stokes at intel.com>; dev at openvswitch.org
> >>>>> Cc: Eli Britstein <elibr at nvidia.com>; i.maximets at ovn.org;
> >>>>> emma.finn at intel.com; i.maximets at ovn.org
> >>>>> Subject: Re: [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken
> >>>>> ethernet matching HWOL for XL710NIC.
> >>>>>
> >>>>>
> >>>>> On 8/14/20 3:38 PM, Ian Stokes wrote:
> >>>>>> From: Emma Finn <emma.finn at intel.com>
> >>>>>>
> >>>>>> This patch introduces a temporary work around to fix partial
> >>>>>> hardware offload for XL710 devices. Currently the incorrect
> >>>>>> ethernet pattern is being set. This patch will be removed once
> >>>>>> this issue is fixed within the i40e PMD.
> >>>>>>
> >>>>>> Signed-off-by: Emma Finn <emma.finn at intel.com>
> >>>>>> Signed-off-by: Eli Britstein <elibr at nvidia.com>
> >>>>>> Co-authored-by: Eli Britstein <elibr at nvidia.com>
> >>>>>> Tested-by: Ian Stokes <ian.stokes at intel.com>
> >>>>>> ---
> >>>>>
> >>>>> Thanks, Ian and Eli.
> >>>>> This patch looks good to me, but I had some issues backporting it
> >>>>> to
> >>>>> 2.12 and below. We have removed the XL710 workaround from these
> >>>>> old branches too, so I'm assuming that we need this patch there.
> >>>>>
> >>>>> On older branches memory for patterns is statically allocated, so
> >>>>> it's not easy to NULL-ify pointers.
> >>>>>
> >>>>> Following version of the code applies to 2.12 and could be easily
> >>>>> backported down to 2.10:
> >>>>>
> >>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> >>>>> index 7146b2aab..b824218af 100644
> >>>>> --- a/lib/netdev-offload-dpdk.c
> >>>>> +++ b/lib/netdev-offload-dpdk.c
> >>>>> @@ -446,9 +446,18 @@ netdev_offload_dpdk_add_flow(struct
> netdev
> >>>>> *netdev,
> >>>>> memset(&mask, 0, sizeof mask);
> >>>>>
> >>>>> /* Eth */
> >>>>> - if (match->wc.masks.dl_type ||
> >>>>> - !eth_addr_is_zero(match->wc.masks.dl_src) ||
> >>>>> - !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> >>>>> + if (match->wc.masks.dl_type == OVS_BE16_MAX &&
> >>>>> + is_ip_any(&match-
> >>>>>> flow)
> >>>>> + && eth_addr_is_zero(match->wc.masks.dl_dst)
> >>>>> + && eth_addr_is_zero(match->wc.masks.dl_src)) {
> >>>>> + /*
> >>>>> + * This is a temporary work around to fix ethernet pattern for
> partial
> >>>>> + * hardware offload for X710 devices. This fix will be reverted
> once
> >>>>> + * the issue is fixed within the i40e PMD driver.
> >>>>> + */
> >>>>> + add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> NULL,
> >>> NULL);
> >>>>> + } else if (match->wc.masks.dl_type ||
> >>>>> + !eth_addr_is_zero(match->wc.masks.dl_src) ||
> >>>>> + !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> >>>>> memcpy(&spec.eth.dst, &match->flow.dl_dst, sizeof
> spec.eth.dst);
> >>>>> memcpy(&spec.eth.src, &match->flow.dl_src, sizeof
> spec.eth.src);
> >>>>> spec.eth.type = match->flow.dl_type;
> >>>>> ---
> >>>>>
> >>>>> If it looks good to you, I could apply original patch (below) to
> >>>>> master,
> >>>>> 2.14 and 2.13, and a slightly different version (above) to 2.12,
> >>>>> 2.11 and 2.10 once travis builds done. What do you think?
> >>>> If intel can also do with zero masks (eth type spec 0x0800 type
> >>>> mask
> >>>> 0 /
> >>> ipv4...), we don't have to NULL-ify but only zero the mask, but
> >>> that's already a logic change than what we know working.
> >>>> Anyway, the above LGTM.
> >>>> Your plan is good, but there is also a slightly different approach.
> >>>> We can adapt
> >>> the above and use it (or similar) for the 2.13+ branches, instead of
> >>> the below, to keep consistent and easier backporting in future.
> >>>
> >>>
> >>> Yes, I thought about this. We will need to duplicate 'consumed'
> >>> part on new branches, but that might be not a big deal. The code
> >>> for new branches will look like this:
> >>>
> >>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> >>> index
> >>> de6101e4d..5b632bac4 100644
> >>> --- a/lib/netdev-offload-dpdk.c
> >>> +++ b/lib/netdev-offload-dpdk.c
> >>> @@ -691,9 +691,22 @@ parse_flow_match(struct flow_patterns
> *patterns,
> >>> consumed_masks->packet_type = 0;
> >>>
> >>> /* Eth */
> >>> - if (match->wc.masks.dl_type ||
> >>> - !eth_addr_is_zero(match->wc.masks.dl_src) ||
> >>> - !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> >>> + if (match->wc.masks.dl_type == OVS_BE16_MAX &&
> >>> + is_ip_any(&match-
> >>>> flow)
> >>> + && eth_addr_is_zero(match->wc.masks.dl_dst)
> >>> + && eth_addr_is_zero(match->wc.masks.dl_src)) {
> >>> + /*
> >>> + * This is a temporary work around to fix ethernet pattern for
> partial
> >>> + * hardware offload for X710 devices. This fix will be reverted once
> >>> + * the issue is fixed within the i40e PMD driver.
> >>> + */
> >>> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL,
> >>> + NULL);
> >>> +
> >>> + memset(&consumed_masks->dl_dst, 0, sizeof consumed_masks-
> >>dl_dst);
> >>> + memset(&consumed_masks->dl_src, 0, sizeof consumed_masks-
> >>dl_src);
> >>> + consumed_masks->dl_type = 0;
> >>> + } else if (match->wc.masks.dl_type ||
> >>> + !eth_addr_is_zero(match->wc.masks.dl_src) ||
> >>> + !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> >>> struct rte_flow_item_eth *spec, *mask;
> >>>
> >>> spec = xzalloc(sizeof *spec);
> >>> ---
> >>>
> >>> If looks good for everyone, I could use above code for the patch on
> >>> new branches and the version without 'consumed_*' lines for older
> ones.
> >>>
> >>> Ian, Eli, what do you think?
> >>
> >> I think this approach sounds ok, Emma is back in office and so can
> >> help validate
> >the patches if there are queries around supporting zeroing on the X710
> Nics.
> >
> >I don't think we should change the logic of a workaround itself at this point.
> >But it will be good to have above diff validated. To be sure that I
> >didn't break anything accidentially.
> Either way looks OK to me.
Yes I can test
> >
> >>
> >> @Emma can you validate what Ilya has proposed?
> >>
> >> Regards
> >> Ian
> >>>
> >>> Do we need v4 for this?
> >>>
> >>>>>
> >>>>>
> >>>>>> lib/netdev-offload-dpdk.c | 28 ++++++++++++++++++++--------
> >>>>>> 1 file changed, 20 insertions(+), 8 deletions(-)
> >>>>>>
> >>>>>> diff --git a/lib/netdev-offload-dpdk.c
> >>>>>> b/lib/netdev-offload-dpdk.c index de6101e4d..2d668275a 100644
> >>>>>> --- a/lib/netdev-offload-dpdk.c
> >>>>>> +++ b/lib/netdev-offload-dpdk.c
> >>>>>> @@ -696,16 +696,28 @@ parse_flow_match(struct flow_patterns
> >>> *patterns,
> >>>>>> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> >>>>>> struct rte_flow_item_eth *spec, *mask;
> >>>>>>
> >>>>>> - spec = xzalloc(sizeof *spec);
> >>>>>> - mask = xzalloc(sizeof *mask);
> >>>>>> + /*
> >>>>>> + * This is a temporary work around to fix ethernet pattern for
> partial
> >>>>>> + * hardware offload for X710 devices. This fix will be reverted
> once
> >>>>>> + * the issue is fixed within the i40e PMD driver.
> >>>>>> + */
> >>>>>> + if (match->wc.masks.dl_type == OVS_BE16_MAX &&
> >>>>>> + is_ip_any(&match-
> >>>>>> flow)
> >>>>>> + && eth_addr_is_zero(match->wc.masks.dl_dst)
> >>>>>> + && eth_addr_is_zero(match->wc.masks.dl_src)) {
> >>>>>> + spec = NULL;
> >>>>>> + mask = NULL;
> >>>>>> + } else {
> >>>>>> + spec = xzalloc(sizeof *spec);
> >>>>>> + mask = xzalloc(sizeof *mask);
> >>>>>>
> >>>>>> - memcpy(&spec->dst, &match->flow.dl_dst, sizeof spec->dst);
> >>>>>> - memcpy(&spec->src, &match->flow.dl_src, sizeof spec->src);
> >>>>>> - spec->type = match->flow.dl_type;
> >>>>>> + memcpy(&spec->dst, &match->flow.dl_dst, sizeof spec-
> >dst);
> >>>>>> + memcpy(&spec->src, &match->flow.dl_src, sizeof spec->src);
> >>>>>> + spec->type = match->flow.dl_type;
> >>>>>>
> >>>>>> - memcpy(&mask->dst, &match->wc.masks.dl_dst, sizeof mask-
> >dst);
> >>>>>> - memcpy(&mask->src, &match->wc.masks.dl_src, sizeof mask-
> >src);
> >>>>>> - mask->type = match->wc.masks.dl_type;
> >>>>>> + memcpy(&mask->dst, &match->wc.masks.dl_dst, sizeof
> >>>>>> + mask-
> >>dst);
> >>>>>> + memcpy(&mask->src, &match->wc.masks.dl_src, sizeof
> >>>>>> + mask-
> >>src);
> >>>>>> + mask->type = match->wc.masks.dl_type;
> >>>>>> + }
> >>>>>>
> >>>>>> memset(&consumed_masks->dl_dst, 0, sizeof
> >>>>>> consumed_masks-
> >>>> dl_dst);
> >>>>>> memset(&consumed_masks->dl_src, 0, sizeof
> >>>>>> consumed_masks->dl_src);
> >>>>>>
> >>>>
> >>
More information about the dev
mailing list