Re: [PATCH V9 4/7] blk-mq: introduce .get_budget and .put_budget in blk_mq_ops

[Bart Van Assche <Bart.VanAssche@wdc.com>]

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.