[PATCH v2] IB/srp: use multiple CPU cores more effectively

[PATCH v2] IB/srp: use multiple CPU cores more effectively

[Bart Van Assche]

SRP I/O with small block sizes causes a high CPU load. Processing IB
completions on the context of a kernel thread instead of in interrupt context
allows to process up to 25% more I/O operations per second. This patch does
add a kernel parameter 'thread' that allows to specify whether to process IB
completions in interrupt context or in kernel thread context. Also, the IB
receive notification processing loop is rewritten as proposed earlier by Ralph
Campbell (see also https://patchwork.kernel.org/patch/89426/). As the
measurement results below show, rewriting the IB receive notification
processing loop did not have a measurable impact on performance. Processing
IB receive notifications in thread context however does have a measurable
impact: workloads with I/O depth one are processed at most 10% slower and
workloads with larger I/O depths are processed up to 25% faster.

Changes compared to version one of this patch:
- Changed default mode from thread=y to thread=n, such that the default
  behavior of ib_srp is unaffected.
- Fixed a bug in the IB completion processing loop that caused session
  abortion to fail after cable disconnect (changed "return" into "break").
- Limited the description of the thread kernel parameter to eighty columns
  such that checkpatch doesn't complain about it.

Measurement results:

block size  number of    IOPS        IOPS      IOPS
 in bytes    threads     without     with      with
  ($bs)     ($numjobs)  this patch  thread=n  thread=y
   512           1        25,400      25,400    23,100
   512         128       122,000     122,000   153,000
  4096           1        25,000      25,000    22,700
  4096         128       122,000     121,000   157,000
 65536           1        14,300      14,400    13,600
 65536           4        36,700      36,700    36,600
524288           1         3,470       3,430     3,420
524288           4         5,020       5,020     4,990

performance test used to gather the above results:
  fio --bs=${bs} --ioengine=sg --buffered=0 --size=128M --rw=read \
      --thread --numjobs=${numjobs} --loops=100 --group_reporting \
      --gtod_reduce=1 --name=${dev} --filename=${dev}
other ib_srp kernel module parameters: srp_sg_tablesize=128
SRP target settings: storage type NULLIO; SCSI queue depth 128.
IB HCA type: QDR.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
Cc: David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org>
Cc: Ralph Campbell <ralph.campbell-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index ed3f9eb..8817804 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2005 Cisco Systems.  All rights reserved.
+ * Copyright (c) 2009-2010 Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -38,6 +39,7 @@
 #include <linux/parser.h>
 #include <linux/random.h>
 #include <linux/jiffies.h>
+#include <linux/kthread.h>
 
 #include <asm/atomic.h>
 
@@ -66,6 +68,12 @@ module_param(srp_sg_tablesize, int, 0444);
 MODULE_PARM_DESC(srp_sg_tablesize,
 		 "Max number of gather/scatter entries per I/O (default is 12, max 255)");
 
+static bool thread;
+module_param(thread, bool, 0444);
+MODULE_PARM_DESC(thread,
+		 "Whether to process IB completions in interrupt context"
+		 " (false) or kernel thread context (true)");
+
 static int topspin_workarounds = 1;
 
 module_param(topspin_workarounds, int, 0444);
@@ -81,6 +89,8 @@ MODULE_PARM_DESC(mellanox_workarounds,
 static void srp_add_one(struct ib_device *device);
 static void srp_remove_one(struct ib_device *device);
 static void srp_recv_completion(struct ib_cq *cq, void *target_ptr);
+static void srp_notify_recv_thread(struct ib_cq *cq, void *target_ptr);
+static int srp_compl_thread(void *target_ptr);
 static void srp_send_completion(struct ib_cq *cq, void *target_ptr);
 static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event);
 
@@ -229,7 +239,9 @@ static int srp_create_target_ib(struct srp_target_port *target)
 		return -ENOMEM;
 
 	target->recv_cq = ib_create_cq(target->srp_host->srp_dev->dev,
-				       srp_recv_completion, NULL, target, SRP_RQ_SIZE, 0);
+				       thread ? srp_notify_recv_thread
+				       : srp_recv_completion,
+				       NULL, target, SRP_RQ_SIZE, 0);
 	if (IS_ERR(target->recv_cq)) {
 		ret = PTR_ERR(target->recv_cq);
 		goto err;
@@ -242,7 +254,18 @@ static int srp_create_target_ib(struct srp_target_port *target)
 		goto err_recv_cq;
 	}
 
