On Mon, Feb 11, 2019 at 11:25:28AM +0000, Roi Dayan wrote: > > > 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
Thanks, applied to master. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev