[v5,2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs

  From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Update xstats by IDs callbacks documentation in accordance with
ethdev usage of these callbacks. Document valid combinations of
input arguments to make driver implementation simpler.

Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)
  

On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Update xstats by IDs callbacks documentation in accordance with
> ethdev usage of these callbacks. Document valid combinations of
> input arguments to make driver implementation simpler.
> 
> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 40e474aa7e..c89eefcc42 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat *stats, unsigned int n);
>  /**< @internal Get extended stats of an Ethernet device. */
>  
> +/**
> + * @internal
> + * Get extended stats of an Ethernet device.
> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param ids
> + *   IDs array to retrieve specific statistics. Must not be NULL.
> + * @param values
> + *   A pointer to a table to be filled with device statistics values.
> + *   Must not be NULL.
> + * @param n
> + *   Element count in @p ids and @p values.
> + *
> + * @return
> + *   - A number of filled in stats.
> + *   - A negative value on error.
> + */
>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>  				      const uint64_t *ids,
>  				      uint64_t *values,
>  				      unsigned int n);
> -/**< @internal Get extended stats of an Ethernet device. */
>  
>  /**
>   * @internal
> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>  /**< @internal Get names of extended stats of an Ethernet device. */
>  
> +/**
> + * @internal
> + * Get names of extended stats of an Ethernet device.
> + * For name count, set @p xstats_names and @p ids to NULL.

Why limiting this behavior to 'xstats_get_names_by_id'?

Internally 'xstats_get_names_by_id' is used to get the count, but I think this
is not intentionally selected, just one of the xstats_*_by_id dev_ops used.

I think it is confusing to have this support for one of the '_by_id' dev_ops but
not for other. Why not require both to support returning 'count'?

> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param xstats_names
> + *   An rte_eth_xstat_name array of at least *size* elements to
> + *   be filled. Can be NULL together with @p ids to retrieve number of
> + *   available statistics.
> + * @param ids
> + *   IDs array to retrieve specific statistics. Can be NULL together
> + *   with @p xstats_names to retrieve number of available statistics.
> + * @param size
> + *   Element count in @p ids and @p xstats_names.
> + *
> + * @return
> + *   - A number of filled in stats if both xstats_names and ids are not NULL.
> + *   - A number of available stats if both xstats_names and ids are NULL.
> + *   - A negative value on error.
> + */
>  typedef int (*eth_xstats_get_names_by_id_t)(struct rte_eth_dev *dev,
>  	struct rte_eth_xstat_name *xstats_names, const uint64_t *ids,
>  	unsigned int size);
> -/**< @internal Get names of extended stats of an Ethernet device. */
>  
>  typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev,
>  					     uint16_t queue_id,
>
  

