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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Oct 2017 23:43:44 +0000
From:   Bart Van Assche <Bart.VanAssche@....com>
To:     "hch@...radead.org" <hch@...radead.org>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "axboe@...com" <axboe@...com>,
        "ming.lei@...hat.com" <ming.lei@...hat.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "tom81094@...il.com" <tom81094@...il.com>,
        "paolo.valente@...aro.org" <paolo.valente@...aro.org>,
        Bart Van Assche <Bart.VanAssche@....com>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "oleksandr@...alenko.name" <oleksandr@...alenko.name>,
        "john.garry@...wei.com" <john.garry@...wei.com>,
        "osandov@...com" <osandov@...com>,
        "loberman@...hat.com" <loberman@...hat.com>
Subject: Re: [PATCH V9 4/7] blk-mq: introduce .get_budget and .put_budget in
 blk_mq_ops

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.