diff mbox series

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

Message ID 20180629180918.38132-1-jpettit@ovn.org
State Changes Requested
Headers show
Series [ovs-dev] dpif-netlink: Add meter support. | expand

Commit Message

Justin Pettit June 29, 2018, 6:09 p.m. UTC
From: Andy Zhou <azhou@ovn.org>

To work with kernel datapath that supports meter.

Signed-off-by: Andy Zhou <azhou@ovn.org>
Co-authored-by: Justin Pettit <jpettit@ovn.org>
Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
 Documentation/faq/releases.rst |   1 +
 lib/dpif-netlink.c             | 302 ++++++++++++++++++++++++++++++---
 2 files changed, 283 insertions(+), 20 deletions(-)

Comments

Alin-Gabriel Serdean July 1, 2018, 9:11 p.m. UTC | #1
> 
> From: Andy Zhou <azhou@ovn.org>
> 
> To work with kernel datapath that supports meter.
> 
> Signed-off-by: Andy Zhou <azhou@ovn.org>
> Co-authored-by: Justin Pettit <jpettit@ovn.org>
> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> ---
>  Documentation/faq/releases.rst |   1 +
>  lib/dpif-netlink.c             | 302 ++++++++++++++++++++++++++++++---
>  2 files changed, 283 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/faq/releases.rst
> b/Documentation/faq/releases.rst index fab93b1888a6..d5f14e384155
> 100644
[Alin Serdean] Small nit: shouldn't it be v3?

Acked-by: Alin Gabriel Serdean <aserdean@ovn.org>
Justin Pettit July 2, 2018, 1:25 a.m. UTC | #2
> On Jul 1, 2018, at 2:11 PM, <aserdean@ovn.org> <aserdean@ovn.org> wrote:
> 
>> 
>> From: Andy Zhou <azhou@ovn.org>
>> 
>> To work with kernel datapath that supports meter.
>> 
>> Signed-off-by: Andy Zhou <azhou@ovn.org>
>> Co-authored-by: Justin Pettit <jpettit@ovn.org>
>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
>> ---
>> Documentation/faq/releases.rst |   1 +
>> lib/dpif-netlink.c             | 302 ++++++++++++++++++++++++++++++---
>> 2 files changed, 283 insertions(+), 20 deletions(-)
>> 
>> diff --git a/Documentation/faq/releases.rst
>> b/Documentation/faq/releases.rst index fab93b1888a6..d5f14e384155
>> 100644
> [Alin Serdean] Small nit: shouldn't it be v3?

Yes.  I realized that as soon as I hit send.  :-)

> Acked-by: Alin Gabriel Serdean <aserdean@ovn.org>

Thanks!

--Justin
Ben Pfaff July 6, 2018, 5:15 p.m. UTC | #3
On Fri, Jun 29, 2018 at 11:09:18AM -0700, Justin Pettit wrote:
> From: Andy Zhou <azhou@ovn.org>
> 
> To work with kernel datapath that supports meter.
> 
> Signed-off-by: Andy Zhou <azhou@ovn.org>
> Co-authored-by: Justin Pettit <jpettit@ovn.org>
> Signed-off-by: Justin Pettit <jpettit@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.

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.

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

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. */
    }

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

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

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.

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().

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.

Thanks,

Ben.
Justin Pettit July 25, 2018, 6:18 a.m. UTC | #4
> On Jul 6, 2018, at 10:15 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Fri, Jun 29, 2018 at 11:09:18AM -0700, Justin Pettit wrote:
>> From: Andy Zhou <azhou@ovn.org>
>> 
>> To work with kernel datapath that supports meter.
>> 
>> Signed-off-by: Andy Zhou <azhou@ovn.org>
>> Co-authored-by: Justin Pettit <jpettit@ovn.org>
>> Signed-off-by: Justin Pettit <jpettit@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.

--Justin
diff mbox series

Patch

diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst
index fab93b1888a6..d5f14e384155 100644
--- a/Documentation/faq/releases.rst
+++ b/Documentation/faq/releases.rst
@@ -124,6 +124,7 @@  Q: Are all features available with all datapaths?
     Set action            YES            YES            YES       PARTIAL
     NIC Bonding           YES            YES            YES       YES
     Multiple VTEPs        YES            YES            YES       YES
+    Meters                4.15           YES            YES       NO
     ===================== ============== ============== ========= =======
 
     Do note, however:
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index aa9bbd9463a7..fd176500849c 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -212,6 +212,7 @@  static int ovs_datapath_family;
 static int ovs_vport_family;
 static int ovs_flow_family;
 static int ovs_packet_family;