On 9/28/21 7:50 PM, Ferruh Yigit wrote:
> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Update xstats by IDs callbacks documentation in accordance with
>> ethdev usage of these callbacks. Document valid combinations of
>> input arguments to make driver implementation simpler.
>>
>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>> ---
>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index 40e474aa7e..c89eefcc42 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>  	struct rte_eth_xstat *stats, unsigned int n);
>>  /**< @internal Get extended stats of an Ethernet device. */
>>  
>> +/**
>> + * @internal
>> + * Get extended stats of an Ethernet device.
>> + *
>> + * @param dev
>> + *   ethdev handle of port.
>> + * @param ids
>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>> + * @param values
>> + *   A pointer to a table to be filled with device statistics values.
>> + *   Must not be NULL.
>> + * @param n
>> + *   Element count in @p ids and @p values.
>> + *
>> + * @return
>> + *   - A number of filled in stats.
>> + *   - A negative value on error.
>> + */
>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>  				      const uint64_t *ids,
>>  				      uint64_t *values,
>>  				      unsigned int n);
>> -/**< @internal Get extended stats of an Ethernet device. */
>>  
>>  /**
>>   * @internal
>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>  
>> +/**
>> + * @internal
>> + * Get names of extended stats of an Ethernet device.
>> + * For name count, set @p xstats_names and @p ids to NULL.
> 
> Why limiting this behavior to 'xstats_get_names_by_id'?
> 
> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
> 
> I think it is confusing to have this support for one of the '_by_id' dev_ops but
> not for other. Why not require both to support returning 'count'?

Simply because it is dead code. There is no point to require
from driver to have dead code.
  

On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>
>>> Update xstats by IDs callbacks documentation in accordance with
>>> ethdev usage of these callbacks. Document valid combinations of
>>> input arguments to make driver implementation simpler.
>>>
>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>> ---
>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>> index 40e474aa7e..c89eefcc42 100644
>>> --- a/lib/ethdev/ethdev_driver.h
>>> +++ b/lib/ethdev/ethdev_driver.h
>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>  
>>> +/**
>>> + * @internal
>>> + * Get extended stats of an Ethernet device.
>>> + *
>>> + * @param dev
>>> + *   ethdev handle of port.
>>> + * @param ids
>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>> + * @param values
>>> + *   A pointer to a table to be filled with device statistics values.
>>> + *   Must not be NULL.
>>> + * @param n
>>> + *   Element count in @p ids and @p values.
>>> + *
>>> + * @return
>>> + *   - A number of filled in stats.
>>> + *   - A negative value on error.
>>> + */
>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>  				      const uint64_t *ids,
>>>  				      uint64_t *values,
>>>  				      unsigned int n);
>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>  
>>>  /**
>>>   * @internal
>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>  
>>> +/**
>>> + * @internal
>>> + * Get names of extended stats of an Ethernet device.
>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>
>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>
>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>
>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>> not for other. Why not require both to support returning 'count'?
> 
> Simply because it is dead code. There is no point to require
> from driver to have dead code.
> 

Let me step back a little, both ethdev APIs can be used to return xstats count
by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
'rte_eth_xstats_get_names_by_id()'
'rte_eth_xstats_get_by_id()'

But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
as said above I believe this selection is done unintentionally.


I am for below two options:

a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
simplifies driver code. As far as I remember I suggested this before, still I
prefer this one.

b) If we will support getting xstats count from '_by_id' dev_ops, I think both
should support it, to not make it more complex to figure out which one support
what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
count, not just one.
  

On 9/29/21 11:44 AM, Ferruh Yigit wrote:
> On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
>> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>
>>>> Update xstats by IDs callbacks documentation in accordance with
>>>> ethdev usage of these callbacks. Document valid combinations of
>>>> input arguments to make driver implementation simpler.
>>>>
>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>> ---
>>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>> index 40e474aa7e..c89eefcc42 100644
>>>> --- a/lib/ethdev/ethdev_driver.h
>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>>  
>>>> +/**
>>>> + * @internal
>>>> + * Get extended stats of an Ethernet device.
>>>> + *
>>>> + * @param dev
>>>> + *   ethdev handle of port.
>>>> + * @param ids
>>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>>> + * @param values
>>>> + *   A pointer to a table to be filled with device statistics values.
>>>> + *   Must not be NULL.
>>>> + * @param n
>>>> + *   Element count in @p ids and @p values.
>>>> + *
>>>> + * @return
>>>> + *   - A number of filled in stats.
>>>> + *   - A negative value on error.
>>>> + */
>>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>>  				      const uint64_t *ids,
>>>>  				      uint64_t *values,
>>>>  				      unsigned int n);
>>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>>  
>>>>  /**
>>>>   * @internal
>>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>>  
>>>> +/**
>>>> + * @internal
>>>> + * Get names of extended stats of an Ethernet device.
>>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>>
>>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>>
>>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>>
>>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>>> not for other. Why not require both to support returning 'count'?
>>
>> Simply because it is dead code. There is no point to require
>> from driver to have dead code.
>>
> 
> Let me step back a little, both ethdev APIs can be used to return xstats count
> by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
> 'rte_eth_xstats_get_names_by_id()'
> 'rte_eth_xstats_get_by_id()'
> 
> But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
> as said above I believe this selection is done unintentionally.
> 
> 
> I am for below two options:
> 
> a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
> count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
> simplifies driver code. As far as I remember I suggested this before, still I
> prefer this one.
> 
> b) If we will support getting xstats count from '_by_id' dev_ops, I think both
> should support it, to not make it more complex to figure out which one support
> what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
> count, not just one.
> 

In (b) do you suggest to change ethdev to use xstats_get_by_id
driver op if users asks for a number of xstats using
rte_eth_xstats_get_by_id()? It will complicate ethdev and
will complicate drivers. Just for the symmetry?

The patch does not change external API, does not change etcdev
bahaviour. It just clarify requirements on driver ops to
allow to check less in all drivers and support less cases
in all drivers.

If we make a one more step back, frankly speaking I think we
have too many functions to retrieve statistics. I can
understand from ethdev API point of view taking API stability
into account etc. But why do we have so many complicated
driver callbacks?

First of all I'd like to do one more cleanup:
change eth_xstats_get_names_by_id_t prototype to
have ids before xstats_names.
I.e. eth_xstats_get_by_id_t(dev, ids, values, size)
eth_xstats_get_names_by_id_t(dev, ids, names, size)

Second, merge eth_xstats_get_names_t and eth_xstats_get_names_by_id_t
callbacks to keep
name of the first, but prototype from the second.
The reason is equal functionality if ids==NULL,
shorter name of the first and optional ids (i.e. no
reason to mention optional parameter in name).
Drivers which do not implement by_id_t,
but implement eth_xstats_get_names_t, will simply
return ENOTSUP if ids!=NULL.

The case of values ops is more complicated,
however since:

2834  * There is an assumption that 'xstat_names' and 'xstats' arrays
are matched
2835  * by array index:
2836  *  xstats_names[i].name => xstats[i].value
2837  *
2838  * And the array index is same with id field of 'struct rte_eth_xstat':
2839  *  xstats[i].id == i
2840  *
2841  * This assumption makes key-value pair matching less flexible but
simpler.

we can switch to eth_xstats_get_by_id_t only callback as
well and fill in stats->id equal to index on ethdev layer.
However, it will require extra buffer for
uint64_t *values and copying in the rte_eth_xstats_get()
implementation. So, I doubt in this case.

In fact, it is sad that we still do not move forward
in accordance with Thomas presentation made 2 years ago.
  

On 9/29/2021 12:54 PM, Andrew Rybchenko wrote:
> On 9/29/21 11:44 AM, Ferruh Yigit wrote:
>> On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
>>> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>>>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>
>>>>> Update xstats by IDs callbacks documentation in accordance with
>>>>> ethdev usage of these callbacks. Document valid combinations of
>>>>> input arguments to make driver implementation simpler.
>>>>>
>>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>>> ---
>>>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>>> index 40e474aa7e..c89eefcc42 100644
>>>>> --- a/lib/ethdev/ethdev_driver.h
>>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>>>  
>>>>> +/**
>>>>> + * @internal
>>>>> + * Get extended stats of an Ethernet device.
>>>>> + *
>>>>> + * @param dev
>>>>> + *   ethdev handle of port.
>>>>> + * @param ids
>>>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>>>> + * @param values
>>>>> + *   A pointer to a table to be filled with device statistics values.
>>>>> + *   Must not be NULL.
>>>>> + * @param n
>>>>> + *   Element count in @p ids and @p values.
>>>>> + *
>>>>> + * @return
>>>>> + *   - A number of filled in stats.
>>>>> + *   - A negative value on error.
>>>>> + */
>>>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>>>  				      const uint64_t *ids,
>>>>>  				      uint64_t *values,
>>>>>  				      unsigned int n);
>>>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>>>  
>>>>>  /**
>>>>>   * @internal
>>>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>>>  
>>>>> +/**
>>>>> + * @internal
>>>>> + * Get names of extended stats of an Ethernet device.
>>>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>>>
>>>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>>>
>>>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>>>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>>>
>>>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>>>> not for other. Why not require both to support returning 'count'?
>>>
>>> Simply because it is dead code. There is no point to require
>>> from driver to have dead code.
>>>
>>
>> Let me step back a little, both ethdev APIs can be used to return xstats count
>> by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
>> 'rte_eth_xstats_get_names_by_id()'
>> 'rte_eth_xstats_get_by_id()'
>>
>> But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
>> as said above I believe this selection is done unintentionally.
>>
>>
>> I am for below two options:
>>
>> a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
>> count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
>> simplifies driver code. As far as I remember I suggested this before, still I
>> prefer this one.
>>
>> b) If we will support getting xstats count from '_by_id' dev_ops, I think both
>> should support it, to not make it more complex to figure out which one support
>> what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
>> count, not just one.
>>
> 
> In (b) do you suggest to change ethdev to use xstats_get_by_id
> driver op if users asks for a number of xstats using
> rte_eth_xstats_get_by_id()? It will complicate ethdev and
> will complicate drivers. Just for the symmetry?
> 

Not just for symmetry, but simplify the logic. Both dev_ops are very similar and
they are having different behavior with same parameters is confusing.
If you want to simplify the drivers why not go with option (a)? Option (a) also
doesn't change external API, and has only minor change in the ethdev layer.

> The patch does not change external API, does not change etcdev
> bahaviour. It just clarify requirements on driver ops to
> allow to check less in all drivers and support less cases
> in all drivers.
> 

It is not clarifying the requirements of dev_ops, but changing it. Previously
there was nothing saying only one of the '_by_id' dev_ops should support
returning element count.

> If we make a one more step back, frankly speaking I think we
> have too many functions to retrieve statistics. I can
> understand from ethdev API point of view taking API stability
> into account etc. But why do we have so many complicated
> driver callbacks?
> > First of all I'd like to do one more cleanup:
> change eth_xstats_get_names_by_id_t prototype to
> have ids before xstats_names.
> I.e. eth_xstats_get_by_id_t(dev, ids, values, size)
> eth_xstats_get_names_by_id_t(dev, ids, names, size)
> 

+1

> Second, merge eth_xstats_get_names_t and eth_xstats_get_names_by_id_t
> callbacks to keep
> name of the first, but prototype from the second.
> The reason is equal functionality if ids==NULL,
> shorter name of the first and optional ids (i.e. no
> reason to mention optional parameter in name).
> Drivers which do not implement by_id_t,
> but implement eth_xstats_get_names_t, will simply
> return ENOTSUP if ids!=NULL.
> 

No objection, _by_id() version is already superset of the other.

> The case of values ops is more complicated,
> however since:
> 
> 2834  * There is an assumption that 'xstat_names' and 'xstats' arrays
> are matched
> 2835  * by array index:
> 2836  *  xstats_names[i].name => xstats[i].value
> 2837  *
> 2838  * And the array index is same with id field of 'struct rte_eth_xstat':
> 2839  *  xstats[i].id == i
> 2840  *
> 2841  * This assumption makes key-value pair matching less flexible but
> simpler.
> 
> we can switch to eth_xstats_get_by_id_t only callback as
> well and fill in stats->id equal to index on ethdev layer.

When ids != NULL, the index from 'ids' can be used, isn't it.

> However, it will require extra buffer for
> uint64_t *values and copying in the rte_eth_xstats_get()
> implementation. So, I doubt in this case.
> 

Overall merging xstats _by_id APIs doesn't look bad idea, but since it will
require change in applications, I am not really sure if benefit worth the
trouble it brings to users.

> In fact, it is sad that we still do not move forward
> in accordance with Thomas presentation made 2 years ago.
> 

In my experience things don't move forward without proper plan (who, what, when).
  

On 9/30/21 3:08 PM, Ferruh Yigit wrote:
> On 9/29/2021 12:54 PM, Andrew Rybchenko wrote:
>> On 9/29/21 11:44 AM, Ferruh Yigit wrote:
>>> On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
>>>> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>>>>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>>
>>>>>> Update xstats by IDs callbacks documentation in accordance with
>>>>>> ethdev usage of these callbacks. Document valid combinations of
>>>>>> input arguments to make driver implementation simpler.
>>>>>>
>>>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>>>> ---
>>>>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>>>> index 40e474aa7e..c89eefcc42 100644
>>>>>> --- a/lib/ethdev/ethdev_driver.h
>>>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>>>>  
>>>>>> +/**
>>>>>> + * @internal
>>>>>> + * Get extended stats of an Ethernet device.
>>>>>> + *
>>>>>> + * @param dev
>>>>>> + *   ethdev handle of port.
>>>>>> + * @param ids
>>>>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>>>>> + * @param values
>>>>>> + *   A pointer to a table to be filled with device statistics values.
>>>>>> + *   Must not be NULL.
>>>>>> + * @param n
>>>>>> + *   Element count in @p ids and @p values.
>>>>>> + *
>>>>>> + * @return
>>>>>> + *   - A number of filled in stats.
>>>>>> + *   - A negative value on error.
>>>>>> + */
>>>>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>>>>  				      const uint64_t *ids,
>>>>>>  				      uint64_t *values,
>>>>>>  				      unsigned int n);
>>>>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>>>>  
>>>>>>  /**
>>>>>>   * @internal
>>>>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>>>>  
>>>>>> +/**
>>>>>> + * @internal
>>>>>> + * Get names of extended stats of an Ethernet device.
>>>>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>>>>
>>>>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>>>>
>>>>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>>>>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>>>>
>>>>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>>>>> not for other. Why not require both to support returning 'count'?
>>>>
>>>> Simply because it is dead code. There is no point to require
>>>> from driver to have dead code.
>>>>
>>>
>>> Let me step back a little, both ethdev APIs can be used to return xstats count
>>> by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
>>> 'rte_eth_xstats_get_names_by_id()'
>>> 'rte_eth_xstats_get_by_id()'
>>>
>>> But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
>>> as said above I believe this selection is done unintentionally.
>>>
>>>
>>> I am for below two options:
>>>
>>> a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
>>> count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
>>> simplifies driver code. As far as I remember I suggested this before, still I
>>> prefer this one.
>>>
>>> b) If we will support getting xstats count from '_by_id' dev_ops, I think both
>>> should support it, to not make it more complex to figure out which one support
>>> what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
>>> count, not just one.
>>>
>>
>> In (b) do you suggest to change ethdev to use xstats_get_by_id
>> driver op if users asks for a number of xstats using
>> rte_eth_xstats_get_by_id()? It will complicate ethdev and
>> will complicate drivers. Just for the symmetry?
>>
> 
> Not just for symmetry, but simplify the logic. Both dev_ops are very similar and

I'm sorry, but could you point of which logic you'd
like to simply. Less requirements on driver ops
means less code required inside.

> they are having different behavior with same parameters is confusing.

Ah, logic from PMD maintainer point of view. Does it
really worse to require extra code inside because of it?

> If you want to simplify the drivers why not go with option (a)? Option (a) also
> doesn't change external API, and has only minor change in the ethdev layer.

Is two extra patches in v6 (discussed below) a step
towards (a)?

>> The patch does not change external API, does not change etcdev
>> bahaviour. It just clarify requirements on driver ops to
>> allow to check less in all drivers and support less cases
>> in all drivers.
>>
> 
> It is not clarifying the requirements of dev_ops, but changing it. Previously
> there was nothing saying only one of the '_by_id' dev_ops should support
> returning element count.

I say "clarifying" since I adjust it a real source of
truth for internals - implementation, usage of these
callback by ethdev layer.

Yes, I agree that it changes documentation.

> 
>> If we make a one more step back, frankly speaking I think we
>> have too many functions to retrieve statistics. I can
>> understand from ethdev API point of view taking API stability
>> into account etc. But why do we have so many complicated
>> driver callbacks?
>>> First of all I'd like to do one more cleanup:
>> change eth_xstats_get_names_by_id_t prototype to
>> have ids before xstats_names.
>> I.e. eth_xstats_get_by_id_t(dev, ids, values, size)
>> eth_xstats_get_names_by_id_t(dev, ids, names, size)
>>
> 
> +1

See patch 3/4 in v6

> 
>> Second, merge eth_xstats_get_names_t and eth_xstats_get_names_by_id_t
>> callbacks to keep
>> name of the first, but prototype from the second.
>> The reason is equal functionality if ids==NULL,
>> shorter name of the first and optional ids (i.e. no
>> reason to mention optional parameter in name).
>> Drivers which do not implement by_id_t,
>> but implement eth_xstats_get_names_t, will simply
>> return ENOTSUP if ids!=NULL.
>>
> 
> No objection, _by_id() version is already superset of the other.

See patch 4/4 in v6

> 
>> The case of values ops is more complicated,
>> however since:
>>
>> 2834  * There is an assumption that 'xstat_names' and 'xstats' arrays
>> are matched
>> 2835  * by array index:
>> 2836  *  xstats_names[i].name => xstats[i].value
>> 2837  *
>> 2838  * And the array index is same with id field of 'struct rte_eth_xstat':
>> 2839  *  xstats[i].id == i
>> 2840  *
>> 2841  * This assumption makes key-value pair matching less flexible but
>> simpler.
>>
>> we can switch to eth_xstats_get_by_id_t only callback as
>> well and fill in stats->id equal to index on ethdev layer.
> 
> When ids != NULL, the index from 'ids' can be used, isn't it.

Yes, of course, but above documentation is for API without IDs.

> 
>> However, it will require extra buffer for
>> uint64_t *values and copying in the rte_eth_xstats_get()
>> implementation. So, I doubt in this case.
>>
> 
> Overall merging xstats _by_id APIs doesn't look bad idea, but since it will
> require change in applications, I am not really sure if benefit worth the
> trouble it brings to users.

Yes, I agree. I'm not going to change external API.

>> In fact, it is sad that we still do not move forward
>> in accordance with Thomas presentation made 2 years ago.
>>
> 
> In my experience things don't move forward without proper plan (who, what, when).
> 

+1
  

On 9/30/2021 3:01 PM, Andrew Rybchenko wrote:
> On 9/30/21 3:08 PM, Ferruh Yigit wrote:
>> On 9/29/2021 12:54 PM, Andrew Rybchenko wrote:
>>> On 9/29/21 11:44 AM, Ferruh Yigit wrote:
>>>> On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
>>>>> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>>>>>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>>>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>>>
>>>>>>> Update xstats by IDs callbacks documentation in accordance with
>>>>>>> ethdev usage of these callbacks. Document valid combinations of
>>>>>>> input arguments to make driver implementation simpler.
>>>>>>>
>>>>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>>>>> Cc: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>>>>> ---
>>>>>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>>>>> index 40e474aa7e..c89eefcc42 100644
>>>>>>> --- a/lib/ethdev/ethdev_driver.h
>>>>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>>>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>>>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>>>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>>>>>  
>>>>>>> +/**
>>>>>>> + * @internal
>>>>>>> + * Get extended stats of an Ethernet device.
>>>>>>> + *
>>>>>>> + * @param dev
>>>>>>> + *   ethdev handle of port.
>>>>>>> + * @param ids
>>>>>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>>>>>> + * @param values
>>>>>>> + *   A pointer to a table to be filled with device statistics values.
>>>>>>> + *   Must not be NULL.
>>>>>>> + * @param n
>>>>>>> + *   Element count in @p ids and @p values.
>>>>>>> + *
>>>>>>> + * @return
>>>>>>> + *   - A number of filled in stats.
>>>>>>> + *   - A negative value on error.
>>>>>>> + */
>>>>>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>>>>>  				      const uint64_t *ids,
>>>>>>>  				      uint64_t *values,
>>>>>>>  				      unsigned int n);
>>>>>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>>>>>  
>>>>>>>  /**
>>>>>>>   * @internal
>>>>>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>>>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>>>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>>>>>  
>>>>>>> +/**
>>>>>>> + * @internal
>>>>>>> + * Get names of extended stats of an Ethernet device.
>>>>>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>>>>>
>>>>>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>>>>>
>>>>>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>>>>>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>>>>>
>>>>>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>>>>>> not for other. Why not require both to support returning 'count'?
>>>>>
>>>>> Simply because it is dead code. There is no point to require
>>>>> from driver to have dead code.
>>>>>
>>>>
>>>> Let me step back a little, both ethdev APIs can be used to return xstats count
>>>> by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
>>>> 'rte_eth_xstats_get_names_by_id()'
>>>> 'rte_eth_xstats_get_by_id()'
>>>>
>>>> But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
>>>> as said above I believe this selection is done unintentionally.
>>>>
>>>>
>>>> I am for below two options:
>>>>
>>>> a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
>>>> count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
>>>> simplifies driver code. As far as I remember I suggested this before, still I
>>>> prefer this one.
>>>>
>>>> b) If we will support getting xstats count from '_by_id' dev_ops, I think both
>>>> should support it, to not make it more complex to figure out which one support
>>>> what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
>>>> count, not just one.
>>>>
>>>
>>> In (b) do you suggest to change ethdev to use xstats_get_by_id
>>> driver op if users asks for a number of xstats using
>>> rte_eth_xstats_get_by_id()? It will complicate ethdev and
>>> will complicate drivers. Just for the symmetry?
>>>
>>
>> Not just for symmetry, but simplify the logic. Both dev_ops are very similar and
> 
> I'm sorry, but could you point of which logic you'd
> like to simply. Less requirements on driver ops
> means less code required inside.
> 
>> they are having different behavior with same parameters is confusing.
> 
> Ah, logic from PMD maintainer point of view. Does it
> really worse to require extra code inside because of it?
> 
>> If you want to simplify the drivers why not go with option (a)? Option (a) also
>> doesn't change external API, and has only minor change in the ethdev layer.
> 
> Is two extra patches in v6 (discussed below) a step
> towards (a)?
> 

I am not sure of it, for (a) ethdev layer will handle getting all stats or
getting count using 'xstats_get_names' || 'xstats_get', and '*_by_id()' dev_ops
will become simpler since they won't accept NULL 'ids' & 'values'.

Also when dev_ops merged, it forces PMDs to implement the getting by ids, but
now it is covered by ethdev layer for the PMDs that doesn't implement '_by_id()'.

>>> The patch does not change external API, does not change etcdev
>>> bahaviour. It just clarify requirements on driver ops to
>>> allow to check less in all drivers and support less cases
>>> in all drivers.
>>>
>>
>> It is not clarifying the requirements of dev_ops, but changing it. Previously
>> there was nothing saying only one of the '_by_id' dev_ops should support
>> returning element count.
> 
> I say "clarifying" since I adjust it a real source of
> truth for internals - implementation, usage of these
> callback by ethdev layer.
> 
> Yes, I agree that it changes documentation.
> 
>>
>>> If we make a one more step back, frankly speaking I think we
>>> have too many functions to retrieve statistics. I can
>>> understand from ethdev API point of view taking API stability
>>> into account etc. But why do we have so many complicated
>>> driver callbacks?
>>>> First of all I'd like to do one more cleanup:
>>> change eth_xstats_get_names_by_id_t prototype to
>>> have ids before xstats_names.
>>> I.e. eth_xstats_get_by_id_t(dev, ids, values, size)
>>> eth_xstats_get_names_by_id_t(dev, ids, names, size)
>>>
>>
>> +1
> 
> See patch 3/4 in v6
> 
>>
>>> Second, merge eth_xstats_get_names_t and eth_xstats_get_names_by_id_t
>>> callbacks to keep
>>> name of the first, but prototype from the second.
>>> The reason is equal functionality if ids==NULL,
>>> shorter name of the first and optional ids (i.e. no
>>> reason to mention optional parameter in name).
>>> Drivers which do not implement by_id_t,
>>> but implement eth_xstats_get_names_t, will simply
>>> return ENOTSUP if ids!=NULL.
>>>
>>
>> No objection, _by_id() version is already superset of the other.
> 
> See patch 4/4 in v6
> 
>>
>>> The case of values ops is more complicated,
>>> however since:
>>>
>>> 2834  * There is an assumption that 'xstat_names' and 'xstats' arrays
>>> are matched
>>> 2835  * by array index:
>>> 2836  *  xstats_names[i].name => xstats[i].value
>>> 2837  *
>>> 2838  * And the array index is same with id field of 'struct rte_eth_xstat':
>>> 2839  *  xstats[i].id == i
>>> 2840  *
>>> 2841  * This assumption makes key-value pair matching less flexible but
>>> simpler.
>>>
>>> we can switch to eth_xstats_get_by_id_t only callback as
>>> well and fill in stats->id equal to index on ethdev layer.
>>
>> When ids != NULL, the index from 'ids' can be used, isn't it.
> 
> Yes, of course, but above documentation is for API without IDs.
> 
>>
>>> However, it will require extra buffer for
>>> uint64_t *values and copying in the rte_eth_xstats_get()
>>> implementation. So, I doubt in this case.
>>>
>>
>> Overall merging xstats _by_id APIs doesn't look bad idea, but since it will
>> require change in applications, I am not really sure if benefit worth the
>> trouble it brings to users.
> 
> Yes, I agree. I'm not going to change external API.
> 
>>> In fact, it is sad that we still do not move forward
>>> in accordance with Thomas presentation made 2 years ago.
>>>
>>
>> In my experience things don't move forward without proper plan (who, what, when).
>>
> 
> +1
>
  

On 9/30/21 6:30 PM, Ferruh Yigit wrote:
> On 9/30/2021 3:01 PM, Andrew Rybchenko wrote:
>> On 9/30/21 3:08 PM, Ferruh Yigit wrote:
>>> On 9/29/2021 12:54 PM, Andrew Rybchenko wrote:
>>>> On 9/29/21 11:44 AM, Ferruh Yigit wrote:
>>>>> On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
>>>>>> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>>>>>>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>>>>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>>>>
>>>>>>>> Update xstats by IDs callbacks documentation in accordance with
>>>>>>>> ethdev usage of these callbacks. Document valid combinations of
>>>>>>>> input arguments to make driver implementation simpler.
>>>>>>>>
>>>>>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>
>>>>>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>>>>>> ---
>>>>>>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>>>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>>>>>> index 40e474aa7e..c89eefcc42 100644
>>>>>>>> --- a/lib/ethdev/ethdev_driver.h
>>>>>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>>>>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>>>>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>>>>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>>>>>>  
>>>>>>>> +/**
>>>>>>>> + * @internal
>>>>>>>> + * Get extended stats of an Ethernet device.
>>>>>>>> + *
>>>>>>>> + * @param dev
>>>>>>>> + *   ethdev handle of port.
>>>>>>>> + * @param ids
>>>>>>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>>>>>>> + * @param values
>>>>>>>> + *   A pointer to a table to be filled with device statistics values.
>>>>>>>> + *   Must not be NULL.
>>>>>>>> + * @param n
>>>>>>>> + *   Element count in @p ids and @p values.
>>>>>>>> + *
>>>>>>>> + * @return
>>>>>>>> + *   - A number of filled in stats.
>>>>>>>> + *   - A negative value on error.
>>>>>>>> + */
>>>>>>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>>>>>>  				      const uint64_t *ids,
>>>>>>>>  				      uint64_t *values,
>>>>>>>>  				      unsigned int n);
>>>>>>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>>>>>>  
>>>>>>>>  /**
>>>>>>>>   * @internal
>>>>>>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>>>>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>>>>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>>>>>>  
>>>>>>>> +/**
>>>>>>>> + * @internal
>>>>>>>> + * Get names of extended stats of an Ethernet device.
>>>>>>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>>>>>>
>>>>>>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>>>>>>
>>>>>>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>>>>>>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>>>>>>
>>>>>>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>>>>>>> not for other. Why not require both to support returning 'count'?
>>>>>>
>>>>>> Simply because it is dead code. There is no point to require
>>>>>> from driver to have dead code.
>>>>>>
>>>>>
>>>>> Let me step back a little, both ethdev APIs can be used to return xstats count
>>>>> by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
>>>>> 'rte_eth_xstats_get_names_by_id()'
>>>>> 'rte_eth_xstats_get_by_id()'
>>>>>
>>>>> But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
>>>>> as said above I believe this selection is done unintentionally.
>>>>>
>>>>>
>>>>> I am for below two options:
>>>>>
>>>>> a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
>>>>> count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
>>>>> simplifies driver code. As far as I remember I suggested this before, still I
>>>>> prefer this one.
>>>>>
>>>>> b) If we will support getting xstats count from '_by_id' dev_ops, I think both
>>>>> should support it, to not make it more complex to figure out which one support
>>>>> what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
>>>>> count, not just one.
>>>>>
>>>>
>>>> In (b) do you suggest to change ethdev to use xstats_get_by_id
>>>> driver op if users asks for a number of xstats using
>>>> rte_eth_xstats_get_by_id()? It will complicate ethdev and
>>>> will complicate drivers. Just for the symmetry?
>>>>
>>>
>>> Not just for symmetry, but simplify the logic. Both dev_ops are very similar and
>>
>> I'm sorry, but could you point of which logic you'd
>> like to simply. Less requirements on driver ops
>> means less code required inside.
>>
>>> they are having different behavior with same parameters is confusing.
>>
>> Ah, logic from PMD maintainer point of view. Does it
>> really worse to require extra code inside because of it?
>>
>>> If you want to simplify the drivers why not go with option (a)? Option (a) also
>>> doesn't change external API, and has only minor change in the ethdev layer.
>>
>> Is two extra patches in v6 (discussed below) a step
>> towards (a)?
>>
> 
> I am not sure of it, for (a) ethdev layer will handle getting all stats or
> getting count using 'xstats_get_names' || 'xstats_get', and '*_by_id()' dev_ops
> will become simpler since they won't accept NULL 'ids' & 'values'.

Got it. Will fix it in v7. I'll remove requirement in a
dedicated patch, but may be it should be squashed with
this one on apply.

> Also when dev_ops merged, it forces PMDs to implement the getting by ids, but
> now it is covered by ethdev layer for the PMDs that doesn't implement '_by_id()'.

Thanks, good catch. I'll handle it v7 as well.