| Message ID | 20200116093850.32188-1-simon.horman@netronome.com |
|---|---|
| State | Accepted |
| Commit | 56c8027b5fd830a810f320a6ded6e8f8289e4fe6 |
| Headers | show |
| Series | [ovs-dev,v2] 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: 44, 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 Thu, Jan 16, 2020 at 04:59:54AM -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: 44, Warnings: 1, Errors: 0 Is the correct tag Co-Authored-by rather than Co-Authored as used in this patch? > > > Please check this out. If you feel there has been an error, please email aconole@redhat.com > > Thanks, > 0-day Robot > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Simon Horman <simon.horman@netronome.com> writes: > On Thu, Jan 16, 2020 at 04:59:54AM -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: 44, Warnings: 1, Errors: 0 > > Is the correct tag Co-Authored-by rather than Co-Authored as used in > this patch? Yes. Co-authored-by Not sure if we should make the tags a bit more lenient, but that's a separate discussion. :) >> >> >> Please check this out. If you feel there has been an error, please email aconole@redhat.com >> >> Thanks, >> 0-day Robot >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On Thu, Jan 16, 2020 at 11:20:20AM +0100, Simon Horman wrote: > On Thu, Jan 16, 2020 at 04:59:54AM -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: 44, Warnings: 1, Errors: 0 > > Is the correct tag Co-Authored-by rather than Co-Authored as used in > this patch? Yes, it should be Co-authored-by. See Documentation/internals/contributing/submitting-patches.rst
Simon Horman <simon.horman@netronome.com> writes: > 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. > > Only the exact-match case is covered by this patch as it addresses the > Openstack use-case and seems most robust against feature evolution: f.e. in > future there may exist hardware offload scenarios where an operation, such > as a BPF offload, sets the SKB mark before proceeding tho the in-HW OVS. > datapath. > > Signed-off-by: John Hurley <john.hurley@netronome.com> > Co-Authored: Simon Horman <simon.horman@netronome.com> > Signed-off-by: Simon Horman <simon.horman@netronome.com> > > --- With the change to the Co-Authored tag: Acked-by: Aaron Conole <aconole@redhat.com> > v0 [John Hurley] > > v1 [Simon Horman] > * Check for exact rather than masked match on skb 0 > > v2 [Simon Horman] > * Add co-authored tag > Add explanation of check against exact-match to changelog > --- > 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 4988dadc3f50..5781d163e276 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;
On Wed, Jan 22, 2020 at 03:25:06AM -0500, Aaron Conole wrote: > Simon Horman <simon.horman@netronome.com> writes: > > > 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. > > > > Only the exact-match case is covered by this patch as it addresses the > > Openstack use-case and seems most robust against feature evolution: f.e. in > > future there may exist hardware offload scenarios where an operation, such > > as a BPF offload, sets the SKB mark before proceeding tho the in-HW OVS. > > datapath. > > > > Signed-off-by: John Hurley <john.hurley@netronome.com> > > Co-Authored: Simon Horman <simon.horman@netronome.com> > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > > > > --- > > With the change to the Co-Authored tag: > > Acked-by: Aaron Conole <aconole@redhat.com> Thanks, pushed to master. My feeling is that this is a fix and appropriate for backporting. Do you have any thoughts on that?
On Wed, Jan 22, 2020 at 02:22:09PM +0100, Simon Horman wrote: > On Wed, Jan 22, 2020 at 03:25:06AM -0500, Aaron Conole wrote: > > Simon Horman <simon.horman@netronome.com> writes: > > > > > 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. > > > > > > Only the exact-match case is covered by this patch as it addresses the > > > Openstack use-case and seems most robust against feature evolution: f.e. in > > > future there may exist hardware offload scenarios where an operation, such > > > as a BPF offload, sets the SKB mark before proceeding tho the in-HW OVS. > > > datapath. > > > > > > Signed-off-by: John Hurley <john.hurley@netronome.com> > > > Co-Authored: Simon Horman <simon.horman@netronome.com> > > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > > > > > > --- > > > > With the change to the Co-Authored tag: > > > > Acked-by: Aaron Conole <aconole@redhat.com> > > Thanks, pushed to master. > > My feeling is that this is a fix and appropriate for backporting. > Do you have any thoughts on that? Seems fine to me.
Simon Horman <simon.horman@netronome.com> writes: > On Wed, Jan 22, 2020 at 03:25:06AM -0500, Aaron Conole wrote: >> Simon Horman <simon.horman@netronome.com> writes: >> >> > 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. >> > >> > Only the exact-match case is covered by this patch as it addresses the >> > Openstack use-case and seems most robust against feature evolution: f.e. in >> > future there may exist hardware offload scenarios where an operation, such >> > as a BPF offload, sets the SKB mark before proceeding tho the in-HW OVS. >> > datapath. >> > >> > Signed-off-by: John Hurley <john.hurley@netronome.com> >> > Co-Authored: Simon Horman <simon.horman@netronome.com> >> > Signed-off-by: Simon Horman <simon.horman@netronome.com> >> > >> > --- >> >> With the change to the Co-Authored tag: >> >> Acked-by: Aaron Conole <aconole@redhat.com> > > Thanks, pushed to master. > > My feeling is that this is a fix and appropriate for backporting. > Do you have any thoughts on that? Sorry for the late response - I was traveling. Yes, please do.
On Mon, Jan 27, 2020 at 07:56:26AM -0500, Aaron Conole wrote: > Simon Horman <simon.horman@netronome.com> writes: > > > On Wed, Jan 22, 2020 at 03:25:06AM -0500, Aaron Conole wrote: > >> Simon Horman <simon.horman@netronome.com> writes: > >> > >> > 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. > >> > > >> > Only the exact-match case is covered by this patch as it addresses the > >> > Openstack use-case and seems most robust against feature evolution: f.e. in > >> > future there may exist hardware offload scenarios where an operation, such > >> > as a BPF offload, sets the SKB mark before proceeding tho the in-HW OVS. > >> > datapath. > >> > > >> > Signed-off-by: John Hurley <john.hurley@netronome.com> > >> > Co-Authored: Simon Horman <simon.horman@netronome.com> > >> > Signed-off-by: Simon Horman <simon.horman@netronome.com> > >> > > >> > --- > >> > >> With the change to the Co-Authored tag: > >> > >> Acked-by: Aaron Conole <aconole@redhat.com> > > > > Thanks, pushed to master. > > > > My feeling is that this is a fix and appropriate for backporting. > > Do you have any thoughts on that? > > Sorry for the late response - I was traveling. Yes, please do. Thanks, pushed back to branch-2.8.
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 4988dadc3f50..5781d163e276 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;