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 443E045C91; Wed, 6 Nov 2024 13:19:43 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D0431402C3; Wed, 6 Nov 2024 13:19:42 +0100 (CET) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 541FD40265 for ; Wed, 6 Nov 2024 13:19:41 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id E110D21D15; Wed, 6 Nov 2024 13:19:40 +0100 (CET) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH 2/2] drivers/net: support single queue per port Date: Wed, 6 Nov 2024 13:19:40 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F88A@smartserver.smartshare.dk> X-MimeOLE: Produced By Microsoft Exchange V6.5 In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH 2/2] drivers/net: support single queue per port Thread-Index: AdswQmFyGLcp20JHQUGuxVyvXAxmIwAAbLOg References: <20241025115223.1230680-1-mb@smartsharesystems.com> <20241025115223.1230680-3-mb@smartsharesystems.com> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" Cc: , "Tyler Retzlaff" , "Thomas Monjalon" , "Ferruh Yigit" , "Andrew Rybchenko" , "Ajit Khaparde" , "Somnath Kotur" , "Gaetan Rivet" , "Jie Hai" , "Long Li" , "Wei Hu" 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 > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > Sent: Wednesday, 6 November 2024 12.52 >=20 > On Fri, Oct 25, 2024 at 11:52:23AM +0000, Morten Br=F8rup wrote: > > When configuring DPDK for one queue per port > > (#define RTE_MAX_QUEUES_PER_PORT 1), compilation of some network > drivers > > fails with e.g.: > > > > ../drivers/net/bnxt/bnxt_rxq.c: In function 'bnxt_rx_queue_stop': > > ../drivers/net/bnxt/bnxt_rxq.c:587:34: error: array subscript 1 is > above array bounds of 'uint8_t[1]' {aka 'unsigned char[1]'} [- > Werror=3Darray-bounds=3D] > > 587 | dev->data->rx_queue_state[q_id] =3D > RTE_ETH_QUEUE_STATE_STOPPED; > > | ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~ > > In file included from ../drivers/net/bnxt/bnxt.h:16, > > from ../drivers/net/bnxt/bnxt_rxq.c:10: > > ../lib/ethdev/ethdev_driver.h:168:17: note: while referencing > 'rx_queue_state' > > 168 | uint8_t rx_queue_state[RTE_MAX_QUEUES_PER_PORT]; > > | ^~~~~~~~~~~~~~ > > > > To fix this, a hint is added to the network drivers where a compiler > in > > the CI has been seen to emit the above error when DPDK is configured > for > > one queue per port, but we know that the error cannot occur. > > > > Signed-off-by: Morten Br=F8rup > > --- > > drivers/net/bnxt/bnxt_ethdev.c | 2 ++ > > drivers/net/bnxt/bnxt_rxq.c | 1 + > > drivers/net/e1000/igb_rxtx.c | 2 ++ > > drivers/net/failsafe/failsafe_ops.c | 10 ++++++++-- > > drivers/net/hns3/hns3_rxtx.c | 2 ++ > > drivers/net/mana/tx.c | 1 + > > 6 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/bnxt/bnxt_ethdev.c > b/drivers/net/bnxt/bnxt_ethdev.c > > index 1f7c0d77d5..136e308437 100644 > > --- a/drivers/net/bnxt/bnxt_ethdev.c > > +++ b/drivers/net/bnxt/bnxt_ethdev.c > > @@ -910,6 +910,7 @@ static int bnxt_start_nic(struct bnxt *bp) > > struct bnxt_rx_queue *rxq =3D bp->rx_queues[j]; > > > > if (!rxq->rx_deferred_start) { > > + __rte_assume(j < RTE_MAX_QUEUES_PER_PORT); > > bp->eth_dev->data->rx_queue_state[j] =3D > > RTE_ETH_QUEUE_STATE_STARTED; > > rxq->rx_started =3D true; > > @@ -930,6 +931,7 @@ static int bnxt_start_nic(struct bnxt *bp) > > struct bnxt_tx_queue *txq =3D bp->tx_queues[j]; > > > > if (!txq->tx_deferred_start) { > > + __rte_assume(j < RTE_MAX_QUEUES_PER_PORT); > > bp->eth_dev->data->tx_queue_state[j] =3D > > RTE_ETH_QUEUE_STATE_STARTED; > > txq->tx_started =3D true; > > diff --git a/drivers/net/bnxt/bnxt_rxq.c > b/drivers/net/bnxt/bnxt_rxq.c > > index 1c25c57ca6..1651c26545 100644 > > --- a/drivers/net/bnxt/bnxt_rxq.c > > +++ b/drivers/net/bnxt/bnxt_rxq.c > > @@ -584,6 +584,7 @@ int bnxt_rx_queue_stop(struct rte_eth_dev *dev, > uint16_t rx_queue_id) > > return -EINVAL; > > } > > > > + __rte_assume(q_id < RTE_MAX_QUEUES_PER_PORT); > > dev->data->rx_queue_state[q_id] =3D RTE_ETH_QUEUE_STATE_STOPPED; > > rxq->rx_started =3D false; > > PMD_DRV_LOG_LINE(DEBUG, "Rx queue stopped"); > > diff --git a/drivers/net/e1000/igb_rxtx.c > b/drivers/net/e1000/igb_rxtx.c > > index d61eaad2de..4276bb6d31 100644 > > --- a/drivers/net/e1000/igb_rxtx.c > > +++ b/drivers/net/e1000/igb_rxtx.c > > @@ -1868,6 +1868,7 @@ igb_dev_clear_queues(struct rte_eth_dev *dev) > > struct igb_rx_queue *rxq; > > > > for (i =3D 0; i < dev->data->nb_tx_queues; i++) { > > + __rte_assume(i < RTE_MAX_QUEUES_PER_PORT); > > txq =3D dev->data->tx_queues[i]; > > if (txq !=3D NULL) { > > igb_tx_queue_release_mbufs(txq); > > @@ -1877,6 +1878,7 @@ igb_dev_clear_queues(struct rte_eth_dev *dev) > > } > > > > for (i =3D 0; i < dev->data->nb_rx_queues; i++) { > > + __rte_assume(i < RTE_MAX_QUEUES_PER_PORT); > > rxq =3D dev->data->rx_queues[i]; > > if (rxq !=3D NULL) { > > igb_rx_queue_release_mbufs(rxq); >=20 > For e1000, this is fine. >=20 > Acked-by: Bruce Richardson >=20 > BTW: is this the only/best way to put in the assumption? If it were = me, > I'd > look to put before the loop the underlying assumption that > (dev->data->nb_XX_queues < RTE_MAX_QUEUES_PER_PORT), rather than > putting > the assumption on "i". I would also prefer putting it outside the loop, but it doesn't work in = cases where the variable is potentially modified inside the loop. And = here's the problem with that: Passing it as a parameter to a logging = macro makes the compiler think it is "potentially modified". And thus, I have to put it where it hurts, and decided to do it = consistently. E.g. /drivers/net/mana/tx.c: int mana_start_tx_queues(struct rte_eth_dev *dev) { struct mana_priv *priv =3D dev->data->dev_private; int ret, i; /* start TX queues */ for (i =3D 0; i < priv->num_queues; i++) ## No warning about dev->data->tx_queue_state[i] here: if (dev->data->tx_queue_state[i] =3D=3D RTE_ETH_QUEUE_STATE_STARTED) return -EINVAL; for (i =3D 0; i < priv->num_queues; i++) { struct mana_txq *txq; struct ibv_qp_init_attr qp_attr =3D { 0 }; struct manadv_obj obj =3D {}; struct manadv_qp dv_qp; struct manadv_cq dv_cq; ## Also no warning about dev->data->tx_queues[i] here ("i" not yet = modified): txq =3D dev->data->tx_queues[i]; ## [...] if (!txq->qp) { ## Compiler considers "i" potentially modified here: DRV_LOG(ERR, "Failed to create qp queue index %d", i); ret =3D -errno; goto fail; } ## [...] __rte_assume(i < RTE_MAX_QUEUES_PER_PORT); ## And warns about dev->data->tx_queue_state[i] here (without = __rte_assume): dev->data->tx_queue_state[i] =3D RTE_ETH_QUEUE_STATE_STARTED; } return 0; fail: mana_stop_tx_queues(dev); return ret; }