On Wed, Nov 06, 2024 at 01:19:40PM +0100, Morten Brørup wrote: > > From: Bruce Richardson [mailto:bruce.richard...@intel.com] > > Sent: Wednesday, 6 November 2024 12.52 > > > > On Fri, Oct 25, 2024 at 11:52:23AM +0000, Morten Brørup 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=array-bounds=] > > > 587 | dev->data->rx_queue_state[q_id] = > > 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ørup <m...@smartsharesystems.com> > > > --- > > > 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 = 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] = > > > RTE_ETH_QUEUE_STATE_STARTED; > > > rxq->rx_started = true; > > > @@ -930,6 +931,7 @@ static int bnxt_start_nic(struct bnxt *bp) > > > struct bnxt_tx_queue *txq = 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] = > > > RTE_ETH_QUEUE_STATE_STARTED; > > > txq->tx_started = 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] = RTE_ETH_QUEUE_STATE_STOPPED; > > > rxq->rx_started = 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 = 0; i < dev->data->nb_tx_queues; i++) { > > > + __rte_assume(i < RTE_MAX_QUEUES_PER_PORT); > > > txq = dev->data->tx_queues[i]; > > > if (txq != NULL) { > > > igb_tx_queue_release_mbufs(txq); > > > @@ -1877,6 +1878,7 @@ igb_dev_clear_queues(struct rte_eth_dev *dev) > > > } > > > > > > for (i = 0; i < dev->data->nb_rx_queues; i++) { > > > + __rte_assume(i < RTE_MAX_QUEUES_PER_PORT); > > > rxq = dev->data->rx_queues[i]; > > > if (rxq != NULL) { > > > igb_rx_queue_release_mbufs(rxq); > > > > For e1000, this is fine. > > > > Acked-by: Bruce Richardson <bruce.richard...@intel.com> > > > > 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". >
That's pretty unfortunate. Thanks for explaining. My ack stands. /Bruce
