Re: [dpdk-dev] [EXT] Re: [PATCH v1 09/38] net/mvpp2: extend xstats support

[Liron Himi <lironh@marvell.com>]

Hi Jerin

PSB

-----Original Message-----
From: Jerin Jacob <jerinjacobk@gmail.com> 
Sent: Monday, 11 January 2021 16:50
To: Michael Shamis <michaelsh@marvell.com>
Cc: Liron Himi <lironh@marvell.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org; Yuri Chipchev <yuric@marvell.com>
Subject: [EXT] Re: [dpdk-dev] [PATCH v1 09/38] net/mvpp2: extend xstats support

External Email

----------------------------------------------------------------------
On Wed, Dec 23, 2020 at 3:14 PM Michael Shamis <michaelsh@marvell.com> wrote:
>
> Reviewed-by: Michael Shamis <michaelsh@marvell.com>
>
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of lironh@marvell.com
> Sent: Wednesday, December 2, 2020 12:12 PM
> To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> Cc: dev@dpdk.org; Yuri Chipchev <yuric@marvell.com>; Liron Himi 
> <lironh@marvell.com>
> Subject: [dpdk-dev] [PATCH v1 09/38] net/mvpp2: extend xstats support
>
> From: Yuri Chipchev <yuric@marvell.com>
>
> add xstats_by_id callbacks
>
> Signed-off-by: Yuri Chipchev <yuric@marvell.com>
> Reviewed-by: Liron Himi <lironh@marvell.com>
> ---
>  drivers/net/mvpp2/mrvl_ethdev.c | 90 
> +++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>
> diff --git a/drivers/net/mvpp2/mrvl_ethdev.c 
> b/drivers/net/mvpp2/mrvl_ethdev.c index 46e7260be..e81d5ee91 100644
> --- a/drivers/net/mvpp2/mrvl_ethdev.c
> +++ b/drivers/net/mvpp2/mrvl_ethdev.c
> @@ -2027,6 +2027,94 @@ mrvl_eth_filter_ctrl(struct rte_eth_dev *dev __rte_unused,
>         }
>  }
>
> +/**
> + * DPDK callback to get xstats by id.
> + *
> + * @param dev
> + *   Pointer to the device structure.
> + * @param ids
> + *   Pointer to the ids table.
> + * @param values
> + *   Pointer to the values table.
> + * @param n
> + *   Values table size.
> + * @returns
> + *   Number of read values, negative value otherwise.
> + */
> +static int
> +mrvl_xstats_get_by_id(struct rte_eth_dev *dev, const uint64_t *ids,
> +                     uint64_t *values, unsigned int n) {
> +       unsigned int i, num = RTE_DIM(mrvl_xstats_tbl);
> +       uint64_t vals[n];

Please add boundary checks for "n".

> +       int ret;
> +
> +       if (!ids) {

According to spec, if ids == NULL, we just need to return the size of max objects, no need to copy to values.
[L.H.] from the spec
* @param ids
 *   A pointer to an ids array passed by application. This tells which
 *   statistics values function should retrieve. This parameter
 *   can be set to NULL if size is 0. In this case function will retrieve
 *   all available statistics.

So from the spec it looks like the code is correct, but actually how the driver knows the size of the 'values' array?
Maybe it is good to treat this condition as 'error' and return the max object as you suggested.

> +               struct rte_eth_xstat xstats[num];
> +               int j;
> +
> +               ret = mrvl_xstats_get(dev, xstats, num);
> +               for (j = 0; j < ret; i++)
> +                       values[j] = xstats[j].value;
> +
> +               return ret;
> +       }
> +
> +       ret = mrvl_xstats_get_by_id(dev, NULL, vals, n);
> +       if (ret < 0)
> +               return ret;
> +
> +       for (i = 0; i < n; i++) {
> +               if (ids[i] >= num) {
> +                       MRVL_LOG(ERR, "id value is not valid\n");
> +                       return -1;
> +               }
> +
> +               values[i] = vals[ids[i]];
> +       }
> +
> +       return n;
> +}
> +
> +/**
> + * DPDK callback to get xstats names by ids.
> + *
> + * @param dev
> + *   Pointer to the device structure.
> + * @param xstats_names
> + *   Pointer to table with xstats names.
> + * @param ids
> + *   Pointer to table with ids.
> + * @param size
> + *   Xstats names table size.
> + * @returns
> + *   Number of names read, negative value otherwise.
> + */
> +static int
> +mrvl_xstats_get_names_by_id(struct rte_eth_dev *dev,
> +                           struct rte_eth_xstat_name *xstats_names,
> +                           const uint64_t *ids, unsigned int size) {
> +       unsigned int i, num = RTE_DIM(mrvl_xstats_tbl);
> +       struct rte_eth_xstat_name names[num];

Above comments appliable here too.

> +
> +       if (!ids)
> +               return mrvl_xstats_get_names(dev, xstats_names, size);
> +
> +       mrvl_xstats_get_names(dev, names, size);
> +       for (i = 0; i < size; i++) {
> +               if (ids[i] >= num) {
> +                       MRVL_LOG(ERR, "id value is not valid");
> +                       return -1;
> +               }
> +
> +               snprintf(xstats_names[i].name, RTE_ETH_XSTATS_NAME_SIZE,
> +                        "%s", names[ids[i]].name);
> +       }
> +
> +       return size;
> +}
> +
>  /**
>   * DPDK callback to get rte_mtr callbacks.
>   *
> @@ -2102,6 +2190,8 @@ static const struct eth_dev_ops mrvl_ops = {
>         .rss_hash_update = mrvl_rss_hash_update,
>         .rss_hash_conf_get = mrvl_rss_hash_conf_get,
>         .filter_ctrl = mrvl_eth_filter_ctrl,
> +       .xstats_get_by_id = mrvl_xstats_get_by_id,
> +       .xstats_get_names_by_id = mrvl_xstats_get_names_by_id,
>         .mtr_ops_get = mrvl_mtr_ops_get,
>         .tm_ops_get = mrvl_tm_ops_get,  };
> --
> 2.28.0
>