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 71E4C42EF9; Mon, 24 Jul 2023 03:43:21 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 477E940ED6; Mon, 24 Jul 2023 03:43:21 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 9BD83406B7 for ; Mon, 24 Jul 2023 03:43:19 +0200 (CEST) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.55]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4R8NDn6cFKzVjYP; Mon, 24 Jul 2023 09:41:45 +0800 (CST) Received: from [10.67.103.231] (10.67.103.231) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27; Mon, 24 Jul 2023 09:43:15 +0800 Message-ID: <8b056404-5cdf-3a6c-0c21-9813a0141814@huawei.com> Date: Mon, 24 Jul 2023 09:43:15 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [RFC] ethdev: clarify device queue state after start and stop To: Ferruh Yigit , Thomas Monjalon , Andrew Rybchenko CC: , David Marchand , Jie Hai , Song Jiale , Yuan Peng , Raslan Darawsheh , Qiming Yang References: <20230721160422.3848154-1-ferruh.yigit@amd.com> From: "lihuisong (C)" In-Reply-To: <20230721160422.3848154-1-ferruh.yigit@amd.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To kwepemm600004.china.huawei.com (7.193.23.242) 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 在 2023/7/22 0:04, Ferruh Yigit 写道: > Drivers start/stop device queues on port start/stop, but not all drivers > update queue state accordingly. > > This becomes more visible if a specific queue stopped explicitly and > port stopped/started later, in this case although all queues are > started, the state of that specific queue is stopped and it is > misleading. > > Misrepresentation of queue state became a defect with commit [1] that > does forwarding decision based on queue state and commit [2] that gets > up to date queue state from ethdev/device before forwarding. > > This patch documents that status of all queues of a device should be > `RTE_ETH_QUEUE_STATE_STOPPED` after port stop and their status should > be`RTE_ETH_QUEUE_STATE_STARTED` after port start. > > Also an unit test added to verify drivers. > > Signed-off-by: Ferruh Yigit > --- > Cc: Jie Hai > Cc: Song Jiale > Cc: Yuan Peng > Cc: Raslan Darawsheh > Cc: Qiming Yang > --- > app/test/meson.build | 2 + > app/test/test_ethdev_api.c | 169 +++++++++++++++++++++++++++++++++++++ > lib/ethdev/rte_ethdev.h | 5 ++ > 3 files changed, 176 insertions(+) > create mode 100644 app/test/test_ethdev_api.c > > diff --git a/app/test/meson.build b/app/test/meson.build > index b89cf0368fb5..8e409cf59c35 100644 > --- a/app/test/meson.build > +++ b/app/test/meson.build > @@ -49,6 +49,7 @@ test_sources = files( > 'test_efd_perf.c', > 'test_errno.c', > 'test_ethdev_link.c', > + 'test_ethdev_api.c', > 'test_event_crypto_adapter.c', > 'test_event_eth_rx_adapter.c', > 'test_event_ring.c', > @@ -187,6 +188,7 @@ fast_tests = [ > ['eal_fs_autotest', true, true], > ['errno_autotest', true, true], > ['ethdev_link_status', true, true], > + ['ethdev_api', true, true], > ['event_ring_autotest', true, true], > ['fib_autotest', true, true], > ['fib6_autotest', true, true], > diff --git a/app/test/test_ethdev_api.c b/app/test/test_ethdev_api.c > new file mode 100644 > index 000000000000..1b4569396dda > --- /dev/null > +++ b/app/test/test_ethdev_api.c > @@ -0,0 +1,169 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright (C) 2023, Advanced Micro Devices, Inc. > + */ > + > +#include > +#include > + > +#include > +#include "test.h" > + > +#define NUM_RXQ 2 > +#define NUM_TXQ 2 > +#define NUM_RXD 512 > +#define NUM_TXD 512 > +#define NUM_MBUF 1024 > +#define MBUF_CACHE_SIZE 256 > + > +static int32_t > +ethdev_api_queue_status(void) > +{ > + struct rte_eth_dev_info dev_info; > + struct rte_eth_rxq_info rx_qinfo; > + struct rte_eth_txq_info tx_qinfo; > + struct rte_mempool *mbuf_pool; > + /*struct rte_eth_rxconf rx_conf;*/ > + /*struct rte_eth_txconf tx_conf;*/ > + struct rte_eth_conf eth_conf; > + uint16_t port_id; > + int ret; > + > + mbuf_pool = rte_pktmbuf_pool_create("MBUF_POOL", NUM_MBUF, MBUF_CACHE_SIZE, 0, > + RTE_MBUF_DEFAULT_BUF_SIZE, rte_socket_id()); > + > + RTE_ETH_FOREACH_DEV(port_id) { > + memset(ð_conf, 0, sizeof(dev_info)); +1 need to modify. > + ret = rte_eth_dev_configure(port_id, NUM_RXQ, NUM_TXQ, ð_conf); > + TEST_ASSERT(ret == 0, > + "Port(%u) failed to configure.\n", port_id); > + > + /* RxQ setup */ > + /*memset(&rx_conf, 0, sizeof(rx_conf));*/ What's here for? > + for (uint16_t queue_id = 0; queue_id < NUM_RXQ; queue_id++) { > + ret = rte_eth_rx_queue_setup(port_id, queue_id, NUM_RXD, > + rte_socket_id(), NULL, mbuf_pool); > + TEST_ASSERT(ret == 0, > + "Port(%u), queue(%u) failed to setup RxQ.\n", > + port_id, queue_id); > + } > + > + /* TxQ setup */ > + /*memset(&tx_conf, 0, sizeof(tx_conf));*/ > + for (uint16_t queue_id = 0; queue_id < NUM_TXQ; queue_id++) { > + ret = rte_eth_tx_queue_setup(port_id, queue_id, NUM_TXD, > + rte_socket_id(), NULL); > + TEST_ASSERT(ret == 0, > + "Port(%u), queue(%u) failed to setup TxQ.\n", > + port_id, queue_id); > + } > + > + ret = rte_eth_dev_info_get(port_id, &dev_info); > + TEST_ASSERT(ret == 0, > + "Port(%u) failed to get dev info.\n", port_id); dev_info_get() seems an useless code here. Because NUM_RXQ and NUM_TXQ can be used directly in following codes. > + > + /* Initial RxQ */ > + for (uint16_t queue_id = 0; queue_id < dev_info.nb_rx_queues; queue_id++) { > + ret = rte_eth_rx_queue_info_get(port_id, queue_id, &rx_qinfo); > + TEST_ASSERT(ret == 0, > + "Port(%u), queue(%u) failed to get RxQ info.\n", > + port_id, queue_id); > + > + TEST_ASSERT(rx_qinfo.queue_state == RTE_ETH_QUEUE_STATE_STOPPED, > + "Wrong initial Rx queue(%u) state(%d)\n", > + queue_id, rx_qinfo.queue_state); > + } > + > + /* Initial TxQ */ > + for (uint16_t queue_id = 0; queue_id < dev_info.nb_tx_queues; queue_id++) { > + ret = rte_eth_tx_queue_info_get(port_id, queue_id, &tx_qinfo); > + TEST_ASSERT(ret == 0, > + "Port(%u), queue(%u) failed to get TxQ info.\n", > + port_id, queue_id); Some drivers don't support this API. So it should be ok for these drivers. > + > + TEST_ASSERT(tx_qinfo.queue_state == RTE_ETH_QUEUE_STATE_STOPPED, > + "Wrong initial Tx queue(%u) state(%d)\n", > + queue_id, tx_qinfo.queue_state); > + } > + > + > + ret = rte_eth_dev_start(port_id); > + TEST_ASSERT(ret == 0, > + "Port(%u) failed to start.\n", port_id); > + > + /* Started RxQ */ > + for (uint16_t queue_id = 0; queue_id < dev_info.nb_rx_queues; queue_id++) { > + ret = rte_eth_rx_queue_info_get(port_id, queue_id, &rx_qinfo); > + TEST_ASSERT(ret == 0, > + "Port(%u), queue(%u) failed to get RxQ info.\n", > + port_id, queue_id); > + > + TEST_ASSERT(rx_qinfo.queue_state == RTE_ETH_QUEUE_STATE_STARTED, > + "Wrong started Rx queue(%u) state(%d)\n", > + queue_id, rx_qinfo.queue_state); > + } > + > + /* Started TxQ */ > + for (uint16_t queue_id = 0; queue_id < dev_info.nb_tx_queues; queue_id++) { > + ret = rte_eth_tx_queue_info_get(port_id, queue_id, &tx_qinfo); > + TEST_ASSERT(ret == 0, > + "Port(%u), queue(%u) failed to get TxQ info.\n", > + port_id, queue_id); > + > + TEST_ASSERT(tx_qinfo.queue_state == RTE_ETH_QUEUE_STATE_STARTED, > + "Wrong started Tx queue(%u) state(%d)\n", > + queue_id, tx_qinfo.queue_state); > + } > + > + > + ret = rte_eth_dev_stop(port_id); > + TEST_ASSERT(ret == 0, > + "Port(%u) failed to stop.\n", port_id); > + > + /* Stopped RxQ */ > + for (uint16_t queue_id = 0; queue_id < dev_info.nb_rx_queues; queue_id++) { > + ret = rte_eth_rx_queue_info_get(port_id, queue_id, &rx_qinfo); > + TEST_ASSERT(ret == 0, > + "Port(%u), queue(%u) failed to get RxQ info.\n", > + port_id, queue_id); > + > + TEST_ASSERT(rx_qinfo.queue_state == RTE_ETH_QUEUE_STATE_STOPPED, > + "Wrong stopped Rx queue(%u) state(%d)\n", > + queue_id, rx_qinfo.queue_state); > + } > + > + /* Stopped TxQ */ > + for (uint16_t queue_id = 0; queue_id < dev_info.nb_tx_queues; queue_id++) { > + ret = rte_eth_tx_queue_info_get(port_id, queue_id, &tx_qinfo); > + TEST_ASSERT(ret == 0, > + "Port(%u), queue(%u) failed to get TxQ info.\n", > + port_id, queue_id); > + > + TEST_ASSERT(tx_qinfo.queue_state == RTE_ETH_QUEUE_STATE_STOPPED, > + "Wrong stopped Tx queue(%u) state(%d)\n", > + queue_id, tx_qinfo.queue_state); > + } > + } > + > + return TEST_SUCCESS; > +} > + > +static struct unit_test_suite ethdev_api_testsuite = { > + .suite_name = "ethdev API tests", > + .setup = NULL, > + .teardown = NULL, > + .unit_test_cases = { > + TEST_CASE(ethdev_api_queue_status), > + TEST_CASES_END() /**< NULL terminate unit test array */ > + } > +}; > + > +static int > +test_ethdev_api(void) > +{ > + rte_log_set_global_level(RTE_LOG_DEBUG); > + rte_log_set_level(RTE_LOGTYPE_EAL, RTE_LOG_DEBUG); > + > + return unit_test_suite_runner(ðdev_api_testsuite); > +} > + > +REGISTER_TEST_COMMAND(ethdev_api, test_ethdev_api); > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h > index 3d44979b44f7..8f2e0f158cc4 100644 > --- a/lib/ethdev/rte_ethdev.h > +++ b/lib/ethdev/rte_ethdev.h > @@ -2788,6 +2788,9 @@ int rte_eth_dev_tx_queue_stop(uint16_t port_id, uint16_t tx_queue_id); > * Device RTE_ETH_DEV_NOLIVE_MAC_ADDR flag causes MAC address to be set before > * PMD port start callback function is invoked. > * > + * All device queues (except form deferred queues) status should be > + * `RTE_ETH_QUEUE_STATE_STARTED` after start. > + * > * On success, all basic functions exported by the Ethernet API (link status, > * receive/transmit, and so on) can be invoked. > * > @@ -2804,6 +2807,8 @@ int rte_eth_dev_start(uint16_t port_id); > * Stop an Ethernet device. The device can be restarted with a call to > * rte_eth_dev_start() > * > + * All device queues status should be `RTE_ETH_QUEUE_STATE_STOPPED` after stop. > + * > * @param port_id > * The port identifier of the Ethernet device. > * @return