[PATCH v2 0/5] Reduce RDMA RW API SGE limit

[PATCH v2 0/5] Reduce RDMA RW API SGE limit

[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 changes compared to v1 of this patch series are:
* max_send_sge is now stored in struct ib_qp. This greatly simplifies
   this patch series.
* An unneeded initialization that I had added to rdma_rw_init_one_mr()
   has been left out again.
* Corrected "Fixes" tag in the patch description where needed.

The individual patches in this series are:

0001-IB-core-Make-rdma_rw_ctx_init-initialize-all-used-fi.patch
0002-IB-core-RDMA-RW-API-Do-not-exceed-QP-SGE-send-limit.patch
0003-IB-srpt-Limit-the-number-of-SG-elements-per-work-req.patch
0004-IB-srpt-Simplify-srpt_queue_response.patch
0005-IB-isert-Remove-an-unused-member-variable.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 v2 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: a060b5629ab0 ("IB/core: 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..1ad2baa 100644
--- a/drivers/infiniband/core/rw.c
+++ b/drivers/infiniband/core/rw.c
@@ -71,6 +71,7 @@ static inline u32 rdma_rw_fr_page_list_len(struct ib_device *dev)
 	return min_t(u32, dev->attrs.max_fast_reg_page_list_len, 256);
 }
 
+/* Caller must have zero-initialized *reg. */
 static int rdma_rw_init_one_mr(struct ib_qp *qp, u8 port_num,
 		struct rdma_rw_reg_ctx *reg, struct scatterlist *sg,
 		u32 sg_cnt, u32 offset)
@@ -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 v2 2/5] IB/core, RDMA RW API: Do not exceed QP SGE send limit

[Bart Van Assche]

The SGE limit for a queue pair is typically lower than what is
defined by the HCA limits. Hence use the QP SGE send limit
instead of the HCA send limit.

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    | 9 +--------
 drivers/infiniband/core/verbs.c | 2 ++
 include/rdma/ib_verbs.h         | 1 +
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
index 1ad2baa..425e711 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 */
@@ -186,7 +179,7 @@ static int rdma_rw_init_map_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
 		u64 remote_addr, u32 rkey, enum dma_data_direction dir)
 {
 	struct ib_device *dev = qp->pd->device;
-	u32 max_sge = rdma_rw_max_sge(dev, dir);
+	u32 max_sge = qp->max_send_sge;
 	struct ib_sge *sge;
 	u32 total_len = 0, i, j;
 
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 6298f54..c7f840e 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -814,6 +814,8 @@ struct ib_qp *ib_create_qp(struct ib_pd *pd,
 		}
 	}
 
+	qp->max_send_sge = qp_init_attr->cap.max_send_sge;
+
 	return qp;
 }
 EXPORT_SYMBOL(ib_create_qp);
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 7e440d4..c44dbf6 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1449,6 +1449,7 @@ struct ib_qp {
 	void                  (*event_handler)(struct ib_event *, void *);
 	void		       *qp_context;
 	u32			qp_num;
+	u32			max_send_sge;
 	enum ib_qp_type		qp_type;
 };
 
-- 
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 v2 3/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")
Reported-by: Parav Pandit <pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
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 | 3 ++-
 drivers/infiniband/ulp/srpt/ib_srpt.h | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 4a41556..9a3b954 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1601,6 +1601,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;
 
@@ -1638,7 +1639,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);
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index 3890304..d444f8d 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,
-- 
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 v2 4/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>
Reviewed-by: 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 9a3b954..dfa23b0 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2262,7 +2262,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;
@@ -2303,11 +2303,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

[PATCH v2 5/5] IB/isert: Remove an unused member variable

