> -----Original Message----- > From: Ilya Maximets [mailto:[email protected]] > Sent: Tuesday, March 20, 2018 9:50 AM > To: Weglicki, MichalX <[email protected]>; [email protected] > Cc: Heetae Ahn <[email protected]>; Ben Pfaff <[email protected]>; Stokes, > Ian <[email protected]> > Subject: Re: [PATCH] netdev-dpdk: Refactor custom stats. > > On 19.03.2018 13:22, Weglicki, MichalX wrote: > > Hello, > > Hello. > > > > > I've went through the patch quite carefully. > > Thanks for reviewing this. > > > Main change refactors creation cached IDs and Names from IF-like code block > > to "Goto" code block. > > Current code is over-nested. It has nesting level of 6 (7 including function > definition). > It's like: > netdev_dpdk_configure_xstats > { > if () { > if () { > ... > } else { > ... > if () { > ... > if () { > } else if { > } > ... > if () { > for () { > if () { <-- level #7 ! > } > } > } else { > } > } > } > } else { > } > if () { > } > } > > > IMHO, This is not readable. Especially, combining with constant line wraps > because > of limited line lengths. I wanted to make plain execution sequence to simplify > understanding of the code. Most of the 'if' statements are just error > checking. > And the single exit point from such conditions usually implemented by 'goto's. > It's a common practice for system programming. It doesn't worth to keep so > deep > nesting level for error checking conditions. This also matches the style of > all > the other code in netdev-dpdk and not only here.
For me it is straightforward error checking, so I don't really see an advantage. Not everything is implemented using "goto" in netdev-dpdk. I assume this is preference thing so maybe we could ask someone to make a call, Ben/Ian? > > > > > 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. > > Could you, please, specify? - - dev->rte_xstats_names = NULL; - dev->rte_xstats_names_size = 0; - - dev->rte_xstats_ids = NULL; - dev->rte_xstats_ids_size = 0; I know you are checking it in runtime against other variables, but still I believe that such initialization should remain nevertheless. > > > 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. > > Hmm. OK. But I think, in this case we could even more simplify the code. > As I understand, the 'netdev_dpdk_reconfigure' is the only function that > could make influence on the stats counters in HW. Correct me, if I'm wrong. > In this case we could call 'netdev_dpdk_configure_xstats()' only once at the > end of reconfiguration. After that all the inconsistency with returned > from DPDK values should be treated as HW or DPDK error and we should not > handle this case in OVS and just do not return any stats. > This will simplify 'netdev_dpdk_get_custom_stats()' too. I'm not sure if what you are stating is true to be honest - "'netdev_dpdk_reconfigure' is the only function that could make influence on the stats counters in HW" - also I'm not sure if it can be called just once . And I'm not sure what gain do you really have in mind. Currently If counters configuration will change, all IDs and Names will be queried again - as I said this is what I've encountered, however It was 3 months ago so I don't recall all the details. Anyhow, I still think that we can't assume that counters will remain the same, And we have to guarantee that cached data is up to date. > > We also could refactor 'netdev_dpdk_get_stats' in a same way and reuse > already configured xstats. > What do you think? Yes Ilya, to be honest I even had it in my backlog, but didn't manage to do that yet. However definitely it should be done. > > Best regards, Ilya Maximets. > > > > > 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
