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