[ovs-dev] [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken ethernet matching HWOL for XL710NIC.
Stokes, Ian
ian.stokes at intel.com
Mon Aug 17 11:30:41 UTC 2020
> >> -----Original Message-----
> >> From: Ilya Maximets <i.maximets at ovn.org>
> >> Sent: Monday, August 17, 2020 12:59 PM
> >> To: Ian Stokes <ian.stokes at intel.com>; dev at openvswitch.org
> >> Cc: Eli Britstein <elibr at nvidia.com>; i.maximets at ovn.org;
> >> emma.finn at intel.com; i.maximets at ovn.org
> >> Subject: Re: [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken
> >> ethernet matching HWOL for XL710NIC.
> >>
> >>
> >> On 8/14/20 3:38 PM, Ian Stokes wrote:
> >>> From: Emma Finn <emma.finn at intel.com>
> >>>
> >>> 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>
> >>> Tested-by: Ian Stokes <ian.stokes at intel.com>
> >>> ---
> >>
> >> Thanks, Ian and Eli.
> >> This patch looks good to me, but I had some issues backporting it to
> >> 2.12 and below. We have removed the XL710 workaround from these old
> >> branches too, so I'm assuming that we need this patch there.
> >>
> >> On older branches memory for patterns is statically allocated, so
> >> it's not easy to NULL-ify pointers.
> >>
> >> Following version of the code applies to 2.12 and could be easily
> >> backported down to 2.10:
> >>
> >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> >> index 7146b2aab..b824218af 100644
> >> --- a/lib/netdev-offload-dpdk.c
> >> +++ b/lib/netdev-offload-dpdk.c
> >> @@ -446,9 +446,18 @@ netdev_offload_dpdk_add_flow(struct netdev
> >> *netdev,
> >> memset(&mask, 0, sizeof mask);
> >>
> >> /* Eth */
> >> - if (match->wc.masks.dl_type ||
> >> - !eth_addr_is_zero(match->wc.masks.dl_src) ||
> >> - !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> >> + if (match->wc.masks.dl_type == OVS_BE16_MAX && is_ip_any(&match-
> >>> flow)
> >> + && eth_addr_is_zero(match->wc.masks.dl_dst)
> >> + && eth_addr_is_zero(match->wc.masks.dl_src)) {
> >> + /*
> >> + * 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.
> >> + */
> >> + add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL,
> NULL);
> >> + } else if (match->wc.masks.dl_type ||
> >> + !eth_addr_is_zero(match->wc.masks.dl_src) ||
> >> + !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> >> memcpy(&spec.eth.dst, &match->flow.dl_dst, sizeof spec.eth.dst);
> >> memcpy(&spec.eth.src, &match->flow.dl_src, sizeof spec.eth.src);
> >> spec.eth.type = match->flow.dl_type;
> >> ---
> >>
> >> If it looks good to you, I could apply original patch (below) to
> >> master,
> >> 2.14 and 2.13, and a slightly different version (above) to 2.12, 2.11
> >> and 2.10 once travis builds done. What do you think?
> > If intel can also do with zero masks (eth type spec 0x0800 type mask 0 /
> ipv4...), we don't have to NULL-ify but only zero the mask, but that's already a
> logic change than what we know working.
> > Anyway, the above LGTM.
> > Your plan is good, but there is also a slightly different approach. We can adapt
> the above and use it (or similar) for the 2.13+ branches, instead of the below, to
> keep consistent and easier backporting in future.
>
>
> Yes, I thought about this. We will need to duplicate 'consumed' part on new
> branches, but that might be not a big deal. The code for new branches will look
> like this:
>
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index
> de6101e4d..5b632bac4 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -691,9 +691,22 @@ parse_flow_match(struct flow_patterns *patterns,
> consumed_masks->packet_type = 0;
>
> /* Eth */
> - if (match->wc.masks.dl_type ||
> - !eth_addr_is_zero(match->wc.masks.dl_src) ||
> - !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> + if (match->wc.masks.dl_type == OVS_BE16_MAX && is_ip_any(&match-
> >flow)
> + && eth_addr_is_zero(match->wc.masks.dl_dst)
> + && eth_addr_is_zero(match->wc.masks.dl_src)) {
> + /*
> + * 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.
> + */
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
> +
> + 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;
> + } else 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);
> ---
>
> If looks good for everyone, I could use above code for the patch on new
> branches and the version without 'consumed_*' lines for older ones.
>
> Ian, Eli, what do you think?
I think this approach sounds ok, Emma is back in office and so can help validate the patches if there are queries around supporting zeroing on the X710 Nics.
@Emma can you validate what Ilya has proposed?
Regards
Ian
>
> Do we need v4 for this?
>
> >>
> >>
> >>> lib/netdev-offload-dpdk.c | 28 ++++++++++++++++++++--------
> >>> 1 file changed, 20 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> >>> index de6101e4d..2d668275a 100644
> >>> --- a/lib/netdev-offload-dpdk.c
> >>> +++ b/lib/netdev-offload-dpdk.c
> >>> @@ -696,16 +696,28 @@ 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);
> >>> + /*
> >>> + * 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.
> >>> + */
> >>> + if (match->wc.masks.dl_type == OVS_BE16_MAX &&
> >>> + is_ip_any(&match-
> >>> flow)
> >>> + && eth_addr_is_zero(match->wc.masks.dl_dst)
> >>> + && eth_addr_is_zero(match->wc.masks.dl_src)) {
> >>> + 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);
> >>>
> >
More information about the dev
mailing list