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 3228EA04AE; Tue, 8 Feb 2022 12:14:33 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0103E410FD; Tue, 8 Feb 2022 12:14:33 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 5B08F410FC for ; Tue, 8 Feb 2022 12:14:31 +0100 (CET) Received: from dggeme756-chm.china.huawei.com (unknown [172.30.72.56]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4JtL3Y0lnyzccqK; Tue, 8 Feb 2022 19:13:29 +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; Tue, 8 Feb 2022 19:14:28 +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> From: "Min Hu (Connor)" Message-ID: <8ca8568e-b88c-e758-fc0b-d11b7cb997af@huawei.com> Date: Tue, 8 Feb 2022 19:14:28 +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: <0c63906d-3979-f8ce-4c32-2ddfcceaf3b6@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, 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 ? 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. >>> >>> . > > .