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 A772CA054D; Tue, 7 Jun 2022 16:00:00 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4F4E54021D; Tue, 7 Jun 2022 16:00:00 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 5F36840156 for ; Tue, 7 Jun 2022 15:59:57 +0200 (CEST) Received: from kwepemi500017.china.huawei.com (unknown [172.30.72.55]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4LHX4Y3KbpzgYXD; Tue, 7 Jun 2022 21:58:05 +0800 (CST) Received: from [10.67.103.235] (10.67.103.235) by kwepemi500017.china.huawei.com (7.221.188.110) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Tue, 7 Jun 2022 21:59:53 +0800 Subject: Re: [PATCH v3 1/4] ethdev: introduce ethdev HW desc dump PI To: Andrew Rybchenko , "Min Hu (Connor)" , References: <20220527023351.40577-1-humin29@huawei.com> <20220601074930.10313-1-humin29@huawei.com> <20220601074930.10313-2-humin29@huawei.com> <108f8a9a-e3a4-d8b8-c29c-6dbdaf3340e8@oktetlabs.ru> CC: Ferruh Yigit , Thomas Monjalon From: Dongdong Liu Message-ID: Date: Tue, 7 Jun 2022 21:59:52 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <108f8a9a-e3a4-d8b8-c29c-6dbdaf3340e8@oktetlabs.ru> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.103.235] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To kwepemi500017.china.huawei.com (7.221.188.110) 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 Andrew Many thanks for your review. On 2022/6/2 3:53, Andrew Rybchenko wrote: > On 6/1/22 10:49, Min Hu (Connor) wrote: >> Added the ethdev HW Rx desc dump API which provides functions for query >> HW descriptor from device. HW descriptor info differs in different NICs. >> The information demonstrates I/O process which is important for debug. >> As the information is different between NICs, the new API is introduced. >> >> Signed-off-by: Min Hu (Connor) >> --- >> doc/guides/rel_notes/release_22_07.rst | 7 ++++ >> lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++ >> lib/ethdev/rte_ethdev.c | 44 ++++++++++++++++++++++++++ >> lib/ethdev/rte_ethdev.h | 44 ++++++++++++++++++++++++++ >> lib/ethdev/version.map | 2 ++ >> 5 files changed, 139 insertions(+) >> >> diff --git a/doc/guides/rel_notes/release_22_07.rst >> b/doc/guides/rel_notes/release_22_07.rst >> index 8932a1d478..56c675121a 100644 >> --- a/doc/guides/rel_notes/release_22_07.rst >> +++ b/doc/guides/rel_notes/release_22_07.rst >> @@ -137,6 +137,13 @@ New Features >> * ``RTE_EVENT_QUEUE_ATTR_WEIGHT`` >> * ``RTE_EVENT_QUEUE_ATTR_AFFINITY`` >> +* **Added ethdev HW desc dump API, to dump Rx/Tx HW desc info from >> device.** >> + >> + Added the ethdev HW Rx desc dump API which provides functions for >> query >> + HW descriptor from device. HW descriptor info differs in different >> NICs. >> + The information demonstrates I/O process which is important for debug. >> + As the information is different between NICs, the new API is >> introduced. >> + >> Removed Items >> ------------- >> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h >> index 69d9dc21d8..9c1726eb2d 100644 >> --- a/lib/ethdev/ethdev_driver.h >> +++ b/lib/ethdev/ethdev_driver.h >> @@ -1073,6 +1073,42 @@ typedef int >> (*eth_ip_reassembly_conf_set_t)(struct rte_eth_dev *dev, >> */ >> typedef int (*eth_dev_priv_dump_t)(struct rte_eth_dev *dev, FILE >> *file); >> +/** >> + * @internal >> + * Dump ethdev Rx descriptor info to a file. > > ethdev is not required in the description above. It is an ethdev > operation type. Will remove it. > > Is there any limitations on caller context? Should be it the same > CPU core (since typically it is the case for per-queue API) which > polls the queue? The answer must be in the API function description. "[PATCH v3 4/4] app/procinfo: support descriptor dump" shows how to use the API. It is used for debug, not a dataplane API, no special limitations on caller context. > >> + * >> + * @param file >> + * A pointer to a file for output. >> + * @param dev >> + * Port (ethdev) handle. >> + * @param queue_id >> + * The selected queue. >> + * @param desc_id >> + * The selected descriptor. > > Is it an ID in the ring regardless of the current position or > is it offset relative to current position in the ring? > It should be clarified in any case. > I'd say that it should be an offset to be consistent with > descriptor status API. It is an ID in the ring regardless of the current position. > > It would be useful to be able to dump many descriptor at once. This can be done by the appliacation. > >> + * @return >> + * Negative errno value on error, zero on success. >> + */ >> +typedef int (*eth_rx_hw_desc_dump_t)(FILE *file, const struct >> rte_eth_dev *dev, >> + uint16_t queue_id, uint16_t desc_id); >> + >> +/** >> + * @internal >> + * Dump ethdev Tx descriptor info to a file. >> + * >> + * @param file >> + * A pointer to a file for output. >> + * @param dev >> + * Port (ethdev) handle. >> + * @param queue_id >> + * The selected queue. >> + * @param desc_id >> + * The selected descriptor. >> + * @return >> + * Negative errno value on error, zero on success. >> + */ >> +typedef int (*eth_tx_hw_desc_dump_t)(FILE *file, const struct >> rte_eth_dev *dev, >> + uint16_t queue_id, uint16_t desc_id); >> + >> /** >> * @internal A structure containing the functions exported by an >> Ethernet driver. >> */ >> @@ -1283,6 +1319,12 @@ struct eth_dev_ops { >> /** Dump private info from device */ >> eth_dev_priv_dump_t eth_dev_priv_dump; >> + >> + /** Dump ethdev Rx descriptor info */\\ > > It is an ethdev operations. So, 'ethdev' is not necessary in the > description. Will fix. > >> + eth_rx_hw_desc_dump_t eth_rx_hw_desc_dump; >> + >> + /** Dump ethdev Tx descriptor info */ >> + eth_tx_hw_desc_dump_t eth_tx_hw_desc_dump; >> }; >> /** >> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >> index 46c088dc88..bbd8439fa0 100644 >> --- a/lib/ethdev/rte_ethdev.c >> +++ b/lib/ethdev/rte_ethdev.c >> @@ -5876,6 +5876,50 @@ rte_eth_dev_priv_dump(uint16_t port_id, FILE >> *file) >> return eth_err(port_id, (*dev->dev_ops->eth_dev_priv_dump)(dev, >> file)); >> } >> +int >> +rte_eth_rx_hw_desc_dump(FILE *file, uint16_t port_id, uint16_t queue_id, >> + uint16_t desc_id) >> +{ >> + struct rte_eth_dev *dev; >> + int ret; >> + >> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> + dev = &rte_eth_devices[port_id]; > > Shouldn't we check file vs null? Yes, will do. > >> + >> + if (queue_id >= dev->data->nb_rx_queues) { >> + RTE_ETHDEV_LOG(ERR, "Invalid Rx queue_id=%u\n", queue_id); >> + return -EINVAL; >> + } > > Don't we need a check vs queue size? This need to be done in pmd as "[PATCH v3 3/4] net/hns3: support Rx/Tx bd dump" shows. Thanks, Dongdong > >> + >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->eth_rx_hw_desc_dump, >> -ENOTSUP); >> + ret = (*dev->dev_ops->eth_rx_hw_desc_dump)(file, dev, queue_id, >> + desc_id); >> + >> + return ret; >> +} > > . >