[ovs-dev] netdev-dpdk: Refactor custom stats.

Message ID 1516713222-25140-1-git-send-email-i.maximets@samsung.com
State Changes Requested
Delegated to: Ian Stokes
Headers show
Series
  • [ovs-dev] netdev-dpdk: Refactor custom stats.
Related show

Commit Message

Ilya Maximets Jan. 23, 2018, 1:13 p.m.
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@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(-)

Comments

Weglicki, MichalX March 19, 2018, 10:22 a.m. | #1
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:i.maximets@samsung.com]
> Sent: Tuesday, January 23, 2018 2:14 PM
> To: ovs-dev@openvswitch.org
> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@intel.com>; Weglicki, MichalX
> <michalx.weglicki@intel.com>; Ilya Maximets <i.maximets@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@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
Ilya Maximets March 20, 2018, 8:49 a.m. | #2
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.

> 
> 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?

> 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.

We also could refactor 'netdev_dpdk_get_stats' in a same way and reuse
already configured xstats.

What do you think?

Best regards, Ilya Maximets.

> 
> Br, 
> Michal. 
> 
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>> Sent: Tuesday, January 23, 2018 2:14 PM
>> To: ovs-dev@openvswitch.org
>> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@intel.com>; Weglicki, MichalX
>> <michalx.weglicki@intel.com>; Ilya Maximets <i.maximets@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@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
> 
> 
> 
>
Weglicki, MichalX March 20, 2018, 9:35 a.m. | #3
> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Tuesday, March 20, 2018 9:50 AM
> To: Weglicki, MichalX <michalx.weglicki@intel.com>; ovs-dev@openvswitch.org
> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@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? 

> 
> >
> > 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:i.maximets@samsung.com]
> >> Sent: Tuesday, January 23, 2018 2:14 PM
> >> To: ovs-dev@openvswitch.org
> >> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@intel.com>; Weglicki,
> MichalX
> >> <michalx.weglicki@intel.com>; Ilya Maximets <i.maximets@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@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
> >
> >
> >
> >
Ilya Maximets March 21, 2018, 2:06 p.m. | #4
On 20.03.2018 12:35, Weglicki, MichalX wrote:
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>> Sent: Tuesday, March 20, 2018 9:50 AM
>> To: Weglicki, MichalX <michalx.weglicki@intel.com>; ovs-dev@openvswitch.org
>> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@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.


About you concern about looking for names by ids, I think we could just create
a 'namemap' for this purpose.
 
> 
>>
>> 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@samsung.com]
>>>> Sent: Tuesday, January 23, 2018 2:14 PM
>>>> To: ovs-dev@openvswitch.org
>>>> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@intel.com>; Weglicki,
>> MichalX
>>>> <michalx.weglicki@intel.com>; Ilya Maximets <i.maximets@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@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
Ian Stokes March 21, 2018, 3:22 p.m. | #5
> On 20.03.2018 12:35, Weglicki, MichalX wrote:
> >> -----Original Message-----
> >> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> >> Sent: Tuesday, March 20, 2018 9:50 AM
> >> To: Weglicki, MichalX <michalx.weglicki@intel.com>;
> >> ovs-dev@openvswitch.org
> >> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>;
> >> Stokes, Ian <ian.stokes@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.
> 

I was familiar with the existing code but after comparing the two approaches, from a purely aesthetic point the goto solution looks cleaner and a little simpler to understand on first pass.

As examples of both approaches are present in netdev-dpdk this comes to preference, I'd be in favor the new goto format being introduced once all error cases are accounted for.

Ian
Ben Pfaff March 21, 2018, 3:35 p.m. | #6
On Tue, Mar 20, 2018 at 09:35:17AM +0000, Weglicki, MichalX wrote:
> > -----Original Message-----
> > From: Ilya Maximets [mailto:i.maximets@samsung.com]
> > Sent: Tuesday, March 20, 2018 9:50 AM
> > To: Weglicki, MichalX <michalx.weglicki@intel.com>; ovs-dev@openvswitch.org
> > Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@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? 

When there's deep nesting, "goto" is often easier to read.  This looks
like one of those cases.

Sometimes, a third choice is to use a wrapper function that does the
deallocation (or other resource freeing) and then substitute "return"
for "goto".
Weglicki, MichalX March 22, 2018, 8:05 a.m. | #7
> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Wednesday, March 21, 2018 3:07 PM
> To: Weglicki, MichalX <michalx.weglicki@intel.com>; ovs-dev@openvswitch.org
> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@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@samsung.com]
> >> Sent: Tuesday, March 20, 2018 9:50 AM
> >> To: Weglicki, MichalX <michalx.weglicki@intel.com>; ovs-dev@openvswitch.org
> >> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@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@samsung.com]
> >>>> Sent: Tuesday, January 23, 2018 2:14 PM
> >>>> To: ovs-dev@openvswitch.org
> >>>> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@intel.com>; Weglicki,
> >> MichalX
> >>>> <michalx.weglicki@intel.com>; Ilya Maximets <i.maximets@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@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
Ilya Maximets April 3, 2018, 3:25 p.m. | #8
On 22.03.2018 11:05, Weglicki, MichalX wrote:
> 
> 
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>> Sent: Wednesday, March 21, 2018 3:07 PM
>> To: Weglicki, MichalX <michalx.weglicki@intel.com>; ovs-dev@openvswitch.org
>> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@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@samsung.com]
>>>> Sent: Tuesday, March 20, 2018 9:50 AM
>>>> To: Weglicki, MichalX <michalx.weglicki@intel.com>; ovs-dev@openvswitch.org
>>>> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@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.

IMHO, this should not happen. Only reconfiguration could change the set of
available counters, so, we should not care about some magical cases where
everything messed up without config changes inside OVS. We could just print
the rate-limited warning and wait for reconfiguration to reconfigure stats.

Anyway, I'll check the behaviour and maybe will keep the current stats'
reconfiguration on failure.

Let me prepare v2. At least, we'll have subject to discuss after that.

Key point was to simplify the code itself, without significant functional changes.

> 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@samsung.com]
>>>>>> Sent: Tuesday, January 23, 2018 2:14 PM
>>>>>> To: ovs-dev@openvswitch.org
>>>>>> Cc: Heetae Ahn <heetae82.ahn@samsung.com>; Ben Pfaff <blp@ovn.org>; Stokes, Ian <ian.stokes@intel.com>; Weglicki,
>>>> MichalX
>>>>>> <michalx.weglicki@intel.com>; Ilya Maximets <i.maximets@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@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
> 
> 
>

Patch

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);