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.
From 988ba32c6437af74aa1873a582f0618ba0bdcd12 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
Date: Fri, 9 Dec 2016 08:35:05 -0800
Subject: [PATCH 1/4] IB/srpt: Do not accept invalid initiator port names
Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
---
drivers/infiniband/ulp/srpt/ib_srpt.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 1ced0731c140..83c8d36f781c 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2778,7 +2778,7 @@ static int srpt_parse_i_port_id(u8 i_port_id[16], const char *name)
{
const char *p;
unsigned len, count, leading_zero_bytes;
- int ret, rc;
+ int ret;
p = name;
if (strncasecmp(p, "0x", 2) == 0)
@@ -2790,10 +2790,9 @@ static int srpt_parse_i_port_id(u8 i_port_id[16], const char *name)
count = min(len / 2, 16U);
leading_zero_bytes = 16 - count;
memset(i_port_id, 0, leading_zero_bytes);
- rc = hex2bin(i_port_id + leading_zero_bytes, p, count);
- if (rc < 0)
- pr_debug("hex2bin failed for srpt_parse_i_port_id: %d\n", rc);
- ret = 0;
+ ret = hex2bin(i_port_id + leading_zero_bytes, p, count);
+ if (ret < 0)
+ pr_debug("hex2bin failed for srpt_parse_i_port_id: %d\n", ret);
out:
return ret;
}
--
2.12.2
From 425814c43bd3e48b2f9f818b84b930f2df91b257 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
Date: Wed, 14 Dec 2016 15:23:36 +0100
Subject: [PATCH 2/4] IB/srpt: Limit the send and receive queue sizes to what
the HCA supports
Additionally, correct the comment about ch->rq_size.
Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
---
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 83c8d36f781c..32641ee59182 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.12.2
From cb27ada5f446529b742d50466e710671d2fec004 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
Date: Wed, 14 Dec 2016 13:51:05 +0100
Subject: [PATCH 3/4] IB/srpt: Cache global L_Key
Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
---
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 32641ee59182..3a376b53f2c1 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 cc1183851af5..d1ec5b4378cc 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.12.2
From ff0062fce95fd82459ce9f7040b5f6a0efba2978 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
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@xxxxxxxxxxx>
---
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
