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

[David Marchand <david.marchand@redhat.com>]

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