+static int ovs_meter_family;
 
 /* Generic Netlink multicast groups for OVS.
  *
@@ -2921,40 +2922,296 @@  dpif_netlink_ct_flush(struct dpif *dpif OVS_UNUSED, const uint16_t *zone,
 
 /* Meters */
 static void
-dpif_netlink_meter_get_features(const struct dpif * dpif OVS_UNUSED,
+dpif_netlink_meter_init(struct dpif_netlink *dpif, struct ofpbuf *buf,
+                        void *stub, size_t size, uint32_t command)
+{
+    struct ovs_header *ovs_header;
+
+    ofpbuf_use_stub(buf, stub, size);
+
+    nl_msg_put_genlmsghdr(buf, 0, ovs_meter_family, NLM_F_REQUEST | NLM_F_ECHO,
+                          command, OVS_METER_VERSION);
+
+    ovs_header = ofpbuf_put_uninit(buf, sizeof *ovs_header);
+    ovs_header->dp_ifindex = dpif->dp_ifindex;
+}
+
+/* Execute meter 'request' in the kernel datapath.  If the command
+ * fails, returns a positive errno value.  Otherwise, stores the reply
+ * in '*replyp', parses the policy according to 'reply_policy' into the
+ * array of Netlink attribute in 'a', and returns 0.  On success, the
+ * caller is responsible for calling ofpbuf_delete() on '*replyp'
+ * ('replyp' will contain pointers into 'a'). */
+static int
+dpif_netlink_meter_transact(struct ofpbuf *request, struct ofpbuf **replyp,
+                            const struct nl_policy *reply_policy,
+                            struct nlattr **a, size_t size_a)
+{
+    int error;
+    struct nlmsghdr *nlmsg;
+    struct genlmsghdr *genl;
+    struct ovs_header *ovs_header;
+
+    error = nl_transact(NETLINK_GENERIC, request, replyp);
+    ofpbuf_uninit(request);
+
+    if (error) {
+        VLOG_INFO("nl_transact for meter failed");
+        return error;
+    }
+
+    nlmsg = ofpbuf_try_pull(*replyp, sizeof *nlmsg);
+    genl = ofpbuf_try_pull(*replyp, sizeof *genl);
+    ovs_header = ofpbuf_try_pull(*replyp, sizeof *ovs_header);
+    if (!nlmsg || !genl || !ovs_header
+        || nlmsg->nlmsg_type != ovs_meter_family
+        || !nl_policy_parse(*replyp, 0, reply_policy, a, size_a)) {
+        VLOG_INFO("Kernel module response to meter tranaction is invalid");
+        return EINVAL;
+    }
+    return 0;
+}
+
+static void
+dpif_netlink_meter_get_features(const struct dpif *dpif_,
                                 struct ofputil_meter_features *features)
 {
-    features->max_meters = 0;
-    features->band_types = 0;
-    features->capabilities = 0;
-    features->max_bands = 0;
-    features->max_color = 0;
+    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
+    struct ofpbuf buf, *msg;
+    uint64_t stub[1024 / 8];
+
+    static const struct nl_policy ovs_meter_features_policy[] = {
+        [OVS_METER_ATTR_MAX_METERS] = { .type = NL_A_U32 },
+        [OVS_METER_ATTR_MAX_BANDS] = { .type = NL_A_U32 },
+        [OVS_METER_ATTR_BANDS] = { .type = NL_A_NESTED, .optional = true },
+    };
+    struct nlattr *a[ARRAY_SIZE(ovs_meter_features_policy)];
+
+    memset(features, 0, sizeof *features);
+
+    dpif_netlink_meter_init(dpif, &buf, stub, sizeof stub,
+                            OVS_METER_CMD_FEATURES);
+    if (dpif_netlink_meter_transact(&buf, &msg, ovs_meter_features_policy, a,
+                                    ARRAY_SIZE(ovs_meter_features_policy))) {
+        return;
+    }
+
+    features->max_meters = nl_attr_get_u32(a[OVS_METER_ATTR_MAX_METERS]);
+    features->max_bands = nl_attr_get_u32(a[OVS_METER_ATTR_MAX_BANDS]);
+
+    /* Bands is a nested attribute of zero or more nested
+     * band attributes.  */
+    if (a[OVS_METER_ATTR_BANDS]) {
+        const struct nlattr *nla;
+        size_t left;
+
+        NL_NESTED_FOR_EACH (nla, left, a[OVS_METER_ATTR_BANDS]) {
+            const struct nlattr *band_nla;
+            size_t band_left;
+
+            NL_NESTED_FOR_EACH (band_nla, band_left, nla) {
+                if (nl_attr_type(band_nla) == OVS_BAND_ATTR_TYPE) {
+                    if (nl_attr_get_size(band_nla) == sizeof(uint32_t)) {
+                        switch (nl_attr_get_u32(band_nla)) {
+                        case OVS_METER_BAND_TYPE_DROP:
+                            features->band_types |= 1 << OFPMBT13_DROP;
+                            break;
+                        }
+                    }
+                }
+            }
+        }
+    }
+    features->capabilities = OFPMF13_KBPS | OFPMF13_PKTPS | OFPMF13_BURST
+        | OFPMF13_STATS;
+
+    ofpbuf_delete(msg);
+}
+
+static int
+dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id *meter_id,
+                       struct ofputil_meter_config *config)
+{
+    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
+    struct ofpbuf buf, *msg;
+    uint64_t stub[1024 / 8];
+    int error;
+    size_t i;
+
+    size_t bands_offset, band_offset;
+
+    static const struct nl_policy ovs_meter_set_response_policy[] = {
+        [OVS_METER_ATTR_ID] = { .type = NL_A_U32 },
+    };
+    struct nlattr *a[ARRAY_SIZE(ovs_meter_set_response_policy)];
+
+    /* Validate arguments. */
+    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. */
+    }
+    if (config->n_bands == 0) {
+        return EINVAL; /* No bands. */
+    }
+
+    dpif_netlink_meter_init(dpif, &buf, stub, sizeof stub, OVS_METER_CMD_SET);
+
+    if (meter_id->uint32 != UINT32_MAX) {
+        nl_msg_put_u32(&buf, OVS_METER_ATTR_ID, meter_id->uint32);
+    }
+    if (config->flags & OFPMF13_KBPS) {
+        nl_msg_put_flag(&buf, OVS_METER_ATTR_KBPS);
+    }
+
+    bands_offset = nl_msg_start_nested(&buf, OVS_METER_ATTR_BANDS);
+    /* Bands */
+    for (i = 0; i < config->n_bands; ++i) {
+        struct ofputil_meter_band * band = &config->bands[i];
+        uint32_t band_type;
+
+        band_offset = nl_msg_start_nested(&buf, OVS_BAND_ATTR_UNSPEC);
+
+        switch (band->type) {
+        case OFPMBT13_DROP:
+            band_type = OVS_METER_BAND_TYPE_DROP;
+            break;
+        default:
+            band_type = OVS_METER_BAND_TYPE_UNSPEC;
+        }
+        nl_msg_put_u32(&buf, OVS_BAND_ATTR_TYPE, band_type);
+        nl_msg_put_u32(&buf, OVS_BAND_ATTR_RATE, band->rate);
+        nl_msg_put_u32(&buf, OVS_BAND_ATTR_BURST,
+                       config->flags & OFPMF13_BURST ?
+                       band->burst_size : band->rate);
+        nl_msg_end_nested(&buf, band_offset);
+    }
+    nl_msg_end_nested(&buf, bands_offset);
+
+    error = dpif_netlink_meter_transact(&buf, &msg,
+                                    ovs_meter_set_response_policy, a,
+                                    ARRAY_SIZE(ovs_meter_set_response_policy));
+    if (error) {
+        VLOG_INFO("dpif_netlink_meter_tranact OVS_METER_CMD_SET failed");
+        return error;
+    }
+
+    meter_id->uint32 = nl_attr_get_u32(a[OVS_METER_ATTR_ID]);
+    ofpbuf_delete(msg);
+    return 0;
 }
 
