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

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

[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));
+		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));*/
+		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);
+
+		/* 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);
+
+			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
-- 
2.34.1

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

[Ivan Malov]

Hi Ferruh,

The commit log says "commit [1]" and "commit [2]" but
does not seem to provide the URLs for the [1] and [2].
What are these resources? I'd like to know.

Thank you.

On Fri, 21 Jul 2023, Ferruh Yigit 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.
>
> 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));
> +		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));*/
> +		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);
> +
> +		/* 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);
> +
> +			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
> -- 
> 2.34.1
>
>

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

[Ivan Malov]

Hi Ferruh,

PSB

Thank you.

On Fri, 21 Jul 2023, Ferruh Yigit 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.
>
> 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));
This clears "eth_conf", but the sizeof is "dev_info". Why?

> +		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));*/
> +		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);
> +
> +		/* 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);
> +
> +			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
> -- 
> 2.34.1
>
>

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

[Ferruh Yigit]

On 7/21/2023 6:33 PM, Ivan Malov wrote:
> Hi Ferruh,
> 
> The commit log says "commit [1]" and "commit [2]" but
> does not seem to provide the URLs for the [1] and [2].
> What are these resources? I'd like to know.
> 

Ahh, I missed to add them to commit log, will add to next version:

[1]
3c4426db54fc ("app/testpmd: do not poll stopped queues")

[2]
5028f207a4fa ("app/testpmd: fix secondary process packet forwarding")


> Thank you.
> 
> On Fri, 21 Jul 2023, Ferruh Yigit 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.
>>
>> 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));
>> +        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));*/
>> +        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);
>> +
>> +        /* 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);
>> +
>> +            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
>> -- 
>> 2.34.1
>>
>>

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

[Ferruh Yigit]

On 7/21/2023 6:42 PM, Ivan Malov wrote:
> Hi Ferruh,
> 
> PSB
> 
> Thank you.
> 
> On Fri, 21 Jul 2023, Ferruh Yigit 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.
>>
>> 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));
> This clears "eth_conf", but the sizeof is "dev_info". Why?
> 

Will fix in next version, thx.

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

[lihuisong (C)]

在 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

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

[Ferruh Yigit]

On 7/24/2023 2:43 AM, lihuisong (C) wrote:
> 
> 在 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.
>

ack


>> +        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?
>

will remove

>> +        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.
>

Yes, it can be used as NUM_RXQ and NUM_TXQ, but I think it is better for
testing to get this from the device, as it can be more close the what
application does.

>> +
>> +        /* 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.
>

True, will skip if ret is '-ENOTSUP'

>> +
>> +            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

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

[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.

[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.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>
Cc: Ivan Malov <ivan.malov@arknetworks.am>
Cc: Huisong Li <lihuisong@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'],
     '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;
+
+	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(eth_conf));
+		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 */
+		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);
+		}
+
+
+		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(&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 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

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

[David Marchand]

Hello Ferruh,

On Thu, Sep 28, 2023 at 10:59 PM Ferruh Yigit <ferruh.yigit@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.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>
> Cc: Ivan Malov <ivan.malov@arknetworks.am>
> Cc: Huisong Li <lihuisong@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(&eth_conf, 0, sizeof(eth_conf));
> +               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 */
> +               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(&ethdev_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

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

[Ferruh Yigit]

On 10/12/2023 10:33 AM, David Marchand wrote:
> Hello Ferruh,
> 
> On Thu, Sep 28, 2023 at 10:59 PM Ferruh Yigit <ferruh.yigit@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.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>
>> Cc: Ivan Malov <ivan.malov@arknetworks.am>
>> Cc: Huisong Li <lihuisong@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
> 


ack

>>      '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.
> 

+1 to TEST_SKIPPED

> 
>> +       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(eth_conf));
>> +               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 */
>> +               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.
> 

ack

>> +               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(&ethdev_api_testsuite);
>> +}
>> +
>> +REGISTER_TEST_COMMAND(ethdev_api, test_ethdev_api);
> 
> REGISTER_FAST_TEST or REGISTER_DRIVER_TEST.
> 

