[PATCH 0/1] net/mlx5: show rx/tx descriptor ring limitations in rte_eth_dev_info

[PATCH 0/1] net/mlx5: show rx/tx descriptor ring limitations in rte_eth_dev_info

[Igor Gutorov]

Hello,

This patch fixes an issue with my ConnectX5 NIC (15b3:1019) showing
65535 as the maximum number of RX descriptors.

So, a disclaimer: I don't know whether other NICs (ConnectX6?) allow
larger values. The value of 8192 was obtained experimentally with
ethtool: ethtool -G *nic* rx 8192, as I do not have the NIC
programmer's manual.

Using anything larger than 8192 resulted in an error. Similarly,
anything in the [8193, 32768) range with the DPDK PMD results in an rx
queue allocation failure.

Igor Gutorov (1):
  net/mlx5: show rx/tx descriptor ring limitations in rte_eth_dev_info

 drivers/net/mlx5/mlx5_defs.h   | 3 +++
 drivers/net/mlx5/mlx5_ethdev.c | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

--
2.45.2

[PATCH 1/1] net/mlx5: show rx/tx descriptor ring limitations in rte_eth_dev_info

[Igor Gutorov]

Currently, rte_eth_dev_info.rx_desc_lim.nb_max shows 65535 as a limit,
which results in a few problems:

* It is an incorrect value
* Allocating an RX queue and passing `rx_desc_lim.nb_max` results in an
  integer overflow and 0 ring size:

```
rte_eth_rx_queue_setup(0, 0, rx_desc_lim.nb_max, 0, NULL, mb_pool);
```

Which overflows ring size and generates the following log:
```
mlx5_net: port 0 increased number of descriptors in Rx queue 0 to the
next power of two (0)
```

This patch fixes these issues.

Signed-off-by: Igor Gutorov <igootorov@gmail.com>
---
 drivers/net/mlx5/mlx5_defs.h   | 3 +++
 drivers/net/mlx5/mlx5_ethdev.c | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index dc5216cb24..df608f0921 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -84,6 +84,9 @@
 #define MLX5_RX_DEFAULT_BURST 64U
 #define MLX5_TX_DEFAULT_BURST 64U
 
+/* Maximum number of descriptors in an RX/TX ring */
+#define MLX5_MAX_RING_DESC 8192
+
 /* Number of packets vectorized Rx can simultaneously process in a loop. */
 #define MLX5_VPMD_DESCS_PER_LOOP      4
 
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index aea799341c..d5be1ff1aa 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -22,6 +22,7 @@
 
 #include <mlx5_malloc.h>
 
+#include "mlx5_defs.h"
 #include "mlx5_rxtx.h"
 #include "mlx5_rx.h"
 #include "mlx5_tx.h"
@@ -345,6 +346,8 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
 	info->flow_type_rss_offloads = ~MLX5_RSS_HF_MASK;
 	mlx5_set_default_params(dev, info);
 	mlx5_set_txlimit_params(dev, info);
+	info->rx_desc_lim.nb_max = MLX5_MAX_RING_DESC;
+	info->tx_desc_lim.nb_max = MLX5_MAX_RING_DESC;
 	if (priv->sh->cdev->config.hca_attr.mem_rq_rmp &&
 	    priv->obj_ops.rxq_obj_new == devx_obj_ops.rxq_obj_new)
 		info->dev_capa |= RTE_ETH_DEV_CAPA_RXQ_SHARE;
@@ -774,7 +777,7 @@ mlx5_hairpin_cap_get(struct rte_eth_dev *dev, struct rte_eth_hairpin_cap *cap)
 	cap->max_nb_queues = UINT16_MAX;
 	cap->max_rx_2_tx = 1;
 	cap->max_tx_2_rx = 1;
-	cap->max_nb_desc = 8192;
+	cap->max_nb_desc = MLX5_MAX_RING_DESC;
 	hca_attr = &priv->sh->cdev->config.hca_attr;
 	cap->rx_cap.locked_device_memory = hca_attr->hairpin_data_buffer_locked;
 	cap->rx_cap.rte_memory = 0;
-- 
2.45.2

RE: [PATCH 1/1] net/mlx5: show rx/tx descriptor ring limitations in rte_eth_dev_info

[Slava Ovsiienko]

Hi, Igor

Thank you for the patch.

1. The absolute max descriptor number supported by ConnectX hardware is 32768.
2. The actual max descriptor number supported by the port (and its related representors)
    reported in log_max_wq_sz in HCA.caps.  This value should be queried and save in mlx5_devx_cmd_query_hca_attr() routine.
3. mlx5_rx_queue_pre_setup() should check requested descriptor number and reject if it exceeds log_max_wq_sz
4. Please, format your patch according to the "fix" template.

With best regards,
Slava

