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

Ilya Maximets i.maximets at ovn.org
Mon Aug 17 10:53:50 UTC 2020


On 8/17/20 12:39 PM, Eli Britstein 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?

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