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

[Eli Britstein]

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?

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) {