[Bart Van Assche]

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/isert/ib_isert.c | 2 --
 drivers/infiniband/ulp/isert/ib_isert.h | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index a990c04..ba6be06 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;
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
index e512ba9..fc791ef 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -138,7 +138,6 @@ struct isert_conn {
 	u32			responder_resources;
 	u32			initiator_depth;
 	bool			pi_support;
-	u32			max_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

Re: [PATCH v2 0/5] Reduce RDMA RW API SGE limit

[Laurence Oberman]

----- Original Message -----
> From: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> To: "Doug Ledford" <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>, "Sagi Grimberg" <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>, "Nicholas A. Bellinger"
> <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>, "Parav Pandit" <pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, "Laurence Oberman" <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Steve
> Wise" <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Sent: Thursday, June 30, 2016 9:46:06 AM
> Subject: [PATCH v2 0/5] Reduce RDMA RW API SGE limit
> 
> 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 changes compared to v1 of this patch series are:
> * max_send_sge is now stored in struct ib_qp. This greatly simplifies
>    this patch series.
> * An unneeded initialization that I had added to rdma_rw_init_one_mr()
>    has been left out again.
> * Corrected "Fixes" tag in the patch description where needed.
> 
> The individual patches in this series are:
> 
> 0001-IB-core-Make-rdma_rw_ctx_init-initialize-all-used-fi.patch
> 0002-IB-core-RDMA-RW-API-Do-not-exceed-QP-SGE-send-limit.patch
> 0003-IB-srpt-Limit-the-number-of-SG-elements-per-work-req.patch
> 0004-IB-srpt-Simplify-srpt_queue_response.patch
> 0005-IB-isert-Remove-an-unused-member-variable.patch
> 
> Thanks,
> 
> Bart.
> 

Hell Bart
Thanks
Pulling and testing
--
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 v2 0/5] Reduce RDMA RW API SGE limit

[Steve Wise]

Hey Bart, 

Did patch 1 make it?  I don’t see it in my inbox nor at:

http://marc.info/?l=linux-rdma&r=1&b=201606&w=2

Steve.

--
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 v2 0/5] Reduce RDMA RW API SGE limit

[Bart Van Assche]

On 06/30/2016 04:27 PM, Steve Wise wrote:
> Did patch 1 make it?  I don’t see it in my inbox nor at:
>
> http://marc.info/?l=linux-rdma&r=1&b=201606&w=2

Can you check again? According to what I see on 
http://thread.gmane.org/gmane.linux.drivers.rdma/38319 patch 1/5 has 
been sent after patch 5/5. It's unfortunate that the SanDisk SMTP server 
reorders outgoing e-mails.

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 v2 0/5] Reduce RDMA RW API SGE limit

[Steve Wise]

> On 06/30/2016 04:27 PM, Steve Wise wrote:
> > Did patch 1 make it?  I don’t see it in my inbox nor at:
> >
> > http://marc.info/?l=linux-rdma&r=1&b=201606&w=2
> 
> Can you check again? According to what I see on
> http://thread.gmane.org/gmane.linux.drivers.rdma/38319 patch 1/5 has
> been sent after patch 5/5. It's unfortunate that the SanDisk SMTP server
> reorders outgoing e-mails.

I received not long after I sent the email...

Thanks,

Steve.

--
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 v2 0/5] Reduce RDMA RW API SGE limit

[Laurence Oberman]

----- Original Message -----
> From: "Laurence Oberman" <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> To: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: "Doug Ledford" <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>, "Sagi Grimberg" <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>,
> "Nicholas A. Bellinger" <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>, "Parav Pandit" <pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, "Steve Wise"
> <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Sent: Thursday, June 30, 2016 9:54:38 AM
> Subject: Re: [PATCH v2 0/5] Reduce RDMA RW API SGE limit
> 
> 
> 
> ----- Original Message -----
> > From: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> > To: "Doug Ledford" <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>, "Sagi Grimberg" <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>,
> > "Nicholas A. Bellinger"
> > <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>, "Parav Pandit" <pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, "Laurence
> > Oberman" <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Steve
> > Wise" <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Sent: Thursday, June 30, 2016 9:46:06 AM
> > Subject: [PATCH v2 0/5] Reduce RDMA RW API SGE limit
> > 
> > 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 changes compared to v1 of this patch series are:
> > * max_send_sge is now stored in struct ib_qp. This greatly simplifies
> >    this patch series.
> > * An unneeded initialization that I had added to rdma_rw_init_one_mr()
> >    has been left out again.
> > * Corrected "Fixes" tag in the patch description where needed.
> > 
> > The individual patches in this series are:
> > 
> > 0001-IB-core-Make-rdma_rw_ctx_init-initialize-all-used-fi.patch
> > 0002-IB-core-RDMA-RW-API-Do-not-exceed-QP-SGE-send-limit.patch
> > 0003-IB-srpt-Limit-the-number-of-SG-elements-per-work-req.patch
> > 0004-IB-srpt-Simplify-srpt_queue_response.patch
> > 0005-IB-isert-Remove-an-unused-member-variable.patch
> > 
> > Thanks,
> > 
> > Bart.
> > 
> 
> Hell Bart
> Thanks
> Pulling and testing

