[ovs-dev] [PATCH] netdev-dpdk: Refactor custom stats.

Ilya Maximets i.maximets at samsung.com
Tue Jan 23 13:13:42 UTC 2018


This code is overcomplicated and completely unreadable. And a
bunch of recently fixed memory leaks confirms that statement.

Main concerns that were fixed:
* Too big nesting level.
* Useless checks like pointer checking after xmalloc.
* Misleading comments.
* Bad names of the variables.

As a bonus, size of the code significantly reduced.

Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
---

This patch made on top of memory leaks' fixes:
* https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343537.html
* https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343538.html

 lib/netdev-dpdk.c | 215 ++++++++++++++++++++----------------------------------
 1 file changed, 80 insertions(+), 135 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 50a94d1..e834c7e 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -437,10 +437,9 @@ struct netdev_dpdk {
 
     PADDED_MEMBERS(CACHE_LINE_SIZE,
         /* Names of all XSTATS counters */
-        struct rte_eth_xstat_name *rte_xstats_names;
-        int rte_xstats_names_size;
-        int rte_xstats_ids_size;
-        uint64_t *rte_xstats_ids;
+        struct rte_eth_xstat_name *xstats_names;
+        uint64_t *xstats_ids;
+        int xstats_count;
     );
 };
 
@@ -901,6 +900,8 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no,
     dev->vhost_reconfigured = false;
     dev->attached = false;
 
+    dev->xstats_count = 0;
+
     ovsrcu_init(&dev->qos_conf, NULL);
 
     ovsrcu_init(&dev->ingress_policer, NULL);
@@ -925,13 +926,6 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no,
     ovs_list_push_back(&dpdk_list, &dev->list_node);
 
     netdev_request_reconfigure(netdev);
-
-    dev->rte_xstats_names = NULL;
-    dev->rte_xstats_names_size = 0;
-
-    dev->rte_xstats_ids = NULL;
-    dev->rte_xstats_ids_size = 0;
-
     return 0;
 }
 
