Re: [PATCH] blk-mq: Remove get_cpu() in __blk_mq_delay_run_hw_queue().

[Ming Lei <ming.lei@redhat.com>]

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