diff mbox series

[03/11] ethdev: fix docs of functions getting xstats by IDs

Message ID 20210604144225.287678-4-andrew.rybchenko@oktetlabs.ru (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers show
Series net/sfc: provide Rx/Tx doorbells stats | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Andrew Rybchenko June 4, 2021, 2:42 p.m. UTC
From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Document valid combinations of input arguments in accordance with
current implementation in ethdev.

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/rte_ethdev.h | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Comments

Ferruh Yigit July 20, 2021, 4:25 p.m. UTC | #1
On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Document valid combinations of input arguments in accordance with
> current implementation in ethdev.
> 
> 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/rte_ethdev.h | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index faf3bd901d..1f63118544 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>   *   The port identifier of the Ethernet device.
>   * @param xstats_names
>   *   An rte_eth_xstat_name array of at least *size* elements to
> - *   be filled. If set to NULL, the function returns the required number
> - *   of elements.
> + *   be filled. Must not be NULL if @p ids are specified (not NULL).

Removed part is also valid. If both 'ids' & 'xstats_names' are NULL, API returns
number of all elements.

Addition part looks good.

>   * @param ids
> - *   IDs array given by app to retrieve specific statistics
> + *   IDs array given by app to retrieve specific statistics. May be NULL
> + *   to retrieve all available statistics.

ack

>   * @param size
> - *   The size of the xstats_names array (number of elements).
> + *   If @p ids is not NULL, number of elements in the array with requested IDs
> + *   and number of elements in @p xstats_names to put names in. If @p ids is
> + *   NULL, number of elements in @p xstats_names to put all available statistics
> + *   names in.

ack

>   * @return
>   *   - A positive value lower or equal to size: success. The return value
>   *     is the number of entries filled in the stats table.
> @@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>   *     is too small. The return value corresponds to the size that should
>   *     be given to succeed. The entries in the table are not valid and
>   *     shall not be used by the caller.
> - *   - A negative value on error (invalid port id).
> + *   - A negative value on error.

ack


The 'eth_dev_get_xstats_count()' API is flexible but it makes API unnecessarily
complex, not for this patch but for future perhaps we can update the API and it
can return error if either 'ids' or 'xstats_names' is NULL. Remove support to
get all elements or getting number of elements support, these already supported
by non _id version of API.


And as a note for future, if we ever consider updating these _by_id APIs, we can
consider making the parameter order same for both, currently it is:
"rte_eth_xstats_get_names_by_id(port_id, values, size, ids)"
"      rte_eth_xstats_get_by_id(port_id, ids, values, size)"

>   */
>  int
>  rte_eth_xstats_get_names_by_id(uint16_t port_id,
> @@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>   *   The port identifier of the Ethernet device.
>   * @param ids
>   *   A pointer to an ids array passed by application. This tells which
> - *   statistics values function should retrieve. This parameter
> - *   can be set to NULL if size is 0. In this case function will retrieve
> + *   statistics values function should retrieve. May be NULL to retrieve
>   *   all available statistics.

Update is good. But what do you think to make it exact same in the both APIs
('rte_eth_xstats_get_names_by_id()' & 'rte_eth_xstats_get_by_id()')? Since it is
used for same purpose and exact same way in both APIs, no need to have slightly
different description.

>   * @param values
>   *   A pointer to a table to be filled with device statistics values.
> + *   Must not be NULL if ids are specified (not NULL).

Same comment on making description similar in both APIs.

Also both 'ids' & 'values' being NULL returns number of all elements should be
addressed.

>   * @param size
> - *   The size of the ids array (number of elements).
> + *   If @p ids is not NULL, number of elements in the array with requested IDs
> + *   and number of elements in values to put statistics in. If @p ids is NULL,
> + *   number of elements in values to put all available statistics in.

ack

>   * @return
>   *   - A positive value lower or equal to size: success. The return value
>   *     is the number of entries filled in the stats table.
> @@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>   *     is too small. The return value corresponds to the size that should
>   *     be given to succeed. The entries in the table are not valid and
>   *     shall not be used by the caller.
> - *   - A negative value on error (invalid port id).
> + *   - A negative value on error.

ack

>   */
>  int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
>  			     uint64_t *values, unsigned int size);
>
Andrew Rybchenko July 22, 2021, 9:12 a.m. UTC | #2
On 7/20/21 7:25 PM, Ferruh Yigit wrote:
> On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Document valid combinations of input arguments in accordance with
>> current implementation in ethdev.
>>
>> 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/rte_ethdev.h | 23 ++++++++++++++---------
>>   1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index faf3bd901d..1f63118544 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>>    *   The port identifier of the Ethernet device.
>>    * @param xstats_names
>>    *   An rte_eth_xstat_name array of at least *size* elements to
>> - *   be filled. If set to NULL, the function returns the required number
>> - *   of elements.
>> + *   be filled. Must not be NULL if @p ids are specified (not NULL).
> 
> Removed part is also valid. If both 'ids' & 'xstats_names' are NULL, API returns
> number of all elements.

Yes, but it is an excessive information. The trigger to return number
of all elements is 'ids == NULL'. Here we are talking about
'xstats_names' parameter. If the parameter is NULL, but ids is not
null, it does not trigger number of all elements return. It is an
invalid input parameters. That's what a new description says.

> 
> Addition part looks good.
> 
>>    * @param ids
>> - *   IDs array given by app to retrieve specific statistics
>> + *   IDs array given by app to retrieve specific statistics. May be NULL
>> + *   to retrieve all available statistics.
> 
> ack
> 
>>    * @param size
>> - *   The size of the xstats_names array (number of elements).
>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>> + *   and number of elements in @p xstats_names to put names in. If @p ids is
>> + *   NULL, number of elements in @p xstats_names to put all available statistics
>> + *   names in.
> 
> ack
> 
>>    * @return
>>    *   - A positive value lower or equal to size: success. The return value
>>    *     is the number of entries filled in the stats table.
>> @@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
>>    *     is too small. The return value corresponds to the size that should
>>    *     be given to succeed. The entries in the table are not valid and
>>    *     shall not be used by the caller.
>> - *   - A negative value on error (invalid port id).
>> + *   - A negative value on error.
> 
> ack
> 
> 
> The 'eth_dev_get_xstats_count()' API is flexible but it makes API unnecessarily
> complex, not for this patch but for future perhaps we can update the API and it
> can return error if either 'ids' or 'xstats_names' is NULL. Remove support to
> get all elements or getting number of elements support, these already supported
> by non _id version of API.

I'm not sure that it is a right direction. The support allows
application to use less number of functions and depend on less
number of function prototypes.

> And as a note for future, if we ever consider updating these _by_id APIs, we can
> consider making the parameter order same for both, currently it is:
> "rte_eth_xstats_get_names_by_id(port_id, values, size, ids)"
> "      rte_eth_xstats_get_by_id(port_id, ids, values, size)"

+1, current difference is terribly bad

>>    */
>>   int
>>   rte_eth_xstats_get_names_by_id(uint16_t port_id,
>> @@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>    *   The port identifier of the Ethernet device.
>>    * @param ids
>>    *   A pointer to an ids array passed by application. This tells which
>> - *   statistics values function should retrieve. This parameter
>> - *   can be set to NULL if size is 0. In this case function will retrieve
>> + *   statistics values function should retrieve. May be NULL to retrieve
>>    *   all available statistics.
> 
> Update is good. But what do you think to make it exact same in the both APIs
> ('rte_eth_xstats_get_names_by_id()' & 'rte_eth_xstats_get_by_id()')? Since it is
> used for same purpose and exact same way in both APIs, no need to have slightly
> different description.

I agree. I'll fix in v2.

>>    * @param values
>>    *   A pointer to a table to be filled with device statistics values.
>> + *   Must not be NULL if ids are specified (not NULL).
> 
> Same comment on making description similar in both APIs.

OK

> Also both 'ids' & 'values' being NULL returns number of all elements should be
> addressed.

I think it is excessibe. It is sufficient to say so for ids==NULL which
is a trigger to get all elements.

>>    * @param size
>> - *   The size of the ids array (number of elements).
>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>> + *   and number of elements in values to put statistics in. If @p ids is NULL,
>> + *   number of elements in values to put all available statistics in.
> 
> ack
> 
>>    * @return
>>    *   - A positive value lower or equal to size: success. The return value
>>    *     is the number of entries filled in the stats table.
>> @@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>    *     is too small. The return value corresponds to the size that should
>>    *     be given to succeed. The entries in the table are not valid and
>>    *     shall not be used by the caller.
>> - *   - A negative value on error (invalid port id).
>> + *   - A negative value on error.
> 
> ack
> 
>>    */
>>   int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
>>   			     uint64_t *values, unsigned int size);
>>
Ferruh Yigit July 23, 2021, 2:19 p.m. UTC | #3
On 7/22/2021 10:12 AM, Andrew Rybchenko wrote:
> On 7/20/21 7:25 PM, Ferruh Yigit wrote:
>> On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:
>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>
>>> Document valid combinations of input arguments in accordance with
>>> current implementation in ethdev.
>>>
>>> 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/rte_ethdev.h | 23 ++++++++++++++---------
>>>   1 file changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>>> index faf3bd901d..1f63118544 100644
>>> --- a/lib/ethdev/rte_ethdev.h
>>> +++ b/lib/ethdev/rte_ethdev.h
>>> @@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct
>>> rte_eth_xstat *xstats,
>>>    *   The port identifier of the Ethernet device.
>>>    * @param xstats_names
>>>    *   An rte_eth_xstat_name array of at least *size* elements to
>>> - *   be filled. If set to NULL, the function returns the required number
>>> - *   of elements.
>>> + *   be filled. Must not be NULL if @p ids are specified (not NULL).
>>
>> Removed part is also valid. If both 'ids' & 'xstats_names' are NULL, API returns
>> number of all elements.
> 
> Yes, but it is an excessive information. The trigger to return number
> of all elements is 'ids == NULL'. Here we are talking about
> 'xstats_names' parameter. If the parameter is NULL, but ids is not
> null, it does not trigger number of all elements return. It is an
> invalid input parameters. That's what a new description says.
> 

   ids		xstats_names
a) NULL		NULL		valid, return _number_ of all elements
b) NULL		!NULL		valid, return all elements (if size big enough)
c) !NULL	NULL		invalid

