diff mbox series

[ovs-dev] datapath: Add meter action support.

Message ID 1529454160-88413-1-git-send-email-jpettit@ovn.org
State Accepted
Headers show
Series [ovs-dev] datapath: Add meter action support. | expand

Commit Message

Justin Pettit June 20, 2018, 12:22 a.m. UTC 
From: Andy Zhou <azhou@ovn.org>

Upstream commit:
    commit cd8a6c33693c1b89d2737ffdbf9611564e9ac907
    Author: Andy Zhou <azhou@ovn.org>
    Date:   Fri Nov 10 12:09:43 2017 -0800

    openvswitch: Add meter action support

    Implements OVS kernel meter action support.

    Signed-off-by: Andy Zhou <azhou@ovn.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
 NEWS                                              | 5 +++--
 datapath/actions.c                                | 6 ++++++
 datapath/datapath.h                               | 1 +
 datapath/flow_netlink.c                           | 6 ++++++
 datapath/linux/compat/include/linux/openvswitch.h | 2 +-
 5 files changed, 17 insertions(+), 3 deletions(-)

Comments

0-day Robot June 20, 2018, 3:04 p.m. UTC | #1 
Bleep bloop.  Greetings Justin Pettit, I am a robot and I have tried out your patch
with message ID  <1529454160-88413-1-git-send-email-jpettit@ovn.org>
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
== Checking "0000.patch" ==
ERROR: Too many signoffs; are you missing Co-authored-by lines?
Lines checked: 118, Warnings: 0, Errors: 1


Please check this out.  If you feel there has been an error, please email me back.

Thanks,
0-day Robot
Aaron Conole June 20, 2018, 3:15 p.m. UTC | #2 
0-day Robot <robot@bytheb.org> writes:

> Bleep bloop.  Greetings Justin Pettit, I am a robot and I have tried out your patch
> with message ID  <1529454160-88413-1-git-send-email-jpettit@ovn.org>
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> checkpatch:
> == Checking "0000.patch" ==
> ERROR: Too many signoffs; are you missing Co-authored-by lines?
> Lines checked: 118, Warnings: 0, Errors: 1

I'll fix the sign off check in checkpatch.  It shouldn't flag on these
kinds of patches.

Sorry for the noise.

>
> Please check this out.  If you feel there has been an error, please email me back.
>
> Thanks,
> 0-day Robot
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Justin Pettit June 20, 2018, 3:20 p.m. UTC | #3 
> On Jun 20, 2018, at 8:15 AM, Aaron Conole <aconole@redhat.com> wrote:
> 
> 0-day Robot <robot@bytheb.org> writes:
> 
>> Bleep bloop.  Greetings Justin Pettit, I am a robot and I have tried out your patch
>> with message ID  <1529454160-88413-1-git-send-email-jpettit@ovn.org>
>> Thanks for your contribution.
>> 
>> I encountered some error that I wasn't expecting.  See the details below.
>> 
>> 
>> checkpatch:
>> == Checking "0000.patch" ==
>> ERROR: Too many signoffs; are you missing Co-authored-by lines?
>> Lines checked: 118, Warnings: 0, Errors: 1
> 
> I'll fix the sign off check in checkpatch.  It shouldn't flag on these
> kinds of patches.
> 
> Sorry for the noise.

No problem.  Thanks for setting it up.  I agree that it shouldn't flag on this issue, however, the bigger concern I have is that the warning seems to be separated from the file names, which is a bit confusing.  And, I imagine it would be very confusing if someone had multiple patches flagged, since it wouldn't be clear which error list applied to which patch.

--Justin
Ben Pfaff June 20, 2018, 3:47 p.m. UTC | #4 
On Wed, Jun 20, 2018 at 08:20:19AM -0700, Justin Pettit wrote:
> 
> > On Jun 20, 2018, at 8:15 AM, Aaron Conole <aconole@redhat.com> wrote:
> > 
> > 0-day Robot <robot@bytheb.org> writes:
> > 
> >> Bleep bloop.  Greetings Justin Pettit, I am a robot and I have tried out your patch
> >> with message ID  <1529454160-88413-1-git-send-email-jpettit@ovn.org>
> >> Thanks for your contribution.
> >> 
> >> I encountered some error that I wasn't expecting.  See the details below.
> >> 
> >> 
> >> checkpatch:
> >> == Checking "0000.patch" ==
> >> ERROR: Too many signoffs; are you missing Co-authored-by lines?
> >> Lines checked: 118, Warnings: 0, Errors: 1
> > 
> > I'll fix the sign off check in checkpatch.  It shouldn't flag on these
> > kinds of patches.
> > 
> > Sorry for the noise.
> 
> No problem.  Thanks for setting it up.  I agree that it shouldn't flag
> on this issue, however, the bigger concern I have is that the warning
> seems to be separated from the file names, which is a bit confusing.
> And, I imagine it would be very confusing if someone had multiple
> patches flagged, since it wouldn't be clear which error list applied
> to which patch.

