Four SRP initiator and target patches

PATCH Applied Bart Van Assche 3 participants 10 messages May 25, 2020 – May 29, 2020

Subsystems (3)

InfiniBand SCSI RDMA protocol (SRP) initiator SCSI RDMA protocol (SRP) target

lore

Share & tools

lore

Bart Van Assche

author maintainer

2020-05-25 17:22 UTC #0 lore raw

v2 cover Four SRP initiator and target patches

Hi Jason,

Please consider these four patches for kernel v5.8 or v5.9 (not sure if it is still possible to include these in v5.8).

Thanks,

Bart.

Changes compared to v1:
- Changed %d into %u in the SRP patch as requested by Leon.
- Simplified patch "RDMA/srpt: Increase max_send_sge".

Bart Van Assche (4):
RDMA/srp: Make the channel count configurable per target RDMA/srpt: Make debug output more detailed RDMA/srpt: Reduce max_recv_sge to 1
RDMA/srpt: Increase max_send_sge

 drivers/infiniband/ulp/srp/ib_srp.c   | 21 ++++++++++++++++-----
 drivers/infiniband/ulp/srpt/ib_srpt.c | 22 +++++++++-------------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  5 -----
 3 files changed, 25 insertions(+), 23 deletions(-)

Bart Van Assche

author maintainer

2020-05-25 17:22 UTC #1 lore raw

PATCH v2 1/4 RDMA/srp: Make the channel count configurable per target

Increase the flexibility of the SRP initiator driver by making the channel count configurable per target instead of only providing a kernel module parameter for configuring the channel count.

Cc Laurence Oberman <[email protected]>

Cc Kamal Heib <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

---
 drivers/infiniband/ulp/srp/ib_srp.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +16 -5

