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

[Dongdong Liu <liudongdong3@huawei.com>]

Hi Ferruh

Many thanks for your review.
On 2022/10/4 6: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' ?
As Andrew said
It does not deserve entry in features.
It is a deep debugging using vendor-specific information.

I agree with him.
>
>  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()'
Looks good, will do.
>
>> +
>> +/**
>> + * @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.
It will be ok to provide 'offset' as parameter. will change as below.

/**
  * @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 port_id
  *   The port identifier of the Ethernet device.
  * @param queue_id
  *   A Rx queue identifier on this port.
  * @param offset
  *  The offset of the descriptor starting from tail. (0 is the next
  *  packet to be received by the driver).
  * @param num
  *   The number of the descriptors to dump.
  * @param file
  *   A pointer to a file for output.
  * @return
  *   - On success, zero.
  *   - On failure, a negative value.
  */
__rte_experimental
int rte_eth_rx_descriptor_dump(uint16_t port_id, uint16_t queue_id,
                                uint16_t offset, uint16_t num, FILE *file);
>
>> +
>> +
>>   #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.
Will rebase on top of latest HEAD.

Thanks,
Dongdong
>
> .
>