[PATCH 0/2] ethdev: support single queue per port

[PATCH 0/2] ethdev: support single queue per port

[Morten Brørup]

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];
      |                 ^~~~~~~~~~~~~~

This series fixes those compiler emitted errros as follows:
1. A precondition hint macro is introduced, which can be used to prevent
   the compiler/optimizer from considering scenarios that cannot occur.
2. The 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.

Morten Brørup (2):
  eal: add unreachable and precondition hints
  drivers/net: support single queue per port

 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 +
 lib/eal/include/rte_common.h        | 27 +++++++++++++++++++++++++++
 7 files changed, 43 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH 1/2] eal: add unreachable and precondition hints

[Morten Brørup]

Added two new compiler/optimizer hints:
* The __rte_unreachable hint for use in points in code known never to be
reached.
* The __rte_assume hint for providing information about preconditions the
compiler/optimizer might be unable to figure out by itself.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eal/include/rte_common.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index c79f9ed319..2f143c87e4 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -366,6 +366,16 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
 #define __rte_noreturn __attribute__((noreturn))
 #endif
 
+/**
+ * Hint point in program never reached
+ */
+#if defined(RTE_TOOLCHAIN_GCC) || defined(RTE_TOOLCHAIN_CLANG)
+#define __rte_unreachable() __extension__(__builtin_unreachable())
+#else
+/* MSVC or ICC */
+#define __rte_unreachable() __assume(0)
+#endif
+
 /**
  * Issue a warning in case the function's return value is ignored.
  *
@@ -423,6 +433,23 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
 #define __rte_cold __attribute__((cold))
 #endif
 
+/**
+ * Hint precondition
+ *
+ * @warning Depending on the compiler, any code in ``condition`` might be executed.
+ * This currently only occurs with GCC prior to version 13.
+ */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 130000)
+#define __rte_assume(condition) __attribute__((assume(condition)))
+#elif defined(RTE_TOOLCHAIN_GCC)
+#define __rte_assume(condition) do { if (!(condition)) __rte_unreachable(); } while (0)
+#elif defined(RTE_TOOLCHAIN_CLANG)
+#define __rte_assume(condition) __extension__(__builtin_assume(condition))
+#else
+/* MSVC or ICC */
+#define __rte_assume(condition) __assume(condition)
+#endif
+
 /**
  * Disable AddressSanitizer on some code
  */
-- 
2.43.0

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

[Morten Brørup]

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);
diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index 9c013e0419..5321c3385c 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -111,12 +111,14 @@ fs_set_queues_state_start(struct rte_eth_dev *dev)
 	uint16_t i;
 
 	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 && !rxq->info.conf.rx_deferred_start)
 			dev->data->rx_queue_state[i] =
 						RTE_ETH_QUEUE_STATE_STARTED;
 	}
 	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 && !txq->info.conf.tx_deferred_start)
 			dev->data->tx_queue_state[i] =
@@ -176,14 +178,18 @@ fs_set_queues_state_stop(struct rte_eth_dev *dev)
 {
 	uint16_t i;
 
-	for (i = 0; i < dev->data->nb_rx_queues; i++)
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
 		if (dev->data->rx_queues[i] != NULL)
 			dev->data->rx_queue_state[i] =
 						RTE_ETH_QUEUE_STATE_STOPPED;
-	for (i = 0; i < dev->data->nb_tx_queues; i++)
+	}
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
 		if (dev->data->tx_queues[i] != NULL)
 			dev->data->tx_queue_state[i] =
 						RTE_ETH_QUEUE_STATE_STOPPED;
+	}
 }
 
 static int
diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index 5941b966e0..03bbbc435f 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -1309,6 +1309,7 @@ hns3_start_tqps(struct hns3_hw *hw)
 	hns3_enable_all_queues(hw, true);
 
 	for (i = 0; i < hw->data->nb_tx_queues; i++) {
+		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
 		txq = hw->data->tx_queues[i];
 		if (txq->enabled)
 			hw->data->tx_queue_state[i] =
@@ -1316,6 +1317,7 @@ hns3_start_tqps(struct hns3_hw *hw)
 	}
 
 	for (i = 0; i < hw->data->nb_rx_queues; i++) {
+		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
 		rxq = hw->data->rx_queues[i];
 		if (rxq->enabled)
 			hw->data->rx_queue_state[i] =
diff --git a/drivers/net/mana/tx.c b/drivers/net/mana/tx.c
index 272a28bcba..40931ac027 100644
--- a/drivers/net/mana/tx.c
+++ b/drivers/net/mana/tx.c
@@ -154,6 +154,7 @@ mana_start_tx_queues(struct rte_eth_dev *dev)
 			txq->gdma_cq.count, txq->gdma_cq.size,
 			txq->gdma_cq.head);
 