index 00b4f88b113e..4018c4abf2e2 100644
@@ -3424,6 +3424,7 @@ enum {
 	SRP_OPT_IP_DEST		= 1 << 16,
 	SRP_OPT_TARGET_CAN_QUEUE= 1 << 17,
 	SRP_OPT_MAX_IT_IU_SIZE  = 1 << 18,
+	SRP_OPT_CH_COUNT	= 1 << 19,
 };
 
 static unsigned int srp_opt_mandatory[] = {

@@ -3457,6 +3458,7 @@ static const match_table_t srp_opt_tokens = {
 	{ SRP_OPT_IP_SRC,		"src=%s"		},
 	{ SRP_OPT_IP_DEST,		"dest=%s"		},
 	{ SRP_OPT_MAX_IT_IU_SIZE,	"max_it_iu_size=%d"	},
+	{ SRP_OPT_CH_COUNT,		"ch_count=%u",		},
 	{ SRP_OPT_ERR,			NULL 			}
 };

@@ -3758,6 +3760,14 @@ static int srp_parse_options(struct net *net, const char *buf,
 			target->max_it_iu_size = token;
 			break;
 
+		case SRP_OPT_CH_COUNT:
+			if (match_int(args, &token) || token < 1) {
+				pr_warn("bad channel count %s\n", p);
+				goto out;
+			}
+			target->ch_count = token;
+			break;
+
 		default:
 			pr_warn("unknown parameter or missing value '%s' in target creation request\n",
 				p);

@@ -3921,11 +3931,12 @@ static ssize_t srp_create_target(struct device *dev,
 		goto out;
 
 	ret = -ENOMEM;
-	target->ch_count = max_t(unsigned, num_online_nodes(),
-				 min(ch_count ? :
-				     min(4 * num_online_nodes(),
-					 ibdev->num_comp_vectors),
-				     num_online_cpus()));
+	if (target->ch_count == 0)
+		target->ch_count = max_t(unsigned, num_online_nodes(),
+					 min(ch_count ? :
+					     min(4 * num_online_nodes(),
+						 ibdev->num_comp_vectors),
+					     num_online_cpus()));
 	target->ch = kcalloc(target->ch_count, sizeof(*target->ch),
 			     GFP_KERNEL);
 	if (!target->ch)

Bart Van Assche

author maintainer

2020-05-25 17:22 UTC #2 lore raw

PATCH v2 2/4 RDMA/srpt: Make debug output more detailed

In reply to Bart Van Assche

Since the session name by itself is not sufficient to uniquely identify a queue pair, include the queue pair number. Show the ASCII channel state name instead of the numeric value. This change makes the ib_srpt debug output more consistent.

Cc Laurence Oberman <[email protected]>

Cc Kamal Heib <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/infiniband/ulp/srpt/ib_srpt.c +5 -4

index a294630f2100..a39ad9fc4224 100644
@@ -214,8 +214,9 @@ static const char *get_ch_state_name(enum rdma_ch_state s)
  */
 static void srpt_qp_event(struct ib_event *event, struct srpt_rdma_ch *ch)
 {
-	pr_debug("QP event %d on ch=%p sess_name=%s state=%d\n",
-		 event->event, ch, ch->sess_name, ch->state);
+	pr_debug("QP event %d on ch=%p sess_name=%s-%d state=%s\n",
+		 event->event, ch, ch->sess_name, ch->qp->qp_num,
+		 get_ch_state_name(ch->state));
 
 	switch (event->event) {
 	case IB_EVENT_COMM_EST:

@@ -1985,8 +1986,8 @@ static void __srpt_close_all_ch(struct srpt_port *sport)
 	list_for_each_entry(nexus, &sport->nexus_list, entry) {
 		list_for_each_entry(ch, &nexus->ch_list, list) {
 			if (srpt_disconnect_ch(ch) >= 0)
-				pr_info("Closing channel %s because target %s_%d has been disabled\n",
-					ch->sess_name,
+				pr_info("Closing channel %s-%d because target %s_%d has been disabled\n",
+					ch->sess_name, ch->qp->qp_num,
 					dev_name(&sport->sdev->device->dev),
 					sport->port);
 			srpt_close_ch(ch);

Bart Van Assche

author maintainer

2020-05-25 17:22 UTC #3 lore raw

PATCH v2 3/4 RDMA/srpt: Reduce max_recv_sge to 1

In reply to Bart Van Assche

Since srpt_post_recv() always sets num_sge to 1, reduce the max_recv_sge parameter that is used at queue pair allocation time to 1.

Cc Laurence Oberman <[email protected]>

Cc Kamal Heib <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

drivers/infiniband/ulp/srpt/ib_srpt.c +3 -7

index a39ad9fc4224..1ad3cc7c553a 100644
@@ -1818,16 +1818,12 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch)
 	qp_init->cap.max_rdma_ctxs = sq_size / 2;
 	qp_init->cap.max_send_sge = min(attrs->max_send_sge,
 					SRPT_MAX_SG_PER_WQE);
-	qp_init->cap.max_recv_sge = min(attrs->max_recv_sge,
-					SRPT_MAX_SG_PER_WQE);
+	qp_init->cap.max_recv_sge = 1;
 	qp_init->port_num = ch->sport->port;
-	if (sdev->use_srq) {
+	if (sdev->use_srq)
 		qp_init->srq = sdev->srq;
-	} else {
+	else
 		qp_init->cap.max_recv_wr = ch->rq_size;
-		qp_init->cap.max_recv_sge = min(attrs->max_recv_sge,
-						SRPT_MAX_SG_PER_WQE);
-	}
 
 	if (ch->using_rdma_cm) {
 		ret = rdma_create_qp(ch->rdma_cm.cm_id, sdev->pd, qp_init);

Bart Van Assche

author maintainer

2020-05-25 17:22 UTC #4 lore raw

PATCH v2 4/4 RDMA/srpt: Increase max_send_sge

In reply to Bart Van Assche

The ib_srpt driver limits max_send_sge to 16. Since that is a workaround for an mlx4 bug that has been fixed, increase max_send_sge. For mlx4, do not use the value advertised by the driver (32) since that causes QP's to transition to the error status.

See also commit f95ccffc715b ("IB/mlx4: Use 4K pages for kernel QP's WQE buffer").

Cc Laurence Oberman <[email protected]>

Cc Kamal Heib <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 3 +--
 drivers/infiniband/ulp/srpt/ib_srpt.h | 5 -----
 2 files changed, 1 insertion(+), 7 deletions(-)

drivers/infiniband/ulp/srpt/ib_srpt.c +1 -2

index 1ad3cc7c553a..86e4c87e7ec2 100644
@@ -1816,8 +1816,7 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch)
 	 */
 	qp_init->cap.max_send_wr = min(sq_size / 2, attrs->max_qp_wr);
 	qp_init->cap.max_rdma_ctxs = sq_size / 2;
-	qp_init->cap.max_send_sge = min(attrs->max_send_sge,
-					SRPT_MAX_SG_PER_WQE);
+	qp_init->cap.max_send_sge = attrs->max_send_sge;
 	qp_init->cap.max_recv_sge = 1;
 	qp_init->port_num = ch->sport->port;
 	if (sdev->use_srq)

drivers/infiniband/ulp/srpt/ib_srpt.h +0 -5

index 2e1a69840857..f31c349d07a1 100644
@@ -105,11 +105,6 @@ enum {
 	SRP_CMD_ACA = 0x4,
 
 	SRPT_DEF_SG_TABLESIZE = 128,
-	/*
-	 * An experimentally determined value that avoids that QP creation
-	 * fails due to "swiotlb buffer is full" on systems using the swiotlb.
-	 */
-	SRPT_MAX_SG_PER_WQE = 16,
 
 	MIN_SRPT_SQ_SIZE = 16,
 	DEF_SRPT_SQ_SIZE = 4096,

Leon Romanovsky

2020-05-25 17:52 UTC #5 lore raw

Re: [PATCH v2 4/4] RDMA/srpt: Increase max_send_sge

Quoted 4 lines · Bart Van Assche

The ib_srpt driver limits max_send_sge to 16. Since that is a workaround for an mlx4 bug that has been fixed, increase max_send_sge. For mlx4, do not use the value advertised by the driver (32) since that causes QP's to transition to the error status.

Bart,

How are you avoiding mlx4 bug in this patch? Isn't "attrs->max_send_sge" come from driver as is?

Thanks

Quoted 42 lines

See also commit f95ccffc715b ("IB/mlx4: Use 4K pages for kernel QP's WQE buffer").

Cc Laurence Oberman <[email protected]>

Cc Kamal Heib <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 3 +--
 drivers/infiniband/ulp/srpt/ib_srpt.h | 5 -----
 2 files changed, 1 insertion(+), 7 deletions(-)

drivers/infiniband/ulp/srpt/ib_srpt.c +1 -2

index 1ad3cc7c553a..86e4c87e7ec2 100644
@@ -1816,8 +1816,7 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch)
 	 */
 	qp_init->cap.max_send_wr = min(sq_size / 2, attrs->max_qp_wr);
 	qp_init->cap.max_rdma_ctxs = sq_size / 2;
-	qp_init->cap.max_send_sge = min(attrs->max_send_sge,
-					SRPT_MAX_SG_PER_WQE);
+	qp_init->cap.max_send_sge = attrs->max_send_sge;
 	qp_init->cap.max_recv_sge = 1;
 	qp_init->port_num = ch->sport->port;
 	if (sdev->use_srq)

drivers/infiniband/ulp/srpt/ib_srpt.h +0 -5

index 2e1a69840857..f31c349d07a1 100644
@@ -105,11 +105,6 @@ enum {
 	SRP_CMD_ACA = 0x4,
 
 	SRPT_DEF_SG_TABLESIZE = 128,
-	/*
-	 * An experimentally determined value that avoids that QP creation
-	 * fails due to "swiotlb buffer is full" on systems using the swiotlb.
-	 */
-	SRPT_MAX_SG_PER_WQE = 16,
 
 	MIN_SRPT_SQ_SIZE = 16,
 	DEF_SRPT_SQ_SIZE = 4096,

Bart Van Assche

author maintainer

2020-05-25 19:15 UTC #6 lore raw

Re: [PATCH v2 4/4] RDMA/srpt: Increase max_send_sge

Quoted 8 lines · Leon Romanovsky

Quoted 4 lines · Bart Van Assche
The ib_srpt driver limits max_send_sge to 16. Since that is a workaround for an mlx4 bug that has been fixed, increase max_send_sge. For mlx4, do not use the value advertised by the driver (32) since that causes QP's to transition to the error status.
How are you avoiding mlx4 bug in this patch? Isn't "attrs->max_send_sge" come from driver as is?

Hi Leon,

Development of this patch started considerable time ago - before the ib_srpt driver was converted to the RDMA R/W API. Before that conversion the ib_srpt driver was using attrs->max_send_sge limit for both RDMA writes and RDMA reads, which is wrong. Hence the need for the "max_sge_delta" parameter (max_send_sge = 32 and max_sge_rd = 30 for mlx4). The following code from drivers/infiniband/core/rw.c selects the proper limit:

       u32 max_sge = dir == DMA_TO_DEVICE ? qp->max_write_sge :
                     qp->max_read_sge;

The following code in drivers/infiniband/core/verbs.c sets these limits:

        qp->max_write_sge = qp_init_attr->cap.max_send_sge;
        qp->max_read_sge = min_t(u32, qp_init_attr->cap.max_send_sge,
                                 device->attrs.max_sge_rd);

Bart.

Bart Van Assche

author maintainer

2020-05-25 21:51 UTC #7 lore raw

Re: [PATCH v2 4/4] RDMA/srpt: Increase max_send_sge

Quoted 20 lines · Bart Van Assche

Quoted 8 lines · Leon Romanovsky
Quoted 4 lines · Bart Van Assche
The ib_srpt driver limits max_send_sge to 16. Since that is a workaround for an mlx4 bug that has been fixed, increase max_send_sge. For mlx4, do not use the value advertised by the driver (32) since that causes QP's to transition to the error status.
How are you avoiding mlx4 bug in this patch? Isn't "attrs->max_send_sge" come from driver as is?
Hi Leon,
Development of this patch started considerable time ago - before the ib_srpt driver was converted to the RDMA R/W API. Before that conversion the ib_srpt driver was using attrs->max_send_sge limit for both RDMA writes and RDMA reads, which is wrong. Hence the need for the "max_sge_delta" parameter (max_send_sge = 32 and max_sge_rd = 30 for mlx4). The following code from drivers/infiniband/core/rw.c selects the proper limit:

Quoted 8 lines

       u32 max_sge = dir == DMA_TO_DEVICE ? qp->max_write_sge :
                     qp->max_read_sge;

The following code in drivers/infiniband/core/verbs.c sets these limits:

        qp->max_write_sge = qp_init_attr->cap.max_send_sge;
        qp->max_read_sge = min_t(u32, qp_init_attr->cap.max_send_sge,
                                 device->attrs.max_sge_rd);

The commit message should be shortened to the following: "The ib_srpt driver limits max_send_sge to 16. Since that is a workaround for an mlx4 bug that has been fixed, increase max_send_sge. See also commit f95ccffc715b ("IB/mlx4: Use 4K pages for kernel QP's WQE buffer")."

Bart.

Leon Romanovsky

2020-05-26 05:55 UTC #8 lore raw

Re: [PATCH v2 4/4] RDMA/srpt: Increase max_send_sge

Quoted 26 lines · Bart Van Assche

Quoted 25 lines · Bart Van Assche
Quoted 8 lines · Leon Romanovsky
Quoted 4 lines · Bart Van Assche
The ib_srpt driver limits max_send_sge to 16. Since that is a workaround for an mlx4 bug that has been fixed, increase max_send_sge. For mlx4, do not use the value advertised by the driver (32) since that causes QP's to transition to the error status.
How are you avoiding mlx4 bug in this patch? Isn't "attrs->max_send_sge" come from driver as is?
Hi Leon,
Development of this patch started considerable time ago - before the ib_srpt driver was converted to the RDMA R/W API. Before that conversion the ib_srpt driver was using attrs->max_send_sge limit for both RDMA writes and RDMA reads, which is wrong. Hence the need for the "max_sge_delta" parameter (max_send_sge = 32 and max_sge_rd = 30 for mlx4). The following code from drivers/infiniband/core/rw.c selects the proper limit:
       u32 max_sge = dir == DMA_TO_DEVICE ? qp->max_write_sge :
                     qp->max_read_sge;
The following code in drivers/infiniband/core/verbs.c sets these limits:

Quoted 8 lines

Quoted 3 lines
        qp->max_write_sge = qp_init_attr->cap.max_send_sge;
        qp->max_read_sge = min_t(u32, qp_init_attr->cap.max_send_sge,
                                 device->attrs.max_sge_rd);
The commit message should be shortened to the following: "The ib_srpt driver limits max_send_sge to 16. Since that is a workaround for an mlx4 bug that has been fixed, increase max_send_sge. See also commit f95ccffc715b ("IB/mlx4: Use 4K pages for kernel QP's WQE buffer")."

Yes, please. The proposed commit message describes better the change.

Thanks

Quoted 2 lines

Bart.

Jason Gunthorpe

Applied

2020-05-29 18:14 UTC #9 lore raw

In reply to Bart Van Assche

Quoted 13 lines · Bart Van Assche

Hi Jason,
Please consider these four patches for kernel v5.8 or v5.9 (not sure if it is still possible to include these in v5.8).
Thanks,
Bart.
Changes compared to v1:
- Changed %d into %u in the SRP patch as requested by Leon.
- Simplified patch "RDMA/srpt: Increase max_send_sge".

Quoted 5 lines

Bart Van Assche (4):
RDMA/srp: Make the channel count configurable per target RDMA/srpt: Make debug output more detailed RDMA/srpt: Reduce max_recv_sge to 1
RDMA/srpt: Increase max_send_sge

Applied to for-next with the revise commit message

Thanks,
Jason