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 DCBDFA04A9; Tue, 8 Feb 2022 03:46:21 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C78E84014E; Tue, 8 Feb 2022 03:46:21 +0100 (CET) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id EC95040141 for ; Tue, 8 Feb 2022 03:46:19 +0100 (CET) Received: from dggeme756-chm.china.huawei.com (unknown [172.30.72.55]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4Jt6jV6ksLz1FD1x; Tue, 8 Feb 2022 10:42:06 +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 10:46:17 +0800 Subject: Re: [PATCH] ethdev: introduce ethdev dump API To: Ferruh Yigit , CC: =?UTF-8?Q?Morten_Br=c3=b8rup?= , 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> From: "Min Hu (Connor)" Message-ID: <7fb33828-97b1-61d9-5355-7024fb30cb13@huawei.com> Date: Tue, 8 Feb 2022 10:46:17 +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: <8b129213-8d64-0b9e-8bb3-5faa8bfdd2d4@intel.com> 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, v2 has been sent according comments, please check it, thanks. 在 2022/2/7 19:46, Ferruh Yigit 写道: > 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 >> --- >>   doc/guides/rel_notes/release_22_03.rst |  7 +++++++ >>   lib/ethdev/ethdev_driver.h             | 17 +++++++++++++++++ >>   lib/ethdev/rte_ethdev.c                | 15 +++++++++++++++ >>   lib/ethdev/rte_ethdev.h                | 16 ++++++++++++++++ >>   lib/ethdev/version.map                 |  3 +++ >>   5 files changed, 58 insertions(+) >> >> diff --git a/doc/guides/rel_notes/release_22_03.rst >> b/doc/guides/rel_notes/release_22_03.rst >> index b20716c521..5895194cde 100644 >> --- a/doc/guides/rel_notes/release_22_03.rst >> +++ b/doc/guides/rel_notes/release_22_03.rst >> @@ -92,6 +92,13 @@ New Features >>     * Called ``rte_ipv4/6_udptcp_cksum_mbuf()`` functions in testpmd >> csum mode >>       to support software UDP/TCP checksum over multiple segments. >> +* **Added the private ethdev dump API, for query private info of >> ethdev.** >> + >> +  Added the private ethdev dump API which provides functions for query >> +  private info from device. There exists many private properties in >> +  different PMD drivers. The information of these properties is >> important >> +  for debug. As the information is private, the new API is introduced. >> + > > Can you please move the update up, just above the ethdev PMD updates? > >>   Removed Items >>   ------------- >> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h >> index 7d27781f7d..d41d8a2c9d 100644 >> --- a/lib/ethdev/ethdev_driver.h >> +++ b/lib/ethdev/ethdev_driver.h >> @@ -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. > >> + * @param file >> + *   A pointer to a file for output. >> + * @param dev >> + *   Port (ethdev) handle. >> + * >> + * @return >> + *   Negative errno value on error, positive value on success. >> + */ >> +typedef int (*eth_dev_priv_dump_t)(FILE *file, struct rte_eth_dev *dev); >> + >>   /** >>    * @internal A structure containing the functions exported by an >> Ethernet driver. >>    */ >> @@ -1186,6 +1200,9 @@ struct eth_dev_ops { >>        * kinds of metadata to the PMD >>        */ >>       eth_rx_metadata_negotiate_t rx_metadata_negotiate; >> + >> +    /** Dump ethdev private info */ > > Ditto, not 'ethdev' private info. > >> +    eth_dev_priv_dump_t eth_dev_priv_dump; >>   }; >>   /** >> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >> index 917a320afa..07838cc2d9 100644 >> --- a/lib/ethdev/rte_ethdev.c >> +++ b/lib/ethdev/rte_ethdev.c >> @@ -6485,6 +6485,21 @@ rte_eth_rx_metadata_negotiate(uint16_t port_id, >> uint64_t *features) >>                  (*dev->dev_ops->rx_metadata_negotiate)(dev, features)); >>   } >> +int >> +rte_eth_dev_priv_dump(FILE *file, uint16_t port_id) >> +{ >> +    struct rte_eth_dev *dev; >> +    int ret; >> + >> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> +    dev = &rte_eth_devices[port_id]; >> + > > What do you think to check 'file' pointer against NULL before passing it to > dev_ops? > >> +    RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->eth_dev_priv_dump, -ENOTSUP); >> +    ret = (*dev->dev_ops->eth_dev_priv_dump)(file, dev); > > Should 'eth_err()' used here? > >> + >> +    return ret; >> +} >> + >>   RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO); >>   RTE_INIT(ethdev_init_telemetry) >> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h >> index 147cc1ced3..1c00fdbcd2 100644 >> --- a/lib/ethdev/rte_ethdev.h >> +++ b/lib/ethdev/rte_ethdev.h >> @@ -5902,6 +5902,22 @@ rte_eth_tx_buffer(uint16_t port_id, uint16_t >> queue_id, >>       return rte_eth_tx_buffer_flush(port_id, queue_id, buffer); >>   } >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change, or be removed, without prior >> notice >> + * >> + * Dump ethdev private info to a file. >> + * > > Ditto. > > Should we highlight that provided data and the order depends on the PMD? > >> + * @param file >> + *   A pointer to a file for output. >> + * @param port_id >> + *   The port identifier of the Ethernet device. >> + * @return >> + *   Negative errno value on error, positive value on success. > > > What does 'errno' mean here, does the API cause errno to be set and > return it? > > > Also it is possible to list possible specific error values, as done with > other > API documentation, like: > (0) if successful. > (-ENODEV) if *port_id* is invalid. > (-ENOTSUP) if hardware doesn't support. > .... > >> + */ >> +__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? > >>   #ifdef __cplusplus >>   } >>   #endif >> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map >> index 1f7359c846..376ea139aa 100644 >> --- a/lib/ethdev/version.map >> +++ b/lib/ethdev/version.map >> @@ -256,6 +256,9 @@ EXPERIMENTAL { >>       rte_flow_flex_item_create; >>       rte_flow_flex_item_release; >>       rte_flow_pick_transfer_proxy; >> + >> +    # added in 22.03 >> +    rte_eth_dev_priv_dump; >>   }; >>   INTERNAL { > > .