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;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PATCH V10 1/8] block, bfq: split sync bfq_queues on a
 per-actuator basis
From:   Paolo Valente <paolo.valente@linaro.org>
In-Reply-To: <eb58939f-567e-c0c1-bafb-383f18f3d58e@opensource.wdc.com>
Date:   Wed, 21 Dec 2022 11:13:07 +0100
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>
Content-Transfer-Encoding: quoted-printable
Message-Id: <3E955C95-3912-46D0-A48C-9709F094FD0D@linaro.org>
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>
 <eb58939f-567e-c0c1-bafb-383f18f3d58e@opensource.wdc.com>
To:     Damien Le Moal <damien.lemoal@opensource.wdc.com>
Precedence: bulk


> Il giorno 21 dic 2022, alle ore 01:50, Damien Le Moal =
<damien.lemoal@opensource.wdc.com> ha scritto:
>=20
> 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 =3D 1;
>>>> +	for (act_idx =3D 0; act_idx < bfqd->num_actuators; act_idx++) {
>>>> +		struct bfq_queue *bfqq =3D
>>>> +			bic ? bic_to_bfqq(bic, op_is_sync(opf), act_idx) =
: NULL;
>>>=20
>>> Commented already: why not add a "if (!bfqq) return NULL;" in
>>> bic_to_bfqq() ?
>>=20
>> 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 :(
>=20
> But then you have *a lot* of "if (bfqd)" tests that are useless =
elsewhere since
> bic_to_bfqq() never returns NULL.
>=20

I'm probably misunderstanding your point, sorry.  Could you point me
to one of the places where there is the useless control that would go
away if we add your proposed control inside bic_to_bfqq?  (of course
apart form the above one, which seems to be the only one to me)

> And for this line, I personally would prefer seeing something like:
>=20
> 		struct bfq_queue *bfqq;
>=20
>=20
> 		if (bic)
> 			bfqd =3D bic_to_bfqq(bic, op_is_sync(opf), =
act_idx)
> 		else
> 			bfqd =3D NULL;
>=20
> Which is a lot simpler to read.
>=20
>=20

Ok, as I have your blessing on this point, I'll send a V12 with also
this change.

Thanks,
Paolo

> --=20
> Damien Le Moal
> Western Digital Research
>=20