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.maxim...@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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev