[04/11] ethdev: fix docs of drivers callbacks getting xstats by IDs
Checks
Commit Message
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 | 43 ++++++++++++++++++++++++++++++++++++--
1 file changed, 41 insertions(+), 2 deletions(-)
Comments
On 6/4/2021 3:42 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 | 43 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 40e474aa7e..fd5b7ca550 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.
Should it mention _by_id detail?
> + *
> + * @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,32 @@ 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.
Should it mention _by_id detail?
> + * For name count, set @p xstats_names and @p ids to NULL.
> + *
isn't the 'count' part handled in the API? I think in the devops both should not
be NULL.
> + * @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.
As far as I understand both _by_id APIs and devops behave same, so argument
descriptions/behavior should be same.
> + * @param ids
> + * IDs array to retrieve specific statistics. Can be NULL together
> + * with @p xstats_names to retrieve number of available statistics.
> + * @param size
> + * Size of ids and xstats_names arrays.
> + * 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.
Again as far as I can see these covered by API, not devops, am I missing something.
> + * - 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 7/20/21 7:51 PM, Ferruh Yigit wrote:
> On 6/4/2021 3:42 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 | 43 ++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index 40e474aa7e..fd5b7ca550 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.
>
> Should it mention _by_id detail?
Yes, will fix in v2.
>> + *
>> + * @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,32 @@ 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.
>
> Should it mention _by_id detail?
Yes, will fix in v2.
>> + * For name count, set @p xstats_names and @p ids to NULL.
>> + *
>
> isn't the 'count' part handled in the API? I think in the devops both should not
> be NULL.
No, eth_dev_get_xstats_count() uses the callback with NULL, NULL, 0.
>
>> + * @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.
>
> As far as I understand both _by_id APIs and devops behave same, so argument
> descriptions/behavior should be same.
In fact no, it is slightly different. For example, devops is never
called with NULL ids and not NULL names or non-zero size. It allows to
check less in drivers.
>> + * @param ids
>> + * IDs array to retrieve specific statistics. Can be NULL together
>> + * with @p xstats_names to retrieve number of available statistics.
>> + * @param size
>> + * Size of ids and xstats_names arrays.
>> + * 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.
>
> Again as far as I can see these covered by API, not devops, am I missing something.
See eth_dev_get_xstats_count()
>
>> + * - 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 7/22/2021 10:33 AM, Andrew Rybchenko wrote:
> On 7/20/21 7:51 PM, Ferruh Yigit wrote:
>> On 6/4/2021 3:42 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 | 43 ++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 41 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>> index 40e474aa7e..fd5b7ca550 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.
>>
>> Should it mention _by_id detail?
>
> Yes, will fix in v2.
>
>>> + *
>>> + * @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,32 @@ 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.
>>
>> Should it mention _by_id detail?
>
> Yes, will fix in v2.
>
>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>> + *
>>
>> isn't the 'count' part handled in the API? I think in the devops both should not
>> be NULL.
>
> No, eth_dev_get_xstats_count() uses the callback with NULL, NULL, 0.
>
Right, I missed it, xstats_get_names_by_id() seems used outside of the ethdev
_by_id() APIs.
>>
>>> + * @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.
>>
>> As far as I understand both _by_id APIs and devops behave same, so argument
>> descriptions/behavior should be same.
>
> In fact no, it is slightly different. For example, devops is never
> called with NULL ids and not NULL names or non-zero size. It allows to
> check less in drivers.
>
I am not sure if this difference is intentional.
'eth_dev_get_xstats_count()' is ethdev internal function, what do you think
removing 'xstats_get_names_by_id()' call from it. And make both _by_id() dev_ops
doesn't accept NULL ids and NULL values/names, so simplifies PMD implementations.
>>> + * @param ids
>>> + * IDs array to retrieve specific statistics. Can be NULL together
>>> + * with @p xstats_names to retrieve number of available statistics.
>>> + * @param size
>>> + * Size of ids and xstats_names arrays.
>>> + * 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.
>>
>> Again as far as I can see these covered by API, not devops, am I missing
>> something.
>
> See eth_dev_get_xstats_count()
>
>>
>>> + * - 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 7/23/21 5:31 PM, Ferruh Yigit wrote:
> On 7/22/2021 10:33 AM, Andrew Rybchenko wrote:
>> On 7/20/21 7:51 PM, Ferruh Yigit wrote:
>>> On 6/4/2021 3:42 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>
[snip]
>>>> + * @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.
>>>
>>> As far as I understand both _by_id APIs and devops behave same, so argument
>>> descriptions/behavior should be same.
>>
>> In fact no, it is slightly different. For example, devops is never
>> called with NULL ids and not NULL names or non-zero size. It allows to
>> check less in drivers.
>>
>
> I am not sure if this difference is intentional.
What we're trying to do here is to document existing usage of these
callbacks to more strictly define valid input parameters to allow
drivers to support less combinations (to make implementation simpler).
So, intentional.
If you're trying to say that API functions could handle less variants,
the answer is simple - it could be discussed, but out of scope of the
patch series since it will be API change.
> 'eth_dev_get_xstats_count()' is ethdev internal function, what do you think
> removing 'xstats_get_names_by_id()' call from it. And make both _by_id() dev_ops
> doesn't accept NULL ids and NULL values/names, so simplifies PMD implementations.
>
In fact the function eth_dev_get_xstats_count() looks buggy since it
does not use xstats_get_names_by_id positive result if
xstats_get_names is not NULL. Really confusing.
Again, it will be slight change on PMD requirements. May be it is OK
to do, but I'd conduct it separately since it is logically a separate
thing. Current patches just clarify documentation.
@@ -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,32 @@ 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.
+ *
+ * @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
+ * Size of ids and xstats_names arrays.
+ * 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,