> On Aug 6, 2018, at 1:27 PM, Han Zhou <zhou...@gmail.com> 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 <jpet...@ovn.org>
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 <jpet...@ovn.org>
Cc: Andy Zhou <az...@ovn.org>
Signed-off-by: David S. Miller <da...@davemloft.net>
Signed-off-by: Justin Pettit <jpet...@ovn.org>
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
