linux-kernel - Re: [PATCH v3 1/5] blk-mq-sched: introduce high level elevator lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6a0a203c-c25d-4d01-9295-8d78bb897a07@kernel.org>
Date: Mon, 11 Aug 2025 12:53:01 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Yu Kuai <yukuai1@...weicloud.com>, hare@...e.de, jack@...e.cz,
 bvanassche@....org, tj@...nel.org, josef@...icpanda.com, axboe@...nel.dk
Cc: cgroups@...r.kernel.org, linux-block@...r.kernel.org,
 linux-kernel@...r.kernel.org, yi.zhang@...wei.com, yangerkun@...wei.com,
 johnny.chenyi@...wei.com, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH v3 1/5] blk-mq-sched: introduce high level elevator lock

On 8/11/25 10:01, Yu Kuai wrote:
>>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
>>> index 55a0fd105147..1a2da5edbe13 100644
>>> --- a/block/blk-mq-sched.c
>>> +++ b/block/blk-mq-sched.c
>>> @@ -113,7 +113,14 @@ static int __blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
>>>   		if (budget_token < 0)
>>>   			break;
>>>   
>>> -		rq = e->type->ops.dispatch_request(hctx);
>>> +		if (blk_queue_sq_sched(q)) {
>>> +			elevator_lock(e);
>>> +			rq = e->type->ops.dispatch_request(hctx);
>>> +			elevator_unlock(e);
>>
>> I do not think this is safe for bfq since bfq uses the irqsave/irqrestore spin
>> lock variant. If it is safe, this needs a big comment block explaining why
>> and/or the rules regarding the scheduler use of this lock.
> 
> It's correct, however, this patch doesn't change bfq yet, and it's like:
> 
> elevator_lock
> spin_lock_irq(&bfqd->lock)
> spin_unlock_irq(&bfqd->lock)
> elevator_unlock
> 
> Patch 3 remove bfqd->lock and convert this to:
> 
> elevator_lock_irq
> elevator_unlock_irq.

I do not understand. Since q->elevator->lock is already taken here, without IRQ
disabled, how can bfq_dispatch_request method again take this same lock with IRQ
disabled ? That cannot possibly work.

The other side of this is that if you use elevator_lock(e) to call
e->type->ops.dispatch_request(hctx), it means that the scheduler *can NOT* use
that same lock in its completion path, since that can be called from IRQ
context. This may not be needed/a problem right now, but I think this needs a
comment in this patch to mention this.

-- 
Damien Le Moal
Western Digital Research