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@xxxxxxxxxxxxx>
> ---
> 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
