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 7CA36A0553; Sat, 11 Jun 2022 11:04:55 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 273BE40222; Sat, 11 Jun 2022 11:04:55 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 3A22D40041 for ; Sat, 11 Jun 2022 11:04:53 +0200 (CEST) Received: from kwepemi500017.china.huawei.com (unknown [172.30.72.54]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4LKsLj4n8fzjXWW; Sat, 11 Jun 2022 17:03:25 +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; Sat, 11 Jun 2022 17:04:49 +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> <90c20152-9dc7-95f6-4229-434bcbf7cfb8@oktetlabs.ru> CC: Ferruh Yigit , Thomas Monjalon , Slava Ovsiienko , Matan Azrad , Jerin Jacob Kollanukkaran , Qiming Yang , Qi Zhang , Beilei Xing , Nithin Dabilpuram , Kiran Kumar K , Sunil Kumar Kori , Satha Rao , Stephen Hemminger , Long Li , Ajit Khaparde , Somnath Kotur From: Dongdong Liu Message-ID: Date: Sat, 11 Jun 2022 17:04:49 +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: <90c20152-9dc7-95f6-4229-434bcbf7cfb8@oktetlabs.ru> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.103.235] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) 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/8 18:09, Andrew Rybchenko wrote: > Cc various driver maintainers looking for opinion about API discussion > below > > On 6/7/22 16:59, Dongdong Liu wrote: >> 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. > > Please, be explicit in the ethdev API documentation. OK, will do > >>> >>>> + * >>>> + * @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. > > IMHO, it is inconvenient since typically it is interesting > what happens nearby current position. > OK, will fix. >>> >>> It would be useful to be able to dump many descriptor at once. >> This can be done by the appliacation. > > Yes, that's true, but easy to do in the driver as well. It would > be especially important if ID semantics changes. How about changing parameter 'desc_id' to 'num' to dump [current position, current position + num - 1] descriptors. /** * @internal * Dump Rx 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 num * The number of the descriptors to dump. * @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 num); > >>> >>>> + * @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. > > All applicable generic checks must be done on ethdev level to avoid code > duplication and missing checks in PMDs. The ethdev level does not know the queue size. This is similar to rte_eth_rx_descriptor_status(), queue size check is also done in PMDs. Thanks, Dongdong. > > Andrew. > . >