Re: [dpdk-dev] [PATCH v2] ethdev: fix xstats retrieve by id API

[Ivan Malov <Ivan.Malov@oktetlabs.ru>]

On 10/12/2017 2:31 PM, Lee Daly wrote:
> From: Lee <lee.daly@intel.com>
>
> Fix xstats functions, rte_eth_xstats_get_names_by_id()
> and rte_eth_xstats_get_by_id(), in current implementation
> ethdev level reads all xstat values and filters out
> the ones requested by the application. This behavior doesn't
> benefit from PMD ops and doesn't provide the benefit the
> API was created in the first place for. APIs are also unnecessarily
> complicated. Both APIs have different returns for the same params.
>
> In this fix, instead of reading all the stats and finding the
> requested value, drivers can provide ops to get selected xstats.
> API no longer crashes with certain params,
>
> rte_eth_get_by_id returned seg fault with
> "ids = NULL && values != NULL && n<max"
> rte_eth_get_names_by_id returned seg fault with
> "ids = NULL && values != NULL && n=0"
> These now return max number of stats available, matching the other API.
>
> rte_eth_get_by_id returned seg fault with
> "ids != NULL && values = NULL && n<max"
> This now returns -22,(EINVAL).
>
> Standardized variable/parameter names between the 2 APIs.
>
> Overall code complexity reduced.
>
> Fixes: 79c913a42f0e ("ethdev: retrieve xstats by ID")
> Cc: kubax.kozak@intel.com
>
> Signed-off-by: Lee Daly <lee.daly@intel.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

I have a serious concern regarding the patch. There is a common scenario
of rte_eth_xstats_get_names_by_id() usage, and the patch breaks it.
Typically, the function is called with 'ids = NULL' and 'xstats_names = NULL'
in order to get the number of figures. Then the function is called one more
time with appropriate storage for 'xstats_names'. According to the patch,
on the former step get_xstats_count() is called to count the figures available.
The resulting number counts for PMD statistics + RTE_NB_STATS + some amount of
per-queue figures. However, on the latter step the following may take place:

> +	if (dev->dev_ops->xstats_get_names_by_id != NULL)
> +		return (*dev->dev_ops->xstats_get_names_by_id)(
> +				dev, xstats_names, ids, size);

This obviously means that in the case when 'xstats_get_names_by_id' is present,
it will be called directly, and RTE statistics will not be filled in the storage
before the PMD figures. Accordingly, the value returned by the function in this
case will count for PMD figures only (i.e. will be less than the value obtained
on the first step). This is an obvious malfunction, and it would be desirable
to provide a fix for that. I hope for your understanding.

-- 
Best regards,
Ivan