+/* Retrieve statistics and/or delete meter 'meter_id'.  Statistics are
+ * stored in 'stats', if it is not null.  If 'command' is
+ * OVS_METER_CMD_DEL, the meter is deleted and statistics are optionally
+ * retrieved.  If 'command' is OVS_METER_CMD_GET, then statistics are
+ * simply retrieved. */
 static int
-dpif_netlink_meter_set(struct dpif *dpif OVS_UNUSED,
-                       ofproto_meter_id *meter_id OVS_UNUSED,
-                       struct ofputil_meter_config *config OVS_UNUSED)
+dpif_netlink_meter_get_stats(const struct dpif *dpif_,
+                             ofproto_meter_id meter_id,
+                             struct ofputil_meter_stats *stats,
+                             uint16_t max_bands,
+                             enum ovs_meter_cmd command)
 {
-    return EFBIG; /* meter_id out of range */
+    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
+    struct ofpbuf buf, *msg;
+    uint64_t stub[1024 / 8];
+    int error;
+    size_t n_bands;
+
+    static const struct nl_policy ovs_meter_stats_policy[] = {
+        [OVS_METER_ATTR_ID] = { .type = NL_A_U32, .optional = true},
+        [OVS_METER_ATTR_STATS] = { NL_POLICY_FOR(struct ovs_flow_stats),
+                                   .optional = true},
+        [OVS_METER_ATTR_BANDS] = { .type = NL_A_NESTED, .optional = true },
+    };
+    struct nlattr *a[ARRAY_SIZE(ovs_meter_stats_policy)];
+
+    dpif_netlink_meter_init(dpif, &buf, stub, sizeof stub, command);
+
+    nl_msg_put_u32(&buf, OVS_METER_ATTR_ID, meter_id.uint32);
+
+    error = dpif_netlink_meter_transact(&buf, &msg,
+                                        ovs_meter_stats_policy, a,
+                                        ARRAY_SIZE(ovs_meter_stats_policy));
+    if (error) {
+        VLOG_INFO("dpif_netlink_meter_transact %s failed",
+                  command == OVS_METER_CMD_GET ? "get" : "del");
+        return error;
+    }
+
+    if (stats
+        && a[OVS_METER_ATTR_ID]
+        && a[OVS_METER_ATTR_STATS]
+        && nl_attr_get_u32(a[OVS_METER_ATTR_ID]) == meter_id.uint32) {
+        /* return stats */
+        const struct ovs_flow_stats *stat;
+        const struct nlattr *nla;
+        size_t left;
+
+        stat = nl_attr_get(a[OVS_METER_ATTR_STATS]);
+        stats->packet_in_count = get_32aligned_u64(&stat->n_packets);
+        stats->byte_in_count = get_32aligned_u64(&stat->n_bytes);
+
+        if (a[OVS_METER_ATTR_BANDS]) {
+            n_bands = 0;
+            NL_NESTED_FOR_EACH (nla, left, a[OVS_METER_ATTR_BANDS]) {
+                const struct nlattr *band_nla;
+                size_t band_left;
+
+                stat = NULL;
+
+                NL_NESTED_FOR_EACH (band_nla, band_left, nla) {
+                    if (nl_attr_type(band_nla) == OVS_BAND_ATTR_STATS
+                        && nl_attr_get_size(band_nla)
+                        == sizeof(struct ovs_flow_stats)) {
+                        stat = nl_attr_get(band_nla);
+                    }
+                }
+                if (!stat) {
+                    error = EINVAL;
+                    goto cleanup;
+                }
+
+                if (n_bands < max_bands) {
+                    stats->bands[n_bands].packet_count
+                        = get_32aligned_u64(&stat->n_packets);
+                    stats->bands[n_bands].byte_count
+                        = get_32aligned_u64(&stat->n_bytes);
+                    ++n_bands;
+                }
+            }
+            stats->n_bands = n_bands;
+        }
+    } else {
+        /* For a non-exist meter, return 0 stats. */
+        if (stats) {
+            stats->packet_in_count = 0;
+            stats->byte_in_count = 0;
+            stats->n_bands = 0;
+        }
+    }
+
+cleanup:
+    ofpbuf_delete(msg);
+    return error;
 }
 
 static int