Above a) is a valid option, the previous comment tries to describe it via "If
set to NULL, the function returns the required number of elements." and that
part is removed.
Since it is valid option I think we should keep it documented, location or
wording is up to you.

>>
>> Addition part looks good.
>>
>>>    * @param ids
>>> - *   IDs array given by app to retrieve specific statistics
>>> + *   IDs array given by app to retrieve specific statistics. May be NULL
>>> + *   to retrieve all available statistics.
>>
>> ack
>>
>>>    * @param size
>>> - *   The size of the xstats_names array (number of elements).
>>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>>> + *   and number of elements in @p xstats_names to put names in. If @p ids is
>>> + *   NULL, number of elements in @p xstats_names to put all available
>>> statistics
>>> + *   names in.
>>
>> ack
>>
>>>    * @return
>>>    *   - A positive value lower or equal to size: success. The return value
>>>    *     is the number of entries filled in the stats table.
>>> @@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct
>>> rte_eth_xstat *xstats,
>>>    *     is too small. The return value corresponds to the size that should
>>>    *     be given to succeed. The entries in the table are not valid and
>>>    *     shall not be used by the caller.
>>> - *   - A negative value on error (invalid port id).
>>> + *   - A negative value on error.
>>
>> ack
>>
>>
>> The 'eth_dev_get_xstats_count()' API is flexible but it makes API unnecessarily
>> complex, not for this patch but for future perhaps we can update the API and it
>> can return error if either 'ids' or 'xstats_names' is NULL. Remove support to
>> get all elements or getting number of elements support, these already supported
>> by non _id version of API.
> 
> I'm not sure that it is a right direction. The support allows
> application to use less number of functions and depend on less
> number of function prototypes.
> 