@@ -1174,123 +1168,83 @@ netdev_dpdk_dealloc(struct netdev *netdev)
 static void
 netdev_dpdk_clear_xstats(struct netdev_dpdk *dev)
 {
-    /* If statistics are already allocated, we have to
-     * reconfigure, as port_id could have been changed. */
-    if (dev->rte_xstats_names) {
-        free(dev->rte_xstats_names);
-        dev->rte_xstats_names = NULL;
-        dev->rte_xstats_names_size = 0;
-    }
-    if (dev->rte_xstats_ids) {
-        free(dev->rte_xstats_ids);
-        dev->rte_xstats_ids = NULL;
-        dev->rte_xstats_ids_size = 0;
-    }
-}
-
-static const char*
-netdev_dpdk_get_xstat_name(struct netdev_dpdk *dev, uint64_t id)
-{
-    if (id >= dev->rte_xstats_names_size) {
-        return "UNKNOWN";
+    if (dev->xstats_count) {
+        free(dev->xstats_ids);
+        free(dev->xstats_names);
+        dev->xstats_count = 0;
     }
-    return dev->rte_xstats_names[id].name;
 }
 
 static bool
 netdev_dpdk_configure_xstats(struct netdev_dpdk *dev)
     OVS_REQUIRES(dev->mutex)
 {
-    int rte_xstats_len;
-    bool ret;
-    struct rte_eth_xstat *rte_xstats;
-    uint64_t id;
-    int xstats_no;
-    const char *name;
-
-    /* Retrieving all XSTATS names. If something will go wrong
-     * or amount of counters will be equal 0, rte_xstats_names
-     * buffer will be marked as NULL, and any further xstats
-     * query won't be performed (e.g. during netdev_dpdk_get_stats
-     * execution). */
-
-    ret = false;
-    rte_xstats = NULL;
-
-    if (dev->rte_xstats_names == NULL || dev->rte_xstats_ids == NULL) {
-        dev->rte_xstats_names_size =
-                rte_eth_xstats_get_names(dev->port_id, NULL, 0);
-
-        if (dev->rte_xstats_names_size < 0) {
-            VLOG_WARN("Cannot get XSTATS for port: %"PRIu8, dev->port_id);
-            dev->rte_xstats_names_size = 0;
-        } else {
-            /* Reserve memory for xstats names and values */
-            dev->rte_xstats_names = xcalloc(dev->rte_xstats_names_size,
-                                            sizeof *dev->rte_xstats_names);
-
-            if (dev->rte_xstats_names) {
-                /* Retreive xstats names */
-                rte_xstats_len =
-                        rte_eth_xstats_get_names(dev->port_id,
-                                                 dev->rte_xstats_names,
-                                                 dev->rte_xstats_names_size);
-
-                if (rte_xstats_len < 0) {
-                    VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8,
-                               dev->port_id);
-                    goto out;
-                } else if (rte_xstats_len != dev->rte_xstats_names_size) {
-                    VLOG_WARN("XSTATS size doesn't match for port: %"PRIu8,
-                              dev->port_id);
-                    goto out;
-                }
-
-                dev->rte_xstats_ids = xcalloc(dev->rte_xstats_names_size,
-                                              sizeof(uint64_t));
-
-                /* We have to calculate number of counters */
-                rte_xstats = xmalloc(rte_xstats_len * sizeof *rte_xstats);
-                memset(rte_xstats, 0xff, sizeof *rte_xstats * rte_xstats_len);
-
-                /* Retreive xstats values */
-                if (rte_eth_xstats_get(dev->port_id, rte_xstats,
-                                       rte_xstats_len) > 0) {
-                    dev->rte_xstats_ids_size = 0;
-                    xstats_no = 0;
-                    for (uint32_t i = 0; i < rte_xstats_len; i++) {
-                        id = rte_xstats[i].id;
-                        name = netdev_dpdk_get_xstat_name(dev, id);
-                        /* We need to filter out everything except
-                         * dropped, error and management counters */
-                        if (string_ends_with(name, "_errors") ||
-                            strstr(name, "_management_") ||
-                            string_ends_with(name, "_dropped")) {
-
-                            dev->rte_xstats_ids[xstats_no] = id;
-                            xstats_no++;
-                        }
-                    }
-                    dev->rte_xstats_ids_size = xstats_no;
-                    ret = true;
-                } else {
-                    VLOG_WARN("Can't get XSTATS IDs for port: %"PRIu8,
-                              dev->port_id);
-                }
+    int i, ret, count;
+    struct rte_eth_xstat *xstats;
+    struct rte_eth_xstat_name *names;
+    uint64_t *ids;
 
-                free(rte_xstats);
-            }
-        }
-    } else {
+    if (dev->xstats_count) {
         /* Already configured */
-        ret = true;
+        return true;
     }
 
-out:
-    if (!ret) {
-        netdev_dpdk_clear_xstats(dev);
+    /* Get number of counters. */
+    count = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
+    if (count < 0) {
+        goto fail;
     }
-    return ret;
+
+    /* Get list of available ids. */
+    xstats = xmalloc(count * sizeof *xstats);
+    ret = rte_eth_xstats_get(dev->port_id, xstats, count);
+    if (ret != count) {
+        free(xstats);
+        goto fail;
+    }
+
+    ids = xmalloc(count * sizeof *ids);
+    for (i = 0; i < count; i++) {
+        ids[i] = xstats[i].id;
+    }
+    free(xstats);
+
+    /* Get names for ids. */
+    names = xmalloc(count * sizeof *names);
+    ret = rte_eth_xstats_get_names_by_id(dev->port_id, names, count, ids);
+    if (ret != count) {
+        goto fail_free;
+    }
+
+    /* Filter out uninteresting counters. */
+    dev->xstats_count = 0;
+    for (i = 0; i < count; i++) {
+        if (string_ends_with(names[i].name, "_errors")
+            || strstr(names[i].name, "_management_")
+            || string_ends_with(names[i].name, "_dropped")) {
+
+            ids[dev->xstats_count] = i;
+            ovs_strlcpy(names[dev->xstats_count].name, names[i].name,
+                        NETDEV_CUSTOM_STATS_NAME_SIZE);
+            dev->xstats_count++;
+        }
+    }
+    if (!dev->xstats_count) {
+        /* No counters left. */
+        goto fail_free;
+    }
+
+    dev->xstats_ids = ids;
+    dev->xstats_names = names;
+
+    return true;
+
+fail_free:
+    free(ids);
+    free(names);
+fail:
+    VLOG_WARN("Cannot get XSTATS for port: %"PRIu8, dev->port_id);
+    return false;
 }
 
 static int
@@ -2326,40 +2280,31 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev,
                              struct netdev_custom_stats *custom_stats)
 {
 
-    uint32_t i;
+    int i, ret;
     struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
-    int rte_xstats_ret;
 
     ovs_mutex_lock(&dev->mutex);
 
     if (netdev_dpdk_configure_xstats(dev)) {
-        uint64_t *values = xcalloc(dev->rte_xstats_ids_size,
-                                   sizeof(uint64_t));
-
-        rte_xstats_ret =
-                rte_eth_xstats_get_by_id(dev->port_id, dev->rte_xstats_ids,
-                                         values, dev->rte_xstats_ids_size);
+        uint64_t *values = xcalloc(dev->xstats_count, sizeof *values);
 
-        if (rte_xstats_ret > 0 &&
-            rte_xstats_ret <= dev->rte_xstats_ids_size) {
+        ret = rte_eth_xstats_get_by_id(dev->port_id, dev->xstats_ids,
+                                       values, dev->xstats_count);
 
-            custom_stats->size = rte_xstats_ret;
-            custom_stats->counters =
-                    (struct netdev_custom_counter *) xcalloc(rte_xstats_ret,
-                            sizeof(struct netdev_custom_counter));
+        if (ret == dev->xstats_count) {
+            custom_stats->size = dev->xstats_count;
+            custom_stats->counters = xcalloc(dev->xstats_count,
+                                             sizeof *custom_stats->counters);
 
-            for (i = 0; i < rte_xstats_ret; i++) {
+            for (i = 0; i < dev->xstats_count; i++) {
                 ovs_strlcpy(custom_stats->counters[i].name,
-                            netdev_dpdk_get_xstat_name(dev,
-                                                       dev->rte_xstats_ids[i]),
+                            dev->xstats_names[i].name,
                             NETDEV_CUSTOM_STATS_NAME_SIZE);
                 custom_stats->counters[i].value = values[i];
             }
         } else {
             VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8,
                       dev->port_id);
-            custom_stats->counters = NULL;
-            custom_stats->size = 0;
             /* Let's clear statistics cache, so it will be
              * reconfigured */
             netdev_dpdk_clear_xstats(dev);
-- 
2.7.4



More information about the dev mailing list