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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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