[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