DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@amd.com>
To: Thomas Monjalon <thomas@monjalon.net>,
	Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Cc: <dev@dpdk.org>, David Marchand <david.marchand@redhat.com>,
	Jie Hai <haijie1@huawei.com>, Song Jiale <songx.jiale@intel.com>,
	Yuan Peng <yuan.peng@intel.com>,
	Raslan Darawsheh <rasland@nvidia.com>,
	Qiming Yang <qiming.yang@intel.com>,
	Ivan Malov <ivan.malov@arknetworks.am>,
	Huisong Li <lihuisong@huawei.com>
Subject: [PATCH v2] ethdev: clarify device queue state after start and stop
Date: Fri, 13 Oct 2023 16:57:23 +0100	[thread overview]
Message-ID: <20231013155723.2631644-1-ferruh.yigit@amd.com> (raw)
In-Reply-To: <20230928205930.2619353-1-ferruh.yigit@amd.com>

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


  parent reply	other threads:[~2023-10-13 15:57 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-21 16:04 [RFC] " Ferruh Yigit
2023-07-21 17:33 ` Ivan Malov
2023-07-21 22:57   ` Ferruh Yigit
2023-07-21 17:42 ` Ivan Malov
2023-07-21 22:58   ` Ferruh Yigit
2023-07-24  1:43 ` lihuisong (C)
2023-09-28 19:30   ` Ferruh Yigit
2023-09-28 20:59 ` [PATCH] " Ferruh Yigit
2023-10-12  9:33   ` David Marchand
2023-10-13 11:36     ` Ferruh Yigit
2023-10-13 15:57   ` Ferruh Yigit [this message]
2023-10-16 14:59     ` [PATCH v2] " Thomas Monjalon
2023-10-16 15:12       ` Ferruh Yigit
2023-10-16 15:52       ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231013155723.2631644-1-ferruh.yigit@amd.com \
    --to=ferruh.yigit@amd.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=haijie1@huawei.com \
    --cc=ivan.malov@arknetworks.am \
    --cc=lihuisong@huawei.com \
    --cc=qiming.yang@intel.com \
    --cc=rasland@nvidia.com \
    --cc=songx.jiale@intel.com \
    --cc=thomas@monjalon.net \
    --cc=yuan.peng@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).