[ovs-dev] [PATCH] dpif-netlink: Add meter support.

Justin Pettit jpettit at ovn.org
Wed Jul 25 06:18:20 UTC 2018

> On Jul 6, 2018, at 10:15 AM, Ben Pfaff <blp at ovn.org> wrote:
> On Fri, Jun 29, 2018 at 11:09:18AM -0700, Justin Pettit wrote:
>> From: Andy Zhou <azhou at ovn.org>
>> To work with kernel datapath that supports meter.
>> Signed-off-by: Andy Zhou <azhou at ovn.org>
>> Co-authored-by: Justin Pettit <jpettit at ovn.org>
>> Signed-off-by: Justin Pettit <jpettit at ovn.org>
> Thanks for reviving this.
> The new code here seems to use our older declaration style where all
> declarations happen at the top of functions, rather than the newer one
> where they tend to be closer to initialization or first use.  This is a
> quibble and feel free to ignore it if you think I am nit-picking.

I love nit-picking!

> It's probably worth carefully looking at the log messages.  Some of them
> seem unnecessary, e.g. the first one in dpif_netlink_meter_transact(),
> because the caller or the callee already logs.  Others seem like the
> wrong log level, e.g. the second one in dpif_netlink_meter_transact().
> And I think that most or all should be rate-limited.

Okay, I think I cleaned those all up.

> In dpif_netlink_meter_get_features(), I think that the memset() call is
> not needed because dpif_meter_get_features() already does it.

Good catch.

> In dpif_netlink_meter_set(), these checks seem like ones that should
> happen centrally at a higher level:
>    if (!(config->flags & (OFPMF13_KBPS | OFPMF13_PKTPS))) {
>        return EBADF; /* Rate unit type not set. */
>    }
>    if ((config->flags & OFPMF13_KBPS) && (config->flags & OFPMF13_PKTPS)) {
>        return EBADF; /* Both rate units may not be set. */
>    }
> Maybe this one too?
>    if (config->n_bands == 0) {
>        return EINVAL; /* No bands. */
>    }

Good point.  I've added a patch before this one to move this logic into "dpif.c" and then removed the same checks from "dpif-netdev.c".

> Shouldn't dpif_netlink_meter_set() reject bands that are not "drop"?

Yes, added.

> The inner NL_NESTED_FOR_EACH in dpif_netlink_get_stats() could use
> nl_attr_find().

I think the function I need is nl_attr_find_nested().  It doesn't seem like the function checks that attribute length, so I added that, too.  Let me know if you think it looks reasonable. 

> In the case where dpif_netlink_get_stats() is used to delete a meter,
> and statistics are wanted, and some bands are present but the statistics
> aren't present in at least one of those bands, the function reports an
> error, even though the meter was deleted successfully.  That seems like
> a weird corner case.

Okay, I just went ahead and zeroed the band stats for that entry in that case.

> In dpif_netlink_get_stats(), 'stats' has lots of members but this
> function seems to only initialize some of them.  This also seems true of
> dpif_meter_get().

I believe those other structures are filled in by meter_request_reply() in ofproto.c.  The way it's handled seems a little odd to me, but that seems to be the existing interface.  Do you think it should be changed?

> In dpif-netlink.c, it looks to me like initialization fails if meters
> aren't available.  I think that breaks backward compatibility with old
> kernel modules.  I doubt we want that.

Excellent point.  I changed the logic, so it logs a message at info level.  I also removed the conditional compile for Windows, so it will just get the warning, too.

Thanks for the thorough review.  I'll run the patch(es) through some tests in the morning and send them out.


More information about the dev mailing list