> 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".
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 = dev->data->dev_private;
int ret, i;
/* start TX queues */
for (i = 0; i < priv->num_queues; i++)
## No warning about dev->data->tx_queue_state[i] here:
if (dev->data->tx_queue_state[i] == RTE_ETH_QUEUE_STATE_STARTED)
return -EINVAL;
for (i = 0; i < priv->num_queues; i++) {
struct mana_txq *txq;
struct ibv_qp_init_attr qp_attr = { 0 };
struct manadv_obj obj = {};
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 = 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 = -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] = RTE_ETH_QUEUE_STATE_STARTED;
}
return 0;
fail:
mana_stop_tx_queues(dev);
return ret;
}