This is not driver test, and not ready enough to add fast test, we need
to be sure all drivers updates queue state properly, so for now I think
better to keep macro as it is.

> 
>> 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.
> 
> 

Thanks for review, will send a new version soon

[PATCH v2] ethdev: clarify device queue state after start and stop

[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.

[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.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>
Cc: Ivan Malov <ivan.malov@arknetworks.am>
Cc: Huisong Li <lihuisong@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

v2:
* return test_skipped when there is no port available
* Syntax fixes
---
 app/test/meson.build       |   1 +
 app/test/test_ethdev_api.c | 185 +++++++++++++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.h    |   5 +
 3 files changed, 191 insertions(+)
 create mode 100644 app/test/test_ethdev_api.c

diff --git a/app/test/meson.build b/app/test/meson.build
index 20a9333c726d..dd0675638b5e 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -68,6 +68,7 @@ source_file_deps = {
     'test_efd.c': ['efd', 'net'],
     'test_efd_perf.c': ['efd', 'hash'],
     'test_errno.c': [],
+    'test_ethdev_api.c': ['ethdev'],
     'test_ethdev_link.c': ['ethdev'],
     'test_event_crypto_adapter.c': ['cryptodev', 'eventdev', 'bus_vdev'],
     'test_event_eth_rx_adapter.c': ['ethdev', 'eventdev', 'bus_vdev'],
diff --git a/app/test/test_ethdev_api.c b/app/test/test_ethdev_api.c
new file mode 100644
index 000000000000..dc72603d000f
--- /dev/null
+++ b/app/test/test_ethdev_api.c
@@ -0,0 +1,185 @@
+/* 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;
+
+	if (rte_eth_dev_count_avail() == 0)
+		return TEST_SKIPPED;
+
+	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(eth_conf));
+		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 */
+		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);
+		}
+
+		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(&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 f949dfc83dc2..85b9af7a02c0 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

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

[Thomas Monjalon]

13/10/2023 17:57, 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.
> 
> [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.

It is so basic that it may look stupid :)
Yes we still have to enforce such basic things, thank you for this work.

> +REGISTER_TEST_COMMAND(ethdev_api, test_ethdev_api);

Maybe add a comment here to explain it is not part of basic suites,
waiting for all drivers being compliant.

> + * 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.
> + *

Acked-by: Thomas Monjalon <thomas@monjalon.net>

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

[Ferruh Yigit]

On 10/16/2023 3:59 PM, Thomas Monjalon wrote:
> 13/10/2023 17:57, 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.
>>
>> [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.
> 
> It is so basic that it may look stupid :)
> Yes we still have to enforce such basic things, thank you for this work.
> 
>> +REGISTER_TEST_COMMAND(ethdev_api, test_ethdev_api);
> 
> Maybe add a comment here to explain it is not part of basic suites,
> waiting for all drivers being compliant.
> 

ack, I will add while merging

>> + * 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.
>> + *
> 
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> 

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

[Ferruh Yigit]

On 10/16/2023 3:59 PM, Thomas Monjalon wrote:
> 13/10/2023 17:57, 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.
>>
>> [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.
> 
> It is so basic that it may look stupid :)
> Yes we still have to enforce such basic things, thank you for this work.
> 
>> +REGISTER_TEST_COMMAND(ethdev_api, test_ethdev_api);
> 
> Maybe add a comment here to explain it is not part of basic suites,
> waiting for all drivers being compliant.
> 

Following comment added while merging:

 /* TODO: Make part of the fast test suite, `REGISTER_FAST_TEST()`,
  *       when all drivers complies to the queue state requirement
  */


>> + * 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.
>> + *
> 
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> 

Applied to dpdk-next-net/main, thanks.