> -----Original Message-----
> From: Finn, Emma
> Sent: Monday 17 August 2020 13:50
> To: Eli Britstein <[email protected]>; Ilya Maximets <[email protected]>;
> Stokes, Ian <[email protected]>; [email protected]
> Subject: RE: [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken ethernet
> matching HWOL for XL710NIC.
>
>
>
> > -----Original Message-----
> > From: Eli Britstein <[email protected]>
> > Sent: Monday 17 August 2020 12:50
> > To: Ilya Maximets <[email protected]>; Stokes, Ian
> > <[email protected]>; [email protected]; Finn, Emma
> > <[email protected]>
> > Subject: RE: [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken
> > ethernet matching HWOL for XL710NIC.
> >
> >
> >
> > >-----Original Message-----
> > >From: Ilya Maximets <[email protected]>
> > >Sent: Monday, August 17, 2020 2:41 PM
> > >To: Stokes, Ian <[email protected]>; Ilya Maximets
> > ><[email protected]>; Eli Britstein <[email protected]>;
> > >[email protected]; Finn, Emma <[email protected]>
> > >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 <[email protected]>
> > >>>>> Sent: Monday, August 17, 2020 12:59 PM
> > >>>>> To: Ian Stokes <[email protected]>; [email protected]
> > >>>>> Cc: Eli Britstein <[email protected]>; [email protected];
> > >>>>> [email protected]; [email protected]
> > >>>>> 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 <[email protected]>
> > >>>>>>
> > >>>>>> 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 <[email protected]>
> > >>>>>> Signed-off-by: Eli Britstein <[email protected]>
> > >>>>>> Co-authored-by: Eli Britstein <[email protected]>
> > >>>>>> Tested-by: Ian Stokes <[email protected]>
> > >>>>>> ---
> > >>>>>
> > >>>>> 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
>
Tested and everything looks good.
2020-08-17T14:46:41.179Z|00006|netdev_offload_dpdk(dp_netdev_flow_48)|DBG|dpdk0:
installed flow 0x23fd71ac0 by ufid c3ba1a2f-e7e5-473b-b303-38ecbd28cc03
> > >
> > >>
> > >> @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);
> > >>>>>>
> > >>>>
> > >>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
