Hello Ferruh, On Thu, Sep 28, 2023 at 10:59 PM Ferruh Yigit <ferruh.yi...@amd.com> wrote: > > 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. > > [1] > commit 3c4426db54fc ("app/testpmd: do not poll stopped queues") > > [2] > commit 5028f207a4fa ("app/testpmd: fix secondary process packet 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 <ferruh.yi...@amd.com> > --- > Cc: Jie Hai <haij...@huawei.com> > Cc: Song Jiale <songx.ji...@intel.com> > Cc: Yuan Peng <yuan.p...@intel.com> > Cc: Raslan Darawsheh <rasl...@nvidia.com> > Cc: Qiming Yang <qiming.y...@intel.com> > Cc: Ivan Malov <ivan.ma...@arknetworks.am> > Cc: Huisong Li <lihuis...@huawei.com> > > v1: > * fix memset > * remove commented out code > * update unit test to skip queue state if > rte_eth_[rt]x_queue_info_get() is not supported > --- > app/test/meson.build | 1 + > app/test/test_ethdev_api.c | 184 +++++++++++++++++++++++++++++++++++++ > lib/ethdev/rte_ethdev.h | 5 + > 3 files changed, 190 insertions(+) > create mode 100644 app/test/test_ethdev_api.c > > diff --git a/app/test/meson.build b/app/test/meson.build > index 05bae9216dbc..05bbe84868f6 100644 > --- a/app/test/meson.build > +++ b/app/test/meson.build > @@ -65,6 +65,7 @@ source_file_deps = { > 'test_efd_perf.c': ['efd', 'hash'], > 'test_errno.c': [], > 'test_ethdev_link.c': ['ethdev'], > + 'test_ethdev_api.c': ['ethdev'],
Nit: aphabetical sort > 'test_event_crypto_adapter.c': ['cryptodev', 'eventdev', 'bus_vdev'], > 'test_event_eth_rx_adapter.c': ['ethdev', 'eventdev', 'bus_vdev'], > 'test_event_eth_tx_adapter.c': ['bus_vdev', 'ethdev', 'net_ring', > 'eventdev'], > diff --git a/app/test/test_ethdev_api.c b/app/test/test_ethdev_api.c > new file mode 100644 > index 000000000000..68239f82ff33 > --- /dev/null > +++ b/app/test/test_ethdev_api.c > @@ -0,0 +1,184 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright (C) 2023, Advanced Micro Devices, Inc. > + */ > + > +#include <rte_log.h> > +#include <rte_ethdev.h> > + > +#include <rte_test.h> > +#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_conf eth_conf; > + uint16_t port_id; > + int ret; > + Should we return TEST_SKIPPED if no ethdev port is present? It seems more valid to me than returning TEST_SUCCESS. > + 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(eth_conf)); > + 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 */ > + 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 */ > + 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); > + > + /* 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); > + if (ret == -ENOTSUP) > + continue; > + > + 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); > + if (ret == -ENOTSUP) > + continue; > + > + 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 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); > + if (ret == -ENOTSUP) > + continue; > + > + 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); > + if (ret == -ENOTSUP) > + continue; > + > + 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); > + } > + > + Nit: I would remove one of those empty lines. > + 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); > + if (ret == -ENOTSUP) > + continue; > + > + 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); > + if (ret == -ENOTSUP) > + continue; > + > + 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), > + /* TODO: Add deferred_start queue status test */ > + 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); REGISTER_FAST_TEST or REGISTER_DRIVER_TEST. > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h > index 8542257721c9..6441fe049b06 100644 > --- a/lib/ethdev/rte_ethdev.h > +++ b/lib/ethdev/rte_ethdev.h > @@ -2812,6 +2812,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 start 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. > * > @@ -2828,6 +2831,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 > -- > 2.34.1 > The rest lgtm. -- David Marchand