* [PATCH] blk-mq: Remove get_cpu() in __blk_mq_delay_run_hw_queue().
@ 2022-02-15 9:11 Sebastian Andrzej Siewior
2022-02-15 9:23 ` Ming Lei
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-02-15 9:11 UTC (permalink / raw)
To: linux-block; +Cc: Jens Axboe, Thomas Gleixner, Paolo Bonzini, Ming Lei
The callchain:
__blk_mq_delay_run_hw_queue(x, false, x)
cpu = get_cpu();
cpumask_test_cpu(cpu, hctx->cpumask);
__blk_mq_run_hw_queue(hctx);
blk_mq_run_dispatch_ops();
__blk_mq_run_dispatch_ops(x, true, x);
might_sleep_if(true);
will trigger the might_sleep warning since get_cpu() disables
preemption. Based on my understanding, __blk_mq_run_hw_queue() should
run on the requested CPU for the benefit of cache locality. The system
won't crash if it runs on another CPU but the performance will be
probably less than optimal.
If the current CPU matches the requested cpumask then it may remain and
fulfill the requirement. If the scheduler moves the task to another CPU
then it will run on the wrong CPU but no harm is done in terms of
correctness.
Remove get_cpu() from __blk_mq_delay_run_hw_queue().
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
I have nothing to trigger that path, this is based on review. I see the
callchain but with blk_queue_has_srcu == 0.
The calls I see are from per-CPU threads and from unbound CPU threads.
If the correct CPU is important then migrate_disable() could be used
(slightly higher overhead than preempt_disable() but preemptible) or
unconditionally pass to kblockd_mod_delayed_work_on() (higher overhead
than migrate_disable() but will be done anyway if the current CPU is
wrong).
block/blk-mq.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 1adfe4824ef5e..90217f1b09add 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2040,14 +2040,10 @@ static void __blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async,
return;
if (!async && !(hctx->flags & BLK_MQ_F_BLOCKING)) {
- int cpu = get_cpu();
- if (cpumask_test_cpu(cpu, hctx->cpumask)) {
+ if (cpumask_test_cpu(raw_smp_processor_id(), hctx->cpumask)) {
__blk_mq_run_hw_queue(hctx);
- put_cpu();
return;
}
-
- put_cpu();
}
kblockd_mod_delayed_work_on(blk_mq_hctx_next_cpu(hctx), &hctx->run_work,
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] blk-mq: Remove get_cpu() in __blk_mq_delay_run_hw_queue().
2022-02-15 9:11 [PATCH] blk-mq: Remove get_cpu() in __blk_mq_delay_run_hw_queue() Sebastian Andrzej Siewior
@ 2022-02-15 9:23 ` Ming Lei
2022-02-15 9:31 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 6+ messages in thread
From: Ming Lei @ 2022-02-15 9:23 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-block, Jens Axboe, Thomas Gleixner, Paolo Bonzini
On Tue, Feb 15, 2022 at 10:11:59AM +0100, Sebastian Andrzej Siewior wrote:
> The callchain:
> __blk_mq_delay_run_hw_queue(x, false, x)
> cpu = get_cpu();
> cpumask_test_cpu(cpu, hctx->cpumask);
> __blk_mq_run_hw_queue(hctx);
> blk_mq_run_dispatch_ops();
> __blk_mq_run_dispatch_ops(x, true, x);
> might_sleep_if(true);
>
> will trigger the might_sleep warning since get_cpu() disables
> preemption. Based on my understanding, __blk_mq_run_hw_queue() should
> run on the requested CPU for the benefit of cache locality. The system
> won't crash if it runs on another CPU but the performance will be
> probably less than optimal.
>
> If the current CPU matches the requested cpumask then it may remain and
> fulfill the requirement. If the scheduler moves the task to another CPU
> then it will run on the wrong CPU but no harm is done in terms of
> correctness.
>
> Remove get_cpu() from __blk_mq_delay_run_hw_queue().
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> I have nothing to trigger that path, this is based on review. I see the
> callchain but with blk_queue_has_srcu == 0.
> The calls I see are from per-CPU threads and from unbound CPU threads.
>
> If the correct CPU is important then migrate_disable() could be used
> (slightly higher overhead than preempt_disable() but preemptible) or
> unconditionally pass to kblockd_mod_delayed_work_on() (higher overhead
> than migrate_disable() but will be done anyway if the current CPU is
> wrong).
>
> block/blk-mq.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 1adfe4824ef5e..90217f1b09add 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2040,14 +2040,10 @@ static void __blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async,
> return;
>
> if (!async && !(hctx->flags & BLK_MQ_F_BLOCKING)) {
> - int cpu = get_cpu();
Did you observe the warning triggered? might_sleep_if(true) in
__blk_mq_run_dispatch_ops() is only supposed to be run in case of
(hctx->flags & BLK_MQ_F_BLOCKING).
Thanks,
Ming
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] blk-mq: Remove get_cpu() in __blk_mq_delay_run_hw_queue().
2022-02-15 9:23 ` Ming Lei
@ 2022-02-15 9:31 ` Sebastian Andrzej Siewior
2022-02-15 11:58 ` Ming Lei
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-02-15 9:31 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-block, Jens Axboe, Thomas Gleixner, Paolo Bonzini
On 2022-02-15 17:23:45 [+0800], Ming Lei wrote:
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index 1adfe4824ef5e..90217f1b09add 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -2040,14 +2040,10 @@ static void __blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async,
> > return;
> >
> > if (!async && !(hctx->flags & BLK_MQ_F_BLOCKING)) {
> > - int cpu = get_cpu();
>
> Did you observe the warning triggered? might_sleep_if(true) in
> __blk_mq_run_dispatch_ops() is only supposed to be run in case of
> (hctx->flags & BLK_MQ_F_BLOCKING).
I haven't seen it triggering but if that call chain is possible then it
will triggert due to get_cpu().
> Thanks,
> Ming
Sebastian
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] blk-mq: Remove get_cpu() in __blk_mq_delay_run_hw_queue().
2022-02-15 9:31 ` Sebastian Andrzej Siewior
@ 2022-02-15 11:58 ` Ming Lei
2022-02-15 12:11 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 6+ messages in thread
From: Ming Lei @ 2022-02-15 11:58 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-block, Jens Axboe, Thomas Gleixner, Paolo Bonzini
On Tue, Feb 15, 2022 at 10:31:49AM +0100, Sebastian Andrzej Siewior wrote:
> On 2022-02-15 17:23:45 [+0800], Ming Lei wrote:
> > > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > > index 1adfe4824ef5e..90217f1b09add 100644
> > > --- a/block/blk-mq.c
> > > +++ b/block/blk-mq.c
> > > @@ -2040,14 +2040,10 @@ static void __blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async,
> > > return;
> > >
> > > if (!async && !(hctx->flags & BLK_MQ_F_BLOCKING)) {
> > > - int cpu = get_cpu();
> >
> > Did you observe the warning triggered? might_sleep_if(true) in
> > __blk_mq_run_dispatch_ops() is only supposed to be run in case of
> > (hctx->flags & BLK_MQ_F_BLOCKING).
>
> I haven't seen it triggering but if that call chain is possible then it
> will triggert due to get_cpu().
As I mentioned, the call chain isn't possible.
Thanks,
Ming
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] blk-mq: Remove get_cpu() in __blk_mq_delay_run_hw_queue().
2022-02-15 11:58 ` Ming Lei
@ 2022-02-15 12:11 ` Sebastian Andrzej Siewior
2022-02-15 12:22 ` Ming Lei
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-02-15 12:11 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-block, Jens Axboe, Thomas Gleixner, Paolo Bonzini
On 2022-02-15 19:58:03 [+0800], Ming Lei wrote:
> On Tue, Feb 15, 2022 at 10:31:49AM +0100, Sebastian Andrzej Siewior wrote:
> > On 2022-02-15 17:23:45 [+0800], Ming Lei wrote:
> > > > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > > > index 1adfe4824ef5e..90217f1b09add 100644
> > > > --- a/block/blk-mq.c
> > > > +++ b/block/blk-mq.c
> > > > @@ -2040,14 +2040,10 @@ static void __blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async,
> > > > return;
> > > >
> > > > if (!async && !(hctx->flags & BLK_MQ_F_BLOCKING)) {
> > > > - int cpu = get_cpu();
> > >
> > > Did you observe the warning triggered? might_sleep_if(true) in
> > > __blk_mq_run_dispatch_ops() is only supposed to be run in case of
> > > (hctx->flags & BLK_MQ_F_BLOCKING).
> >
> > I haven't seen it triggering but if that call chain is possible then it
> > will triggert due to get_cpu().
>
> As I mentioned, the call chain isn't possible.
Why not? Is BLK_MQ_F_BLOCKING and blk_queue_has_srcu() exclusive?
> Thanks,
> Ming
Sebastian
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] blk-mq: Remove get_cpu() in __blk_mq_delay_run_hw_queue().
2022-02-15 12:11 ` Sebastian Andrzej Siewior
@ 2022-02-15 12:22 ` Ming Lei
0 siblings, 0 replies; 6+ messages in thread
From: Ming Lei @ 2022-02-15 12:22 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-block, Jens Axboe, Thomas Gleixner, Paolo Bonzini
On Tue, Feb 15, 2022 at 01:11:24PM +0100, Sebastian Andrzej Siewior wrote:
> On 2022-02-15 19:58:03 [+0800], Ming Lei wrote:
> > On Tue, Feb 15, 2022 at 10:31:49AM +0100, Sebastian Andrzej Siewior wrote:
> > > On 2022-02-15 17:23:45 [+0800], Ming Lei wrote:
> > > > > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > > > > index 1adfe4824ef5e..90217f1b09add 100644
> > > > > --- a/block/blk-mq.c
> > > > > +++ b/block/blk-mq.c
> > > > > @@ -2040,14 +2040,10 @@ static void __blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async,
> > > > > return;
> > > > >
> > > > > if (!async && !(hctx->flags & BLK_MQ_F_BLOCKING)) {
> > > > > - int cpu = get_cpu();
> > > >
> > > > Did you observe the warning triggered? might_sleep_if(true) in
> > > > __blk_mq_run_dispatch_ops() is only supposed to be run in case of
> > > > (hctx->flags & BLK_MQ_F_BLOCKING).
> > >
> > > I haven't seen it triggering but if that call chain is possible then it
> > > will triggert due to get_cpu().
> >
> > As I mentioned, the call chain isn't possible.
>
> Why not? Is BLK_MQ_F_BLOCKING and blk_queue_has_srcu() exclusive?
blk_queue_has_srcu() returns true iff BLK_MQ_F_BLOCKING is set.
thanks,
Ming
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-15 12:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-15 9:11 [PATCH] blk-mq: Remove get_cpu() in __blk_mq_delay_run_hw_queue() Sebastian Andrzej Siewior
2022-02-15 9:23 ` Ming Lei
2022-02-15 9:31 ` Sebastian Andrzej Siewior
2022-02-15 11:58 ` Ming Lei
2022-02-15 12:11 ` Sebastian Andrzej Siewior
2022-02-15 12:22 ` Ming Lei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox