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://patchwork.ozlabs.org/patch/1026771/ _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
