On 8/17/20 1:30 PM, Stokes, Ian wrote:
>>>> -----Original Message-----
>>>> From: Ilya Maximets <[email protected]>
>>>> Sent: Monday, August 17, 2020 12:59 PM
>>>> To: Ian Stokes <[email protected]>; [email protected]
>>>> Cc: Eli Britstein <[email protected]>; [email protected];
>>>> [email protected]; [email protected]
>>>> 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 <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Signed-off-by: Eli Britstein <[email protected]>
>>>>> Co-authored-by: Eli Britstein <[email protected]>
>>>>> Tested-by: Ian Stokes <[email protected]>
>>>>> ---
>>>>
>>>> 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.
>
> @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);
>>>>>
>>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