+		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
 		dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
 	}
 
-- 
2.43.0

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

[Ajit Khaparde]

[-- Attachment #1: Type: text/plain, Size: 7489 bytes --]

On Fri, Oct 25, 2024 at 4:52 AM Morten Brørup <mb@smartsharesystems.com> 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>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.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);
> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> index 9c013e0419..5321c3385c 100644
> --- a/drivers/net/failsafe/failsafe_ops.c
> +++ b/drivers/net/failsafe/failsafe_ops.c
> @@ -111,12 +111,14 @@ fs_set_queues_state_start(struct rte_eth_dev *dev)
>         uint16_t i;
>
>         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 && !rxq->info.conf.rx_deferred_start)
>                         dev->data->rx_queue_state[i] =
>                                                 RTE_ETH_QUEUE_STATE_STARTED;
>         }
>         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 && !txq->info.conf.tx_deferred_start)
>                         dev->data->tx_queue_state[i] =
> @@ -176,14 +178,18 @@ fs_set_queues_state_stop(struct rte_eth_dev *dev)
>  {
>         uint16_t i;
>
> -       for (i = 0; i < dev->data->nb_rx_queues; i++)
> +       for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +               __rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
>                 if (dev->data->rx_queues[i] != NULL)
>                         dev->data->rx_queue_state[i] =
>                                                 RTE_ETH_QUEUE_STATE_STOPPED;
> -       for (i = 0; i < dev->data->nb_tx_queues; i++)
> +       }
> +       for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +               __rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
>                 if (dev->data->tx_queues[i] != NULL)
>                         dev->data->tx_queue_state[i] =
>                                                 RTE_ETH_QUEUE_STATE_STOPPED;
> +       }
>  }
>
>  static int
> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
> index 5941b966e0..03bbbc435f 100644
> --- a/drivers/net/hns3/hns3_rxtx.c
> +++ b/drivers/net/hns3/hns3_rxtx.c
> @@ -1309,6 +1309,7 @@ hns3_start_tqps(struct hns3_hw *hw)
>         hns3_enable_all_queues(hw, true);
>
>         for (i = 0; i < hw->data->nb_tx_queues; i++) {
> +               __rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
>                 txq = hw->data->tx_queues[i];
>                 if (txq->enabled)
>                         hw->data->tx_queue_state[i] =
> @@ -1316,6 +1317,7 @@ hns3_start_tqps(struct hns3_hw *hw)
>         }
>
>         for (i = 0; i < hw->data->nb_rx_queues; i++) {
> +               __rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
>                 rxq = hw->data->rx_queues[i];
>                 if (rxq->enabled)
>                         hw->data->rx_queue_state[i] =
> diff --git a/drivers/net/mana/tx.c b/drivers/net/mana/tx.c
> index 272a28bcba..40931ac027 100644
> --- a/drivers/net/mana/tx.c
> +++ b/drivers/net/mana/tx.c
> @@ -154,6 +154,7 @@ mana_start_tx_queues(struct rte_eth_dev *dev)
>                         txq->gdma_cq.count, txq->gdma_cq.size,
>                         txq->gdma_cq.head);
>
> +               __rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
>                 dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
>         }
>
> --
> 2.43.0
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]

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

[Long Li]

> Subject: [PATCH 2/2] drivers/net: support single queue per port
> 
> 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>

