linux-kernel - Re: [PATCH v2 2/5] mq-deadline: switch to use 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: <a3ce55a4-0756-bfe7-3606-296b78672104@huaweicloud.com>
Date: Thu, 31 Jul 2025 14:32:48 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Damien Le Moal <dlemoal@...nel.org>, Hannes Reinecke <hare@...e.de>,
 Yu Kuai <yukuai1@...weicloud.com>, jack@...e.cz, 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 v2 2/5] mq-deadline: switch to use elevator lock

Hi,

在 2025/07/31 14:22, Damien Le Moal 写道:
> On 7/31/25 3:20 PM, Hannes Reinecke wrote:
>> On 7/30/25 10:22, Yu Kuai wrote:
>>> From: Yu Kuai <yukuai3@...wei.com>
>>>
>>> Replace the internal spinlock 'dd->lock' with the new spinlock in
>>> elevator_queue, there are no functional changes.
>>>
>>> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
>>> ---
>>>    block/mq-deadline.c | 58 +++++++++++++++++++++------------------------
>>>    1 file changed, 27 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
>>> index 9ab6c6256695..2054c023e855 100644
>>> --- a/block/mq-deadline.c
>>> +++ b/block/mq-deadline.c
>>> @@ -101,7 +101,7 @@ struct deadline_data {
>>>        u32 async_depth;
>>>        int prio_aging_expire;
>>>    -    spinlock_t lock;
>>> +    spinlock_t *lock;
>>>    };
>>>      /* Maps an I/O priority class to a deadline scheduler priority. */
>>> @@ -213,7 +213,7 @@ static void dd_merged_requests(struct request_queue *q,
>>> struct request *req,
>>>        const u8 ioprio_class = dd_rq_ioclass(next);
>>>        const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>>>    -    lockdep_assert_held(&dd->lock);
>>> +    lockdep_assert_held(dd->lock);
>>>          dd->per_prio[prio].stats.merged++;
>>>    @@ -253,7 +253,7 @@ static u32 dd_queued(struct deadline_data *dd, enum
>>> dd_prio prio)
>>>    {
>>>        const struct io_stats_per_prio *stats = &dd->per_prio[prio].stats;
>>>    -    lockdep_assert_held(&dd->lock);
>>> +    lockdep_assert_held(dd->lock);
>>>          return stats->inserted - atomic_read(&stats->completed);
>>>    }
>>> @@ -323,7 +323,7 @@ static struct request *__dd_dispatch_request(struct
>>> deadline_data *dd,
>>>        enum dd_prio prio;
>>>        u8 ioprio_class;
>>>    -    lockdep_assert_held(&dd->lock);
>>> +    lockdep_assert_held(dd->lock);
>>>          if (!list_empty(&per_prio->dispatch)) {
>>>            rq = list_first_entry(&per_prio->dispatch, struct request,
>>> @@ -434,7 +434,7 @@ static struct request
>>> *dd_dispatch_prio_aged_requests(struct deadline_data *dd,
>>>        enum dd_prio prio;
>>>        int prio_cnt;
>>>    -    lockdep_assert_held(&dd->lock);
>>> +    lockdep_assert_held(dd->lock);
>>>          prio_cnt = !!dd_queued(dd, DD_RT_PRIO) + !!dd_queued(dd, DD_BE_PRIO) +
>>>               !!dd_queued(dd, DD_IDLE_PRIO);
>>> @@ -466,10 +466,9 @@ static struct request *dd_dispatch_request(struct
>>> blk_mq_hw_ctx *hctx)
>>>        struct request *rq;
>>>        enum dd_prio prio;
>>>    -    spin_lock(&dd->lock);
>>>        rq = dd_dispatch_prio_aged_requests(dd, now);
>>>        if (rq)
>>> -        goto unlock;
>>> +        return rq;
>>>          /*
>>>         * Next, dispatch requests in priority order. Ignore lower priority
>>> @@ -481,9 +480,6 @@ static struct request *dd_dispatch_request(struct
>>> blk_mq_hw_ctx *hctx)
>>>                break;
>>>        }
>>>    -unlock:
>>> -    spin_unlock(&dd->lock);
>>> -
>>>        return rq;
>>>    }
>>>    @@ -538,9 +534,9 @@ static void dd_exit_sched(struct elevator_queue *e)
>>>            WARN_ON_ONCE(!list_empty(&per_prio->fifo_list[DD_READ]));
>>>            WARN_ON_ONCE(!list_empty(&per_prio->fifo_list[DD_WRITE]));
>>>    -        spin_lock(&dd->lock);
>>> +        spin_lock(dd->lock);
>>>            queued = dd_queued(dd, prio);
>>> -        spin_unlock(&dd->lock);
>>> +        spin_unlock(dd->lock);
>>>              WARN_ONCE(queued != 0,
>>>                  "statistics for priority %d: i %u m %u d %u c %u\n",
>>
>> Do you still need 'dd->lock'? Can't you just refer to the lock from the
>> elevator_queue structure directly?
> 
> Indeed. Little inline helpers for locking/unlocking q->elevator->lock would be
> nice.

How about the first patch to factor out inline helpers like dd_lock()
and dd_unlock(), still use dd->lock without any functional changes, and
then switch to use q->elevator->lock in the next patch? (same for bfq)

Thanks,
Kuai

>