Hello, I've went through the patch quite carefully. Main change refactors creation cached IDs and Names from IF-like code block to "Goto" code block.
There are also some initializations removal, which I don't personally agree with - even if those seems to be not needed, code may always evolve in the future. There is one xcalloc pointless check, true. The last important thing is that counters configuration can change during runtime, and I was facing a problem, where amount of counters was different during some configuration stages. That is why my initial implementation was looking for counter name based on given ID, not returned index in array => That was a reason to keep whole list of names, but only limited IDs(filtered out). Also I do think that returned array should be checked against IDs, as implementation can always change in the future as well. Br, Michal. > -----Original Message----- > From: Ilya Maximets [mailto:[email protected]] > Sent: Tuesday, January 23, 2018 2:14 PM > To: [email protected] > Cc: Heetae Ahn <[email protected]>; Ben Pfaff <[email protected]>; Stokes, > Ian <[email protected]>; Weglicki, MichalX > <[email protected]>; Ilya Maximets <[email protected]> > Subject: [PATCH] netdev-dpdk: Refactor custom stats. > > 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 <[email protected]> > --- > > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
