[ovs-dev] [ACL Meters 7/7] ovn: Add rate-limiting for ACL logs.

[Justin Pettit]

> On Aug 6, 2018, at 1:27 PM, Han Zhou <zhouhan at 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 <jpettit at 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 <jpettit at ovn.org>
        Cc: Andy Zhou <azhou at ovn.org>
        Signed-off-by: David S. Miller <davem at davemloft.net>
    
    Signed-off-by: Justin Pettit <jpettit at 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. */