[RFC PATCH 0/2] block: rq_affinity performance fixes

[RFC PATCH 0/2] block: rq_affinity performance fixes

[Dan Williams]

Jens,

Per the "rq_affinity doesn't seem to work?" thread [1].  We found that the
default cpu steering that rq_affinity performs is insufficient to keep up with
some completion loads.

Dave collected the following relative iops measures:
pre-patches  rq_affinity=0: 1x
pre-patches  rq_affinity=1: 1x
post-patches rq_affinity=1: 1.08x
post-patches rq_affinity=2: 1.35x

The "adaptive rq_affinity" patch is more of a discussion point than a
real fix.  However, it shows that even something straightforward and
seemingly aggressive does not really come close to the performance of
just turning cpu grouping off altogether.  The "strict rq_affinity"
patch follows the "nomerges" interface precedent of setting 2 to
indicate "I really mean it".

So, at a minimum consider taking patch 1, and we can leave the search
for a better adaptive algorithm as future work.

[1]: http://marc.info/?l=linux-kernel&m=131049742925413&w=4

---

Dan Williams (2):
      block: strict rq_affinity
      block: adaptive rq_affinity

 Documentation/block/queue-sysfs.txt |   10 +++++++---
 block/blk-core.c                    |    6 ++----
 block/blk-softirq.c                 |   23 ++++++++++++++++++-----
 block/blk-sysfs.c                   |   13 +++++++++----
 include/linux/blkdev.h              |    3 ++-
 5 files changed, 38 insertions(+), 17 deletions(-)

[RFC PATCH 1/2] block: strict rq_affinity

[Dan Williams]

Some storage controllers benefit from completions always being steered
to the strict requester cpu rather than the looser "per-socket" steering
that blk_cpu_to_group() attempts by default.

echo 2 > /sys/block/<bdev>/queue/rq_affinity

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Roland Dreier <roland@purestorage.com>
Tested-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/block/queue-sysfs.txt |   10 +++++++---
 block/blk-core.c                    |    6 ++----
 block/blk-softirq.c                 |   11 +++++++----
 block/blk-sysfs.c                   |   13 +++++++++----
 include/linux/blkdev.h              |    3 ++-
 5 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/Documentation/block/queue-sysfs.txt b/Documentation/block/queue-sysfs.txt
index f652740..d8147b3 100644
--- a/Documentation/block/queue-sysfs.txt
+++ b/Documentation/block/queue-sysfs.txt
@@ -45,9 +45,13 @@ device.
 
 rq_affinity (RW)
 ----------------
-If this option is enabled, the block layer will migrate request completions
-to the CPU that originally submitted the request. For some workloads
-this provides a significant reduction in CPU cycles due to caching effects.
+If this option is '1', the block layer will migrate request completions to the
+cpu "group" that originally submitted the request. For some workloads this
+provides a significant reduction in CPU cycles due to caching effects.
+
+For storage configurations that need to maximize distribution of completion
+processing setting this option to '2' forces the completion to run on the
+requesting cpu (bypassing the "group" aggregation logic).
 
 scheduler (RW)
 --------------
diff --git a/block/blk-core.c b/block/blk-core.c
index d2f8f40..9c7ba87 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1279,10 +1279,8 @@ get_rq:
 	init_request_from_bio(req, bio);
 
 	if (test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags) ||
