| Message ID | 20200110103601.19381-1-simon.horman@netronome.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] tc: handle packet mark of zero | expand |
Bleep bloop. Greetings Simon Horman, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Simon Horman <simon.horman@netronome.com> Lines checked: 38, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Fri, Jan 10, 2020 at 06:59:00AM -0500, 0-day Robot wrote: > Bleep bloop. Greetings Simon Horman, I am a robot and I have tried out your patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > checkpatch: > WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Simon Horman <simon.horman@netronome.com> > Lines checked: 38, Warnings: 1, Errors: 0 > > > Please check this out. If you feel there has been an error, please email aconole@redhat.com I will add Co-Authored-by: Simon Horman <simon.horman@netronome.com> but if someone offers a positive review then I'll do so when applying. In any case I'll wait before reposting to see if there is any further feedback.
On Fri, Jan 10, 2020 at 7:06 PM Simon Horman <simon.horman@netronome.com> wrote: > > From: John Hurley <john.hurley@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@netronome.com> > [simon; check for exact-match rather than any match] > Signed-off-by: Simon Horman <simon.horman@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)) > err = test_key_and_mask(match); > if (err) { > return err; > -- > 2.20.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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@netronome.com> wrote: > > > > From: John Hurley <john.hurley@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@netronome.com> > > [simon; check for exact-match rather than any match] > > Signed-off-by: Simon Horman <simon.horman@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. > > > err = test_key_and_mask(match); > > if (err) { > > return err; > > -- > > 2.20.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Mon, Jan 13, 2020 at 6:03 PM Simon Horman <simon.horman@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@netronome.com> wrote: > > > > > > From: John Hurley <john.hurley@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@netronome.com> > > > [simon; check for exact-match rather than any match] > > > Signed-off-by: Simon Horman <simon.horman@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. > > > > > err = test_key_and_mask(match); > > > if (err) { > > > return err; > > > -- > > > 2.20.1 > > > > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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@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@netronome.com> wrote: > > > > > > > > From: John Hurley <john.hurley@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@netronome.com> > > > > [simon; check for exact-match rather than any match] > > > > Signed-off-by: Simon Horman <simon.horman@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@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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; + } + err = test_key_and_mask(match); if (err) { return err;