RE: [PATCH 2/2] drivers/net: support single queue per port

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Bruce Richardson [mailto:bruce.richardson@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 <mb@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.richardson@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;
}