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

[Ilya Maximets]

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