[ovs-dev] [PATCH 1/1] netdev-offload-dpdk: Support vxlan encap offload with load actions

Ilya Maximets i.maximets at ovn.org
Fri Oct 16 09:43:30 UTC 2020


On 10/16/20 11:38 AM, Ilya Maximets wrote:
> On 10/16/20 7:57 AM, Eli Britstein wrote:
>>
>> On 10/16/2020 3:02 AM, Ilya Maximets wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 10/12/20 5:25 PM, Sriharsha Basavapatna via dev wrote:
>>>> On Mon, Oct 12, 2020 at 4:48 PM Eli Britstein <elibr at nvidia.com> wrote:
>>>>>
>>>>> On 10/12/2020 10:20 AM, Sriharsha Basavapatna wrote:
>>>>>> On Sun, Sep 6, 2020 at 5:51 PM Eli Britstein <elibr at nvidia.com> wrote:
>>>>>>> ping
>>>>>>>
>>>>>>> On 7/30/2020 1:58 PM, Eli Britstein wrote:
>>>>>>>> From: Lei Wang <leiw at mellanox.com>
>>>>>>>>
>>>>>>>> Struct match has the tunnel values/masks in
>>>>>>>> match->flow.tunnel/match->wc.masks.tunnel.
>>>>>>>> Load actions such as load:0xa566c10->NXM_NX_TUN_IPV4_DST[],
>>>>>>>> load:0xbba->NXM_NX_TUN_ID[] are utilizing the tunnel masks fields,
>>>>>>>> but those should not be used for matching.
>>>>>>>> Offloading fails if masks is not clear. Clear it if no tunnel used.
>>>>>>>>
>>>>>>>> Signed-off-by: Lei Wang <leiw at mellanox.com>
>>>>>>>> Reviewed-by: Eli Britstein <elibr at mellanox.com>
>>>>>>>> Reviewed-by: Gaetan Rivet <gaetanr at mellanox.com>
>>>>>>>> Signed-off-by: Eli Britstein <elibr at mellanox.com>
>>>> Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna at broadcom.com>
>>>>
>>>> See my comment below.
>>>>
>>>>>>>> ---
>>>>>>>>     lib/netdev-offload-dpdk.c | 4 ++++
>>>>>>>>     1 file changed, 4 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>>>>>>> index de6101e4d..0d23e4879 100644
>>>>>>>> --- a/lib/netdev-offload-dpdk.c
>>>>>>>> +++ b/lib/netdev-offload-dpdk.c
>>>>>>>> @@ -682,6 +682,10 @@ parse_flow_match(struct flow_patterns *patterns,
>>>>>>>>
>>>>>>>>         consumed_masks = &match->wc.masks;
>>>>>>>>
>>>>>>>> +    if (!flow_tnl_dst_is_set(&match->flow.tunnel)) {
>>>>>>>> +        memset(&match->wc.masks.tunnel, 0, sizeof match->wc.masks.tunnel);
>>> Small nit:  For consistency, I think, we should access via 'consumed_masks'
>>> pointer, not directly.  What do you think?
>>> I could change that on commit.
>> OK.
>>>
>>>
>>> Another point is that we could actually consider this as a bug fix, but
>>> I'm not sure which commit to use for a 'Fixes' tag.  Suggestions?
>>> Technically, the issue exists on all branches down to 2.10, but 2.13 and
>>> lower will require a bit different fix due t major rework of this code.
>>
>> I see the original "validate" code also has it, so maybe e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte flow")
>>
>> Do you want me to send v2 with this fixes tag and usage of consumed_masks (above), or you can do it when applying?
> 
> I can do that.
> I will be good if you can prepare fix for branch-2.13, i.e. for the

*it

> original "validate" code.
> 
>>
>> Thanks, Eli
>>
>>>
>>>>>>>> +    }
>>>>>>>> +
>>>>>> This fix looks good to me.  Did you take a look to see if this can be
>>>>>> fixed in the code that generates these invalid masks in the first
>>>>>> place ?
>>>>> I wouldn't say it's "invalid masks". OVS takes those masks into
>>>>> consideration with such usage of flow_tnl_dst_is_set, that is done in
>>>>> several places in the code. For example odp-util.c, lib/netdev-offload-tc.c.
>>>> It is invalid because the datapath rule is specifying tunnel masks
>>>> when we are not really matching on them. In the context of encap
>>>> actions (which is what this patch is fixing), tunnel masks shouldn't
>>>> be set.
>>> It seems like tunnel metadata and a couple of other fields are special
>>> cases that allowed to have masks set without having keys.  Datapaths
>>> handles these cases.  This was done for some reason, but it hard to
>>> track down why exactly.  Following commit made higher layers to always
>>> provide tunnel masks even if tunnel keys are not specified:
>>> 01fcdfc6b322 ("odp-util: Always serialize tunnel mask attributes.")
>>> It looks like it was intended to simplify processing, but I'm not sure.
>>> Anyway, datapath should not look at these masks if tnl_dst is not
>>> specified in keys as this is a sign of a correct tunnel metadata.  So,
>>> the fix looks correct.
>>>
>>>>>> Thanks,
>>>>>> -Harsha
>>>>>>>>         memset(&consumed_masks->in_port, 0, sizeof consumed_masks->in_port);
>>>>>>>>         /* recirc id must be zero. */
>>>>>>>>         if (match->wc.masks.recirc_id & match->flow.recirc_id) {
> 



More information about the dev mailing list