fix MPRQ prepare

From: Michael Baum<[email protected]>

Some devices have a WQE size limit for striding RQ. On some newer devices, this limitation is smaller and information on its size is provided by the firmware.

This patch adds the attribute query from firmware: the minimum required size of WQE in a strided RQ in granularity of Bytes.

Cc [email protected]

Signed-off-by Michael Baum<[email protected]>

Acked-by Matan Azrad<[email protected]>

Quoted 2 lines · On 11/23/2021 6:38 PM, [email protected] wrote:

From: Michael Baum<[email protected]>

Quoted 6 lines

Some devices have a WQE size limit for striding RQ. On some newer devices, this limitation is smaller and information on its size is provided by the firmware.

This patch adds the attribute query from firmware: the minimum required size of WQE in a strided RQ in granularity of Bytes.

? s/strided/strode/

Quoted 4 lines

Cc [email protected]

This is not a fix, why requesting to backport it?
Patch is to use FW provided capability value, which is not used before.

Quoted 3 lines

Signed-off-by Michael Baum<[email protected]>

Acked-by Matan Azrad<[email protected]>

From: Michael Baum<[email protected]>

In the striding RQ management there are two important parameters, the size of the single stride in bytes and the number of strides.

Both the data-path structure and config structure keep the log of the above parameters. However, in their names there is no mention that the value is a log which may be misleading as if the fields represent the values themselves.

This patch updates their names describing the values more accurately.

Cc [email protected]

Signed-off-by Michael Baum<[email protected]>

Acked-by Matan Azrad<[email protected]>

Quoted 10 lines · On 11/23/2021 6:38 PM, [email protected] wrote:

From: Michael Baum<[email protected]>

In the striding RQ management there are two important parameters, the size of the single stride in bytes and the number of strides.

Both the data-path structure and config structure keep the log of the above parameters. However, in their names there is no mention that the value is a log which may be misleading as if the fields represent the values themselves.

Quoted 4 lines

This patch updates their names describing the values more accurately.

Cc [email protected]

Can you please provide fixes tag for it?

In this case commit(s) adding the parameters with bad name.

Quoted 3 lines

Signed-off-by Michael Baum<[email protected]>

Acked-by Matan Azrad<[email protected]>

-----Original Message-----
From: Michael Baum <[email protected]>
Sent: Tuesday, November 23, 2021 8:38 PM
To: [email protected]
Cc: Matan Azrad <[email protected]>; Raslan Darawsheh <[email protected]>; Slava Ovsiienko <[email protected]>; Michael Baum <[email protected]>; [email protected]
Subject: [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs

From: Michael Baum <[email protected]>

In Multy-Packet RQ creation, the user can choose the number of strides and their size in bytes. The user updates it using specific devargs for both of these parameters.
The above two parameters determine the size of the WQE which is actually their product of multiplication.

If the user selects values that are not in the supported range, the PMD changes them to default values. However, apart from the range limitations for each parameter individually there is also a minimum value on their multiplication. When the user selects values that their multiplication are lower than minimum value, no adjustment is made and the creation of the WQE fails.

This patch adds an adjustment in these cases as well. When the user selects values whose multiplication is lower than the minimum, they are replaced with the default values.

Fixes ecb160456aed ("net/mlx5: add device parameter for MPRQ stride

size")
Cc: [email protected]

Signed-off-by Michael Baum <[email protected]>

Acked-by Matan Azrad <[email protected]>

---
 drivers/net/mlx5/linux/mlx5_os.c |  56 +++------
 drivers/net/mlx5/mlx5.h          |   4 +
 drivers/net/mlx5/mlx5_rxq.c      | 209 +++++++++++++++++++++----------
 3 files changed, 159 insertions(+), 110 deletions(-)

drivers/net/mlx5/linux/mlx5_os.c +6 -11

index 70472efc29..3e496d68ea 100644
@@ -881,10 +881,6 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
 	unsigned int mpls_en = 0;
 	unsigned int swp = 0;
 	unsigned int mprq = 0;
-	unsigned int mprq_min_stride_size_n = 0;
-	unsigned int mprq_max_stride_size_n = 0;
-	unsigned int mprq_min_stride_num_n = 0;
-	unsigned int mprq_max_stride_num_n = 0;
 	struct rte_ether_addr mac;
 	char name[RTE_ETH_NAME_MAX_LEN];
 	int own_domain_id = 0;

@@ -1039,15 +1035,17 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
 			mprq_caps.max_single_wqe_log_num_of_strides);
 		DRV_LOG(DEBUG, "\tsupported_qpts: %d",
 			mprq_caps.supported_qpts);
+		DRV_LOG(DEBUG, "\tmin_stride_wqe_log_size: %d",
+			config->mprq.log_min_stride_wqe_size);
 		DRV_LOG(DEBUG, "device supports Multi-Packet RQ");
 		mprq = 1;
-		mprq_min_stride_size_n =
+		config->mprq.log_min_stride_size =
 			mprq_caps.min_single_stride_log_num_of_bytes;
-		mprq_max_stride_size_n =
+		config->mprq.log_max_stride_size =
 			mprq_caps.max_single_stride_log_num_of_bytes;
-		mprq_min_stride_num_n =
+		config->mprq.log_min_stride_num =
 			mprq_caps.min_single_wqe_log_num_of_strides;
-		mprq_max_stride_num_n =
+		config->mprq.log_max_stride_num =
 			mprq_caps.max_single_wqe_log_num_of_strides;
 	}
 #endif

