> >-----Original Message----- > >From: Stokes, Ian <ian.sto...@intel.com> > >Sent: Friday, August 14, 2020 4:03 PM > >To: Ilya Maximets <i.maxim...@ovn.org>; Eli Britstein > ><el...@nvidia.com>; Finn, Emma <emma.f...@intel.com>; > >d...@openvswitch.org > >Cc: Xing, Beilei <beilei.x...@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.maxim...@ovn.org> > >> >>>>> Sent: Thursday, August 13, 2020 9:26 PM > >> >>>>> To: Emma Finn <emma.f...@intel.com>; d...@openvswitch.org > >> >>>>> Cc: ian.sto...@intel.com; i.maxim...@ovn.org; Eli Britstein > >> >>>>> <el...@nvidia.com>; beilei.x...@intel.com; i.maxim...@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.f...@intel.com> > >> >>>>>> Signed-off-by: Eli Britstein <el...@nvidia.com> > >> >>>>>> Co-authored-by: Eli Britstein <el...@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); > >> >>>>>> > >> >>> > >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev