[ovs-dev,1/2] netdev-dpdk: Fix statistics when changing Rx/Tx queues count.

When changing number of Rx or Tx queues, per queue basic stats can be
renumbered in DPDK ethdev layer [1].

OVS maintains an internal xstats IDs cache that was refreshed when a
cached id was not valid anymore (in netdev_dpdk_get_custom_stats) or if
a new DPDK port was created.
This did not handle changes of Rx/Tx queues count.

For example, with a mlx5 port:
$ ovs-vsctl set interface dpdk0 options:n_rxq=2
$ ovs-vsctl get interface dpdk0 statistics |
  sed -e 's#[{}]##g' -e 's#, #\n#g' |
  grep rx_q._errors
rx_q0_errors=0

Move the cache filling after reconfiguring and starting the port.
There is no need to flush the cache in netdev_dpdk_get_custom_stats.

While at it, the xstats code can be cleaned up:
- remove wrong or Lapalissade comments,
- don't check x*alloc return value,
- expect that consecutive calls to xstats API return the same number of
  elements,
- only write to dev-> when all DPDK calls succeeded,
- add missing lock annotations to netdev_dpdk_clear_xstats and
  netdev_dpdk_get_xstat_name,

1: https://git.dpdk.org/dpdk/tree/lib/librte_ethdev/rte_ethdev.c?h=v20.11#n2696

Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-November/389456.html
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/netdev-dpdk.c | 160 ++++++++++++++++++----------------------------
 1 file changed, 62 insertions(+), 98 deletions(-)

On 02/12/2021 21:16, David Marchand wrote:
> When changing number of Rx or Tx queues, per queue basic stats can be
> renumbered in DPDK ethdev layer [1].
> 
> OVS maintains an internal xstats IDs cache that was refreshed when a
> cached id was not valid anymore (in netdev_dpdk_get_custom_stats) or if
> a new DPDK port was created.
> This did not handle changes of Rx/Tx queues count.
> 
> For example, with a mlx5 port:
> $ ovs-vsctl set interface dpdk0 options:n_rxq=2
> $ ovs-vsctl get interface dpdk0 statistics |
>    sed -e 's#[{}]##g' -e 's#, #\n#g' |
>    grep rx_q._errors
> rx_q0_errors=0
> 
> Move the cache filling after reconfiguring and starting the port.
> There is no need to flush the cache in netdev_dpdk_get_custom_stats.
> 
> While at it, the xstats code can be cleaned up:
> - remove wrong or Lapalissade comments,
> - don't check x*alloc return value,
> - expect that consecutive calls to xstats API return the same number of
>    elements,
> - only write to dev-> when all DPDK calls succeeded,
> - add missing lock annotations to netdev_dpdk_clear_xstats and
>    netdev_dpdk_get_xstat_name,
> 
> 1: https://git.dpdk.org/dpdk/tree/lib/librte_ethdev/rte_ethdev.c?h=v20.11#n2696
> 
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-November/389456.html
> Signed-off-by: David Marchand <david.marchand@redhat.com>

I tested it and the rx q errors were reported for the right number of qs 
when increasing/decreasing num of qs. Code lgtm.

Acked-by: Kevin Traynor <ktraynor@redhat.com>

As a side note, I noticed that xstats only reported for q0-q15, but as 
we discussed that is a DPDK configuration item.

