Message ID | 1597412329-3133-1-git-send-email-ian.stokes@intel.com |
---|---|
State | Accepted |
Delegated to: | Ilya Maximets |
Headers | show |
Series | [ovs-dev,v3,1/1] netdev-offload-dpdk: Fix for broken ethernet matching HWOL for XL710NIC. | expand |
On 8/14/20 3:38 PM, Ian Stokes wrote: > From: Emma Finn <emma.finn@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@intel.com> > Signed-off-by: Eli Britstein <elibr@nvidia.com> > Co-authored-by: Eli Britstein <elibr@nvidia.com> > Tested-by: Ian Stokes <ian.stokes@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? > 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); >
>-----Original Message----- >From: Ilya Maximets <i.maximets@ovn.org> >Sent: Monday, August 17, 2020 12:59 PM >To: Ian Stokes <ian.stokes@intel.com>; dev@openvswitch.org >Cc: Eli Britstein <elibr@nvidia.com>; i.maximets@ovn.org; >emma.finn@intel.com; i.maximets@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@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@intel.com> >> Signed-off-by: Eli Britstein <elibr@nvidia.com> >> Co-authored-by: Eli Britstein <elibr@nvidia.com> >> Tested-by: Ian Stokes <ian.stokes@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. > > >> 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); >>
On 8/17/20 12:39 PM, Eli Britstein wrote: > > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Monday, August 17, 2020 12:59 PM >> To: Ian Stokes <ian.stokes@intel.com>; dev@openvswitch.org >> Cc: Eli Britstein <elibr@nvidia.com>; i.maximets@ovn.org; >> emma.finn@intel.com; i.maximets@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@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@intel.com> >>> Signed-off-by: Eli Britstein <elibr@nvidia.com> >>> Co-authored-by: Eli Britstein <elibr@nvidia.com> >>> Tested-by: Ian Stokes <ian.stokes@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? 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); >>> >
> >> -----Original Message----- > >> From: Ilya Maximets <i.maximets@ovn.org> > >> Sent: Monday, August 17, 2020 12:59 PM > >> To: Ian Stokes <ian.stokes@intel.com>; dev@openvswitch.org > >> Cc: Eli Britstein <elibr@nvidia.com>; i.maximets@ovn.org; > >> emma.finn@intel.com; i.maximets@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@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@intel.com> > >>> Signed-off-by: Eli Britstein <elibr@nvidia.com> > >>> Co-authored-by: Eli Britstein <elibr@nvidia.com> > >>> Tested-by: Ian Stokes <ian.stokes@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. @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); > >>> > >
On 8/17/20 1:30 PM, Stokes, Ian wrote: >>>> -----Original Message----- >>>> From: Ilya Maximets <i.maximets@ovn.org> >>>> Sent: Monday, August 17, 2020 12:59 PM >>>> To: Ian Stokes <ian.stokes@intel.com>; dev@openvswitch.org >>>> Cc: Eli Britstein <elibr@nvidia.com>; i.maximets@ovn.org; >>>> emma.finn@intel.com; i.maximets@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@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@intel.com> >>>>> Signed-off-by: Eli Britstein <elibr@nvidia.com> >>>>> Co-authored-by: Eli Britstein <elibr@nvidia.com> >>>>> Tested-by: Ian Stokes <ian.stokes@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. > > @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); >>>>> >>> >
>-----Original Message----- >From: Ilya Maximets <i.maximets@ovn.org> >Sent: Monday, August 17, 2020 2:41 PM >To: Stokes, Ian <ian.stokes@intel.com>; Ilya Maximets <i.maximets@ovn.org>; >Eli Britstein <elibr@nvidia.com>; dev@openvswitch.org; Finn, Emma ><emma.finn@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@ovn.org> >>>>> Sent: Monday, August 17, 2020 12:59 PM >>>>> To: Ian Stokes <ian.stokes@intel.com>; dev@openvswitch.org >>>>> Cc: Eli Britstein <elibr@nvidia.com>; i.maximets@ovn.org; >>>>> emma.finn@intel.com; i.maximets@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@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@intel.com> >>>>>> Signed-off-by: Eli Britstein <elibr@nvidia.com> >>>>>> Co-authored-by: Eli Britstein <elibr@nvidia.com> >>>>>> Tested-by: Ian Stokes <ian.stokes@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. > >> >> @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); >>>>>> >>>> >>
> -----Original Message----- > From: Eli Britstein <elibr@nvidia.com> > Sent: Monday 17 August 2020 12:50 > To: Ilya Maximets <i.maximets@ovn.org>; Stokes, Ian > <ian.stokes@intel.com>; dev@openvswitch.org; Finn, Emma > <emma.finn@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@ovn.org> > >Sent: Monday, August 17, 2020 2:41 PM > >To: Stokes, Ian <ian.stokes@intel.com>; Ilya Maximets > ><i.maximets@ovn.org>; Eli Britstein <elibr@nvidia.com>; > >dev@openvswitch.org; Finn, Emma <emma.finn@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@ovn.org> > >>>>> Sent: Monday, August 17, 2020 12:59 PM > >>>>> To: Ian Stokes <ian.stokes@intel.com>; dev@openvswitch.org > >>>>> Cc: Eli Britstein <elibr@nvidia.com>; i.maximets@ovn.org; > >>>>> emma.finn@intel.com; i.maximets@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@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@intel.com> > >>>>>> Signed-off-by: Eli Britstein <elibr@nvidia.com> > >>>>>> Co-authored-by: Eli Britstein <elibr@nvidia.com> > >>>>>> Tested-by: Ian Stokes <ian.stokes@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); > >>>>>> > >>>> > >>
> -----Original Message----- > From: Finn, Emma > Sent: Monday 17 August 2020 13:50 > To: Eli Britstein <elibr@nvidia.com>; Ilya Maximets <i.maximets@ovn.org>; > Stokes, Ian <ian.stokes@intel.com>; dev@openvswitch.org > Subject: RE: [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken ethernet > matching HWOL for XL710NIC. > > > > > -----Original Message----- > > From: Eli Britstein <elibr@nvidia.com> > > Sent: Monday 17 August 2020 12:50 > > To: Ilya Maximets <i.maximets@ovn.org>; Stokes, Ian > > <ian.stokes@intel.com>; dev@openvswitch.org; Finn, Emma > > <emma.finn@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@ovn.org> > > >Sent: Monday, August 17, 2020 2:41 PM > > >To: Stokes, Ian <ian.stokes@intel.com>; Ilya Maximets > > ><i.maximets@ovn.org>; Eli Britstein <elibr@nvidia.com>; > > >dev@openvswitch.org; Finn, Emma <emma.finn@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@ovn.org> > > >>>>> Sent: Monday, August 17, 2020 12:59 PM > > >>>>> To: Ian Stokes <ian.stokes@intel.com>; dev@openvswitch.org > > >>>>> Cc: Eli Britstein <elibr@nvidia.com>; i.maximets@ovn.org; > > >>>>> emma.finn@intel.com; i.maximets@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@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@intel.com> > > >>>>>> Signed-off-by: Eli Britstein <elibr@nvidia.com> > > >>>>>> Co-authored-by: Eli Britstein <elibr@nvidia.com> > > >>>>>> Tested-by: Ian Stokes <ian.stokes@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 > 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); > > >>>>>> > > >>>> > > >>
On 8/17/20 4:49 PM, Finn, Emma wrote: > > >> -----Original Message----- >> From: Finn, Emma >> Sent: Monday 17 August 2020 13:50 >> To: Eli Britstein <elibr@nvidia.com>; Ilya Maximets <i.maximets@ovn.org>; >> Stokes, Ian <ian.stokes@intel.com>; dev@openvswitch.org >> Subject: RE: [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken ethernet >> matching HWOL for XL710NIC. >> >> >> >>> -----Original Message----- >>> From: Eli Britstein <elibr@nvidia.com> >>> Sent: Monday 17 August 2020 12:50 >>> To: Ilya Maximets <i.maximets@ovn.org>; Stokes, Ian >>> <ian.stokes@intel.com>; dev@openvswitch.org; Finn, Emma >>> <emma.finn@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@ovn.org> >>>> Sent: Monday, August 17, 2020 2:41 PM >>>> To: Stokes, Ian <ian.stokes@intel.com>; Ilya Maximets >>>> <i.maximets@ovn.org>; Eli Britstein <elibr@nvidia.com>; >>>> dev@openvswitch.org; Finn, Emma <emma.finn@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@ovn.org> >>>>>>>> Sent: Monday, August 17, 2020 12:59 PM >>>>>>>> To: Ian Stokes <ian.stokes@intel.com>; dev@openvswitch.org >>>>>>>> Cc: Eli Britstein <elibr@nvidia.com>; i.maximets@ovn.org; >>>>>>>> emma.finn@intel.com; i.maximets@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@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@intel.com> >>>>>>>>> Signed-off-by: Eli Britstein <elibr@nvidia.com> >>>>>>>>> Co-authored-by: Eli Britstein <elibr@nvidia.com> >>>>>>>>> Tested-by: Ian Stokes <ian.stokes@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 >> > 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 Thanks! I'll apply this version as soon as travis builds finished. > >>>> >>>>> >>>>> @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); >>>>>>>>> >>>>>>> >>>>> >
On 8/17/20 6:38 PM, Ilya Maximets wrote: >>>>>>> >>>>>>> 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 > > Thanks! > I'll apply this version as soon as travis builds finished. Applied and backported down to 2.10. Best regards, Ilya Maximets.
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);