[ovs-dev] [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken ethernet matching HWOL for XL710NIC.

Eli Britstein elibr at nvidia.com
Mon Aug 17 11:49:30 UTC 2020



>-----Original Message-----
>From: Ilya Maximets <i.maximets at ovn.org>
>Sent: Monday, August 17, 2020 2:41 PM
>To: Stokes, Ian <ian.stokes at intel.com>; Ilya Maximets <i.maximets at ovn.org>;
>Eli Britstein <elibr at nvidia.com>; dev at openvswitch.org; Finn, Emma
><emma.finn at intel.com>
>Subject: Re: [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken ethernet
>matching HWOL for XL710NIC.
>
>
>On 8/17/20 1:30 PM, Stokes, Ian wrote:
>>>>> -----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.
>
>I don't think we should change the logic of a workaround itself at this point.
>But it will be good to have above diff validated.  To be sure that I didn't break
>anything accidentially.
Either way looks OK to me.
>
>>
>> @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