Re: [dpdk-dev] [PATCH v5 1/3] ethdev: new xstats API add retrieving by ID

[Thomas Monjalon <thomas.monjalon@6wind.com>]

> --- a/app/proc_info/main.c
> +++ b/app/proc_info/main.c
>  		"  --stats: to display port statistics, enabled by default\n"
>  		"  --xstats: to display extended port statistics, disabled by "
>  			"default\n"
> -		"  --metrics: to display derived metrics of the ports, disabled by "
> -			"default\n"
> +		"  --xstats-name NAME: to display single xstat value by NAME\n"
> +		"  --xstats-ids IDLIST: to display xstat values by id. "
> +			"The argument is comma-separated list of xstat ids to print out.\n"
>  		"  --stats-reset: to reset port statistics\n"
>  		"  --xstats-reset: to reset port extended statistics\n"

Why removing --metrics? Is it a rebase mistake?

Please, could you introduce these new proc_info options in a separate patch?

> --- a/doc/guides/prog_guide/poll_mode_drv.rst
> +++ b/doc/guides/prog_guide/poll_mode_drv.rst
> +The extended statistics API allows a PMD to expose all statistics that are
> +available to it, including statistics that are unique to the device.
> +Each statistic has three properties ``name``, ``id`` and ``value``:
> +
> +* ``name``: A human readable string formatted by the scheme detailed below.
> +* ``id``: An integer that represents only that statistic.

Suggestion: we could say that the id/name pair may change from a device
to another one.

> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
>  int
> -rte_eth_xstats_get_names(uint8_t port_id,
> -	struct rte_eth_xstat_name *xstats_names,
> -	unsigned size)
> +rte_eth_xstats_get_id_by_name(uint8_t port_id, const char *xstat_name,
> +		uint64_t *id)
[...]
> +/* retrieve ethdev extended statistics */
> +int
> +rte_eth_xstats_get_v22(uint8_t port_id, struct rte_eth_xstat *xstats,
> +	unsigned int n)

I do not manage to review the function changes because of the mix
of versioning old functions and introduce new ones.
It would have been simpler to separate these two steps in separate patches.

> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> -	eth_xstats_get_names_t     xstats_get_names;
> -	/**< Get names of extended statistics. */
> +	eth_xstats_get_names_t	   xstats_get_names;
> +	/**< Get names of extended device statistics. */

It seems a space alignment has been removed here.

> - * Retrieve names of extended statistics of an Ethernet device.
> +* Gets the ID of a statistic from its name.
> +*
> +* Note this function searches for the statistics using string compares, and
> +* as such should not be used on the fast-path. For fast-path retrieval of
> +* specific statistics, store the ID as provided in *id* from this function,
> +* and pass the ID to rte_eth_xstats_get()

Good to note :)

> -int rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstat *xstats,
> -		unsigned n);
[...]
> +int rte_eth_xstats_get_v1705(uint8_t port_id, uint64_t *ids, uint64_t *values,
> +	unsigned int n);
> +
> +int rte_eth_xstats_get(uint8_t port_id, uint64_t *ids, uint64_t *values,
> +	unsigned int n);

It is an API change.
Please reference it in the release notes.

> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -159,5 +159,10 @@ DPDK_17.05 {
>  	global:
>  
>  	rte_eth_find_next;
> +	rte_eth_xstats_get_names;
> +	rte_eth_xstats_get;
> +	rte_eth_xstats_get_all;
> +	rte_eth_xstats_get_names_all;
> +	rte_eth_xstats_get_id_by_name;

Please keep alphabetical order.


Conclusion: The API looks right.
I have not checked the implementation and hope there will be less bugs
than when introducing the old one ;)