For the series:
Applied to target server and srp client, tested with MLX4 at FDR 56Gbit

Tested-by: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@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 v2 1/5] IB/core: Make rdma_rw_ctx_init() initialize all used fields

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@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 v2 2/5] IB/core, RDMA RW API: Do not exceed QP SGE send limit

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@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 v2 3/5] IB/srpt: Limit the number of SG elements per work request

[Sagi Grimberg]

> +	qp_init->cap.max_send_sge = min(attrs->max_sge, SRPT_MAX_SG_PER_WQE);

Why are you limiting the max_sge with SRPT_MAX_SG_PER_WQE? what is the
motivation?
--
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 v2 2/5] IB/core, RDMA RW API: Do not exceed QP SGE send limit

[Sagi Grimberg]

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

Wait, this looks wrong...

iWARP devices have max_sge_rd = 1, Steve, did you get to test this?

>   static inline u32 rdma_rw_fr_page_list_len(struct ib_device *dev)
>   {
>   	/* arbitrary limit to avoid allocating gigantic resources */
> @@ -186,7 +179,7 @@ static int rdma_rw_init_map_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
>   		u64 remote_addr, u32 rkey, enum dma_data_direction dir)
>   {
>   	struct ib_device *dev = qp->pd->device;
> -	u32 max_sge = rdma_rw_max_sge(dev, dir);
> +	u32 max_sge = qp->max_send_sge;

Here, rdma_read will use qp.max_send_sge = max_sge which is set by the
ULP.

If the ULP used max(max_sge, max_sge_rd) which is the right thing, then
we get a bug, if the ULP set min(max_sge, max_sge_rd) then it has
effectively a single sge even for writes (which is not good).

The QP user needs to set the maximum sge allowed for reads or writes,
but for reads use max_sge_rd and for writes use max_sge. Otherwise this
will break.
--
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 v2 3/5] IB/srpt: Limit the number of SG elements per work request

[Sagi Grimberg]

>
>> +    qp_init->cap.max_send_sge = min(attrs->max_sge,
>> SRPT_MAX_SG_PER_WQE);
>
> Why are you limiting the max_sge with SRPT_MAX_SG_PER_WQE? what is the
> motivation?

OK, I see the discussion around this (was on a vacation). This is
for limiting the queue-pair send buf size. got it.
--
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 v2 3/5] IB/srpt: Limit the number of SG elements per work request

[Bart Van Assche]

On 07/13/2016 04:27 PM, Sagi Grimberg wrote:
>> +	qp_init->cap.max_send_sge = min(attrs->max_sge, SRPT_MAX_SG_PER_WQE);
>
> Why are you limiting the max_sge with SRPT_MAX_SG_PER_WQE? what is the
> motivation?

Hello Sagi,

The reason that I want to use a lower value than max_sge is to avoid QP 
allocation failures. During my tests with mlx4 adapters I had noticed 
that the buffers that are allocated with kmalloc() during QP creation 
with max_send_sge = max_sge are so large that QP allocation often fails. 
Hence min(..., SRPT_MAX_SGE_PER_WQE).

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 v2 2/5] IB/core, RDMA RW API: Do not exceed QP SGE send limit

[Bart Van Assche]