> ---
>   lib/netdev-dpdk.c | 160 ++++++++++++++++++----------------------------
>   1 file changed, 62 insertions(+), 98 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index ca92c947a2..51bb41551b 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -540,6 +540,7 @@ static void netdev_dpdk_vhost_destruct(struct netdev *netdev);
>   
>   static int netdev_dpdk_get_sw_custom_stats(const struct netdev *,
>                                              struct netdev_custom_stats *);
> +static void netdev_dpdk_configure_xstats(struct netdev_dpdk *dev);
>   static void netdev_dpdk_clear_xstats(struct netdev_dpdk *dev);
>   
>   int netdev_dpdk_get_vid(const struct netdev_dpdk *dev);
> @@ -1161,6 +1162,8 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>       }
>       dev->started = true;
>   
> +    netdev_dpdk_configure_xstats(dev);
> +
>       rte_eth_promiscuous_enable(dev->port_id);
>       rte_eth_allmulticast_enable(dev->port_id);
>   
> @@ -1559,23 +1562,19 @@ netdev_dpdk_dealloc(struct netdev *netdev)
>   
>   static void
>   netdev_dpdk_clear_xstats(struct netdev_dpdk *dev)
> +    OVS_REQUIRES(dev->mutex)
>   {
> -    /* 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;
> -    }
> +    free(dev->rte_xstats_names);
> +    dev->rte_xstats_names = NULL;
> +    dev->rte_xstats_names_size = 0;
> +    free(dev->rte_xstats_ids);
> +    dev->rte_xstats_ids = NULL;
> +    dev->rte_xstats_ids_size = 0;
>   }
>   
> -static const char*
> +static const char *
>   netdev_dpdk_get_xstat_name(struct netdev_dpdk *dev, uint64_t id)
> +    OVS_REQUIRES(dev->mutex)
>   {
>       if (id >= dev->rte_xstats_names_size) {
>           return "UNKNOWN";
> @@ -1583,101 +1582,70 @@ netdev_dpdk_get_xstat_name(struct netdev_dpdk *dev, uint64_t id)
>       return dev->rte_xstats_names[id].name;
>   }
>   
> -static bool
> +static void
>   netdev_dpdk_configure_xstats(struct netdev_dpdk *dev)
>       OVS_REQUIRES(dev->mutex)
>   {
> +    struct rte_eth_xstat_name *rte_xstats_names = NULL;
> +    struct rte_eth_xstat *rte_xstats = NULL;
> +    int rte_xstats_names_size;
>       int rte_xstats_len;
> -    bool ret;
> -    struct rte_eth_xstat *rte_xstats;
> -    uint64_t id;
> -    int xstats_no;
>       const char *name;
> +    uint64_t id;
>   
> -    /* 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). */
> +    netdev_dpdk_clear_xstats(dev);
>   
> -    ret = false;
> -    rte_xstats = NULL;
> +    rte_xstats_names_size = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
> +    if (rte_xstats_names_size < 0) {
> +        VLOG_WARN("Cannot get XSTATS names for port: "DPDK_PORT_ID_FMT,
> +                  dev->port_id);
> +        goto out;
> +    }
>   
> -    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);
> +    rte_xstats_names = xcalloc(rte_xstats_names_size,
> +                               sizeof *rte_xstats_names);
> +    rte_xstats_len = rte_eth_xstats_get_names(dev->port_id,
> +                                              rte_xstats_names,
> +                                              rte_xstats_names_size);
> +    if (rte_xstats_len < 0 || rte_xstats_len != rte_xstats_names_size) {
> +        VLOG_WARN("Cannot get XSTATS names for port: "DPDK_PORT_ID_FMT,
> +                  dev->port_id);
> +        goto out;
> +    }
>   
> -        if (dev->rte_xstats_names_size < 0) {
> -            VLOG_WARN("Cannot get XSTATS for port: "DPDK_PORT_ID_FMT,
> -                      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: "
> -                              DPDK_PORT_ID_FMT, dev->port_id);
> -                    goto out;
> -                } else if (rte_xstats_len != dev->rte_xstats_names_size) {
> -                    VLOG_WARN("XSTATS size doesn't match for port: "
> -                              DPDK_PORT_ID_FMT, dev->port_id);
> -                    goto out;
> -                }
> +    rte_xstats = xcalloc(rte_xstats_names_size, sizeof *rte_xstats);
> +    rte_xstats_len = rte_eth_xstats_get(dev->port_id, rte_xstats,
> +                                        rte_xstats_names_size);
> +    if (rte_xstats_len < 0 || rte_xstats_len != rte_xstats_names_size) {
> +        VLOG_WARN("Cannot get XSTATS for port: "DPDK_PORT_ID_FMT,
> +                  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: "
> -                              DPDK_PORT_ID_FMT, dev->port_id);
> -                }
> +    dev->rte_xstats_names = rte_xstats_names;
> +    rte_xstats_names = NULL;
> +    dev->rte_xstats_names_size = rte_xstats_names_size;
>   
> -                free(rte_xstats);
> -            }
> +    dev->rte_xstats_ids = xcalloc(rte_xstats_names_size,
> +                                  sizeof *dev->rte_xstats_ids);
> +    for (unsigned int i = 0; i < rte_xstats_names_size; 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[dev->rte_xstats_ids_size] = id;
> +            dev->rte_xstats_ids_size++;
>           }
> -    } else {
> -        /* Already configured */
> -        ret = true;
>       }
>   
>   out:
> -    if (!ret) {
> -        netdev_dpdk_clear_xstats(dev);
> -    }
> -    return ret;
> +    free(rte_xstats);
> +    free(rte_xstats_names);
>   }
>   
>   static bool
> @@ -1963,7 +1931,6 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>                       dev->devargs = xstrdup(new_devargs);
>                       dev->port_id = new_port_id;
>                       netdev_request_reconfigure(&dev->up);
> -                    netdev_dpdk_clear_xstats(dev);
>                       err = 0;
>                   }
>               }
> @@ -3196,7 +3163,7 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev,
>   
>       ovs_mutex_lock(&dev->mutex);
>   
> -    if (netdev_dpdk_configure_xstats(dev)) {
> +    if (dev->rte_xstats_ids_size > 0) {
>           uint64_t *values = xcalloc(dev->rte_xstats_ids_size,
>                                      sizeof(uint64_t));
>   
> @@ -3223,9 +3190,6 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev,
>           } else {
>               VLOG_WARN("Cannot get XSTATS values for port: "DPDK_PORT_ID_FMT,
>                         dev->port_id);
> -            /* Let's clear statistics cache, so it will be
> -             * reconfigured */
> -            netdev_dpdk_clear_xstats(dev);
>           }
>   
>           free(values);
>