Re: [PATCH] ethdev: introduce ethdev dump API

["Min Hu (Connor)" <humin29@huawei.com>]

Hi, Ferruh,

在 2022/2/8 18:21, Ferruh Yigit 写道:
> On 2/8/2022 12:39 AM, Min Hu (Connor) wrote:
>> Hi, Ferruh,
>>
>> 在 2022/2/7 23:35, Ferruh Yigit 写道:
>>> On 2/7/2022 12:56 PM, Morten Brørup wrote:
>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>> Sent: Monday, 7 February 2022 13.36
>>>>>
>>>>> On 2/7/2022 12:18 PM, Morten Brørup wrote:
>>>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>>>> Sent: Monday, 7 February 2022 12.46
>>>>>>>
>>>>>>> On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
>>>>>>>> Added the ethdev dump API which provides functions for query
>>>>> private
>>>>>>> info
>>>>>>>
>>>>>>> Isn't API and function are same thing in this contexts?
>>>>>>>
>>>>>>>> from device. There exists many private properties in different PMD
>>>>>>> drivers,
>>>>>>>> such as adapter state, Rx/Tx func algorithm in hns3 PMD. The
>>>>>>> information of
>>>>>>>> these properties is important for debug. As the information is
>>>>>>> private,
>>>>>>>> the new API is introduced.>
>>>>>>>
>>>>>>> In the patch title 'ethdev' is duplicated, can you fix it?
>>>>>>>
>>>>>>>
>>>>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>>>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>>>>>>> Acked-by: Ray Kinsella <mdr@ashroe.eu>
>>>>>>>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>> @@ -990,6 +990,20 @@ typedef int
>>>>> (*eth_representor_info_get_t)(struct
>>>>>>> rte_eth_dev *dev,
>>>>>>>>     typedef int (*eth_rx_metadata_negotiate_t)(struct rte_eth_dev
>>>>> *dev,
>>>>>>>>                            uint64_t *features);
>>>>>>>>
>>>>>>>> +/**
>>>>>>>> + * @internal
>>>>>>>> + * Dump ethdev private info to a file.
>>>>>>>> + *
>>>>>>>
>>>>>>> It doesn't dump the 'ethdev' private info, it dumps the private info
>>>>>>> from device.
>>>>>>
>>>>>> It seems perfectly clear to me. How would you prefer it phrased
>>>>> instead?
>>>>>>
>>>>>
>>>>> What described in the document is more accurate,
>>>>> "query private info from device".
>>>>>
>>>>> What we are dumping here is not ethdev private info, it is device
>>>>> private info,
>>
>> what is the difference between ethdev and device?
> 
> It is not very clear, but for me 'ethdev' is refers to device abstract
> layer (ethdev library) specific private data 
Could you give an example for 'ethdev'specific private data ?

and device refers to ethdev
> device (PMD) private data. ethdev is common for all drivers.
OK, we could treat it as convention in future.
> 
>>>>> and we really don't know what that data may be in the ethdev layer.
>>>>>
>>>>> Also there is a chance that 'ethdev private info' can be confused with
>>>>> 'ethdev->data->dev_private'
>> what I want to dump is exactly the 'ethdev->data->dev_private'.
>> 'ethdev private info' means 'ethdev->data->dev_private'.
>> why confused?
> 
> What I understand was this API can return any device private information,
> it is not limited to 'ethdev->data->dev_private', (although most of the 
I think this API is limited to 'ethdev->data->dev_private'.
> data
> is represented in this struct), like if you want to dump queue state,
> this is out of 'ethdev->data->dev_private'.
Queue state can be dumped using API 'rte_eth_tx_queue_info_get'.

> 
>>>>
>>>> OK. Now I got your point! The difference is very subtle.
>>>>
>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>
>>>>>>>> + */
>>>>>>>> +__rte_experimental
>>>>>>>> +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
>>>>>>>> +
>>>>>>>
>>>>>>> What do you think to have the 'port_id' as first argument to be
>>>>>>> consistent
>>>>>>> with the other APIs?
>>>>>>
>>>>>> The _dump APIs in other libraries have the file pointer as the first
>>>>> parameter, so let's follow that convention here too. No need to move
>>>>> the port_id parameter here.
>>>>>>
>>>>>
>>>>> Yes, for most of the _dump() APIs, file pointer seems is the first
>>>>> argument,
>>>>> bu they are from various libraries.
>>>>>
>>>>> Within the ethdev APIs, I think it makes sense that all APIs start 
>>>>> with
>>>>> 'port_id' parameter for consistency, like done in:
>>>>> rte_flow_dev_dump(uint16_t port_id, ...)
>>>>>
>>>>>> Only rte_dma_dump() has the file pointer last, and I didn't catch it
>>>>> when the function was defined.
>>>>>>
>>>>
>>>> OK. Then I agree with you about following the convention like 
>>>> rte_flow_dev_dump() with the port_id first.
>>>>
>>>> I even think Connor got it right the first time, and I proposed 
>>>> following the other convention.
>>>>
>>>
>>> Ahh, may bad I missed that, sorry for not commenting on time.
>>>
>>>
>>>> It's not easy when there are two opposite conventions. :-)
>>>>
>>>
>>> Yep, that is the main issue.
>>>
>>> .
> 
> .