diff mbox

Kernel crash with target-pending/for-next

Message ID 1496774537.2692.19.camel@sandisk.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bart Van Assche June 6, 2017, 6:42 p.m. UTC
On Fri, 2017-06-02 at 21:19 -0700, Nicholas A. Bellinger wrote:
> Can I have a look at the full debug with the missing
> srpt_close_sessions() messages to see if it's being called twice for the
> same se_session, and the code changes against v4.12-rc3 you're testing
> with that account for the ~50 lines offset..?

Hello Nic,

I should have mentioned that I had also merged my branch with pending ib_srpt
patches in the kernel tree that I used in my test. Since these patches are
almost six months old, have not caused trouble before and are not related to
session shutdown I don't think these patches are related to this crash. Anyway,
I have attached these patches to this e-mail.

It's probably easier for you if you can reproduce the reported crash yourself
instead of having to rely on me for testing? This is the test that triggered
the reported crash:

# git clone https://github.com/bvanassche/srp-test.git
# cd srp-test
# ./run-tests -d -r 30 -e none

Bart.

Comments

Nicholas A. Bellinger June 9, 2017, 5:34 a.m. UTC | #1
On Tue, 2017-06-06 at 18:42 +0000, Bart Van Assche wrote:
> On Fri, 2017-06-02 at 21:19 -0700, Nicholas A. Bellinger wrote:
> > Can I have a look at the full debug with the missing
> > srpt_close_sessions() messages to see if it's being called twice for the
> > same se_session, and the code changes against v4.12-rc3 you're testing
> > with that account for the ~50 lines offset..?
> 
> Hello Nic,
> 
> I should have mentioned that I had also merged my branch with pending ib_srpt
> patches in the kernel tree that I used in my test. Since these patches are
> almost six months old, have not caused trouble before and are not related to
> session shutdown I don't think these patches are related to this crash. Anyway,
> I have attached these patches to this e-mail.
> 
> It's probably easier for you if you can reproduce the reported crash yourself
> instead of having to rely on me for testing?

Heh, no.

You are the ib_srpt maintainer, so I'm going to rely on you exactly for
that.  :)

So I'd like to get to the bottom of this because:

1) The last patch didn't actually change any behavior.
2) HCH's change in v4.7+ to always drain every session by restarting the
list walk in target_shutdown_sessions() breaks the case where the
se_node_acl->queue_depth.

That said, I'd appreciate it if you could figure out why a patch that
doesn't actually change any behavior is causing this to appear.

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche June 22, 2017, 9:14 p.m. UTC | #2
On 06/08/17 22:34, Nicholas A. Bellinger wrote:
> On Tue, 2017-06-06 at 18:42 +0000, Bart Van Assche wrote:
>> On Fri, 2017-06-02 at 21:19 -0700, Nicholas A. Bellinger wrote:
>>> Can I have a look at the full debug with the missing
>>> srpt_close_sessions() messages to see if it's being called twice for the
>>> same se_session, and the code changes against v4.12-rc3 you're testing
>>> with that account for the ~50 lines offset..?
>>
>> Hello Nic,
>>
>> I should have mentioned that I had also merged my branch with pending ib_srpt
>> patches in the kernel tree that I used in my test. Since these patches are
>> almost six months old, have not caused trouble before and are not related to
>> session shutdown I don't think these patches are related to this crash. Anyway,
>> I have attached these patches to this e-mail.
>>
>> It's probably easier for you if you can reproduce the reported crash yourself
>> instead of having to rely on me for testing?
> 
> Heh, no.
> 
> You are the ib_srpt maintainer, so I'm going to rely on you exactly for
> that.  :)

Please do not expect that I will do that. Anyone (you in this case) who
wants to change something in the target core is responsible for making
sure that such a change does not break any existing target driver.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicholas A. Bellinger July 7, 2017, 5:59 a.m. UTC | #3
On Thu, 2017-06-22 at 14:14 -0700, Bart Van Assche wrote:
> On 06/08/17 22:34, Nicholas A. Bellinger wrote:
> > On Tue, 2017-06-06 at 18:42 +0000, Bart Van Assche wrote:
> >> On Fri, 2017-06-02 at 21:19 -0700, Nicholas A. Bellinger wrote:
> >>> Can I have a look at the full debug with the missing
> >>> srpt_close_sessions() messages to see if it's being called twice for the
> >>> same se_session, and the code changes against v4.12-rc3 you're testing
> >>> with that account for the ~50 lines offset..?
> >>
> >> Hello Nic,
> >>
> >> I should have mentioned that I had also merged my branch with pending ib_srpt
> >> patches in the kernel tree that I used in my test. Since these patches are
> >> almost six months old, have not caused trouble before and are not related to
> >> session shutdown I don't think these patches are related to this crash. Anyway,
> >> I have attached these patches to this e-mail.
> >>
> >> It's probably easier for you if you can reproduce the reported crash yourself
> >> instead of having to rely on me for testing?
> > 
> > Heh, no.
> > 
> > You are the ib_srpt maintainer, so I'm going to rely on you exactly for
> > that.  :)
> 
> Please do not expect that I will do that. Anyone (you in this case) who
> wants to change something in the target core is responsible for making
> sure that such a change does not break any existing target driver.
> 

That is unfortunate.

However, keep in mind this is a regression, so it would at least be nice
if you would take to time to figure out why a patch that doesn't change
actual logic is causing ib_srpt problems, as it might be indicative of a
deeper issue.

In any event, I'll drop this patch for -rc1 and just add a separate
function specific to se_node_acl->queue_depth change that doesn't effect
any other code.

Thanks for the heads up.

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

From ff0062fce95fd82459ce9f7040b5f6a0efba2978 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Wed, 14 Dec 2016 13:07:14 +0100
Subject: [PATCH 4/4] IB/srpt: Change default behavior from using SRQ to not
 using SRQ

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@sandisk.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 119 ++++++++++++++++++++++++----------
 drivers/infiniband/ulp/srpt/ib_srpt.h |   4 ++
 2 files changed, 89 insertions(+), 34 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 3a376b53f2c1..b75d48d1af17 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",
@@ -1975,6 +2005,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) {
@@ -2502,20 +2545,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);
+
+		sdev->use_srq = true;
 
-	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->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,"
@@ -2536,16 +2597,6 @@  static void srpt_add_one(struct ib_device *device)
 	if (ib_register_event_handler(&sdev->event_handler))
 		goto err_cm;
 
-	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++) {
@@ -2560,7 +2611,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;
 		}
 	}
 
@@ -2573,16 +2624,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:
@@ -2626,12 +2677,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 d1ec5b4378cc..7d0acfe1cf34 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.12.2