[PATCH 0/7] SRP patches for kernel v4.15

[PATCH 0/7] SRP patches for kernel v4.15

[Bart Van Assche]

Hello Doug,

This patch series includes several the SRP initiator and target patches I came
up with during the past months. The initiator patches are independent of the
target patches. Please consider these patches for kernel v4.15.

Thanks,

Bart.

Bart Van Assche (7):
  IB/srpt: Limit the send and receive queue sizes to what the HCA
    supports
  IB/srpt: Cache global L_Key
  IB/srpt: Change default behavior from using SRQ to not using SRQ
  IB/srp: Avoid a cable pull can trigger a kernel crash
  IB/srp: Remove second argument of srp_destroy_qp()
  IB/srp: Cache global rkey
  IB/srp: Make CM timeout dependent on subnet timeout

 drivers/infiniband/ulp/srp/ib_srp.c   |  88 ++++++++++++++-------
 drivers/infiniband/ulp/srp/ib_srp.h   |   3 +-
 drivers/infiniband/ulp/srpt/ib_srpt.c | 145 ++++++++++++++++++++++++----------
 drivers/infiniband/ulp/srpt/ib_srpt.h |   7 +-
 4 files changed, 173 insertions(+), 70 deletions(-)

-- 
2.14.2

--
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/7] IB/srp: Avoid a cable pull can trigger a kernel crash

[Bart Van Assche]

This patch fixes the following kernel crash:

general protection fault: 0000 [#1] PREEMPT SMP
Workqueue: ib_mad2 timeout_sends [ib_core]
Call Trace:
 ib_sa_path_rec_callback+0x1c4/0x1d0 [ib_core]
 send_handler+0xb2/0xd0 [ib_core]
 timeout_sends+0x14d/0x220 [ib_core]
 process_one_work+0x200/0x630
 worker_thread+0x4e/0x3b0
 kthread+0x113/0x150

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: <stable@vger.kernel.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 778a08250f16..8ceb4bb011f1 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -665,12 +665,19 @@ static void srp_path_rec_completion(int status,
 static int srp_lookup_path(struct srp_rdma_ch *ch)
 {
 	struct srp_target_port *target = ch->target;
-	int ret;
+	int ret = -ENODEV;
 
 	ch->path.numb_path = 1;
 
 	init_completion(&ch->done);
 
+	/*
+	 * Avoid that the SCSI host can be removed by srp_remove_target()
+	 * before srp_path_rec_completion() is called.
+	 */
+	if (!scsi_host_get(target->scsi_host))
+		goto out;
+
 	ch->path_query_id = ib_sa_path_rec_get(&srp_sa_client,
 					       target->srp_host->srp_dev->dev,
 					       target->srp_host->port,
@@ -684,18 +691,24 @@ static int srp_lookup_path(struct srp_rdma_ch *ch)
 					       GFP_KERNEL,
 					       srp_path_rec_completion,
 					       ch, &ch->path_query);
-	if (ch->path_query_id < 0)
-		return ch->path_query_id;
+	ret = ch->path_query_id;
+	if (ret < 0)
+		goto put;
 
 	ret = wait_for_completion_interruptible(&ch->done);
 	if (ret < 0)
-		return ret;
+		goto put;
 
-	if (ch->status < 0)
+	ret = ch->status;
+	if (ret < 0)
 		shost_printk(KERN_WARNING, target->scsi_host,
 			     PFX "Path record query failed\n");
 
-	return ch->status;
+put:
+	scsi_host_put(target->scsi_host);
+
+out:
+	return ret;
 }
 
 static int srp_send_req(struct srp_rdma_ch *ch, bool multich)
-- 
2.14.2

Re: [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

On Fri, Oct 06, 2017 at 02:42:40PM -0700, Bart Van Assche wrote:
> This patch fixes the following kernel crash:
>
> general protection fault: 0000 [#1] PREEMPT SMP
> Workqueue: ib_mad2 timeout_sends [ib_core]
> Call Trace:
>  ib_sa_path_rec_callback+0x1c4/0x1d0 [ib_core]
>  send_handler+0xb2/0xd0 [ib_core]
>  timeout_sends+0x14d/0x220 [ib_core]
>  process_one_work+0x200/0x630
>  worker_thread+0x4e/0x3b0
>  kthread+0x113/0x150
>
> Signed-off-by: Bart Van Assche <bart.vanassche-Sjgp3cTcYWE@public.gmane.org>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>

You need Fixes tag for the stable tree.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 570 bytes --]

On Fri, Oct 06, 2017 at 02:42:40PM -0700, Bart Van Assche wrote:
> This patch fixes the following kernel crash:
>
> general protection fault: 0000 [#1] PREEMPT SMP
> Workqueue: ib_mad2 timeout_sends [ib_core]
> Call Trace:
>  ib_sa_path_rec_callback+0x1c4/0x1d0 [ib_core]
>  send_handler+0xb2/0xd0 [ib_core]
>  timeout_sends+0x14d/0x220 [ib_core]
>  process_one_work+0x200/0x630
>  worker_thread+0x4e/0x3b0
>  kthread+0x113/0x150
>
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: <stable@vger.kernel.org>

You need Fixes tag for the stable tree.

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash

[Bart Van Assche]

On Sun, 2017-10-08 at 16:22 +0300, Leon Romanovsky wrote:
> On Fri, Oct 06, 2017 at 02:42:40PM -0700, Bart Van Assche wrote:
> > This patch fixes the following kernel crash:
> > 
> > general protection fault: 0000 [#1] PREEMPT SMP
> > Workqueue: ib_mad2 timeout_sends [ib_core]
> > Call Trace:
> >  ib_sa_path_rec_callback+0x1c4/0x1d0 [ib_core]
> >  send_handler+0xb2/0xd0 [ib_core]
> >  timeout_sends+0x14d/0x220 [ib_core]
> >  process_one_work+0x200/0x630
> >  worker_thread+0x4e/0x3b0
> >  kthread+0x113/0x150
> > 
> > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> > Cc: <stable@vger.kernel.org>
> 
> You need Fixes tag for the stable tree.

Hello Leon,

OK, I will add that tag when I repost this patch series.

Bart.

Re: [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash

[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 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 1/7] IB/srpt: Limit the send and receive queue sizes to what the HCA supports

[Bart Van Assche]

Additionally, correct the comment about ch->rq_size.

Signed-off-by: Bart Van Assche <bart.vanassche-Sjgp3cTcYWE@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 95178b4e3565..76370e39857d 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1650,7 +1650,7 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch)
 	 * both both, as RDMA contexts will also post completions for the
 	 * RDMA READ case.
 	 */
-	qp_init->cap.max_send_wr = srp_sq_size / 2;
+	qp_init->cap.max_send_wr = min(srp_sq_size / 2, attrs->max_qp_wr + 0U);
 	qp_init->cap.max_rdma_ctxs = srp_sq_size / 2;
 	qp_init->cap.max_send_sge = min(attrs->max_sge, SRPT_MAX_SG_PER_WQE);
 	qp_init->port_num = ch->sport->port;
@@ -1953,10 +1953,11 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	ch->cm_id = cm_id;
 	cm_id->context = ch;
 	/*
-	 * Avoid QUEUE_FULL conditions by limiting the number of buffers used
-	 * for the SRP protocol to the command queue size.
+	 * ch->rq_size should be at least as large as the initiator queue
+	 * depth to avoid that the initiator driver has to report QUEUE_FULL
+	 * to the SCSI mid-layer.
 	 */
-	ch->rq_size = SRPT_RQ_SIZE;
+	ch->rq_size = min(SRPT_RQ_SIZE, sdev->device->attrs.max_qp_wr);
 	spin_lock_init(&ch->spinlock);
 	ch->state = CH_CONNECTING;
 	INIT_LIST_HEAD(&ch->cmd_wait_list);
-- 
2.14.2

--
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/7] IB/srpt: Limit the send and receive queue sizes to what the HCA supports

[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

[PATCH 2/7] IB/srpt: Cache global L_Key

[Bart Van Assche]

This patch is a micro-optimization for the hot path.

Signed-off-by: Bart Van Assche <bart.vanassche-Sjgp3cTcYWE@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 6 ++++--
 drivers/infiniband/ulp/srpt/ib_srpt.h | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 76370e39857d..6cf95ad870cc 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -766,7 +766,7 @@ static int srpt_post_recv(struct srpt_device *sdev,
 	BUG_ON(!sdev);
 	list.addr = ioctx->ioctx.dma;
 	list.length = srp_max_req_size;
-	list.lkey = sdev->pd->local_dma_lkey;
+	list.lkey = sdev->lkey;
 
 	ioctx->ioctx.cqe.done = srpt_recv_done;
 	wr.wr_cqe = &ioctx->ioctx.cqe;
@@ -2343,7 +2343,7 @@ static void srpt_queue_response(struct se_cmd *cmd)
 
 	sge.addr = ioctx->ioctx.dma;
 	sge.length = resp_len;
-	sge.lkey = sdev->pd->local_dma_lkey;
+	sge.lkey = sdev->lkey;
 
 	ioctx->ioctx.cqe.done = srpt_send_done;
 	send_wr.next = NULL;
@@ -2491,6 +2491,8 @@ static void srpt_add_one(struct ib_device *device)
 	if (IS_ERR(sdev->pd))
 		goto free_dev;
 
+	sdev->lkey = sdev->pd->local_dma_lkey;
+
 	sdev->srq_size = min(srpt_srq_size, sdev->device->attrs.max_srq_wr);
 
 	srq_attr.event_handler = srpt_srq_event;
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index 1b817e51b84b..976e924d7400 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -343,7 +343,7 @@ struct srpt_port {
  * struct srpt_device - Information associated by SRPT with a single HCA.
  * @device:        Backpointer to the struct ib_device managed by the IB core.
  * @pd:            IB protection domain.
- * @mr:            L_Key (local key) with write access to all local memory.
+ * @lkey:          L_Key (local key) with write access to all local memory.
  * @srq:           Per-HCA SRQ (shared receive queue).
  * @cm_id:         Connection identifier.
  * @srq_size:      SRQ size.
@@ -358,6 +358,7 @@ struct srpt_port {
 struct srpt_device {
 	struct ib_device	*device;
 	struct ib_pd		*pd;
+	u32			lkey;
 	struct ib_srq		*srq;
 	struct ib_cm_id		*cm_id;
 	int			srq_size;
-- 
2.14.2

--
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/7] IB/srpt: Cache global L_Key

[Christoph Hellwig]

On Fri, Oct 06, 2017 at 02:42:38PM -0700, Bart Van Assche wrote:
> This patch is a micro-optimization for the hot path.

How much does it buy you on what workload?
--
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/7] IB/srpt: Cache global L_Key

[Bart Van Assche]

On Sun, 2017-10-08 at 02:01 -0700, Christoph Hellwig wrote:
> On Fri, Oct 06, 2017 at 02:42:38PM -0700, Bart Van Assche wrote:
> > This patch is a micro-optimization for the hot path.
> 
> How much does it buy you on what workload?

Hello Christoph,

Because I assume that the improvement realized by this patch will be smaller
than the measurement error I have not yet tried to measure how much this
patch impacts performance. Yet I think this patch is beneficial because it
touches code in the hot path and because it reduces the number of pointer
indirections in that code path.

Bart.

[PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Bart Van Assche]

Although the non-SRQ mode needs more resources that mode has three
advantages over SRQ:
- It works with all RDMA adapters, even those that do not support
  SRQ.
- Posting WRs and polling WCs does not trigger lock contention
  because only one thread at a time accesses a WR or WC queue in
  non-SRQ mode.
- The end-to-end flow control mechanism is used.

>From the IB spec:

    C9-150.2.1: For QPs that are not associated with an SRQ, each HCA
    receive queue shall generate end-to-end flow control credits. If
    a QP is associated with an SRQ, the HCA receive queue shall not
    generate end-to-end flow control credits.

Signed-off-by: Bart Van Assche <bart.vanassche-Sjgp3cTcYWE@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 130 +++++++++++++++++++++++++---------
 drivers/infiniband/ulp/srpt/ib_srpt.h |   4 ++
 2 files changed, 99 insertions(+), 35 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 6cf95ad870cc..a21b7a96635c 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -75,11 +75,19 @@ module_param(srp_max_req_size, int, 0444);
 MODULE_PARM_DESC(srp_max_req_size,
 		 "Maximum size of SRP request messages in bytes.");
 
+static bool use_srq;
+module_param(use_srq, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(use_srq, "Whether or not to use SRQ");
+
 static int srpt_srq_size = DEFAULT_SRPT_SRQ_SIZE;
 module_param(srpt_srq_size, int, 0444);
 MODULE_PARM_DESC(srpt_srq_size,
 		 "Shared receive queue (SRQ) size.");
 
+static int srpt_sq_size = DEF_SRPT_SQ_SIZE;
+module_param(srpt_sq_size, int, 0444);
+MODULE_PARM_DESC(srpt_sq_size, "Per-channel send queue (SQ) size.");
+
 static int srpt_get_u64_x(char *buffer, struct kernel_param *kp)
 {
 	return sprintf(buffer, "0x%016llx", *(u64 *)kp->arg);
@@ -295,6 +303,7 @@ static void srpt_get_ioc(struct srpt_port *sport, u32 slot,
 {
 	struct srpt_device *sdev = sport->sdev;
 	struct ib_dm_ioc_profile *iocp;
+	int send_queue_depth;
 
 	iocp = (struct ib_dm_ioc_profile *)mad->data;
 
@@ -310,6 +319,12 @@ static void srpt_get_ioc(struct srpt_port *sport, u32 slot,
 		return;
 	}
 
+	if (sdev->use_srq)
+		send_queue_depth = sdev->srq_size;
+	else
+		send_queue_depth = min(SRPT_RQ_SIZE,
+				       sdev->device->attrs.max_qp_wr);
+
 	memset(iocp, 0, sizeof(*iocp));
 	strcpy(iocp->id_string, SRPT_ID_STRING);
 	iocp->guid = cpu_to_be64(srpt_service_guid);
@@ -322,7 +337,7 @@ static void srpt_get_ioc(struct srpt_port *sport, u32 slot,
 	iocp->io_subclass = cpu_to_be16(SRP_IO_SUBCLASS);
 	iocp->protocol = cpu_to_be16(SRP_PROTOCOL);
 	iocp->protocol_version = cpu_to_be16(SRP_PROTOCOL_VERSION);
-	iocp->send_queue_depth = cpu_to_be16(sdev->srq_size);
+	iocp->send_queue_depth = cpu_to_be16(send_queue_depth);
 	iocp->rdma_read_depth = 4;
 	iocp->send_size = cpu_to_be32(srp_max_req_size);
 	iocp->rdma_size = cpu_to_be32(min(sport->port_attrib.srp_max_rdma_size,
@@ -686,6 +701,9 @@ static void srpt_free_ioctx_ring(struct srpt_ioctx **ioctx_ring,
 {
 	int i;
 
+	if (!ioctx_ring)
+		return;
+
 	for (i = 0; i < ring_size; ++i)
 		srpt_free_ioctx(sdev, ioctx_ring[i], dma_size, dir);
 	kfree(ioctx_ring);
@@ -757,7 +775,7 @@ static bool srpt_test_and_set_cmd_state(struct srpt_send_ioctx *ioctx,
 /**
  * srpt_post_recv() - Post an IB receive request.
  */
-static int srpt_post_recv(struct srpt_device *sdev,
+static int srpt_post_recv(struct srpt_device *sdev, struct srpt_rdma_ch *ch,
 			  struct srpt_recv_ioctx *ioctx)
 {
 	struct ib_sge list;
@@ -774,7 +792,10 @@ static int srpt_post_recv(struct srpt_device *sdev,
 	wr.sg_list = &list;
 	wr.num_sge = 1;
 
-	return ib_post_srq_recv(sdev->srq, &wr, &bad_wr);
+	if (sdev->use_srq)
+		return ib_post_srq_recv(sdev->srq, &wr, &bad_wr);
+	else
+		return ib_post_recv(ch->qp, &wr, &bad_wr);
 }
 
 /**
@@ -1517,7 +1538,7 @@ static void srpt_handle_new_iu(struct srpt_rdma_ch *ch,
 		break;
 	}
 
-	srpt_post_recv(ch->sport->sdev, recv_ioctx);
+	srpt_post_recv(ch->sport->sdev, ch, recv_ioctx);
 	return;
 
 out_wait:
@@ -1616,7 +1637,7 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch)
 	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;
+	int i, ret;
 
 	WARN_ON(ch->rq_size < 1);
 
@@ -1640,7 +1661,6 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch)
 		= (void(*)(struct ib_event *, void*))srpt_qp_event;
 	qp_init->send_cq = ch->cq;
 	qp_init->recv_cq = ch->cq;
-	qp_init->srq = sdev->srq;
 	qp_init->sq_sig_type = IB_SIGNAL_REQ_WR;
 	qp_init->qp_type = IB_QPT_RC;
 	/*
@@ -1654,6 +1674,12 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch)
 	qp_init->cap.max_rdma_ctxs = srp_sq_size / 2;
 	qp_init->cap.max_send_sge = min(attrs->max_sge, SRPT_MAX_SG_PER_WQE);
 	qp_init->port_num = ch->sport->port;
+	if (sdev->use_srq) {
+		qp_init->srq = sdev->srq;
+	} else {
+		qp_init->cap.max_recv_wr = ch->rq_size;
+		qp_init->cap.max_recv_sge = qp_init->cap.max_send_sge;
+	}
 
 	ch->qp = ib_create_qp(sdev->pd, qp_init);
 	if (IS_ERR(ch->qp)) {
@@ -1669,6 +1695,10 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch)
 		goto err_destroy_cq;
 	}
 
+	if (!sdev->use_srq)
+		for (i = 0; i < ch->rq_size; i++)
+			srpt_post_recv(sdev, ch, ch->ioctx_recv_ring[i]);
+
 	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",
@@ -1818,6 +1848,10 @@ static void srpt_release_channel_work(struct work_struct *w)
 			     ch->sport->sdev, ch->rq_size,
 			     ch->rsp_size, DMA_TO_DEVICE);
 
+	srpt_free_ioctx_ring((struct srpt_ioctx **)ch->ioctx_recv_ring,
+			     sdev, ch->rq_size,
+			     srp_max_req_size, DMA_FROM_DEVICE);
+
 	mutex_lock(&sdev->mutex);
 	list_del_init(&ch->list);
 	if (ch->release_done)
@@ -1975,6 +2009,19 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 		ch->ioctx_ring[i]->ch = ch;
 		list_add_tail(&ch->ioctx_ring[i]->free_list, &ch->free_list);
 	}
+	if (!sdev->use_srq) {
+		ch->ioctx_recv_ring = (struct srpt_recv_ioctx **)
+			srpt_alloc_ioctx_ring(ch->sport->sdev, ch->rq_size,
+					      sizeof(*ch->ioctx_recv_ring[0]),
+					      srp_max_req_size,
+					      DMA_FROM_DEVICE);
+		if (!ch->ioctx_recv_ring) {
+			pr_err("rejected SRP_LOGIN_REQ because creating a new QP RQ ring failed.\n");
+			rej->reason =
+			    cpu_to_be32(SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
+			goto free_ring;
+		}
+	}
 
 	ret = srpt_create_ch_ib(ch);
 	if (ret) {
@@ -1982,7 +2029,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 			      SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
 		pr_err("rejected SRP_LOGIN_REQ because creating"
 		       " a new RDMA channel failed.\n");
-		goto free_ring;
+		goto free_recv_ring;
 	}
 
 	ret = srpt_ch_qp_rtr(ch, ch->qp);
@@ -2073,6 +2120,11 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 destroy_ib:
 	srpt_destroy_ch_ib(ch);
 
+free_recv_ring:
+	srpt_free_ioctx_ring((struct srpt_ioctx **)ch->ioctx_recv_ring,
+			     ch->sport->sdev, ch->rq_size,
+			     srp_max_req_size, DMA_FROM_DEVICE);
+
 free_ring:
 	srpt_free_ioctx_ring((struct srpt_ioctx **)ch->ioctx_ring,
 			     ch->sport->sdev, ch->rq_size,
@@ -2502,20 +2554,38 @@ static void srpt_add_one(struct ib_device *device)
 	srq_attr.attr.srq_limit = 0;
 	srq_attr.srq_type = IB_SRQT_BASIC;
 
-	sdev->srq = ib_create_srq(sdev->pd, &srq_attr);
-	if (IS_ERR(sdev->srq))
-		goto err_pd;
+	sdev->srq = use_srq ? ib_create_srq(sdev->pd, &srq_attr) :
+		ERR_PTR(-ENOTSUPP);
+	if (IS_ERR(sdev->srq)) {
+		pr_debug("ib_create_srq() failed: %ld\n", PTR_ERR(sdev->srq));
+
+		/* SRQ not supported. */
+		sdev->use_srq = false;
+	} else {
+		pr_debug("create SRQ #wr= %d max_allow=%d dev= %s\n",
+			 sdev->srq_size, sdev->device->attrs.max_srq_wr,
+			 device->name);
 
-	pr_debug("%s: create SRQ #wr= %d max_allow=%d dev= %s\n",
-		 __func__, sdev->srq_size, sdev->device->attrs.max_srq_wr,
-		 device->name);
+		sdev->use_srq = true;
+
+		sdev->ioctx_ring = (struct srpt_recv_ioctx **)
+			srpt_alloc_ioctx_ring(sdev, sdev->srq_size,
+					      sizeof(*sdev->ioctx_ring[0]),
+					      srp_max_req_size,
+					      DMA_FROM_DEVICE);
+		if (!sdev->ioctx_ring)
+			goto err_pd;
+
+		for (i = 0; i < sdev->srq_size; ++i)
+			srpt_post_recv(sdev, NULL, sdev->ioctx_ring[i]);
+	}
 
 	if (!srpt_service_guid)
 		srpt_service_guid = be64_to_cpu(device->node_guid);
 
 	sdev->cm_id = ib_create_cm_id(device, srpt_cm_handler, sdev);
 	if (IS_ERR(sdev->cm_id))
-		goto err_srq;
+		goto err_ring;
 
 	/* print out target login information */
 	pr_debug("Target login info: id_ext=%016llx,ioc_guid=%016llx,"
@@ -2535,16 +2605,6 @@ static void srpt_add_one(struct ib_device *device)
 			      srpt_event_handler);
 	ib_register_event_handler(&sdev->event_handler);
 
-	sdev->ioctx_ring = (struct srpt_recv_ioctx **)
-		srpt_alloc_ioctx_ring(sdev, sdev->srq_size,
-				      sizeof(*sdev->ioctx_ring[0]),
-				      srp_max_req_size, DMA_FROM_DEVICE);
-	if (!sdev->ioctx_ring)
-		goto err_event;
-
-	for (i = 0; i < sdev->srq_size; ++i)
-		srpt_post_recv(sdev, sdev->ioctx_ring[i]);
-
 	WARN_ON(sdev->device->phys_port_cnt > ARRAY_SIZE(sdev->port));
 
 	for (i = 1; i <= sdev->device->phys_port_cnt; i++) {
@@ -2559,7 +2619,7 @@ static void srpt_add_one(struct ib_device *device)
 		if (srpt_refresh_port(sport)) {
 			pr_err("MAD registration failed for %s-%d.\n",
 			       sdev->device->name, i);
-			goto err_ring;
+			goto err_event;
 		}
 	}
 
@@ -2572,16 +2632,16 @@ static void srpt_add_one(struct ib_device *device)
 	pr_debug("added %s.\n", device->name);
 	return;
 
-err_ring:
-	srpt_free_ioctx_ring((struct srpt_ioctx **)sdev->ioctx_ring, sdev,
-			     sdev->srq_size, srp_max_req_size,
-			     DMA_FROM_DEVICE);
 err_event:
 	ib_unregister_event_handler(&sdev->event_handler);
 err_cm:
 	ib_destroy_cm_id(sdev->cm_id);
-err_srq:
-	ib_destroy_srq(sdev->srq);
+err_ring:
+	if (sdev->use_srq)
+		ib_destroy_srq(sdev->srq);
+	srpt_free_ioctx_ring((struct srpt_ioctx **)sdev->ioctx_ring, sdev,
+			     sdev->srq_size, srp_max_req_size,
+			     DMA_FROM_DEVICE);
 err_pd:
 	ib_dealloc_pd(sdev->pd);
 free_dev:
@@ -2625,12 +2685,12 @@ static void srpt_remove_one(struct ib_device *device, void *client_data)
 	spin_unlock(&srpt_dev_lock);
 	srpt_release_sdev(sdev);
 
-	ib_destroy_srq(sdev->srq);
-	ib_dealloc_pd(sdev->pd);
-
+	if (sdev->use_srq)
+		ib_destroy_srq(sdev->srq);
 	srpt_free_ioctx_ring((struct srpt_ioctx **)sdev->ioctx_ring, sdev,
 			     sdev->srq_size, srp_max_req_size, DMA_FROM_DEVICE);
-	sdev->ioctx_ring = NULL;
+	ib_dealloc_pd(sdev->pd);
+
 	kfree(sdev);
 }
 
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index 976e924d7400..e0dbd5444d5e 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -252,6 +252,7 @@ enum rdma_ch_state {
  * @free_list:     Head of list with free send I/O contexts.
  * @state:         channel state. See also enum rdma_ch_state.
  * @ioctx_ring:    Send ring.
+ * @ioctx_recv_ring: Receive I/O context ring.
  * @list:          Node for insertion in the srpt_device.rch_list list.
  * @cmd_wait_list: List of SCSI commands that arrived before the RTU event. This
  *                 list contains struct srpt_ioctx elements and is protected
@@ -281,6 +282,7 @@ struct srpt_rdma_ch {
 	struct list_head	free_list;
 	enum rdma_ch_state	state;
 	struct srpt_send_ioctx	**ioctx_ring;
+	struct srpt_recv_ioctx	**ioctx_recv_ring;
 	struct list_head	list;
 	struct list_head	cmd_wait_list;
 	struct se_session	*sess;
@@ -347,6 +349,7 @@ struct srpt_port {
  * @srq:           Per-HCA SRQ (shared receive queue).
  * @cm_id:         Connection identifier.
  * @srq_size:      SRQ size.
+ * @use_srq:       Whether or not to use SRQ.
  * @ioctx_ring:    Per-HCA SRQ.
  * @rch_list:      Per-device channel list -- see also srpt_rdma_ch.list.
  * @ch_releaseQ:   Enables waiting for removal from rch_list.
@@ -362,6 +365,7 @@ struct srpt_device {
 	struct ib_srq		*srq;
 	struct ib_cm_id		*cm_id;
 	int			srq_size;
+	bool			use_srq;
 	struct srpt_recv_ioctx	**ioctx_ring;
 	struct list_head	rch_list;
 	wait_queue_head_t	ch_releaseQ;
-- 
2.14.2

--
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/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1994 bytes --]

On Fri, Oct 06, 2017 at 02:42:39PM -0700, Bart Van Assche wrote:
> Although the non-SRQ mode needs more resources that mode has three
> advantages over SRQ:
> - It works with all RDMA adapters, even those that do not support
>   SRQ.
> - Posting WRs and polling WCs does not trigger lock contention
>   because only one thread at a time accesses a WR or WC queue in
>   non-SRQ mode.
> - The end-to-end flow control mechanism is used.
>
> From the IB spec:
>
>     C9-150.2.1: For QPs that are not associated with an SRQ, each HCA
>     receive queue shall generate end-to-end flow control credits. If
>     a QP is associated with an SRQ, the HCA receive queue shall not
>     generate end-to-end flow control credits.
>
> Signed-off-by: Bart Van Assche <bart.vanassche-Sjgp3cTcYWE@public.gmane.org>
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c | 130 +++++++++++++++++++++++++---------
>  drivers/infiniband/ulp/srpt/ib_srpt.h |   4 ++
>  2 files changed, 99 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 6cf95ad870cc..a21b7a96635c 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -75,11 +75,19 @@ module_param(srp_max_req_size, int, 0444);
>  MODULE_PARM_DESC(srp_max_req_size,
>  		 "Maximum size of SRP request messages in bytes.");
>
> +static bool use_srq;
> +module_param(use_srq, bool, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(use_srq, "Whether or not to use SRQ");
> +

It is a little bit strange to ask from user to decide if his adapter
supports SRQ or not.

It should be automatically.


>  static int srpt_srq_size = DEFAULT_SRPT_SRQ_SIZE;
>  module_param(srpt_srq_size, int, 0444);
>  MODULE_PARM_DESC(srpt_srq_size,
>  		 "Shared receive queue (SRQ) size.");
>
> +static int srpt_sq_size = DEF_SRPT_SQ_SIZE;
> +module_param(srpt_sq_size, int, 0444);
> +MODULE_PARM_DESC(srpt_sq_size, "Per-channel send queue (SQ) size.");
> +

Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Doug Ledford]

On Sun, 2017-10-08 at 13:03 +0300, Leon Romanovsky wrote:
> On Fri, Oct 06, 2017 at 02:42:39PM -0700, Bart Van Assche wrote:
> > Although the non-SRQ mode needs more resources that mode has three
> > advantages over SRQ:
> > - It works with all RDMA adapters, even those that do not support
> >   SRQ.
> > - Posting WRs and polling WCs does not trigger lock contention
> >   because only one thread at a time accesses a WR or WC queue in
> >   non-SRQ mode.
> > - The end-to-end flow control mechanism is used.
> > 
> > From the IB spec:
> > 
> >     C9-150.2.1: For QPs that are not associated with an SRQ, each
> > HCA
> >     receive queue shall generate end-to-end flow control credits.
> > If
> >     a QP is associated with an SRQ, the HCA receive queue shall not
> >     generate end-to-end flow control credits.
> > 
> > Signed-off-by: Bart Van Assche <bart.vanassche-Sjgp3cTcYWE@public.gmane.org>
> > ---
> >  drivers/infiniband/ulp/srpt/ib_srpt.c | 130
> > +++++++++++++++++++++++++---------
> >  drivers/infiniband/ulp/srpt/ib_srpt.h |   4 ++
> >  2 files changed, 99 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c
> > b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > index 6cf95ad870cc..a21b7a96635c 100644
> > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > @@ -75,11 +75,19 @@ module_param(srp_max_req_size, int, 0444);
> >  MODULE_PARM_DESC(srp_max_req_size,
> >  		 "Maximum size of SRP request messages in
> > bytes.");
> > 
> > +static bool use_srq;
> > +module_param(use_srq, bool, S_IRUGO | S_IWUSR);
> > +MODULE_PARM_DESC(use_srq, "Whether or not to use SRQ");
> > +
> 
> It is a little bit strange to ask from user to decide if his adapter
> supports SRQ or not.
> 
> It should be automatically.

I think Bart's intent is that the driver not use SRQ as the default
behavior even if the adapter supports it, so querying the adapter for
support and enabling it if it exists would not achieve his desired
result.  This would then be used to override that behavior.  Is that
correct Bart?

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
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/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Bart Van Assche]

On Mon, 2017-10-09 at 12:56 -0400, Doug Ledford wrote:
> On Sun, 2017-10-08 at 13:03 +0300, Leon Romanovsky wrote:
> > It is a little bit strange to ask from user to decide if his adapter
> > supports SRQ or not.
> > 
> > It should be automatically.
> 
> I think Bart's intent is that the driver not use SRQ as the default
> behavior even if the adapter supports it, so querying the adapter for
> support and enabling it if it exists would not achieve his desired
> result.  This would then be used to override that behavior.  Is that
> correct Bart?

Hello Leon and Doug,

The changes realized by this patch are:
- Instead of using SRQ as default, use non-SRQ mode as default.
- If SRQ has been chosen as default, and if SRQ is not supported, fall back
  to non-SRQ mode (see also the if (IS_ERR(sdev->srq)) ... code).

Please let me know if you have any further questions about this patch.

Bart.

Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Doug Ledford]

On Mon, 2017-10-09 at 17:01 +0000, Bart Van Assche wrote:
> On Mon, 2017-10-09 at 12:56 -0400, Doug Ledford wrote:
> > On Sun, 2017-10-08 at 13:03 +0300, Leon Romanovsky wrote:
> > > It is a little bit strange to ask from user to decide if his
> > > adapter
> > > supports SRQ or not.
> > > 
> > > It should be automatically.
> > 
> > I think Bart's intent is that the driver not use SRQ as the default
> > behavior even if the adapter supports it, so querying the adapter
> > for
> > support and enabling it if it exists would not achieve his desired
> > result.  This would then be used to override that behavior.  Is
> > that
> > correct Bart?
> 
> Hello Leon and Doug,
> 
> The changes realized by this patch are:
> - Instead of using SRQ as default, use non-SRQ mode as default.
> - If SRQ has been chosen as default, and if SRQ is not supported,
> fall back
>   to non-SRQ mode (see also the if (IS_ERR(sdev->srq)) ... code).
> 
> Please let me know if you have any further questions about this
> patch.

Nope, I'm good.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
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/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]

On Mon, Oct 09, 2017 at 05:01:33PM +0000, Bart Van Assche wrote:
> On Mon, 2017-10-09 at 12:56 -0400, Doug Ledford wrote:
> > On Sun, 2017-10-08 at 13:03 +0300, Leon Romanovsky wrote:
> > > It is a little bit strange to ask from user to decide if his adapter
> > > supports SRQ or not.
> > >
> > > It should be automatically.
> >
> > I think Bart's intent is that the driver not use SRQ as the default
> > behavior even if the adapter supports it, so querying the adapter for
> > support and enabling it if it exists would not achieve his desired
> > result.  This would then be used to override that behavior.  Is that
> > correct Bart?
>
> Hello Leon and Doug,
>
> The changes realized by this patch are:
> - Instead of using SRQ as default, use non-SRQ mode as default.
> - If SRQ has been chosen as default, and if SRQ is not supported, fall back
>   to non-SRQ mode (see also the if (IS_ERR(sdev->srq)) ... code).
>
> Please let me know if you have any further questions about this patch.

Yes, in case HCA supports SRQ, when do you set that module parameter?

In the commit message, you mentioned disadvantages of using SRQ is a
default and among them - locks contention, which can be changed in the
future. Won't it mean that users stuck with current default, because
change of default will "break" their scripts?

Setting visible to user default won't allow us to change SRP behavior in
the future.

I wouldn't recommend to make such option accessible by users.

Thanks

>
> Bart.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Doug Ledford]

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

On 10/10/2017 12:14 AM, Leon Romanovsky wrote:
> On Mon, Oct 09, 2017 at 05:01:33PM +0000, Bart Van Assche wrote:
>> On Mon, 2017-10-09 at 12:56 -0400, Doug Ledford wrote:
>>> On Sun, 2017-10-08 at 13:03 +0300, Leon Romanovsky wrote:
>>>> It is a little bit strange to ask from user to decide if his adapter
>>>> supports SRQ or not.
>>>>
>>>> It should be automatically.
>>>
>>> I think Bart's intent is that the driver not use SRQ as the default
>>> behavior even if the adapter supports it, so querying the adapter for
>>> support and enabling it if it exists would not achieve his desired
>>> result.  This would then be used to override that behavior.  Is that
>>> correct Bart?
>>
>> Hello Leon and Doug,
>>
>> The changes realized by this patch are:
>> - Instead of using SRQ as default, use non-SRQ mode as default.
>> - If SRQ has been chosen as default, and if SRQ is not supported, fall back
>>   to non-SRQ mode (see also the if (IS_ERR(sdev->srq)) ... code).
>>
>> Please let me know if you have any further questions about this patch.
> 
> Yes, in case HCA supports SRQ, when do you set that module parameter?

You set it in your /etc/modprobe.d/ib_srp.conf file or the equivalent in
your OS.

> In the commit message, you mentioned disadvantages of using SRQ is a
> default and among them - locks contention, which can be changed in the
> future. Won't it mean that users stuck with current default, because
> change of default will "break" their scripts?

No, it won't.  If you change the default, you don't remove the variable,
you just change what its setting is.  Then existing modprobe.d files
become redundant, but nothing breaks.  People that don't want the new
setting add a new file to the modprobe.d directory to change the option.

> Setting visible to user default won't allow us to change SRP behavior in
> the future.

No it doesn't.

> I wouldn't recommend to make such option accessible by users.
> 
> Thanks
> 
>>
>> Bart.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


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

Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2564 bytes --]

On Tue, Oct 10, 2017 at 10:34:19AM -0400, Doug Ledford wrote:
> On 10/10/2017 12:14 AM, Leon Romanovsky wrote:
> > On Mon, Oct 09, 2017 at 05:01:33PM +0000, Bart Van Assche wrote:
> >> On Mon, 2017-10-09 at 12:56 -0400, Doug Ledford wrote:
> >>> On Sun, 2017-10-08 at 13:03 +0300, Leon Romanovsky wrote:
> >>>> It is a little bit strange to ask from user to decide if his adapter
> >>>> supports SRQ or not.
> >>>>
> >>>> It should be automatically.
> >>>
> >>> I think Bart's intent is that the driver not use SRQ as the default
> >>> behavior even if the adapter supports it, so querying the adapter for
> >>> support and enabling it if it exists would not achieve his desired
> >>> result.  This would then be used to override that behavior.  Is that
> >>> correct Bart?
> >>
> >> Hello Leon and Doug,
> >>
> >> The changes realized by this patch are:
> >> - Instead of using SRQ as default, use non-SRQ mode as default.
> >> - If SRQ has been chosen as default, and if SRQ is not supported, fall back
> >>   to non-SRQ mode (see also the if (IS_ERR(sdev->srq)) ... code).
> >>
> >> Please let me know if you have any further questions about this patch.
> >
> > Yes, in case HCA supports SRQ, when do you set that module parameter?
>
> You set it in your /etc/modprobe.d/ib_srp.conf file or the equivalent in
> your OS.

Doug,
But my question was "when" and not "how". When should I set this
parameter to true?

>
> > In the commit message, you mentioned disadvantages of using SRQ is a
> > default and among them - locks contention, which can be changed in the
> > future. Won't it mean that users stuck with current default, because
> > change of default will "break" their scripts?
>
> No, it won't.  If you change the default, you don't remove the variable,
> you just change what its setting is.  Then existing modprobe.d files
> become redundant, but nothing breaks.  People that don't want the new
> setting add a new file to the modprobe.d directory to change the option.

Not accurate, now I won't set any parameter because I'm relying on the
fact that the default is without SRQ. Once the default will be changed,
it will break my assumption.

>
> > Setting visible to user default won't allow us to change SRP behavior in
> > the future.
>
> No it doesn't.
>
> > I wouldn't recommend to make such option accessible by users.
> >
> > Thanks
> >
> >>
> >> Bart.
>
>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG Key ID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Doug Ledford]

On Tue, 2017-10-10 at 18:00 +0300, Leon Romanovsky wrote:
> On Tue, Oct 10, 2017 at 10:34:19AM -0400, Doug Ledford wrote:
> > On 10/10/2017 12:14 AM, Leon Romanovsky wrote:
> > > 
> Doug,
> But my question was "when" and not "how". When should I set this
> parameter to true?

Whenever you want.  When do you set the maximum TCP window size to 4MB?
 If you have an issue with things being one way, you try another. 
Things like this are system specific and often there is no universal
answer, especially since the answer here could very easily depend on
things like how many targets you are connecting to, how fast those
targets are, etc.

> > 
> > > In the commit message, you mentioned disadvantages of using SRQ
> > > is a
> > > default and among them - locks contention, which can be changed
> > > in the
> > > future. Won't it mean that users stuck with current default,
> > > because
> > > change of default will "break" their scripts?
> > 
> > No, it won't.  If you change the default, you don't remove the
> > variable,
> > you just change what its setting is.  Then existing modprobe.d
> > files
> > become redundant, but nothing breaks.  People that don't want the
> > new
> > setting add a new file to the modprobe.d directory to change the
> > option.
> 
> Not accurate, now I won't set any parameter because I'm relying on
> the
> fact that the default is without SRQ. Once the default will be
> changed,
> it will break my assumption.

So?  Your assumptions are never a reason to prevent ongoing
development.  By this argument, we would never have been able to turn
on SCSI Multi-queue by default because when it was first added to the
kernel it was off by default and people might have made an assumption
that we would later break when we turned it on by default.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
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/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2615 bytes --]

On Tue, Oct 10, 2017 at 11:13:22AM -0400, Doug Ledford wrote:
> On Tue, 2017-10-10 at 18:00 +0300, Leon Romanovsky wrote:
> > On Tue, Oct 10, 2017 at 10:34:19AM -0400, Doug Ledford wrote:
> > > On 10/10/2017 12:14 AM, Leon Romanovsky wrote:
> > > >
> > Doug,
> > But my question was "when" and not "how". When should I set this
> > parameter to true?
>
> Whenever you want.  When do you set the maximum TCP window size to 4MB?
>  If you have an issue with things being one way, you try another.
> Things like this are system specific and often there is no universal
> answer, especially since the answer here could very easily depend on
> things like how many targets you are connecting to, how fast those
> targets are, etc.

Bart clearly mentioned disadvantages of XRQ and left me wonder why user
needs to enable it anyway. This is what I'm asking and this is what I'm
hoping to see in the commit message.

I know little about SRP and by asking the questions, I'm trying to fill
the gaps.

>
> > >
> > > > In the commit message, you mentioned disadvantages of using SRQ
> > > > is a
> > > > default and among them - locks contention, which can be changed
> > > > in the
> > > > future. Won't it mean that users stuck with current default,
> > > > because
> > > > change of default will "break" their scripts?
> > >
> > > No, it won't.  If you change the default, you don't remove the
> > > variable,
> > > you just change what its setting is.  Then existing modprobe.d
> > > files
> > > become redundant, but nothing breaks.  People that don't want the
> > > new
> > > setting add a new file to the modprobe.d directory to change the
> > > option.
> >
> > Not accurate, now I won't set any parameter because I'm relying on
> > the
> > fact that the default is without SRQ. Once the default will be
> > changed,
> > it will break my assumption.
>
> So?  Your assumptions are never a reason to prevent ongoing
> development.  By this argument, we would never have been able to turn
> on SCSI Multi-queue by default because when it was first added to the
> kernel it was off by default and people might have made an assumption
> that we would later break when we turned it on by default.

I'm not fully understand the example, The transition to MQ was long
process and one of the difficulties was requirement to smoothly transit
all devices in such way that users won't be affected. This is why it was
disabled by default.

Thanks

>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Bart Van Assche]

On Tue, 2017-10-10 at 18:44 +0300, Leon Romanovsky wrote:
> Bart clearly mentioned disadvantages of XRQ and left me wonder why user
> needs to enable it anyway. This is what I'm asking and this is what I'm
> hoping to see in the commit message.

I assume that you meant SRQ instead of XRQ? For HCA's that support SRQ a
choice has to be made between the lower memory usage of SRQ or the higher
performance of RC. I think only the user can make that choice. Hence the
new kernel module parameter.

Bart.

Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Jason Gunthorpe]

On Tue, Oct 10, 2017 at 04:04:34PM +0000, Bart Van Assche wrote:
> On Tue, 2017-10-10 at 18:44 +0300, Leon Romanovsky wrote:
> > Bart clearly mentioned disadvantages of XRQ and left me wonder why user
> > needs to enable it anyway. This is what I'm asking and this is what I'm
> > hoping to see in the commit message.
> 
> I assume that you meant SRQ instead of XRQ? For HCA's that support SRQ a
> choice has to be made between the lower memory usage of SRQ or the higher
> performance of RC. I think only the user can make that choice. Hence the
> new kernel module parameter.

considering our general dislike of module parameters, could you
achieve this via some communicationfrom srp_daemon instead?

Perhaps even on a per target basis?

Jason
--
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/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Bart Van Assche]

On Tue, 2017-10-10 at 11:04 -0600, Jason Gunthorpe wrote:
> considering our general dislike of module parameters, could you
> achieve this via some communication from srp_daemon instead?
> 
> Perhaps even on a per target basis?

The SRP target driver already creates one /sys/kernel/config/target/srpt/$GUID
directory per HCA port. How about adding an attribute to that configfs directory?

Bart.

Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

On Tue, Oct 10, 2017 at 05:10:55PM +0000, Bart Van Assche wrote:
> On Tue, 2017-10-10 at 11:04 -0600, Jason Gunthorpe wrote:
> > considering our general dislike of module parameters, could you
> > achieve this via some communication from srp_daemon instead?
> >
> > Perhaps even on a per target basis?
>
> The SRP target driver already creates one /sys/kernel/config/target/srpt/$GUID
> directory per HCA port. How about adding an attribute to that configfs directory?

Sounds good.

Thanks

>
> Bart.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ

[Doug Ledford]

On Tue, 2017-10-10 at 18:44 +0300, Leon Romanovsky wrote:
> On Tue, Oct 10, 2017 at 11:13:22AM -0400, Doug Ledford wrote:
> > On Tue, 2017-10-10 at 18:00 +0300, Leon Romanovsky wrote:
> > > On Tue, Oct 10, 2017 at 10:34:19AM -0400, Doug Ledford wrote:
> > > > On 10/10/2017 12:14 AM, Leon Romanovsky wrote:
> > > > > 
> > > 
> > > Doug,
> > > But my question was "when" and not "how". When should I set this
> > > parameter to true?
> > 
> > Whenever you want.  When do you set the maximum TCP window size to
> > 4MB?
> >  If you have an issue with things being one way, you try another.
> > Things like this are system specific and often there is no
> > universal
> > answer, especially since the answer here could very easily depend
> > on
> > things like how many targets you are connecting to, how fast those
> > targets are, etc.
> 
> Bart clearly mentioned disadvantages of XRQ and left me wonder why
> user
> needs to enable it anyway. This is what I'm asking and this is what
> I'm
> hoping to see in the commit message.

That's more than a commit message needs to be IMO.

> I know little about SRP and by asking the questions, I'm trying to
> fill
> the gaps.

It's not really SRP related.  As Bart points out in his next message,
it's a tradeoff between lower resource consumption and higher
contention.  It's a general tradeoff that is common to RDMA.

> > 
> > > > 
> > > > > In the commit message, you mentioned disadvantages of using
> > > > > SRQ
> > > > > is a
> > > > > default and among them - locks contention, which can be
> > > > > changed
> > > > > in the
> > > > > future. Won't it mean that users stuck with current default,
> > > > > because
> > > > > change of default will "break" their scripts?
> > > > 
> > > > No, it won't.  If you change the default, you don't remove the
> > > > variable,
> > > > you just change what its setting is.  Then existing modprobe.d
> > > > files
> > > > become redundant, but nothing breaks.  People that don't want
> > > > the
> > > > new
> > > > setting add a new file to the modprobe.d directory to change
> > > > the
> > > > option.
> > > 
> > > Not accurate, now I won't set any parameter because I'm relying
> > > on
> > > the
> > > fact that the default is without SRQ. Once the default will be
> > > changed,
> > > it will break my assumption.
> > 
> > So?  Your assumptions are never a reason to prevent ongoing
> > development.  By this argument, we would never have been able to
> > turn
> > on SCSI Multi-queue by default because when it was first added to
> > the
> > kernel it was off by default and people might have made an
> > assumption
> > that we would later break when we turned it on by default.
> 
> I'm not fully understand the example, The transition to MQ was long
> process and one of the difficulties was requirement to smoothly
> transit
> all devices in such way that users won't be affected.

The difficulty was making sure it was robust.

>  This is why it was
> disabled by default.

It was off by default because they wanted it to get testing and bake
time before enabling it by default.  However, the reason they did it is
really immaterial to my point: we can change defaults and that is not
considered "breaking user's assumptions".  The only way we would break
user's system in this specific situation is if we removed the module
option.  That would break working setups that had a modprobe.d file to
set the variable.  Changing what the variable is by default doesn't do
that.

> Thanks
> 
> > 
> > --
> > Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >     GPG KeyID: B826A3330E572FDD
> >     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57
> > 2FDD
> > 
-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
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/7] IB/srp: Remove second argument of srp_destroy_qp()

[Bart Van Assche]

This patch does not change any functionality.

Signed-off-by: Bart Van Assche <bart.vanassche-Sjgp3cTcYWE@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 8ceb4bb011f1..96750ad253c4 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -464,20 +464,20 @@ static struct srp_fr_pool *srp_alloc_fr_pool(struct srp_target_port *target)
 
 /**
  * srp_destroy_qp() - destroy an RDMA queue pair
- * @qp: RDMA queue pair.
+ * @ch: SRP RDMA channel.
  *
  * Drain the qp before destroying it.  This avoids that the receive
  * completion handler can access the queue pair while it is
  * being destroyed.
  */
-static void srp_destroy_qp(struct srp_rdma_ch *ch, struct ib_qp *qp)
+static void srp_destroy_qp(struct srp_rdma_ch *ch)
 {
 	spin_lock_irq(&ch->lock);
 	ib_process_cq_direct(ch->send_cq, -1);
 	spin_unlock_irq(&ch->lock);
 
-	ib_drain_qp(qp);
-	ib_destroy_qp(qp);
+	ib_drain_qp(ch->qp);
+	ib_destroy_qp(ch->qp);
 }
 
 static int srp_create_ch_ib(struct srp_rdma_ch *ch)
@@ -550,7 +550,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 	}
 
 	if (ch->qp)
-		srp_destroy_qp(ch, ch->qp);
+		srp_destroy_qp(ch);
 	if (ch->recv_cq)
 		ib_free_cq(ch->recv_cq);
 	if (ch->send_cq)
@@ -617,7 +617,7 @@ static void srp_free_ch_ib(struct srp_target_port *target,
 			ib_destroy_fmr_pool(ch->fmr_pool);
 	}
 
-	srp_destroy_qp(ch, ch->qp);
+	srp_destroy_qp(ch);
 	ib_free_cq(ch->send_cq);
 	ib_free_cq(ch->recv_cq);
 
-- 
2.14.2

--
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/7] IB/srp: Remove second argument of srp_destroy_qp()

[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

[PATCH 6/7] IB/srp: Cache global rkey

[Bart Van Assche]

This is a micro-optimization for the hot path.

Signed-off-by: Bart Van Assche <bart.vanassche-Sjgp3cTcYWE@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 27 ++++++++++++++-------------
 drivers/infiniband/ulp/srp/ib_srp.h |  3 ++-
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 96750ad253c4..463c3ed440eb 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1292,7 +1292,6 @@ static int srp_map_finish_fmr(struct srp_map_state *state,
 {
 	struct srp_target_port *target = ch->target;
 	struct srp_device *dev = target->srp_host->srp_dev;
-	struct ib_pd *pd = target->pd;
 	struct ib_pool_fmr *fmr;
 	u64 io_addr = 0;
 
@@ -1308,9 +1307,9 @@ static int srp_map_finish_fmr(struct srp_map_state *state,
 	if (state->npages == 0)
 		return 0;
 
-	if (state->npages == 1 && (pd->flags & IB_PD_UNSAFE_GLOBAL_RKEY)) {
+	if (state->npages == 1 && target->global_rkey) {
 		srp_map_desc(state, state->base_dma_addr, state->dma_len,
-			     pd->unsafe_global_rkey);
+			     target->global_rkey);
 		goto reset_state;
 	}
 
@@ -1350,7 +1349,6 @@ static int srp_map_finish_fr(struct srp_map_state *state,
 {
 	struct srp_target_port *target = ch->target;
 	struct srp_device *dev = target->srp_host->srp_dev;
-	struct ib_pd *pd = target->pd;
 	struct ib_send_wr *bad_wr;
 	struct ib_reg_wr wr;
 	struct srp_fr_desc *desc;
@@ -1366,12 +1364,12 @@ static int srp_map_finish_fr(struct srp_map_state *state,
 
 	WARN_ON_ONCE(!dev->use_fast_reg);
 
-	if (sg_nents == 1 && (pd->flags & IB_PD_UNSAFE_GLOBAL_RKEY)) {
+	if (sg_nents == 1 && target->global_rkey) {
 		unsigned int sg_offset = sg_offset_p ? *sg_offset_p : 0;
 
 		srp_map_desc(state, sg_dma_address(state->sg) + sg_offset,
 			     sg_dma_len(state->sg) - sg_offset,
-			     pd->unsafe_global_rkey);
+			     target->global_rkey);
 		if (sg_offset_p)
 			*sg_offset_p = 0;
 		return 1;
@@ -1533,7 +1531,7 @@ static int srp_map_sg_dma(struct srp_map_state *state, struct srp_rdma_ch *ch,
 	for_each_sg(scat, sg, count, i) {
 		srp_map_desc(state, ib_sg_dma_address(dev->dev, sg),
 			     ib_sg_dma_len(dev->dev, sg),
-			     target->pd->unsafe_global_rkey);
+			     target->global_rkey);
 	}
 
 	return 0;
@@ -1631,7 +1629,6 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 			struct srp_request *req)
 {
 	struct srp_target_port *target = ch->target;
-	struct ib_pd *pd = target->pd;
 	struct scatterlist *scat;
 	struct srp_cmd *cmd = req->cmd->buf;
 	int len, nents, count, ret;
@@ -1667,7 +1664,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 	fmt = SRP_DATA_DESC_DIRECT;
 	len = sizeof (struct srp_cmd) +	sizeof (struct srp_direct_buf);
 
-	if (count == 1 && (pd->flags & IB_PD_UNSAFE_GLOBAL_RKEY)) {
+	if (count == 1 && target->global_rkey) {
 		/*
 		 * The midlayer only generated a single gather/scatter
 		 * entry, or DMA mapping coalesced everything to a
@@ -1677,7 +1674,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 		struct srp_direct_buf *buf = (void *) cmd->add_data;
 
 		buf->va  = cpu_to_be64(ib_sg_dma_address(ibdev, scat));
-		buf->key = cpu_to_be32(pd->unsafe_global_rkey);
+		buf->key = cpu_to_be32(target->global_rkey);
 		buf->len = cpu_to_be32(ib_sg_dma_len(ibdev, scat));
 
 		req->nmdesc = 0;
@@ -1748,14 +1745,14 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 	memcpy(indirect_hdr->desc_list, req->indirect_desc,
 	       count * sizeof (struct srp_direct_buf));
 
-	if (!(pd->flags & IB_PD_UNSAFE_GLOBAL_RKEY)) {
+	if (!target->global_rkey) {
 		ret = srp_map_idb(ch, req, state.gen.next, state.gen.end,
 				  idb_len, &idb_rkey);
 		if (ret < 0)
 			goto unmap;
 		req->nmdesc++;
 	} else {
-		idb_rkey = cpu_to_be32(pd->unsafe_global_rkey);
+		idb_rkey = cpu_to_be32(target->global_rkey);
 	}
 
 	indirect_hdr->table_desc.va = cpu_to_be64(req->indirect_dma_addr);
@@ -3331,8 +3328,8 @@ static ssize_t srp_create_target(struct device *dev,
 	target->io_class	= SRP_REV16A_IB_IO_CLASS;
 	target->scsi_host	= target_host;
 	target->srp_host	= host;
-	target->pd		= host->srp_dev->pd;
 	target->lkey		= host->srp_dev->pd->local_dma_lkey;
+	target->global_rkey	= host->srp_dev->global_rkey;
 	target->cmd_sg_cnt	= cmd_sg_entries;
 	target->sg_tablesize	= indirect_sg_entries ? : cmd_sg_entries;
 	target->allow_ext_sg	= allow_ext_sg;
@@ -3651,6 +3648,10 @@ static void srp_add_one(struct ib_device *device)
 	if (IS_ERR(srp_dev->pd))
 		goto free_dev;
 
+	if (flags & IB_PD_UNSAFE_GLOBAL_RKEY) {
+		srp_dev->global_rkey = srp_dev->pd->unsafe_global_rkey;
+		WARN_ON_ONCE(srp_dev->global_rkey == 0);
+	}
 
 	for (p = rdma_start_port(device); p <= rdma_end_port(device); ++p) {
 		host = srp_add_port(srp_dev, p);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index ab9077b81d5a..a814f5ef16f9 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -90,6 +90,7 @@ struct srp_device {
 	struct list_head	dev_list;
 	struct ib_device       *dev;
 	struct ib_pd	       *pd;
+	u32			global_rkey;
 	u64			mr_page_mask;
 	int			mr_page_size;
 	int			mr_max_size;
@@ -179,7 +180,7 @@ struct srp_target_port {
 	spinlock_t		lock;
 
 	/* read only in the hot path */
-	struct ib_pd		*pd;
+	u32			global_rkey;
 	struct srp_rdma_ch	*ch;
 	u32			ch_count;
 	u32			lkey;
-- 
2.14.2

--
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 7/7] IB/srp: Make CM timeout dependent on subnet timeout

[Bart Van Assche]

For small networks it is safe to reduce the subnet timeout from
its default value (18 for opensm) to 16. Make the SRP CM timeout
dependent on the subnet timeout such that decreasing the subnet
timeout also causes SRP failover and failback to occur faster.

Signed-off-by: Bart Van Assche <bart.vanassche-Sjgp3cTcYWE@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 463c3ed440eb..972d4b3c5223 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -711,6 +711,23 @@ static int srp_lookup_path(struct srp_rdma_ch *ch)
 	return ret;
 }
 
+static u8 srp_get_subnet_timeout(struct srp_host *host)
+{
+	struct ib_port_attr attr;
+	int ret;
+	u8 subnet_timeout = 18;
+
+	ret = ib_query_port(host->srp_dev->dev, host->port, &attr);
+	if (ret == 0)
+		subnet_timeout = attr.subnet_timeout;
+
+	if (unlikely(subnet_timeout < 15))
+		pr_warn("%s: subnet timeout %d may cause SRP login to fail.\n",
+			dev_name(&host->srp_dev->dev->dev), subnet_timeout);
+
+	return subnet_timeout;
+}
+
 static int srp_send_req(struct srp_rdma_ch *ch, bool multich)
 {
 	struct srp_target_port *target = ch->target;
@@ -719,6 +736,9 @@ static int srp_send_req(struct srp_rdma_ch *ch, bool multich)
 		struct srp_login_req   priv;
 	} *req = NULL;
 	int status;
+	u8 subnet_timeout;
+
+	subnet_timeout = srp_get_subnet_timeout(target->srp_host);
 
 	req = kzalloc(sizeof *req, GFP_KERNEL);
 	if (!req)
@@ -741,8 +761,8 @@ static int srp_send_req(struct srp_rdma_ch *ch, bool multich)
 	 * module parameters if anyone cared about setting them.
 	 */
 	req->param.responder_resources	      = 4;
-	req->param.remote_cm_response_timeout = 20;
-	req->param.local_cm_response_timeout  = 20;
+	req->param.remote_cm_response_timeout = subnet_timeout + 2;
+	req->param.local_cm_response_timeout  = subnet_timeout + 2;
 	req->param.retry_count                = target->tl_retry_count;
 	req->param.rnr_retry_count 	      = 7;
 	req->param.max_cm_retries 	      = 15;
-- 
2.14.2

--
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 7/7] IB/srp: Make CM timeout dependent on subnet timeout

[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