-	    bio_flagged(bio, BIO_CPU_AFFINE)) {
-		req->cpu = blk_cpu_to_group(get_cpu());
-		put_cpu();
-	}
+	    bio_flagged(bio, BIO_CPU_AFFINE))
+		req->cpu = smp_processor_id();
 
 	plug = current->plug;
 	if (plug) {
diff --git a/block/blk-softirq.c b/block/blk-softirq.c
index ee9c216..475fab8 100644
--- a/block/blk-softirq.c
+++ b/block/blk-softirq.c
@@ -103,22 +103,25 @@ static struct notifier_block __cpuinitdata blk_cpu_notifier = {
 
 void __blk_complete_request(struct request *req)
 {
+	int ccpu, cpu, group_cpu = NR_CPUS;
 	struct request_queue *q = req->q;
 	unsigned long flags;
-	int ccpu, cpu, group_cpu;
 
 	BUG_ON(!q->softirq_done_fn);
 
 	local_irq_save(flags);
 	cpu = smp_processor_id();
-	group_cpu = blk_cpu_to_group(cpu);
 
 	/*
 	 * Select completion CPU
 	 */
-	if (test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags) && req->cpu != -1)
+	if (test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags) && req->cpu != -1) {
 		ccpu = req->cpu;
-	else
+		if (!test_bit(QUEUE_FLAG_SAME_FORCE, &q->queue_flags)) {
+			ccpu = blk_cpu_to_group(ccpu);
+			group_cpu = blk_cpu_to_group(cpu);
+		}
+	} else
 		ccpu = cpu;
 
 	if (ccpu == cpu || ccpu == group_cpu) {
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index d935bd8..0ee17b5 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -244,8 +244,9 @@ static ssize_t queue_nomerges_store(struct request_queue *q, const char *page,
 static ssize_t queue_rq_affinity_show(struct request_queue *q, char *page)
 {
 	bool set = test_bit(QUEUE_FLAG_SAME_COMP, &q->queue_flags);
+	bool force = test_bit(QUEUE_FLAG_SAME_FORCE, &q->queue_flags);
 
-	return queue_var_show(set, page);
+	return queue_var_show(set << force, page);
 }
 
 static ssize_t
@@ -257,10 +258,14 @@ queue_rq_affinity_store(struct request_queue *q, const char *page, size_t count)
 
 	ret = queue_var_store(&val, page, count);
 	spin_lock_irq(q->queue_lock);
-	if (val)
+	if (val) {
 		queue_flag_set(QUEUE_FLAG_SAME_COMP, q);
-	else
-		queue_flag_clear(QUEUE_FLAG_SAME_COMP,  q);
+		if (val == 2)
+			queue_flag_set(QUEUE_FLAG_SAME_FORCE, q);
+	} else {
+		queue_flag_clear(QUEUE_FLAG_SAME_COMP, q);
+		queue_flag_clear(QUEUE_FLAG_SAME_FORCE, q);
+	}
 	spin_unlock_irq(q->queue_lock);
 #endif
 	return ret;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1a23722..b228825 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -393,7 +393,7 @@ struct request_queue
 #define QUEUE_FLAG_ELVSWITCH	6	/* don't use elevator, just do FIFO */
 #define QUEUE_FLAG_BIDI		7	/* queue supports bidi requests */
 #define QUEUE_FLAG_NOMERGES     8	/* disable merge attempts */
-#define QUEUE_FLAG_SAME_COMP	9	/* force complete on same CPU */
+#define QUEUE_FLAG_SAME_COMP	9	/* complete on same CPU-group */
 #define QUEUE_FLAG_FAIL_IO     10	/* fake timeout */
 #define QUEUE_FLAG_STACKABLE   11	/* supports request stacking */
 #define QUEUE_FLAG_NONROT      12	/* non-rotational device (SSD) */
@@ -403,6 +403,7 @@ struct request_queue
 #define QUEUE_FLAG_NOXMERGES   15	/* No extended merges */
 #define QUEUE_FLAG_ADD_RANDOM  16	/* Contributes to random pool */
 #define QUEUE_FLAG_SECDISCARD  17	/* supports SECDISCARD */
+#define QUEUE_FLAG_SAME_FORCE  18	/* force complete on same CPU */
 
 #define QUEUE_FLAG_DEFAULT	((1 << QUEUE_FLAG_IO_STAT) |		\
 				 (1 << QUEUE_FLAG_STACKABLE)	|	\

[RFC PATCH 2/2] block: adaptive rq_affinity

[Dan Williams]

For some storage configurations the coarse grained cpu grouping (socket)
does not supply enough cpu to keep up with the demands of high iops.
Bypass the grouping and complete on the direct requester cpu when the
local cpu is under softirq pressure (as measured by ksoftirqd being in
the running state).

Cc: Matthew Wilcox <matthew@wil.cx>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Roland Dreier <roland@purestorage.com>
Tested-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 block/blk-softirq.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/block/blk-softirq.c b/block/blk-softirq.c
index 475fab8..f0cda19 100644
--- a/block/blk-softirq.c
+++ b/block/blk-softirq.c
@@ -101,16 +101,20 @@ static struct notifier_block __cpuinitdata blk_cpu_notifier = {
 	.notifier_call	= blk_cpu_notify,
 };
 
+DECLARE_PER_CPU(struct task_struct *, ksoftirqd);
+
 void __blk_complete_request(struct request *req)
 {
 	int ccpu, cpu, group_cpu = NR_CPUS;
 	struct request_queue *q = req->q;
+	struct task_struct *tsk;
 	unsigned long flags;
 
 	BUG_ON(!q->softirq_done_fn);
 
 	local_irq_save(flags);
 	cpu = smp_processor_id();
+	tsk = per_cpu(ksoftirqd, cpu);
 
 	/*
 	 * Select completion CPU
@@ -124,7 +128,13 @@ void __blk_complete_request(struct request *req)
 	} else
 		ccpu = cpu;
 
-	if (ccpu == cpu || ccpu == group_cpu) {
+	/*
+	 * try to skip a remote softirq-trigger if the completion is
+	 * within the same group, but not if local softirqs have already
+	 * spilled to ksoftirqd
+	 */
+	if (ccpu == cpu ||
+	    (ccpu == group_cpu && tsk->state != TASK_RUNNING)) {
 		struct list_head *list;
 do_local:
 		list = &__get_cpu_var(blk_cpu_done);

Re: [RFC PATCH 1/2] block: strict rq_affinity

[Christoph Hellwig]

On Fri, Jul 22, 2011 at 01:59:39PM -0700, Dan Williams wrote:
> Some storage controllers benefit from completions always being steered
> to the strict requester cpu rather than the looser "per-socket" steering
> that blk_cpu_to_group() attempts by default.

Isn't this actually dependent on the cpu, and not the storage
controller?

Re: [RFC PATCH 1/2] block: strict rq_affinity

[Jens Axboe]

On 2011-07-23 03:46, Christoph Hellwig wrote:
> On Fri, Jul 22, 2011 at 01:59:39PM -0700, Dan Williams wrote:
>> Some storage controllers benefit from completions always being steered
>> to the strict requester cpu rather than the looser "per-socket" steering
>> that blk_cpu_to_group() attempts by default.
> 
> Isn't this actually dependent on the cpu, and not the storage
> controller?

It is, it's completely indendent of the controller used. Perhaps some
drivers could have a very heavy end io completion handling causing the
problem to become larger, but in general it's an artifact of the CPU and
not the controller.

-- 
Jens Axboe

Re: [RFC PATCH 1/2] block: strict rq_affinity

[Jens Axboe]

On 2011-07-22 22:59, Dan Williams wrote:
> Some storage controllers benefit from completions always being steered
> to the strict requester cpu rather than the looser "per-socket" steering
> that blk_cpu_to_group() attempts by default.
> 
> echo 2 > /sys/block/<bdev>/queue/rq_affinity

I have applied this one, with a modified patch description.

I like the adaptive solution, but it should be rewritten to not declare
and expose softirq internals. Essentially have an API from
kernel/softirq.c that can return whether a given (or perhaps just local)
softirq handler is busy or not.


-- 
Jens Axboe

Re: [RFC PATCH 1/2] block: strict rq_affinity

[Shaohua Li]

2011/7/24 Jens Axboe <jaxboe@fusionio.com>:
> On 2011-07-22 22:59, Dan Williams wrote:
>> Some storage controllers benefit from completions always being steered
>> to the strict requester cpu rather than the looser "per-socket" steering
>> that blk_cpu_to_group() attempts by default.
>>
>> echo 2 > /sys/block/<bdev>/queue/rq_affinity
>
> I have applied this one, with a modified patch description.
>
> I like the adaptive solution, but it should be rewritten to not declare
> and expose softirq internals. Essentially have an API from
> kernel/softirq.c that can return whether a given (or perhaps just local)
> softirq handler is busy or not.
Jens,
I posted a similar patch about two years ago(
http://marc.info/?l=linux-kernel&m=126136252929329&w=2).
At that time, you actually did a lot of tests and said the same cpu
approach will cause huge lock contention and bounce. Is that get fixed?

Thanks,
Shaohua

Re: [RFC PATCH 1/2] block: strict rq_affinity

[Jens Axboe]

On 2011-07-25 03:14, Shaohua Li wrote:
> 2011/7/24 Jens Axboe <jaxboe@fusionio.com>:
>> On 2011-07-22 22:59, Dan Williams wrote:
>>> Some storage controllers benefit from completions always being steered
>>> to the strict requester cpu rather than the looser "per-socket" steering
>>> that blk_cpu_to_group() attempts by default.
>>>
>>> echo 2 > /sys/block/<bdev>/queue/rq_affinity
>>
>> I have applied this one, with a modified patch description.
>>
>> I like the adaptive solution, but it should be rewritten to not declare
>> and expose softirq internals. Essentially have an API from
>> kernel/softirq.c that can return whether a given (or perhaps just local)
>> softirq handler is busy or not.
> Jens,
> I posted a similar patch about two years ago(
> http://marc.info/?l=linux-kernel&m=126136252929329&w=2).
> At that time, you actually did a lot of tests and said the same cpu
> approach will cause huge lock contention and bounce. Is that get fixed?

Yep, it's not ideal. But if we are running out of steam on a single
processor, there's really not much of an option currently.

-- 
Jens Axboe