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

[Ilya Maximets]

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.


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.

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