[PATCH 0/2] blk-mq: switch from preempt_disable/enable to get/put_cpu

[PATCH 0/2] blk-mq: switch from preempt_disable/enable to get/put_cpu

[Paolo Bonzini]

blk-mq is using preempt_disable/enable in order to ensure that the
queue runners are placed on the right CPU.  This does not work with
the RT patches, because __blk_mq_run_hw_queue takes a non-raw
spinlock with the preemption-disabled region.  If there is contention
on the lock, this violates the rules for preemption-disabled regions.

While this could be fixed easily within the RT patches just by doing
migrate_disable/enable (note: this was not tested :)), we can do
better.  The first patch concentrates the preempt_disable/enable in
a single place in blk_mq_run_hw_queue, and also avoids useless calls
when the caller wants to start __blk_mq_run_hw_queue asynchronously.
(There is already a call to __blk_mq_run_hw_queue that does not disable
preemption in blk-flush.c; not coincidentially, it passes async=true).

Once this is done, it is trivial to use get/put_cpu instead of
preempt_disable/smp_processor_id/preempt_enable, which is what the
second patch does.  The RT patches then can change this to use
get_cpu_light.

With these changes (and the additional switch to get_cpu_light),
virtio-blk can be used again with RT kernels.

Paolo

Paolo Bonzini (2):
  blk_mq: call preempt_disable/enable in blk_mq_run_hw_queue, and only
    if needed
  blk-mq: use get_cpu/put_cpu instead of preempt_disable/preempt_enable

 block/blk-mq.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

-- 
2.1.0

[PATCH 1/2] blk_mq: call preempt_disable/enable in blk_mq_run_hw_queue, and only if needed

[Paolo Bonzini]

preempt_disable/enable surrounds every call to blk_mq_run_hw_queue,
except the one in blk-flush.c.  In fact that one is always asynchronous,
and it does not need smp_processor_id().

We can do the same for all other calls, avoiding preempt_disable when
async is true.  This avoids peppering blk-mq.c with preemption-disabled
regions.

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Reported-by: Clark Williams <williams@redhat.com>
Tested-by: Clark Williams <williams@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/blk-mq.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index df8e1e09dd17..c6192ba78950 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -828,9 +828,18 @@ void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
 	if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
 		return;
 
-	if (!async && cpumask_test_cpu(smp_processor_id(), hctx->cpumask))
-		__blk_mq_run_hw_queue(hctx);
-	else if (hctx->queue->nr_hw_queues == 1)
+	if (!async) {
+		preempt_disable();
+		if (cpumask_test_cpu(smp_processor_id(), hctx->cpumask)) {
+			__blk_mq_run_hw_queue(hctx);
+			preempt_enable();
+			return;
+		}
+
+		preempt_enable();
+	}
+
+	if (hctx->queue->nr_hw_queues == 1)
 		kblockd_schedule_delayed_work(&hctx->run_work, 0);
 	else {
 		unsigned int cpu;
@@ -851,9 +860,7 @@ void blk_mq_run_queues(struct request_queue *q, bool async)
 		    test_bit(BLK_MQ_S_STOPPED, &hctx->state))
 			continue;
 
-		preempt_disable();
 		blk_mq_run_hw_queue(hctx, async);
-		preempt_enable();
 	}
 }
 EXPORT_SYMBOL(blk_mq_run_queues);
@@ -880,9 +887,7 @@ void blk_mq_start_hw_queue(struct blk_mq_hw_ctx *hctx)
 {
 	clear_bit(BLK_MQ_S_STOPPED, &hctx->state);
 
-	preempt_disable();
 	blk_mq_run_hw_queue(hctx, false);
-	preempt_enable();
 }
 EXPORT_SYMBOL(blk_mq_start_hw_queue);
 
@@ -907,9 +912,7 @@ void blk_mq_start_stopped_hw_queues(struct request_queue *q, bool async)
 			continue;
 
 		clear_bit(BLK_MQ_S_STOPPED, &hctx->state);
-		preempt_disable();
 		blk_mq_run_hw_queue(hctx, async);
-		preempt_enable();
 	}
 }
 EXPORT_SYMBOL(blk_mq_start_stopped_hw_queues);
-- 
2.1.0

[PATCH 2/2] blk-mq: use get_cpu/put_cpu instead of preempt_disable/preempt_enable

[Paolo Bonzini]

