On Sun, 3 Feb 2019 at 12:03, Roi Dayan <[email protected]> wrote:
> On Fri, Feb 1, 2019 at 4:05 PM Simon Horman <[email protected]>
> wrote:
> >
> > Thanks Roi,
> >
> > On Thu, 31 Jan 2019 at 15:52, Roi Dayan <[email protected]> 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 <[email protected]>
> > > >>> Acked-by: Paul Blakey <[email protected]>
> > > >>> ---
> > > >>> 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;
>
> > >
> > >
> > > 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
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
