> -----Original Message----- > From: Ilya Maximets [mailto:[email protected]] > Sent: Wednesday, March 21, 2018 3:07 PM > 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 20.03.2018 12:35, Weglicki, MichalX wrote: > >> -----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? > > > Would like to hear some opinions too. > > > > >> > >>> > >>> 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. > > If the stats could change without touching the device, we can't work with > that at all, > because we can not guarantee atomicity of 2 dpdk calls (rte_eth_stats_get and > rte_eth_xstats_get_names). I.e. sequential calls to these functions could > return > stats for different ids and we will never be sure that returned stats and the > names > are really connected. IMHO, that is not how device or driver should work. > Without changing configuration, stats (ids and names) should remain the same. > (We could check this additionally with DPDK community.) > > This means that we could call 'netdev_dpdk_configure_xstats()' at the end of > 'netdev_dpdk_reconfigure()' and use cached stats ids and names until next > reconfiguration. > > In this case 'netdev_dpdk_get_custom_stats()' could be simplified by removing > 'if (netdev_dpdk_configure_xstats(dev))' checking. In case of error while > getting > xstats by id, we could only print rate limited warning.
Don't really follow your atomicity explanation - I didn't say that dpdk calls aren't atomic (there is mutex to guarantee that), I just said that counters may change, or rather that we should be prepared for it to be changed. As far as I recall mutex guards getting the stats. In general it works exactly how your wrote, changing configuration of the device trigger cache refresh. The trigger is in set_config, maybe it should be in reconfigure. However if something will not match, for any reason, stats will also will be reconfigured. I don't really understand what you would like to simplify, it is already very simple. > > > About you concern about looking for names by ids, I think we could just create > a 'namemap' for this purpose. But it works like that already. > > > > >> > >> 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