@@ -1548,36 +1546,7 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
 		config->hw_fcs_strip = 0;
 	DRV_LOG(DEBUG, "FCS stripping configuration is %ssupported",
 		(config->hw_fcs_strip ? "" : "not "));
-	if (config->mprq.enabled && mprq) {
-		if (config->mprq.log_stride_num &&
-		    (config->mprq.log_stride_num >

mprq_max_stride_num_n ||

drivers/net/mlx5/linux/mlx5_os.c +0 -1

-		     config->mprq.log_stride_num <

mprq_min_stride_num_n)) {

drivers/net/mlx5/linux/mlx5_os.c +0 -2

-			config->mprq.log_stride_num =
-

RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM,

drivers/net/mlx5/linux/mlx5_os.c +0 -10

-					       mprq_min_stride_num_n),
-				       mprq_max_stride_num_n);
-			DRV_LOG(WARNING,
-				"the number of strides"
-				" for Multi-Packet RQ is out of range,"
-				" setting default value (%u)",
-				1 << config->mprq.log_stride_num);
-		}
-		if (config->mprq.log_stride_size &&
-		    (config->mprq.log_stride_size > mprq_max_stride_size_n

||

drivers/net/mlx5/linux/mlx5_os.c +0 -1

-		     config->mprq.log_stride_size < mprq_min_stride_size_n))

{

drivers/net/mlx5/linux/mlx5_os.c +0 -2

-			config->mprq.log_stride_size =
-

RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE,

drivers/net/mlx5/linux/mlx5_os.c +0 -9

-					      mprq_min_stride_size_n),
-				      mprq_max_stride_size_n);
-			DRV_LOG(WARNING,
-				"the size of a stride"
-				" for Multi-Packet RQ is out of range,"
-				" setting default value (%u)",
-				1 << config->mprq.log_stride_size);
-		}
-		config->mprq.log_min_stride_size =

mprq_min_stride_size_n;

drivers/net/mlx5/linux/mlx5_os.c +0 -1

-		config->mprq.log_max_stride_size =

mprq_max_stride_size_n;

drivers/net/mlx5/linux/mlx5_os.c +5 -2

