Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PATCH BUGFIX/IMPROVEMENT 6/6] block, bfq: merge bursts of
 newly-created queues
From:   Paolo Valente <paolo.valente@linaro.org>
In-Reply-To: <a4874bb3-006c-85d9-5015-12443baa1e87@kernel.dk>
Date:   Thu, 25 Feb 2021 18:25:53 +0100
Cc:     linux-block <linux-block@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Jan Kara <jack@suse.cz>
Content-Transfer-Encoding: quoted-printable
Message-Id: <4F6EF0B5-A63D-4A0B-A563-71495D7FFDCE@linaro.org>
References: <20210126105102.53102-1-paolo.valente@linaro.org>
 <20210126105102.53102-7-paolo.valente@linaro.org>
 <a4874bb3-006c-85d9-5015-12443baa1e87@kernel.dk>
To:     Jens Axboe <axboe@kernel.dk>
Precedence: bulk


> Il giorno 26 gen 2021, alle ore 17:15, Jens Axboe <axboe@kernel.dk> ha =
scritto:
>=20
> On 1/26/21 3:51 AM, Paolo Valente wrote:
>> @@ -2809,6 +2853,12 @@ void bfq_release_process_ref(struct bfq_data =
*bfqd, struct bfq_queue *bfqq)
>> 	    bfqq !=3D bfqd->in_service_queue)
>> 		bfq_del_bfqq_busy(bfqd, bfqq, false);
>>=20
>> +	if (bfqq->entity.parent &&
>> +	    bfqq->entity.parent->last_bfqq_created =3D=3D bfqq)
>> +		bfqq->entity.parent->last_bfqq_created =3D NULL;
>> +	else if (bfqq->bfqd && bfqq->bfqd->last_bfqq_created =3D=3D =
bfqq)
>> +		bfqq->bfqd->last_bfqq_created =3D NULL;
>> +
>> 	bfq_put_queue(bfqq);
>> }
>>=20
>> @@ -2905,6 +2955,13 @@ bfq_merge_bfqqs(struct bfq_data *bfqd, struct =
bfq_io_cq *bic,
>> 	 */
>> 	new_bfqq->pid =3D -1;
>> 	bfqq->bic =3D NULL;
>> +
>> +	if (bfqq->entity.parent &&
>> +	    bfqq->entity.parent->last_bfqq_created =3D=3D bfqq)
>> +		bfqq->entity.parent->last_bfqq_created =3D new_bfqq;
>> +	else if (bfqq->bfqd && bfqq->bfqd->last_bfqq_created =3D=3D =
bfqq)
>> +		bfqq->bfqd->last_bfqq_created =3D new_bfqq;
>> +
>> 	bfq_release_process_ref(bfqd, bfqq);
>> }
>=20
> Almost identical code constructs makes it seem like this should have a
> helper instead.
>=20

Right, sorry. Improved in V2.

>> @@ -5033,6 +5090,12 @@ void bfq_put_queue(struct bfq_queue *bfqq)
>> 	bfqg_and_blkg_put(bfqg);
>> }
>>=20
>> +static void bfq_put_stable_ref(struct bfq_queue *bfqq)
>> +{
>> +	bfqq->stable_ref--;
>> +	bfq_put_queue(bfqq);
>> +}
>> +
>> static void bfq_put_cooperator(struct bfq_queue *bfqq)
>> {
>> 	struct bfq_queue *__bfqq, *next;
>> @@ -5089,6 +5152,17 @@ static void bfq_exit_icq(struct io_cq *icq)
>> {
>> 	struct bfq_io_cq *bic =3D icq_to_bic(icq);
>>=20
>> +	if (bic->stable_merge_bfqq) {
>> +		unsigned long flags;
>> +		struct bfq_data *bfqd =3D bic->stable_merge_bfqq->bfqd;
>> +
>> +		if (bfqd)
>> +			spin_lock_irqsave(&bfqd->lock, flags);
>> +		bfq_put_stable_ref(bic->stable_merge_bfqq);
>> +		if (bfqd)
>> +			spin_unlock_irqrestore(&bfqd->lock, flags);
>> +	}
>> +
>=20
> Construct like this are really painful. Just do:
>=20
> if (bfqd) {
> 	unsigned long flags;
>=20
> 	spin_lock_irqsave(&bfqd->lock, flags);
> 	bfq_put_stable_ref(bic->stable_merge_bfqq);
> 	spin_unlock_irqrestore(&bfqd->lock, flags);
> } else {
> 	bfq_put_stable_ref(bic->stable_merge_bfqq);
> }
>=20
> which is also less likely to cause code analyzer false warnings.

Done, thanks.

> Outside
> of that, it needs a comment on why it's ok NOT to grab the lock when
> bfqd is zero, because that seems counter-intuitive and more a case of
> "well we can't grab a lock for something we don't have". Maybe it's
> because bfqd is no longer visible at this point, and it's ok,

yes

> but it's
> definitely not clear just looking at this patch.

Right, the reason is already reported a few lines above, but not
repeated in this function.  I'll repeat it.


> Even with that, is the
> bfqq visible? Should the ref be atomic, and locking happen further =
down
> instead?
>=20

Since the scheduler is gone, no pending I/O is expected to still
reference bfqq.  I'll write this too in V2.

As I stated in my reply to another comments of yours, I'll submit the
V2 soon, unless I receive a reply before.

Thanks.
Paolo

> --=20
> Jens Axboe
>=20