Re: [ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.
On Tue, Aug 7, 2018 at 2:03 AM, Justin Pettit wrote:
>
>
> > On Aug 6, 2018, at 1:27 PM, Han Zhou wrote:
> >
> > Thanks Justin for the great work!!
> > Sorry that I didn't get time to review the series, just some quick
questions regarding the kernel bug you mentioned.
>
> Yes, I think you were on vacation, and I was running up against my own,
so it all went in pretty quickly. I'm sorry I cut it so close to the 2.10
release date and couldn't have gotten some initial testing from you and
your team.
>
> > - What's the impact of having meter ID = 0? Does it mean the meters and
ACL rate limit feature can't be used at all, or is it just some limitations
in certain scenarios?
>
> In theory, it would mean that a single meter could be put in the kernel
(there is a layer of indirection in the mapping between the OpenFlow meter
id and the one chosen for the datapath). However, my current plan is to
add a probe to OVS which just disables meters on those kernels with broken
meters, since otherwise it will just lead to confusion. I suspect the
issue will be with kernels 4.15, 4.16, and 4.17. (And hopefully some of
those kernels will be fixed as they do bug fix releases.)
>
> > - For the bug fix, can we get it reviewed (and probably merged) in
datapath/compact first here in the OVS community without having to wait for
kernel upstream? What's the usual practice for kernel module patches?
>
> The patch was committed without comment from David Miller:
>
>
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=25432eba9
>
> The backport patch is ready (I've appended it to the end of this
message), but I've been on vacation and I wanted to get the probe patch in
at the same time. I plan to send both patches out Tuesday night. However,
if you want to apply the patch at the bottom and give ACL rate-limiting a
try, I'd love to get some initial feedback.
>
> Thanks,
>
> --Justin
>
>
> -=-=-=-=-=-=-=-=-=-
>
> commit bc89eebb0e918d2e9a903d7e4a24ab1b5b522eab (HEAD -> meter-datapath)
> Author: Justin Pettit
> Date: Thu Jul 26 22:28:11 2018 -0700
>
> datapath: Fix setting meter id for new entries.
>
> Upstream commit:
>
> openvswitch: meter: Fix setting meter id for new entries
>
> The meter code would create an entry for each new meter.
However, it
> would not set the meter id in the new entry, so every meter would
appear
> to have a meter id of zero. This commit properly sets the meter
id when
> adding the entry.
>
> Fixes: 96fbc13d7e77 ("openvswitch: Add meter infrastructure")
> Signed-off-by: Justin Pettit
> Cc: Andy Zhou
> Signed-off-by: David S. Miller
>
> Signed-off-by: Justin Pettit
>
> diff --git a/datapath/meter.c b/datapath/meter.c
> index 1c2ed4626c2a..281d86937555 100644
> --- a/datapath/meter.c
> +++ b/datapath/meter.c
> @@ -221,6 +221,7 @@ static struct dp_meter *dp_meter_create(struct nlattr
**a)
> if (!meter)
> return ERR_PTR(-ENOMEM);
>
> + meter->id = nla_get_u32(a[OVS_METER_ATTR_ID]);
> meter->used = div_u64(ktime_get_ns(), 1000 * 1000);
> meter->kbps = a[OVS_METER_ATTR_KBPS] ? 1 : 0;
> meter->keep_stats = !a[OVS_METER_ATTR_CLEAR];
> @@ -290,6 +291,10 @@ static int ovs_meter_cmd_set(struct sk_buff *skb,
struct genl_info *info)
> u32 meter_id;
> bool failed;
>
> + if (!a[OVS_METER_ATTR_ID]) {
> + return -ENODEV;
> + }
> +
> meter = dp_meter_create(a);
> if (IS_ERR_OR_NULL(meter))
> return PTR_ERR(meter);
> @@ -308,11 +313,6 @@ static int ovs_meter_cmd_set(struct sk_buff *skb,
struct genl_info *info)
> goto exit_unlock;
> }
>
> - if (!a[OVS_METER_ATTR_ID]) {
> - err = -ENODEV;
> - goto exit_unlock;
> - }
> -
> meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]);
>
> /* Cannot fail after this. */
>
>
>
>
Thanks Justin! This is very nice and we will try it.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.
> On Aug 6, 2018, at 1:27 PM, Han Zhou wrote:
>
> Thanks Justin for the great work!!
> Sorry that I didn't get time to review the series, just some quick questions
> regarding the kernel bug you mentioned.
Yes, I think you were on vacation, and I was running up against my own, so it
all went in pretty quickly. I'm sorry I cut it so close to the 2.10 release
date and couldn't have gotten some initial testing from you and your team.
> - What's the impact of having meter ID = 0? Does it mean the meters and ACL
> rate limit feature can't be used at all, or is it just some limitations in
> certain scenarios?
In theory, it would mean that a single meter could be put in the kernel (there
is a layer of indirection in the mapping between the OpenFlow meter id and the
one chosen for the datapath). However, my current plan is to add a probe to
OVS which just disables meters on those kernels with broken meters, since
otherwise it will just lead to confusion. I suspect the issue will be with
kernels 4.15, 4.16, and 4.17. (And hopefully some of those kernels will be
fixed as they do bug fix releases.)
> - For the bug fix, can we get it reviewed (and probably merged) in
> datapath/compact first here in the OVS community without having to wait for
> kernel upstream? What's the usual practice for kernel module patches?
The patch was committed without comment from David Miller:
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=25432eba9
The backport patch is ready (I've appended it to the end of this message), but
I've been on vacation and I wanted to get the probe patch in at the same time.
I plan to send both patches out Tuesday night. However, if you want to apply
the patch at the bottom and give ACL rate-limiting a try, I'd love to get some
initial feedback.
Thanks,
--Justin
-=-=-=-=-=-=-=-=-=-
commit bc89eebb0e918d2e9a903d7e4a24ab1b5b522eab (HEAD -> meter-datapath)
Author: Justin Pettit
Date: Thu Jul 26 22:28:11 2018 -0700
datapath: Fix setting meter id for new entries.
Upstream commit:
openvswitch: meter: Fix setting meter id for new entries
The meter code would create an entry for each new meter. However, it
would not set the meter id in the new entry, so every meter would appear
to have a meter id of zero. This commit properly sets the meter id when
adding the entry.
Fixes: 96fbc13d7e77 ("openvswitch: Add meter infrastructure")
Signed-off-by: Justin Pettit
Cc: Andy Zhou
Signed-off-by: David S. Miller
Signed-off-by: Justin Pettit
diff --git a/datapath/meter.c b/datapath/meter.c
index 1c2ed4626c2a..281d86937555 100644
--- a/datapath/meter.c
+++ b/datapath/meter.c
@@ -221,6 +221,7 @@ static struct dp_meter *dp_meter_create(struct nlattr **a)
if (!meter)
return ERR_PTR(-ENOMEM);
+ meter->id = nla_get_u32(a[OVS_METER_ATTR_ID]);
meter->used = div_u64(ktime_get_ns(), 1000 * 1000);
meter->kbps = a[OVS_METER_ATTR_KBPS] ? 1 : 0;
meter->keep_stats = !a[OVS_METER_ATTR_CLEAR];
@@ -290,6 +291,10 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct
genl_info *info)
u32 meter_id;
bool failed;
+ if (!a[OVS_METER_ATTR_ID]) {
+ return -ENODEV;
+ }
+
meter = dp_meter_create(a);
if (IS_ERR_OR_NULL(meter))
return PTR_ERR(meter);
@@ -308,11 +313,6 @@ static int ovs_meter_cmd_set(struct sk_buff *skb, struct
genl_info *info)
goto exit_unlock;
}
- if (!a[OVS_METER_ATTR_ID]) {
- err = -ENODEV;
- goto exit_unlock;
- }
-
meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]);
/* Cannot fail after this. */
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.
On Mon, Jul 30, 2018 at 8:12 PM, Justin Pettit wrote: > > > > On Jul 30, 2018, at 5:58 PM, Justin Pettit wrote: > > > > Thanks for the review! I've pushed this series to master. > > I also just pushed this to branch-2.10. > > The rate-limiting is implemented using meters. Unfortunately, there's a bug in the current kernels that prevents meters from working properly. I've sent a patch to upstream Linux, which should solve the problem: > > https://marc.info/?l=linux-netdev=153281677212826=2 > > Once that is merged, I'll commit it to OVS, and it should work with both Linux and userspace datapaths. > > --Justin > > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Thanks Justin for the great work!! Sorry that I didn't get time to review the series, just some quick questions regarding the kernel bug you mentioned. - What's the impact of having meter ID = 0? Does it mean the meters and ACL rate limit feature can't be used at all, or is it just some limitations in certain scenarios? - For the bug fix, can we get it reviewed (and probably merged) in datapath/compact first here in the OVS community without having to wait for kernel upstream? What's the usual practice for kernel module patches? Thanks, Han ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.
> On Jul 30, 2018, at 5:58 PM, Justin Pettit wrote: > > Thanks for the review! I've pushed this series to master. I also just pushed this to branch-2.10. The rate-limiting is implemented using meters. Unfortunately, there's a bug in the current kernels that prevents meters from working properly. I've sent a patch to upstream Linux, which should solve the problem: https://marc.info/?l=linux-netdev=153281677212826=2 Once that is merged, I'll commit it to OVS, and it should work with both Linux and userspace datapaths. --Justin ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.
> On Jul 30, 2018, at 12:55 PM, Ben Pfaff wrote:
>
> On Sun, Jul 29, 2018 at 11:46:38PM -0700, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit
>
> Besides the comments I gave on patch 6 (oops), I suggest the following
> for more consistent formatting:
>
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index 6d4ed1e2c4ed..4f3cd48ce713 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -2157,7 +2157,7 @@ format_LOG(const struct ovnact_log *log, struct ds *s)
> ds_put_format(s, "severity=%s", log_severity_to_string(log->severity));
>
> if (log->meter) {
> -ds_put_format(s, ",meter=%s", log->meter);
> +ds_put_format(s, ", meter=%s", log->meter);
> }
>
> ds_put_cstr(s, ");");
Applied.
> Acked-by: Ben Pfaff
Thanks for the review! I've pushed this series to master.
--Justin
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.
On Sun, Jul 29, 2018 at 11:46:38PM -0700, Justin Pettit wrote:
> Signed-off-by: Justin Pettit
Besides the comments I gave on patch 6 (oops), I suggest the following
for more consistent formatting:
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 6d4ed1e2c4ed..4f3cd48ce713 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -2157,7 +2157,7 @@ format_LOG(const struct ovnact_log *log, struct ds *s)
ds_put_format(s, "severity=%s", log_severity_to_string(log->severity));
if (log->meter) {
-ds_put_format(s, ",meter=%s", log->meter);
+ds_put_format(s, ", meter=%s", log->meter);
}
ds_put_cstr(s, ");");
Acked-by: Ben Pfaff
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.
Signed-off-by: Justin Pettit
---
include/ovn/actions.h | 1 +
ovn/lib/actions.c | 56 +--
ovn/northd/ovn-northd.c | 4 ++
ovn/ovn-nb.ovsschema | 5 ++-
ovn/ovn-nb.xml| 9 +
ovn/utilities/ovn-nbctl.8.xml | 6 ++-
ovn/utilities/ovn-nbctl.c | 13 +--
tests/ovn.at | 73 +++
8 files changed, 149 insertions(+), 18 deletions(-)
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 6384651938f1..1c0c67ce6ffa 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -284,6 +284,7 @@ struct ovnact_log {
uint8_t verdict;/* One of LOG_VERDICT_*. */
uint8_t severity; /* One of LOG_SEVERITY_*. */
char *name;
+char *meter;
};
/* OVNACT_SET_METER. */
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 26cdb4fdd482..6d4ed1e2c4ed 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -78,7 +78,7 @@ ovnact_init(struct ovnact *ovnact, enum ovnact_type type,
size_t len)
static size_t
encode_start_controller_op(enum action_opcode opcode, bool pause,
- struct ofpbuf *ofpacts)
+ uint32_t meter_id, struct ofpbuf *ofpacts)
{
size_t ofs = ofpacts->size;
@@ -86,6 +86,7 @@ encode_start_controller_op(enum action_opcode opcode, bool
pause,
oc->max_len = UINT16_MAX;
oc->reason = OFPR_ACTION;
oc->pause = pause;
+oc->meter_id = meter_id;
struct action_header ah = { .opcode = htonl(opcode) };
ofpbuf_put(ofpacts, , sizeof ah);
@@ -105,7 +106,8 @@ encode_finish_controller_op(size_t ofs, struct ofpbuf
*ofpacts)
static void
encode_controller_op(enum action_opcode opcode, struct ofpbuf *ofpacts)
{
-size_t ofs = encode_start_controller_op(opcode, false, ofpacts);
+size_t ofs = encode_start_controller_op(opcode, false, NX_CTLR_NO_METER,
+ofpacts);
encode_finish_controller_op(ofs, ofpacts);
}
@@ -1245,7 +1247,8 @@ encode_nested_actions(const struct ovnact_nest *on,
* converted to OpenFlow, as its userdata. ovn-controller will convert the
* packet to ARP or NA and then send the packet and actions back to the
* switch inside an OFPT_PACKET_OUT message. */
-size_t oc_offset = encode_start_controller_op(opcode, false, ofpacts);
+size_t oc_offset = encode_start_controller_op(opcode, false,
+ NX_CTLR_NO_METER, ofpacts);
ofpacts_put_openflow_actions(inner_ofpacts.data, inner_ofpacts.size,
ofpacts, OFP13_VERSION);
encode_finish_controller_op(oc_offset, ofpacts);
@@ -1738,7 +1741,8 @@ encode_PUT_DHCPV4_OPTS(const struct ovnact_put_opts *pdo,
struct mf_subfield dst = expr_resolve_field(>dst);
size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_PUT_DHCP_OPTS,
- true, ofpacts);
+ true, NX_CTLR_NO_METER,
+ ofpacts);
nx_put_header(ofpacts, dst.field->id, OFP13_VERSION, false);
ovs_be32 ofs = htonl(dst.ofs);
ofpbuf_put(ofpacts, , sizeof ofs);
@@ -1769,7 +1773,7 @@ encode_PUT_DHCPV6_OPTS(const struct ovnact_put_opts *pdo,
struct mf_subfield dst = expr_resolve_field(>dst);
size_t oc_offset = encode_start_controller_op(
-ACTION_OPCODE_PUT_DHCPV6_OPTS, true, ofpacts);
+ACTION_OPCODE_PUT_DHCPV6_OPTS, true, NX_CTLR_NO_METER, ofpacts);
nx_put_header(ofpacts, dst.field->id, OFP13_VERSION, false);
ovs_be32 ofs = htonl(dst.ofs);
ofpbuf_put(ofpacts, , sizeof ofs);
@@ -1864,7 +1868,8 @@ encode_DNS_LOOKUP(const struct ovnact_dns_lookup *dl,
struct mf_subfield dst = expr_resolve_field(>dst);
size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_DNS_LOOKUP,
- true, ofpacts);
+ true, NX_CTLR_NO_METER,
+ ofpacts);
nx_put_header(ofpacts, dst.field->id, OFP13_VERSION, false);
ovs_be32 ofs = htonl(dst.ofs);
ofpbuf_put(ofpacts, , sizeof ofs);
@@ -2027,7 +2032,7 @@ encode_PUT_ND_RA_OPTS(const struct ovnact_put_opts *po,
struct mf_subfield dst = expr_resolve_field(>dst);
size_t oc_offset = encode_start_controller_op(
-ACTION_OPCODE_PUT_ND_RA_OPTS, true, ofpacts);
+ACTION_OPCODE_PUT_ND_RA_OPTS, true, NX_CTLR_NO_METER, ofpacts);
nx_put_header(ofpacts, dst.field->id, OFP13_VERSION, false);
ovs_be32 ofs = htonl(dst.ofs);
ofpbuf_put(ofpacts, , sizeof ofs);
@@ -2101,6 +2106,19 @@ parse_log_arg(struct action_context *ctx, struct
ovnact_log *log)
}
}
lexer_syntax_error(ctx->lexer, "expecting severity");
+} else if