> -----Original Message-----
> From: Igor Gutorov <igootorov@gmail.com>
> Sent: Sunday, June 16, 2024 8:38 PM
> To: Dariusz Sosnowski <dsosnowski@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Ori Kam <orika@nvidia.com>; Suanming Mou
> <suanmingm@nvidia.com>; Matan Azrad <matan@nvidia.com>
> Cc: dev@dpdk.org; Igor Gutorov <igootorov@gmail.com>
> Subject: [PATCH 1/1] net/mlx5: show rx/tx descriptor ring limitations in
> rte_eth_dev_info
> 
> Currently, rte_eth_dev_info.rx_desc_lim.nb_max shows 65535 as a limit, which
> results in a few problems:
> 
> * It is an incorrect value
> * Allocating an RX queue and passing `rx_desc_lim.nb_max` results in an
>   integer overflow and 0 ring size:
> 
> ```
> rte_eth_rx_queue_setup(0, 0, rx_desc_lim.nb_max, 0, NULL, mb_pool); ```
> 
> Which overflows ring size and generates the following log:
> ```
> mlx5_net: port 0 increased number of descriptors in Rx queue 0 to the next
> power of two (0) ```
> 
> This patch fixes these issues.
> 
> Signed-off-by: Igor Gutorov <igootorov@gmail.com>
> ---
>  drivers/net/mlx5/mlx5_defs.h   | 3 +++
>  drivers/net/mlx5/mlx5_ethdev.c | 5 ++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h index
> dc5216cb24..df608f0921 100644
> --- a/drivers/net/mlx5/mlx5_defs.h
> +++ b/drivers/net/mlx5/mlx5_defs.h
> @@ -84,6 +84,9 @@
>  #define MLX5_RX_DEFAULT_BURST 64U
>  #define MLX5_TX_DEFAULT_BURST 64U
> 
> +/* Maximum number of descriptors in an RX/TX ring */ #define
> +MLX5_MAX_RING_DESC 8192
> +
>  /* Number of packets vectorized Rx can simultaneously process in a loop. */
>  #define MLX5_VPMD_DESCS_PER_LOOP      4
> 
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index aea799341c..d5be1ff1aa 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -22,6 +22,7 @@
> 
>  #include <mlx5_malloc.h>
> 
> +#include "mlx5_defs.h"
>  #include "mlx5_rxtx.h"
>  #include "mlx5_rx.h"
>  #include "mlx5_tx.h"
> @@ -345,6 +346,8 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *info)
>  	info->flow_type_rss_offloads = ~MLX5_RSS_HF_MASK;
>  	mlx5_set_default_params(dev, info);
>  	mlx5_set_txlimit_params(dev, info);
> +	info->rx_desc_lim.nb_max = MLX5_MAX_RING_DESC;
> +	info->tx_desc_lim.nb_max = MLX5_MAX_RING_DESC;
>  	if (priv->sh->cdev->config.hca_attr.mem_rq_rmp &&
>  	    priv->obj_ops.rxq_obj_new == devx_obj_ops.rxq_obj_new)
>  		info->dev_capa |= RTE_ETH_DEV_CAPA_RXQ_SHARE; @@ -
> 774,7 +777,7 @@ mlx5_hairpin_cap_get(struct rte_eth_dev *dev, struct
> rte_eth_hairpin_cap *cap)
>  	cap->max_nb_queues = UINT16_MAX;
>  	cap->max_rx_2_tx = 1;
>  	cap->max_tx_2_rx = 1;
> -	cap->max_nb_desc = 8192;
> +	cap->max_nb_desc = MLX5_MAX_RING_DESC;
>  	hca_attr = &priv->sh->cdev->config.hca_attr;
>  	cap->rx_cap.locked_device_memory = hca_attr-
> >hairpin_data_buffer_locked;
>  	cap->rx_cap.rte_memory = 0;
> --
> 2.45.2

[PATCH v2 0/1] net/mlx5: fix incorrect rx/tx descriptor limitations in rte_eth_dev_info

[Igor Gutorov]

Hi, Slava

> 1. The absolute max descriptor number supported by ConnectX hardware is 32768.
> 2. The actual max descriptor number supported by the port (and its related representors)
>     reported in log_max_wq_sz in HCA.caps.  This value should be queried and save in mlx5_devx_cmd_query_hca_attr() routine.
> 3. mlx5_rx_queue_pre_setup() should check requested descriptor number and reject if it exceeds log_max_wq_sz

Thank you for the guidelines! I've also added the same check to mlx5_tx_queue_pre_setup(),
I'm assuming log_max_wq_sz can be used for both RX and TX.

Is an `int` appropriate for `log_max_wq_sz`? Seems like a `uint8_t` is sufficient, but I've left it
an `int` for consistency with the other `log_max_*` values.

Also, I've noticed a similar issue with MTU, it is also reported as 65535 in
`rte_eth_dev_info.max_mtu`. I'd like to send a separate patch to fix that too. What's
the procedure for reading max supported MTU?

> 4. Please, format your patch according to the "fix" template.

I've reworded the commit message a little bit. But I don't see these issues on Bugzilla, I've
stumbled upon them independently. If you'd like the bug reports to be created, let me know.

Sincerely,
Igor

Igor Gutorov (1):
  net/mlx5: fix incorrect rx/tx descriptor limitations in
    rte_eth_dev_info

 drivers/common/mlx5/mlx5_devx_cmds.c | 1 +
 drivers/common/mlx5/mlx5_devx_cmds.h | 1 +
 drivers/net/mlx5/mlx5_ethdev.c       | 4 ++++
 drivers/net/mlx5/mlx5_rxq.c          | 8 ++++++++
 drivers/net/mlx5/mlx5_txq.c          | 8 ++++++++
 5 files changed, 22 insertions(+)

--
2.45.2

[PATCH v2] net/mlx5: fix incorrect rx/tx descriptor limitations in rte_eth_dev_info

[Igor Gutorov]

Currently, `rte_eth_dev_info.rx_desc_lim.nb_max` as well as
`rte_eth_dev_info.tx_desc_lim.nb_max` shows 65535 as the limit,
which results in a few problems:

* It is an incorrect value
* Allocating an RX queue and passing `rx_desc_lim.nb_max` results in an
  integer overflow and 0 ring size:

```
rte_eth_rx_queue_setup(0, 0, rx_desc_lim.nb_max, 0, NULL, mb_pool);
```

Which overflows ring size and generates the following log:
```
mlx5_net: port 0 increased number of descriptors in Rx queue 0 to the
next power of two (0)
```
The same holds for allocating a TX queue.

This patch fixes these issues.

Signed-off-by: Igor Gutorov <igootorov@gmail.com>
---
 drivers/common/mlx5/mlx5_devx_cmds.c | 1 +
 drivers/common/mlx5/mlx5_devx_cmds.h | 1 +
 drivers/net/mlx5/mlx5_ethdev.c       | 4 ++++
 drivers/net/mlx5/mlx5_rxq.c          | 8 ++++++++
 drivers/net/mlx5/mlx5_txq.c          | 8 ++++++++
 5 files changed, 22 insertions(+)

diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c b/drivers/common/mlx5/mlx5_devx_cmds.c
index 9b7ababae7..fe7a53835a 100644
--- a/drivers/common/mlx5/mlx5_devx_cmds.c
+++ b/drivers/common/mlx5/mlx5_devx_cmds.c
@@ -1027,6 +1027,7 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
 	attr->log_max_qp = MLX5_GET(cmd_hca_cap, hcattr, log_max_qp);
 	attr->log_max_cq_sz = MLX5_GET(cmd_hca_cap, hcattr, log_max_cq_sz);
 	attr->log_max_qp_sz = MLX5_GET(cmd_hca_cap, hcattr, log_max_qp_sz);
+	attr->log_max_wq_sz = MLX5_GET(cmd_hca_cap, hcattr, log_max_wq_sz);
 	attr->log_max_mrw_sz = MLX5_GET(cmd_hca_cap, hcattr, log_max_mrw_sz);
 	attr->log_max_pd = MLX5_GET(cmd_hca_cap, hcattr, log_max_pd);
 	attr->log_max_srq = MLX5_GET(cmd_hca_cap, hcattr, log_max_srq);
