On Tue, Aug 7, 2018 at 2:03 AM, Justin Pettit <[email protected]> wrote: > > > > On Aug 6, 2018, at 1:27 PM, Han Zhou <[email protected]> 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 <[email protected]> > 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 <[email protected]> > Cc: Andy Zhou <[email protected]> > Signed-off-by: David S. Miller <[email protected]> > > Signed-off-by: Justin Pettit <[email protected]> > > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
