[PATCH 1/3] common/mlx5: add min WQE size for striding RQ

patches for DPDK stable branches
 help / color / mirror / Atom feed

* [PATCH 1/3] common/mlx5: add min WQE size for striding RQ
       [not found] <20211123183805.2905792-1-michaelba@nvidia.com>
@ 2021-11-23 18:38 ` michaelba
  2021-12-07 13:32   ` Ferruh Yigit
  2021-11-23 18:38 ` [PATCH 2/3] net/mlx5: improve stride parameter names michaelba
  2021-11-23 18:38 ` [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs michaelba
  2 siblings, 1 reply; 14+ messages in thread
From: michaelba @ 2021-11-23 18:38 UTC (permalink / raw)
  To: dev
  Cc: Matan Azrad, Raslan Darawsheh, Viacheslav Ovsiienko,
	Michael Baum, stable

From: Michael Baum <michaelba@nvidia.com>

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: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>
---
 drivers/common/mlx5/mlx5_devx_cmds.c | 16 ++++++++++++++++
 drivers/common/mlx5/mlx5_devx_cmds.h |  1 +
 drivers/common/mlx5/mlx5_prm.h       | 11 +++++++++--
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c b/drivers/common/mlx5/mlx5_devx_cmds.c
index e52b995ee3..a8efdbe1ae 100644
--- a/drivers/common/mlx5/mlx5_devx_cmds.c
+++ b/drivers/common/mlx5/mlx5_devx_cmds.c
@@ -823,6 +823,7 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
 {
 	uint32_t in[MLX5_ST_SZ_DW(query_hca_cap_in)] = {0};
 	uint32_t out[MLX5_ST_SZ_DW(query_hca_cap_out)] = {0};
+	bool hca_cap_2_sup;
 	uint64_t general_obj_types_supported = 0;
 	void *hcattr;
 	int rc, i;
@@ -832,6 +833,7 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
 			MLX5_HCA_CAP_OPMOD_GET_CUR);
 	if (!hcattr)
 		return rc;
+	hca_cap_2_sup = MLX5_GET(cmd_hca_cap, hcattr, hca_cap_2);
 	attr->max_wqe_sz_sq = MLX5_GET(cmd_hca_cap, hcattr, max_wqe_sz_sq);
 	attr->flow_counter_bulk_alloc_bitmap =
 			MLX5_GET(cmd_hca_cap, hcattr, flow_counter_bulk_alloc);
@@ -967,6 +969,20 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
 					 general_obj_types) &
 			      MLX5_GENERAL_OBJ_TYPES_CAP_CONN_TRACK_OFFLOAD);
 	attr->rq_delay_drop = MLX5_GET(cmd_hca_cap, hcattr, rq_delay_drop);
