[ovs-dev] [PATCH] netdev-offload-dpdk: Fix for broken ethernet matching HWOL for XL710 NIC

Eli Britstein elibr at nvidia.com
Fri Aug 14 09:37:24 UTC 2020


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.
________________________________
From: Stokes, Ian <ian.stokes at intel.com>
Sent: Friday, August 14, 2020 11:58 AM
To: Eli Britstein <elibr at nvidia.com>; Ilya Maximets <i.maximets at ovn.org>; Finn, Emma <emma.finn at intel.com>; dev at openvswitch.org <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

External email: Use caution opening links or attachments


> >-----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?

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