For failsafe and mana,
Acked-by: Long Li <longli@microsoft.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);
> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> index 9c013e0419..5321c3385c 100644
> --- a/drivers/net/failsafe/failsafe_ops.c
> +++ b/drivers/net/failsafe/failsafe_ops.c
> @@ -111,12 +111,14 @@ fs_set_queues_state_start(struct rte_eth_dev *dev)
>  	uint16_t i;
> 
>  	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 && !rxq->info.conf.rx_deferred_start)
>  			dev->data->rx_queue_state[i] =
> 
> 	RTE_ETH_QUEUE_STATE_STARTED;
>  	}
>  	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 && !txq->info.conf.tx_deferred_start)
>  			dev->data->tx_queue_state[i] =
> @@ -176,14 +178,18 @@ fs_set_queues_state_stop(struct rte_eth_dev *dev)  {
>  	uint16_t i;
> 
> -	for (i = 0; i < dev->data->nb_rx_queues; i++)
> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
>  		if (dev->data->rx_queues[i] != NULL)
>  			dev->data->rx_queue_state[i] =
> 
> 	RTE_ETH_QUEUE_STATE_STOPPED;
> -	for (i = 0; i < dev->data->nb_tx_queues; i++)
> +	}
> +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
>  		if (dev->data->tx_queues[i] != NULL)
>  			dev->data->tx_queue_state[i] =
> 
> 	RTE_ETH_QUEUE_STATE_STOPPED;
> +	}
>  }
> 
>  static int
> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c index
> 5941b966e0..03bbbc435f 100644
> --- a/drivers/net/hns3/hns3_rxtx.c
> +++ b/drivers/net/hns3/hns3_rxtx.c
> @@ -1309,6 +1309,7 @@ hns3_start_tqps(struct hns3_hw *hw)
>  	hns3_enable_all_queues(hw, true);
> 
>  	for (i = 0; i < hw->data->nb_tx_queues; i++) {
> +		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
>  		txq = hw->data->tx_queues[i];
>  		if (txq->enabled)
>  			hw->data->tx_queue_state[i] =
> @@ -1316,6 +1317,7 @@ hns3_start_tqps(struct hns3_hw *hw)
>  	}
> 
>  	for (i = 0; i < hw->data->nb_rx_queues; i++) {
> +		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
>  		rxq = hw->data->rx_queues[i];
>  		if (rxq->enabled)
>  			hw->data->rx_queue_state[i] =
> diff --git a/drivers/net/mana/tx.c b/drivers/net/mana/tx.c index
> 272a28bcba..40931ac027 100644
> --- a/drivers/net/mana/tx.c
> +++ b/drivers/net/mana/tx.c
> @@ -154,6 +154,7 @@ mana_start_tx_queues(struct rte_eth_dev *dev)
>  			txq->gdma_cq.count, txq->gdma_cq.size,
>  			txq->gdma_cq.head);
> 
> +		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
>  		dev->data->tx_queue_state[i] =
> RTE_ETH_QUEUE_STATE_STARTED;
>  	}
> 
> --
> 2.43.0

RE: [PATCH 1/2] eal: add unreachable and precondition hints

[Morten Brørup]

Ping for review.

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Friday, 25 October 2024 13.52
> 
> Added two new compiler/optimizer hints:
> * The __rte_unreachable hint for use in points in code known never to
> be
> reached.
> * The __rte_assume hint for providing information about preconditions
> the
> compiler/optimizer might be unable to figure out by itself.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  lib/eal/include/rte_common.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/lib/eal/include/rte_common.h
> b/lib/eal/include/rte_common.h
> index c79f9ed319..2f143c87e4 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -366,6 +366,16 @@ static void
> __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>  #define __rte_noreturn __attribute__((noreturn))
>  #endif
> 
> +/**
> + * Hint point in program never reached
> + */
> +#if defined(RTE_TOOLCHAIN_GCC) || defined(RTE_TOOLCHAIN_CLANG)
> +#define __rte_unreachable() __extension__(__builtin_unreachable())
> +#else
> +/* MSVC or ICC */
> +#define __rte_unreachable() __assume(0)
> +#endif
> +
>  /**
>   * Issue a warning in case the function's return value is ignored.
>   *
> @@ -423,6 +433,23 @@ static void
> __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>  #define __rte_cold __attribute__((cold))
>  #endif
> 
> +/**
> + * Hint precondition
> + *
> + * @warning Depending on the compiler, any code in ``condition`` might
> be executed.
> + * This currently only occurs with GCC prior to version 13.
> + */
> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 130000)
> +#define __rte_assume(condition) __attribute__((assume(condition)))
> +#elif defined(RTE_TOOLCHAIN_GCC)
> +#define __rte_assume(condition) do { if (!(condition))
> __rte_unreachable(); } while (0)
> +#elif defined(RTE_TOOLCHAIN_CLANG)
> +#define __rte_assume(condition)
> __extension__(__builtin_assume(condition))
> +#else
> +/* MSVC or ICC */
> +#define __rte_assume(condition) __assume(condition)
> +#endif
> +
>  /**
>   * Disable AddressSanitizer on some code
>   */
> --
> 2.43.0

Re: [PATCH 1/2] eal: add unreachable and precondition hints

[Bruce Richardson]

