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 D4E1DA04A7; Tue, 8 Feb 2022 01:39:46 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 588C44014E; Tue, 8 Feb 2022 01:39:46 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 537B240141 for ; Tue, 8 Feb 2022 01:39:45 +0100 (CET) Received: from dggeme756-chm.china.huawei.com (unknown [172.30.72.54]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4Jt3z6576Kzccll; Tue, 8 Feb 2022 08:38:42 +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 08:39:42 +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> From: "Min Hu (Connor)" Message-ID: <8a8fbfae-7547-67e0-08ff-4faa7e9d8a50@huawei.com> Date: Tue, 8 Feb 2022 08:39:42 +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: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) 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/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? >>> 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? >> >> 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. > > .