diff --git a/drivers/common/mlx5/mlx5_devx_cmds.h b/drivers/common/mlx5/mlx5_devx_cmds.h
index c79f8dc48d..abd12ea6e1 100644
--- a/drivers/common/mlx5/mlx5_devx_cmds.h
+++ b/drivers/common/mlx5/mlx5_devx_cmds.h
@@ -267,6 +267,7 @@ struct mlx5_hca_attr {
 	struct mlx5_hca_flow_attr flow;
 	struct mlx5_hca_flex_attr flex;
 	struct mlx5_hca_crypto_mmo_attr crypto_mmo;
+	int log_max_wq_sz;
 	int log_max_qp_sz;
 	int log_max_cq_sz;
 	int log_max_qp;
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index aea799341c..4ef47b3cc7 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -345,6 +345,10 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
 	info->flow_type_rss_offloads = ~MLX5_RSS_HF_MASK;
 	mlx5_set_default_params(dev, info);
 	mlx5_set_txlimit_params(dev, info);
+	info->rx_desc_lim.nb_max =
+		1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz;
+	info->tx_desc_lim.nb_max =
+		1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz;
 	if (priv->sh->cdev->config.hca_attr.mem_rq_rmp &&
 	    priv->obj_ops.rxq_obj_new == devx_obj_ops.rxq_obj_new)
 		info->dev_capa |= RTE_ETH_DEV_CAPA_RXQ_SHARE;
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index f67aaa6178..f66a283983 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -655,6 +655,14 @@ mlx5_rx_queue_pre_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t *desc,
 	struct mlx5_rxq_priv *rxq;
 	bool empty;
 
+	if (*desc > 1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz) {
+		DRV_LOG(ERR,
+			"port %u number of descriptors requested for Rx queue"
+			" %u is more than supported",
+			dev->data->port_id, idx);
+		rte_errno = EINVAL;
+		return -EINVAL;
+	}
 	if (!rte_is_power_of_2(*desc)) {
 		*desc = 1 << log2above(*desc);
 		DRV_LOG(WARNING,
diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c
index da4236f99a..07128c37e7 100644
--- a/drivers/net/mlx5/mlx5_txq.c
+++ b/drivers/net/mlx5/mlx5_txq.c
@@ -332,6 +332,14 @@ mlx5_tx_queue_pre_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t *desc)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
 
+	if (*desc > 1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz) {
+		DRV_LOG(ERR,
+			"port %u number of descriptors requested for Tx queue"
+			" %u is more than supported",
+			dev->data->port_id, idx);
+		rte_errno = EINVAL;
+		return -EINVAL;
+	}
 	if (*desc <= MLX5_TX_COMP_THRESH) {
 		DRV_LOG(WARNING,
 			"port %u number of descriptors requested for Tx queue"
-- 
2.45.2

Re: [PATCH v2] net/mlx5: fix incorrect rx/tx descriptor limitations in rte_eth_dev_info

[Stephen Hemminger]

On Wed, 19 Jun 2024 01:56:42 +0300
Igor Gutorov <igootorov@gmail.com> wrote:

> Currently, `rte_eth_dev_info.rx_desc_lim.nb_max` as well as
> `rte_eth_dev_info.tx_desc_lim.nb_max` shows 65535 as the limit,
> which results in a few problems:
> 
> * It is an incorrect value
> * Allocating an RX queue and passing `rx_desc_lim.nb_max` results in an
>   integer overflow and 0 ring size:
> 
> ```
> rte_eth_rx_queue_setup(0, 0, rx_desc_lim.nb_max, 0, NULL, mb_pool);
> ```
> 
> Which overflows ring size and generates the following log:
> ```
> mlx5_net: port 0 increased number of descriptors in Rx queue 0 to the
> next power of two (0)
> ```
> The same holds for allocating a TX queue.
> 
> This patch fixes these issues.
> 
> Signed-off-by: Igor Gutorov <igootorov@gmail.com>
> ---
>  drivers/common/mlx5/mlx5_devx_cmds.c | 1 +
>  drivers/common/mlx5/mlx5_devx_cmds.h | 1 +
>  drivers/net/mlx5/mlx5_ethdev.c       | 4 ++++
>  drivers/net/mlx5/mlx5_rxq.c          | 8 ++++++++
>  drivers/net/mlx5/mlx5_txq.c          | 8 ++++++++
>  5 files changed, 22 insertions(+)
> 
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c b/drivers/common/mlx5/mlx5_devx_cmds.c
> index 9b7ababae7..fe7a53835a 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.c
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.c
> @@ -1027,6 +1027,7 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
>  	attr->log_max_qp = MLX5_GET(cmd_hca_cap, hcattr, log_max_qp);
>  	attr->log_max_cq_sz = MLX5_GET(cmd_hca_cap, hcattr, log_max_cq_sz);
>  	attr->log_max_qp_sz = MLX5_GET(cmd_hca_cap, hcattr, log_max_qp_sz);
> +	attr->log_max_wq_sz = MLX5_GET(cmd_hca_cap, hcattr, log_max_wq_sz);
>  	attr->log_max_mrw_sz = MLX5_GET(cmd_hca_cap, hcattr, log_max_mrw_sz);
>  	attr->log_max_pd = MLX5_GET(cmd_hca_cap, hcattr, log_max_pd);
>  	attr->log_max_srq = MLX5_GET(cmd_hca_cap, hcattr, log_max_srq);
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.h b/drivers/common/mlx5/mlx5_devx_cmds.h
> index c79f8dc48d..abd12ea6e1 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.h
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.h
> @@ -267,6 +267,7 @@ struct mlx5_hca_attr {
>  	struct mlx5_hca_flow_attr flow;
>  	struct mlx5_hca_flex_attr flex;
>  	struct mlx5_hca_crypto_mmo_attr crypto_mmo;
> +	int log_max_wq_sz;
>  	int log_max_qp_sz;
>  	int log_max_cq_sz;
>  	int log_max_qp;
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index aea799341c..4ef47b3cc7 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -345,6 +345,10 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
>  	info->flow_type_rss_offloads = ~MLX5_RSS_HF_MASK;
>  	mlx5_set_default_params(dev, info);
>  	mlx5_set_txlimit_params(dev, info);
> +	info->rx_desc_lim.nb_max =
> +		1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz;
> +	info->tx_desc_lim.nb_max =
> +		1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz;
>  	if (priv->sh->cdev->config.hca_attr.mem_rq_rmp &&
>  	    priv->obj_ops.rxq_obj_new == devx_obj_ops.rxq_obj_new)
>  		info->dev_capa |= RTE_ETH_DEV_CAPA_RXQ_SHARE;
> diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
> index f67aaa6178..f66a283983 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -655,6 +655,14 @@ mlx5_rx_queue_pre_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t *desc,
>  	struct mlx5_rxq_priv *rxq;
>  	bool empty;
>  
> +	if (*desc > 1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz) {
> +		DRV_LOG(ERR,
> +			"port %u number of descriptors requested for Rx queue"
> +			" %u is more than supported",
> +			dev->data->port_id, idx);
> +		rte_errno = EINVAL;
> +		return -EINVAL;
> +	}
>  	if (!rte_is_power_of_2(*desc)) {
>  		*desc = 1 << log2above(*desc);
>  		DRV_LOG(WARNING,
> diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c
> index da4236f99a..07128c37e7 100644
> --- a/drivers/net/mlx5/mlx5_txq.c
> +++ b/drivers/net/mlx5/mlx5_txq.c
> @@ -332,6 +332,14 @@ mlx5_tx_queue_pre_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t *desc)
>  {
>  	struct mlx5_priv *priv = dev->data->dev_private;
>  
> +	if (*desc > 1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz) {
> +		DRV_LOG(ERR,
> +			"port %u number of descriptors requested for Tx queue"
> +			" %u is more than supported",

Would help to print out value, something like:
			"Port %u Tx descriptors %u exceeds limit %u"

> +			dev->data->port_id, idx);
> +		rte_errno = EINVAL;
> +		return -EINVAL;
> +	}
>  	if (*desc <= MLX5_TX_COMP_THRESH) {
>  		DRV_LOG(WARNING,
>  			"port %u number of descriptors requested for Tx queue"

It is preferable not to break error messages across source lines.
Often, long messages are not that helpful anyway.

The values for log_max_XXX should really be unsigned and ideally a type that corresponds
to a width (like uint32).

RE: [PATCH v2 0/1] net/mlx5: fix incorrect rx/tx descriptor limitations in rte_eth_dev_info

[Slava Ovsiienko]

Hi, Igor

Thank you for the v2. The patch looks good to me,  please see my further comments below.

> > 1. The absolute max descriptor number supported by ConnectX hardware is
> 32768.
> > 2. The actual max descriptor number supported by the port (and its related
> representors)
> >     reported in log_max_wq_sz in HCA.caps.  This value should be queried and
> save in mlx5_devx_cmd_query_hca_attr() routine.
> > 3. mlx5_rx_queue_pre_setup() should check requested descriptor number
> > and reject if it exceeds log_max_wq_sz
> 
> Thank you for the guidelines! I've also added the same check to
> mlx5_tx_queue_pre_setup(), I'm assuming log_max_wq_sz can be used for
> both RX and TX.
> 
> Is an `int` appropriate for `log_max_wq_sz`? Seems like a `uint8_t` is sufficient,
> but I've left it an `int` for consistency with the other `log_max_*` values.

Right, uint8_t looks to be enough. No objection to optimize others to uint8_t.

> Also, I've noticed a similar issue with MTU, it is also reported as 65535 in
> `rte_eth_dev_info.max_mtu`. I'd like to send a separate patch to fix that too.
> What's the procedure for reading max supported MTU?

MTU is not reported directly by HCA. It is per port settings and can be read from
PMTU - Port MTU Register.  ACCESS_REGISTER command should be used.
Please, see:
 https://network.nvidia.com/files/doc-2020/ethernet-adapters-programming-manual.pdf

And thorough  testing of accessing this register is needed - over physical port,
over the representors, over the VFs and SFs. Rollback to 0xFFFF should be implemented,
if register can't be accessed.

Also, this reported max MTU might be not supported for SFs/VFs, where MTU is defined
by hypervisor settings.

> 
> > 4. Please, format your patch according to the "fix" template.
> 
> I've reworded the commit message a little bit. But I don't see these issues on
> Bugzilla, I've stumbled upon them independently. If you'd like the bug reports to
> be created, let me know.

I meant this: https://doc.dpdk.org/guides/contributing/patches.html
Please see chapter "8.7. Commit Messages: Body" about "Fixes" and "Cc: stable@dpdk.org".

Also, please run checking script: /devtools/check-git-log.sh' -1 to verify commit message compliance.

With best regards,
Slava

[PATCH v3 0/2] net/mlx5: fix reported Rx/Tx desc limits

[Igor Gutorov]

Hi, Slava

> > Is an `int` appropriate for `log_max_wq_sz`? Seems like a `uint8_t`
> > is sufficient, but I've left it an `int` for consistency with the
> > other `log_max_*` values.
> 
> Right, uint8_t looks to be enough. No objection to optimize others to
> uint8_t.

Changed log_max_wq_sz to uint8_t in the main patch. The others are
changed in a separate patch. Let me know if it'd be preferrable to
squash the patches.

> > 
> > > 4. Please, format your patch according to the "fix" template.
> > 
> > I've reworded the commit message a little bit. But I don't see these
> > issues on Bugzilla, I've stumbled upon them independently. If you'd
> > like the bug reports to be created, let me know.
> 
> I meant this: https://doc.dpdk.org/guides/contributing/patches.html
> Please see chapter "8.7. Commit Messages: Body" about "Fixes" and "Cc:
> stable@dpdk.org".

It is a bit difficult for me to reference a commit for the "Fixes",
since it's a bit hard to call this a regression specifically. I set this
tag to the commit that first introduced configuring the device. Is that
appropriate?

> 
> Also, please run checking script: /devtools/check-git-log.sh' -1 to
> verify commit message compliance.

Thanks! No warnings now, except for "Wrong headline prefix" for the
first patch because it modifies both common/mlx5 and net/mlx5. I can
split the patch into two if needed.


v3:
* Added uint8_t optimization
* Fixed commit messages

v2:
* Patch reworked to query HCA attributes

Igor Gutorov (2):
  net/mlx5: fix reported Rx/Tx desc limits
  common/mlx5: reduce HCA attribute type sizes

 drivers/common/mlx5/mlx5_devx_cmds.c | 1 +
 drivers/common/mlx5/mlx5_devx_cmds.h | 9 +++++----
 drivers/net/mlx5/mlx5_ethdev.c       | 4 ++++
 drivers/net/mlx5/mlx5_rxq.c          | 8 ++++++++
 drivers/net/mlx5/mlx5_txq.c          | 8 ++++++++
 5 files changed, 26 insertions(+), 4 deletions(-)

-- 
2.45.2

[PATCH v3 1/2] net/mlx5: fix reported Rx/Tx desc limits

[Igor Gutorov]

Currently, `rte_eth_dev_info.rx_desc_lim.nb_max` as well as
`rte_eth_dev_info.tx_desc_lim.nb_max` shows 65535 as the limit,
which results in a few problems:

* It is not the actual Rx/Tx queue limit
* Allocating an Rx queue and passing `rx_desc_lim.nb_max` results in an
  integer overflow and 0 ring size:

```
rte_eth_rx_queue_setup(0, 0, rx_desc_lim.nb_max, 0, NULL, mb_pool);
```

Which overflows ring size and generates the following log:
```
mlx5_net: port 0 increased number of descriptors in Rx queue 0 to the
next power of two (0)
```
The same holds for allocating a Tx queue.

Fixes: e60fbd5b24fc ("mlx5: add device configure/start/stop")
Cc: stable@dpdk.org

Signed-off-by: Igor Gutorov <igootorov@gmail.com>
---
 drivers/common/mlx5/mlx5_devx_cmds.c | 1 +
 drivers/common/mlx5/mlx5_devx_cmds.h | 1 +
 drivers/net/mlx5/mlx5_ethdev.c       | 4 ++++
 drivers/net/mlx5/mlx5_rxq.c          | 8 ++++++++
 drivers/net/mlx5/mlx5_txq.c          | 8 ++++++++
 5 files changed, 22 insertions(+)

diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c b/drivers/common/mlx5/mlx5_devx_cmds.c
index 9710dcedd3..a75f011750 100644
--- a/drivers/common/mlx5/mlx5_devx_cmds.c
+++ b/drivers/common/mlx5/mlx5_devx_cmds.c
@@ -1027,6 +1027,7 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
 	attr->log_max_qp = MLX5_GET(cmd_hca_cap, hcattr, log_max_qp);
 	attr->log_max_cq_sz = MLX5_GET(cmd_hca_cap, hcattr, log_max_cq_sz);
 	attr->log_max_qp_sz = MLX5_GET(cmd_hca_cap, hcattr, log_max_qp_sz);
+	attr->log_max_wq_sz = MLX5_GET(cmd_hca_cap, hcattr, log_max_wq_sz);
 	attr->log_max_mrw_sz = MLX5_GET(cmd_hca_cap, hcattr, log_max_mrw_sz);
 	attr->log_max_pd = MLX5_GET(cmd_hca_cap, hcattr, log_max_pd);
 	attr->log_max_srq = MLX5_GET(cmd_hca_cap, hcattr, log_max_srq);
diff --git a/drivers/common/mlx5/mlx5_devx_cmds.h b/drivers/common/mlx5/mlx5_devx_cmds.h
index 6cf7999c46..2ad9e5414f 100644
--- a/drivers/common/mlx5/mlx5_devx_cmds.h
+++ b/drivers/common/mlx5/mlx5_devx_cmds.h
@@ -267,6 +267,7 @@ struct mlx5_hca_attr {
 	struct mlx5_hca_flow_attr flow;
 	struct mlx5_hca_flex_attr flex;
 	struct mlx5_hca_crypto_mmo_attr crypto_mmo;
+	uint8_t log_max_wq_sz;
 	int log_max_qp_sz;
 	int log_max_cq_sz;
 	int log_max_qp;
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 6a678d6dcc..cac55f7a72 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -359,6 +359,10 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
 	info->flow_type_rss_offloads = ~MLX5_RSS_HF_MASK;
 	mlx5_set_default_params(dev, info);
 	mlx5_set_txlimit_params(dev, info);
+	info->rx_desc_lim.nb_max =
+		1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz;
+	info->tx_desc_lim.nb_max =
+		1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz;
 	if (priv->sh->cdev->config.hca_attr.mem_rq_rmp &&
 	    priv->obj_ops.rxq_obj_new == devx_obj_ops.rxq_obj_new)
 		info->dev_capa |= RTE_ETH_DEV_CAPA_RXQ_SHARE;
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index f13fc3b353..7e171039eb 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -655,6 +655,14 @@ mlx5_rx_queue_pre_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t *desc,
 	struct mlx5_rxq_priv *rxq;
 	bool empty;
 
+	if (*desc > 1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz) {
+		DRV_LOG(ERR,
+			"port %u number of descriptors requested for Rx queue"
+			" %u is more than supported",
+			dev->data->port_id, idx);
+		rte_errno = EINVAL;
+		return -EINVAL;
+	}
 	if (!rte_is_power_of_2(*desc)) {
 		*desc = 1 << log2above(*desc);
 		DRV_LOG(WARNING,
diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c
index f05534e168..3e93517323 100644
--- a/drivers/net/mlx5/mlx5_txq.c
+++ b/drivers/net/mlx5/mlx5_txq.c
@@ -333,6 +333,14 @@ mlx5_tx_queue_pre_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t *desc)
 {
 	struct mlx5_priv *priv = dev->data->dev_private;
 
+	if (*desc > 1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz) {
+		DRV_LOG(ERR,
+			"port %u number of descriptors requested for Tx queue"
+			" %u is more than supported",
+			dev->data->port_id, idx);
+		rte_errno = EINVAL;
+		return -EINVAL;
+	}
 	if (*desc <= MLX5_TX_COMP_THRESH) {
 		DRV_LOG(WARNING,
 			"port %u number of descriptors requested for Tx queue"
-- 
2.45.2

RE: [PATCH v3 1/2] net/mlx5: fix reported Rx/Tx desc limits

[Slava Ovsiienko]

Thank you for the patch.
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

> -----Original Message-----
> From: Igor Gutorov <igootorov@gmail.com>
> Sent: Wednesday, August 7, 2024 11:44 PM
> To: dev@dpdk.org
> Cc: Igor Gutorov <igootorov@gmail.com>; stable@dpdk.org
> Subject: [PATCH v3 1/2] net/mlx5: fix reported Rx/Tx desc limits
> 
> Currently, `rte_eth_dev_info.rx_desc_lim.nb_max` as well as
> `rte_eth_dev_info.tx_desc_lim.nb_max` shows 65535 as the limit, which
> results in a few problems:
> 
> * It is not the actual Rx/Tx queue limit
> * Allocating an Rx queue and passing `rx_desc_lim.nb_max` results in an
>   integer overflow and 0 ring size:
> 
> ```
> rte_eth_rx_queue_setup(0, 0, rx_desc_lim.nb_max, 0, NULL, mb_pool); ```
> 
> Which overflows ring size and generates the following log:
> ```
> mlx5_net: port 0 increased number of descriptors in Rx queue 0 to the next
> power of two (0) ``` The same holds for allocating a Tx queue.
> 
> Fixes: e60fbd5b24fc ("mlx5: add device configure/start/stop")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Igor Gutorov <igootorov@gmail.com>
> ---
>  drivers/common/mlx5/mlx5_devx_cmds.c | 1 +
> drivers/common/mlx5/mlx5_devx_cmds.h | 1 +
>  drivers/net/mlx5/mlx5_ethdev.c       | 4 ++++
>  drivers/net/mlx5/mlx5_rxq.c          | 8 ++++++++
>  drivers/net/mlx5/mlx5_txq.c          | 8 ++++++++
>  5 files changed, 22 insertions(+)
> 
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c
> b/drivers/common/mlx5/mlx5_devx_cmds.c
> index 9710dcedd3..a75f011750 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.c
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.c
> @@ -1027,6 +1027,7 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
>  	attr->log_max_qp = MLX5_GET(cmd_hca_cap, hcattr, log_max_qp);
>  	attr->log_max_cq_sz = MLX5_GET(cmd_hca_cap, hcattr,
> log_max_cq_sz);
>  	attr->log_max_qp_sz = MLX5_GET(cmd_hca_cap, hcattr,
> log_max_qp_sz);
> +	attr->log_max_wq_sz = MLX5_GET(cmd_hca_cap, hcattr,
> log_max_wq_sz);
>  	attr->log_max_mrw_sz = MLX5_GET(cmd_hca_cap, hcattr,
> log_max_mrw_sz);
>  	attr->log_max_pd = MLX5_GET(cmd_hca_cap, hcattr, log_max_pd);
>  	attr->log_max_srq = MLX5_GET(cmd_hca_cap, hcattr, log_max_srq);
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.h
> b/drivers/common/mlx5/mlx5_devx_cmds.h
> index 6cf7999c46..2ad9e5414f 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.h
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.h
> @@ -267,6 +267,7 @@ struct mlx5_hca_attr {
>  	struct mlx5_hca_flow_attr flow;
>  	struct mlx5_hca_flex_attr flex;
>  	struct mlx5_hca_crypto_mmo_attr crypto_mmo;
> +	uint8_t log_max_wq_sz;
>  	int log_max_qp_sz;
>  	int log_max_cq_sz;
>  	int log_max_qp;
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> b/drivers/net/mlx5/mlx5_ethdev.c index 6a678d6dcc..cac55f7a72 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -359,6 +359,10 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev,
> struct rte_eth_dev_info *info)
>  	info->flow_type_rss_offloads = ~MLX5_RSS_HF_MASK;
>  	mlx5_set_default_params(dev, info);
>  	mlx5_set_txlimit_params(dev, info);
> +	info->rx_desc_lim.nb_max =
> +		1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz;
> +	info->tx_desc_lim.nb_max =
> +		1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz;
>  	if (priv->sh->cdev->config.hca_attr.mem_rq_rmp &&
>  	    priv->obj_ops.rxq_obj_new == devx_obj_ops.rxq_obj_new)
>  		info->dev_capa |= RTE_ETH_DEV_CAPA_RXQ_SHARE; diff --
> git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c index
> f13fc3b353..7e171039eb 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -655,6 +655,14 @@ mlx5_rx_queue_pre_setup(struct rte_eth_dev
> *dev, uint16_t idx, uint16_t *desc,
>  	struct mlx5_rxq_priv *rxq;
>  	bool empty;
> 
> +	if (*desc > 1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz) {
> +		DRV_LOG(ERR,
> +			"port %u number of descriptors requested for Rx
> queue"
> +			" %u is more than supported",
> +			dev->data->port_id, idx);
> +		rte_errno = EINVAL;
> +		return -EINVAL;
> +	}
>  	if (!rte_is_power_of_2(*desc)) {
>  		*desc = 1 << log2above(*desc);
>  		DRV_LOG(WARNING,
> diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c
> index f05534e168..3e93517323 100644
> --- a/drivers/net/mlx5/mlx5_txq.c
> +++ b/drivers/net/mlx5/mlx5_txq.c
> @@ -333,6 +333,14 @@ mlx5_tx_queue_pre_setup(struct rte_eth_dev
> *dev, uint16_t idx, uint16_t *desc)  {
>  	struct mlx5_priv *priv = dev->data->dev_private;
> 
> +	if (*desc > 1 << priv->sh->cdev->config.hca_attr.log_max_wq_sz) {
> +		DRV_LOG(ERR,
> +			"port %u number of descriptors requested for Tx
> queue"
> +			" %u is more than supported",
> +			dev->data->port_id, idx);
> +		rte_errno = EINVAL;
> +		return -EINVAL;
> +	}
>  	if (*desc <= MLX5_TX_COMP_THRESH) {
>  		DRV_LOG(WARNING,
>  			"port %u number of descriptors requested for Tx
> queue"
> --
> 2.45.2

[PATCH v3 2/2] common/mlx5: reduce HCA attribute type sizes

[Igor Gutorov]

A number of `log_max_*` fields' types are unnecessarily large, and can
be reduced to `uint8_t`.

Signed-off-by: Igor Gutorov <igootorov@gmail.com>
---
 drivers/common/mlx5/mlx5_devx_cmds.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/common/mlx5/mlx5_devx_cmds.h b/drivers/common/mlx5/mlx5_devx_cmds.h
index 2ad9e5414f..f523bf8529 100644
--- a/drivers/common/mlx5/mlx5_devx_cmds.h
+++ b/drivers/common/mlx5/mlx5_devx_cmds.h
@@ -268,10 +268,10 @@ struct mlx5_hca_attr {
 	struct mlx5_hca_flex_attr flex;
 	struct mlx5_hca_crypto_mmo_attr crypto_mmo;
 	uint8_t log_max_wq_sz;
-	int log_max_qp_sz;
-	int log_max_cq_sz;
-	int log_max_qp;
-	int log_max_cq;
+	uint8_t log_max_qp_sz;
+	uint8_t log_max_cq_sz;
+	uint8_t log_max_qp;
+	uint8_t log_max_cq;
 	uint32_t log_max_pd;
 	uint32_t log_max_mrw_sz;
 	uint32_t log_max_srq;
-- 
2.45.2

RE: [PATCH v3 2/2] common/mlx5: reduce HCA attribute type sizes

[Slava Ovsiienko]

Thank you for the patch.
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

> -----Original Message-----
> From: Igor Gutorov <igootorov@gmail.com>
> Sent: Wednesday, August 7, 2024 11:44 PM
> To: dev@dpdk.org
> Cc: Igor Gutorov <igootorov@gmail.com>
> Subject: [PATCH v3 2/2] common/mlx5: reduce HCA attribute type sizes
> 
> A number of `log_max_*` fields' types are unnecessarily large, and can be
> reduced to `uint8_t`.
> 
> Signed-off-by: Igor Gutorov <igootorov@gmail.com>
> ---
>  drivers/common/mlx5/mlx5_devx_cmds.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.h
> b/drivers/common/mlx5/mlx5_devx_cmds.h
> index 2ad9e5414f..f523bf8529 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.h
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.h
> @@ -268,10 +268,10 @@ struct mlx5_hca_attr {
>  	struct mlx5_hca_flex_attr flex;
>  	struct mlx5_hca_crypto_mmo_attr crypto_mmo;
>  	uint8_t log_max_wq_sz;
> -	int log_max_qp_sz;
> -	int log_max_cq_sz;
> -	int log_max_qp;
> -	int log_max_cq;
> +	uint8_t log_max_qp_sz;
> +	uint8_t log_max_cq_sz;
> +	uint8_t log_max_qp;
> +	uint8_t log_max_cq;
>  	uint32_t log_max_pd;
>  	uint32_t log_max_mrw_sz;
>  	uint32_t log_max_srq;
> --
> 2.45.2

Re: [PATCH v3 0/2] net/mlx5: fix reported Rx/Tx desc limits

[Igor Gutorov]

Hi,

Sorry, I used the wrong --to and --cc switches.
Adding Slava just in case.

Sincerely,
Igor.

On Wed, Aug 7, 2024 at 11:44 PM Igor Gutorov <igootorov@gmail.com> wrote:
>
> Hi, Slava
>
> > > Is an `int` appropriate for `log_max_wq_sz`? Seems like a `uint8_t`
> > > is sufficient, but I've left it an `int` for consistency with the
> > > other `log_max_*` values.
> >
> > Right, uint8_t looks to be enough. No objection to optimize others to
> > uint8_t.
>
> Changed log_max_wq_sz to uint8_t in the main patch. The others are
> changed in a separate patch. Let me know if it'd be preferrable to
> squash the patches.
>
> > >
> > > > 4. Please, format your patch according to the "fix" template.
> > >
> > > I've reworded the commit message a little bit. But I don't see these
> > > issues on Bugzilla, I've stumbled upon them independently. If you'd
> > > like the bug reports to be created, let me know.
> >
> > I meant this: https://doc.dpdk.org/guides/contributing/patches.html
> > Please see chapter "8.7. Commit Messages: Body" about "Fixes" and "Cc:
> > stable@dpdk.org".
>
> It is a bit difficult for me to reference a commit for the "Fixes",
> since it's a bit hard to call this a regression specifically. I set this
> tag to the commit that first introduced configuring the device. Is that
> appropriate?
>
> >
> > Also, please run checking script: /devtools/check-git-log.sh' -1 to
> > verify commit message compliance.
>
> Thanks! No warnings now, except for "Wrong headline prefix" for the
> first patch because it modifies both common/mlx5 and net/mlx5. I can
> split the patch into two if needed.
>
>
> v3:
> * Added uint8_t optimization
> * Fixed commit messages
>
> v2:
> * Patch reworked to query HCA attributes
>
> Igor Gutorov (2):
>   net/mlx5: fix reported Rx/Tx desc limits
>   common/mlx5: reduce HCA attribute type sizes
>
>  drivers/common/mlx5/mlx5_devx_cmds.c | 1 +
>  drivers/common/mlx5/mlx5_devx_cmds.h | 9 +++++----
>  drivers/net/mlx5/mlx5_ethdev.c       | 4 ++++
>  drivers/net/mlx5/mlx5_rxq.c          | 8 ++++++++
>  drivers/net/mlx5/mlx5_txq.c          | 8 ++++++++
>  5 files changed, 26 insertions(+), 4 deletions(-)
>
> --
> 2.45.2
>

Re: [PATCH v3 0/2] net/mlx5: fix reported Rx/Tx desc limits

[Igor Gutorov]

Hi,

On Wed, Aug 7, 2024 at 11:58 PM Igor Gutorov <igootorov@gmail.com> wrote:
>
> Hi,
>
> Sorry, I used the wrong --to and --cc switches.
> Adding Slava just in case.
>
> Sincerely,
> Igor.
>
> On Wed, Aug 7, 2024 at 11:44 PM Igor Gutorov <igootorov@gmail.com> wrote:
> >
> > Hi, Slava
> >
> > > > Is an `int` appropriate for `log_max_wq_sz`? Seems like a `uint8_t`
> > > > is sufficient, but I've left it an `int` for consistency with the
> > > > other `log_max_*` values.
> > >
> > > Right, uint8_t looks to be enough. No objection to optimize others to
> > > uint8_t.
> >
> > Changed log_max_wq_sz to uint8_t in the main patch. The others are
> > changed in a separate patch. Let me know if it'd be preferrable to
> > squash the patches.
> >
> > > >
> > > > > 4. Please, format your patch according to the "fix" template.
> > > >
> > > > I've reworded the commit message a little bit. But I don't see these
> > > > issues on Bugzilla, I've stumbled upon them independently. If you'd
> > > > like the bug reports to be created, let me know.
> > >
> > > I meant this: https://doc.dpdk.org/guides/contributing/patches.html
> > > Please see chapter "8.7. Commit Messages: Body" about "Fixes" and "Cc:
> > > stable@dpdk.org".
> >
> > It is a bit difficult for me to reference a commit for the "Fixes",
> > since it's a bit hard to call this a regression specifically. I set this
> > tag to the commit that first introduced configuring the device. Is that
> > appropriate?
> >
> > >
> > > Also, please run checking script: /devtools/check-git-log.sh' -1 to
> > > verify commit message compliance.
> >
> > Thanks! No warnings now, except for "Wrong headline prefix" for the
> > first patch because it modifies both common/mlx5 and net/mlx5. I can
> > split the patch into two if needed.
> >
> >
> > v3:
> > * Added uint8_t optimization
> > * Fixed commit messages
> >
> > v2:
> > * Patch reworked to query HCA attributes
> >
> > Igor Gutorov (2):
> >   net/mlx5: fix reported Rx/Tx desc limits
> >   common/mlx5: reduce HCA attribute type sizes
> >
> >  drivers/common/mlx5/mlx5_devx_cmds.c | 1 +
> >  drivers/common/mlx5/mlx5_devx_cmds.h | 9 +++++----
> >  drivers/net/mlx5/mlx5_ethdev.c       | 4 ++++
> >  drivers/net/mlx5/mlx5_rxq.c          | 8 ++++++++
> >  drivers/net/mlx5/mlx5_txq.c          | 8 ++++++++
> >  5 files changed, 26 insertions(+), 4 deletions(-)
> >
> > --
> > 2.45.2
> >

Review ping :)
Thanks!

Sincerely,
Igor.

Re: [PATCH v3 0/2] net/mlx5: fix reported Rx/Tx desc limits

[Raslan Darawsheh]

Hi,

From: Igor Gutorov <igootorov@gmail.com>
Sent: Wednesday, August 7, 2024 11:44 PM
To: dev@dpdk.org
Cc: Igor Gutorov
Subject: [PATCH v3 0/2] net/mlx5: fix reported Rx/Tx desc limits

Hi, Slava

> > Is an `int` appropriate for `log_max_wq_sz`? Seems like a `uint8_t`
> > is sufficient, but I've left it an `int` for consistency with the
> > other `log_max_*` values.
>
> Right, uint8_t looks to be enough. No objection to optimize others to
> uint8_t.

Changed log_max_wq_sz to uint8_t in the main patch. The others are
changed in a separate patch. Let me know if it'd be preferrable to
squash the patches.

> >
> > > 4. Please, format your patch according to the "fix" template.
> >
> > I've reworded the commit message a little bit. But I don't see these
> > issues on Bugzilla, I've stumbled upon them independently. If you'd
> > like the bug reports to be created, let me know.
>
> I meant this: https://doc.dpdk.org/guides/contributing/patches.html
> Please see chapter "8.7. Commit Messages: Body" about "Fixes" and "Cc:
> stable@dpdk.org".

It is a bit difficult for me to reference a commit for the "Fixes",
since it's a bit hard to call this a regression specifically. I set this
tag to the commit that first introduced configuring the device. Is that
appropriate?

>
> Also, please run checking script: /devtools/check-git-log.sh' -1 to
> verify commit message compliance.

Thanks! No warnings now, except for "Wrong headline prefix" for the
first patch because it modifies both common/mlx5 and net/mlx5. I can
split the patch into two if needed.


v3:
* Added uint8_t optimization
* Fixed commit messages

v2:
* Patch reworked to query HCA attributes

Igor Gutorov (2):
  net/mlx5: fix reported Rx/Tx desc limits
  common/mlx5: reduce HCA attribute type sizes

 drivers/common/mlx5/mlx5_devx_cmds.c | 1 +
 drivers/common/mlx5/mlx5_devx_cmds.h | 9 +++++----
 drivers/net/mlx5/mlx5_ethdev.c       | 4 ++++
 drivers/net/mlx5/mlx5_rxq.c          | 8 ++++++++
 drivers/net/mlx5/mlx5_txq.c          | 8 ++++++++
 5 files changed, 26 insertions(+), 4 deletions(-)

--
2.45.2

Series applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh

Re: [PATCH 1/1] net/mlx5: show rx/tx descriptor ring limitations in rte_eth_dev_info

[Stephen Hemminger]

On Mon, 17 Jun 2024 07:18:58 +0000
Slava Ovsiienko <viacheslavo@nvidia.com> wrote:

> Hi, Igor
> 
> Thank you for the patch.
> 
> 1. The absolute max descriptor number supported by ConnectX hardware is 32768.
> 2. The actual max descriptor number supported by the port (and its related representors)
>     reported in log_max_wq_sz in HCA.caps.  This value should be queried and save in mlx5_devx_cmd_query_hca_attr() routine.
> 3. mlx5_rx_queue_pre_setup() should check requested descriptor number and reject if it exceeds log_max_wq_sz
> 4. Please, format your patch according to the "fix" template.
> 
> With best regards,
> Slava


Marking this patch as Changes Requested.
Yes, this comment expands the scope of the original patch; but if this part of the mlx5
is going to change, might as well get it right.

Re: [PATCH 1/1] net/mlx5: show rx/tx descriptor ring limitations in rte_eth_dev_info

[Igor Gutorov]

Hi Stephen,

On Thu, Oct 10, 2024 at 3:18 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Mon, 17 Jun 2024 07:18:58 +0000
> Slava Ovsiienko <viacheslavo@nvidia.com> wrote:
>
> > Hi, Igor
> >
> > Thank you for the patch.
> >
> > 1. The absolute max descriptor number supported by ConnectX hardware is 32768.
> > 2. The actual max descriptor number supported by the port (and its related representors)
> >     reported in log_max_wq_sz in HCA.caps.  This value should be queried and save in mlx5_devx_cmd_query_hca_attr() routine.
> > 3. mlx5_rx_queue_pre_setup() should check requested descriptor number and reject if it exceeds log_max_wq_sz
> > 4. Please, format your patch according to the "fix" template.
> >
> > With best regards,
> > Slava
>
>
> Marking this patch as Changes Requested.
> Yes, this comment expands the scope of the original patch; but if this part of the mlx5
> is going to change, might as well get it right.

I'm new to DPDK development, but if it makes sense, I guess this can
be marked as "Canceled" (or something like that).
There's a v2 of this patch [1], and a v3 [2]. You've left some
suggestions on v2, so I'm going to send a v4.

[1]: https://lore.kernel.org/all/20241009171853.77e8dce7@hermes.local/T/#mecef40b9e98711313e4189d42b0d50676e6d9e5e
[2]: https://lore.kernel.org/all/20241009171853.77e8dce7@hermes.local/T/#md042e15b4642df69127b023144948681b3072af5

--
Igor