On Mon, Nov 04, 2024 at 12:40:49PM +0100, Morten Brørup wrote:
> Ping for review.
> 
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > Sent: Friday, 25 October 2024 13.52
> > 
> > Added two new compiler/optimizer hints:
> > * The __rte_unreachable hint for use in points in code known never to
> > be
> > reached.
> > * The __rte_assume hint for providing information about preconditions
> > the
> > compiler/optimizer might be unable to figure out by itself.
> > 
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >  lib/eal/include/rte_common.h | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/lib/eal/include/rte_common.h
> > b/lib/eal/include/rte_common.h
> > index c79f9ed319..2f143c87e4 100644
> > --- a/lib/eal/include/rte_common.h
> > +++ b/lib/eal/include/rte_common.h
> > @@ -366,6 +366,16 @@ static void
> > __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> >  #define __rte_noreturn __attribute__((noreturn))
> >  #endif
> > 
> > +/**
> > + * Hint point in program never reached
> > + */
> > +#if defined(RTE_TOOLCHAIN_GCC) || defined(RTE_TOOLCHAIN_CLANG)
> > +#define __rte_unreachable() __extension__(__builtin_unreachable())
> > +#else
> > +/* MSVC or ICC */
> > +#define __rte_unreachable() __assume(0)
> > +#endif
> > +

Is there already somewhere in the code where we might want to use this? I'm
not sure about just adding macros just in case they might be needed in
future. Having unreachable code seems a bit problematic to me generally
anyway.

> >  /**
> >   * Issue a warning in case the function's return value is ignored.
> >   *
> > @@ -423,6 +433,23 @@ static void
> > __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> >  #define __rte_cold __attribute__((cold))
> >  #endif
> > 
> > +/**
> > + * Hint precondition
> > + *
> > + * @warning Depending on the compiler, any code in ``condition`` might
> > be executed.
> > + * This currently only occurs with GCC prior to version 13.
> > + */
> > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 130000)
> > +#define __rte_assume(condition) __attribute__((assume(condition)))
> > +#elif defined(RTE_TOOLCHAIN_GCC)
> > +#define __rte_assume(condition) do { if (!(condition))
> > __rte_unreachable(); } while (0)
> > +#elif defined(RTE_TOOLCHAIN_CLANG)
> > +#define __rte_assume(condition)
> > __extension__(__builtin_assume(condition))
> > +#else
> > +/* MSVC or ICC */
> > +#define __rte_assume(condition) __assume(condition)
> > +#endif
> > +

This part seems ok, I see it used in the next patch, and also looks rather
useful to have.

/Bruce

RE: [PATCH 1/2] eal: add unreachable and precondition hints

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 4 November 2024 13.19
> 
> On Mon, Nov 04, 2024 at 12:40:49PM +0100, Morten Brørup wrote:
> > Ping for review.
> >
> > > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > Sent: Friday, 25 October 2024 13.52
> > >
> > > Added two new compiler/optimizer hints:
> > > * The __rte_unreachable hint for use in points in code known never
> to
> > > be
> > > reached.
> > > * The __rte_assume hint for providing information about
> preconditions
> > > the
> > > compiler/optimizer might be unable to figure out by itself.
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > ---
> > >  lib/eal/include/rte_common.h | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > >
> > > diff --git a/lib/eal/include/rte_common.h
> > > b/lib/eal/include/rte_common.h
> > > index c79f9ed319..2f143c87e4 100644
> > > --- a/lib/eal/include/rte_common.h
> > > +++ b/lib/eal/include/rte_common.h
> > > @@ -366,6 +366,16 @@ static void
> > > __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> > >  #define __rte_noreturn __attribute__((noreturn))
> > >  #endif
> > >
> > > +/**
> > > + * Hint point in program never reached
> > > + */
> > > +#if defined(RTE_TOOLCHAIN_GCC) || defined(RTE_TOOLCHAIN_CLANG)
> > > +#define __rte_unreachable() __extension__(__builtin_unreachable())
> > > +#else
> > > +/* MSVC or ICC */
> > > +#define __rte_unreachable() __assume(0)
> > > +#endif
> > > +
> 
> Is there already somewhere in the code where we might want to use this?

Yes. It's used in the __rte_assume() macro below for GCC before version 13.

> I'm
> not sure about just adding macros just in case they might be needed in
> future.

IMHO, such hints might be useful for applications, and DPDK could use them in the future.
But if it came to a vote about adding unused/dead code, it would probably not be accepted.
Anyway, this macro is not unused, so no problem here.

> Having unreachable code seems a bit problematic to me generally
> anyway.

Agree.
The correct use of __rte_unreachable() is to provide information to the compiler/optimizer/sanitizer it cannot figure out by itself, and/or as information to the code reviewer.
Using it to prevent compiler warnings from unreachable code would be utterly wrong. I don't know if that is even possible.