True, but makes API more complex, and creates multiple way to do same thing (get
number of all entries), trade off.

>> And as a note for future, if we ever consider updating these _by_id APIs, we can
>> consider making the parameter order same for both, currently it is:
>> "rte_eth_xstats_get_names_by_id(port_id, values, size, ids)"
>> "      rte_eth_xstats_get_by_id(port_id, ids, values, size)"
> 
> +1, current difference is terribly bad
> 
>>>    */
>>>   int
>>>   rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>> @@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>>    *   The port identifier of the Ethernet device.
>>>    * @param ids
>>>    *   A pointer to an ids array passed by application. This tells which
>>> - *   statistics values function should retrieve. This parameter
>>> - *   can be set to NULL if size is 0. In this case function will retrieve
>>> + *   statistics values function should retrieve. May be NULL to retrieve
>>>    *   all available statistics.
>>
>> Update is good. But what do you think to make it exact same in the both APIs
>> ('rte_eth_xstats_get_names_by_id()' & 'rte_eth_xstats_get_by_id()')? Since it is
>> used for same purpose and exact same way in both APIs, no need to have slightly
>> different description.
> 
> I agree. I'll fix in v2.
> 
>>>    * @param values
>>>    *   A pointer to a table to be filled with device statistics values.
>>> + *   Must not be NULL if ids are specified (not NULL).
>>
>> Same comment on making description similar in both APIs.
> 
> OK
> 
>> Also both 'ids' & 'values' being NULL returns number of all elements should be
>> addressed.
> 
> I think it is excessibe. It is sufficient to say so for ids==NULL which
> is a trigger to get all elements.
> 

