[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:03:19 UTC 2020


> 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(&eth_mask->src, sizeof
> >>>>>> +eth_mask->src)
> >> && \
> >>>>>> +        is_all_zeros(&eth_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(&eth_spec->dst, &match->flow.dl_dst, sizeof
> >>>>>> + eth_spec-
> >>> dst);
> >>>>>> +        memcpy(&eth_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(&eth_mask->dst, &match->wc.masks.dl_dst, sizeof
> >>>>>> + eth_mask-
> >>>>>> dst);
> >>>>>> +        memcpy(&eth_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