-	} else if (config->mprq.enabled && !mprq) {
+	if (config->mprq.enabled && !mprq) {
 		DRV_LOG(WARNING, "Multi-Packet RQ isn't supported");
 		config->mprq.enabled = 0;
 	}

@@ -2068,7 +2037,8 @@ mlx5_device_bond_pci_match(const char
 *ibdev_name,  }
 
 static void
-mlx5_os_config_default(struct mlx5_dev_config *config)
+mlx5_os_config_default(struct mlx5_dev_config *config,
+		       struct mlx5_common_dev_config *cconf)
 {
 	memset(config, 0, sizeof(*config));
 	config->mps = MLX5_ARG_UNSET;

@@ -2080,6 +2050,10 @@ mlx5_os_config_default(struct mlx5_dev_config
 *config)
 	config->vf_nl_en = 1;
 	config->mprq.max_memcpy_len =
 MLX5_MPRQ_MEMCPY_DEFAULT_LEN;
 	config->mprq.min_rxqs_num = MLX5_MPRQ_MIN_RXQS;
+	config->mprq.log_min_stride_wqe_size = cconf->devx ?
+					cconf-

Quoted 1 line

hca_attr.log_min_stride_wqe_sz :

+

	MLX5_MPRQ_LOG_MIN_STRIDE_WQE_SIZE;

drivers/net/mlx5/linux/mlx5_os.c +1 -0

+	config->mprq.log_stride_num =

MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM;

drivers/net/mlx5/linux/mlx5_os.c +1 -1

 	config->dv_esw_en = 1;
 	config->dv_flow_en = 1;
 	config->decap_en = 1;

@@ -2496,7 +2470,7 @@ mlx5_os_pci_probe_pf(struct
 mlx5_common_device *cdev,
 		uint32_t restore;
 
 		/* Default configuration. */
-		mlx5_os_config_default(&dev_config);
+		mlx5_os_config_default(&dev_config, &cdev->config);
 		dev_config.vf = dev_config_vf;
 		list[i].eth_dev = mlx5_dev_spawn(cdev->dev, &list[i],

drivers/net/mlx5/linux/mlx5_os.c +1 -1

 						 &dev_config, &eth_da);

@@ -2666,7 +2640,7 @@ mlx5_os_auxiliary_probe(struct
 mlx5_common_device *cdev)
 	if (ret != 0)
 		return ret;
 	/* Set default config data. */
-	mlx5_os_config_default(&config);
+	mlx5_os_config_default(&config, &cdev->config);
 	config.sf = 1;
 	/* Init spawn data. */

drivers/net/mlx5/linux/mlx5_os.c +0 -0

 	spawn.max_port = 1;

drivers/net/mlx5/mlx5.h index renamed from drivers/net/mlx5/mlx5.h +0 -0

4ba90db816..c01fb9566e 100644

drivers/net/mlx5/mlx5.h +0 -0

@@ -279,6 +279,10 @@ struct mlx5_dev_config {
 		unsigned int log_stride_size; /* Log size of a stride. */
 		unsigned int log_min_stride_size; /* Log min size of a

stride.*/
unsigned int log_max_stride_size; /* Log max size of a
stride.*/

drivers/net/mlx5/mlx5.h +1 -0

+		unsigned int log_min_stride_num; /* Log min num of strides.

*/

drivers/net/mlx5/mlx5.h +1 -0

+		unsigned int log_max_stride_num; /* Log max num of

strides. */

drivers/net/mlx5/mlx5.h +2 -0

+		unsigned int log_min_stride_wqe_size;
+		/* Log min WQE size, (size of single stride)*(num of

strides).*/

drivers/net/mlx5/mlx5.h +0 -0

 		unsigned int max_memcpy_len;
 		/* Maximum packet size to memcpy Rx packets. */
 		unsigned int min_rxqs_num;

drivers/net/mlx5/mlx5_rxq.c +36 -0

index e76bfaa000..891ac3d874 100644
@@ -1528,6 +1528,126 @@ mlx5_max_lro_msg_size_adjust(struct
 rte_eth_dev *dev, uint16_t idx,
 		priv->max_lro_msg_size * MLX5_LRO_SEG_CHUNK_SIZE);  }
 
+/**
+ * Prepare both size and number of stride for Multi-Packet RQ.
+ *
+ * @param dev
+ *   Pointer to Ethernet device.
+ * @param idx
+ *   RX queue index.
+ * @param desc
+ *   Number of descriptors to configure in queue.
+ * @param rx_seg_en
+ *   Indicator if Rx segment enables, if so Multi-Packet RQ doesn't enable.
+ * @param min_mbuf_size
+ *   Non scatter min mbuf size, max_rx_pktlen plus overhead.
+ * @param actual_log_stride_num
+ *   Log number of strides to configure for this queue.
+ * @param actual_log_stride_size
+ *   Log stride size to configure for this queue.
+ *
+ * @return
+ *   0 if Multi-Packet RQ is supported, otherwise -1.
+ */
+static int
+mlx5_mprq_prepare(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc,
+		  bool rx_seg_en, uint32_t min_mbuf_size,
+		  uint32_t *actual_log_stride_num,
+		  uint32_t *actual_log_stride_size)
+{
+	struct mlx5_priv *priv = dev->data->dev_private;
+	struct mlx5_dev_config *config = &priv->config;
+	uint32_t log_min_stride_num = config->mprq.log_min_stride_num;
+	uint32_t log_max_stride_num = config->mprq.log_max_stride_num;
+	uint32_t log_def_stride_num =
+
 	RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM,
+					log_min_stride_num),
+				log_max_stride_num);
+	uint32_t log_min_stride_size = config->mprq.log_min_stride_size;

From: Michael Baum<[email protected]>

In Multy-Packet RQ creation, the user can choose the number of strides

and their size in bytes. The user updates it using specific devargs for both of these parameters.
The above two parameters determine the size of the WQE which is actually their product of multiplication.

If the user selects values that are not in the supported range, the PMD changes them to default values. However, apart from the range limitations for each parameter individually there is also a minimum value on their multiplication. When the user selects values that their multiplication are lower than minimum value, no adjustment is made and the creation of the WQE fails.

Quoted 1 line

This patch adds an adjustment in these cases as well. When the user

selects values whose multiplication is lower than the minimum, they are replaced with the default values.

Fixes ecb160456aed ("net/mlx5: add device parameter for MPRQ stride size")

Cc [email protected]

Signed-off-by Michael Baum<[email protected]>

Acked-by Matan Azrad<[email protected]>

---

Quoted 3 lines · On 11/23/2021 6:38 PM, [email protected] wrote:

From: Michael Baum<[email protected]>

In Multy-Packet RQ creation, the user can choose the number of strides

Multi-Packet ?

Quoted 12 lines

and their size in bytes. The user updates it using specific devargs for both of these parameters.
The above two parameters determine the size of the WQE which is actually their product of multiplication.

If the user selects values that are not in the supported range, the PMD changes them to default values. However, apart from the range limitations for each parameter individually there is also a minimum value on their multiplication. When the user selects values that their multiplication are lower than minimum value, no adjustment is made and the creation of the WQE fails.

Quoted 7 lines

Quoted 1 line

This patch adds an adjustment in these cases as well. When the user

selects values whose multiplication is lower than the minimum, they are replaced with the default values.

Fixes ecb160456aed ("net/mlx5: add device parameter for MPRQ stride

size") Cc:[email protected]

Again, not sure if we can backport this patch, this looks a behavior change more than a fix.

Previously if the user provided values ends up being invalid, PMD seems returning error.
With this patch, instead of returning error PMD prefers to use default values and doesn't return error.

I am not sure if it is correct thing to ignore (adjust) user provided values, but that can be up to the PMD as long as the behavior is documented.

But I think it is wrong to backport the behavior change.

Quoted 3 lines

Signed-off-by Michael Baum<[email protected]>

Acked-by Matan Azrad<[email protected]>

---

Quoted 7 lines · On 12/07/2021 3:41 PM, [email protected] wrote:

Quoted 3 lines · On 11/23/2021 6:38 PM, [email protected] wrote:

From: Michael Baum<[email protected]>

In Multy-Packet RQ creation, the user can choose the number of strides

Multi-Packet ?

Yes, you're right. It should have been Multi-Packet, thank you for that.

Quoted 20 lines

Quoted 12 lines

and their size in bytes. The user updates it using specific devargs for both of these parameters.
The above two parameters determine the size of the WQE which is actually their product of multiplication.

If the user selects values that are not in the supported range, the PMD changes them to default values. However, apart from the range limitations for each parameter individually there is also a minimum value on their multiplication. When the user selects values that their multiplication are lower than minimum value, no adjustment is made and the creation of the WQE fails.

Quoted 7 lines

Quoted 1 line

This patch adds an adjustment in these cases as well. When the user

selects values whose multiplication is lower than the minimum, they are replaced with the default values.

Fixes ecb160456aed ("net/mlx5: add device parameter for MPRQ stride

size") Cc:[email protected]

Quoted 7 lines

Again, not sure if we can backport this patch, this looks a behavior change more than a fix.

Previously if the user provided values ends up being invalid, PMD seems returning error.
With this patch, instead of returning error PMD prefers to use default values and doesn't return error.

It isn't behavior change.
It existed before, except that it is concentrated into one function.

Quoted 1 line

Quoted 2 lines

I am not sure if it is correct thing to ignore (adjust) user provided values, but that can be up to the PMD as long as the behavior is documented.

Adjustment is the likely thing to do because the range depends on the device and the user does not necessarily know it.
This behavior is documented here https://doc.dpdk.org/guides/nics/mlx5.html#run-time-configuration (Run-time configuration -> Driver options -> mprq_log_stride_num/size)

Quoted 6 lines

But I think it is wrong to backport the behavior change.

Quoted 3 lines

Signed-off-by Michael Baum<[email protected]>

Acked-by Matan Azrad<[email protected]>

---

Quoted 36 lines · On 12/07/2021 3:41 PM, [email protected] wrote:

Quoted 8 lines · On 11/23/2021 6:38 PM, [email protected] wrote:

Quoted 4 lines

From: Michael Baum<[email protected]>

In Multy-Packet RQ creation, the user can choose the number of strides

Multi-Packet ?

Yes, you're right. It should have been Multi-Packet, thank you for that.

Quoted 23 lines

Quoted 12 lines

and their size in bytes. The user updates it using specific devargs for both of these parameters.
The above two parameters determine the size of the WQE which is actually their product of multiplication.

If the user selects values that are not in the supported range, the PMD changes them to default values. However, apart from the range limitations for each parameter individually there is also a minimum value on their multiplication. When the user selects values that their multiplication are lower than minimum value, no adjustment is made and the creation of the WQE fails.

Quoted 7 lines

Quoted 1 line

This patch adds an adjustment in these cases as well. When the user

selects values whose multiplication is lower than the minimum, they are replaced with the default values.

Fixes ecb160456aed ("net/mlx5: add device parameter for MPRQ stride

size") Cc:[email protected]

Again, not sure if we can backport this patch, this looks a behavior change more than a fix.

Quoted 11 lines

Quoted 4 lines

Previously if the user provided values ends up being invalid, PMD seems returning error.
With this patch, instead of returning error PMD prefers to use default values and doesn't return error.

It isn't behavior change.
It existed before, except that it is concentrated into one function.

Quoted 3 lines

I am not sure if it is correct thing to ignore (adjust) user provided values, but that can be up to the PMD as long as the behavior is

documented.

Quoted 2 lines

Adjustment is the likely thing to do because the range depends on the

device and the user does not necessarily know it.

Quoted 4 lines

This behavior is documented here
https://doc.dpdk.org/guides/nics/mlx5.html#run-time-configuration
(Run-time configuration -> Driver options -> mprq_log_stride_num/size)

It is documented that adjustments will be done if any specific argument is not in the range of the device capability.

It is not clear what will happen if the calculated value from both variables are not valid.

If it is not documented before, and previously it was returning error, now adjusting values to make it work looks like behavior change to me.

This is more of a process question, than technical detail in the driver, so @Luca, @Kevin, @Christian, can you please comment? I will follow your suggestion.

Quoted 7 lines

Quoted 6 lines

But I think it is wrong to backport the behavior change.

Quoted 3 lines

Signed-off-by Michael Baum<[email protected]>

Acked-by Matan Azrad<[email protected]>

---

Hi Ferruh

Thanks for the review.

Please see inside some clarifications.

From: Ferruh Yigit

Quoted 57 lines · Michael Baum

Quoted 36 lines · On 12/07/2021 3:41 PM, [email protected] wrote:

Quoted 8 lines · On 11/23/2021 6:38 PM, [email protected] wrote:

Quoted 4 lines

From: Michael Baum<[email protected]>

In Multy-Packet RQ creation, the user can choose the number of strides

Multi-Packet ?

Yes, you're right. It should have been Multi-Packet, thank you for that.

Quoted 23 lines

Quoted 12 lines

and their size in bytes. The user updates it using specific devargs for both of these parameters.
The above two parameters determine the size of the WQE which is actually their product of multiplication.

If the user selects values that are not in the supported range, the PMD changes them to default values. However, apart from the range limitations for each parameter individually there is also a minimum value on their multiplication. When the user selects values that their multiplication are lower than minimum value, no adjustment is made and the creation of the WQE fails.

Quoted 7 lines

Quoted 1 line

This patch adds an adjustment in these cases as well. When the user

selects values whose multiplication is lower than the minimum, they are replaced with the default values.

Fixes ecb160456aed ("net/mlx5: add device parameter for MPRQ stride

size") Cc:[email protected]

Again, not sure if we can backport this patch, this looks a behavior change more than a fix.

Quoted 11 lines

Quoted 4 lines

Previously if the user provided values ends up being invalid, PMD seems returning error.
With this patch, instead of returning error PMD prefers to use default values and doesn't return error.

It isn't behavior change.
It existed before, except that it is concentrated into one function.

Quoted 3 lines

I am not sure if it is correct thing to ignore (adjust) user provided values, but that can be up to the PMD as long as the behavior is

documented.

Quoted 2 lines

Adjustment is the likely thing to do because the range depends on the

device and the user does not necessarily know it.

Quoted 4 lines

This behavior is documented here
https://doc.dpdk.org/guides/nics/mlx5.html#run-time-configuration
(Run-time configuration -> Driver options -> mprq_log_stride_num/size)

Quoted 5 lines

It is documented that adjustments will be done if any specific argument is not in the range of the device capability.

It is not clear what will happen if the calculated value from both variables are not valid.

The driver should adjust it to a legal value.

Quoted 2 lines

If it is not documented before, and previously it was returning error, now adjusting values to make it work looks like behavior change to me.

The driver should not return an error - the driver should adjust to a legal value in case of illegal values by the user. It is documented in the devargs description.

Not behavior change but a bug fix; previously, the adjustment may return an error(which is a bug) or cause unexpected behavior in the HW(which is an old FW bug). Now, no error, no unexpected behavior - bug should be fixed for any FW version.

Quoted 4 lines

This is more of a process question, than technical detail in the driver, so @Luca, @Kevin, @Christian, can you please comment? I will follow your suggestion.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

But I think it is wrong to backport the behavior change.

Quoted 3 lines

Signed-off-by Michael Baum<[email protected]>

Acked-by Matan Azrad<[email protected]>

---

Quoted 72 lines · Matan Azrad

Hi Ferruh

Thanks for the review.

Please see inside some clarifications.

From: Ferruh Yigit

Quoted 57 lines · Michael Baum

Quoted 36 lines · On 12/07/2021 3:41 PM, [email protected] wrote:

Quoted 8 lines · On 11/23/2021 6:38 PM, [email protected] wrote:

Quoted 4 lines

From: Michael Baum<[email protected]>

In Multy-Packet RQ creation, the user can choose the number of strides

Multi-Packet ?

Yes, you're right. It should have been Multi-Packet, thank you for that.

Quoted 23 lines

Quoted 12 lines

and their size in bytes. The user updates it using specific devargs for both of these parameters.
The above two parameters determine the size of the WQE which is actually their product of multiplication.

If the user selects values that are not in the supported range, the PMD changes them to default values. However, apart from the range limitations for each parameter individually there is also a minimum value on their multiplication. When the user selects values that their multiplication are lower than minimum value, no adjustment is made and the creation of the WQE fails.

Quoted 7 lines

Quoted 1 line

This patch adds an adjustment in these cases as well. When the user

selects values whose multiplication is lower than the minimum, they are replaced with the default values.

Fixes ecb160456aed ("net/mlx5: add device parameter for MPRQ stride

size") Cc:[email protected]

Again, not sure if we can backport this patch, this looks a behavior change more than a fix.

Quoted 11 lines

Quoted 4 lines

Previously if the user provided values ends up being invalid, PMD seems returning error.
With this patch, instead of returning error PMD prefers to use default values and doesn't return error.

It isn't behavior change.
It existed before, except that it is concentrated into one function.

Quoted 3 lines

I am not sure if it is correct thing to ignore (adjust) user provided values, but that can be up to the PMD as long as the behavior is

documented.

Quoted 2 lines

Adjustment is the likely thing to do because the range depends on the

device and the user does not necessarily know it.

Quoted 4 lines

This behavior is documented here
https://doc.dpdk.org/guides/nics/mlx5.html#run-time-configuration
(Run-time configuration -> Driver options -> mprq_log_stride_num/size)

Quoted 5 lines

It is documented that adjustments will be done if any specific argument is not in the range of the device capability.

It is not clear what will happen if the calculated value from both variables are not valid.

The driver should adjust it to a legal value.

Quoted 9 lines

Quoted 2 lines

If it is not documented before, and previously it was returning error, now adjusting values to make it work looks like behavior change to me.

The driver should not return an error - the driver should adjust to a legal value in case of illegal values by the user. It is documented in the devargs description.

Not behavior change but a bug fix; previously, the adjustment may return an error(which is a bug) or cause unexpected behavior in the HW(which is an old FW bug). Now, no error, no unexpected behavior - bug should be fixed for any FW version.

I can understand both arguments. It is a behaviour change as the user will see a different behaviour for a given set of values.

However, each parameter is already validated and defaults are provided as backup. The combination not being checked could be seen a piece of missed validation for those values and a bug. In this case, given it is unlikely any user would be happy with the WQE creation failure, i think it is ok to backport the missing validation/adjustment.

Quoted 4 lines

Quoted 4 lines

This is more of a process question, than technical detail in the driver, so @Luca, @Kevin, @Christian, can you please comment? I will follow your suggestion.

Thanks for raising it Ferruh.

-----Original Message-----
From: Michael Baum <[email protected]>
Sent: Tuesday, November 23, 2021 8:38 PM
To: [email protected]
Cc: Matan Azrad <[email protected]>; Raslan Darawsheh <[email protected]>; Slava Ovsiienko <[email protected]>; Michael Baum <[email protected]>
Subject: [PATCH 0/3] fix MPRQ prepare

From: Michael Baum <[email protected]>

Prepare Multy-Packet RQ and fix missing devargs check.

Michael Baum (3):
common/mlx5: add min WQE size for striding RQ net/mlx5: improve stride parameter names net/mlx5: fix missing adjustment MPRQ stride devargs

 drivers/common/mlx5/mlx5_devx_cmds.c |  16 ++
 drivers/common/mlx5/mlx5_devx_cmds.h |   1 +
 drivers/common/mlx5/mlx5_prm.h       |  11 +-
 drivers/net/mlx5/linux/mlx5_os.c     |  56 ++----
 drivers/net/mlx5/linux/mlx5_verbs.c  |   4 +-
 drivers/net/mlx5/mlx5.c              |   4 +-
 drivers/net/mlx5/mlx5.h              |  12 +-
 drivers/net/mlx5/mlx5_defs.h         |   4 +-
 drivers/net/mlx5/mlx5_devx.c         |   4 +-
 drivers/net/mlx5/mlx5_rx.c           |  22 +--
 drivers/net/mlx5/mlx5_rx.h           |  12 +-
 drivers/net/mlx5/mlx5_rxq.c          | 247 ++++++++++++++++++---------
 drivers/net/mlx5/mlx5_rxtx_vec.c     |   8 +-
 13 files changed, 240 insertions(+), 161 deletions(-)

--
2.25.1