[PATCH v4 3/3] app/procinfo: support descriptor dump

[Dongdong Liu]

Hi Reshma

Many thanks for your view.
On 2022/9/23 20:18, Pattan, Reshma wrote:
>
>
>> -----Original Message-----
>> From: Dongdong Liu <liudongdong3 at huawei.com>
>> Subject: [PATCH v4 3/3] app/procinfo: support descriptor dump
>>
>> From: "Min Hu (Connor)" <humin29 at huawei.com>
>>
>> This patch support HW Rx/Tx descriptor dump
>>
>> The command is like:
>> dpdk-proc-info -a xxxx:xx:xx.x --file-prefix=xxx -- -- --show-rx-descriptor
>> queue_id:num
>
>
> What is num here?  You need to describe about this in commit message.
> Is num means number of descriptors? Better use the descriptor word here in num

Yes, It means number of descriptors. How about rename it to desc_num.
>
>>
>> dpdk-proc-info -a xxxx:xx:xx.x --file-prefix=xxx -- -- --show-tx-descriptor
>> queue_id:num
>
> Same here.
Will fix.
>
>> +/* Enable dump buffer descriptor. */
>> +#define MAX_NB_ITEM 2
>> +static uint16_t rx_nb_item;
>> +static uint16_t tx_nb_item;
>> +static uint16_t rx_item_opt[MAX_NB_ITEM]; static uint16_t
>
>
> Instead of using array to keep queid and number of descriptors info , better have structure with 2 parameters one for queue and one for the number of descriptors.
> And You can use  the same structure to declare tx_item.
Good point, will do.
>
>> +tx_item_opt[MAX_NB_ITEM];
>>
>
>> +		"  --show-rx-descriptor queue_id:num: to display ports Rx
>> buffer description by queue id and num\n"
>> +		"  --show-tx-descriptor queue_id:num: to display ports Tx
>> buffer description by queue id and num\n"
>
> Here also use enough description what is num means or use the another name.
Will do.
>
>> +				if (ret < MAX_NB_ITEM) {
>
> You can check ret < 0 no need to use MAX_NB_ITEM again.
>> +					printf("Rx descriptor param parse error.\n");
>> +					return -1;
>> +				}
>
> Instead of saying parse error. You might need to give clear reason of failure.
> That is invalid queueid:num passed by user or user passed more than one pair.
Will do.
>
>> +				int ret = parse_descriptor_param(optarg,
>> +								 rx_item_opt,
>> +								 MAX_NB_ITEM);
>
> Do we need to really pass the MAX_NB_ITEM> I don't think we need you can directly use the macro in the called function.
>
>
>> +				if (ret < MAX_NB_ITEM) {
>
> You can check ret < 0 no need to use MAX_NB_ITEM again.
Will fix.
>
>> +					printf("Tx descriptor param parse error.\n");
>> +					return -1;
>> +				}
>
> Same here.
Will do.

Thanks,
Dongdong
>
> Thanks,
> Reshma
>
>
> .
>