Re: [PATCH v4 1/3] ethdev: introduce ethdev desc dump API

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 10/4/22 01:40, Ferruh Yigit wrote:
> On 9/23/2022 8:43 AM, Dongdong Liu wrote:
> 
>>
>> From: "Min Hu (Connor)" <humin29@huawei.com>
>>
>> Added the ethdev Rx/Tx desc dump API which provides functions for query
>> descriptor from device. HW descriptor info differs in different NICs.
>> The information demonstrates I/O process which is important for debug.
>> As the information is different
> 
> Overall OK to have these new APIs, please find comments below.
> 
> Do you think does it worth to list this as one of the PMD future in 
> future list, in 'doc/guides/nics/features.rst' ?

IMHO it does not deserve entry in features.
It is a deep debugging using vendor-specific information.

> 
>   between NICs, the new API is introduced.
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> Acked-by: Ray Kinsella <mdr@ashroe.eu>
> 
> <...>
> 
>>   int rte_eth_dev_priv_dump(uint16_t port_id, FILE *file);
>>
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change, or be removed, without prior 
>> notice
>> + *
>> + * Dump ethdev Rx descriptor info to a file.
>> + *
>> + * This API is used for debugging, not a dataplane API.
>> + *
>> + * @param file
>> + *   A pointer to a file for output.
>> + * @param dev
>> + *   Port (ethdev) handle.
>> + * @param queue_id
>> + *   The selected queue.
>> + * @param num
>> + *   The number of the descriptors to dump.
>> + * @return
>> + *   - On success, zero.
>> + *   - On failure, a negative value.
>> + */
>> +__rte_experimental
>> +int rte_eth_rx_hw_desc_dump(FILE *file, uint16_t port_id, uint16_t 
>> queue_id,
>> +                           uint16_t num);
> 
> There are other HW desc related APIs, like 
> 'rte_eth_rx_descriptor_status()'.
> Should this APIs follow same naming convention:
> 'rte_eth_rx_descriptor_dump()'
> 'rte_eth_tx_descriptor_dump()'

+1 on naming, it should not be bound to HW. SW parts could be
interesting as well.

> 
>> +
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change, or be removed, without prior 
>> notice
>> + *
>> + * Dump ethdev Tx descriptor info to a file.
>> + *
>> + * This API is used for debugging, not a dataplane API.
>> + *
>> + * @param file
>> + *   A pointer to a file for output.
>> + * @param dev
>> + *   Port (ethdev) handle.
>> + * @param queue_id
>> + *   The selected queue.
>> + * @param num
>> + *   The number of the descriptors to dump.
>> + * @return
>> + *   - On success, zero.
>> + *   - On failure, a negative value.
>> + */
>> +__rte_experimental
>> +int rte_eth_tx_hw_desc_dump(FILE *file, uint16_t port_id, uint16_t 
>> queue_id,
>> +                           uint16_t num);
> 
> 'num' is provided, does it assume it starts from offset 0, what do you 
> think to provide 'offset' as parameter?
> It may be a use case to start from where tail/head pointer is.
> 
>> +
>> +
>>   #include <rte_ethdev_core.h>
>>
>>   /**
>> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
>> index 03f52fee91..3c7c75b582 100644
>> --- a/lib/ethdev/version.map
>> +++ b/lib/ethdev/version.map
>> @@ -285,6 +285,8 @@ EXPERIMENTAL {
>>          rte_mtr_color_in_protocol_priority_get;
>>          rte_mtr_color_in_protocol_set;
>>          rte_mtr_meter_vlan_table_update;
>> +       rte_eth_rx_hw_desc_dump;
>> +       rte_eth_tx_hw_desc_dump;
> 
> These new APIs should go after "# added in 22.11" comment, if you rebase 
> on top of latest HEAD, comment is already there.
>