-----Original Message-----
From: Ferruh Yigit <ferruh.yi...@intel.com>
Sent: Tuesday, 26 January 2021 20:27
To: Liron Himi <lir...@marvell.com>; Jerin Jacob Kollanukkaran
<jer...@marvell.com>
Cc: dev@dpdk.org; Yuri Chipchev <yu...@marvell.com>
Subject: [EXT] Re: [dpdk-dev] [PATCH v2 08/37] net/mvpp2: extend xstats support
External Email
----------------------------------------------------------------------
On 1/22/2021 7:18 PM, lir...@marvell.com wrote:
> From: Yuri Chipchev <yu...@marvell.com>
>
> Add xstats_by_id callbacks
>
> Signed-off-by: Yuri Chipchev <yu...@marvell.com>
> Reviewed-by: Liron Himi <lir...@marvell.com>
> ---
> drivers/net/mvpp2/mrvl_ethdev.c | 98 ++++++++++++++++++++++++++++++++-
> 1 file changed, 95 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/mvpp2/mrvl_ethdev.c
> b/drivers/net/mvpp2/mrvl_ethdev.c index e4ec343d5..ba1882266 100644
> --- a/drivers/net/mvpp2/mrvl_ethdev.c
> +++ b/drivers/net/mvpp2/mrvl_ethdev.c
> @@ -173,6 +173,8 @@ static struct {
> MRVL_XSTATS_TBL_ENTRY(tx_errors)
> };
>
> +#define MRVL_NUM_XSTATS RTE_DIM(mrvl_xstats_tbl)
> +
> static inline void
> mrvl_fill_shadowq(struct mrvl_shadow_txq *sq, struct rte_mbuf *buf)
> {
> @@ -1376,7 +1378,7 @@ mrvl_xstats_get(struct rte_eth_dev *dev,
> return 0;
>
> pp2_ppio_get_statistics(priv->ppio, &ppio_stats, 0);
> - for (i = 0; i < n && i < RTE_DIM(mrvl_xstats_tbl); i++) {
> + for (i = 0; i < n && i < MRVL_NUM_XSTATS; i++) {
> uint64_t val;
>
> if (mrvl_xstats_tbl[i].size == sizeof(uint32_t)) @@ -1430,9
> +1432,9 @@ mrvl_xstats_get_names(struct rte_eth_dev *dev __rte_unused,
> unsigned int i;
>
> if (!xstats_names)
> - return RTE_DIM(mrvl_xstats_tbl);
> + return MRVL_NUM_XSTATS;
>
> - for (i = 0; i < size && i < RTE_DIM(mrvl_xstats_tbl); i++)
> + for (i = 0; i < size && i < MRVL_NUM_XSTATS; i++)
> strlcpy(xstats_names[i].name, mrvl_xstats_tbl[i].name,
> RTE_ETH_XSTATS_NAME_SIZE);
>
> @@ -2015,6 +2017,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) {
> + struct rte_eth_xstat xstats[MRVL_NUM_XSTATS];
> + uint16_t i;
> +
> + if (n < MRVL_NUM_XSTATS && ids == NULL)
> + return MRVL_NUM_XSTATS;
> +
> + if (n > MRVL_NUM_XSTATS)
> + return -EINVAL;
Why 'n > MRVL_NUM_XSTATS' is an error? I am not aware of this kind of
restriction, can you please point if I am missing it.
> +
> + if (values == NULL)
> + return -ENOMEM;
> +
> + mrvl_xstats_get(dev, xstats, n);
> +
What if 'n' is a small number, like in a case only single id is requested, so
n==1, will above work?
Overall there is ethdev level implementation of "_by_id" APIs, if driver has
'.xstats_get' & '.xstats_get_names' implemented.
Unless driver has more performant way to get these ids, I suggest using the
ethdev layer ones, and drop these.
[L.H.] missed that. In that case I will drop this patch in v3
> + for (i = 0; i < MRVL_NUM_XSTATS; i++) {
> + if (ids[i] >= MRVL_NUM_XSTATS) {
> + MRVL_LOG(ERR, "id value is not valid\n");
> + return -EINVAL;
> + }
> + values[i] = xstats[ids[i]].value;
> + }
Why this loop goes up to 'MRVL_NUM_XSTATS', does it assumes "n ==
MRVL_NUM_XSTATS", if so this assumption can be wrong.
> +
> + 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) {
> + struct rte_eth_xstat_name names[MRVL_NUM_XSTATS];
> + uint16_t i;
> +
> + if (size < MRVL_NUM_XSTATS && ids == NULL)
> + return MRVL_NUM_XSTATS;
> +
> + if (size > MRVL_NUM_XSTATS)
> + return -EINVAL;
> +
> + if (xstats_names == NULL)
> + return -ENOMEM;
> +
> + mrvl_xstats_get_names(dev, names, size);
> +
> + for (i = 0; i < MRVL_NUM_XSTATS; i++) {
> + if (ids[i] >= MRVL_NUM_XSTATS) {
> + MRVL_LOG(ERR, "id value is not valid");
> + return -EINVAL;
> + }
> + strncpy(xstats_names[i].name, names[ids[i]].name,
> + sizeof(xstats_names[i].name));
> + }
> +
> + return size;
> +}
> +
> /**
> * DPDK callback to get rte_mtr callbacks.
> *
> @@ -2090,6 +2180,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,
> };
>
