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. https://travis-ci.org/horms2/ovs/builds/491523655 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