But these two are different things,
one fills values with stats and return number of filled elements
other doesn't fill any value, but only return number of all elements

>>>    * @param size
>>> - *   The size of the ids array (number of elements).
>>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>>> + *   and number of elements in values to put statistics in. If @p ids is NULL,
>>> + *   number of elements in values to put all available statistics in.
>>
>> ack
>>
>>>    * @return
>>>    *   - A positive value lower or equal to size: success. The return value
>>>    *     is the number of entries filled in the stats table.
>>> @@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>>    *     is too small. The return value corresponds to the size that should
>>>    *     be given to succeed. The entries in the table are not valid and
>>>    *     shall not be used by the caller.
>>> - *   - A negative value on error (invalid port id).
>>> + *   - A negative value on error.
>>
>> ack
>>
>>>    */
>>>   int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
>>>                    uint64_t *values, unsigned int size);
>>>
>
Andrew Rybchenko July 24, 2021, 12:06 p.m. UTC | #4
On 7/23/21 5:19 PM, Ferruh Yigit wrote:
> On 7/22/2021 10:12 AM, Andrew Rybchenko wrote:
>> On 7/20/21 7:25 PM, Ferruh Yigit wrote:
>>> On 6/4/2021 3:42 PM, Andrew Rybchenko wrote:
>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>
>>>> Document valid combinations of input arguments in accordance with
>>>> current implementation in ethdev.
>>>>
>>>> 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/rte_ethdev.h | 23 ++++++++++++++---------
>>>>    1 file changed, 14 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>>>> index faf3bd901d..1f63118544 100644
>>>> --- a/lib/ethdev/rte_ethdev.h
>>>> +++ b/lib/ethdev/rte_ethdev.h
>>>> @@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct
>>>> rte_eth_xstat *xstats,
>>>>     *   The port identifier of the Ethernet device.
>>>>     * @param xstats_names
>>>>     *   An rte_eth_xstat_name array of at least *size* elements to
>>>> - *   be filled. If set to NULL, the function returns the required number
>>>> - *   of elements.
>>>> + *   be filled. Must not be NULL if @p ids are specified (not NULL).
>>>
>>> Removed part is also valid. If both 'ids' & 'xstats_names' are NULL, API returns
>>> number of all elements.
>>
>> Yes, but it is an excessive information. The trigger to return number
>> of all elements is 'ids == NULL'. Here we are talking about
>> 'xstats_names' parameter. If the parameter is NULL, but ids is not
>> null, it does not trigger number of all elements return. It is an
>> invalid input parameters. That's what a new description says.
>>
> 
>     ids		xstats_names
> a) NULL		NULL		valid, return _number_ of all elements
> b) NULL		!NULL		valid, return all elements (if size big enough)
> c) !NULL	NULL		invalid
> 
> Above a) is a valid option, the previous comment tries to describe it via "If
> set to NULL, the function returns the required number of elements." and that
> part is removed.
> Since it is valid option I think we should keep it documented, location or
> wording is up to you.

