[ovs-dev] [PATCH] netdev-dpdk: Refactor custom stats.
Weglicki, MichalX
michalx.weglicki at intel.com
Thu Mar 22 08:05:08 UTC 2018
> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets at samsung.com]
> Sent: Wednesday, March 21, 2018 3:07 PM
> To: Weglicki, MichalX <michalx.weglicki at intel.com>; ovs-dev at openvswitch.org
> Cc: Heetae Ahn <heetae82.ahn at samsung.com>; Ben Pfaff <blp at ovn.org>; Stokes, Ian <ian.stokes at intel.com>
> Subject: Re: [PATCH] netdev-dpdk: Refactor custom stats.
>
> On 20.03.2018 12:35, Weglicki, MichalX wrote:
> >> -----Original Message-----
> >> From: Ilya Maximets [mailto:i.maximets at samsung.com]
> >> Sent: Tuesday, March 20, 2018 9:50 AM
> >> To: Weglicki, MichalX <michalx.weglicki at intel.com>; ovs-dev at openvswitch.org
> >> Cc: Heetae Ahn <heetae82.ahn at samsung.com>; Ben Pfaff <blp at ovn.org>; Stokes, Ian <ian.stokes at intel.com>
> >> 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:i.maximets at samsung.com]
> >>>> Sent: Tuesday, January 23, 2018 2:14 PM
> >>>> To: ovs-dev at openvswitch.org
> >>>> Cc: Heetae Ahn <heetae82.ahn at samsung.com>; Ben Pfaff <blp at ovn.org>; Stokes, Ian <ian.stokes at intel.com>; Weglicki,
> >> MichalX
> >>>> <michalx.weglicki at intel.com>; Ilya Maximets <i.maximets at samsung.com>
> >>>> 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 <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