On 11/02/2019 12:13, Simon Horman wrote: > On Mon, Feb 11, 2019 at 09:57:10AM +0000, Roi Dayan wrote: >> >> >> 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 ? > > Sure, can I confirm that you mean this version from 17 Jan: > > "[PATCH] lib/tc: Support optional tunnel id" > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F1026771%2F&data=02%7C01%7Croid%40mellanox.com%7Ca5fa4a3f7136417620d508d69009845d%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636854767892746889&sdata=dU0B%2BPf3H6a4fHCTQfXFkuyuuFcZoYZFihfcGb9dIMY%3D&reserved=0 >
yes _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
