| Message ID | YgtuX9dDpXe8Xbs+@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: Remove get_cpu() in __blk_mq_delay_run_hw_queue(). | expand |
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
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
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
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
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
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,
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(-)