Re: [RFC] ethdev: clarify device queue state after start and stop

["lihuisong (C)" <lihuisong@huawei.com>]

在 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 <ferruh.yigit@amd.com>
> ---
> Cc: Jie Hai <haijie1@huawei.com>
> Cc: Song Jiale <songx.jiale@intel.com>
> Cc: Yuan Peng <yuan.peng@intel.com>
> Cc: Raslan Darawsheh <rasland@nvidia.com>
> Cc: Qiming Yang <qiming.yang@intel.com>
> ---
>   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 <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_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(&eth_conf, 0, sizeof(dev_info));
+1 need to modify.
> +		ret = rte_eth_dev_configure(port_id, NUM_RXQ, NUM_TXQ, &eth_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(&ethdev_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