linux-kernel - Re: [PATCH V10 1/8] block, bfq: split sync bfq

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]

Message-ID: <eb58939f-567e-c0c1-bafb-383f18f3d58e@opensource.wdc.com>
Date:   Wed, 21 Dec 2022 09:50:37 +0900
From:   Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To:     Paolo Valente <paolo.valente@...aro.org>
Cc:     Jens Axboe <axboe@...nel.dk>,
        linux-block <linux-block@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Arie van der Hoeven <arie.vanderhoeven@...gate.com>,
        Rory Chen <rory.c.chen@...gate.com>,
        Glen Valante <glen.valante@...aro.org>,
        Gabriele Felici <felicigb@...il.com>,
        Carmine Zaccagnino <carmine@...minezacc.com>
Subject: Re: [PATCH V10 1/8] block, bfq: split sync bfq_queues on a
 per-actuator basis

On 2022/12/20 22:10, Paolo Valente wrote:
>>> -	/*
>>> -	 * Does queue (or any parent entity) exceed number of requests that
>>> -	 * should be available to it? Heavily limit depth so that it cannot
>>> -	 * consume more available requests and thus starve other entities.
>>> -	 */
>>> -	if (bfqq && bfqq_request_over_limit(bfqq, limit))
>>> -		depth = 1;
>>> +	for (act_idx = 0; act_idx < bfqd->num_actuators; act_idx++) {
>>> +		struct bfq_queue *bfqq =
>>> +			bic ? bic_to_bfqq(bic, op_is_sync(opf), act_idx) : NULL;
>>
>> Commented already: why not add a "if (!bfqq) return NULL;" in
>> bic_to_bfqq() ?
> 
> You have probably missed my reply on this.  The problem is that your
> proposal would improve code (only) here, but it would entail the above
> control for all the other invocations, for which it is useless :(

But then you have *a lot* of "if (bfqd)" tests that are useless elsewhere since
bic_to_bfqq() never returns NULL.

And for this line, I personally would prefer seeing something like:

		struct bfq_queue *bfqq;


		if (bic)
			bfqd = bic_to_bfqq(bic, op_is_sync(opf), act_idx)
		else
			bfqd = NULL;

Which is a lot simpler to read.


-- 
Damien Le Moal
Western Digital Research