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

[Simon Horman]

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/