[ovs-dev] [PATCH] tc: handle packet mark of zero

Simon Horman simon.horman at netronome.com
Tue Jan 14 09:25:44 UTC 2020


On Mon, Jan 13, 2020 at 06:50:50PM +0800, Tonghao Zhang wrote:
> On Mon, Jan 13, 2020 at 6:03 PM Simon Horman <simon.horman at netronome.com> wrote:
> >
> > Hi,
> >
> > On Mon, Jan 13, 2020 at 05:28:04PM +0800, Tonghao Zhang wrote:
> > > On Fri, Jan 10, 2020 at 7:06 PM Simon Horman <simon.horman at netronome.com> wrote:
> > > >
> > > > From: John Hurley <john.hurley at netronome.com>
> > > >
> > > > Openstack may set an skb mark of 0 in tunnel rules. This is considered to
> > > > be an unused/unset value. However, it prevents the rule from being
> > > > offloaded.
> > > >
> > > > Check if the key value of the skb mark is 0 when it is in use (mask is
> > > > set to all ones). If it is then ignore the field and continue with TC offload.
> > > >
> > > > Signed-off-by: John Hurley <john.hurley at netronome.com>
> > > > [simon; check for exact-match rather than any match]
> > > > Signed-off-by: Simon Horman <simon.horman at netronome.com>
> > > > ---
> > > >  lib/netdev-offload-tc.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > > > index 7453078d535f..daff8a379e97 100644
> > > > --- a/lib/netdev-offload-tc.c
> > > > +++ b/lib/netdev-offload-tc.c
> > > > @@ -1619,6 +1619,11 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
> > > >          mask->ct_label = OVS_U128_ZERO;
> > > >      }
> > > >
> > > > +    /* ignore exact match on skb_mark of 0. */
> > > > +    if (mask->pkt_mark == UINT32_MAX && !key->pkt_mark) {
> > > > +        mask->pkt_mark = 0;
> > > > +    }
> > > Why not: if (!(mask->pkt_mark & key->pkt_mark))
> >
> > Its not clear to me that only returns true in the case
> > where there is an exact match on pkt_mark 0. Which is the case
> > that is considered to be an unused/unset value from a HW offload
> > perspective.
> if mask->pkt_mark & key->pkt_mark == 0, the HW offload will return error ?
> if so, there is a case that key->pkt_mark != 0, but mask->pkt_mark &
> key->pkt_mark == 0.

I think there are some subtle issues here.

As it stands HW offload support in the Linux kernel does not support
matching pkt_mark. So there is actually no way to express such a match
and request the Kernel that it be offloaded. Instead, ovs-vswitchd
rejects such flows.

The intention of this patch is to add an exception, whereby a match on
pkt_mark 0 is considered to be the same as no match on the pkt_mark.
The reasoning is that on the wire pkt_mark does not exist. And that when
a packet arrives the network stack sets a default value of 0. So in
a sense packets on the wire have this default value. As do those in
a datapath in a NIC which is not pkt_mark aware.

So we can think of the pkt_mark in the HW datapath as being 0.
And I believe your suggestion that as 0 anded with any mask is 0
then we can do so with any mask. I agree to some extent but I am concerned
about forwards compatibility.

In theory a HW datapath may become pkt_mark aware. For example a BPF
program might run in the HW before the OVS datapath and the BPF program
may set the pkt_mark to a non-zero value. I believe that in this
case the change you are suggesting would lead to incorrect behaviour.

So I think that it is best to keep a very restrictive test for this
exception to avoid problems in future.

> > > >      err = test_key_and_mask(match);
> > > >      if (err) {
> > > >          return err;
> > > > --
> > > > 2.20.1
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev at openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 


More information about the dev mailing list