* [PATCH 0/7] SRP patches for kernel v4.15 @ 2017-10-06 21:42 Bart Van Assche 2017-10-06 21:42 ` [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash Bart Van Assche [not found] ` <20171006214243.11296-1-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 0 siblings, 2 replies; 32+ messages in thread From: Bart Van Assche @ 2017-10-06 21:42 UTC (permalink / raw) To: Doug Ledford; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash 2017-10-06 21:42 [PATCH 0/7] SRP patches for kernel v4.15 Bart Van Assche @ 2017-10-06 21:42 ` Bart Van Assche [not found] ` <20171006214243.11296-5-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> [not found] ` <20171006214243.11296-1-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 1 sibling, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-10-06 21:42 UTC (permalink / raw) To: Doug Ledford; +Cc: linux-rdma, Bart Van Assche, stable 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
[parent not found: <20171006214243.11296-5-bart.vanassche-Sjgp3cTcYWE@public.gmane.org>]
* Re: [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash 2017-10-06 21:42 ` [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash Bart Van Assche @ 2017-10-08 13:22 ` Leon Romanovsky 0 siblings, 0 replies; 32+ messages in thread From: Leon Romanovsky @ 2017-10-08 13:22 UTC (permalink / raw) To: Bart Van Assche Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA, stable-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash @ 2017-10-08 13:22 ` Leon Romanovsky 0 siblings, 0 replies; 32+ messages in thread From: Leon Romanovsky @ 2017-10-08 13:22 UTC (permalink / raw) To: Bart Van Assche; +Cc: Doug Ledford, linux-rdma, stable [-- 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 --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash 2017-10-08 13:22 ` Leon Romanovsky (?) @ 2017-10-09 17:02 ` Bart Van Assche -1 siblings, 0 replies; 32+ messages in thread From: Bart Van Assche @ 2017-10-09 17:02 UTC (permalink / raw) To: leon; +Cc: linux-rdma, stable, dledford 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash 2017-10-06 21:42 ` [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash Bart Van Assche @ 2017-10-11 12:36 ` Sagi Grimberg 0 siblings, 0 replies; 32+ messages in thread From: Sagi Grimberg @ 2017-10-11 12:36 UTC (permalink / raw) To: Bart Van Assche, Doug Ledford Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, stable-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash @ 2017-10-11 12:36 ` Sagi Grimberg 0 siblings, 0 replies; 32+ messages in thread From: Sagi Grimberg @ 2017-10-11 12:36 UTC (permalink / raw) To: Bart Van Assche, Doug Ledford; +Cc: linux-rdma, stable Looks good, Reviewed-by: Sagi Grimberg <sagi@grimberg.me> ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171006214243.11296-1-bart.vanassche-Sjgp3cTcYWE@public.gmane.org>]
* [PATCH 1/7] IB/srpt: Limit the send and receive queue sizes to what the HCA supports [not found] ` <20171006214243.11296-1-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> @ 2017-10-06 21:42 ` Bart Van Assche [not found] ` <20171006214243.11296-2-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-06 21:42 ` [PATCH 2/7] IB/srpt: Cache global L_Key Bart Van Assche ` (4 subsequent siblings) 5 siblings, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-10-06 21:42 UTC (permalink / raw) To: Doug Ledford; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
[parent not found: <20171006214243.11296-2-bart.vanassche-Sjgp3cTcYWE@public.gmane.org>]
* Re: [PATCH 1/7] IB/srpt: Limit the send and receive queue sizes to what the HCA supports [not found] ` <20171006214243.11296-2-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> @ 2017-10-11 12:31 ` Sagi Grimberg 0 siblings, 0 replies; 32+ messages in thread From: Sagi Grimberg @ 2017-10-11 12:31 UTC (permalink / raw) To: Bart Van Assche, Doug Ledford; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 2/7] IB/srpt: Cache global L_Key [not found] ` <20171006214243.11296-1-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-06 21:42 ` [PATCH 1/7] IB/srpt: Limit the send and receive queue sizes to what the HCA supports Bart Van Assche @ 2017-10-06 21:42 ` Bart Van Assche [not found] ` <20171006214243.11296-3-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-06 21:42 ` [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ Bart Van Assche ` (3 subsequent siblings) 5 siblings, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-10-06 21:42 UTC (permalink / raw) To: Doug Ledford; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
[parent not found: <20171006214243.11296-3-bart.vanassche-Sjgp3cTcYWE@public.gmane.org>]
* Re: [PATCH 2/7] IB/srpt: Cache global L_Key [not found] ` <20171006214243.11296-3-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> @ 2017-10-08 9:01 ` Christoph Hellwig [not found] ` <20171008090107.GA17153-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Christoph Hellwig @ 2017-10-08 9:01 UTC (permalink / raw) To: Bart Van Assche; +Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171008090107.GA17153-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>]
* Re: [PATCH 2/7] IB/srpt: Cache global L_Key [not found] ` <20171008090107.GA17153-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> @ 2017-10-09 16:57 ` Bart Van Assche 0 siblings, 0 replies; 32+ messages in thread From: Bart Van Assche @ 2017-10-09 16:57 UTC (permalink / raw) To: hch-wEGCiKHe2LqWVfeAwA7xHQ Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, dledford-H+wXaHxf7aLQT0dZR+AlfA 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <20171006214243.11296-1-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-06 21:42 ` [PATCH 1/7] IB/srpt: Limit the send and receive queue sizes to what the HCA supports Bart Van Assche 2017-10-06 21:42 ` [PATCH 2/7] IB/srpt: Cache global L_Key Bart Van Assche @ 2017-10-06 21:42 ` Bart Van Assche [not found] ` <20171006214243.11296-4-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-06 21:42 ` [PATCH 5/7] IB/srp: Remove second argument of srp_destroy_qp() Bart Van Assche ` (2 subsequent siblings) 5 siblings, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-10-06 21:42 UTC (permalink / raw) To: Doug Ledford; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
[parent not found: <20171006214243.11296-4-bart.vanassche-Sjgp3cTcYWE@public.gmane.org>]
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <20171006214243.11296-4-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> @ 2017-10-08 10:03 ` Leon Romanovsky [not found] ` <20171008100317.GR25829-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Leon Romanovsky @ 2017-10-08 10:03 UTC (permalink / raw) To: Bart Van Assche; +Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171008100317.GR25829-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>]
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <20171008100317.GR25829-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> @ 2017-10-09 16:56 ` Doug Ledford [not found] ` <1507568205.46071.46.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Doug Ledford @ 2017-10-09 16:56 UTC (permalink / raw) To: Leon Romanovsky, Bart Van Assche; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <1507568205.46071.46.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <1507568205.46071.46.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> @ 2017-10-09 17:01 ` Bart Van Assche [not found] ` <1507568492.2674.11.camel-Sjgp3cTcYWE@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-10-09 17:01 UTC (permalink / raw) To: dledford-H+wXaHxf7aLQT0dZR+AlfA, leon-DgEjT+Ai2ygdnm+yROfE0A Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <1507568492.2674.11.camel-Sjgp3cTcYWE@public.gmane.org>]
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <1507568492.2674.11.camel-Sjgp3cTcYWE@public.gmane.org> @ 2017-10-09 17:12 ` Doug Ledford 2017-10-10 4:14 ` Leon Romanovsky 1 sibling, 0 replies; 32+ messages in thread From: Doug Ledford @ 2017-10-09 17:12 UTC (permalink / raw) To: Bart Van Assche, leon-DgEjT+Ai2ygdnm+yROfE0A Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <1507568492.2674.11.camel-Sjgp3cTcYWE@public.gmane.org> 2017-10-09 17:12 ` Doug Ledford @ 2017-10-10 4:14 ` Leon Romanovsky [not found] ` <20171010041423.GJ1252-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> 1 sibling, 1 reply; 32+ messages in thread From: Leon Romanovsky @ 2017-10-10 4:14 UTC (permalink / raw) To: Bart Van Assche Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA, linux-rdma-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171010041423.GJ1252-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>]
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <20171010041423.GJ1252-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> @ 2017-10-10 14:34 ` Doug Ledford [not found] ` <9443ec1f-0acd-9fa3-4621-a29085d2c606-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Doug Ledford @ 2017-10-10 14:34 UTC (permalink / raw) To: Leon Romanovsky, Bart Van Assche; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <9443ec1f-0acd-9fa3-4621-a29085d2c606-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <9443ec1f-0acd-9fa3-4621-a29085d2c606-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> @ 2017-10-10 15:00 ` Leon Romanovsky [not found] ` <20171010150018.GC2106-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Leon Romanovsky @ 2017-10-10 15:00 UTC (permalink / raw) To: Doug Ledford; +Cc: Bart Van Assche, linux-rdma-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171010150018.GC2106-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>]
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <20171010150018.GC2106-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> @ 2017-10-10 15:13 ` Doug Ledford [not found] ` <1507648402.46071.53.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Doug Ledford @ 2017-10-10 15:13 UTC (permalink / raw) To: Leon Romanovsky; +Cc: Bart Van Assche, linux-rdma-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <1507648402.46071.53.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <1507648402.46071.53.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> @ 2017-10-10 15:44 ` Leon Romanovsky [not found] ` <20171010154439.GE2106-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Leon Romanovsky @ 2017-10-10 15:44 UTC (permalink / raw) To: Doug Ledford; +Cc: Bart Van Assche, linux-rdma-u79uwXL29TY76Z2rM5mHXA [-- 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 --] ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171010154439.GE2106-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>]
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <20171010154439.GE2106-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> @ 2017-10-10 16:04 ` Bart Van Assche [not found] ` <1507651473.2815.20.camel-Sjgp3cTcYWE@public.gmane.org> 2017-10-10 16:11 ` Doug Ledford 1 sibling, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-10-10 16:04 UTC (permalink / raw) To: leon-DgEjT+Ai2ygdnm+yROfE0A, dledford-H+wXaHxf7aLQT0dZR+AlfA Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <1507651473.2815.20.camel-Sjgp3cTcYWE@public.gmane.org>]
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <1507651473.2815.20.camel-Sjgp3cTcYWE@public.gmane.org> @ 2017-10-10 17:04 ` Jason Gunthorpe [not found] ` <20171010170429.GA21288-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Jason Gunthorpe @ 2017-10-10 17:04 UTC (permalink / raw) To: Bart Van Assche Cc: leon-DgEjT+Ai2ygdnm+yROfE0A, dledford-H+wXaHxf7aLQT0dZR+AlfA, linux-rdma-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20171010170429.GA21288-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <20171010170429.GA21288-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> @ 2017-10-10 17:10 ` Bart Van Assche [not found] ` <1507655454.2815.43.camel-Sjgp3cTcYWE@public.gmane.org> 0 siblings, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-10-10 17:10 UTC (permalink / raw) To: jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/ Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, leon-DgEjT+Ai2ygdnm+yROfE0A, dledford-H+wXaHxf7aLQT0dZR+AlfA 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <1507655454.2815.43.camel-Sjgp3cTcYWE@public.gmane.org>]
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <1507655454.2815.43.camel-Sjgp3cTcYWE@public.gmane.org> @ 2017-10-10 18:01 ` Leon Romanovsky 0 siblings, 0 replies; 32+ messages in thread From: Leon Romanovsky @ 2017-10-10 18:01 UTC (permalink / raw) To: Bart Van Assche Cc: jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/, linux-rdma-u79uwXL29TY76Z2rM5mHXA, dledford-H+wXaHxf7aLQT0dZR+AlfA [-- 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 --] ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ [not found] ` <20171010154439.GE2106-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> 2017-10-10 16:04 ` Bart Van Assche @ 2017-10-10 16:11 ` Doug Ledford 1 sibling, 0 replies; 32+ messages in thread From: Doug Ledford @ 2017-10-10 16:11 UTC (permalink / raw) To: Leon Romanovsky; +Cc: Bart Van Assche, linux-rdma-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 5/7] IB/srp: Remove second argument of srp_destroy_qp() [not found] ` <20171006214243.11296-1-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> ` (2 preceding siblings ...) 2017-10-06 21:42 ` [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ Bart Van Assche @ 2017-10-06 21:42 ` Bart Van Assche [not found] ` <20171006214243.11296-6-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-06 21:42 ` [PATCH 6/7] IB/srp: Cache global rkey Bart Van Assche 2017-10-06 21:42 ` [PATCH 7/7] IB/srp: Make CM timeout dependent on subnet timeout Bart Van Assche 5 siblings, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-10-06 21:42 UTC (permalink / raw) To: Doug Ledford; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
[parent not found: <20171006214243.11296-6-bart.vanassche-Sjgp3cTcYWE@public.gmane.org>]
* Re: [PATCH 5/7] IB/srp: Remove second argument of srp_destroy_qp() [not found] ` <20171006214243.11296-6-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> @ 2017-10-11 12:36 ` Sagi Grimberg 0 siblings, 0 replies; 32+ messages in thread From: Sagi Grimberg @ 2017-10-11 12:36 UTC (permalink / raw) To: Bart Van Assche, Doug Ledford; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 6/7] IB/srp: Cache global rkey [not found] ` <20171006214243.11296-1-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> ` (3 preceding siblings ...) 2017-10-06 21:42 ` [PATCH 5/7] IB/srp: Remove second argument of srp_destroy_qp() Bart Van Assche @ 2017-10-06 21:42 ` Bart Van Assche 2017-10-06 21:42 ` [PATCH 7/7] IB/srp: Make CM timeout dependent on subnet timeout Bart Van Assche 5 siblings, 0 replies; 32+ messages in thread From: Bart Van Assche @ 2017-10-06 21:42 UTC (permalink / raw) To: Doug Ledford; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 7/7] IB/srp: Make CM timeout dependent on subnet timeout [not found] ` <20171006214243.11296-1-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> ` (4 preceding siblings ...) 2017-10-06 21:42 ` [PATCH 6/7] IB/srp: Cache global rkey Bart Van Assche @ 2017-10-06 21:42 ` Bart Van Assche [not found] ` <20171006214243.11296-8-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 5 siblings, 1 reply; 32+ messages in thread From: Bart Van Assche @ 2017-10-06 21:42 UTC (permalink / raw) To: Doug Ledford; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
[parent not found: <20171006214243.11296-8-bart.vanassche-Sjgp3cTcYWE@public.gmane.org>]
* Re: [PATCH 7/7] IB/srp: Make CM timeout dependent on subnet timeout [not found] ` <20171006214243.11296-8-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> @ 2017-10-11 12:38 ` Sagi Grimberg 0 siblings, 0 replies; 32+ messages in thread From: Sagi Grimberg @ 2017-10-11 12:38 UTC (permalink / raw) To: Bart Van Assche, Doug Ledford; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2017-10-11 12:38 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-06 21:42 [PATCH 0/7] SRP patches for kernel v4.15 Bart Van Assche 2017-10-06 21:42 ` [PATCH 4/7] IB/srp: Avoid a cable pull can trigger a kernel crash Bart Van Assche [not found] ` <20171006214243.11296-5-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-08 13:22 ` Leon Romanovsky 2017-10-08 13:22 ` Leon Romanovsky 2017-10-09 17:02 ` Bart Van Assche 2017-10-11 12:36 ` Sagi Grimberg 2017-10-11 12:36 ` Sagi Grimberg [not found] ` <20171006214243.11296-1-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-06 21:42 ` [PATCH 1/7] IB/srpt: Limit the send and receive queue sizes to what the HCA supports Bart Van Assche [not found] ` <20171006214243.11296-2-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-11 12:31 ` Sagi Grimberg 2017-10-06 21:42 ` [PATCH 2/7] IB/srpt: Cache global L_Key Bart Van Assche [not found] ` <20171006214243.11296-3-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-08 9:01 ` Christoph Hellwig [not found] ` <20171008090107.GA17153-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> 2017-10-09 16:57 ` Bart Van Assche 2017-10-06 21:42 ` [PATCH 3/7] IB/srpt: Change default behavior from using SRQ to not using SRQ Bart Van Assche [not found] ` <20171006214243.11296-4-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-08 10:03 ` Leon Romanovsky [not found] ` <20171008100317.GR25829-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> 2017-10-09 16:56 ` Doug Ledford [not found] ` <1507568205.46071.46.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2017-10-09 17:01 ` Bart Van Assche [not found] ` <1507568492.2674.11.camel-Sjgp3cTcYWE@public.gmane.org> 2017-10-09 17:12 ` Doug Ledford 2017-10-10 4:14 ` Leon Romanovsky [not found] ` <20171010041423.GJ1252-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> 2017-10-10 14:34 ` Doug Ledford [not found] ` <9443ec1f-0acd-9fa3-4621-a29085d2c606-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2017-10-10 15:00 ` Leon Romanovsky [not found] ` <20171010150018.GC2106-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> 2017-10-10 15:13 ` Doug Ledford [not found] ` <1507648402.46071.53.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 2017-10-10 15:44 ` Leon Romanovsky [not found] ` <20171010154439.GE2106-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org> 2017-10-10 16:04 ` Bart Van Assche [not found] ` <1507651473.2815.20.camel-Sjgp3cTcYWE@public.gmane.org> 2017-10-10 17:04 ` Jason Gunthorpe [not found] ` <20171010170429.GA21288-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 2017-10-10 17:10 ` Bart Van Assche [not found] ` <1507655454.2815.43.camel-Sjgp3cTcYWE@public.gmane.org> 2017-10-10 18:01 ` Leon Romanovsky 2017-10-10 16:11 ` Doug Ledford 2017-10-06 21:42 ` [PATCH 5/7] IB/srp: Remove second argument of srp_destroy_qp() Bart Van Assche [not found] ` <20171006214243.11296-6-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-11 12:36 ` Sagi Grimberg 2017-10-06 21:42 ` [PATCH 6/7] IB/srp: Cache global rkey Bart Van Assche 2017-10-06 21:42 ` [PATCH 7/7] IB/srp: Make CM timeout dependent on subnet timeout Bart Van Assche [not found] ` <20171006214243.11296-8-bart.vanassche-Sjgp3cTcYWE@public.gmane.org> 2017-10-11 12:38 ` Sagi Grimberg
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.