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