OK, I see. IMHO, it looks better if it is mentioned in ids parameter
description.  See v4.

>>>
>>> Addition part looks good.
>>>
>>>>     * @param ids
>>>> - *   IDs array given by app to retrieve specific statistics
>>>> + *   IDs array given by app to retrieve specific statistics. May be NULL
>>>> + *   to retrieve all available statistics.
>>>
>>> ack
>>>
>>>>     * @param size
>>>> - *   The size of the xstats_names array (number of elements).
>>>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>>>> + *   and number of elements in @p xstats_names to put names in. If @p ids is
>>>> + *   NULL, number of elements in @p xstats_names to put all available
>>>> statistics
>>>> + *   names in.
>>>
>>> ack
>>>
>>>>     * @return
>>>>     *   - A positive value lower or equal to size: success. The return value
>>>>     *     is the number of entries filled in the stats table.
>>>> @@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct
>>>> rte_eth_xstat *xstats,
>>>>     *     is too small. The return value corresponds to the size that should
>>>>     *     be given to succeed. The entries in the table are not valid and
>>>>     *     shall not be used by the caller.
>>>> - *   - A negative value on error (invalid port id).
>>>> + *   - A negative value on error.
>>>
>>> ack
>>>
>>>
>>> The 'eth_dev_get_xstats_count()' API is flexible but it makes API unnecessarily
>>> complex, not for this patch but for future perhaps we can update the API and it
>>> can return error if either 'ids' or 'xstats_names' is NULL. Remove support to
>>> get all elements or getting number of elements support, these already supported
>>> by non _id version of API.
>>
>> I'm not sure that it is a right direction. The support allows
>> application to use less number of functions and depend on less
>> number of function prototypes.
>>
> 
> True, but makes API more complex, and creates multiple way to do same thing (get
> number of all entries), trade off.

Yes.

>>> And as a note for future, if we ever consider updating these _by_id APIs, we can
>>> consider making the parameter order same for both, currently it is:
>>> "rte_eth_xstats_get_names_by_id(port_id, values, size, ids)"
>>> "      rte_eth_xstats_get_by_id(port_id, ids, values, size)"
>>
>> +1, current difference is terribly bad
>>
>>>>     */
>>>>    int
>>>>    rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>>> @@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>>>     *   The port identifier of the Ethernet device.
>>>>     * @param ids
>>>>     *   A pointer to an ids array passed by application. This tells which
>>>> - *   statistics values function should retrieve. This parameter
>>>> - *   can be set to NULL if size is 0. In this case function will retrieve
>>>> + *   statistics values function should retrieve. May be NULL to retrieve
>>>>     *   all available statistics.
>>>
>>> Update is good. But what do you think to make it exact same in the both APIs
>>> ('rte_eth_xstats_get_names_by_id()' & 'rte_eth_xstats_get_by_id()')? Since it is
>>> used for same purpose and exact same way in both APIs, no need to have slightly
>>> different description.
>>
>> I agree. I'll fix in v2.
>>
>>>>     * @param values
>>>>     *   A pointer to a table to be filled with device statistics values.
>>>> + *   Must not be NULL if ids are specified (not NULL).
>>>
>>> Same comment on making description similar in both APIs.
>>
>> OK
>>
>>> Also both 'ids' & 'values' being NULL returns number of all elements should be
>>> addressed.
>>
>> I think it is excessibe. It is sufficient to say so for ids==NULL which
>> is a trigger to get all elements.
>>
> 
> But these two are different things,
> one fills values with stats and return number of filled elements
> other doesn't fill any value, but only return number of all elements

