[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 16:38:49 UTC 2020
On 8/17/20 4:49 PM, Finn, Emma wrote:
>
>
>> -----Original Message-----
>> From: Finn, Emma
>> Sent: Monday 17 August 2020 13:50
>> To: Eli Britstein <elibr at nvidia.com>; Ilya Maximets <i.maximets at ovn.org>;
>> Stokes, Ian <ian.stokes at intel.com>; dev at openvswitch.org
>> Subject: RE: [PATCH v3 1/1] netdev-offload-dpdk: Fix for broken ethernet
>> matching HWOL for XL710NIC.
>>
>>
>>
>>> -----Original Message-----
>>> From: Eli Britstein <elibr at nvidia.com>
>>> Sent: Monday 17 August 2020 12:50
>>> To: Ilya Maximets <i.maximets at ovn.org>; Stokes, Ian
>>> <ian.stokes at intel.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.
>>>
>>>
>>>
>>>> -----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.
>>
>> Yes I can test
>>
> Tested and everything looks good.
>
> 2020-08-17T14:46:41.179Z|00006|netdev_offload_dpdk(dp_netdev_flow_48)|DBG|dpdk0: installed flow 0x23fd71ac0 by ufid c3ba1a2f-e7e5-473b-b303-38ecbd28cc03
Thanks!
I'll apply this version as soon as travis builds finished.
>
>>>>
>>>>>
>>>>> @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