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 V11 1/8] block, bfq: split sync bfq_queues on a
 per-actuator basis
From:   Paolo Valente <paolo.valente@linaro.org>
In-Reply-To: <8a49432d-642f-cd58-8e4d-2b320aef5edd@opensource.wdc.com>
Date:   Wed, 21 Dec 2022 11:27:28 +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: <D429FA01-9510-41FF-AD85-F74EB8AD171F@linaro.org>
References: <20221220095013.55803-1-paolo.valente@linaro.org>
 <20221220095013.55803-2-paolo.valente@linaro.org>
 <8a49432d-642f-cd58-8e4d-2b320aef5edd@opensource.wdc.com>
To:     Damien Le Moal <damien.lemoal@opensource.wdc.com>
Precedence: bulk


> Il giorno 21 dic 2022, alle ore 01:46, Damien Le Moal =
<damien.lemoal@opensource.wdc.com> ha scritto:
>=20
>=20
> [...]
>=20
>> -static void bfq_exit_icq_bfqq(struct bfq_io_cq *bic, bool is_sync)
>> +static void bfq_exit_icq_bfqq(struct bfq_io_cq *bic, bool is_sync,
>> +			      unsigned int actuator_idx)
>> {
>> -	struct bfq_queue *bfqq =3D bic_to_bfqq(bic, is_sync);
>> +	struct bfq_queue *bfqq =3D bic_to_bfqq(bic, is_sync, =
actuator_idx);
>> 	struct bfq_data *bfqd;
>>=20
>> 	if (bfqq)
>=20
> With your current bic_to_bfqq() implementation, you will *never* get =
NULL as a
> return value.

I'm afraid this is not true.  A bic is associated with a sync and an
async queue, or with both.  So, in the hunk above, bic_to_bfqq returns
NULL if:
- either the bic is associated with a sync queue, but is_sync happens to =
be false;
- or the bic is associate with an async queue, but is_sync happens to be =
true.

Of course, with these patches, the associations move from "with a
sync/async queue" to "with a set of sync/async queues, one per
actuator".

> So why is this if necessary ?
>> 		bfqd =3D bfqq->bfqd; /* NULL if scheduler already exited =
*/
>>=20
>> 	if (bfqq && bfqd) {
>> -		unsigned long flags;
>> -
>> -		spin_lock_irqsave(&bfqd->lock, flags);
>> 		bfqq->bic =3D NULL;
>> 		bfq_exit_bfqq(bfqd, bfqq);
>> -		bic_set_bfqq(bic, NULL, is_sync);
>> -		spin_unlock_irqrestore(&bfqd->lock, flags);
>> +		bic_set_bfqq(bic, NULL, is_sync, actuator_idx);
>> 	}
>> }
>>=20
>> static void bfq_exit_icq(struct io_cq *icq)
>> {
>> 	struct bfq_io_cq *bic =3D icq_to_bic(icq);
>> +	struct bfq_data *bfqd =3D bic_to_bfqd(bic);
>> +	unsigned long flags;
>> +	unsigned int act_idx;
>> +	/*
>> +	 * If bfqd and thus bfqd->num_actuators is not available any
>> +	 * longer, then cycle over all possible per-actuator bfqqs in
>> +	 * next loop. We rely on bic being zeroed on creation, and
>> +	 * therefore on its unused per-actuator fields being NULL.
>> +	 */
>> +	unsigned int num_actuators =3D BFQ_MAX_ACTUATORS;
>>=20
>> -	if (bic->stable_merge_bfqq) {
>> -		struct bfq_data *bfqd =3D bic->stable_merge_bfqq->bfqd;
>> +	/*
>> +	 * bfqd is NULL if scheduler already exited, and in that case
>> +	 * this is the last time these queues are accessed.
>> +	 */
>> +	if (bfqd) {
>=20
> Same here. bfqd can never be NULL. Or I am really missing something... =
Lots of
> other places like this where checking bic_to_bfqd() seems unnecessary.

As written in the comment above, bfqd is NULL if the scheduler already
exited.  That is, bic->icq.q->elevator->elevator_data =3D=3D NULL.  This
is an event I have checked several years ago, probably while porting
cfq to bfq.  If boundary conditions changed later, and nobody realized
that this was not true any longer, then bfqd would never be NULL as
you say.  At any rate, I guess that such a change would then belong to
a separate patch series.

Thanks,
Paolo=