[ovs-dev] [PATCH] netdev-offload-dpdk: Fix for broken ethernet matching HWOL for XL710 NIC
Stokes, Ian
ian.stokes at intel.com
Fri Aug 14 13:35:13 UTC 2020
> >-----Original Message-----
> >From: Stokes, Ian <ian.stokes at intel.com>
> >Sent: Friday, August 14, 2020 4:03 PM
> >To: Ilya Maximets <i.maximets at ovn.org>; Eli Britstein
> ><elibr at nvidia.com>; Finn, Emma <emma.finn at intel.com>;
> >dev at openvswitch.org
> >Cc: Xing, Beilei <beilei.xing at 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 at ovn.org>
> >> >>>>> Sent: Thursday, August 13, 2020 9:26 PM
> >> >>>>> To: Emma Finn <emma.finn at intel.com>; dev at openvswitch.org
> >> >>>>> Cc: ian.stokes at intel.com; i.maximets at ovn.org; Eli Britstein
> >> >>>>> <elibr at nvidia.com>; beilei.xing at intel.com; i.maximets at 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 at intel.com>
> >> >>>>>> Signed-off-by: Eli Britstein <elibr at nvidia.com>
> >> >>>>>> Co-authored-by: Eli Britstein <elibr at 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);
> >> >>>>>>
> >> >>>
> >> >
More information about the dev
mailing list