> 
> > >  /**
> > >   * Issue a warning in case the function's return value is ignored.
> > >   *
> > > @@ -423,6 +433,23 @@ static void
> > > __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> > >  #define __rte_cold __attribute__((cold))
> > >  #endif
> > >
> > > +/**
> > > + * Hint precondition
> > > + *
> > > + * @warning Depending on the compiler, any code in ``condition``
> might
> > > be executed.
> > > + * This currently only occurs with GCC prior to version 13.
> > > + */
> > > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 130000)
> > > +#define __rte_assume(condition) __attribute__((assume(condition)))
> > > +#elif defined(RTE_TOOLCHAIN_GCC)
> > > +#define __rte_assume(condition) do { if (!(condition))
> > > __rte_unreachable(); } while (0)

Note: I did not come up with this myself; this seems to be the "official" way of doing it with older GCC versions.

> > > +#elif defined(RTE_TOOLCHAIN_CLANG)
> > > +#define __rte_assume(condition)
> > > __extension__(__builtin_assume(condition))
> > > +#else
> > > +/* MSVC or ICC */
> > > +#define __rte_assume(condition) __assume(condition)
> > > +#endif
> > > +
> 
> This part seems ok, I see it used in the next patch, and also looks
> rather
> useful to have.
> 
> /Bruce

Re: [PATCH 1/2] eal: add unreachable and precondition hints

[Bruce Richardson]

On Fri, Oct 25, 2024 at 11:52:22AM +0000, Morten Brørup wrote:
> Added two new compiler/optimizer hints:
> * The __rte_unreachable hint for use in points in code known never to be
> reached.
> * The __rte_assume hint for providing information about preconditions the
> compiler/optimizer might be unable to figure out by itself.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  lib/eal/include/rte_common.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

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

[Bruce Richardson]

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".

/Bruce

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

[Morten Brørup]

> 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;
}

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

[Bruce Richardson]

On Wed, Nov 06, 2024 at 01:19:40PM +0100, Morten Brørup wrote:
> > 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".
> 

That's pretty unfortunate. Thanks for explaining. My ack stands.

/Bruce

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

[Thomas Monjalon]

06/11/2024 13:19, Morten Brørup:
> > 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.
[...]
> > >  	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > +		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
> > >  		rxq = dev->data->rx_queues[i];
[...]
> > 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".

I don't understand this part.
"i" is not a pointer, so how it can be modified?

> And thus, I have to put it where it hurts, and decided to do it consistently.

Why doing something heavier consistently?
I would prefer to catch the problematic cases only with this macro.

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

[Morten Brørup]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, 6 November 2024 17.07
> 
> 06/11/2024 13:19, Morten Brørup:
> > > 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.
> [...]
> > > >  	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > > +		__rte_assume(i < RTE_MAX_QUEUES_PER_PORT);
> > > >  		rxq = dev->data->rx_queues[i];
> [...]
> > > 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".
> 
> I don't understand this part.
> "i" is not a pointer, so how it can be modified?

After a lot of speculation, it was the only reason I could come up with why the compiler would think "i" had been modified. I am not a compiler specialist, and haven't looked deep into GCC, so my conclusion might be wrong.
Also I don't have a deep understanding of GCC's builtin va_args implementation; but I have seen other implementations of the va_args macros using references, so passing "i" they effectively becomes "&i" when using the va_args macros.

Anyway, I have now tested putting the assumption outside the loop (TEST v11), and compilation fails with the same warnings. So the assumptions need to be put very close to the line causing the warning.

Test patch (TEST v8) with assumptions located like this patch, compilation succeeds:
https://patchwork.dpdk.org/project/dpdk/patch/20241025082338.1165360-1-mb@smartsharesystems.com/

Test patch (TEST v11) with assumption outside the loop, compilation fails:
https://patchwork.dpdk.org/project/dpdk/patch/20241107092649.1507375-1-mb@smartsharesystems.com/
http://mails.dpdk.org/archives/test-report/2024-November/821886.html

> 
> > And thus, I have to put it where it hurts, and decided to do it
> consistently.
> 
> Why doing something heavier consistently?
> I would prefer to catch the problematic cases only with this macro.

Agree. I put it only with the problematic cases.
By "consistently" I mean: always just before the line causing the warning, not sometimes outside the loop.

It's always a pain having work around compiler bugs, so I'm trying to keep the ugliness of this workaround at a minimum. :-(