| Message ID | 1597335188-49025-1-git-send-email-emma.finn@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] netdev-offload-dpdk: Fix for broken ethernet matching HWOL for XL710 NIC | expand |
On 8/13/20 6:13 PM, Emma Finn wrote: > 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> > --- > lib/netdev-offload-dpdk.c | 35 +++++++++++++++++++++++++---------- > 1 file changed, 25 insertions(+), 10 deletions(-) > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > index de6101e..ede2077 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions *actions) > actions->cnt = 0; > } > > +/* > +* 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. > +*/ > +#define NULL_ETH_MASK_IF_ZEROS \ > + if (eth_mask && is_all_zeros(ð_mask->src, sizeof eth_mask->src) && \ > + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \ > + patterns->items[0].mask = NULL; \ > + free(eth_mask); \ > + } Could you please address my comments from this e-mail: https://mail.openvswitch.org/pipermail/ovs-dev/2020-August/373873.html ? i.e. convert this macro definition into function. > + > static int > parse_flow_match(struct flow_patterns *patterns, > struct match *match) > { > + struct rte_flow_item_eth *eth_mask = NULL; > + struct rte_flow_item_eth *eth_spec = NULL; > uint8_t *next_proto_mask = NULL; > struct flow *consumed_masks; > uint8_t proto = 0; > @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns *patterns, > 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); > - mask = xzalloc(sizeof *mask); > + eth_spec = xzalloc(sizeof *eth_spec); > + eth_mask = xzalloc(sizeof *eth_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 eth_spec->dst); > + memcpy(ð_spec->src, &match->flow.dl_src, sizeof eth_spec->src); > + eth_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 eth_mask->dst); > + memcpy(ð_mask->src, &match->wc.masks.dl_src, sizeof eth_mask->src); > + eth_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); > consumed_masks->dl_type = 0; > > - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask); > + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, eth_spec, eth_mask); > } > > /* VLAN */ > @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns *patterns, > /* IP v4 */ > if (match->flow.dl_type == htons(ETH_TYPE_IP)) { > struct rte_flow_item_ipv4 *spec, *mask; Empty line needed. > + NULL_ETH_MASK_IF_ZEROS; > > spec = xzalloc(sizeof *spec); > mask = xzalloc(sizeof *mask); > @@ -776,6 +790,7 @@ parse_flow_match(struct flow_patterns *patterns, > /* IP v6 */ > if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) { > struct rte_flow_item_ipv6 *spec, *mask; ditto. > + NULL_ETH_MASK_IF_ZEROS; > > spec = xzalloc(sizeof *spec); > mask = xzalloc(sizeof *mask); >
>-----Original Message----- >From: Ilya Maximets <i.maximets@ovn.org> >Sent: Thursday, August 13, 2020 9:26 PM >To: Emma Finn <emma.finn@intel.com>; dev@openvswitch.org >Cc: ian.stokes@intel.com; i.maximets@ovn.org; Eli Britstein <elibr@nvidia.com>; >beilei.xing@intel.com; i.maximets@ovn.org >Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken ethernet matching >HWOL for XL710 NIC > > >On 8/13/20 6:13 PM, Emma Finn wrote: >> 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> >> --- >> lib/netdev-offload-dpdk.c | 35 +++++++++++++++++++++++++---------- >> 1 file changed, 25 insertions(+), 10 deletions(-) >> >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >> index de6101e..ede2077 100644 >> --- a/lib/netdev-offload-dpdk.c >> +++ b/lib/netdev-offload-dpdk.c >> @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions *actions) >> actions->cnt = 0; >> } >> >> +/* >> +* 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. >> +*/ >> +#define NULL_ETH_MASK_IF_ZEROS \ >> + if (eth_mask && is_all_zeros(ð_mask->src, sizeof eth_mask->src) && \ >> + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \ >> + patterns->items[0].mask = NULL; \ >> + free(eth_mask); \ >> + } > >Could you please address my comments from this e-mail: > >https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.op >envswitch.org%2Fpipermail%2Fovs-dev%2F2020- >August%2F373873.html&data=02%7C01%7Celibr%40nvidia.com%7C08c8f >a0338e14858343d08d83fb664bf%7C43083d15727340c1b7db39efd9ccc17a% >7C0%7C1%7C637329399898256323&sdata=bM8dq1Da0%2F%2FL7m7y >m470T1aClwH9ZE%2BhlNcED1m7sdQ%3D&reserved=0 >? > >i.e. convert this macro definition into function. My previous patch was a POC. Here is a more "nicer" one. No macro/function, and no redundant allocation/free: Need to test and obviously finalize. diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index de6101e4d..c6d293af3 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -696,16 +696,26 @@ 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); + /* WRITE A COMMENT ABOUT THIS WORKAROUND. */ + if (is_all_zeros(&match->wc.masks.dl_dst, sizeof mask->dst) && + is_all_zeros(&match->wc.masks.dl_src, sizeof mask->src) && + match->wc.masks.dl_type == 0xFFFF && + (match->flow.dl_type == htons(ETH_TYPE_IP) || + match->flow.dl_type == htons(ETH_TYPE_IPV6))) { + 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); > >> + >> static int >> parse_flow_match(struct flow_patterns *patterns, >> struct match *match) { >> + struct rte_flow_item_eth *eth_mask = NULL; >> + struct rte_flow_item_eth *eth_spec = NULL; >> uint8_t *next_proto_mask = NULL; >> struct flow *consumed_masks; >> uint8_t proto = 0; >> @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns *patterns, >> 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); >> - mask = xzalloc(sizeof *mask); >> + eth_spec = xzalloc(sizeof *eth_spec); >> + eth_mask = xzalloc(sizeof *eth_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 eth_spec->dst); >> + memcpy(ð_spec->src, &match->flow.dl_src, sizeof eth_spec->src); >> + eth_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 eth_mask- >>dst); >> + memcpy(ð_mask->src, &match->wc.masks.dl_src, sizeof eth_mask- >>src); >> + eth_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); >> consumed_masks->dl_type = 0; >> >> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask); >> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, eth_spec, >> + eth_mask); >> } >> >> /* VLAN */ >> @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns *patterns, >> /* IP v4 */ >> if (match->flow.dl_type == htons(ETH_TYPE_IP)) { >> struct rte_flow_item_ipv4 *spec, *mask; > >Empty line needed. > >> + NULL_ETH_MASK_IF_ZEROS; >> >> spec = xzalloc(sizeof *spec); >> mask = xzalloc(sizeof *mask); @@ -776,6 +790,7 @@ >> parse_flow_match(struct flow_patterns *patterns, >> /* IP v6 */ >> if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) { >> struct rte_flow_item_ipv6 *spec, *mask; > >ditto. > >> + NULL_ETH_MASK_IF_ZEROS; >> >> spec = xzalloc(sizeof *spec); >> mask = xzalloc(sizeof *mask); >>
> >-----Original Message----- > >From: Ilya Maximets <i.maximets@ovn.org> > >Sent: Thursday, August 13, 2020 9:26 PM > >To: Emma Finn <emma.finn@intel.com>; dev@openvswitch.org > >Cc: ian.stokes@intel.com; i.maximets@ovn.org; Eli Britstein > ><elibr@nvidia.com>; beilei.xing@intel.com; i.maximets@ovn.org > >Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken ethernet > >matching HWOL for XL710 NIC > > > > > >On 8/13/20 6:13 PM, Emma Finn wrote: > >> 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> > >> --- > >> lib/netdev-offload-dpdk.c | 35 +++++++++++++++++++++++++---------- > >> 1 file changed, 25 insertions(+), 10 deletions(-) > >> > >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > >> index de6101e..ede2077 100644 > >> --- a/lib/netdev-offload-dpdk.c > >> +++ b/lib/netdev-offload-dpdk.c > >> @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions *actions) > >> actions->cnt = 0; > >> } > >> > >> +/* > >> +* 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. > >> +*/ > >> +#define NULL_ETH_MASK_IF_ZEROS \ > >> + if (eth_mask && is_all_zeros(ð_mask->src, sizeof eth_mask->src) && \ > >> + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \ > >> + patterns->items[0].mask = NULL; \ > >> + free(eth_mask); \ > >> + } > > > >Could you please address my comments from this e-mail: > > > >https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail. > >op > >envswitch.org%2Fpipermail%2Fovs-dev%2F2020- > >August%2F373873.html&data=02%7C01%7Celibr%40nvidia.com%7C08c8f > >a0338e14858343d08d83fb664bf%7C43083d15727340c1b7db39efd9ccc17a% > >7C0%7C1%7C637329399898256323&sdata=bM8dq1Da0%2F%2FL7m7y > >m470T1aClwH9ZE%2BhlNcED1m7sdQ%3D&reserved=0 > >? > > > >i.e. convert this macro definition into function. > My previous patch was a POC. Here is a more "nicer" one. No macro/function, > and no redundant allocation/free: > Need to test and obviously finalize. Thanks for this Eli, just a heads up Emma is out of office today but I can test this and submit the v2 if you prefer? @Ilya, what are your thoughts on this approach below? BR Ian > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index > de6101e4d..c6d293af3 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -696,16 +696,26 @@ 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); > + /* WRITE A COMMENT ABOUT THIS WORKAROUND. */ > + if (is_all_zeros(&match->wc.masks.dl_dst, sizeof mask->dst) && > + is_all_zeros(&match->wc.masks.dl_src, sizeof mask->src) && > + match->wc.masks.dl_type == 0xFFFF && > + (match->flow.dl_type == htons(ETH_TYPE_IP) || > + match->flow.dl_type == htons(ETH_TYPE_IPV6))) { > + 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); > > > >> + > >> static int > >> parse_flow_match(struct flow_patterns *patterns, > >> struct match *match) { > >> + struct rte_flow_item_eth *eth_mask = NULL; > >> + struct rte_flow_item_eth *eth_spec = NULL; > >> uint8_t *next_proto_mask = NULL; > >> struct flow *consumed_masks; > >> uint8_t proto = 0; > >> @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns *patterns, > >> 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); > >> - mask = xzalloc(sizeof *mask); > >> + eth_spec = xzalloc(sizeof *eth_spec); > >> + eth_mask = xzalloc(sizeof *eth_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 eth_spec->dst); > >> + memcpy(ð_spec->src, &match->flow.dl_src, sizeof eth_spec->src); > >> + eth_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 > >> + eth_mask- > >>dst); > >> + memcpy(ð_mask->src, &match->wc.masks.dl_src, sizeof > >> + eth_mask- > >>src); > >> + eth_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); > >> consumed_masks->dl_type = 0; > >> > >> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask); > >> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, eth_spec, > >> + eth_mask); > >> } > >> > >> /* VLAN */ > >> @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns *patterns, > >> /* IP v4 */ > >> if (match->flow.dl_type == htons(ETH_TYPE_IP)) { > >> struct rte_flow_item_ipv4 *spec, *mask; > > > >Empty line needed. > > > >> + NULL_ETH_MASK_IF_ZEROS; > >> > >> spec = xzalloc(sizeof *spec); > >> mask = xzalloc(sizeof *mask); @@ -776,6 +790,7 @@ > >> parse_flow_match(struct flow_patterns *patterns, > >> /* IP v6 */ > >> if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) { > >> struct rte_flow_item_ipv6 *spec, *mask; > > > >ditto. > > > >> + NULL_ETH_MASK_IF_ZEROS; > >> > >> spec = xzalloc(sizeof *spec); > >> mask = xzalloc(sizeof *mask); > >>
I am not going to submit any of it. BTW, it would already be v3. please make sure to revert it once the fix in i40e pmd is in place.
Thanks for flagging Eli, yes will revert once the PMD changes are in place, we're looking into that now.
I'll change the Patch header to v3 also.
Regards
Ian
From: Eli Britstein <elibr@nvidia.com>
Sent: Friday, August 14, 2020 10:37 AM
To: Stokes, Ian <ian.stokes@intel.com>; Ilya Maximets <i.maximets@ovn.org>; Finn, Emma <emma.finn@intel.com>; dev@openvswitch.org
Cc: Xing, Beilei <beilei.xing@intel.com>
Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken ethernet matching HWOL for XL710 NIC
I am not going to submit any of it. BTW, it would already be v3.
please make sure to revert it once the fix in i40e pmd is in place.
On 8/14/20 10:58 AM, Stokes, Ian wrote: >>> -----Original Message----- >>> From: Ilya Maximets <i.maximets@ovn.org> >>> Sent: Thursday, August 13, 2020 9:26 PM >>> To: Emma Finn <emma.finn@intel.com>; dev@openvswitch.org >>> Cc: ian.stokes@intel.com; i.maximets@ovn.org; Eli Britstein >>> <elibr@nvidia.com>; beilei.xing@intel.com; i.maximets@ovn.org >>> Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken ethernet >>> matching HWOL for XL710 NIC >>> >>> >>> On 8/13/20 6:13 PM, Emma Finn wrote: >>>> 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> >>>> --- >>>> lib/netdev-offload-dpdk.c | 35 +++++++++++++++++++++++++---------- >>>> 1 file changed, 25 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>>> index de6101e..ede2077 100644 >>>> --- a/lib/netdev-offload-dpdk.c >>>> +++ b/lib/netdev-offload-dpdk.c >>>> @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions *actions) >>>> actions->cnt = 0; >>>> } >>>> >>>> +/* >>>> +* 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. >>>> +*/ >>>> +#define NULL_ETH_MASK_IF_ZEROS \ >>>> + if (eth_mask && is_all_zeros(ð_mask->src, sizeof eth_mask->src) && \ >>>> + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \ >>>> + patterns->items[0].mask = NULL; \ >>>> + free(eth_mask); \ >>>> + } >>> >>> Could you please address my comments from this e-mail: >>> >>> https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail. >>> op >>> envswitch.org%2Fpipermail%2Fovs-dev%2F2020- >>> August%2F373873.html&data=02%7C01%7Celibr%40nvidia.com%7C08c8f >>> a0338e14858343d08d83fb664bf%7C43083d15727340c1b7db39efd9ccc17a% >>> 7C0%7C1%7C637329399898256323&sdata=bM8dq1Da0%2F%2FL7m7y >>> m470T1aClwH9ZE%2BhlNcED1m7sdQ%3D&reserved=0 >>> ? >>> >>> i.e. convert this macro definition into function. >> My previous patch was a POC. Here is a more "nicer" one. No macro/function, >> and no redundant allocation/free: >> Need to test and obviously finalize. > > Thanks for this Eli, just a heads up Emma is out of office today but I can test this and submit the v2 if you prefer? > > @Ilya, what are your thoughts on this approach below? It looks fine, but I'd re-write the if statement in a following way: if (match->wc.masks.dl_type == 0xffff && is_ip_any(&match->flow) && eth_addr_is_zero(match->wc.masks.dl_dst) && eth_addr_is_zero(match->wc.masks.dl_src)) { i.e. lightweight operations first, special functions for the flow type and eth_addr checking. > > BR > Ian >> >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index >> de6101e4d..c6d293af3 100644 >> --- a/lib/netdev-offload-dpdk.c >> +++ b/lib/netdev-offload-dpdk.c >> @@ -696,16 +696,26 @@ 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); >> + /* WRITE A COMMENT ABOUT THIS WORKAROUND. */ >> + if (is_all_zeros(&match->wc.masks.dl_dst, sizeof mask->dst) && >> + is_all_zeros(&match->wc.masks.dl_src, sizeof mask->src) && >> + match->wc.masks.dl_type == 0xFFFF && >> + (match->flow.dl_type == htons(ETH_TYPE_IP) || >> + match->flow.dl_type == htons(ETH_TYPE_IPV6))) { >> + 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); >>> >>>> + >>>> static int >>>> parse_flow_match(struct flow_patterns *patterns, >>>> struct match *match) { >>>> + struct rte_flow_item_eth *eth_mask = NULL; >>>> + struct rte_flow_item_eth *eth_spec = NULL; >>>> uint8_t *next_proto_mask = NULL; >>>> struct flow *consumed_masks; >>>> uint8_t proto = 0; >>>> @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns *patterns, >>>> 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); >>>> - mask = xzalloc(sizeof *mask); >>>> + eth_spec = xzalloc(sizeof *eth_spec); >>>> + eth_mask = xzalloc(sizeof *eth_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 eth_spec->dst); >>>> + memcpy(ð_spec->src, &match->flow.dl_src, sizeof eth_spec->src); >>>> + eth_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 >>>> + eth_mask- >>>> dst); >>>> + memcpy(ð_mask->src, &match->wc.masks.dl_src, sizeof >>>> + eth_mask- >>>> src); >>>> + eth_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); >>>> consumed_masks->dl_type = 0; >>>> >>>> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask); >>>> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, eth_spec, >>>> + eth_mask); >>>> } >>>> >>>> /* VLAN */ >>>> @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns *patterns, >>>> /* IP v4 */ >>>> if (match->flow.dl_type == htons(ETH_TYPE_IP)) { >>>> struct rte_flow_item_ipv4 *spec, *mask; >>> >>> Empty line needed. >>> >>>> + NULL_ETH_MASK_IF_ZEROS; >>>> >>>> spec = xzalloc(sizeof *spec); >>>> mask = xzalloc(sizeof *mask); @@ -776,6 +790,7 @@ >>>> parse_flow_match(struct flow_patterns *patterns, >>>> /* IP v6 */ >>>> if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) { >>>> struct rte_flow_item_ipv6 *spec, *mask; >>> >>> ditto. >>> >>>> + NULL_ETH_MASK_IF_ZEROS; >>>> >>>> spec = xzalloc(sizeof *spec); >>>> mask = xzalloc(sizeof *mask); >>>> >
> On 8/14/20 10:58 AM, Stokes, Ian wrote: > >>> -----Original Message----- > >>> From: Ilya Maximets <i.maximets@ovn.org> > >>> Sent: Thursday, August 13, 2020 9:26 PM > >>> To: Emma Finn <emma.finn@intel.com>; dev@openvswitch.org > >>> Cc: ian.stokes@intel.com; i.maximets@ovn.org; Eli Britstein > >>> <elibr@nvidia.com>; beilei.xing@intel.com; i.maximets@ovn.org > >>> Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken ethernet > >>> matching HWOL for XL710 NIC > >>> > >>> > >>> On 8/13/20 6:13 PM, Emma Finn wrote: > >>>> 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> > >>>> --- > >>>> lib/netdev-offload-dpdk.c | 35 +++++++++++++++++++++++++---------- > >>>> 1 file changed, 25 insertions(+), 10 deletions(-) > >>>> > >>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > >>>> index de6101e..ede2077 100644 > >>>> --- a/lib/netdev-offload-dpdk.c > >>>> +++ b/lib/netdev-offload-dpdk.c > >>>> @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions *actions) > >>>> actions->cnt = 0; > >>>> } > >>>> > >>>> +/* > >>>> +* 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. > >>>> +*/ > >>>> +#define NULL_ETH_MASK_IF_ZEROS \ > >>>> + if (eth_mask && is_all_zeros(ð_mask->src, sizeof eth_mask->src) > && \ > >>>> + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \ > >>>> + patterns->items[0].mask = NULL; \ > >>>> + free(eth_mask); \ > >>>> + } > >>> > >>> Could you please address my comments from this e-mail: > >>> > >>> > https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail. > >>> op > >>> envswitch.org%2Fpipermail%2Fovs-dev%2F2020- > >>> > August%2F373873.html&data=02%7C01%7Celibr%40nvidia.com%7C08c8f > >>> > a0338e14858343d08d83fb664bf%7C43083d15727340c1b7db39efd9ccc17a% > >>> > 7C0%7C1%7C637329399898256323&sdata=bM8dq1Da0%2F%2FL7m7y > >>> m470T1aClwH9ZE%2BhlNcED1m7sdQ%3D&reserved=0 > >>> ? > >>> > >>> i.e. convert this macro definition into function. > >> My previous patch was a POC. Here is a more "nicer" one. No > >> macro/function, and no redundant allocation/free: > >> Need to test and obviously finalize. > > > > Thanks for this Eli, just a heads up Emma is out of office today but I can test > this and submit the v2 if you prefer? > > > > @Ilya, what are your thoughts on this approach below? > > It looks fine, but I'd re-write the if statement in a following way: > > if (match->wc.masks.dl_type == 0xffff && is_ip_any(&match->flow) > && eth_addr_is_zero(match->wc.masks.dl_dst) > && eth_addr_is_zero(match->wc.masks.dl_src)) { > > i.e. lightweight operations first, special functions for the flow type and eth_addr > checking. > Sure, just testing this now, looks good so far, going to run through the usual quick validation and will send the v3 soon. Regards Ian > > > > BR > > Ian > >> > >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > >> index > >> de6101e4d..c6d293af3 100644 > >> --- a/lib/netdev-offload-dpdk.c > >> +++ b/lib/netdev-offload-dpdk.c > >> @@ -696,16 +696,26 @@ 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); > >> + /* WRITE A COMMENT ABOUT THIS WORKAROUND. */ > >> + if (is_all_zeros(&match->wc.masks.dl_dst, sizeof mask->dst) && > >> + is_all_zeros(&match->wc.masks.dl_src, sizeof mask->src) && > >> + match->wc.masks.dl_type == 0xFFFF && > >> + (match->flow.dl_type == htons(ETH_TYPE_IP) || > >> + match->flow.dl_type == htons(ETH_TYPE_IPV6))) { > >> + 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); > >>> > >>>> + > >>>> static int > >>>> parse_flow_match(struct flow_patterns *patterns, > >>>> struct match *match) { > >>>> + struct rte_flow_item_eth *eth_mask = NULL; > >>>> + struct rte_flow_item_eth *eth_spec = NULL; > >>>> uint8_t *next_proto_mask = NULL; > >>>> struct flow *consumed_masks; > >>>> uint8_t proto = 0; > >>>> @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns > *patterns, > >>>> 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); > >>>> - mask = xzalloc(sizeof *mask); > >>>> + eth_spec = xzalloc(sizeof *eth_spec); > >>>> + eth_mask = xzalloc(sizeof *eth_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 eth_spec- > >dst); > >>>> + memcpy(ð_spec->src, &match->flow.dl_src, sizeof eth_spec- > >src); > >>>> + eth_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 > >>>> + eth_mask- > >>>> dst); > >>>> + memcpy(ð_mask->src, &match->wc.masks.dl_src, sizeof > >>>> + eth_mask- > >>>> src); > >>>> + eth_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); > >>>> consumed_masks->dl_type = 0; > >>>> > >>>> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, > mask); > >>>> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, > >>>> + eth_spec, eth_mask); > >>>> } > >>>> > >>>> /* VLAN */ > >>>> @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns *patterns, > >>>> /* IP v4 */ > >>>> if (match->flow.dl_type == htons(ETH_TYPE_IP)) { > >>>> struct rte_flow_item_ipv4 *spec, *mask; > >>> > >>> Empty line needed. > >>> > >>>> + NULL_ETH_MASK_IF_ZEROS; > >>>> > >>>> spec = xzalloc(sizeof *spec); > >>>> mask = xzalloc(sizeof *mask); @@ -776,6 +790,7 @@ > >>>> parse_flow_match(struct flow_patterns *patterns, > >>>> /* IP v6 */ > >>>> if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) { > >>>> struct rte_flow_item_ipv6 *spec, *mask; > >>> > >>> ditto. > >>> > >>>> + NULL_ETH_MASK_IF_ZEROS; > >>>> > >>>> spec = xzalloc(sizeof *spec); > >>>> mask = xzalloc(sizeof *mask); > >>>> > >
> On 8/14/20 10:58 AM, Stokes, Ian wrote: > >>> -----Original Message----- > >>> From: Ilya Maximets <i.maximets@ovn.org> > >>> Sent: Thursday, August 13, 2020 9:26 PM > >>> To: Emma Finn <emma.finn@intel.com>; dev@openvswitch.org > >>> Cc: ian.stokes@intel.com; i.maximets@ovn.org; Eli Britstein > >>> <elibr@nvidia.com>; beilei.xing@intel.com; i.maximets@ovn.org > >>> Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken ethernet > >>> matching HWOL for XL710 NIC > >>> > >>> > >>> On 8/13/20 6:13 PM, Emma Finn wrote: > >>>> 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> > >>>> --- > >>>> lib/netdev-offload-dpdk.c | 35 +++++++++++++++++++++++++---------- > >>>> 1 file changed, 25 insertions(+), 10 deletions(-) > >>>> > >>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > >>>> index de6101e..ede2077 100644 > >>>> --- a/lib/netdev-offload-dpdk.c > >>>> +++ b/lib/netdev-offload-dpdk.c > >>>> @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions *actions) > >>>> actions->cnt = 0; > >>>> } > >>>> > >>>> +/* > >>>> +* 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. > >>>> +*/ > >>>> +#define NULL_ETH_MASK_IF_ZEROS \ > >>>> + if (eth_mask && is_all_zeros(ð_mask->src, sizeof eth_mask->src) > && \ > >>>> + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \ > >>>> + patterns->items[0].mask = NULL; \ > >>>> + free(eth_mask); \ > >>>> + } > >>> > >>> Could you please address my comments from this e-mail: > >>> > >>> > https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail. > >>> op > >>> envswitch.org%2Fpipermail%2Fovs-dev%2F2020- > >>> > August%2F373873.html&data=02%7C01%7Celibr%40nvidia.com%7C08c8f > >>> > a0338e14858343d08d83fb664bf%7C43083d15727340c1b7db39efd9ccc17a% > >>> > 7C0%7C1%7C637329399898256323&sdata=bM8dq1Da0%2F%2FL7m7y > >>> m470T1aClwH9ZE%2BhlNcED1m7sdQ%3D&reserved=0 > >>> ? > >>> > >>> i.e. convert this macro definition into function. > >> My previous patch was a POC. Here is a more "nicer" one. No > >> macro/function, and no redundant allocation/free: > >> Need to test and obviously finalize. > > > > Thanks for this Eli, just a heads up Emma is out of office today but I can test > this and submit the v2 if you prefer? > > > > @Ilya, what are your thoughts on this approach below? > > It looks fine, but I'd re-write the if statement in a following way: > > if (match->wc.masks.dl_type == 0xffff && is_ip_any(&match->flow) Just a quick one here Ilya, sparse throws an restricted ovs_be16 degrades to integer error with the above line, are you ok with the following change if (match->wc.masks.dl_type == htons(0xFFFF) && is_ip_any(&match->flow) BR Ian > && eth_addr_is_zero(match->wc.masks.dl_dst) > && eth_addr_is_zero(match->wc.masks.dl_src)) { > > i.e. lightweight operations first, special functions for the flow type and eth_addr > checking. > > > > > BR > > Ian > >> > >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > >> index > >> de6101e4d..c6d293af3 100644 > >> --- a/lib/netdev-offload-dpdk.c > >> +++ b/lib/netdev-offload-dpdk.c > >> @@ -696,16 +696,26 @@ 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); > >> + /* WRITE A COMMENT ABOUT THIS WORKAROUND. */ > >> + if (is_all_zeros(&match->wc.masks.dl_dst, sizeof mask->dst) && > >> + is_all_zeros(&match->wc.masks.dl_src, sizeof mask->src) && > >> + match->wc.masks.dl_type == 0xFFFF && > >> + (match->flow.dl_type == htons(ETH_TYPE_IP) || > >> + match->flow.dl_type == htons(ETH_TYPE_IPV6))) { > >> + 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); > >>> > >>>> + > >>>> static int > >>>> parse_flow_match(struct flow_patterns *patterns, > >>>> struct match *match) { > >>>> + struct rte_flow_item_eth *eth_mask = NULL; > >>>> + struct rte_flow_item_eth *eth_spec = NULL; > >>>> uint8_t *next_proto_mask = NULL; > >>>> struct flow *consumed_masks; > >>>> uint8_t proto = 0; > >>>> @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns > *patterns, > >>>> 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); > >>>> - mask = xzalloc(sizeof *mask); > >>>> + eth_spec = xzalloc(sizeof *eth_spec); > >>>> + eth_mask = xzalloc(sizeof *eth_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 eth_spec- > >dst); > >>>> + memcpy(ð_spec->src, &match->flow.dl_src, sizeof eth_spec- > >src); > >>>> + eth_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 > >>>> + eth_mask- > >>>> dst); > >>>> + memcpy(ð_mask->src, &match->wc.masks.dl_src, sizeof > >>>> + eth_mask- > >>>> src); > >>>> + eth_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); > >>>> consumed_masks->dl_type = 0; > >>>> > >>>> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, > mask); > >>>> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, > >>>> + eth_spec, eth_mask); > >>>> } > >>>> > >>>> /* VLAN */ > >>>> @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns *patterns, > >>>> /* IP v4 */ > >>>> if (match->flow.dl_type == htons(ETH_TYPE_IP)) { > >>>> struct rte_flow_item_ipv4 *spec, *mask; > >>> > >>> Empty line needed. > >>> > >>>> + NULL_ETH_MASK_IF_ZEROS; > >>>> > >>>> spec = xzalloc(sizeof *spec); > >>>> mask = xzalloc(sizeof *mask); @@ -776,6 +790,7 @@ > >>>> parse_flow_match(struct flow_patterns *patterns, > >>>> /* IP v6 */ > >>>> if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) { > >>>> struct rte_flow_item_ipv6 *spec, *mask; > >>> > >>> ditto. > >>> > >>>> + NULL_ETH_MASK_IF_ZEROS; > >>>> > >>>> spec = xzalloc(sizeof *spec); > >>>> mask = xzalloc(sizeof *mask); > >>>> > >
On 8/14/20 2:29 PM, Stokes, Ian wrote: >> On 8/14/20 10:58 AM, Stokes, Ian wrote: >>>>> -----Original Message----- >>>>> From: Ilya Maximets <i.maximets@ovn.org> >>>>> Sent: Thursday, August 13, 2020 9:26 PM >>>>> To: Emma Finn <emma.finn@intel.com>; dev@openvswitch.org >>>>> Cc: ian.stokes@intel.com; i.maximets@ovn.org; Eli Britstein >>>>> <elibr@nvidia.com>; beilei.xing@intel.com; i.maximets@ovn.org >>>>> Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken ethernet >>>>> matching HWOL for XL710 NIC >>>>> >>>>> >>>>> On 8/13/20 6:13 PM, Emma Finn wrote: >>>>>> 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> >>>>>> --- >>>>>> lib/netdev-offload-dpdk.c | 35 +++++++++++++++++++++++++---------- >>>>>> 1 file changed, 25 insertions(+), 10 deletions(-) >>>>>> >>>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>>>>> index de6101e..ede2077 100644 >>>>>> --- a/lib/netdev-offload-dpdk.c >>>>>> +++ b/lib/netdev-offload-dpdk.c >>>>>> @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions *actions) >>>>>> actions->cnt = 0; >>>>>> } >>>>>> >>>>>> +/* >>>>>> +* 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. >>>>>> +*/ >>>>>> +#define NULL_ETH_MASK_IF_ZEROS \ >>>>>> + if (eth_mask && is_all_zeros(ð_mask->src, sizeof eth_mask->src) >> && \ >>>>>> + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \ >>>>>> + patterns->items[0].mask = NULL; \ >>>>>> + free(eth_mask); \ >>>>>> + } >>>>> >>>>> Could you please address my comments from this e-mail: >>>>> >>>>> >> https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail. >>>>> op >>>>> envswitch.org%2Fpipermail%2Fovs-dev%2F2020- >>>>> >> August%2F373873.html&data=02%7C01%7Celibr%40nvidia.com%7C08c8f >>>>> >> a0338e14858343d08d83fb664bf%7C43083d15727340c1b7db39efd9ccc17a% >>>>> >> 7C0%7C1%7C637329399898256323&sdata=bM8dq1Da0%2F%2FL7m7y >>>>> m470T1aClwH9ZE%2BhlNcED1m7sdQ%3D&reserved=0 >>>>> ? >>>>> >>>>> i.e. convert this macro definition into function. >>>> My previous patch was a POC. Here is a more "nicer" one. No >>>> macro/function, and no redundant allocation/free: >>>> Need to test and obviously finalize. >>> >>> Thanks for this Eli, just a heads up Emma is out of office today but I can test >> this and submit the v2 if you prefer? >>> >>> @Ilya, what are your thoughts on this approach below? >> >> It looks fine, but I'd re-write the if statement in a following way: >> >> if (match->wc.masks.dl_type == 0xffff && is_ip_any(&match->flow) > > Just a quick one here Ilya, sparse throws an restricted ovs_be16 degrades to integer error with the above line, are you ok with the following change > > if (match->wc.masks.dl_type == htons(0xFFFF) && is_ip_any(&match->flow) Sure. I'd prefer keep the lowercase for a constant, but that is not really important. > > BR > Ian >> && eth_addr_is_zero(match->wc.masks.dl_dst) >> && eth_addr_is_zero(match->wc.masks.dl_src)) { >> >> i.e. lightweight operations first, special functions for the flow type and eth_addr >> checking. >> >>> >>> BR >>> Ian >>>> >>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>>> index >>>> de6101e4d..c6d293af3 100644 >>>> --- a/lib/netdev-offload-dpdk.c >>>> +++ b/lib/netdev-offload-dpdk.c >>>> @@ -696,16 +696,26 @@ 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); >>>> + /* WRITE A COMMENT ABOUT THIS WORKAROUND. */ >>>> + if (is_all_zeros(&match->wc.masks.dl_dst, sizeof mask->dst) && >>>> + is_all_zeros(&match->wc.masks.dl_src, sizeof mask->src) && >>>> + match->wc.masks.dl_type == 0xFFFF && >>>> + (match->flow.dl_type == htons(ETH_TYPE_IP) || >>>> + match->flow.dl_type == htons(ETH_TYPE_IPV6))) { >>>> + 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); >>>>> >>>>>> + >>>>>> static int >>>>>> parse_flow_match(struct flow_patterns *patterns, >>>>>> struct match *match) { >>>>>> + struct rte_flow_item_eth *eth_mask = NULL; >>>>>> + struct rte_flow_item_eth *eth_spec = NULL; >>>>>> uint8_t *next_proto_mask = NULL; >>>>>> struct flow *consumed_masks; >>>>>> uint8_t proto = 0; >>>>>> @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns >> *patterns, >>>>>> 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); >>>>>> - mask = xzalloc(sizeof *mask); >>>>>> + eth_spec = xzalloc(sizeof *eth_spec); >>>>>> + eth_mask = xzalloc(sizeof *eth_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 eth_spec- >>> dst); >>>>>> + memcpy(ð_spec->src, &match->flow.dl_src, sizeof eth_spec- >>> src); >>>>>> + eth_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 >>>>>> + eth_mask- >>>>>> dst); >>>>>> + memcpy(ð_mask->src, &match->wc.masks.dl_src, sizeof >>>>>> + eth_mask- >>>>>> src); >>>>>> + eth_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); >>>>>> consumed_masks->dl_type = 0; >>>>>> >>>>>> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, >> mask); >>>>>> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, >>>>>> + eth_spec, eth_mask); >>>>>> } >>>>>> >>>>>> /* VLAN */ >>>>>> @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns *patterns, >>>>>> /* IP v4 */ >>>>>> if (match->flow.dl_type == htons(ETH_TYPE_IP)) { >>>>>> struct rte_flow_item_ipv4 *spec, *mask; >>>>> >>>>> Empty line needed. >>>>> >>>>>> + NULL_ETH_MASK_IF_ZEROS; >>>>>> >>>>>> spec = xzalloc(sizeof *spec); >>>>>> mask = xzalloc(sizeof *mask); @@ -776,6 +790,7 @@ >>>>>> parse_flow_match(struct flow_patterns *patterns, >>>>>> /* IP v6 */ >>>>>> if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) { >>>>>> struct rte_flow_item_ipv6 *spec, *mask; >>>>> >>>>> ditto. >>>>> >>>>>> + NULL_ETH_MASK_IF_ZEROS; >>>>>> >>>>>> spec = xzalloc(sizeof *spec); >>>>>> mask = xzalloc(sizeof *mask); >>>>>> >>> >
> On 8/14/20 2:29 PM, Stokes, Ian wrote: > >> On 8/14/20 10:58 AM, Stokes, Ian wrote: > >>>>> -----Original Message----- > >>>>> From: Ilya Maximets <i.maximets@ovn.org> > >>>>> Sent: Thursday, August 13, 2020 9:26 PM > >>>>> To: Emma Finn <emma.finn@intel.com>; dev@openvswitch.org > >>>>> Cc: ian.stokes@intel.com; i.maximets@ovn.org; Eli Britstein > >>>>> <elibr@nvidia.com>; beilei.xing@intel.com; i.maximets@ovn.org > >>>>> Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken ethernet > >>>>> matching HWOL for XL710 NIC > >>>>> > >>>>> > >>>>> On 8/13/20 6:13 PM, Emma Finn wrote: > >>>>>> 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> > >>>>>> --- > >>>>>> lib/netdev-offload-dpdk.c | 35 > >>>>>> +++++++++++++++++++++++++---------- > >>>>>> 1 file changed, 25 insertions(+), 10 deletions(-) > >>>>>> > >>>>>> diff --git a/lib/netdev-offload-dpdk.c > >>>>>> b/lib/netdev-offload-dpdk.c index de6101e..ede2077 100644 > >>>>>> --- a/lib/netdev-offload-dpdk.c > >>>>>> +++ b/lib/netdev-offload-dpdk.c > >>>>>> @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions > *actions) > >>>>>> actions->cnt = 0; > >>>>>> } > >>>>>> > >>>>>> +/* > >>>>>> +* 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. > >>>>>> +*/ > >>>>>> +#define NULL_ETH_MASK_IF_ZEROS \ > >>>>>> + if (eth_mask && is_all_zeros(ð_mask->src, sizeof > >>>>>> +eth_mask->src) > >> && \ > >>>>>> + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \ > >>>>>> + patterns->items[0].mask = NULL; \ > >>>>>> + free(eth_mask); \ > >>>>>> + } > >>>>> > >>>>> Could you please address my comments from this e-mail: > >>>>> > >>>>> > >> https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail. > >>>>> op > >>>>> envswitch.org%2Fpipermail%2Fovs-dev%2F2020- > >>>>> > >> > August%2F373873.html&data=02%7C01%7Celibr%40nvidia.com%7C08c8f > >>>>> > >> a0338e14858343d08d83fb664bf%7C43083d15727340c1b7db39efd9ccc17a% > >>>>> > >> 7C0%7C1%7C637329399898256323&sdata=bM8dq1Da0%2F%2FL7m7y > >>>>> m470T1aClwH9ZE%2BhlNcED1m7sdQ%3D&reserved=0 > >>>>> ? > >>>>> > >>>>> i.e. convert this macro definition into function. > >>>> My previous patch was a POC. Here is a more "nicer" one. No > >>>> macro/function, and no redundant allocation/free: > >>>> Need to test and obviously finalize. > >>> > >>> Thanks for this Eli, just a heads up Emma is out of office today but > >>> I can test > >> this and submit the v2 if you prefer? > >>> > >>> @Ilya, what are your thoughts on this approach below? > >> > >> It looks fine, but I'd re-write the if statement in a following way: > >> > >> if (match->wc.masks.dl_type == 0xffff && is_ip_any(&match->flow) > > > > Just a quick one here Ilya, sparse throws an restricted ovs_be16 > > degrades to integer error with the above line, are you ok with the > > following change > > > > if (match->wc.masks.dl_type == htons(0xFFFF) && > > is_ip_any(&match->flow) > > Sure. I'd prefer keep the lowercase for a constant, but that is not really > important. No problem, I'll change it to lower before sending the v3, just waiting on Travis now but looks ok. Regards Ian > > > > > BR > > Ian > >> && eth_addr_is_zero(match->wc.masks.dl_dst) > >> && eth_addr_is_zero(match->wc.masks.dl_src)) { > >> > >> i.e. lightweight operations first, special functions for the flow > >> type and eth_addr checking. > >> > >>> > >>> BR > >>> Ian > >>>> > >>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > >>>> index > >>>> de6101e4d..c6d293af3 100644 > >>>> --- a/lib/netdev-offload-dpdk.c > >>>> +++ b/lib/netdev-offload-dpdk.c > >>>> @@ -696,16 +696,26 @@ 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); > >>>> + /* WRITE A COMMENT ABOUT THIS WORKAROUND. */ > >>>> + if (is_all_zeros(&match->wc.masks.dl_dst, sizeof mask->dst) && > >>>> + is_all_zeros(&match->wc.masks.dl_src, sizeof mask->src) && > >>>> + match->wc.masks.dl_type == 0xFFFF && > >>>> + (match->flow.dl_type == htons(ETH_TYPE_IP) || > >>>> + match->flow.dl_type == htons(ETH_TYPE_IPV6))) { > >>>> + 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); > >>>>> > >>>>>> + > >>>>>> static int > >>>>>> parse_flow_match(struct flow_patterns *patterns, > >>>>>> struct match *match) { > >>>>>> + struct rte_flow_item_eth *eth_mask = NULL; > >>>>>> + struct rte_flow_item_eth *eth_spec = NULL; > >>>>>> uint8_t *next_proto_mask = NULL; > >>>>>> struct flow *consumed_masks; > >>>>>> uint8_t proto = 0; > >>>>>> @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns > >> *patterns, > >>>>>> 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); > >>>>>> - mask = xzalloc(sizeof *mask); > >>>>>> + eth_spec = xzalloc(sizeof *eth_spec); > >>>>>> + eth_mask = xzalloc(sizeof *eth_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 > >>>>>> + eth_spec- > >>> dst); > >>>>>> + memcpy(ð_spec->src, &match->flow.dl_src, sizeof > >>>>>> + eth_spec- > >>> src); > >>>>>> + eth_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 > >>>>>> + eth_mask- > >>>>>> dst); > >>>>>> + memcpy(ð_mask->src, &match->wc.masks.dl_src, sizeof > >>>>>> + eth_mask- > >>>>>> src); > >>>>>> + eth_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); > >>>>>> consumed_masks->dl_type = 0; > >>>>>> > >>>>>> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, > >> mask); > >>>>>> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, > >>>>>> + eth_spec, eth_mask); > >>>>>> } > >>>>>> > >>>>>> /* VLAN */ > >>>>>> @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns > *patterns, > >>>>>> /* IP v4 */ > >>>>>> if (match->flow.dl_type == htons(ETH_TYPE_IP)) { > >>>>>> struct rte_flow_item_ipv4 *spec, *mask; > >>>>> > >>>>> Empty line needed. > >>>>> > >>>>>> + NULL_ETH_MASK_IF_ZEROS; > >>>>>> > >>>>>> spec = xzalloc(sizeof *spec); > >>>>>> mask = xzalloc(sizeof *mask); @@ -776,6 +790,7 @@ > >>>>>> parse_flow_match(struct flow_patterns *patterns, > >>>>>> /* IP v6 */ > >>>>>> if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) { > >>>>>> struct rte_flow_item_ipv6 *spec, *mask; > >>>>> > >>>>> ditto. > >>>>> > >>>>>> + NULL_ETH_MASK_IF_ZEROS; > >>>>>> > >>>>>> spec = xzalloc(sizeof *spec); > >>>>>> mask = xzalloc(sizeof *mask); > >>>>>> > >>> > >
>-----Original Message----- >From: Stokes, Ian <ian.stokes@intel.com> >Sent: Friday, August 14, 2020 4:03 PM >To: Ilya Maximets <i.maximets@ovn.org>; Eli Britstein <elibr@nvidia.com>; Finn, >Emma <emma.finn@intel.com>; dev@openvswitch.org >Cc: Xing, Beilei <beilei.xing@intel.com> >Subject: RE: [PATCH] netdev-offload-dpdk: Fix for broken ethernet matching >HWOL for XL710 NIC > > >> On 8/14/20 2:29 PM, Stokes, Ian wrote: >> >> On 8/14/20 10:58 AM, Stokes, Ian wrote: >> >>>>> -----Original Message----- >> >>>>> From: Ilya Maximets <i.maximets@ovn.org> >> >>>>> Sent: Thursday, August 13, 2020 9:26 PM >> >>>>> To: Emma Finn <emma.finn@intel.com>; dev@openvswitch.org >> >>>>> Cc: ian.stokes@intel.com; i.maximets@ovn.org; Eli Britstein >> >>>>> <elibr@nvidia.com>; beilei.xing@intel.com; i.maximets@ovn.org >> >>>>> Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken >> >>>>> ethernet matching HWOL for XL710 NIC >> >>>>> >> >>>>> >> >>>>> On 8/13/20 6:13 PM, Emma Finn wrote: >> >>>>>> 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> >> >>>>>> --- >> >>>>>> lib/netdev-offload-dpdk.c | 35 >> >>>>>> +++++++++++++++++++++++++---------- >> >>>>>> 1 file changed, 25 insertions(+), 10 deletions(-) >> >>>>>> >> >>>>>> diff --git a/lib/netdev-offload-dpdk.c >> >>>>>> b/lib/netdev-offload-dpdk.c index de6101e..ede2077 100644 >> >>>>>> --- a/lib/netdev-offload-dpdk.c >> >>>>>> +++ b/lib/netdev-offload-dpdk.c >> >>>>>> @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions >> *actions) >> >>>>>> actions->cnt = 0; >> >>>>>> } >> >>>>>> >> >>>>>> +/* >> >>>>>> +* 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. >> >>>>>> +*/ >> >>>>>> +#define NULL_ETH_MASK_IF_ZEROS \ >> >>>>>> + if (eth_mask && is_all_zeros(ð_mask->src, sizeof >> >>>>>> +eth_mask->src) >> >> && \ >> >>>>>> + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \ >> >>>>>> + patterns->items[0].mask = NULL; \ >> >>>>>> + free(eth_mask); \ >> >>>>>> + } >> >>>>> >> >>>>> Could you please address my comments from this e-mail: >> >>>>> >> >>>>> >> >> >https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail. >> >>>>> op >> >>>>> envswitch.org%2Fpipermail%2Fovs-dev%2F2020- >> >>>>> >> >> >> >August%2F373873.html&data=02%7C01%7Celibr%40nvidia.com%7C08c8f >> >>>>> >> >> >a0338e14858343d08d83fb664bf%7C43083d15727340c1b7db39efd9ccc17a% >> >>>>> >> >> >7C0%7C1%7C637329399898256323&sdata=bM8dq1Da0%2F%2FL7m7y >> >>>>> m470T1aClwH9ZE%2BhlNcED1m7sdQ%3D&reserved=0 >> >>>>> ? >> >>>>> >> >>>>> i.e. convert this macro definition into function. >> >>>> My previous patch was a POC. Here is a more "nicer" one. No >> >>>> macro/function, and no redundant allocation/free: >> >>>> Need to test and obviously finalize. >> >>> >> >>> Thanks for this Eli, just a heads up Emma is out of office today >> >>> but I can test >> >> this and submit the v2 if you prefer? >> >>> >> >>> @Ilya, what are your thoughts on this approach below? >> >> >> >> It looks fine, but I'd re-write the if statement in a following way: >> >> >> >> if (match->wc.masks.dl_type == 0xffff && >> >> is_ip_any(&match->flow) >> > >> > Just a quick one here Ilya, sparse throws an restricted ovs_be16 >> > degrades to integer error with the above line, are you ok with the >> > following change >> > >> > if (match->wc.masks.dl_type == htons(0xFFFF) && >> > is_ip_any(&match->flow) >> >> Sure. I'd prefer keep the lowercase for a constant, but that is not >> really important. > >No problem, I'll change it to lower before sending the v3, just waiting on Travis >now but looks ok. OVS_BE16_MAX > >Regards >Ian >> >> > >> > BR >> > Ian >> >> && eth_addr_is_zero(match->wc.masks.dl_dst) >> >> && eth_addr_is_zero(match->wc.masks.dl_src)) { >> >> >> >> i.e. lightweight operations first, special functions for the flow >> >> type and eth_addr checking. >> >> >> >>> >> >>> BR >> >>> Ian >> >>>> >> >>>> diff --git a/lib/netdev-offload-dpdk.c >> >>>> b/lib/netdev-offload-dpdk.c index >> >>>> de6101e4d..c6d293af3 100644 >> >>>> --- a/lib/netdev-offload-dpdk.c >> >>>> +++ b/lib/netdev-offload-dpdk.c >> >>>> @@ -696,16 +696,26 @@ 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); >> >>>> + /* WRITE A COMMENT ABOUT THIS WORKAROUND. */ >> >>>> + if (is_all_zeros(&match->wc.masks.dl_dst, sizeof mask->dst) && >> >>>> + is_all_zeros(&match->wc.masks.dl_src, sizeof mask->src) && >> >>>> + match->wc.masks.dl_type == 0xFFFF && >> >>>> + (match->flow.dl_type == htons(ETH_TYPE_IP) || >> >>>> + match->flow.dl_type == htons(ETH_TYPE_IPV6))) { >> >>>> + 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); >> >>>>> >> >>>>>> + >> >>>>>> static int >> >>>>>> parse_flow_match(struct flow_patterns *patterns, >> >>>>>> struct match *match) { >> >>>>>> + struct rte_flow_item_eth *eth_mask = NULL; >> >>>>>> + struct rte_flow_item_eth *eth_spec = NULL; >> >>>>>> uint8_t *next_proto_mask = NULL; >> >>>>>> struct flow *consumed_masks; >> >>>>>> uint8_t proto = 0; >> >>>>>> @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns >> >> *patterns, >> >>>>>> 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); >> >>>>>> - mask = xzalloc(sizeof *mask); >> >>>>>> + eth_spec = xzalloc(sizeof *eth_spec); >> >>>>>> + eth_mask = xzalloc(sizeof *eth_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 >> >>>>>> + eth_spec- >> >>> dst); >> >>>>>> + memcpy(ð_spec->src, &match->flow.dl_src, sizeof >> >>>>>> + eth_spec- >> >>> src); >> >>>>>> + eth_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 >> >>>>>> + eth_mask- >> >>>>>> dst); >> >>>>>> + memcpy(ð_mask->src, &match->wc.masks.dl_src, sizeof >> >>>>>> + eth_mask- >> >>>>>> src); >> >>>>>> + eth_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); >> >>>>>> consumed_masks->dl_type = 0; >> >>>>>> >> >>>>>> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, >> >> mask); >> >>>>>> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, >> >>>>>> + eth_spec, eth_mask); >> >>>>>> } >> >>>>>> >> >>>>>> /* VLAN */ >> >>>>>> @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns >> *patterns, >> >>>>>> /* IP v4 */ >> >>>>>> if (match->flow.dl_type == htons(ETH_TYPE_IP)) { >> >>>>>> struct rte_flow_item_ipv4 *spec, *mask; >> >>>>> >> >>>>> Empty line needed. >> >>>>> >> >>>>>> + NULL_ETH_MASK_IF_ZEROS; >> >>>>>> >> >>>>>> spec = xzalloc(sizeof *spec); >> >>>>>> mask = xzalloc(sizeof *mask); @@ -776,6 +790,7 @@ >> >>>>>> parse_flow_match(struct flow_patterns *patterns, >> >>>>>> /* IP v6 */ >> >>>>>> if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) { >> >>>>>> struct rte_flow_item_ipv6 *spec, *mask; >> >>>>> >> >>>>> ditto. >> >>>>> >> >>>>>> + NULL_ETH_MASK_IF_ZEROS; >> >>>>>> >> >>>>>> spec = xzalloc(sizeof *spec); >> >>>>>> mask = xzalloc(sizeof *mask); >> >>>>>> >> >>> >> >
> >-----Original Message----- > >From: Stokes, Ian <ian.stokes@intel.com> > >Sent: Friday, August 14, 2020 4:03 PM > >To: Ilya Maximets <i.maximets@ovn.org>; Eli Britstein > ><elibr@nvidia.com>; Finn, Emma <emma.finn@intel.com>; > >dev@openvswitch.org > >Cc: Xing, Beilei <beilei.xing@intel.com> > >Subject: RE: [PATCH] netdev-offload-dpdk: Fix for broken ethernet > >matching HWOL for XL710 NIC > > > > > >> On 8/14/20 2:29 PM, Stokes, Ian wrote: > >> >> On 8/14/20 10:58 AM, Stokes, Ian wrote: > >> >>>>> -----Original Message----- > >> >>>>> From: Ilya Maximets <i.maximets@ovn.org> > >> >>>>> Sent: Thursday, August 13, 2020 9:26 PM > >> >>>>> To: Emma Finn <emma.finn@intel.com>; dev@openvswitch.org > >> >>>>> Cc: ian.stokes@intel.com; i.maximets@ovn.org; Eli Britstein > >> >>>>> <elibr@nvidia.com>; beilei.xing@intel.com; i.maximets@ovn.org > >> >>>>> Subject: Re: [PATCH] netdev-offload-dpdk: Fix for broken > >> >>>>> ethernet matching HWOL for XL710 NIC > >> >>>>> > >> >>>>> > >> >>>>> On 8/13/20 6:13 PM, Emma Finn wrote: > >> >>>>>> 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> > >> >>>>>> --- > >> >>>>>> lib/netdev-offload-dpdk.c | 35 > >> >>>>>> +++++++++++++++++++++++++---------- > >> >>>>>> 1 file changed, 25 insertions(+), 10 deletions(-) > >> >>>>>> > >> >>>>>> diff --git a/lib/netdev-offload-dpdk.c > >> >>>>>> b/lib/netdev-offload-dpdk.c index de6101e..ede2077 100644 > >> >>>>>> --- a/lib/netdev-offload-dpdk.c > >> >>>>>> +++ b/lib/netdev-offload-dpdk.c > >> >>>>>> @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions > >> *actions) > >> >>>>>> actions->cnt = 0; > >> >>>>>> } > >> >>>>>> > >> >>>>>> +/* > >> >>>>>> +* 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. > >> >>>>>> +*/ > >> >>>>>> +#define NULL_ETH_MASK_IF_ZEROS \ > >> >>>>>> + if (eth_mask && is_all_zeros(ð_mask->src, sizeof > >> >>>>>> +eth_mask->src) > >> >> && \ > >> >>>>>> + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \ > >> >>>>>> + patterns->items[0].mask = NULL; \ > >> >>>>>> + free(eth_mask); \ > >> >>>>>> + } > >> >>>>> > >> >>>>> Could you please address my comments from this e-mail: > >> >>>>> > >> >>>>> > >> >> > >https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail. > >> >>>>> op > >> >>>>> envswitch.org%2Fpipermail%2Fovs-dev%2F2020- > >> >>>>> > >> >> > >> > >August%2F373873.html&data=02%7C01%7Celibr%40nvidia.com%7C08c8f > >> >>>>> > >> >> > >a0338e14858343d08d83fb664bf%7C43083d15727340c1b7db39efd9ccc17a% > >> >>>>> > >> >> > >7C0%7C1%7C637329399898256323&sdata=bM8dq1Da0%2F%2FL7m7y > >> >>>>> m470T1aClwH9ZE%2BhlNcED1m7sdQ%3D&reserved=0 > >> >>>>> ? > >> >>>>> > >> >>>>> i.e. convert this macro definition into function. > >> >>>> My previous patch was a POC. Here is a more "nicer" one. No > >> >>>> macro/function, and no redundant allocation/free: > >> >>>> Need to test and obviously finalize. > >> >>> > >> >>> Thanks for this Eli, just a heads up Emma is out of office today > >> >>> but I can test > >> >> this and submit the v2 if you prefer? > >> >>> > >> >>> @Ilya, what are your thoughts on this approach below? > >> >> > >> >> It looks fine, but I'd re-write the if statement in a following way: > >> >> > >> >> if (match->wc.masks.dl_type == 0xffff && > >> >> is_ip_any(&match->flow) > >> > > >> > Just a quick one here Ilya, sparse throws an restricted ovs_be16 > >> > degrades to integer error with the above line, are you ok with the > >> > following change > >> > > >> > if (match->wc.masks.dl_type == htons(0xFFFF) && > >> > is_ip_any(&match->flow) > >> > >> Sure. I'd prefer keep the lowercase for a constant, but that is not > >> really important. > > > >No problem, I'll change it to lower before sending the v3, just waiting > >on Travis now but looks ok. > OVS_BE16_MAX Sure, that might look better again rather than the call to htons. Will change and send the v3 now. Regards Ian > > > >Regards > >Ian > >> > >> > > >> > BR > >> > Ian > >> >> && eth_addr_is_zero(match->wc.masks.dl_dst) > >> >> && eth_addr_is_zero(match->wc.masks.dl_src)) { > >> >> > >> >> i.e. lightweight operations first, special functions for the flow > >> >> type and eth_addr checking. > >> >> > >> >>> > >> >>> BR > >> >>> Ian > >> >>>> > >> >>>> diff --git a/lib/netdev-offload-dpdk.c > >> >>>> b/lib/netdev-offload-dpdk.c index > >> >>>> de6101e4d..c6d293af3 100644 > >> >>>> --- a/lib/netdev-offload-dpdk.c > >> >>>> +++ b/lib/netdev-offload-dpdk.c > >> >>>> @@ -696,16 +696,26 @@ 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); > >> >>>> + /* WRITE A COMMENT ABOUT THIS WORKAROUND. */ > >> >>>> + if (is_all_zeros(&match->wc.masks.dl_dst, sizeof mask->dst) && > >> >>>> + is_all_zeros(&match->wc.masks.dl_src, sizeof mask->src) && > >> >>>> + match->wc.masks.dl_type == 0xFFFF && > >> >>>> + (match->flow.dl_type == htons(ETH_TYPE_IP) || > >> >>>> + match->flow.dl_type == htons(ETH_TYPE_IPV6))) { > >> >>>> + 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); > >> >>>>> > >> >>>>>> + > >> >>>>>> static int > >> >>>>>> parse_flow_match(struct flow_patterns *patterns, > >> >>>>>> struct match *match) { > >> >>>>>> + struct rte_flow_item_eth *eth_mask = NULL; > >> >>>>>> + struct rte_flow_item_eth *eth_spec = NULL; > >> >>>>>> uint8_t *next_proto_mask = NULL; > >> >>>>>> struct flow *consumed_masks; > >> >>>>>> uint8_t proto = 0; > >> >>>>>> @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns > >> >> *patterns, > >> >>>>>> 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); > >> >>>>>> - mask = xzalloc(sizeof *mask); > >> >>>>>> + eth_spec = xzalloc(sizeof *eth_spec); > >> >>>>>> + eth_mask = xzalloc(sizeof *eth_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 > >> >>>>>> + eth_spec- > >> >>> dst); > >> >>>>>> + memcpy(ð_spec->src, &match->flow.dl_src, sizeof > >> >>>>>> + eth_spec- > >> >>> src); > >> >>>>>> + eth_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 > >> >>>>>> + eth_mask- > >> >>>>>> dst); > >> >>>>>> + memcpy(ð_mask->src, &match->wc.masks.dl_src, > >> >>>>>> + sizeof > >> >>>>>> + eth_mask- > >> >>>>>> src); > >> >>>>>> + eth_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); > >> >>>>>> consumed_masks->dl_type = 0; > >> >>>>>> > >> >>>>>> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, > >> >> mask); > >> >>>>>> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, > >> >>>>>> + eth_spec, eth_mask); > >> >>>>>> } > >> >>>>>> > >> >>>>>> /* VLAN */ > >> >>>>>> @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns > >> *patterns, > >> >>>>>> /* IP v4 */ > >> >>>>>> if (match->flow.dl_type == htons(ETH_TYPE_IP)) { > >> >>>>>> struct rte_flow_item_ipv4 *spec, *mask; > >> >>>>> > >> >>>>> Empty line needed. > >> >>>>> > >> >>>>>> + NULL_ETH_MASK_IF_ZEROS; > >> >>>>>> > >> >>>>>> spec = xzalloc(sizeof *spec); > >> >>>>>> mask = xzalloc(sizeof *mask); @@ -776,6 +790,7 @@ > >> >>>>>> parse_flow_match(struct flow_patterns *patterns, > >> >>>>>> /* IP v6 */ > >> >>>>>> if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) { > >> >>>>>> struct rte_flow_item_ipv6 *spec, *mask; > >> >>>>> > >> >>>>> ditto. > >> >>>>> > >> >>>>>> + NULL_ETH_MASK_IF_ZEROS; > >> >>>>>> > >> >>>>>> spec = xzalloc(sizeof *spec); > >> >>>>>> mask = xzalloc(sizeof *mask); > >> >>>>>> > >> >>> > >> >
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index de6101e..ede2077 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -672,10 +672,24 @@ free_flow_actions(struct flow_actions *actions) actions->cnt = 0; } +/* +* 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. +*/ +#define NULL_ETH_MASK_IF_ZEROS \ + if (eth_mask && is_all_zeros(ð_mask->src, sizeof eth_mask->src) && \ + is_all_zeros(ð_mask->dst, sizeof eth_mask->dst)) { \ + patterns->items[0].mask = NULL; \ + free(eth_mask); \ + } + static int parse_flow_match(struct flow_patterns *patterns, struct match *match) { + struct rte_flow_item_eth *eth_mask = NULL; + struct rte_flow_item_eth *eth_spec = NULL; uint8_t *next_proto_mask = NULL; struct flow *consumed_masks; uint8_t proto = 0; @@ -694,24 +708,23 @@ parse_flow_match(struct flow_patterns *patterns, 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); - mask = xzalloc(sizeof *mask); + eth_spec = xzalloc(sizeof *eth_spec); + eth_mask = xzalloc(sizeof *eth_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 eth_spec->dst); + memcpy(ð_spec->src, &match->flow.dl_src, sizeof eth_spec->src); + eth_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 eth_mask->dst); + memcpy(ð_mask->src, &match->wc.masks.dl_src, sizeof eth_mask->src); + eth_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); consumed_masks->dl_type = 0; - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask); + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, eth_spec, eth_mask); } /* VLAN */ @@ -738,6 +751,7 @@ parse_flow_match(struct flow_patterns *patterns, /* IP v4 */ if (match->flow.dl_type == htons(ETH_TYPE_IP)) { struct rte_flow_item_ipv4 *spec, *mask; + NULL_ETH_MASK_IF_ZEROS; spec = xzalloc(sizeof *spec); mask = xzalloc(sizeof *mask); @@ -776,6 +790,7 @@ parse_flow_match(struct flow_patterns *patterns, /* IP v6 */ if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) { struct rte_flow_item_ipv6 *spec, *mask; + NULL_ETH_MASK_IF_ZEROS; spec = xzalloc(sizeof *spec); mask = xzalloc(sizeof *mask);