[PATCH 0/5] Make RDMA RW API SGE limit configurable

[PATCH 0/5] Make RDMA RW API SGE limit configurable

[Bart Van Assche]

Hello Doug,

The five patches in this series modify the RDMA RW API slightly. This is 
needed to avoid that the SRP and iSER target drivers submit RDMA 
requests with an SGE list that exceeds the queue pair limits. The 
ib_srpt changes in this series have been tested but the ib_isert changes 
not yet. The individual patches in this series are:

0001-IB-core-Make-rdma_rw_ctx_init-initialize-all-used-fi.patch
0002-IB-core-Add-max_sge-argument-to-rdma_rw_ctx_init.patch
0003-IB-isert-Limit-the-number-of-SG-elements-per-work-re.patch
0004-IB-srpt-Limit-the-number-of-SG-elements-per-work-req.patch
0005-IB-srpt-Simplify-srpt_queue_response.patch

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] IB/core: Make rdma_rw_ctx_init() initialize all used fields

[Bart Van Assche]

Some but not all callers of rdma_rw_ctx_init() zero-initialize
struct rdma_rw_ctx. Hence make rdma_rw_ctx_init() initialize all
work request fields that will be read by ib_post_send().

Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #v4.7+
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
Cc: Parav Pandit <pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/rw.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
index 1eb9b12..13d4067 100644
--- a/drivers/infiniband/core/rw.c
+++ b/drivers/infiniband/core/rw.c
@@ -99,6 +99,7 @@ static int rdma_rw_init_one_mr(struct ib_qp *qp, u8 port_num,
 	}
 
 	reg->reg_wr.wr.opcode = IB_WR_REG_MR;
+	reg->reg_wr.wr.next = NULL;
 	reg->reg_wr.mr = reg->mr;
 	reg->reg_wr.access = IB_ACCESS_LOCAL_WRITE;
 	if (rdma_protocol_iwarp(qp->device, port_num))
@@ -114,6 +115,7 @@ static int rdma_rw_init_mr_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
 		u8 port_num, struct scatterlist *sg, u32 sg_cnt, u32 offset,
 		u64 remote_addr, u32 rkey, enum dma_data_direction dir)
 {
+	struct rdma_rw_reg_ctx *prev = NULL;
 	u32 pages_per_mr = rdma_rw_fr_page_list_len(qp->pd->device);
 	int i, j, ret = 0, count = 0;
 
@@ -125,7 +127,6 @@ static int rdma_rw_init_mr_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
 	}
 
 	for (i = 0; i < ctx->nr_ops; i++) {
-		struct rdma_rw_reg_ctx *prev = i ? &ctx->reg[i - 1] : NULL;
 		struct rdma_rw_reg_ctx *reg = &ctx->reg[i];
 		u32 nents = min(sg_cnt, pages_per_mr);
 
@@ -162,9 +163,13 @@ static int rdma_rw_init_mr_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
 		sg_cnt -= nents;
 		for (j = 0; j < nents; j++)
 			sg = sg_next(sg);
+		prev = reg;
 		offset = 0;
 	}
 
+	if (prev)
+		prev->wr.wr.next = NULL;
+
 	ctx->type = RDMA_RW_MR;
 	return count;
 
@@ -205,11 +210,10 @@ static int rdma_rw_init_map_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
 			rdma_wr->wr.opcode = IB_WR_RDMA_READ;
 		rdma_wr->remote_addr = remote_addr + total_len;
 		rdma_wr->rkey = rkey;
+		rdma_wr->wr.num_sge = nr_sge;
 		rdma_wr->wr.sg_list = sge;
 
 		for (j = 0; j < nr_sge; j++, sg = sg_next(sg)) {
-			rdma_wr->wr.num_sge++;
-
 			sge->addr = ib_sg_dma_address(dev, sg) + offset;
 			sge->length = ib_sg_dma_len(dev, sg) - offset;
 			sge->lkey = qp->pd->local_dma_lkey;
@@ -220,8 +224,8 @@ static int rdma_rw_init_map_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
 			offset = 0;
 		}
 
-		if (i + 1 < ctx->nr_ops)
-			rdma_wr->wr.next = &ctx->map.wrs[i + 1].wr;
+		rdma_wr->wr.next = i + 1 < ctx->nr_ops ?
+			&ctx->map.wrs[i + 1].wr : NULL;
 	}
 
 	ctx->type = RDMA_RW_MULTI_WR;
-- 
2.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] IB/core: Add max_sge argument to rdma_rw_ctx_init()

[Bart Van Assche]

The SGE limit for a queue pair is typically lower than what is
defined by the HCA limits. Hence make max_sge an argument.

Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #v4.7+
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
Cc: Parav Pandit <pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/rw.c            | 17 ++++++-----------
 drivers/infiniband/ulp/isert/ib_isert.c |  7 +++++--
 drivers/infiniband/ulp/srpt/ib_srpt.c   |  8 +++++++-
 include/rdma/rw.h                       |  3 ++-
 4 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
