[ovs-dev,v2] tc: handle packet mark of zero
diff mbox series

Message ID 20200116093850.32188-1-simon.horman@netronome.com
State New
Headers show
Series
  • [ovs-dev,v2] tc: handle packet mark of zero
Related show

Commit Message

Simon Horman Jan. 16, 2020, 9:38 a.m. UTC
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>

---
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(+)

Comments

0-day Robot Jan. 16, 2020, 9:59 a.m. UTC | #1
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
Simon Horman Jan. 16, 2020, 10:20 a.m. UTC | #2
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
>
Aaron Conole Jan. 16, 2020, 2:23 p.m. UTC | #3
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
>>
Ben Pfaff Jan. 16, 2020, 7:35 p.m. UTC | #4
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
Aaron Conole Jan. 22, 2020, 8:25 a.m. UTC | #5
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;
Simon Horman Jan. 22, 2020, 1:22 p.m. UTC | #6
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?
Ben Pfaff Jan. 23, 2020, 9:38 p.m. UTC | #7
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.
Aaron Conole Jan. 27, 2020, 12:56 p.m. UTC | #8
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.
Simon Horman Jan. 28, 2020, 8:08 p.m. UTC | #9
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.

Patch
diff mbox series

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;