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

[Simon Horman]

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.