Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
IronPort-SDR: s/Dixj2h4cU9ZZqhMUwL3bKObAI0bTLDhX27O94doz8T/+KRDlJROjYVXwQbYmyeG3umX2N1w0
 pr2lPNqmdsQtiJuWKSz8mpG5pzL7YmO/hK0IcnS+0ZUM0oop6DoS/8g4o9t5Kpw8hWVIc/ZCSo
 oRe3FkWzNlJdjoJxt1seDwqGIgR41FDXOR+9ddZPHMvi813jNC+33Nc9WOUyqPeLL6BW8Ip/Ay
 tLruYJhEm0oeNv30GAEz8H3ux9hcbZCQV/kxg85BT4IiUkPWrbnpWm6pVKgBkmsudFG9X8hSYe
 YDE=
IronPort-SDR: C7ttfXAjIYrLvzclU+swGgL4Qu3XYAlxcTnkTMcOk2xoPXYkqErC2++G7J7MP58U+rTKx/G2iJ
 BIW/KzK7KssSis5gmQqFCAr9/VKIzsSx0iM1/D2Dj/EGparkikSmwdN9o/+j8ANFFPf0cQVJ8u
 Mny+X4oTkX0Go2CketGcPEsG1OtsWbOejStc1itsdey0DSkmwEQ8hxs/6sysQ20LYrXT4esof5
 GT2SMkTaGOYx9HubfPTDQMel9apBAR2/oUhe7yBkvZFQ5kZAwWK54Xi/rI6UXaXbs3OrosbSNM
 FsI=
WDCIronportException: Internal
Message-ID: <eb58939f-567e-c0c1-bafb-383f18f3d58e@opensource.wdc.com>
Date:   Wed, 21 Dec 2022 09:50:37 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.0
Subject: Re: [PATCH V10 1/8] block, bfq: split sync bfq_queues on a
 per-actuator basis
Content-Language: en-US
To:     Paolo Valente <paolo.valente@linaro.org>
Cc:     Jens Axboe <axboe@kernel.dk>,
        linux-block <linux-block@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Arie van der Hoeven <arie.vanderhoeven@seagate.com>,
        Rory Chen <rory.c.chen@seagate.com>,
        Glen Valante <glen.valante@linaro.org>,
        Gabriele Felici <felicigb@gmail.com>,
        Carmine Zaccagnino <carmine@carminezacc.com>
References: <20221209094442.36896-1-paolo.valente@linaro.org>
 <20221209094442.36896-2-paolo.valente@linaro.org>
 <cd41583b-ef11-a3b7-1e39-c4a224050c7d@opensource.wdc.com>
 <60582F89-8020-4468-80FE-BC52202D1129@linaro.org>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <60582F89-8020-4468-80FE-BC52202D1129@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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