I get it. See v4. Hopefully I'll address it in ids parameter
description.

>>>>     * @param size
>>>> - *   The size of the ids array (number of elements).
>>>> + *   If @p ids is not NULL, number of elements in the array with requested IDs
>>>> + *   and number of elements in values to put statistics in. If @p ids is NULL,
>>>> + *   number of elements in values to put all available statistics in.
>>>
>>> ack
>>>
>>>>     * @return
>>>>     *   - A positive value lower or equal to size: success. The return value
>>>>     *     is the number of entries filled in the stats table.
>>>> @@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
>>>>     *     is too small. The return value corresponds to the size that should
>>>>     *     be given to succeed. The entries in the table are not valid and
>>>>     *     shall not be used by the caller.
>>>> - *   - A negative value on error (invalid port id).
>>>> + *   - A negative value on error.
>>>
>>> ack
>>>
>>>>     */
>>>>    int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
>>>>                     uint64_t *values, unsigned int size);
>>>>
>>
diff mbox series

Patch

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index faf3bd901d..1f63118544 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2873,12 +2873,15 @@  int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  *   The port identifier of the Ethernet device.
  * @param xstats_names
  *   An rte_eth_xstat_name array of at least *size* elements to
- *   be filled. If set to NULL, the function returns the required number
- *   of elements.
+ *   be filled. Must not be NULL if @p ids are specified (not NULL).
  * @param ids
- *   IDs array given by app to retrieve specific statistics
+ *   IDs array given by app to retrieve specific statistics. May be NULL
+ *   to retrieve all available statistics.
  * @param size
- *   The size of the xstats_names array (number of elements).
+ *   If @p ids is not NULL, number of elements in the array with requested IDs
+ *   and number of elements in @p xstats_names to put names in. If @p ids is
+ *   NULL, number of elements in @p xstats_names to put all available statistics
+ *   names in.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
@@ -2886,7 +2889,7 @@  int rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int
 rte_eth_xstats_get_names_by_id(uint16_t port_id,
@@ -2900,13 +2903,15 @@  rte_eth_xstats_get_names_by_id(uint16_t port_id,
  *   The port identifier of the Ethernet device.
  * @param ids
  *   A pointer to an ids array passed by application. This tells which
- *   statistics values function should retrieve. This parameter
- *   can be set to NULL if size is 0. In this case function will retrieve
+ *   statistics values function should retrieve. May be NULL to retrieve
  *   all available statistics.
  * @param values
  *   A pointer to a table to be filled with device statistics values.
+ *   Must not be NULL if ids are specified (not NULL).
  * @param size
- *   The size of the ids array (number of elements).
+ *   If @p ids is not NULL, number of elements in the array with requested IDs
+ *   and number of elements in values to put statistics in. If @p ids is NULL,
+ *   number of elements in values to put all available statistics in.
  * @return
  *   - A positive value lower or equal to size: success. The return value
  *     is the number of entries filled in the stats table.
@@ -2914,7 +2919,7 @@  rte_eth_xstats_get_names_by_id(uint16_t port_id,
  *     is too small. The return value corresponds to the size that should
  *     be given to succeed. The entries in the table are not valid and
  *     shall not be used by the caller.
- *   - A negative value on error (invalid port id).
+ *   - A negative value on error.
  */
 int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids,
 			     uint64_t *values, unsigned int size);