[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <db66d308-5c3f-ee54-becf-682397b71b38@huaweicloud.com>
Date: Thu, 31 Jul 2025 08:49:01 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Bart Van Assche <bvanassche@....org>, Yu Kuai <yukuai1@...weicloud.com>,
dlemoal@...nel.org, hare@...e.de, 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 4/5] blk-mq-sched: refactor
__blk_mq_do_dispatch_sched()
Hi,
在 2025/07/31 2:32, Bart Van Assche 写道:
> On 7/30/25 1:22 AM, Yu Kuai wrote:
>> Introduce struct sched_dispatch_ctx, and split the helper into
>> elevator_dispatch_one_request() and elevator_finish_dispatch(). Also
>> and comments about the non-error return value.
>
> and -> add
>
>> +struct sched_dispatch_ctx {
>> + struct blk_mq_hw_ctx *hctx;
>> + struct elevator_queue *e;
>> + struct request_queue *q;
>
> 'e' is always equal to q->elevator so I'm not sure whether it's worth to
> have the member 'e'?
>
>> +static bool elevator_can_dispatch(struct sched_dispatch_ctx *ctx)
>> +{
>> + if (ctx->e->type->ops.has_work &&
>> + !ctx->e->type->ops.has_work(ctx->hctx))
>> + return false;
>> - if (!list_empty_careful(&hctx->dispatch)) {
>> - busy = true;
>> - break;
>> - }
>> + if (!list_empty_careful(&ctx->hctx->dispatch)) {
>> + ctx->busy = true;
>> + return false;
>> + }
>> - budget_token = blk_mq_get_dispatch_budget(q);
>> - if (budget_token < 0)
>> - break;
>> + return true;
>> +}
>
> Shouldn't all function names in this file start with the blk_mq_ prefix?
Ok
>
> Additionally, please rename elevator_can_dispatch() into
> elevator_should_dispatch(). I think the latter name better reflects the
> purpose of this function.
Sounds good.
>
>> + if (sq_sched)
>> + spin_lock_irq(&ctx->e->lock);
>> + rq = ctx->e->type->ops.dispatch_request(ctx->hctx);
>> + if (sq_sched)
>> + spin_unlock_irq(&ctx->e->lock);
>
> Same comment here as on patch 1/5: code like the above makes it
> harder than necessary for static analyzers to verify this code.
Ok
>
>> + if (!rq) {
>> + blk_mq_put_dispatch_budget(ctx->q, budget_token);
>> /*
>> - * If we cannot get tag for the request, stop dequeueing
>> - * requests from the IO scheduler. We are unlikely to be able
>> - * to submit them anyway and it creates false impression for
>> - * scheduling heuristics that the device can take more IO.
>> + * We're releasing without dispatching. Holding the
>> + * budget could have blocked any "hctx"s with the
>> + * same queue and if we didn't dispatch then there's
>> + * no guarantee anyone will kick the queue. Kick it
>> + * ourselves.
>> */
>
> Please keep the original comment. To me the new comment seems less clear
> than the existing comment.
Please note that I didn't change the comment here, above comment is for
setting the run_queue. The original comment for blk_mq_get_driver_tag()
is still there.
>
>> +static int __blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
>> +{
>> + unsigned int max_dispatch;
>> + struct sched_dispatch_ctx ctx = {
>> + .hctx = hctx,
>> + .q = hctx->queue,
>> + .e = hctx->queue->elevator,
>> + };
>> +
>> + INIT_LIST_HEAD(&ctx.rq_list);
>
> Please remove the INIT_LIST_HEAD() invocation and add the following in
> the ctx declaration:
>
> .rq_list = LIST_HEAD_INIT(ctx.rq_list),
>
> This is a common pattern in kernel code. The following grep command
> yields about 200 results:
>
> $ git grep -nH '= LIST_HEAD_INIT.*\.'
Ok
>
> Otherwise this patch looks good to me.
>
Thanks for the review!
Kuai
> Thanks,
>
> Bart.
> .
>
Powered by blists - more mailing lists