-	ib_req_notify_cq(target->recv_cq, IB_CQ_NEXT_COMP);
+	if (thread) {
+		init_waitqueue_head(&target->wait_queue);
+		target->thread = kthread_run(srp_compl_thread, target,
+					     "ib_srp_compl");
+		if (IS_ERR(target->thread)) {
+			ret = PTR_ERR(target->thread);
+			goto err_send_cq;
+		}
+	} else {
+		target->thread = NULL;
+		ib_req_notify_cq(target->recv_cq, IB_CQ_NEXT_COMP);
+	}
 
 	init_attr->event_handler       = srp_qp_event;
 	init_attr->cap.max_send_wr     = SRP_SQ_SIZE;
@@ -257,7 +280,7 @@ static int srp_create_target_ib(struct srp_target_port *target)
 	target->qp = ib_create_qp(target->srp_host->srp_dev->pd, init_attr);
 	if (IS_ERR(target->qp)) {
 		ret = PTR_ERR(target->qp);
-		goto err_send_cq;
+		goto err_thread;
 	}
 
 	ret = srp_init_qp(target, target->qp);
@@ -270,6 +293,10 @@ static int srp_create_target_ib(struct srp_target_port *target)
 err_qp:
 	ib_destroy_qp(target->qp);
 
+err_thread:
+	if (target->thread)
+		kthread_stop(target->thread);
+
 err_send_cq:
 	ib_destroy_cq(target->send_cq);
 
@@ -286,6 +313,8 @@ static void srp_free_target_ib(struct srp_target_port *target)
 	int i;
 
 	ib_destroy_qp(target->qp);
+	if (target->thread)
+		kthread_stop(target->thread);
 	ib_destroy_cq(target->send_cq);
 	ib_destroy_cq(target->recv_cq);
 
@@ -917,23 +946,45 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 				      DMA_FROM_DEVICE);
 }
 
