[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