On 07/13/2016 04:32 PM, Sagi Grimberg wrote:
>
>> -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;
>> -}
>> -
>
> Wait, this looks wrong...
>
> iWARP devices have max_sge_rd = 1, Steve, did you get to test this?
>
>>   static inline u32 rdma_rw_fr_page_list_len(struct ib_device *dev)
>>   {
>>   	/* arbitrary limit to avoid allocating gigantic resources */
>> @@ -186,7 +179,7 @@ static int rdma_rw_init_map_wrs(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
>>   		u64 remote_addr, u32 rkey, enum dma_data_direction dir)
>>   {
>>   	struct ib_device *dev = qp->pd->device;
>> -	u32 max_sge = rdma_rw_max_sge(dev, dir);
>> +	u32 max_sge = qp->max_send_sge;
>
> Here, rdma_read will use qp.max_send_sge = max_sge which is set by the
> ULP.
>
> If the ULP used max(max_sge, max_sge_rd) which is the right thing, then
> we get a bug, if the ULP set min(max_sge, max_sge_rd) then it has
> effectively a single sge even for writes (which is not good).
>
> The QP user needs to set the maximum sge allowed for reads or writes,
> but for reads use max_sge_rd and for writes use max_sge. Otherwise this
> will break.

Hello Sagi,

How about keeping rdma_rw_max_sge() and using the minimum of 
qp->max_send_sge and dev->attrs.max_sge_rd for RDMA READs?

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 v2 2/5] IB/core, RDMA RW API: Do not exceed QP SGE send limit

[Sagi Grimberg]

On 13/07/16 13:18, Bart Van Assche wrote:
> On 07/13/2016 04:32 PM, Sagi Grimberg wrote:
>>
>>> -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;
>>> -}
>>> -
>>
>> Wait, this looks wrong...
>>
>> iWARP devices have max_sge_rd = 1, Steve, did you get to test this?
>>
>>>   static inline u32 rdma_rw_fr_page_list_len(struct ib_device *dev)
>>>   {
>>>       /* arbitrary limit to avoid allocating gigantic resources */
>>> @@ -186,7 +179,7 @@ static int rdma_rw_init_map_wrs(struct
>>> rdma_rw_ctx *ctx, struct ib_qp *qp,
>>>           u64 remote_addr, u32 rkey, enum dma_data_direction dir)
>>>   {
>>>       struct ib_device *dev = qp->pd->device;
>>> -    u32 max_sge = rdma_rw_max_sge(dev, dir);
>>> +    u32 max_sge = qp->max_send_sge;
>>
>> Here, rdma_read will use qp.max_send_sge = max_sge which is set by the
>> ULP.
>>
>> If the ULP used max(max_sge, max_sge_rd) which is the right thing, then
>> we get a bug, if the ULP set min(max_sge, max_sge_rd) then it has
>> effectively a single sge even for writes (which is not good).
>>
>> The QP user needs to set the maximum sge allowed for reads or writes,
>> but for reads use max_sge_rd and for writes use max_sge. Otherwise this
>> will break.
>
> Hello Sagi,
>
> How about keeping rdma_rw_max_sge() and using the minimum of
> qp->max_send_sge and dev->attrs.max_sge_rd for RDMA READs?

That would work.
--
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 v2 5/5] IB/isert: Remove an unused member variable

[Sagi Grimberg]

Thanks Bart,

Acked-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@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 v2 0/5] Reduce RDMA RW API SGE limit

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1245 bytes --]

On 6/30/2016 9:46 AM, Bart Van Assche wrote:
> 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 changes compared to v1 of this patch series are:
> * max_send_sge is now stored in struct ib_qp. This greatly simplifies
>   this patch series.
> * An unneeded initialization that I had added to rdma_rw_init_one_mr()
>   has been left out again.
> * Corrected "Fixes" tag in the patch description where needed.
> 
> The individual patches in this series are:
> 
> 0001-IB-core-Make-rdma_rw_ctx_init-initialize-all-used-fi.patch
> 0002-IB-core-RDMA-RW-API-Do-not-exceed-QP-SGE-send-limit.patch
> 0003-IB-srpt-Limit-the-number-of-SG-elements-per-work-req.patch
> 0004-IB-srpt-Simplify-srpt_queue_response.patch
> 0005-IB-isert-Remove-an-unused-member-variable.patch
> 
> Thanks,
> 
> Bart.