+	if (hca_cap_2_sup) {
+		hcattr = mlx5_devx_get_hca_cap(ctx, in, out, &rc,
+				MLX5_GET_HCA_CAP_OP_MOD_GENERAL_DEVICE_2 |
+				MLX5_HCA_CAP_OPMOD_GET_CUR);
+		if (!hcattr) {
+			DRV_LOG(DEBUG,
+				"Failed to query DevX HCA capabilities 2.");
+			return rc;
+		}
+		attr->log_min_stride_wqe_sz = MLX5_GET(cmd_hca_cap_2, hcattr,
+						       log_min_stride_wqe_sz);
+	}
+	if (attr->log_min_stride_wqe_sz == 0)
+		attr->log_min_stride_wqe_sz = MLX5_MPRQ_LOG_MIN_STRIDE_WQE_SIZE;
 	if (attr->qos.sup) {
 		hcattr = mlx5_devx_get_hca_cap(ctx, in, out, &rc,
 				MLX5_GET_HCA_CAP_OP_MOD_QOS_CAP |
diff --git a/drivers/common/mlx5/mlx5_devx_cmds.h b/drivers/common/mlx5/mlx5_devx_cmds.h
index d7f71646a3..37821b493e 100644
--- a/drivers/common/mlx5/mlx5_devx_cmds.h
+++ b/drivers/common/mlx5/mlx5_devx_cmds.h
@@ -251,6 +251,7 @@ struct mlx5_hca_attr {
 	uint32_t log_max_mmo_decompress:5;
 	uint32_t umr_modify_entity_size_disabled:1;
 	uint32_t umr_indirect_mkey_disabled:1;
+	uint32_t log_min_stride_wqe_sz:5;
 	uint16_t max_wqe_sz_sq;
 };
 
diff --git a/drivers/common/mlx5/mlx5_prm.h b/drivers/common/mlx5/mlx5_prm.h
index 2ded67e85e..8a7cb0e673 100644
--- a/drivers/common/mlx5/mlx5_prm.h
+++ b/drivers/common/mlx5/mlx5_prm.h
@@ -264,6 +264,9 @@
 /* The maximum log value of segments per RQ WQE. */
 #define MLX5_MAX_LOG_RQ_SEGS 5u
 
+/* Log 2 of the default size of a WQE for Multi-Packet RQ. */
+#define MLX5_MPRQ_LOG_MIN_STRIDE_WQE_SIZE 14U
+
 /* The alignment needed for WQ buffer. */
 #define MLX5_WQE_BUF_ALIGNMENT rte_mem_page_size()
 
@@ -1342,7 +1345,9 @@ enum {
 #define MLX5_STEERING_LOGIC_FORMAT_CONNECTX_6DX 0x1
 
 struct mlx5_ifc_cmd_hca_cap_bits {
-	u8 reserved_at_0[0x30];
+	u8 reserved_at_0[0x20];
+	u8 hca_cap_2[0x1];
+	u8 reserved_at_21[0xf];
 	u8 vhca_id[0x10];
 	u8 reserved_at_40[0x20];
 	u8 reserved_at_60[0x3];
@@ -1909,7 +1914,8 @@ struct mlx5_ifc_cmd_hca_cap_2_bits {
 	u8 max_reformat_insert_offset[0x8];
 	u8 max_reformat_remove_size[0x8];
 	u8 max_reformat_remove_offset[0x8]; /* End of DW6. */
-	u8 aso_conntrack_reg_id[0x8];
+	u8 reserved_at_c0[0x3];
+	u8 log_min_stride_wqe_sz[0x5];
 	u8 reserved_at_c8[0x3];
 	u8 log_conn_track_granularity[0x5];
 	u8 reserved_at_d0[0x3];
@@ -1922,6 +1928,7 @@ struct mlx5_ifc_cmd_hca_cap_2_bits {
 
 union mlx5_ifc_hca_cap_union_bits {
 	struct mlx5_ifc_cmd_hca_cap_bits cmd_hca_cap;
+	struct mlx5_ifc_cmd_hca_cap_2_bits cmd_hca_cap_2;
 	struct mlx5_ifc_per_protocol_networking_offload_caps_bits
 	       per_protocol_networking_offload_caps;
 	struct mlx5_ifc_qos_cap_bits qos_cap;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/3] net/mlx5: improve stride parameter names
       [not found] <20211123183805.2905792-1-michaelba@nvidia.com>
  2021-11-23 18:38 ` [PATCH 1/3] common/mlx5: add min WQE size for striding RQ michaelba
@ 2021-11-23 18:38 ` michaelba
  2021-12-07 13:33   ` Ferruh Yigit
  2021-11-23 18:38 ` [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs michaelba
  2 siblings, 1 reply; 14+ messages in thread
From: michaelba @ 2021-11-23 18:38 UTC (permalink / raw)
  To: dev
  Cc: Matan Azrad, Raslan Darawsheh, Viacheslav Ovsiienko,
	Michael Baum, stable

From: Michael Baum <michaelba@nvidia.com>

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: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>
---
 drivers/net/mlx5/linux/mlx5_os.c    |  38 +++++------
 drivers/net/mlx5/linux/mlx5_verbs.c |   4 +-
 drivers/net/mlx5/mlx5.c             |   4 +-
 drivers/net/mlx5/mlx5.h             |   8 +--
 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         | 102 +++++++++++++++-------------
 drivers/net/mlx5/mlx5_rxtx_vec.c    |   8 +--
 10 files changed, 106 insertions(+), 100 deletions(-)

diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
index c29fe3d92b..70472efc29 100644
--- a/drivers/net/mlx5/linux/mlx5_os.c
+++ b/drivers/net/mlx5/linux/mlx5_os.c
@@ -1549,34 +1549,34 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
 	DRV_LOG(DEBUG, "FCS stripping configuration is %ssupported",
 		(config->hw_fcs_strip ? "" : "not "));
 	if (config->mprq.enabled && mprq) {
-		if (config->mprq.stride_num_n &&
-		    (config->mprq.stride_num_n > mprq_max_stride_num_n ||
-		     config->mprq.stride_num_n < mprq_min_stride_num_n)) {
-			config->mprq.stride_num_n =
-				RTE_MIN(RTE_MAX(MLX5_MPRQ_STRIDE_NUM_N,
-						mprq_min_stride_num_n),
-					mprq_max_stride_num_n);
+		if (config->mprq.log_stride_num &&
+		    (config->mprq.log_stride_num > mprq_max_stride_num_n ||
+		     config->mprq.log_stride_num < mprq_min_stride_num_n)) {
+			config->mprq.log_stride_num =
+			       RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM,
+					       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.stride_num_n);
-		}
-		if (config->mprq.stride_size_n &&
-		    (config->mprq.stride_size_n > mprq_max_stride_size_n ||
-		     config->mprq.stride_size_n < mprq_min_stride_size_n)) {
-			config->mprq.stride_size_n =
-				RTE_MIN(RTE_MAX(MLX5_MPRQ_STRIDE_SIZE_N,
-						mprq_min_stride_size_n),
-					mprq_max_stride_size_n);
+				1 << config->mprq.log_stride_num);
+		}
+		if (config->mprq.log_stride_size &&
+		    (config->mprq.log_stride_size > mprq_max_stride_size_n ||
+		     config->mprq.log_stride_size < mprq_min_stride_size_n)) {
+			config->mprq.log_stride_size =
+			      RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE,
+					      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.stride_size_n);
+				1 << config->mprq.log_stride_size);
 		}
-		config->mprq.min_stride_size_n = mprq_min_stride_size_n;
-		config->mprq.max_stride_size_n = mprq_max_stride_size_n;
+		config->mprq.log_min_stride_size = mprq_min_stride_size_n;
+		config->mprq.log_max_stride_size = mprq_max_stride_size_n;
 	} else if (config->mprq.enabled && !mprq) {
 		DRV_LOG(WARNING, "Multi-Packet RQ isn't supported");
 		config->mprq.enabled = 0;
diff --git a/drivers/net/mlx5/linux/mlx5_verbs.c b/drivers/net/mlx5/linux/mlx5_verbs.c
index 58556d2bf0..2b6eef44a7 100644
--- a/drivers/net/mlx5/linux/mlx5_verbs.c
+++ b/drivers/net/mlx5/linux/mlx5_verbs.c
@@ -272,8 +272,8 @@ mlx5_rxq_ibv_wq_create(struct mlx5_rxq_priv *rxq)
 
 		wq_attr.mlx5.comp_mask |= MLX5DV_WQ_INIT_ATTR_MASK_STRIDING_RQ;
 		*mprq_attr = (struct mlx5dv_striding_rq_init_attr){
-			.single_stride_log_num_of_bytes = rxq_data->strd_sz_n,
-			.single_wqe_log_num_of_strides = rxq_data->strd_num_n,
+			.single_stride_log_num_of_bytes = rxq_data->log_strd_sz,
+			.single_wqe_log_num_of_strides = rxq_data->log_strd_num,
 			.two_byte_shift_en = MLX5_MPRQ_TWO_BYTE_SHIFT,
 		};
 	}
diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 4e04817d11..8c654045c6 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -1884,9 +1884,9 @@ mlx5_args_check(const char *key, const char *val, void *opaque)
 	} else if (strcmp(MLX5_RX_MPRQ_EN, key) == 0) {
 		config->mprq.enabled = !!tmp;
 	} else if (strcmp(MLX5_RX_MPRQ_LOG_STRIDE_NUM, key) == 0) {
-		config->mprq.stride_num_n = tmp;
+		config->mprq.log_stride_num = tmp;
 	} else if (strcmp(MLX5_RX_MPRQ_LOG_STRIDE_SIZE, key) == 0) {
-		config->mprq.stride_size_n = tmp;
+		config->mprq.log_stride_size = tmp;
 	} else if (strcmp(MLX5_RX_MPRQ_MAX_MEMCPY_LEN, key) == 0) {
 		config->mprq.max_memcpy_len = tmp;
 	} else if (strcmp(MLX5_RXQS_MIN_MPRQ, key) == 0) {
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 8466531060..4ba90db816 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -275,10 +275,10 @@ struct mlx5_dev_config {
 	unsigned int hp_delay_drop:1; /* Enable hairpin Rxq delay drop. */
 	struct {
 		unsigned int enabled:1; /* Whether MPRQ is enabled. */
-		unsigned int stride_num_n; /* Number of strides. */
-		unsigned int stride_size_n; /* Size of a stride. */
-		unsigned int min_stride_size_n; /* Min size of a stride. */
-		unsigned int max_stride_size_n; /* Max size of a stride. */
+		unsigned int log_stride_num; /* Log number of strides. */
+		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.*/
 		unsigned int max_memcpy_len;
 		/* Maximum packet size to memcpy Rx packets. */
 		unsigned int min_rxqs_num;
diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index 258475ed2c..36b384fa08 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -113,10 +113,10 @@
 #define MLX5_UAR_PAGE_NUM_MASK ((MLX5_UAR_PAGE_NUM_MAX) - 1)
 
 /* Log 2 of the default number of strides per WQE for Multi-Packet RQ. */
-#define MLX5_MPRQ_STRIDE_NUM_N 6U
+#define MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM 6U
 
 /* Log 2 of the default size of a stride per WQE for Multi-Packet RQ. */
-#define MLX5_MPRQ_STRIDE_SIZE_N 11U
+#define MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE 11U
 
 /* Two-byte shift is disabled for Multi-Packet RQ. */
 #define MLX5_MPRQ_TWO_BYTE_SHIFT 0
diff --git a/drivers/net/mlx5/mlx5_devx.c b/drivers/net/mlx5/mlx5_devx.c
index 105c3d67f0..91243f684f 100644
--- a/drivers/net/mlx5/mlx5_devx.c
+++ b/drivers/net/mlx5/mlx5_devx.c
@@ -257,11 +257,11 @@ mlx5_rxq_create_devx_rq_resources(struct mlx5_rxq_priv *rxq)
 		 * 512*2^single_wqe_log_num_of_strides.
 		 */
 		rq_attr.wq_attr.single_wqe_log_num_of_strides =
-				rxq_data->strd_num_n -
+				rxq_data->log_strd_num -
 				MLX5_MIN_SINGLE_WQE_LOG_NUM_STRIDES;
 		/* Stride size = (2^single_stride_log_num_of_bytes)*64B. */
 		rq_attr.wq_attr.single_stride_log_num_of_bytes =
-				rxq_data->strd_sz_n -
+				rxq_data->log_strd_sz -
 				MLX5_MIN_SINGLE_STRIDE_LOG_NUM_BYTES;
 		wqe_size = sizeof(struct mlx5_wqe_mprq);
 	} else {
diff --git a/drivers/net/mlx5/mlx5_rx.c b/drivers/net/mlx5/mlx5_rx.c
index e8215f7381..6b169b33c9 100644
--- a/drivers/net/mlx5/mlx5_rx.c
+++ b/drivers/net/mlx5/mlx5_rx.c
@@ -73,7 +73,7 @@ rx_queue_count(struct mlx5_rxq_data *rxq)
 	const unsigned int cqe_n = (1 << rxq->cqe_n);
 	const unsigned int sges_n = (1 << rxq->sges_n);
 	const unsigned int elts_n = (1 << rxq->elts_n);
-	const unsigned int strd_n = (1 << rxq->strd_num_n);
+	const unsigned int strd_n = RTE_BIT32(rxq->log_strd_num);
 	const unsigned int cqe_cnt = cqe_n - 1;
 	unsigned int cq_ci, used;
 
@@ -167,8 +167,8 @@ mlx5_rxq_info_get(struct rte_eth_dev *dev, uint16_t rx_queue_id,
 	qinfo->conf.offloads = dev->data->dev_conf.rxmode.offloads;
 	qinfo->scattered_rx = dev->data->scattered_rx;
 	qinfo->nb_desc = mlx5_rxq_mprq_enabled(rxq) ?
-		(1 << rxq->elts_n) * (1 << rxq->strd_num_n) :
-		(1 << rxq->elts_n);
+		RTE_BIT32(rxq->elts_n) * RTE_BIT32(rxq->log_strd_num) :
+		RTE_BIT32(rxq->elts_n);
 }
 
 /**
@@ -354,10 +354,10 @@ mlx5_rxq_initialize(struct mlx5_rxq_data *rxq)
 
 			scat = &((volatile struct mlx5_wqe_mprq *)
 				rxq->wqes)[i].dseg;
-			addr = (uintptr_t)mlx5_mprq_buf_addr(buf,
-							 1 << rxq->strd_num_n);
-			byte_count = (1 << rxq->strd_sz_n) *
-					(1 << rxq->strd_num_n);
+			addr = (uintptr_t)mlx5_mprq_buf_addr
+					(buf, RTE_BIT32(rxq->log_strd_num));
+			byte_count = RTE_BIT32(rxq->log_strd_sz) *
+				     RTE_BIT32(rxq->log_strd_num);
 			lkey = mlx5_rx_addr2mr(rxq, addr);
 		} else {
 			struct rte_mbuf *buf = (*rxq->elts)[i];
@@ -383,7 +383,7 @@ mlx5_rxq_initialize(struct mlx5_rxq_data *rxq)
 		.ai = 0,
 	};
 	rxq->elts_ci = mlx5_rxq_mprq_enabled(rxq) ?
-		(wqe_n >> rxq->sges_n) * (1 << rxq->strd_num_n) : 0;
+		(wqe_n >> rxq->sges_n) * RTE_BIT32(rxq->log_strd_num) : 0;
 	/* Update doorbell counter. */
 	rxq->rq_ci = wqe_n >> rxq->sges_n;
 	rte_io_wmb();
@@ -412,7 +412,7 @@ mlx5_rx_err_handle(struct mlx5_rxq_data *rxq, uint8_t vec)
 	const uint16_t cqe_n = 1 << rxq->cqe_n;
 	const uint16_t cqe_mask = cqe_n - 1;
 	const uint16_t wqe_n = 1 << rxq->elts_n;
-	const uint16_t strd_n = 1 << rxq->strd_num_n;
+	const uint16_t strd_n = RTE_BIT32(rxq->log_strd_num);
 	struct mlx5_rxq_ctrl *rxq_ctrl =
 			container_of(rxq, struct mlx5_rxq_ctrl, rxq);
 	union {
@@ -1045,8 +1045,8 @@ uint16_t
 mlx5_rx_burst_mprq(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
 {
 	struct mlx5_rxq_data *rxq = dpdk_rxq;
-	const uint32_t strd_n = 1 << rxq->strd_num_n;
-	const uint32_t strd_sz = 1 << rxq->strd_sz_n;
+	const uint32_t strd_n = RTE_BIT32(rxq->log_strd_num);
+	const uint32_t strd_sz = RTE_BIT32(rxq->log_strd_sz);
 	const uint32_t cq_mask = (1 << rxq->cqe_n) - 1;
 	const uint32_t wq_mask = (1 << rxq->elts_n) - 1;
 	volatile struct mlx5_cqe *cqe = &(*rxq->cqes)[rxq->cq_ci & cq_mask];
diff --git a/drivers/net/mlx5/mlx5_rx.h b/drivers/net/mlx5/mlx5_rx.h
index 9cc1a2703b..4651826a1d 100644
--- a/drivers/net/mlx5/mlx5_rx.h
+++ b/drivers/net/mlx5/mlx5_rx.h
@@ -88,8 +88,8 @@ struct mlx5_rxq_data {
 	unsigned int elts_n:4; /* Log 2 of Mbufs. */
 	unsigned int rss_hash:1; /* RSS hash result is enabled. */
 	unsigned int mark:1; /* Marked flow available on the queue. */
-	unsigned int strd_num_n:5; /* Log 2 of the number of stride. */
-	unsigned int strd_sz_n:4; /* Log 2 of stride size. */
+	unsigned int log_strd_num:5; /* Log 2 of the number of stride. */
+	unsigned int log_strd_sz:4; /* Log 2 of stride size. */
 	unsigned int strd_shift_en:1; /* Enable 2bytes shift on a stride. */
 	unsigned int err_state:2; /* enum mlx5_rxq_err_state. */
 	unsigned int strd_scatter_en:1; /* Scattered packets from a stride. */
@@ -395,7 +395,7 @@ mlx5_timestamp_set(struct rte_mbuf *mbuf, int offset,
 static __rte_always_inline void
 mprq_buf_replace(struct mlx5_rxq_data *rxq, uint16_t rq_idx)
 {
-	const uint32_t strd_n = 1 << rxq->strd_num_n;
+	const uint32_t strd_n = RTE_BIT32(rxq->log_strd_num);
 	struct mlx5_mprq_buf *rep = rxq->mprq_repl;
 	volatile struct mlx5_wqe_data_seg *wqe =
 		&((volatile struct mlx5_wqe_mprq *)rxq->wqes)[rq_idx].dseg;
@@ -453,8 +453,8 @@ static __rte_always_inline enum mlx5_rqx_code
 mprq_buf_to_pkt(struct mlx5_rxq_data *rxq, struct rte_mbuf *pkt, uint32_t len,
 		struct mlx5_mprq_buf *buf, uint16_t strd_idx, uint16_t strd_cnt)
 {
-	const uint32_t strd_n = 1 << rxq->strd_num_n;
-	const uint16_t strd_sz = 1 << rxq->strd_sz_n;
+	const uint32_t strd_n = RTE_BIT32(rxq->log_strd_num);
+	const uint16_t strd_sz = RTE_BIT32(rxq->log_strd_sz);
 	const uint16_t strd_shift =
 		MLX5_MPRQ_STRIDE_SHIFT_BYTE * rxq->strd_shift_en;
 	const int32_t hdrm_overlap =
@@ -599,7 +599,7 @@ mlx5_check_mprq_support(struct rte_eth_dev *dev)
 static __rte_always_inline int
 mlx5_rxq_mprq_enabled(struct mlx5_rxq_data *rxq)
 {
-	return rxq->strd_num_n > 0;
+	return rxq->log_strd_num > 0;
 }
 
 /**
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index e406779faf..e76bfaa000 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -67,7 +67,7 @@ mlx5_rxq_cqe_num(struct mlx5_rxq_data *rxq_data)
 	unsigned int wqe_n = 1 << rxq_data->elts_n;
 
 	if (mlx5_rxq_mprq_enabled(rxq_data))
-		cqe_n = wqe_n * (1 << rxq_data->strd_num_n) - 1;
+		cqe_n = wqe_n * RTE_BIT32(rxq_data->log_strd_num) - 1;
 	else
 		cqe_n = wqe_n - 1;
 	return cqe_n;
@@ -137,8 +137,9 @@ rxq_alloc_elts_sprq(struct mlx5_rxq_ctrl *rxq_ctrl)
 {
 	const unsigned int sges_n = 1 << rxq_ctrl->rxq.sges_n;
 	unsigned int elts_n = mlx5_rxq_mprq_enabled(&rxq_ctrl->rxq) ?
-		(1 << rxq_ctrl->rxq.elts_n) * (1 << rxq_ctrl->rxq.strd_num_n) :
-		(1 << rxq_ctrl->rxq.elts_n);
+			      RTE_BIT32(rxq_ctrl->rxq.elts_n) *
+			      RTE_BIT32(rxq_ctrl->rxq.log_strd_num) :
+			      RTE_BIT32(rxq_ctrl->rxq.elts_n);
 	unsigned int i;
 	int err;
 
@@ -293,8 +294,8 @@ rxq_free_elts_sprq(struct mlx5_rxq_ctrl *rxq_ctrl)
 {
 	struct mlx5_rxq_data *rxq = &rxq_ctrl->rxq;
 	const uint16_t q_n = mlx5_rxq_mprq_enabled(&rxq_ctrl->rxq) ?
-		(1 << rxq->elts_n) * (1 << rxq->strd_num_n) :
-		(1 << rxq->elts_n);
+		RTE_BIT32(rxq->elts_n) * RTE_BIT32(rxq->log_strd_num) :
+		RTE_BIT32(rxq->elts_n);
 	const uint16_t q_mask = q_n - 1;
 	uint16_t elts_ci = mlx5_rxq_mprq_enabled(&rxq_ctrl->rxq) ?
 		rxq->elts_ci : rxq->rq_ci;
@@ -1373,8 +1374,8 @@ mlx5_mprq_alloc_mp(struct rte_eth_dev *dev)
 	unsigned int buf_len;
 	unsigned int obj_num;
 	unsigned int obj_size;
-	unsigned int strd_num_n = 0;
-	unsigned int strd_sz_n = 0;
+	unsigned int log_strd_num = 0;
+	unsigned int log_strd_sz = 0;
 	unsigned int i;
 	unsigned int n_ibv = 0;
 	int ret;
@@ -1393,16 +1394,18 @@ mlx5_mprq_alloc_mp(struct rte_eth_dev *dev)
 		n_ibv++;
 		desc += 1 << rxq->elts_n;
 		/* Get the max number of strides. */
-		if (strd_num_n < rxq->strd_num_n)
-			strd_num_n = rxq->strd_num_n;
+		if (log_strd_num < rxq->log_strd_num)
+			log_strd_num = rxq->log_strd_num;
 		/* Get the max size of a stride. */
-		if (strd_sz_n < rxq->strd_sz_n)
-			strd_sz_n = rxq->strd_sz_n;
-	}
-	MLX5_ASSERT(strd_num_n && strd_sz_n);
-	buf_len = (1 << strd_num_n) * (1 << strd_sz_n);
-	obj_size = sizeof(struct mlx5_mprq_buf) + buf_len + (1 << strd_num_n) *
-		sizeof(struct rte_mbuf_ext_shared_info) + RTE_PKTMBUF_HEADROOM;
+		if (log_strd_sz < rxq->log_strd_sz)
+			log_strd_sz = rxq->log_strd_sz;
+	}
+	MLX5_ASSERT(log_strd_num && log_strd_sz);
+	buf_len = RTE_BIT32(log_strd_num) * RTE_BIT32(log_strd_sz);
+	obj_size = sizeof(struct mlx5_mprq_buf) + buf_len +
+		   RTE_BIT32(log_strd_num) *
+		   sizeof(struct rte_mbuf_ext_shared_info) +
+		   RTE_PKTMBUF_HEADROOM;
 	/*
 	 * Received packets can be either memcpy'd or externally referenced. In
 	 * case that the packet is attached to an mbuf as an external buffer, as
@@ -1448,7 +1451,7 @@ mlx5_mprq_alloc_mp(struct rte_eth_dev *dev)
 	snprintf(name, sizeof(name), "port-%u-mprq", dev->data->port_id);
 	mp = rte_mempool_create(name, obj_num, obj_size, MLX5_MPRQ_MP_CACHE_SZ,
 				0, NULL, NULL, mlx5_mprq_buf_init,
-				(void *)((uintptr_t)1 << strd_num_n),
+				(void *)((uintptr_t)1 << log_strd_num),
 				dev->device->numa_node, 0);
 	if (mp == NULL) {
 		DRV_LOG(ERR,
@@ -1564,15 +1567,18 @@ mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 	unsigned int first_mb_free_size = mb_len - RTE_PKTMBUF_HEADROOM;
 	const int mprq_en = mlx5_check_mprq_support(dev) > 0 && n_seg == 1 &&
 			    !rx_seg[0].offset && !rx_seg[0].length;
-	unsigned int mprq_stride_nums = config->mprq.stride_num_n ?
-		config->mprq.stride_num_n : MLX5_MPRQ_STRIDE_NUM_N;
-	unsigned int mprq_stride_size = non_scatter_min_mbuf_size <=
-		(1U << config->mprq.max_stride_size_n) ?
-		log2above(non_scatter_min_mbuf_size) : MLX5_MPRQ_STRIDE_SIZE_N;
-	unsigned int mprq_stride_cap = (config->mprq.stride_num_n ?
-		(1U << config->mprq.stride_num_n) : (1U << mprq_stride_nums)) *
-		(config->mprq.stride_size_n ?
-		(1U << config->mprq.stride_size_n) : (1U << mprq_stride_size));
+	unsigned int log_mprq_stride_nums = config->mprq.log_stride_num ?
+		config->mprq.log_stride_num : MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM;
+	unsigned int log_mprq_stride_size = non_scatter_min_mbuf_size <=
+		RTE_BIT32(config->mprq.log_max_stride_size) ?
+		log2above(non_scatter_min_mbuf_size) :
+		MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE;
+	unsigned int mprq_stride_cap = (config->mprq.log_stride_num ?
+					RTE_BIT32(config->mprq.log_stride_num) :
+					RTE_BIT32(log_mprq_stride_nums)) *
+				       (config->mprq.log_stride_size ?
+				       RTE_BIT32(config->mprq.log_stride_size) :
+					RTE_BIT32(log_mprq_stride_size));
 	/*
 	 * Always allocate extra slots, even if eventually
 	 * the vector Rx will not be used.
@@ -1584,7 +1590,7 @@ mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 	tmpl = mlx5_malloc(MLX5_MEM_RTE | MLX5_MEM_ZERO,
 		sizeof(*tmpl) + desc_n * sizeof(struct rte_mbuf *) +
 		(!!mprq_en) *
-		(desc >> mprq_stride_nums) * sizeof(struct mlx5_mprq_buf *),
+		(desc >> log_mprq_stride_nums) * sizeof(struct mlx5_mprq_buf *),
 		0, socket);
 	if (!tmpl) {
 		rte_errno = ENOMEM;
@@ -1689,37 +1695,37 @@ mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 	 *  - MPRQ is enabled.
 	 *  - The number of descs is more than the number of strides.
 	 *  - max_rx_pktlen plus overhead is less than the max size
-	 *    of a stride or mprq_stride_size is specified by a user.
+	 *    of a stride or log_mprq_stride_size is specified by a user.
 	 *    Need to make sure that there are enough strides to encap
-	 *    the maximum packet size in case mprq_stride_size is set.
+	 *    the maximum packet size in case log_mprq_stride_size is set.
 	 *  Otherwise, enable Rx scatter if necessary.
 	 */
-	if (mprq_en && desc > (1U << mprq_stride_nums) &&
+	if (mprq_en && desc > RTE_BIT32(log_mprq_stride_nums) &&
 	    (non_scatter_min_mbuf_size <=
-	     (1U << config->mprq.max_stride_size_n) ||
-	     (config->mprq.stride_size_n &&
+	     RTE_BIT32(config->mprq.log_max_stride_size) ||
+	     (config->mprq.log_stride_size &&
 	      non_scatter_min_mbuf_size <= mprq_stride_cap))) {
 		/* TODO: Rx scatter isn't supported yet. */
 		tmpl->rxq.sges_n = 0;
 		/* Trim the number of descs needed. */
-		desc >>= mprq_stride_nums;
-		tmpl->rxq.strd_num_n = config->mprq.stride_num_n ?
-			config->mprq.stride_num_n : mprq_stride_nums;
-		tmpl->rxq.strd_sz_n = config->mprq.stride_size_n ?
-			config->mprq.stride_size_n : mprq_stride_size;
+		desc >>= log_mprq_stride_nums;
+		tmpl->rxq.log_strd_num = config->mprq.log_stride_num ?
+			config->mprq.log_stride_num : log_mprq_stride_nums;
+		tmpl->rxq.log_strd_sz = config->mprq.log_stride_size ?
+			config->mprq.log_stride_size : log_mprq_stride_size;
 		tmpl->rxq.strd_shift_en = MLX5_MPRQ_TWO_BYTE_SHIFT;
 		tmpl->rxq.strd_scatter_en =
 				!!(offloads & RTE_ETH_RX_OFFLOAD_SCATTER);
 		tmpl->rxq.mprq_max_memcpy_len = RTE_MIN(first_mb_free_size,
 				config->mprq.max_memcpy_len);
 		max_lro_size = RTE_MIN(max_rx_pktlen,
-				       (1u << tmpl->rxq.strd_num_n) *
-				       (1u << tmpl->rxq.strd_sz_n));
+				       RTE_BIT32(tmpl->rxq.log_strd_num) *
+				       RTE_BIT32(tmpl->rxq.log_strd_sz));
 		DRV_LOG(DEBUG,
 			"port %u Rx queue %u: Multi-Packet RQ is enabled"
 			" strd_num_n = %u, strd_sz_n = %u",
 			dev->data->port_id, idx,
-			tmpl->rxq.strd_num_n, tmpl->rxq.strd_sz_n);
+			tmpl->rxq.log_strd_num, tmpl->rxq.log_strd_sz);
 	} else if (tmpl->rxq.rxseg_n == 1) {
 		MLX5_ASSERT(max_rx_pktlen <= first_mb_free_size);
 		tmpl->rxq.sges_n = 0;
@@ -1762,15 +1768,15 @@ mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 			" min_stride_sz = %u, max_stride_sz = %u).",
 			dev->data->port_id, non_scatter_min_mbuf_size,
 			desc, priv->rxqs_n,
-			config->mprq.stride_size_n ?
-				(1U << config->mprq.stride_size_n) :
-				(1U << mprq_stride_size),
-			config->mprq.stride_num_n ?
-				(1U << config->mprq.stride_num_n) :
-				(1U << mprq_stride_nums),
+			config->mprq.log_stride_size ?
+				RTE_BIT32(config->mprq.log_stride_size) :
+				RTE_BIT32(log_mprq_stride_size),
+			config->mprq.log_stride_num ?
+				RTE_BIT32(config->mprq.log_stride_num) :
+				RTE_BIT32(log_mprq_stride_nums),
 			config->mprq.min_rxqs_num,
-			(1U << config->mprq.min_stride_size_n),
-			(1U << config->mprq.max_stride_size_n));
+			RTE_BIT32(config->mprq.log_min_stride_size),
+			RTE_BIT32(config->mprq.log_max_stride_size));
 	DRV_LOG(DEBUG, "port %u maximum number of segments per packet: %u",
 		dev->data->port_id, 1 << tmpl->rxq.sges_n);
 	if (desc % (1 << tmpl->rxq.sges_n)) {
diff --git a/drivers/net/mlx5/mlx5_rxtx_vec.c b/drivers/net/mlx5/mlx5_rxtx_vec.c
index 6212ce8247..0e2eab068a 100644
--- a/drivers/net/mlx5/mlx5_rxtx_vec.c
+++ b/drivers/net/mlx5/mlx5_rxtx_vec.c
@@ -148,7 +148,7 @@ static inline void
 mlx5_rx_mprq_replenish_bulk_mbuf(struct mlx5_rxq_data *rxq)
 {
 	const uint16_t wqe_n = 1 << rxq->elts_n;
-	const uint32_t strd_n = 1 << rxq->strd_num_n;
+	const uint32_t strd_n = RTE_BIT32(rxq->log_strd_num);
 	const uint32_t elts_n = wqe_n * strd_n;
 	const uint32_t wqe_mask = elts_n - 1;
 	uint32_t n = elts_n - (rxq->elts_ci - rxq->rq_pi);
@@ -197,8 +197,8 @@ rxq_copy_mprq_mbuf_v(struct mlx5_rxq_data *rxq,
 {
 	const uint16_t wqe_n = 1 << rxq->elts_n;
 	const uint16_t wqe_mask = wqe_n - 1;
-	const uint16_t strd_sz = 1 << rxq->strd_sz_n;
-	const uint32_t strd_n = 1 << rxq->strd_num_n;
+	const uint16_t strd_sz = RTE_BIT32(rxq->log_strd_sz);
+	const uint32_t strd_n = RTE_BIT32(rxq->log_strd_num);
 	const uint32_t elts_n = wqe_n * strd_n;
 	const uint32_t elts_mask = elts_n - 1;
 	uint32_t elts_idx = rxq->rq_pi & elts_mask;
@@ -428,7 +428,7 @@ rxq_burst_mprq_v(struct mlx5_rxq_data *rxq, struct rte_mbuf **pkts,
 	const uint16_t q_n = 1 << rxq->cqe_n;
 	const uint16_t q_mask = q_n - 1;
 	const uint16_t wqe_n = 1 << rxq->elts_n;
-	const uint32_t strd_n = 1 << rxq->strd_num_n;
+	const uint32_t strd_n = RTE_BIT32(rxq->log_strd_num);
 	const uint32_t elts_n = wqe_n * strd_n;
 	const uint32_t elts_mask = elts_n - 1;
 	volatile struct mlx5_cqe *cq;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs
       [not found] <20211123183805.2905792-1-michaelba@nvidia.com>
  2021-11-23 18:38 ` [PATCH 1/3] common/mlx5: add min WQE size for striding RQ michaelba
  2021-11-23 18:38 ` [PATCH 2/3] net/mlx5: improve stride parameter names michaelba
@ 2021-11-23 18:38 ` michaelba
  2021-11-23 20:41   ` Raslan Darawsheh
  2021-12-07 13:40   ` Ferruh Yigit
  2 siblings, 2 replies; 14+ messages in thread
From: michaelba @ 2021-11-23 18:38 UTC (permalink / raw)
  To: dev
  Cc: Matan Azrad, Raslan Darawsheh, Viacheslav Ovsiienko,
	Michael Baum, stable

From: Michael Baum <michaelba@nvidia.com>

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: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>
---
 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(-)

diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
index 70472efc29..3e496d68ea 100644
--- a/drivers/net/mlx5/linux/mlx5_os.c
+++ b/drivers/net/mlx5/linux/mlx5_os.c
@@ -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 ||
-		     config->mprq.log_stride_num < mprq_min_stride_num_n)) {
-			config->mprq.log_stride_num =
-			       RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM,
-					       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 ||
-		     config->mprq.log_stride_size < mprq_min_stride_size_n)) {
-			config->mprq.log_stride_size =
-			      RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE,
-					      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;
-		config->mprq.log_max_stride_size = mprq_max_stride_size_n;
-	} 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->hca_attr.log_min_stride_wqe_sz :
+					MLX5_MPRQ_LOG_MIN_STRIDE_WQE_SIZE;
+	config->mprq.log_stride_num = MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM;
 	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],
 						 &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. */
 	spawn.max_port = 1;
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 4ba90db816..c01fb9566e 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -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.*/
+		unsigned int log_min_stride_num; /* Log min num of strides. */
+		unsigned int log_max_stride_num; /* Log max num of strides. */
+		unsigned int log_min_stride_wqe_size;
+		/* Log min WQE size, (size of single stride)*(num of strides).*/
 		unsigned int max_memcpy_len;
 		/* Maximum packet size to memcpy Rx packets. */
 		unsigned int min_rxqs_num;
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index e76bfaa000..891ac3d874 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -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;
+	uint32_t log_max_stride_size = config->mprq.log_max_stride_size;
+	uint32_t log_def_stride_size =
+			RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE,
+					log_min_stride_size),
+				log_max_stride_size);
+	uint32_t log_stride_wqe_size;
+
+	if (mlx5_check_mprq_support(dev) != 1 || rx_seg_en)
+		goto unsupport;
+	/* Checks if chosen number of strides is in supported range. */
+	if (config->mprq.log_stride_num > log_max_stride_num ||
+	    config->mprq.log_stride_num < log_min_stride_num) {
+		*actual_log_stride_num = log_def_stride_num;
+		DRV_LOG(WARNING,
+			"Port %u Rx queue %u number of strides for Multi-Packet RQ is out of range, setting default value (%u)",
+			dev->data->port_id, idx, RTE_BIT32(log_def_stride_num));
+	} else {
+		*actual_log_stride_num = config->mprq.log_stride_num;
+	}
+	if (config->mprq.log_stride_size) {
+		/* Checks if chosen size of stride is in supported range. */
+		if (config->mprq.log_stride_size > log_max_stride_size ||
+		    config->mprq.log_stride_size < log_min_stride_size) {
+			*actual_log_stride_size = log_def_stride_size;
+			DRV_LOG(WARNING,
+				"Port %u Rx queue %u size of a stride for Multi-Packet RQ is out of range, setting default value (%u)",
+				dev->data->port_id, idx,
+				RTE_BIT32(log_def_stride_size));
+		} else {
+			*actual_log_stride_size = config->mprq.log_stride_size;
+		}
+	} else {
+		if (min_mbuf_size <= RTE_BIT32(log_max_stride_size))
+			*actual_log_stride_size = log2above(min_mbuf_size);
+		else
+			goto unsupport;
+	}
+	log_stride_wqe_size = *actual_log_stride_num + *actual_log_stride_size;
+	/* Check if WQE buffer size is supported by hardware. */
+	if (log_stride_wqe_size < config->mprq.log_min_stride_wqe_size) {
+		*actual_log_stride_num = log_def_stride_num;
+		*actual_log_stride_size = log_def_stride_size;
+		DRV_LOG(WARNING,
+			"Port %u Rx queue %u size of WQE buffer for Multi-Packet RQ is too small, setting default values (stride_num_n=%u, stride_size_n=%u)",
+			dev->data->port_id, idx, RTE_BIT32(log_def_stride_num),
+			RTE_BIT32(log_def_stride_size));
+		log_stride_wqe_size = log_def_stride_num + log_def_stride_size;
+	}
+	MLX5_ASSERT(log_stride_wqe_size < config->mprq.log_min_stride_wqe_size);
+	if (desc <= RTE_BIT32(*actual_log_stride_num))
+		goto unsupport;
+	if (min_mbuf_size > RTE_BIT32(log_stride_wqe_size)) {
+		DRV_LOG(WARNING, "Port %u Rx queue %u "
+			"Multi-Packet RQ is unsupported, WQE buffer size (%u) "
+			"is smaller than min mbuf size (%u)",
+			dev->data->port_id, idx, RTE_BIT32(log_stride_wqe_size),
+			min_mbuf_size);
+		goto unsupport;
+	}
+	DRV_LOG(DEBUG, "Port %u Rx queue %u "
+		"Multi-Packet RQ is enabled strd_num_n = %u, strd_sz_n = %u",
+		dev->data->port_id, idx, RTE_BIT32(*actual_log_stride_num),
+		RTE_BIT32(*actual_log_stride_size));
+	return 0;
+unsupport:
+	if (config->mprq.enabled)
+		DRV_LOG(WARNING,
+			"Port %u MPRQ is requested but cannot be enabled\n"
+			" (requested: pkt_sz = %u, desc_num = %u,"
+			" rxq_num = %u, stride_sz = %u, stride_num = %u\n"
+			"  supported: min_rxqs_num = %u, min_buf_wqe_sz = %u"
+			" min_stride_sz = %u, max_stride_sz = %u).\n"
+			"Rx segment is %senable.",
+			dev->data->port_id, min_mbuf_size, desc, priv->rxqs_n,
+			RTE_BIT32(config->mprq.log_stride_size),
+			RTE_BIT32(config->mprq.log_stride_num),
+			config->mprq.min_rxqs_num,
+			RTE_BIT32(config->mprq.log_min_stride_wqe_size),
+			RTE_BIT32(config->mprq.log_min_stride_size),
+			RTE_BIT32(config->mprq.log_max_stride_size),
+			rx_seg_en ? "" : "not ");
+	return -1;
+}
+
 /**
  * Create a DPDK Rx queue.
  *
@@ -1565,33 +1685,28 @@ mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 							RTE_PKTMBUF_HEADROOM;
 	unsigned int max_lro_size = 0;
 	unsigned int first_mb_free_size = mb_len - RTE_PKTMBUF_HEADROOM;
-	const int mprq_en = mlx5_check_mprq_support(dev) > 0 && n_seg == 1 &&
-			    !rx_seg[0].offset && !rx_seg[0].length;
-	unsigned int log_mprq_stride_nums = config->mprq.log_stride_num ?
-		config->mprq.log_stride_num : MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM;
-	unsigned int log_mprq_stride_size = non_scatter_min_mbuf_size <=
-		RTE_BIT32(config->mprq.log_max_stride_size) ?
-		log2above(non_scatter_min_mbuf_size) :
-		MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE;
-	unsigned int mprq_stride_cap = (config->mprq.log_stride_num ?
-					RTE_BIT32(config->mprq.log_stride_num) :
-					RTE_BIT32(log_mprq_stride_nums)) *
-				       (config->mprq.log_stride_size ?
-				       RTE_BIT32(config->mprq.log_stride_size) :
-					RTE_BIT32(log_mprq_stride_size));
+	uint32_t mprq_log_actual_stride_num = 0;
+	uint32_t mprq_log_actual_stride_size = 0;
+	bool rx_seg_en = n_seg != 1 || rx_seg[0].offset || rx_seg[0].length;
+	const int mprq_en = !mlx5_mprq_prepare(dev, idx, desc, rx_seg_en,
+					       non_scatter_min_mbuf_size,
+					       &mprq_log_actual_stride_num,
+					       &mprq_log_actual_stride_size);
 	/*
 	 * Always allocate extra slots, even if eventually
 	 * the vector Rx will not be used.
 	 */
 	uint16_t desc_n = desc + config->rx_vec_en * MLX5_VPMD_DESCS_PER_LOOP;
+	size_t alloc_size = sizeof(*tmpl) + desc_n * sizeof(struct rte_mbuf *);
 	const struct rte_eth_rxseg_split *qs_seg = rx_seg;
 	unsigned int tail_len;
 
-	tmpl = mlx5_malloc(MLX5_MEM_RTE | MLX5_MEM_ZERO,
-		sizeof(*tmpl) + desc_n * sizeof(struct rte_mbuf *) +
-		(!!mprq_en) *
-		(desc >> log_mprq_stride_nums) * sizeof(struct mlx5_mprq_buf *),
-		0, socket);
+	if (mprq_en) {
+		/* Trim the number of descs needed. */
+		desc >>= mprq_log_actual_stride_num;
+		alloc_size += desc * sizeof(struct mlx5_mprq_buf *);
+	}
+	tmpl = mlx5_malloc(MLX5_MEM_RTE | MLX5_MEM_ZERO, alloc_size, 0, socket);
 	if (!tmpl) {
 		rte_errno = ENOMEM;
 		return NULL;
@@ -1689,30 +1804,11 @@ mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 	tmpl->socket = socket;
 	if (dev->data->dev_conf.intr_conf.rxq)
 		tmpl->irq = 1;
-	/*
-	 * This Rx queue can be configured as a Multi-Packet RQ if all of the
-	 * following conditions are met:
-	 *  - MPRQ is enabled.
-	 *  - The number of descs is more than the number of strides.
-	 *  - max_rx_pktlen plus overhead is less than the max size
-	 *    of a stride or log_mprq_stride_size is specified by a user.
-	 *    Need to make sure that there are enough strides to encap
-	 *    the maximum packet size in case log_mprq_stride_size is set.
-	 *  Otherwise, enable Rx scatter if necessary.
-	 */
-	if (mprq_en && desc > RTE_BIT32(log_mprq_stride_nums) &&
-	    (non_scatter_min_mbuf_size <=
-	     RTE_BIT32(config->mprq.log_max_stride_size) ||
-	     (config->mprq.log_stride_size &&
-	      non_scatter_min_mbuf_size <= mprq_stride_cap))) {
+	if (mprq_en) {
 		/* TODO: Rx scatter isn't supported yet. */
 		tmpl->rxq.sges_n = 0;
-		/* Trim the number of descs needed. */
-		desc >>= log_mprq_stride_nums;
-		tmpl->rxq.log_strd_num = config->mprq.log_stride_num ?
-			config->mprq.log_stride_num : log_mprq_stride_nums;
-		tmpl->rxq.log_strd_sz = config->mprq.log_stride_size ?
-			config->mprq.log_stride_size : log_mprq_stride_size;
+		tmpl->rxq.log_strd_num = mprq_log_actual_stride_num;
+		tmpl->rxq.log_strd_sz = mprq_log_actual_stride_size;
 		tmpl->rxq.strd_shift_en = MLX5_MPRQ_TWO_BYTE_SHIFT;
 		tmpl->rxq.strd_scatter_en =
 				!!(offloads & RTE_ETH_RX_OFFLOAD_SCATTER);
@@ -1721,11 +1817,6 @@ mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 		max_lro_size = RTE_MIN(max_rx_pktlen,
 				       RTE_BIT32(tmpl->rxq.log_strd_num) *
 				       RTE_BIT32(tmpl->rxq.log_strd_sz));
-		DRV_LOG(DEBUG,
-			"port %u Rx queue %u: Multi-Packet RQ is enabled"
-			" strd_num_n = %u, strd_sz_n = %u",
-			dev->data->port_id, idx,
-			tmpl->rxq.log_strd_num, tmpl->rxq.log_strd_sz);
 	} else if (tmpl->rxq.rxseg_n == 1) {
 		MLX5_ASSERT(max_rx_pktlen <= first_mb_free_size);
 		tmpl->rxq.sges_n = 0;
@@ -1759,24 +1850,6 @@ mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 		tmpl->rxq.sges_n = sges_n;
 		max_lro_size = max_rx_pktlen;
 	}
-	if (config->mprq.enabled && !mlx5_rxq_mprq_enabled(&tmpl->rxq))
-		DRV_LOG(WARNING,
-			"port %u MPRQ is requested but cannot be enabled\n"
-			" (requested: pkt_sz = %u, desc_num = %u,"
-			" rxq_num = %u, stride_sz = %u, stride_num = %u\n"
-			"  supported: min_rxqs_num = %u,"
-			" min_stride_sz = %u, max_stride_sz = %u).",
-			dev->data->port_id, non_scatter_min_mbuf_size,
-			desc, priv->rxqs_n,
-			config->mprq.log_stride_size ?
-				RTE_BIT32(config->mprq.log_stride_size) :
-				RTE_BIT32(log_mprq_stride_size),
-			config->mprq.log_stride_num ?
-				RTE_BIT32(config->mprq.log_stride_num) :
-				RTE_BIT32(log_mprq_stride_nums),
-			config->mprq.min_rxqs_num,
-			RTE_BIT32(config->mprq.log_min_stride_size),
-			RTE_BIT32(config->mprq.log_max_stride_size));
 	DRV_LOG(DEBUG, "port %u maximum number of segments per packet: %u",
 		dev->data->port_id, 1 << tmpl->rxq.sges_n);
 	if (desc % (1 << tmpl->rxq.sges_n)) {
@@ -1834,17 +1907,15 @@ mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 		dev->data->port_id,
 		tmpl->rxq.crc_present ? "disabled" : "enabled",
 		tmpl->rxq.crc_present << 2);
-	/* Save port ID. */
 	tmpl->rxq.rss_hash = !!priv->rss_conf.rss_hf &&
 		(!!(dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_RSS));
+	/* Save port ID. */
 	tmpl->rxq.port_id = dev->data->port_id;
 	tmpl->sh = priv->sh;
 	tmpl->rxq.mp = rx_seg[0].mp;
 	tmpl->rxq.elts_n = log2above(desc);
-	tmpl->rxq.rq_repl_thresh =
-		MLX5_VPMD_RXQ_RPLNSH_THRESH(desc_n);
-	tmpl->rxq.elts =
-		(struct rte_mbuf *(*)[desc_n])(tmpl + 1);
+	tmpl->rxq.rq_repl_thresh = MLX5_VPMD_RXQ_RPLNSH_THRESH(desc_n);
+	tmpl->rxq.elts = (struct rte_mbuf *(*)[desc_n])(tmpl + 1);
 	tmpl->rxq.mprq_bufs =
 		(struct mlx5_mprq_buf *(*)[desc])(*tmpl->rxq.elts + desc_n);
 	tmpl->rxq.idx = idx;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs
  2021-11-23 18:38 ` [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs michaelba
@ 2021-11-23 20:41   ` Raslan Darawsheh
  2021-12-07 13:40   ` Ferruh Yigit
  1 sibling, 0 replies; 14+ messages in thread
From: Raslan Darawsheh @ 2021-11-23 20:41 UTC (permalink / raw)
  To: Michael Baum, dev; +Cc: Matan Azrad, Slava Ovsiienko, stable

Hi,

> -----Original Message-----
> From: Michael Baum <michaelba@nvidia.com>
> Sent: Tuesday, November 23, 2021 8:38 PM
> To: dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Raslan Darawsheh
> <rasland@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Michael
> Baum <michaelba@nvidia.com>; stable@dpdk.org
> Subject: [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs
> 
> From: Michael Baum <michaelba@nvidia.com>
> 
> 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: stable@dpdk.org
> 
> Signed-off-by: Michael Baum <michaelba@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>
> ---
>  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(-)
> 
> diff --git a/drivers/net/mlx5/linux/mlx5_os.c
> b/drivers/net/mlx5/linux/mlx5_os.c
> index 70472efc29..3e496d68ea 100644
> --- a/drivers/net/mlx5/linux/mlx5_os.c
> +++ b/drivers/net/mlx5/linux/mlx5_os.c
> @@ -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 ||
> -		     config->mprq.log_stride_num <
> mprq_min_stride_num_n)) {
> -			config->mprq.log_stride_num =
> -
> RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM,
> -					       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
> ||
> -		     config->mprq.log_stride_size < mprq_min_stride_size_n))
> {
> -			config->mprq.log_stride_size =
> -
> RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE,
> -					      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;
> -		config->mprq.log_max_stride_size =
> mprq_max_stride_size_n;
> -	} 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-
> >hca_attr.log_min_stride_wqe_sz :
> +
> 	MLX5_MPRQ_LOG_MIN_STRIDE_WQE_SIZE;
> +	config->mprq.log_stride_num =
> MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM;
>  	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],
>  						 &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. */
>  	spawn.max_port = 1;
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> 4ba90db816..c01fb9566e 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -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.*/
> +		unsigned int log_min_stride_num; /* Log min num of strides.
> */
> +		unsigned int log_max_stride_num; /* Log max num of
> strides. */
> +		unsigned int log_min_stride_wqe_size;
> +		/* Log min WQE size, (size of single stride)*(num of
> strides).*/
>  		unsigned int max_memcpy_len;
>  		/* Maximum packet size to memcpy Rx packets. */
>  		unsigned int min_rxqs_num;
> diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
> index e76bfaa000..891ac3d874 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -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;
> +	uint32_t log_max_stride_size = config->mprq.log_max_stride_size;
> +	uint32_t log_def_stride_size =
> +
> 	RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE,
> +					log_min_stride_size),
> +				log_max_stride_size);
> +	uint32_t log_stride_wqe_size;
> +
> +	if (mlx5_check_mprq_support(dev) != 1 || rx_seg_en)
> +		goto unsupport;
typo in this label name, will fix during integration.
[..]
Kindest regards
Raslan Darawsheh

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] common/mlx5: add min WQE size for striding RQ
  2021-11-23 18:38 ` [PATCH 1/3] common/mlx5: add min WQE size for striding RQ michaelba
@ 2021-12-07 13:32   ` Ferruh Yigit
  2021-12-08 12:52     ` Michael Baum
  0 siblings, 1 reply; 14+ messages in thread
From: Ferruh Yigit @ 2021-12-07 13:32 UTC (permalink / raw)
  To: michaelba, dev
  Cc: Matan Azrad, Raslan Darawsheh, Viacheslav Ovsiienko, stable

On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
> From: Michael Baum<michaelba@nvidia.com>
> 
> 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/

> 
> Cc:stable@dpdk.org
> 

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

> Signed-off-by: Michael Baum<michaelba@nvidia.com>
> Acked-by: Matan Azrad<matan@nvidia.com>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] net/mlx5: improve stride parameter names
  2021-11-23 18:38 ` [PATCH 2/3] net/mlx5: improve stride parameter names michaelba
@ 2021-12-07 13:33   ` Ferruh Yigit
  2021-12-08 12:52     ` Michael Baum
  0 siblings, 1 reply; 14+ messages in thread
From: Ferruh Yigit @ 2021-12-07 13:33 UTC (permalink / raw)
  To: michaelba, dev
  Cc: Matan Azrad, Raslan Darawsheh, Viacheslav Ovsiienko, stable

On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
> From: Michael Baum<michaelba@nvidia.com>
> 
> 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:stable@dpdk.org
> 

Can you please provide fixes tag for it?
In this case commit(s) adding the parameters with bad name.

> Signed-off-by: Michael Baum<michaelba@nvidia.com>
> Acked-by: Matan Azrad<matan@nvidia.com>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs
  2021-11-23 18:38 ` [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs michaelba
  2021-11-23 20:41   ` Raslan Darawsheh
@ 2021-12-07 13:40   ` Ferruh Yigit
  2021-12-08 12:52     ` Michael Baum
  1 sibling, 1 reply; 14+ messages in thread
From: Ferruh Yigit @ 2021-12-07 13:40 UTC (permalink / raw)
  To: michaelba, dev
  Cc: Matan Azrad, Raslan Darawsheh, Viacheslav Ovsiienko, stable

On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
> From: Michael Baum<michaelba@nvidia.com>
> 
> In Multy-Packet RQ creation, the user can choose the number of strides

Multi-Packet ?

> 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:stable@dpdk.org
> 

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.

> Signed-off-by: Michael Baum<michaelba@nvidia.com>
> Acked-by: Matan Azrad<matan@nvidia.com>
> ---


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH 1/3] common/mlx5: add min WQE size for striding RQ
  2021-12-07 13:32   ` Ferruh Yigit
@ 2021-12-08 12:52     ` Michael Baum
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Baum @ 2021-12-08 12:52 UTC (permalink / raw)
  To: Ferruh Yigit, dev; +Cc: Matan Azrad, Raslan Darawsheh, Slava Ovsiienko, stable


On 12/07/2021 3:32 PM, ferruh.yigit@intel.com wrote: 
> 
> On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
> > From: Michael Baum<michaelba@nvidia.com>
> >
> > 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/

Thanks for the comment.
Let's replace it to "the minimum required size of WQE buffer for striding RQ in granularity of Bytes".

> 
> >
> > Cc:stable@dpdk.org
> >
> 
> This is not a fix, why requesting to backport it?
> Patch is to use FW provided capability value, which is not used before.

It is requesting for fix coming after (net/mlx5: fix missing adjustment MPRQ stride devargs).
The fix use this capability.

> 
> > Signed-off-by: Michael Baum<michaelba@nvidia.com>
> > Acked-by: Matan Azrad<matan@nvidia.com>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH 2/3] net/mlx5: improve stride parameter names
  2021-12-07 13:33   ` Ferruh Yigit
@ 2021-12-08 12:52     ` Michael Baum
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Baum @ 2021-12-08 12:52 UTC (permalink / raw)
  To: Ferruh Yigit, dev; +Cc: Matan Azrad, Raslan Darawsheh, Slava Ovsiienko, stable

On 12/07/2021 3:34 PM, ferruh.yigit@intel.com wrote: 
> 
> On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
> > From: Michael Baum<michaelba@nvidia.com>
> >
> > 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:stable@dpdk.org
> >
> 
> Can you please provide fixes tag for it?

I think this is the relevant fixes tag:
Fixes: ecb160456aed ("net/mlx5: add device parameter for MPRQ stride size")

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

I added Cc:stable here just to make next commit easy for the backport, because following patch is based on it.
For this commit itself there is no reason backporting it ,because it does not solve something that affects the user but prevents confusion from future programmers.

> 
> > Signed-off-by: Michael Baum<michaelba@nvidia.com>
> > Acked-by: Matan Azrad<matan@nvidia.com>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs
  2021-12-07 13:40   ` Ferruh Yigit
@ 2021-12-08 12:52     ` Michael Baum
  2021-12-08 14:00       ` Ferruh Yigit
  0 siblings, 1 reply; 14+ messages in thread
From: Michael Baum @ 2021-12-08 12:52 UTC (permalink / raw)
  To: Ferruh Yigit, dev; +Cc: Matan Azrad, Raslan Darawsheh, Slava Ovsiienko, stable


On 12/07/2021 3:41 PM, ferruh.yigit@intel.com wrote: 
> 
> On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
> > From: Michael Baum<michaelba@nvidia.com>
> >
> > 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.

> 
> > 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:stable@dpdk.org
> >
> 
> 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.

> 
> 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)

> 
> But I think it is wrong to backport the behavior change.
> 
> > Signed-off-by: Michael Baum<michaelba@nvidia.com>
> > Acked-by: Matan Azrad<matan@nvidia.com>
> > ---


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs
  2021-12-08 12:52     ` Michael Baum
@ 2021-12-08 14:00       ` Ferruh Yigit
  2021-12-08 15:40         ` Matan Azrad
  0 siblings, 1 reply; 14+ messages in thread
From: Ferruh Yigit @ 2021-12-08 14:00 UTC (permalink / raw)
  To: Michael Baum, dev, Luca Boccassi, Kevin Traynor, Christian Ehrhardt
  Cc: Matan Azrad, Raslan Darawsheh, Slava Ovsiienko, stable

On 12/8/2021 12:52 PM, Michael Baum wrote:
> 
> On 12/07/2021 3:41 PM, ferruh.yigit@intel.com wrote:
>>
>> On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
>>> From: Michael Baum<michaelba@nvidia.com>
>>>
>>> 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.
> 
>>
>>> 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:stable@dpdk.org
>>>
>>
>> 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.
> 
>>
>> 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)
> 

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.


>>
>> But I think it is wrong to backport the behavior change.
>>
>>> Signed-off-by: Michael Baum<michaelba@nvidia.com>
>>> Acked-by: Matan Azrad<matan@nvidia.com>
>>> ---
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs
  2021-12-08 14:00       ` Ferruh Yigit
@ 2021-12-08 15:40         ` Matan Azrad
  2021-12-09 12:33           ` Kevin Traynor
  0 siblings, 1 reply; 14+ messages in thread
From: Matan Azrad @ 2021-12-08 15:40 UTC (permalink / raw)
  To: Ferruh Yigit, Michael Baum, dev, Luca Boccassi, Kevin Traynor,
	Christian Ehrhardt
  Cc: Raslan Darawsheh, Slava Ovsiienko, stable

Hi Ferruh

Thanks for the review.

Please see inside some clarifications.

From: Ferruh Yigit
> On 12/8/2021 12:52 PM, Michael Baum wrote:
> >
> > On 12/07/2021 3:41 PM, ferruh.yigit@intel.com wrote:
> >>
> >> On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
> >>> From: Michael Baum<michaelba@nvidia.com>
> >>>
> >>> 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.
> >
> >>
> >>> 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:stable@dpdk.org
> >>>
> >>
> >> 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.
> >
> >>
> >> 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)
> >
> 
> 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.

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

> 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.
> 
> 
> >>
> >> But I think it is wrong to backport the behavior change.
> >>
> >>> Signed-off-by: Michael Baum<michaelba@nvidia.com>
> >>> Acked-by: Matan Azrad<matan@nvidia.com>
> >>> ---
> >


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs
  2021-12-08 15:40         ` Matan Azrad
@ 2021-12-09 12:33           ` Kevin Traynor
  2021-12-10 16:58             ` Ferruh Yigit
  0 siblings, 1 reply; 14+ messages in thread
From: Kevin Traynor @ 2021-12-09 12:33 UTC (permalink / raw)
  To: Matan Azrad, Ferruh Yigit, Michael Baum, dev, Luca Boccassi,
	Christian Ehrhardt
  Cc: Raslan Darawsheh, Slava Ovsiienko, stable

On 08/12/2021 15:40, Matan Azrad wrote:
> Hi Ferruh
> 
> Thanks for the review.
> 
> Please see inside some clarifications.
> 
> From: Ferruh Yigit
>> On 12/8/2021 12:52 PM, Michael Baum wrote:
>>>
>>> On 12/07/2021 3:41 PM, ferruh.yigit@intel.com wrote:
>>>>
>>>> On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
>>>>> From: Michael Baum<michaelba@nvidia.com>
>>>>>
>>>>> 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.
>>>
>>>>
>>>>> 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:stable@dpdk.org
>>>>>
>>>>
>>>> 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.
>>>
>>>>
>>>> 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)
>>>
>>
>> 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.
> 
>> 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.

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

Kevin.

>>
>>>>
>>>> But I think it is wrong to backport the behavior change.
>>>>
>>>>> Signed-off-by: Michael Baum<michaelba@nvidia.com>
>>>>> Acked-by: Matan Azrad<matan@nvidia.com>
>>>>> ---
>>>
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs
  2021-12-09 12:33           ` Kevin Traynor
@ 2021-12-10 16:58             ` Ferruh Yigit
  0 siblings, 0 replies; 14+ messages in thread
From: Ferruh Yigit @ 2021-12-10 16:58 UTC (permalink / raw)
  To: Kevin Traynor, Matan Azrad, Michael Baum, dev, Luca Boccassi,
	Christian Ehrhardt
  Cc: Raslan Darawsheh, Slava Ovsiienko, stable

On 12/9/2021 12:33 PM, Kevin Traynor wrote:
> On 08/12/2021 15:40, Matan Azrad wrote:
>> Hi Ferruh
>>
>> Thanks for the review.
>>
>> Please see inside some clarifications.
>>
>> From: Ferruh Yigit
>>> On 12/8/2021 12:52 PM, Michael Baum wrote:
>>>>
>>>> On 12/07/2021 3:41 PM, ferruh.yigit@intel.com wrote:
>>>>>
>>>>> On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
>>>>>> From: Michael Baum<michaelba@nvidia.com>
>>>>>>
>>>>>> 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.
>>>>
>>>>>
>>>>>> 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:stable@dpdk.org
>>>>>>
>>>>>
>>>>> 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.
>>>>
>>>>>
>>>>> 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)
>>>>
>>>
>>> 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.
>>
>>> 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.
> 
>>> 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.
> 


Proceeding with patch then, updated fixes tag for 2/3 in next-net.


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-12-10 16:58 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20211123183805.2905792-1-michaelba@nvidia.com>
2021-11-23 18:38 ` [PATCH 1/3] common/mlx5: add min WQE size for striding RQ michaelba
2021-12-07 13:32   ` Ferruh Yigit
2021-12-08 12:52     ` Michael Baum
2021-11-23 18:38 ` [PATCH 2/3] net/mlx5: improve stride parameter names michaelba
2021-12-07 13:33   ` Ferruh Yigit
2021-12-08 12:52     ` Michael Baum
2021-11-23 18:38 ` [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs michaelba
2021-11-23 20:41   ` Raslan Darawsheh
2021-12-07 13:40   ` Ferruh Yigit
2021-12-08 12:52     ` Michael Baum
2021-12-08 14:00       ` Ferruh Yigit
2021-12-08 15:40         ` Matan Azrad
2021-12-09 12:33           ` Kevin Traynor
2021-12-10 16:58             ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).