From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B3B85A034E; Wed, 9 Feb 2022 02:06:18 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3A9E4410F3; Wed, 9 Feb 2022 02:06:18 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 8BB394067E for ; Wed, 9 Feb 2022 02:06:16 +0100 (CET) Received: from dggeme756-chm.china.huawei.com (unknown [172.30.72.53]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4JthRX3zmWzZfQF; Wed, 9 Feb 2022 09:02:00 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by dggeme756-chm.china.huawei.com (10.3.19.102) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2308.21; Wed, 9 Feb 2022 09:06:12 +0800 Subject: Re: [PATCH] ethdev: introduce ethdev dump API To: Ferruh Yigit , =?UTF-8?Q?Morten_Br=c3=b8rup?= , CC: Ray Kinsella , Ajit Khaparde , Thomas Monjalon , Andrew Rybchenko References: <20220111115437.32855-1-humin29@huawei.com> <20220207014719.16611-1-humin29@huawei.com> <8b129213-8d64-0b9e-8bb3-5faa8bfdd2d4@intel.com> <98CBD80474FA8B44BF855DF32C47DC35D86E79@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D86E7A@smartserver.smartshare.dk> <8a8fbfae-7547-67e0-08ff-4faa7e9d8a50@huawei.com> <0c63906d-3979-f8ce-4c32-2ddfcceaf3b6@intel.com> <8ca8568e-b88c-e758-fc0b-d11b7cb997af@huawei.com> <9900011f-89a2-dc69-523f-2259cb4a7085@intel.com> From: "Min Hu (Connor)" Message-ID: <064e1980-8565-02aa-7611-4c63afaeea37@huawei.com> Date: Wed, 9 Feb 2022 09:06:12 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <9900011f-89a2-dc69-523f-2259cb4a7085@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To dggeme756-chm.china.huawei.com (10.3.19.102) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi, 在 2022/2/8 20:59, Ferruh Yigit 写道: > On 2/8/2022 11:14 AM, Min Hu (Connor) wrote: >> 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) >>>>>>>>>> Acked-by: Morten Brørup >>>>>>>>>> Acked-by: Ray Kinsella >>>>>>>>>> Acked-by: Ajit Khaparde >>>>>>>> >>>>>>>> [...] >>>>>>>> >>>>>>>>>> @@ -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 ? >> > > I think 'struct rte_eth_dev' content can be a sample. > > But I hear you, diff is not clear, it is subtle as Morten said, > when doc and commit log refers it as "private info from device", > I think we can use the same in the API documentation as well. > Agreed， I will fix it in release_22_03.rst. >> 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'. >> > > Yes it can be. But as far as I can see there is nothing prevents the dump() > API to provide the same, it is up to PMD. > > If the intention is to limit what can be dump to > 'ethdev->data->dev_private', > it is not clear from API documentation/implementation. > >>> >>>>>> >>>>>> 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. >>>>> >>>>> . >>> >>> . > > .