What file names?  Sign-offs aren't associated with files.
Justin Pettit June 20, 2018, 3:51 p.m. UTC | #5 
> On Jun 20, 2018, at 8:47 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
>> On Wed, Jun 20, 2018 at 08:20:19AM -0700, Justin Pettit wrote:
>> 
>>> On Jun 20, 2018, at 8:15 AM, Aaron Conole <aconole@redhat.com> wrote:
>>> 
>>> 0-day Robot <robot@bytheb.org> writes:
>>> 
>>>> Bleep bloop.  Greetings Justin Pettit, I am a robot and I have tried out your patch
>>>> with message ID  <1529454160-88413-1-git-send-email-jpettit@ovn.org>
>>>> Thanks for your contribution.
>>>> 
>>>> I encountered some error that I wasn't expecting.  See the details below.
>>>> 
>>>> 
>>>> checkpatch:
>>>> == Checking "0000.patch" ==
>>>> ERROR: Too many signoffs; are you missing Co-authored-by lines?
>>>> Lines checked: 118, Warnings: 0, Errors: 1
>>> 
>>> I'll fix the sign off check in checkpatch.  It shouldn't flag on these
>>> kinds of patches.
>>> 
>>> Sorry for the noise.
>> 
>> No problem.  Thanks for setting it up.  I agree that it shouldn't flag
>> on this issue, however, the bigger concern I have is that the warning
>> seems to be separated from the file names, which is a bit confusing.
>> And, I imagine it would be very confusing if someone had multiple
>> patches flagged, since it wouldn't be clear which error list applied
>> to which patch.
> 
> What file names?  Sign-offs aren't associated with files.

Sorry, I meant patch name. 

—Justin
Aaron Conole June 20, 2018, 4:12 p.m. UTC | #6 
Justin Pettit <jpettit@ovn.org> writes:

>> On Jun 20, 2018, at 8:47 AM, Ben Pfaff <blp@ovn.org> wrote:
>> 
>>> On Wed, Jun 20, 2018 at 08:20:19AM -0700, Justin Pettit wrote:
>>> 
>>>> On Jun 20, 2018, at 8:15 AM, Aaron Conole <aconole@redhat.com> wrote:
>>>> 
>>>> 0-day Robot <robot@bytheb.org> writes:
>>>> 
>>>>> Bleep bloop.  Greetings Justin Pettit, I am a robot and I have
>>>>> tried out your patch
>>>>> with message ID  <1529454160-88413-1-git-send-email-jpettit@ovn.org>
>>>>> Thanks for your contribution.
>>>>> 
>>>>> I encountered some error that I wasn't expecting.  See the details below.
>>>>> 
>>>>> 
>>>>> checkpatch:
>>>>> == Checking "0000.patch" ==
>>>>> ERROR: Too many signoffs; are you missing Co-authored-by lines?
>>>>> Lines checked: 118, Warnings: 0, Errors: 1
>>>> 
>>>> I'll fix the sign off check in checkpatch.  It shouldn't flag on these
>>>> kinds of patches.
>>>> 
>>>> Sorry for the noise.
>>> 
>>> No problem.  Thanks for setting it up.  I agree that it shouldn't flag
>>> on this issue, however, the bigger concern I have is that the warning
>>> seems to be separated from the file names, which is a bit confusing.
>>> And, I imagine it would be very confusing if someone had multiple
>>> patches flagged, since it wouldn't be clear which error list applied
>>> to which patch.
>> 
>> What file names?  Sign-offs aren't associated with files.
>
> Sorry, I meant patch name. 

I'll work on the job a bit more.  I've made it manually triggered.  I'll
try and fix a few flaws I've found (mostly false positives).

Thanks for not flaming me :)