+static void srp_notify_recv_thread(struct ib_cq *cq, void *target_ptr)
+{
+	struct srp_target_port *target = target_ptr;
+
+	wake_up_interruptible(&target->wait_queue);
+}
+
 static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)
 {
 	struct srp_target_port *target = target_ptr;
 	struct ib_wc wc;
 
-	ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
-	while (ib_poll_cq(cq, 1, &wc) > 0) {
-		if (wc.status) {
-			shost_printk(KERN_ERR, target->scsi_host,
-				     PFX "failed receive status %d\n",
-				     wc.status);
-			target->qp_in_error = 1;
-			break;
+	do {
+		while (ib_poll_cq(cq, 1, &wc) > 0) {
+			if (wc.status) {
+				shost_printk(KERN_ERR, target->scsi_host,
+					     PFX "failed receive status %d\n",
+					     wc.status);
+				target->qp_in_error = 1;
+				break;
+			}
+
+			srp_handle_recv(target, &wc);
 		}
+	} while (ib_req_notify_cq(cq, IB_CQ_NEXT_COMP
+				      | IB_CQ_REPORT_MISSED_EVENTS) > 0);
+}
+
+static int srp_compl_thread(void *target_ptr)
+{
+	struct srp_target_port *target = target_ptr;
 
-		srp_handle_recv(target, &wc);
+	while (!kthread_should_stop()) {
+		wait_event_interruptible(target->wait_queue,
+			(srp_recv_completion(target->recv_cq, target),
+			 kthread_should_stop()));
 	}
+
+	return 0;
 }
 
 static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 5a80eac..5ceb4a4 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -129,6 +129,8 @@ struct srp_target_port {
 	struct ib_sa_query     *path_query;
 	int			path_query_id;
 
+	wait_queue_head_t       wait_queue;
+	struct task_struct     *thread;
 	struct ib_cm_id	       *cm_id;
 	struct ib_cq	       *recv_cq;
 	struct ib_cq	       *send_cq;
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] IB/srp: use multiple CPU cores more effectively

[Vladislav Bolkhovitin]

Bart Van Assche, on 08/03/2010 06:02 PM wrote:
> SRP I/O with small block sizes causes a high CPU load. Processing IB
> completions on the context of a kernel thread instead of in interrupt context
> allows to process up to 25% more I/O operations per second. This patch does
> add a kernel parameter 'thread' that allows to specify whether to process IB
> completions in interrupt context or in kernel thread context. Also, the IB
> receive notification processing loop is rewritten as proposed earlier by Ralph
> Campbell (see also https://patchwork.kernel.org/patch/89426/). As the
> measurement results below show, rewriting the IB receive notification
> processing loop did not have a measurable impact on performance. Processing
> IB receive notifications in thread context however does have a measurable
> impact: workloads with I/O depth one are processed at most 10% slower and
> workloads with larger I/O depths are processed up to 25% faster.

I believe this is a wrong approach for this problem. You are 
workarounding it, not solving, and introducing a bad side effect of 
additional context switch per command, so increasing its processing 
latency. It doesn't matter that it can be switched off. Linux already 
has too many magic knobs where average user for long ago get lost.

If you want to spread requests post processing among several CPUs, you 
should consider real approaches for that:

1. Consider if it's possible to program hardware to spread IRQs for 
incoming packets among several CPU. At least some networking hardware 
allows that, so, I guess, IB also could.

2. Modify SCSI/block layer, so they perform the post processing not on 
the SIRQ level, but in the context of the processes which originated the 
corresponding request. This is, basically, what networking code is doing.

Thus, I'd say NACK to this patch. I don't like hackish workarounds 
instead of real solutions.

Sorry,
Vlad
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] IB/srp: use multiple CPU cores more effectively

[Roland Dreier]

 > I believe this is a wrong approach for this problem. You are
 > workarounding it, not solving, and introducing a bad side effect of
 > additional context switch per command, so increasing its processing
 > latency. It doesn't matter that it can be switched off. Linux already
 > has too many magic knobs where average user for long ago get lost.

agree ... how do we expect users to know what value to set for the
thread parameter?

 > If you want to spread requests post processing among several CPUs, you
 > should consider real approaches for that:

And furthermore, as far as I can see, the subject line of this patch is
quite misleading.  It does not use multiple CPUs at all for the numbers
posted, since it creates a single kthread and moves completion
processing there (although maybe this would help with multiple targets).

It seems the real improvement comes exactly from the increased latency:
by waiting a little while (overhead of going from completion interrupt
to kthread scheduling) before processing completions, we take fewer
interrupts because we process completions in bigger batches.

So maybe a better solution would be something NAPI-like -- ie when a
completion occurs, schedule a tasklet that polls the CQ some number of
times, and if the CQ is not empty at the end, reschedule to tasklet to
poll the CQ again without reenabling the CQ notification... ie
adaptively go into a polling mode when the interrupt load goes up.

But maybe this should be done higher in the block stack?

 - R.
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] IB/srp: use multiple CPU cores more effectively

[Bart Van Assche]

On Wed, Aug 4, 2010 at 10:40 PM, Roland Dreier <rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> wrote:
>
>  > I believe this is a wrong approach for this problem. You are
>  > workarounding it, not solving, and introducing a bad side effect of
>  > additional context switch per command, so increasing its processing
>  > latency. It doesn't matter that it can be switched off. Linux already
>  > has too many magic knobs where average user for long ago get lost.
>
> agree ... how do we expect users to know what value to set for the
> thread parameter?
>
>  > If you want to spread requests post processing among several CPUs, you
>  > should consider real approaches for that:
>
> And furthermore, as far as I can see, the subject line of this patch is
> quite misleading.  It does not use multiple CPUs at all for the numbers
> posted, since it creates a single kthread and moves completion
> processing there (although maybe this would help with multiple targets).
>
> It seems the real improvement comes exactly from the increased latency:
> by waiting a little while (overhead of going from completion interrupt
> to kthread scheduling) before processing completions, we take fewer
> interrupts because we process completions in bigger batches.
>
> So maybe a better solution would be something NAPI-like -- ie when a
> completion occurs, schedule a tasklet that polls the CQ some number of
> times, and if the CQ is not empty at the end, reschedule to tasklet to
> poll the CQ again without reenabling the CQ notification... ie
> adaptively go into a polling mode when the interrupt load goes up.
>
> But maybe this should be done higher in the block stack?

How about adding blk-iopoll support in ib_srp ? blk-iopoll is the NAPI
equivalent for block devices. More information about blk-iopoll can be
found here: http://lkml.org/lkml/2009/8/6/395.

Bart.
--
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

How to know if SRQ is being used in SRP?

[Suresh Shelvapille]

Folks:

I have the envious task of figuring out whether SRQ is being used in an SRP application (which I have very little
knowledge about). I only have IB transport layer traces from the customer. 
>From the traces I can see Valid Credits in the Syndrome field of the AETH in RDMA_ReadResponse_First/last messages.
Although there are a bunch of RC SENDs where the AETH syndrome field is Invalid, unfortunately I cannot seem to
correlate these RC SENDS with the RDMA read/responses. 
My questions is, since the RDMA response AETH field has valid CC credits is it saying SRQ is not being used?

Many thanks,
Suri   

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] IB/srp: use multiple CPU cores more effectively

[Roland Dreier]

  > How about adding blk-iopoll support in ib_srp ? blk-iopoll is the NAPI
  > equivalent for block devices. More information about blk-iopoll can be
  > found here: http://lkml.org/lkml/2009/8/6/395.

Wow, I was not aware of that work at all.  Thanks for the pointer.

Anyway, yes, that does look like it should give a lot of the performance
boost of your kthread change without hurting latency as much.

 - R.
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: How to know if SRQ is being used in SRP?