index 13d4067..3894f9e 100644
--- a/drivers/infiniband/core/rw.c
+++ b/drivers/infiniband/core/rw.c
@@ -58,13 +58,6 @@ static inline bool rdma_rw_io_needs_mr(struct ib_device *dev, u8 port_num,
 	return false;
 }
 
-static inline u32 rdma_rw_max_sge(struct ib_device *dev,
-		enum dma_data_direction dir)
-{
-	return dir == DMA_TO_DEVICE ?
-		dev->attrs.max_sge : dev->attrs.max_sge_rd;
-}
-
 static inline u32 rdma_rw_fr_page_list_len(struct ib_device *dev)
 {
 	/* arbitrary limit to avoid allocating gigantic resources */
@@ -183,10 +176,10 @@ out:
 
 static int rdma_rw_init_map_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
 		struct scatterlist *sg, u32 sg_cnt, u32 offset,
-		u64 remote_addr, u32 rkey, enum dma_data_direction dir)
+		u64 remote_addr, u32 rkey, enum dma_data_direction dir,
+		u32 max_sge)
 {
 	struct ib_device *dev = qp->pd->device;
-	u32 max_sge = rdma_rw_max_sge(dev, dir);
 	struct ib_sge *sge;
 	u32 total_len = 0, i, j;
 
@@ -275,13 +268,15 @@ static int rdma_rw_init_single_wr(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
  * @remote_addr:remote address to read/write (relative to @rkey)
  * @rkey:	remote key to operate on
  * @dir:	%DMA_TO_DEVICE for RDMA WRITE, %DMA_FROM_DEVICE for RDMA READ
+ * @max_sge:    maximum number of SG elements per work request
  *
  * Returns the number of WQEs that will be needed on the workqueue if
  * successful, or a negative error code.
  */
 int rdma_rw_ctx_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num,
 		struct scatterlist *sg, u32 sg_cnt, u32 sg_offset,
-		u64 remote_addr, u32 rkey, enum dma_data_direction dir)
+		u64 remote_addr, u32 rkey, enum dma_data_direction dir,
+		u32 max_sge)
 {
 	struct ib_device *dev = qp->pd->device;
 	int ret;
@@ -314,7 +309,7 @@ int rdma_rw_ctx_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num,
 				sg_offset, remote_addr, rkey, dir);
 	} else if (sg_cnt > 1) {
 		ret = rdma_rw_init_map_wrs(ctx, qp, sg, sg_cnt, sg_offset,
-				remote_addr, rkey, dir);
+				remote_addr, rkey, dir, max_sge);
 	} else {
 		ret = rdma_rw_init_single_wr(ctx, qp, sg, sg_offset,
 				remote_addr, rkey, dir);
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index a990c04..06a5671 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -2075,21 +2075,24 @@ static int
 isert_rdma_rw_ctx_post(struct isert_cmd *cmd, struct isert_conn *conn,
 		struct ib_cqe *cqe, struct ib_send_wr *chain_wr)
 {
+	struct ib_device *dev = conn->device->ib_device;
 	struct se_cmd *se_cmd = &cmd->iscsi_cmd->se_cmd;
 	enum dma_data_direction dir = target_reverse_dma_direction(se_cmd);
 	u8 port_num = conn->cm_id->port_num;
 	u64 addr;
-	u32 rkey, offset;
+	u32 rkey, offset, max_sge;
 	int ret;
 
 	if (dir == DMA_FROM_DEVICE) {
 		addr = cmd->write_va;
 		rkey = cmd->write_stag;
 		offset = cmd->iscsi_cmd->write_data_done;
+		max_sge = dev->attrs.max_sge_rd;
 	} else {
 		addr = cmd->read_va;
 		rkey = cmd->read_stag;
 		offset = 0;
+		max_sge = dev->attrs.max_sge;
 	}
 
 	if (isert_prot_cmd(conn, se_cmd)) {
@@ -2107,7 +2110,7 @@ isert_rdma_rw_ctx_post(struct isert_cmd *cmd, struct isert_conn *conn,
 	} else {
 		ret = rdma_rw_ctx_init(&cmd->rw, conn->qp, port_num,
 				se_cmd->t_data_sg, se_cmd->t_data_nents,
-				offset, addr, rkey, dir);
+				offset, addr, rkey, dir, max_sge);
 	}
 	if (ret < 0) {
 		isert_err("Cmd: %p failed to prepare RDMA res\n", cmd);
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 4a41556..2ed65f5 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -803,7 +803,9 @@ static int srpt_alloc_rw_ctxs(struct srpt_send_ioctx *ioctx,
 {
 	enum dma_data_direction dir = target_reverse_dma_direction(&ioctx->cmd);
 	struct srpt_rdma_ch *ch = ioctx->ch;
+	struct ib_device *dev = ch->qp->pd->device;
 	struct scatterlist *prev = NULL;
+	u32 max_sge;
 	unsigned prev_nents;
 	int ret, i;
 
@@ -816,6 +818,9 @@ static int srpt_alloc_rw_ctxs(struct srpt_send_ioctx *ioctx,
 			return -ENOMEM;
 	}
 
+	max_sge = dir == DMA_TO_DEVICE ? dev->attrs.max_sge :
+		dev->attrs.max_sge_rd;
+
 	for (i = ioctx->n_rw_ctx; i < nbufs; i++, db++) {
 		struct srpt_rw_ctx *ctx = &ioctx->rw_ctxs[i];
 		u64 remote_addr = be64_to_cpu(db->va);
@@ -828,7 +833,8 @@ static int srpt_alloc_rw_ctxs(struct srpt_send_ioctx *ioctx,
 			goto unwind;
 
 		ret = rdma_rw_ctx_init(&ctx->rw, ch->qp, ch->sport->port,
-				ctx->sg, ctx->nents, 0, remote_addr, rkey, dir);
+				ctx->sg, ctx->nents, 0, remote_addr, rkey, dir,
+				max_sge);
 		if (ret < 0) {
 			target_free_sgl(ctx->sg, ctx->nents);
 			goto unwind;
diff --git a/include/rdma/rw.h b/include/rdma/rw.h
index 377d865..590a8aec 100644
--- a/include/rdma/rw.h
+++ b/include/rdma/rw.h
@@ -61,7 +61,8 @@ struct rdma_rw_ctx {
 
 int rdma_rw_ctx_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num,
 		struct scatterlist *sg, u32 sg_cnt, u32 sg_offset,
-		u64 remote_addr, u32 rkey, enum dma_data_direction dir);
+		u64 remote_addr, u32 rkey, enum dma_data_direction dir,
+		u32 max_sge);
 void rdma_rw_ctx_destroy(struct rdma_rw_ctx *ctx, struct ib_qp *qp, u8 port_num,
 		struct scatterlist *sg, u32 sg_cnt,
 		enum dma_data_direction dir);
-- 
2.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] IB/isert: Limit the number of SG elements per work request

[Bart Van Assche]

Limit the number of SG elements per work request to what the queue
pair supports.

Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #v4.7+
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
Cc: Parav Pandit <pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 10 +++++-----
 drivers/infiniband/ulp/isert/ib_isert.h |  3 ++-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 06a5671..b5d6556 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -137,8 +137,6 @@ isert_create_qp(struct isert_conn *isert_conn,
 	attr.cap.max_recv_wr = ISERT_QP_MAX_RECV_DTOS + 1;
 	attr.cap.max_rdma_ctxs = ISCSI_DEF_XMIT_CMDS_MAX;
 	attr.cap.max_send_sge = device->ib_device->attrs.max_sge;
-	isert_conn->max_sge = min(device->ib_device->attrs.max_sge,
-				  device->ib_device->attrs.max_sge_rd);
 	attr.cap.max_recv_sge = 1;
 	attr.sq_sig_type = IB_SIGNAL_REQ_WR;
 	attr.qp_type = IB_QPT_RC;
@@ -151,6 +149,9 @@ isert_create_qp(struct isert_conn *isert_conn,
 		return ERR_PTR(ret);
 	}
 
+	isert_conn->max_send_sge = attr.cap.max_send_sge;
+	isert_conn->max_recv_sge = attr.cap.max_recv_sge;
+
 	return cma_id->qp;
 }
 
@@ -2075,7 +2076,6 @@ static int
 isert_rdma_rw_ctx_post(struct isert_cmd *cmd, struct isert_conn *conn,
 		struct ib_cqe *cqe, struct ib_send_wr *chain_wr)
 {
-	struct ib_device *dev = conn->device->ib_device;
 	struct se_cmd *se_cmd = &cmd->iscsi_cmd->se_cmd;
 	enum dma_data_direction dir = target_reverse_dma_direction(se_cmd);
 	u8 port_num = conn->cm_id->port_num;
@@ -2087,12 +2087,12 @@ isert_rdma_rw_ctx_post(struct isert_cmd *cmd, struct isert_conn *conn,
 		addr = cmd->write_va;
 		rkey = cmd->write_stag;
 		offset = cmd->iscsi_cmd->write_data_done;
-		max_sge = dev->attrs.max_sge_rd;
+		max_sge = conn->max_recv_sge;
 	} else {
 		addr = cmd->read_va;
 		rkey = cmd->read_stag;
 		offset = 0;
-		max_sge = dev->attrs.max_sge;
+		max_sge = conn->max_send_sge;
 	}
 
 	if (isert_prot_cmd(conn, se_cmd)) {
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
index e512ba9..ccd6927 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -138,7 +138,8 @@ struct isert_conn {
 	u32			responder_resources;
 	u32			initiator_depth;
 	bool			pi_support;
-	u32			max_sge;
+	u32			max_send_sge;
+	u32			max_recv_sge;
 	struct iser_rx_desc	*login_req_buf;
 	char			*login_rsp_buf;
 	u64			login_req_dma;
-- 
2.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] IB/srpt: Limit the number of SG elements per work request

[Bart Van Assche]

Limit the number of SG elements per work request to what the HCA
and the queue pair support.

Fixes: 34693573fde0 ("IB/srpt: Reduce QP buffer size")
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #v4.7+
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
Cc: Parav Pandit <pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 17 +++++++++++------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  6 +++++-
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 2ed65f5..6314d10 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -803,7 +803,6 @@ static int srpt_alloc_rw_ctxs(struct srpt_send_ioctx *ioctx,
 {
 	enum dma_data_direction dir = target_reverse_dma_direction(&ioctx->cmd);
 	struct srpt_rdma_ch *ch = ioctx->ch;
-	struct ib_device *dev = ch->qp->pd->device;
 	struct scatterlist *prev = NULL;
 	u32 max_sge;
 	unsigned prev_nents;
@@ -818,8 +817,7 @@ static int srpt_alloc_rw_ctxs(struct srpt_send_ioctx *ioctx,
 			return -ENOMEM;
 	}
 
-	max_sge = dir == DMA_TO_DEVICE ? dev->attrs.max_sge :
-		dev->attrs.max_sge_rd;
+	max_sge = dir == DMA_TO_DEVICE ? ch->max_send_sge : ch->max_recv_sge;
 
 	for (i = ioctx->n_rw_ctx; i < nbufs; i++, db++) {
 		struct srpt_rw_ctx *ctx = &ioctx->rw_ctxs[i];
@@ -1607,6 +1605,7 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch)
 	struct ib_qp_init_attr *qp_init;
 	struct srpt_port *sport = ch->sport;
 	struct srpt_device *sdev = sport->sdev;
+	const struct ib_device_attr *attrs = &sdev->device->attrs;
 	u32 srp_sq_size = sport->port_attrib.srp_sq_size;
 	int ret;
 
@@ -1644,7 +1643,7 @@ retry:
 	 */
 	qp_init->cap.max_send_wr = srp_sq_size / 2;
 	qp_init->cap.max_rdma_ctxs = srp_sq_size / 2;
-	qp_init->cap.max_send_sge = SRPT_DEF_SG_PER_WQE;
+	qp_init->cap.max_send_sge = min(attrs->max_sge, SRPT_MAX_SG_PER_WQE);
 	qp_init->port_num = ch->sport->port;
 
 	ch->qp = ib_create_qp(sdev->pd, qp_init);
@@ -1663,8 +1662,14 @@ retry:
 
 	atomic_set(&ch->sq_wr_avail, qp_init->cap.max_send_wr);
 
-	pr_debug("%s: max_cqe= %d max_sge= %d sq_size = %d cm_id= %p\n",
-		 __func__, ch->cq->cqe, qp_init->cap.max_send_sge,
+	/*
+	 * qp_init->cap.max_recv_sge is not relevant for SRQ so use
+	 * max_send_sge instead for SRQ.
+	 */
+	ch->max_send_sge = qp_init->cap.max_send_sge;
+	ch->max_recv_sge = qp_init->cap.max_send_sge;
+	pr_debug("%s: max_cqe= %d max_sge= %d w %d r sq_size = %d cm_id= %p\n",
+		 __func__, ch->cq->cqe, ch->max_send_sge, ch->max_recv_sge,
 		 qp_init->cap.max_send_wr, ch->cm_id);
 
 	ret = srpt_init_ch_qp(ch, ch->qp);
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index 3890304..b0876d9 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -106,7 +106,7 @@ enum {
 	SRP_LOGIN_RSP_MULTICHAN_MAINTAINED = 0x2,
 
 	SRPT_DEF_SG_TABLESIZE = 128,
-	SRPT_DEF_SG_PER_WQE = 16,
+	SRPT_MAX_SG_PER_WQE = 16,
 
 	MIN_SRPT_SQ_SIZE = 16,
 	DEF_SRPT_SQ_SIZE = 4096,
@@ -235,6 +235,8 @@ enum rdma_ch_state {
  * @cq:            IB completion queue for this channel.
  * @rq_size:       IB receive queue size.
  * @rsp_size	   IB response message size in bytes.
+ * @max_send_sge:  Maximum SG elements per WR for send requests posted on @qp.
+ * @max_recv_sge:  Maximum SG elements per WR for recv requests posted on @qp.
  * @sq_wr_avail:   number of work requests available in the send queue.
  * @sport:         pointer to the information of the HCA port used by this
  *                 channel.
@@ -265,6 +267,8 @@ struct srpt_rdma_ch {
 	struct kref		kref;
 	int			rq_size;
 	u32			rsp_size;
+	int			max_send_sge;
+	int			max_recv_sge;
 	atomic_t		sq_wr_avail;
 	struct srpt_port	*sport;
 	u8			i_port_id[16];
-- 
2.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] IB/srpt: Simplify srpt_queue_response()

[Bart Van Assche]

Initialize first_wr to &send_wr. This allows to remove a ternary
operator and an else branch. This patch does not change the behavior
of srpt_queue_response().

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
Cc: Parav Pandit <pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 6314d10..41d1159 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2272,7 +2272,7 @@ static void srpt_queue_response(struct se_cmd *cmd)
 		container_of(cmd, struct srpt_send_ioctx, cmd);
 	struct srpt_rdma_ch *ch = ioctx->ch;
 	struct srpt_device *sdev = ch->sport->sdev;
-	struct ib_send_wr send_wr, *first_wr = NULL, *bad_wr;
+	struct ib_send_wr send_wr, *first_wr = &send_wr, *bad_wr;
 	struct ib_sge sge;
 	enum srpt_command_state state;
 	unsigned long flags;
@@ -2313,11 +2313,8 @@ static void srpt_queue_response(struct se_cmd *cmd)
 			struct srpt_rw_ctx *ctx = &ioctx->rw_ctxs[i];
 
 			first_wr = rdma_rw_ctx_wrs(&ctx->rw, ch->qp,
-					ch->sport->port, NULL,
-					first_wr ? first_wr : &send_wr);
+					ch->sport->port, NULL, first_wr);
 		}
-	} else {
-		first_wr = &send_wr;
 	}
 
 	if (state != SRPT_STATE_MGMT)
-- 
2.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] IB/core: Make rdma_rw_ctx_init() initialize all used fields

[Christoph Hellwig]

On Tue, Jun 28, 2016 at 01:25:15PM +0200, Bart Van Assche wrote:
> Some but not all callers of rdma_rw_ctx_init() zero-initialize
> struct rdma_rw_ctx. Hence make rdma_rw_ctx_init() initialize all
> work request fields that will be read by ib_post_send().
> 
> Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> #v4.7+
> Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
> Cc: Parav Pandit <pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/core/rw.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
> index 1eb9b12..13d4067 100644
> --- a/drivers/infiniband/core/rw.c
> +++ b/drivers/infiniband/core/rw.c
> @@ -99,6 +99,7 @@ static int rdma_rw_init_one_mr(struct ib_qp *qp, u8 port_num,
>  	}
>  
>  	reg->reg_wr.wr.opcode = IB_WR_REG_MR;
> +	reg->reg_wr.wr.next = NULL;

This one is always set up by the callers of the function.

> @@ -114,6 +115,7 @@ static int rdma_rw_init_mr_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
>  		u8 port_num, struct scatterlist *sg, u32 sg_cnt, u32 offset,
>  		u64 remote_addr, u32 rkey, enum dma_data_direction dir)
>  {
> +	struct rdma_rw_reg_ctx *prev = NULL;
>  	u32 pages_per_mr = rdma_rw_fr_page_list_len(qp->pd->device);
>  	int i, j, ret = 0, count = 0;
>  
> @@ -125,7 +127,6 @@ static int rdma_rw_init_mr_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
>  	}
>  
>  	for (i = 0; i < ctx->nr_ops; i++) {
> -		struct rdma_rw_reg_ctx *prev = i ? &ctx->reg[i - 1] : NULL;
>  		struct rdma_rw_reg_ctx *reg = &ctx->reg[i];
>  		u32 nents = min(sg_cnt, pages_per_mr);
>  
> @@ -162,9 +163,13 @@ static int rdma_rw_init_mr_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
>  		sg_cnt -= nents;
>  		for (j = 0; j < nents; j++)
>  			sg = sg_next(sg);
> +		prev = reg;
>  		offset = 0;
>  	}
>  
> +	if (prev)
> +		prev->wr.wr.next = NULL;
> +
>  	ctx->type = RDMA_RW_MR;
>  	return count;

I think the right fix here is to set last_wr->next to NULL for
the !chain_wr case in rdma_rw_ctx_wrs, or in fact maybe just simplify
the end of rdma_rw_ctx_wrs to:

	last_wr->next = chain_wr;
	if (!chain_wr) {
		last_wr->wr_cqe = cqe;
		last_wr->send_flags |= IB_SEND_SIGNALED;
	}

> @@ -205,11 +210,10 @@ static int rdma_rw_init_map_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
>  			rdma_wr->wr.opcode = IB_WR_RDMA_READ;
>  		rdma_wr->remote_addr = remote_addr + total_len;
>  		rdma_wr->rkey = rkey;
> +		rdma_wr->wr.num_sge = nr_sge;
>  		rdma_wr->wr.sg_list = sge;
>  
>  		for (j = 0; j < nr_sge; j++, sg = sg_next(sg)) {
> -			rdma_wr->wr.num_sge++;
> -

This parts looks fine.

> -		if (i + 1 < ctx->nr_ops)
> -			rdma_wr->wr.next = &ctx->map.wrs[i + 1].wr;
> +		rdma_wr->wr.next = i + 1 < ctx->nr_ops ?
> +			&ctx->map.wrs[i + 1].wr : NULL;

I think the rdma_rw_ctx_wrs should take care of this as well.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] IB/core: Add max_sge argument to rdma_rw_ctx_init()

[Christoph Hellwig]

On Tue, Jun 28, 2016 at 01:26:03PM +0200, Bart Van Assche wrote:
> The SGE limit for a queue pair is typically lower than what is
> defined by the HCA limits. Hence make max_sge an argument.

I don't think this is the right way to approach any limit.  I'd rather
have a current limit in the ib_qp structure than having to pass it
explicitly and growing driver specific policy once again.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] IB/srpt: Simplify srpt_queue_response()

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] IB/core: Add max_sge argument to rdma_rw_ctx_init()

[Bart Van Assche]

On 06/28/2016 01:52 PM, Christoph Hellwig wrote:
> On Tue, Jun 28, 2016 at 01:26:03PM +0200, Bart Van Assche wrote:
>> The SGE limit for a queue pair is typically lower than what is
>> defined by the HCA limits. Hence make max_sge an argument.
>
> I don't think this is the right way to approach any limit.  I'd rather
> have a current limit in the ib_qp structure than having to pass it
> explicitly and growing driver specific policy once again.

Hello Christoph,

The memory needed for QP buffers strongly depends on max_send_sge and 
max_recv_sge. Using a large value for max_send_sge and/or max_recv_sge 
for all drivers would increase the memory consumption for drivers that 
only submit RDMA requests with a low number of SG elements. Using a 
small value for max_send_sge and/or max_recv_sge would result in a 
performance decrease for drivers that benefit from large max_send_sge 
and/or max_recv_sge values. This is why I think that drivers should have 
the freedom to chose the max_send_sge and max_recv_sge values.

Bart.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] IB/core: Make rdma_rw_ctx_init() initialize all used fields

[Bart Van Assche]

On 06/28/2016 01:51 PM, Christoph Hellwig wrote:
> On Tue, Jun 28, 2016 at 01:25:15PM +0200, Bart Van Assche wrote:
>> -		if (i + 1 < ctx->nr_ops)
>> -			rdma_wr->wr.next = &ctx->map.wrs[i + 1].wr;
>> +		rdma_wr->wr.next = i + 1 < ctx->nr_ops ?
>> +			&ctx->map.wrs[i + 1].wr : NULL;
>
> I think the rdma_rw_ctx_wrs should take care of this as well.

I agree that it is possible to let rdma_rw_ctx_wrs() initialize wr.next 
of the last WR. But if we chose that approach then drivers that call 
rdma_rw_ctx_init() but not rdma_rw_ctx_wrs(), e.g. ib_isert, can end up 
with a last wr.next field that has not been initialized. Hence my 
preference for performing that initialization in rdma_rw_init_mr_wrs() 
and rdma_rw_init_map_wrs().

Bart.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request

[Steve Wise]

> 
> Limit the number of SG elements per work request to what the queue
> pair supports.
> 
> Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")

I think the above is the wrong commit.  Maybe this one?

commit 38a2d0d429f1d87315c55d9139b8bdf66d51c4f4
Author: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Date:   Tue May 3 18:01:13 2016 +0200

    IB/isert: convert to the generic RDMA READ/WRITE API



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request

[Steve Wise]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Bart Van Assche
> Sent: Tuesday, June 28, 2016 6:27 AM
> To: Doug Ledford
> Cc: Christoph Hellwig; Sagi Grimberg; Nicholas A. Bellinger; Parav Pandit; Laurence
> Oberman; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request
> 
> Limit the number of SG elements per work request to what the queue
> pair supports.
> 

Hey Bart, do you think this patch fixes this error we're seeing?

isert: isert_rdma_rw_ctx_post: Cmd: ffff880f30eb0278 failed to post RDMA res
isert: isert_rdma_rw_ctx_post: Cmd: ffff880f30e85ef8 failed to post RDMA res
ABORT_TASK: Found referenced iSCSI task_tag: 14
ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: 14
ABORT_TASK: Found referenced iSCSI task_tag: 40
ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: 40
isert: isert_rdma_rw_ctx_post: Cmd: ffff880f311519f0 failed to post RDMA res
ABORT_TASK: Found referenced iSCSI task_tag: 70
ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: 70
isert: isert_rdma_rw_ctx_post: Cmd: ffff880f311720a0 failed to post RDMA res
ABORT_TASK: Found referenced iSCSI task_tag: 86
ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: 86
isert: isert_rdma_rw_ctx_post: Cmd: ffff880f31160528 failed to post RDMA res
isert: isert_rdma_rw_ctx_post: Cmd: ffff880f30ead238 failed to post RDMA res
isert: isert_rdma_rw_ctx_post: Cmd: ffff880f30ea8fe0 failed to post RDMA res
ABORT_TASK: Found referenced iSCSI task_tag: 124
ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: 124
ABORT_TASK: Found referenced iSCSI task_tag: 120
ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: 120
ABORT_TASK: Found referenced iSCSI task_tag: 111
ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST for ref_tag: 111

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request

[Bart Van Assche]

On 06/28/2016 04:28 PM, Steve Wise wrote:
>
>>
>> Limit the number of SG elements per work request to what the queue
>> pair supports.
>>
>> Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
>
> I think the above is the wrong commit.  Maybe this one?
>
> commit 38a2d0d429f1d87315c55d9139b8bdf66d51c4f4
> Author: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> Date:   Tue May 3 18:01:13 2016 +0200
>
>     IB/isert: convert to the generic RDMA READ/WRITE API

Thanks Steve. I will fix this copy/paste error.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request

[Steve Wise]

> 
> On 06/28/2016 04:28 PM, Steve Wise wrote:
> >
> >>
> >> Limit the number of SG elements per work request to what the queue
> >> pair supports.
> >>
> >> Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
> >
> > I think the above is the wrong commit.  Maybe this one?
> >
> > commit 38a2d0d429f1d87315c55d9139b8bdf66d51c4f4
> > Author: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> > Date:   Tue May 3 18:01:13 2016 +0200
> >
> >     IB/isert: convert to the generic RDMA READ/WRITE API
> 
> Thanks Steve. I will fix this copy/paste error.

Patch 2 has the same problem, I think.

Stevo


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request

[Bart Van Assche]

On 06/28/2016 04:33 PM, Steve Wise wrote:
>> -----Original Message-----
>> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
>> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Bart Van Assche
>> Sent: Tuesday, June 28, 2016 6:27 AM
>> To: Doug Ledford
>> Cc: Christoph Hellwig; Sagi Grimberg; Nicholas A. Bellinger; Parav Pandit; Laurence
>> Oberman; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request
>>
>> Limit the number of SG elements per work request to what the queue
>> pair supports.
>>
>
> Hey Bart, do you think this patch fixes this error we're seeing?
>
> isert: isert_rdma_rw_ctx_post: Cmd: ffff880f30eb0278 failed to post RDMA res

Hello Steve,

Do you perhaps know how many SGE elements the failed work request 
contains? The reason I came up with the max_sge patch is because I 
noticed that the ib_srpt driver reported ib_post_send() failures for 
certain RDMA work requests. A debug printk() confirmed that 
ib_post_send() failed because the following test in the mlx4 driver was hit:

		if (unlikely(wr->num_sge > qp->sq.max_gs)) {
			err = -EINVAL;
			*bad_wr = wr;
			goto out;
		}

For the mlx4 HCA in my test setup max_sge == 32, max_sge_rd == 30 and 
with the QP creation parameters I used max_gs == 28.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request

[Steve Wise]

> On 06/28/2016 04:33 PM, Steve Wise wrote:
> >> -----Original Message-----
> >> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> >> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Bart Van Assche
> >> Sent: Tuesday, June 28, 2016 6:27 AM
> >> To: Doug Ledford
> >> Cc: Christoph Hellwig; Sagi Grimberg; Nicholas A. Bellinger; Parav Pandit;
> Laurence
> >> Oberman; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> Subject: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request
> >>
> >> Limit the number of SG elements per work request to what the queue
> >> pair supports.
> >>
> >
> > Hey Bart, do you think this patch fixes this error we're seeing?
> >
> > isert: isert_rdma_rw_ctx_post: Cmd: ffff880f30eb0278 failed to post RDMA res
> 
> Hello Steve,
> 
> Do you perhaps know how many SGE elements the failed work request
> contains? The reason I came up with the max_sge patch is because I
> noticed that the ib_srpt driver reported ib_post_send() failures for
> certain RDMA work requests. A debug printk() confirmed that
> ib_post_send() failed because the following test in the mlx4 driver was hit:
> 
> 		if (unlikely(wr->num_sge > qp->sq.max_gs)) {
> 			err = -EINVAL;
> 			*bad_wr = wr;
> 			goto out;
> 		}
> 
> For the mlx4 HCA in my test setup max_sge == 32, max_sge_rd == 30 and
> with the QP creation parameters I used max_gs == 28.
> 
> Bart.
> 

I'll let you know as I gather more data.  For now I'm retrying with patches 1-3 of this series to see if it alleviates the problem.  

Stevo

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] IB/core: Add max_sge argument to rdma_rw_ctx_init()

[Christoph Hellwig]

On Tue, Jun 28, 2016 at 02:37:29PM +0200, Bart Van Assche wrote:
> The memory needed for QP buffers strongly depends on max_send_sge and 
> max_recv_sge. Using a large value for max_send_sge and/or max_recv_sge for 
> all drivers would increase the memory consumption for drivers that only 
> submit RDMA requests with a low number of SG elements. Using a small value 
> for max_send_sge and/or max_recv_sge would result in a performance decrease 
> for drivers that benefit from large max_send_sge and/or max_recv_sge 
> values. This is why I think that drivers should have the freedom to chose 
> the max_send_sge and max_recv_sge values.

No problem with that - but please configure it on the QP as part of the
qp_init_attr structure instead of passing it to every RDMA READ/WRITE call.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request

[Christoph Hellwig]

>  	attr.cap.max_send_sge = device->ib_device->attrs.max_sge;
> -	isert_conn->max_sge = min(device->ib_device->attrs.max_sge,
> -				  device->ib_device->attrs.max_sge_rd);
>  	attr.cap.max_recv_sge = 1;
>  	attr.sq_sig_type = IB_SIGNAL_REQ_WR;
>  	attr.qp_type = IB_QPT_RC;
> @@ -151,6 +149,9 @@ isert_create_qp(struct isert_conn *isert_conn,
>  		return ERR_PTR(ret);
>  	}
>  
> +	isert_conn->max_send_sge = attr.cap.max_send_sge;
> +	isert_conn->max_recv_sge = attr.cap.max_recv_sge;

Can you explain how cap.max_recv_sge fits in here?  My understanding
is that it is about the number of SGEs for a receive WR...

> @@ -2087,12 +2087,12 @@ isert_rdma_rw_ctx_post(struct isert_cmd *cmd, struct isert_conn *conn,
>  		addr = cmd->write_va;
>  		rkey = cmd->write_stag;
>  		offset = cmd->iscsi_cmd->write_data_done;
> -		max_sge = dev->attrs.max_sge_rd;
> +		max_sge = conn->max_recv_sge;

.. and not about the number of SGEs for a RDMA READ WR.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request

[Bart Van Assche]

On 06/28/2016 05:46 PM, Christoph Hellwig wrote:
>>  	attr.cap.max_send_sge = device->ib_device->attrs.max_sge;
>> -	isert_conn->max_sge = min(device->ib_device->attrs.max_sge,
>> -				  device->ib_device->attrs.max_sge_rd);
>>  	attr.cap.max_recv_sge = 1;
>>  	attr.sq_sig_type = IB_SIGNAL_REQ_WR;
>>  	attr.qp_type = IB_QPT_RC;
>> @@ -151,6 +149,9 @@ isert_create_qp(struct isert_conn *isert_conn,
>>  		return ERR_PTR(ret);
>>  	}
>>
>> +	isert_conn->max_send_sge = attr.cap.max_send_sge;
>> +	isert_conn->max_recv_sge = attr.cap.max_recv_sge;
>
> Can you explain how cap.max_recv_sge fits in here?  My understanding
> is that it is about the number of SGEs for a receive WR...
>
>> @@ -2087,12 +2087,12 @@ isert_rdma_rw_ctx_post(struct isert_cmd *cmd, struct isert_conn *conn,
>>  		addr = cmd->write_va;
>>  		rkey = cmd->write_stag;
>>  		offset = cmd->iscsi_cmd->write_data_done;
>> -		max_sge = dev->attrs.max_sge_rd;
>> +		max_sge = conn->max_recv_sge;
>
> .. and not about the number of SGEs for a RDMA READ WR.

Indeed. I will update the ib_srpt and ib_isert patches accordingly.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] IB/core: Add max_sge argument to rdma_rw_ctx_init()

[Bart Van Assche]

On 06/28/2016 05:42 PM, Christoph Hellwig wrote:
> On Tue, Jun 28, 2016 at 02:37:29PM +0200, Bart Van Assche wrote:
>> The memory needed for QP buffers strongly depends on max_send_sge and
>> max_recv_sge. Using a large value for max_send_sge and/or max_recv_sge for
>> all drivers would increase the memory consumption for drivers that only
>> submit RDMA requests with a low number of SG elements. Using a small value
>> for max_send_sge and/or max_recv_sge would result in a performance decrease
>> for drivers that benefit from large max_send_sge and/or max_recv_sge
>> values. This is why I think that drivers should have the freedom to chose
>> the max_send_sge and max_recv_sge values.
>
> No problem with that - but please configure it on the QP as part of the
> qp_init_attr structure instead of passing it to every RDMA READ/WRITE call.

That sounds like a good idea to me. I will make this change, retest and 
repost this patch series.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request

[Bart Van Assche]

On 06/28/2016 05:09 PM, Steve Wise wrote:
> I'll let you know as I gather more data.  For now I'm retrying with patches 1-3 of this series to see if it alleviates the problem.

Hi Steve,

Can you use the code from the for-next branch in the following 
repository for your tests: https://github.com/bvanassche/linux.

I will post these patches tomorrow.

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request

[Steve Wise]

> Hi Steve,
> 
> Can you use the code from the for-next branch in the following
> repository for your tests: https://github.com/bvanassche/linux.
> 

Which branch?

> I will post these patches tomorrow.
> 
> Thanks,
> 
> Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] IB/isert: Limit the number of SG elements per work request

[Bart Van Assche]

On 06/28/2016 09:13 PM, Steve Wise wrote:
>> Can you use the code from the for-next branch in the following
>> repository for your tests: https://github.com/bvanassche/linux.
>
> Which branch?

The for-next branch. I thought I had mentioned this in my previous message?

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html