[ovs-dev] [PATCH v2] lib/tc: Support optional tunnel id

[Roi Dayan]

On 11/02/2019 11:11, Simon Horman wrote:
> On Sun, Feb 10, 2019 at 08:04:39AM +0000, Roi Dayan wrote:
>>
>>
>> On 04/02/2019 15:20, Roi Dayan wrote:
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>> I did some checks and think the correct fix is to offload exact match.
>>>>>>> if key is partial we can ignore the mask and offload exact match and
>>>>>>> it will be correct as we do more strict matching.
>>>>>>>
>>>>>>> it is also documented that the kernel datapath is doing the same
>>>>>>> (from datapath.rst)
>>>>>>>
>>>>>>> "The kernel can ignore the mask attribute, installing an exact
>>>>>>> match flow"
>>>>>>>
>>>>>>> So I think the first patch V0 is actually correct as we
>>>>>>> check the tunnel key flag exists and offload exact match if
>>>>>>> there was any mask or offload without a key if the mask is 0
>>>>>>> or no key.
>>>>>>>
>>>>>>> in netdev-tc-offloads.c
>>>>>>>
>>>>>>> +        flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ?
>>>>>>> +                                 tnl_mask->tun_id : 0;
>>>>>>>
>>>>>> I think this is fine so long as tun_id is all-ones. Is that always the case?
>>>>>> Should the code check that it is the case? Am I missing the point?
>>>>>>
>>>>> it looks like tun_id mask is always set to all-ones.
>>>>> but even if it won't be in the future, we shouldn't really care here.
>>>>> tc adds exact match on the tun_id and ignores the tun_id mask.
>>>>> this is considered ok as the matching is more strict.
>>>>> if new match is needed with different tun_id then ovs will try to add
>>>>> another rule for it.
>>>>> so with tc we could have multiple rules vs 1 rule that support mask.
>>>>
>>>> Thanks for looking into this. That sounds find to me but I wonder if we should make
>>>> this behaviour explicit.
>>>>
>>>>         /*
>>>>          * Comment describing why the mask is 0 or all-ones
>>>>          */
>>>>         flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX : 0;
>>>>
>>> I think its nicer like this and symetric currently. here it's generic
>>> and "use" mask.
>>> in tc.c when we fill the netlink msg we ignore the mas and also when
>>> parsing tc dump,
>>> tun mask is set to FFFF here (tc.c) and not netdev-tc-offloads.c
>>>
>>
>> Hi Simon,
>>
>> any update? i remind i suggested v0 is the correct one.
> 
> Thanks and sorry for the ongoing delays.
> 
> I have added this (v2) to a travisci instance and plan to apply the patch
> to master if that passes.

Hi Simon,

I agreed with you v2 has an issue and we should use v0.
v2 will not pass id to tc if there is an id with partial mask which is incorrect.
v0 will add rule with exact match.
can we go with v0 ?

Thanks,
Roi


> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fhorms2%2Fovs%2Fbuilds%2F491523655&data=02%7C01%7Croid%40mellanox.com%7C8eae4ff8d37947fcd5c208d69000eb6e%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636854730964289416&sdata=iWAaOzcJf%2B5L4ahKeHb2Hcfxp0LpNOQfto%2BMaohkskk%3D&reserved=0
>