[dpdk-dev] [PATCH v6 0/5] Extended xstats API in ethdev library to allow grouping of stats

Olivier Matz olivier.matz at 6wind.com
Mon Apr 24 14:32:11 CEST 2017


Hi,

On Thu, 20 Apr 2017 22:31:35 +0200, Thomas Monjalon <thomas at monjalon.net> wrote:
> 13/04/2017 16:59, Kuba Kozak:
> > Extended xstats API in ethdev library to allow grouping of stats logically
> > so they can be retrieved per logical grouping  managed by the application.
> > Changed existing functions rte_eth_xstats_get_names and rte_eth_xstats_get
> > to use a new list of arguments: array of ids and array of values.
> > ABI versioning mechanism was used to support backward compatibility.
> > Introduced two new functions rte_eth_xstats_get_all and
> > rte_eth_xstats_get_names_all which keeps functionality of the previous
> > ones (respectively rte_eth_xstats_get and rte_eth_xstats_get_names)
> > but use new API inside. Both functions marked as deprecated.
> > Introduced new function: rte_eth_xstats_get_id_by_name to retrieve
> > xstats ids by its names.
> > Extended functionality of proc_info application:
> > --xstats-name NAME: to display single xstat value by NAME
> > Updated test-pmd application to use new API.  
> 
> Applied, thanks

I'm adapting my application to the upcoming dpdk 17.05. I see
several problems with this patchset:

- the API of rte_eth_xstats_get() and rte_eth_xstats_get_names()
  has been modified, and from what I see it was not announced.
  It looks that ABI is preserved however.

- the new functions rte_eth_xstats_get_all() and
  rte_eth_xstats_get_names_all() are marked as deprecated, which
  looks strange for new functions.

About the new api:

int rte_eth_xstats_get(uint8_t port_id, uint64_t *ids, uint64_t *values,
       unsigned int n);

int rte_eth_xstats_get_names(uint8_t port_id,
       struct rte_eth_xstat_name *xstats_names, unsigned int size,
       uint64_t *ids);

- the argument "id" is not at the same place

- why having "size" in one function and "n" in the second (it was
  renamed in the patch)?

- the argument "id" should be const

- a table of uint64_t is returned in place of the struct rte_eth_xstat
  table: if no ids are given, the driver cannot return partial or
  disordered stats anymore. See
  513c78ae3fd6 ("ethdev: fix extended statistics name index")


So, I wonder if it wouldn't be more simple to keep the old
API intact (it would avoid unannounced breakage). The new feature
can be implemented in an additional API:

 rte_eth_xstats_get_by_id(uint8_t port_id, const uint64_t *ids,
       uint64_t *values, unsigned int size)
 rte_eth_xstats_get_names_by_id(uint8_t port_id, const uint64_t *ids,
       struct rte_eth_xstat_name *xstats_names, unsigned int size)

Or:

 rte_eth_xstats_get_by_id(uint8_t port_id, const uint64_t *ids,
       struct rte_eth_xstat *values, unsigned int size)
 rte_eth_xstats_get_names_by_id(uint8_t port_id, const uint64_t *ids,
       struct rte_eth_xstat_name *xstats_names, unsigned int size)

 (which would allow to deprecate the old API, but I'm not sure
  we need to)


Can we fix that for 17.05?

Regards,
Olivier


More information about the dev mailing list