Message ID | 1529454160-88413-1-git-send-email-jpettit@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev] datapath: Add meter action support. | expand |
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
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
> 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
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.
> 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
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
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>
> 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 --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. */