Re: [PATCH v2 4/5] blk-mq-sched: refactor __blk_mq_do_dispatch_sched()

[Yu Kuai <yukuai1@huaweicloud.com>]

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.
> .
>