Based on the review from Sagi, I'll wait on v3 of this set.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

RE: [PATCH v2 2/5] IB/core, RDMA RW API: Do not exceed QP SGE send limit

[Steve Wise]

> 
> 
> > -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;
> > -}
> > -
> 
> Wait, this looks wrong...
> 
> iWARP devices have max_sge_rd = 1, Steve, did you get to test this?
>

No I haven't. 
 
> >   static inline u32 rdma_rw_fr_page_list_len(struct ib_device *dev)
> >   {
> >   	/* arbitrary limit to avoid allocating gigantic resources */
> > @@ -186,7 +179,7 @@ static int rdma_rw_init_map_wrs(struct rdma_rw_ctx
> *ctx, struct ib_qp *qp,
> >   		u64 remote_addr, u32 rkey, enum dma_data_direction dir)
> >   {
> >   	struct ib_device *dev = qp->pd->device;
> > -	u32 max_sge = rdma_rw_max_sge(dev, dir);
> > +	u32 max_sge = qp->max_send_sge;
> 
> Here, rdma_read will use qp.max_send_sge = max_sge which is set by the
> ULP.
> 
> If the ULP used max(max_sge, max_sge_rd) which is the right thing, then
> we get a bug, if the ULP set min(max_sge, max_sge_rd) then it has
> effectively a single sge even for writes (which is not good).
> 
> The QP user needs to set the maximum sge allowed for reads or writes,
> but for reads use max_sge_rd and for writes use max_sge. Otherwise this
> will break.

I haven't paid enough attention to these changes, but definitely max_sge_rd is for reads and max_sge is for writes...


--
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 v2 2/5] IB/core, RDMA RW API: Do not exceed QP SGE send limit

[Steve Wise]

> 
> 
> On 13/07/16 13:18, Bart Van Assche wrote:
> > On 07/13/2016 04:32 PM, Sagi Grimberg wrote:
> >>
> >>> -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;
> >>> -}
> >>> -
> >>
> >> Wait, this looks wrong...
> >>
> >> iWARP devices have max_sge_rd = 1, Steve, did you get to test this?
> >>
> >>>   static inline u32 rdma_rw_fr_page_list_len(struct ib_device *dev)
> >>>   {
> >>>       /* arbitrary limit to avoid allocating gigantic resources */
> >>> @@ -186,7 +179,7 @@ static int rdma_rw_init_map_wrs(struct
> >>> rdma_rw_ctx *ctx, struct ib_qp *qp,
> >>>           u64 remote_addr, u32 rkey, enum dma_data_direction dir)
> >>>   {
> >>>       struct ib_device *dev = qp->pd->device;
> >>> -    u32 max_sge = rdma_rw_max_sge(dev, dir);
> >>> +    u32 max_sge = qp->max_send_sge;
> >>
> >> Here, rdma_read will use qp.max_send_sge = max_sge which is set by the
> >> ULP.
> >>
> >> If the ULP used max(max_sge, max_sge_rd) which is the right thing, then
> >> we get a bug, if the ULP set min(max_sge, max_sge_rd) then it has
> >> effectively a single sge even for writes (which is not good).
> >>
> >> The QP user needs to set the maximum sge allowed for reads or writes,
> >> but for reads use max_sge_rd and for writes use max_sge. Otherwise this
> >> will break.
> >
> > Hello Sagi,
> >
> > How about keeping rdma_rw_max_sge() and using the minimum of
> > qp->max_send_sge and dev->attrs.max_sge_rd for RDMA READs?
> 
> That would work.


I'll be sure and test v3 of this series on cxgb4 (please CC me).

Steve.

--
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 v2 2/5] IB/core, RDMA RW API: Do not exceed QP SGE send limit

[Bart Van Assche]

On 07/13/2016 09:29 PM, Steve Wise wrote:
> I'll be sure and test v3 of this series on cxgb4 (please CC me).

Thanks Steve. I will CC you for v3 of this 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