Re: [dpdk-dev] [PATCH v2 6/8] net/mrvl: add extended statistics

[Tomasz Duszynski <tdu@semihalf.com>]

On Wed, Mar 14, 2018 at 05:21:07PM +0000, Ferruh Yigit wrote:
> On 3/12/2018 8:42 AM, Tomasz Duszynski wrote:
> > Add extended statistics implementation.
> >
> > Signed-off-by: Natalie Samsonov <nsamsono@marvell.com>
> > Signed-off-by: Tomasz Duszynski <tdu@semihalf.com>
>
> <...>
>
> > @@ -1674,6 +1784,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];
> > +	int ret;
> > +
> > +	if (!ids) {
>
> You will not get NULL ids, this case covered by ethdev layer, for both by_id()
> functions.
>
> > +		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) {
> > +			RTE_LOG(ERR, PMD, "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];
> > +
> > +	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) {
> > +			RTE_LOG(ERR, PMD, "id value is not valid");
> > +			return -1;
> > +		}
> > +
> > +		snprintf(xstats_names[i].name, RTE_ETH_XSTATS_NAME_SIZE,
> > +			 "%s", names[ids[i]].name);
> > +	}
> > +
> > +	return size;
> > +}
>
> Specific to *_by_id() implementations, please check ethdev layer APIs for these
> devops, they already do same thing as you did here.
>
> These devops are to access specific ids efficiently with support of PMD, if you
> don't have a quick way to access to an extended stat by id, you may just left
> these unimplemented and abstraction layer will do the work for you, it is up to you.

Good point. Since *_by_id() are using xstats_get_names()/xstats_get()
anyway they do not provide a real speedup. I'll drop that in v3.

>
>
> Thanks,
> ferruh

--
- Tomasz Duszyński