On Sat, 2017-10-14 at 02:05 +0800, Ming Lei wrote:
> @@ -89,19 +89,36 @@ static bool blk_mq_sched_restart_hctx(struct
> blk_mq_hw_ctx *hctx)
> return false;
> }
>
> -static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
> +static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
Shouldn't the meaning of the return value of this function be documented?
> {
> struct request_queue *q = hctx->queue;
> struct elevator_queue *e = q->elevator;
> LIST_HEAD(rq_list);
>
> do {
> - struct request *rq = e->type->ops.mq.dispatch_request(hctx);
> + struct request *rq;
> + blk_status_t ret;
>
> - if (!rq)
> + if (e->type->ops.mq.has_work &&
> + !e->type->ops.mq.has_work(hctx))
> break;
> +
> + ret = blk_mq_get_dispatch_budget(hctx);
> + if (ret == BLK_STS_RESOURCE)
> + return true;
> +
> + rq = e->type->ops.mq.dispatch_request(hctx);
> + if (!rq) {
> + blk_mq_put_dispatch_budget(hctx, true);
> + break;
> + } else if (ret != BLK_STS_OK) {
> + blk_mq_end_request(rq, ret);
> + continue;
> + }
> list_add(&rq->queuelist, &rq_list);
> - } while (blk_mq_dispatch_rq_list(q, &rq_list));
> + } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
> +
> + return false;
> }
This means that the request in rq_list becomes the owner of the budget allocated
by blk_mq_get_dispatch_budget(). Shouldn't that be mentioned as a comment above
blk_mq_dispatch_rq_list()?
> + if (run_queue) {
> + if (!blk_mq_sched_needs_restart(hctx) &&
> + !test_bit(BLK_MQ_S_TAG_WAITING, &hctx->state)) {
> + blk_mq_sched_mark_restart_hctx(hctx);
> + blk_mq_run_hw_queue(hctx, true);
> + }
> }
> }
The above if-statement can be changed from a nested if into a single
if-statement.
Additionally, why has the code been added to blk_mq_sched_dispatch_requests()
that reruns the queue if blk_mq_get_dispatch_budget() returned BLK_STS_RESOURCE?
Is that code necessary or can it be left out?
> +static inline void blk_mq_put_dispatch_budget(struct blk_mq_hw_ctx *hctx,
> + bool got_budget)
> +{
> + struct request_queue *q = hctx->queue;
> +
> + if (q->mq_ops->put_budget && got_budget)
> + q->mq_ops->put_budget(hctx);
> +}
So the above function is passed a boolean as second argument and all what
that boolean is used for is to decide whether or not the function is executed?
Sorry but I think that's wrong and that the second argument should be removed
and that it should be evaluated by the caller instead of inside
blk_mq_put_dispatch_budget().
Bart.