> —Justin
Gregory Rose June 20, 2018, 4:59 p.m. UTC | #7 
On 6/19/2018 5:22 PM, Justin Pettit wrote:
> From: Andy Zhou <azhou@ovn.org>
>
> Upstream commit:
>      commit cd8a6c33693c1b89d2737ffdbf9611564e9ac907
>      Author: Andy Zhou <azhou@ovn.org>
>      Date:   Fri Nov 10 12:09:43 2017 -0800
>
>      openvswitch: Add meter action support
>
>      Implements OVS kernel meter action support.
>
>      Signed-off-by: Andy Zhou <azhou@ovn.org>
>      Signed-off-by: David S. Miller <davem@davemloft.net>
>
> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> ---
>   NEWS                                              | 5 +++--
>   datapath/actions.c                                | 6 ++++++
>   datapath/datapath.h                               | 1 +
>   datapath/flow_netlink.c                           | 6 ++++++
>   datapath/linux/compat/include/linux/openvswitch.h | 2 +-
>   5 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 7f6589a46878..cd15a332c47e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -19,8 +19,9 @@ Post-v2.9.0
>        * New OpenFlow 1.0 extensions for group support.
>        * Default selection method for select groups is now dp_hash with improved
>          accuracy.
> -   - Linux kernel 4.14
> -     * Add support for compiling OVS with the latest Linux 4.14 kernel
> +   - Linux datapath
> +     * Add support for compiling OVS with the latest Linux 4.14 kernel.
> +     * Added support for meters.
>      - ovn:
>        * implemented icmp4/icmp6/tcp_reset actions in order to drop the packet
>          and reply with a RST for TCP or ICMPv4/ICMPv6 unreachable message for
> diff --git a/datapath/actions.c b/datapath/actions.c
> index eab147617c8b..56b013601393 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -1341,6 +1341,12 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>   		case OVS_ACTION_ATTR_POP_NSH:
>   			err = pop_nsh(skb, key);
>   			break;
> +
> +		case OVS_ACTION_ATTR_METER:
> +			if (ovs_meter_execute(dp, skb, key, nla_get_u32(a))) {
> +				consume_skb(skb);
> +				return 0;
> +			}
>   		}
>   
>   		if (unlikely(err)) {
> diff --git a/datapath/datapath.h b/datapath/datapath.h
> index 93c9ed505448..c38286df75c7 100644
> --- a/datapath/datapath.h
> +++ b/datapath/datapath.h
> @@ -31,6 +31,7 @@
>   #include "compat.h"
>   #include "flow.h"
>   #include "flow_table.h"
> +#include "meter.h"
>   #include "vport-internal_dev.h"
>   
>   #define DP_MAX_PORTS           USHRT_MAX
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index 1b7bad8fe2ab..bea525a5dfcb 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -92,6 +92,7 @@ static bool actions_may_change_flow(const struct nlattr *actions)
>   		case OVS_ACTION_ATTR_SAMPLE:
>   		case OVS_ACTION_ATTR_SET:
>   		case OVS_ACTION_ATTR_SET_MASKED:
> +		case OVS_ACTION_ATTR_METER:
>   		default:
>   			return true;
>   		}
> @@ -2853,6 +2854,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>   			[OVS_ACTION_ATTR_POP_ETH] = 0,
>   			[OVS_ACTION_ATTR_PUSH_NSH] = (u32)-1,
>   			[OVS_ACTION_ATTR_POP_NSH] = 0,
> +			[OVS_ACTION_ATTR_METER] = sizeof(u32),
>   		};
>   		const struct ovs_action_push_vlan *vlan;
>   		int type = nla_type(a);
> @@ -3038,6 +3040,10 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
>   			break;
>   		}
>   
> +		case OVS_ACTION_ATTR_METER:
> +			/* Non-existent meters are simply ignored.  */
> +			break;
> +
>   		default:
>   			OVS_NLERR(log, "Unknown Action type %d", type);
>   			return -EINVAL;
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
> index 5c1e2383f4f9..8e5f3b6fbfb1 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -934,12 +934,12 @@ enum ovs_action_attr {
>   	OVS_ACTION_ATTR_CT_CLEAR,     /* No argument. */
>   	OVS_ACTION_ATTR_PUSH_NSH,     /* Nested OVS_NSH_KEY_ATTR_*. */
>   	OVS_ACTION_ATTR_POP_NSH,      /* No argument. */
> +	OVS_ACTION_ATTR_METER,         /* u32 meter number. */
>   
>   #ifndef __KERNEL__
>   	OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
>   	OVS_ACTION_ATTR_TUNNEL_POP,    /* u32 port number. */
>   	OVS_ACTION_ATTR_CLONE,         /* Nested OVS_CLONE_ATTR_*.  */
> -	OVS_ACTION_ATTR_METER,         /* u32 meter number. */
>   #endif
>   	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
>   				       * from userspace. */

LGTM

Reviewed-by: Greg Rose <gvrose8192@gmail.com>
Tested-by: Greg Rose <gvrose8192@gmail.com>
Justin Pettit June 20, 2018, 5:18 p.m. UTC | #8 
> On Jun 20, 2018, at 9:59 AM, Gregory Rose <gvrose8192@gmail.com> wrote:
> 
> On 6/19/2018 5:22 PM, Justin Pettit wrote:
>> From: Andy Zhou <azhou@ovn.org>
>> 
>> Upstream commit:
>>     commit cd8a6c33693c1b89d2737ffdbf9611564e9ac907
>>     Author: Andy Zhou <azhou@ovn.org>
>>     Date:   Fri Nov 10 12:09:43 2017 -0800
>> 
>>     openvswitch: Add meter action support
>> 
>>     Implements OVS kernel meter action support.
>> 
>>     Signed-off-by: Andy Zhou <azhou@ovn.org>
>>     Signed-off-by: David S. Miller <davem@davemloft.net>
>> 
>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> 
> LGTM
> 
> Reviewed-by: Greg Rose <gvrose8192@gmail.com>
> Tested-by: Greg Rose <gvrose8192@gmail.com>

Thanks.  I pushed this to master.

--Justin
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 7f6589a46878..cd15a332c47e 100644
--- a/NEWS
+++ b/NEWS
@@ -19,8 +19,9 @@  Post-v2.9.0
      * New OpenFlow 1.0 extensions for group support.
      * Default selection method for select groups is now dp_hash with improved
        accuracy.
-   - Linux kernel 4.14
-     * Add support for compiling OVS with the latest Linux 4.14 kernel
+   - Linux datapath
+     * Add support for compiling OVS with the latest Linux 4.14 kernel.
+     * Added support for meters.
    - ovn:
      * implemented icmp4/icmp6/tcp_reset actions in order to drop the packet
        and reply with a RST for TCP or ICMPv4/ICMPv6 unreachable message for
diff --git a/datapath/actions.c b/datapath/actions.c
index eab147617c8b..56b013601393 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -1341,6 +1341,12 @@  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 		case OVS_ACTION_ATTR_POP_NSH:
 			err = pop_nsh(skb, key);
 			break;
+
+		case OVS_ACTION_ATTR_METER:
+			if (ovs_meter_execute(dp, skb, key, nla_get_u32(a))) {
+				consume_skb(skb);
+				return 0;
+			}
 		}
 
 		if (unlikely(err)) {
diff --git a/datapath/datapath.h b/datapath/datapath.h
index 93c9ed505448..c38286df75c7 100644
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -31,6 +31,7 @@ 
 #include "compat.h"
 #include "flow.h"
 #include "flow_table.h"
+#include "meter.h"
 #include "vport-internal_dev.h"
 
 #define DP_MAX_PORTS           USHRT_MAX
diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index 1b7bad8fe2ab..bea525a5dfcb 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -92,6 +92,7 @@  static bool actions_may_change_flow(const struct nlattr *actions)
 		case OVS_ACTION_ATTR_SAMPLE:
 		case OVS_ACTION_ATTR_SET:
 		case OVS_ACTION_ATTR_SET_MASKED:
+		case OVS_ACTION_ATTR_METER:
 		default:
 			return true;
 		}
