From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DF0C842365; Thu, 12 Oct 2023 11:34:12 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CC08E402AE; Thu, 12 Oct 2023 11:34:12 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 6982D4028C for ; Thu, 12 Oct 2023 11:34:11 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1697103250; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ZBX8O3YGpLn+UVXw9kYvwihxYacoPuivlj/XwtKKB8c=; b=fM8Q2/DnHhFKebO5ZcCfIHKW7ChvG4QL89zeZHsmQGMFjiVE05WJXBhlYuk/oj2p+qpPQ8 dLtjj/cwU7PlIbkT78GqXCa6IHDbjbv6wUwz1I80JimYzNMjkIBSEfCdJCXdVaLmSN/DLP s+WE5P2087if7JpWki7Ldn1MVxCLMVU= Received: from mail-lj1-f200.google.com (mail-lj1-f200.google.com [209.85.208.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-518-Lvs3ccJJM5u5hBY6-kb19w-1; Thu, 12 Oct 2023 05:34:08 -0400 X-MC-Unique: Lvs3ccJJM5u5hBY6-kb19w-1 Received: by mail-lj1-f200.google.com with SMTP id 38308e7fff4ca-2c3c658e60aso6718031fa.3 for ; Thu, 12 Oct 2023 02:34:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697103247; x=1697708047; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ZBX8O3YGpLn+UVXw9kYvwihxYacoPuivlj/XwtKKB8c=; b=ThFEVouG7Bjr2kxoSnoryFhgtLoUKIrN9FGsupgEkHwlPwMygupoqXIUiH1sHIqRVz vhLj/Edf+LOiVu9T4nnk9l0MWkE4fOJUxpsGr224Bh4h2qAiiU1RxRayxbTzJuc6tePf VcPVTQ6f9T+5GGhCdFchfP4guc7WhXEM7fDNI7Wz0T4m/k2qnOZKlnk7YvFC51ddGyeT CsD0bCrfXXGtgykg6whcJ4pBhZIuajaxLmsVqGQIAEptbt/GJ5jwBE6KMb6NXLr5jsrA Q25NqzWCVebUl3sXyURFHaR5g5mdb2nsURmF4WVek3/hG/iJhgc2a8pDtNhL/oFAwVD0 vDtQ== X-Gm-Message-State: AOJu0YyiSRYgQuTzhWtW1X7N5+KBj+E9iXj2sO/cPYCdWrhy0/eZhqEf 8JiOVmeSXDjJZL/t04szQ/IljEDXtCkmhFfFpYEt6uBHk0fuqijKIKsE9UQhOan7/iew0biS4p5 6bGMBFePpRiueY3sQ37E= X-Received: by 2002:a2e:8914:0:b0:2c0:1385:8c86 with SMTP id d20-20020a2e8914000000b002c013858c86mr21967919lji.25.1697103247225; Thu, 12 Oct 2023 02:34:07 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGLTyYYhVXu8WQRZM+eiUAjb96gsuqU49o/FvPfT8vxqUrAM0G5lvZRNKZmfuPmyP4BhJNT5icBMK99b/n6e3E= X-Received: by 2002:a2e:8914:0:b0:2c0:1385:8c86 with SMTP id d20-20020a2e8914000000b002c013858c86mr21967899lji.25.1697103246899; Thu, 12 Oct 2023 02:34:06 -0700 (PDT) MIME-Version: 1.0 References: <20230721160422.3848154-1-ferruh.yigit@amd.com> <20230928205930.2619353-1-ferruh.yigit@amd.com> In-Reply-To: <20230928205930.2619353-1-ferruh.yigit@amd.com> From: David Marchand Date: Thu, 12 Oct 2023 11:33:55 +0200 Message-ID: Subject: Re: [PATCH] ethdev: clarify device queue state after start and stop To: Ferruh Yigit Cc: Thomas Monjalon , Andrew Rybchenko , dev@dpdk.org, Jie Hai , Song Jiale , Yuan Peng , Raslan Darawsheh , Qiming Yang , Ivan Malov , Huisong Li X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hello Ferruh, On Thu, Sep 28, 2023 at 10:59=E2=80=AFPM 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. > > [1] > commit 3c4426db54fc ("app/testpmd: do not poll stopped queues") > > [2] > commit 5028f207a4fa ("app/testpmd: fix secondary process packet forwardin= g") > > 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 > --- > Cc: Jie Hai > Cc: Song Jiale > Cc: Yuan Peng > Cc: Raslan Darawsheh > Cc: Qiming Yang > Cc: Ivan Malov > Cc: Huisong Li > > 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 =3D { > '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', 'e= ventdev'], > 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 > +#include > + > +#include > +#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 =3D 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(ð_conf, 0, sizeof(eth_conf)); > + ret =3D rte_eth_dev_configure(port_id, NUM_RXQ, NUM_TXQ, = ð_conf); > + TEST_ASSERT(ret =3D=3D 0, > + "Port(%u) failed to configure.\n", port_id); > + > + /* RxQ setup */ > + for (uint16_t queue_id =3D 0; queue_id < NUM_RXQ; queue_i= d++) { > + ret =3D rte_eth_rx_queue_setup(port_id, queue_id,= NUM_RXD, > + rte_socket_id(), NULL, mbuf_pool); > + TEST_ASSERT(ret =3D=3D 0, > + "Port(%u), queue(%u) failed to setup RxQ.= \n", > + port_id, queue_id); > + } > + > + /* TxQ setup */ > + for (uint16_t queue_id =3D 0; queue_id < NUM_TXQ; queue_i= d++) { > + ret =3D rte_eth_tx_queue_setup(port_id, queue_id,= NUM_TXD, > + rte_socket_id(), NULL); > + TEST_ASSERT(ret =3D=3D 0, > + "Port(%u), queue(%u) failed to setup TxQ.= \n", > + port_id, queue_id); > + } > + > + ret =3D rte_eth_dev_info_get(port_id, &dev_info); > + TEST_ASSERT(ret =3D=3D 0, > + "Port(%u) failed to get dev info.\n", port_id); > + > + /* Initial RxQ */ > + for (uint16_t queue_id =3D 0; queue_id < dev_info.nb_rx_q= ueues; queue_id++) { > + ret =3D rte_eth_rx_queue_info_get(port_id, queue_= id, &rx_qinfo); > + if (ret =3D=3D -ENOTSUP) > + continue; > + > + TEST_ASSERT(ret =3D=3D 0, > + "Port(%u), queue(%u) failed to get RxQ in= fo.\n", > + port_id, queue_id); > + > + TEST_ASSERT(rx_qinfo.queue_state =3D=3D RTE_ETH_Q= UEUE_STATE_STOPPED, > + "Wrong initial Rx queue(%u) state(%d)\n", > + queue_id, rx_qinfo.queue_state); > + } > + > + /* Initial TxQ */ > + for (uint16_t queue_id =3D 0; queue_id < dev_info.nb_tx_q= ueues; queue_id++) { > + ret =3D rte_eth_tx_queue_info_get(port_id, queue_= id, &tx_qinfo); > + if (ret =3D=3D -ENOTSUP) > + continue; > + > + TEST_ASSERT(ret =3D=3D 0, > + "Port(%u), queue(%u) failed to get TxQ in= fo.\n", > + port_id, queue_id); > + > + TEST_ASSERT(tx_qinfo.queue_state =3D=3D RTE_ETH_Q= UEUE_STATE_STOPPED, > + "Wrong initial Tx queue(%u) state(%d)\n", > + queue_id, tx_qinfo.queue_state); > + } > + > + > + ret =3D rte_eth_dev_start(port_id); > + TEST_ASSERT(ret =3D=3D 0, > + "Port(%u) failed to start.\n", port_id); > + > + /* Started RxQ */ > + for (uint16_t queue_id =3D 0; queue_id < dev_info.nb_rx_q= ueues; queue_id++) { > + ret =3D rte_eth_rx_queue_info_get(port_id, queue_= id, &rx_qinfo); > + if (ret =3D=3D -ENOTSUP) > + continue; > + > + TEST_ASSERT(ret =3D=3D 0, > + "Port(%u), queue(%u) failed to get RxQ in= fo.\n", > + port_id, queue_id); > + > + TEST_ASSERT(rx_qinfo.queue_state =3D=3D RTE_ETH_Q= UEUE_STATE_STARTED, > + "Wrong started Rx queue(%u) state(%d)\n", > + queue_id, rx_qinfo.queue_state); > + } > + > + /* Started TxQ */ > + for (uint16_t queue_id =3D 0; queue_id < dev_info.nb_tx_q= ueues; queue_id++) { > + ret =3D rte_eth_tx_queue_info_get(port_id, queue_= id, &tx_qinfo); > + if (ret =3D=3D -ENOTSUP) > + continue; > + > + TEST_ASSERT(ret =3D=3D 0, > + "Port(%u), queue(%u) failed to get TxQ in= fo.\n", > + port_id, queue_id); > + > + TEST_ASSERT(tx_qinfo.queue_state =3D=3D RTE_ETH_Q= UEUE_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 =3D rte_eth_dev_stop(port_id); > + TEST_ASSERT(ret =3D=3D 0, > + "Port(%u) failed to stop.\n", port_id); > + > + /* Stopped RxQ */ > + for (uint16_t queue_id =3D 0; queue_id < dev_info.nb_rx_q= ueues; queue_id++) { > + ret =3D rte_eth_rx_queue_info_get(port_id, queue_= id, &rx_qinfo); > + if (ret =3D=3D -ENOTSUP) > + continue; > + > + TEST_ASSERT(ret =3D=3D 0, > + "Port(%u), queue(%u) failed to get RxQ in= fo.\n", > + port_id, queue_id); > + > + TEST_ASSERT(rx_qinfo.queue_state =3D=3D RTE_ETH_Q= UEUE_STATE_STOPPED, > + "Wrong stopped Rx queue(%u) state(%d)\n", > + queue_id, rx_qinfo.queue_state); > + } > + > + /* Stopped TxQ */ > + for (uint16_t queue_id =3D 0; queue_id < dev_info.nb_tx_q= ueues; queue_id++) { > + ret =3D rte_eth_tx_queue_info_get(port_id, queue_= id, &tx_qinfo); > + if (ret =3D=3D -ENOTSUP) > + continue; > + > + TEST_ASSERT(ret =3D=3D 0, > + "Port(%u), queue(%u) failed to get TxQ in= fo.\n", > + port_id, queue_id); > + > + TEST_ASSERT(tx_qinfo.queue_state =3D=3D RTE_ETH_Q= UEUE_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 =3D { > + .suite_name =3D "ethdev API tests", > + .setup =3D NULL, > + .teardown =3D NULL, > + .unit_test_cases =3D { > + 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(ðdev_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, uin= t16_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 b= e > + * `RTE_ETH_QUEUE_STATE_STARTED` after start. > + * > * On success, all basic functions exported by the Ethernet API (link st= atus, > * 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` afte= r stop. > + * > * @param port_id > * The port identifier of the Ethernet device. > * @return > -- > 2.34.1 > The rest lgtm. --=20 David Marchand