-dpif_netlink_meter_get(const struct dpif *dpif OVS_UNUSED,
-                       ofproto_meter_id meter_id OVS_UNUSED,
-                       struct ofputil_meter_stats *stats OVS_UNUSED,
-                       uint16_t n_bands OVS_UNUSED)
+dpif_netlink_meter_get(const struct dpif *dpif, ofproto_meter_id meter_id,
+                       struct ofputil_meter_stats *stats, uint16_t max_bands)
 {
-    return EFBIG; /* meter_id out of range */
+    return dpif_netlink_meter_get_stats(dpif, meter_id, stats, max_bands,
+                                        OVS_METER_CMD_GET);
 }
 
 static int
-dpif_netlink_meter_del(struct dpif *dpif OVS_UNUSED,
-                       ofproto_meter_id meter_id OVS_UNUSED,
-                       struct ofputil_meter_stats *stats OVS_UNUSED,
-                       uint16_t n_bands OVS_UNUSED)
+dpif_netlink_meter_del(struct dpif *dpif, ofproto_meter_id meter_id,
+                       struct ofputil_meter_stats *stats, uint16_t max_bands)
 {
-    return EFBIG; /* meter_id out of range */
+    return dpif_netlink_meter_get_stats(dpif, meter_id, stats, max_bands,
+                                        OVS_METER_CMD_DEL);
 }
 
 
@@ -3040,6 +3297,11 @@  dpif_netlink_init(void)
             error = nl_lookup_genl_mcgroup(OVS_VPORT_FAMILY, OVS_VPORT_MCGROUP,
                                            &ovs_vport_mcgroup);
         }
+#ifndef _WIN32
+        if (!error) {
+            error = nl_lookup_genl_family(OVS_METER_FAMILY, &ovs_meter_family);
+        }
+#endif
 
         ovs_tunnels_out_of_tree = dpif_netlink_rtnl_probe_oot_tunnels();