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

Roi Dayan roi.dayan at gmail.com
Mon Feb 4 13:20:41 UTC 2019


On Mon, Feb 4, 2019 at 11:00 AM Simon Horman <simon.horman at netronome.com> wrote:
>
>
>
> On Sun, 3 Feb 2019 at 12:03, Roi Dayan <roi.dayan at gmail.com> wrote:
>>
>> On Fri, Feb 1, 2019 at 4:05 PM Simon Horman <simon.horman at netronome.com> wrote:
>> >
>> > Thanks Roi,
>> >
>> > On Thu, 31 Jan 2019 at 15:52, Roi Dayan <roid at mellanox.com> wrote:
>> >
>> > >
>> > >
>> > > On 31/01/2019 15:32, Roi Dayan wrote:
>> > > >
>> > > > On 31/01/2019 11:58, Simon Horman wrote:
>> > > >> On Mon, Jan 21, 2019 at 05:32:37PM +0200, Adi Nissim wrote:
>> > > >>> Currently the TC tunnel_key action is always
>> > > >>> initialized with the given tunnel id value. However,
>> > > >>> some tunneling protocols define the tunnel id as an optional field.
>> > > >>>
>> > > >>> This patch initializes the id field of tunnel_key:set and
>> > > tunnel_key:unset
>> > > >>> only if a value is provided.
>> > > >>>
>> > > >>> In the case that a tunnel key value is not provided by the user
>> > > >>> the key flag will not be set.
>> > > >>>
>> > > >>> Signed-off-by: Adi Nissim <adin at mellanox.com>
>> > > >>> Acked-by: Paul Blakey <paulb at mellanox.com>
>> > > >>> ---
>> > > >>> v1->v2: check if mask.tunnel.id == OVS_BE64_MAX
>> > > >>>         so we won't do match in the case of a partial mask.
>> > > >> I am still a bit concerned about the partial mask case.
>> > > >> It looks like the code will now silently not offload such matches.
>> > > >>
>> > > >> I think that a partial mask should either be offloaded or
>> > > >> offload of the entire flow should be rejected.
>> > > > thanks. you are right. I didn't notice it. partial masks should be
>> > > rejected
>> > > > to fallback to ovs dp instead of ignoring the mask.
>> > > >
>> > >
>> > >
>> > > 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

>> >
>> > >
>> > >
>> > > in tc.c
>> > >
>> > > -    nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
>> > > +    if (id_mask) {
>> > > +        nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
>> > > +    }
>> > >
>> > >
>> > > let me know what you think.
>> > >
>> > > Thanks,
>> > > Roi
>> > >
>> > _______________________________________________
>> > dev mailing list
>> > dev at openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list