[Ralph Campbell]

SRP doesn't use a SRQ.

Look at drivers/infiniband/ulp/srp/ib_srp.c for
ib_create_qp() and the init_attr.cap.max_recv_wr is set
and init_attr.ib_srq is not set.

On Thu, 2010-08-05 at 09:38 -0700, Suresh Shelvapille wrote:
> Folks:
> 
> I have the envious task of figuring out whether SRQ is being used in an SRP application (which I have very little
> knowledge about). I only have IB transport layer traces from the customer. 
> From the traces I can see Valid Credits in the Syndrome field of the AETH in RDMA_ReadResponse_First/last messages.
> Although there are a bunch of RC SENDs where the AETH syndrome field is Invalid, unfortunately I cannot seem to
> correlate these RC SENDS with the RDMA read/responses. 
> My questions is, since the RDMA response AETH field has valid CC credits is it saying SRQ is not being used?
> 
> Many thanks,
> Suri   
> 
> --
> 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
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: How to know if SRQ is being used in SRP?

[Suresh Shelvapille]

Ralph:
I am told that the customer has modified the SRP code, which is why I cannot go by the ib_srp.c code. 
Hence, I am trying to figure out from the IB transport traces whether SRQ is being used or not. 
1. Would the Syndrome field show CC INVALID if SRQ were being used? If so, in which IB message would it show---RDMA read
response, or in an initial RC SEND message or both?

Thanks,
Suri 


> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Ralph
> Campbell
> Sent: Thursday, August 05, 2010 1:42 PM
> To: Suresh Shelvapille
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: How to know if SRQ is being used in SRP?
> 
> SRP doesn't use a SRQ.
> 
> Look at drivers/infiniband/ulp/srp/ib_srp.c for
> ib_create_qp() and the init_attr.cap.max_recv_wr is set
> and init_attr.ib_srq is not set.
> 
> On Thu, 2010-08-05 at 09:38 -0700, Suresh Shelvapille wrote:
> > Folks:
> >
> > I have the envious task of figuring out whether SRQ is being used in an SRP application (which I
> have very little
> > knowledge about). I only have IB transport layer traces from the customer.
> > From the traces I can see Valid Credits in the Syndrome field of the AETH in
> RDMA_ReadResponse_First/last messages.
> > Although there are a bunch of RC SENDs where the AETH syndrome field is Invalid, unfortunately I
> cannot seem to
> > correlate these RC SENDS with the RDMA read/responses.
> > My questions is, since the RDMA response AETH field has valid CC credits is it saying SRQ is not
> being used?
> >
> > Many thanks,
> > Suri
> >
> > --
> > 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
> >
> 
> 
> --
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: How to know if SRQ is being used in SRP?

[Roland Dreier]

  > I have the envious task of figuring out whether SRQ is being used in
  > an SRP application (which I have very little knowledge about). I
  > only have IB transport layer traces from the customer.  From the
  > traces I can see Valid Credits in the Syndrome field of the AETH in
  > RDMA_ReadResponse_First/last messages.  Although there are a bunch
  > of RC SENDs where the AETH syndrome field is Invalid, unfortunately
  > I cannot seem to correlate these RC SENDS with the RDMA
  > read/responses.  My questions is, since the RDMA response AETH field
  > has valid CC credits is it saying SRQ is not being used?

As I guess you know, if end-to-end credits are used on an RC connection,
then SRQ cannot be used on that connection.  The opposite implication
does not hold: eg a TCA might not implement the optional end-to-end flow
control protocol, and hence a non-SRQ connection might still not use
end-to-end flow control.

Also end-to-end flow control is negotiated for each direction of a
connection independently, so you might see valid end-to-end credits sent
in only one direction.

Essentially the only thing you can learn from a wire-level trace is the
following: if you see an AETH with valid end-to-end credits being sent
from CA X, then the corresponding receive queue on CA X is not attached
to an SRQ.  I don't believe there is anything further you can deduce.
(Certainly invalid end-to-end credits do not imply that an SRQ is being
used, they simply imply that the end-to-end flow control protocol is not
in use)

 - R.
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: How to know if SRQ is being used in SRP?

[Roland Dreier]

 > I am told that the customer has modified the SRP code, which is why I cannot go by the ib_srp.c code. 

Can you not ask the customer??  Or if you have the object code, look for
calls to srq functions in the binary??

  > 1. Would the Syndrome field show CC INVALID if SRQ were being used?
  > If so, in which IB message would it show---RDMA read response, or in
  > an initial RC SEND message or both?

As I said before, the implication is only one way.  If end-to-end flow
control is not used, that does not imply anything -- the CM protocol
allows flow control to be turned off even if an SRQ is not used.

 - R.
-- 
Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
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