@@ -2853,6 +2854,7 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			[OVS_ACTION_ATTR_POP_ETH] = 0,
 			[OVS_ACTION_ATTR_PUSH_NSH] = (u32)-1,
 			[OVS_ACTION_ATTR_POP_NSH] = 0,
+			[OVS_ACTION_ATTR_METER] = sizeof(u32),
 		};
 		const struct ovs_action_push_vlan *vlan;
 		int type = nla_type(a);
@@ -3038,6 +3040,10 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			break;
 		}
 
+		case OVS_ACTION_ATTR_METER:
+			/* Non-existent meters are simply ignored.  */
+			break;
+
 		default:
 			OVS_NLERR(log, "Unknown Action type %d", type);
 			return -EINVAL;
diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
index 5c1e2383f4f9..8e5f3b6fbfb1 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -934,12 +934,12 @@  enum ovs_action_attr {
 	OVS_ACTION_ATTR_CT_CLEAR,     /* No argument. */
 	OVS_ACTION_ATTR_PUSH_NSH,     /* Nested OVS_NSH_KEY_ATTR_*. */
 	OVS_ACTION_ATTR_POP_NSH,      /* No argument. */
+	OVS_ACTION_ATTR_METER,         /* u32 meter number. */
 
 #ifndef __KERNEL__
 	OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
 	OVS_ACTION_ATTR_TUNNEL_POP,    /* u32 port number. */
 	OVS_ACTION_ATTR_CLONE,         /* Nested OVS_CLONE_ATTR_*.  */
-	OVS_ACTION_ATTR_METER,         /* u32 meter number. */
 #endif
 	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
 				       * from userspace. */