blk-mq is using preempt_disable/enable in order to ensure that the
queue runners are placed on the right CPU.  This does not work with
the RT patches, because __blk_mq_run_hw_queue takes a non-raw
spinlock with the preemption-disabled region.  If there is contention
on the lock, this violates the rules for preemption-disabled regions.

While this should be easily fixable within the RT patches just by doing
migrate_disable/enable, we can do better and document _why_ this
particular region runs with disabled preemption.  After the previous
patch, it is trivial to switch it to get/put_cpu; the RT patches then
can change it to get_cpu_light, which lets virtio-blk run under RT
kernels.

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Reported-by: Clark Williams <williams@redhat.com>
Tested-by: Clark Williams <williams@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/blk-mq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index c6192ba78950..a958851d9dd2 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -829,14 +829,14 @@ void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
 		return;
 
 	if (!async) {
-		preempt_disable();
-		if (cpumask_test_cpu(smp_processor_id(), hctx->cpumask)) {
+		int cpu = get_cpu();
+		if (cpumask_test_cpu(cpu, hctx->cpumask)) {
 			__blk_mq_run_hw_queue(hctx);
-			preempt_enable();
+			put_cpu();
 			return;
 		}
 
-		preempt_enable();
+		put_cpu();
 	}
 
 	if (hctx->queue->nr_hw_queues == 1)
-- 
2.1.0

Re: [PATCH 0/2] blk-mq: switch from preempt_disable/enable to get/put_cpu

[Jens Axboe]

On 2014-11-07 15:03, Paolo Bonzini wrote:
> blk-mq is using preempt_disable/enable in order to ensure that the
> queue runners are placed on the right CPU.  This does not work with
> the RT patches, because __blk_mq_run_hw_queue takes a non-raw
> spinlock with the preemption-disabled region.  If there is contention
> on the lock, this violates the rules for preemption-disabled regions.
>
> While this could be fixed easily within the RT patches just by doing
> migrate_disable/enable (note: this was not tested :)), we can do
> better.  The first patch concentrates the preempt_disable/enable in
> a single place in blk_mq_run_hw_queue, and also avoids useless calls
> when the caller wants to start __blk_mq_run_hw_queue asynchronously.
> (There is already a call to __blk_mq_run_hw_queue that does not disable
> preemption in blk-flush.c; not coincidentially, it passes async=true).
>
> Once this is done, it is trivial to use get/put_cpu instead of
> preempt_disable/smp_processor_id/preempt_enable, which is what the
> second patch does.  The RT patches then can change this to use
> get_cpu_light.
>
> With these changes (and the additional switch to get_cpu_light),
> virtio-blk can be used again with RT kernels.

These both look good. I'll double check and run through some testing, 
then apply for 3.19 if it all passes.


-- 
Jens Axboe

Re: [PATCH 0/2] blk-mq: switch from preempt_disable/enable to get/put_cpu

[Jens Axboe]

On 2014-11-09 20:16, Jens Axboe wrote:
> On 2014-11-07 15:03, Paolo Bonzini wrote:
>> blk-mq is using preempt_disable/enable in order to ensure that the
>> queue runners are placed on the right CPU.  This does not work with
>> the RT patches, because __blk_mq_run_hw_queue takes a non-raw
>> spinlock with the preemption-disabled region.  If there is contention
>> on the lock, this violates the rules for preemption-disabled regions.
>>
>> While this could be fixed easily within the RT patches just by doing
>> migrate_disable/enable (note: this was not tested :)), we can do
>> better.  The first patch concentrates the preempt_disable/enable in
>> a single place in blk_mq_run_hw_queue, and also avoids useless calls
>> when the caller wants to start __blk_mq_run_hw_queue asynchronously.
>> (There is already a call to __blk_mq_run_hw_queue that does not disable
>> preemption in blk-flush.c; not coincidentially, it passes async=true).
>>
>> Once this is done, it is trivial to use get/put_cpu instead of
>> preempt_disable/smp_processor_id/preempt_enable, which is what the
>> second patch does.  The RT patches then can change this to use
>> get_cpu_light.
>>
>> With these changes (and the additional switch to get_cpu_light),
>> virtio-blk can be used again with RT kernels.
>
> These both look good. I'll double check and run through some testing,
> then apply for 3.19 if it all passes.

Looks fine to me, I've applied it now. Thanks!

-- 
Jens Axboe