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

[Simon Horman]

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?


>
>
> 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
>