[PATCH] bfq: Fix computation of shallow depth

[PATCH] bfq: Fix computation of shallow depth

[Jan Kara]

BFQ computes number of tags it allows to be allocated for each request type
based on tag bitmap. However it uses 1 << bitmap.shift as number of
available tags which is wrong. 'shift' is just an internal bitmap value
containing logarithm of how many bits bitmap uses in each bitmap word.
Thus number of tags allowed for some request types can be far to low.
Use proper bitmap.depth which has the number of tags instead.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/bfq-iosched.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 9e81d1052091..9e4eb0fc1c16 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -6332,13 +6332,13 @@ static unsigned int bfq_update_depths(struct bfq_data *bfqd,
 	 * limit 'something'.
 	 */
 	/* no more than 50% of tags for async I/O */
-	bfqd->word_depths[0][0] = max((1U << bt->sb.shift) >> 1, 1U);
+	bfqd->word_depths[0][0] = max(bt->sb.depth >> 1, 1U);
 	/*
 	 * no more than 75% of tags for sync writes (25% extra tags
 	 * w.r.t. async I/O, to prevent async I/O from starving sync
 	 * writes)
 	 */
-	bfqd->word_depths[0][1] = max(((1U << bt->sb.shift) * 3) >> 2, 1U);
+	bfqd->word_depths[0][1] = max((bt->sb.depth * 3) >> 2, 1U);
 
 	/*
 	 * In-word depths in case some bfq_queue is being weight-
@@ -6348,9 +6348,9 @@ static unsigned int bfq_update_depths(struct bfq_data *bfqd,
 	 * shortage.
 	 */
 	/* no more than ~18% of tags for async I/O */
-	bfqd->word_depths[1][0] = max(((1U << bt->sb.shift) * 3) >> 4, 1U);
+	bfqd->word_depths[1][0] = max((bt->sb.depth * 3) >> 4, 1U);
 	/* no more than ~37% of tags for sync writes (~20% extra tags) */
-	bfqd->word_depths[1][1] = max(((1U << bt->sb.shift) * 6) >> 4, 1U);
+	bfqd->word_depths[1][1] = max((bt->sb.depth * 6) >> 4, 1U);
 
 	for (i = 0; i < 2; i++)
 		for (j = 0; j < 2; j++)
-- 
2.16.4

Re: [PATCH] bfq: Fix computation of shallow depth

[Jan Kara]

On Thu 10-12-20 10:44:33, Jan Kara wrote:
> BFQ computes number of tags it allows to be allocated for each request type
> based on tag bitmap. However it uses 1 << bitmap.shift as number of
> available tags which is wrong. 'shift' is just an internal bitmap value
> containing logarithm of how many bits bitmap uses in each bitmap word.
> Thus number of tags allowed for some request types can be far to low.
> Use proper bitmap.depth which has the number of tags instead.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Ping Jens? I think it has fallen through the cracks?

								Honza

> ---
>  block/bfq-iosched.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 9e81d1052091..9e4eb0fc1c16 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -6332,13 +6332,13 @@ static unsigned int bfq_update_depths(struct bfq_data *bfqd,
>  	 * limit 'something'.
>  	 */
>  	/* no more than 50% of tags for async I/O */
> -	bfqd->word_depths[0][0] = max((1U << bt->sb.shift) >> 1, 1U);
> +	bfqd->word_depths[0][0] = max(bt->sb.depth >> 1, 1U);
>  	/*
>  	 * no more than 75% of tags for sync writes (25% extra tags
>  	 * w.r.t. async I/O, to prevent async I/O from starving sync
>  	 * writes)
>  	 */
> -	bfqd->word_depths[0][1] = max(((1U << bt->sb.shift) * 3) >> 2, 1U);
> +	bfqd->word_depths[0][1] = max((bt->sb.depth * 3) >> 2, 1U);
>  
>  	/*
>  	 * In-word depths in case some bfq_queue is being weight-
> @@ -6348,9 +6348,9 @@ static unsigned int bfq_update_depths(struct bfq_data *bfqd,
>  	 * shortage.
>  	 */
>  	/* no more than ~18% of tags for async I/O */
> -	bfqd->word_depths[1][0] = max(((1U << bt->sb.shift) * 3) >> 4, 1U);
> +	bfqd->word_depths[1][0] = max((bt->sb.depth * 3) >> 4, 1U);
>  	/* no more than ~37% of tags for sync writes (~20% extra tags) */
> -	bfqd->word_depths[1][1] = max(((1U << bt->sb.shift) * 6) >> 4, 1U);
> +	bfqd->word_depths[1][1] = max((bt->sb.depth * 6) >> 4, 1U);
>  
>  	for (i = 0; i < 2; i++)
>  		for (j = 0; j < 2; j++)
> -- 
> 2.16.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] bfq: Fix computation of shallow depth

[Jens Axboe]

On 1/5/21 9:21 AM, Jan Kara wrote:
> On Thu 10-12-20 10:44:33, Jan Kara wrote:
>> BFQ computes number of tags it allows to be allocated for each request type
>> based on tag bitmap. However it uses 1 << bitmap.shift as number of
>> available tags which is wrong. 'shift' is just an internal bitmap value
>> containing logarithm of how many bits bitmap uses in each bitmap word.
>> Thus number of tags allowed for some request types can be far to low.
>> Use proper bitmap.depth which has the number of tags instead.
>>
>> Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Ping Jens? I think it has fallen through the cracks?

More like waiting for Paolo to take a look. Don't mind taking it, and
I'll do that now, but I do expect him to review any BFQ patches being
sent out.

-- 
Jens Axboe

Re: [PATCH] bfq: Fix computation of shallow depth

[Paolo Valente]

> Il giorno 5 gen 2021, alle ore 17:29, Jens Axboe <axboe@kernel.dk> ha scritto:
> 
> On 1/5/21 9:21 AM, Jan Kara wrote:
>> On Thu 10-12-20 10:44:33, Jan Kara wrote:
>>> BFQ computes number of tags it allows to be allocated for each request type
>>> based on tag bitmap. However it uses 1 << bitmap.shift as number of
>>> available tags which is wrong. 'shift' is just an internal bitmap value
>>> containing logarithm of how many bits bitmap uses in each bitmap word.
>>> Thus number of tags allowed for some request types can be far to low.
>>> Use proper bitmap.depth which has the number of tags instead.
>>> 
>>> Signed-off-by: Jan Kara <jack@suse.cz>
>> 
>> Ping Jens? I think it has fallen through the cracks?
> 
> More like waiting for Paolo to take a look. Don't mind taking it, and
> I'll do that now, but I do expect him to review any BFQ patches being
> sent out.
> 

Sorry for the delay Jan.  As you know, my priority is currently to
finalize the patches I have developed with your help; and
unfortunately I'm way behind.  This is delaying also my review
activity.

As for your proposal, I remember I found the right parameter rather
empirically.  In particular, I seem to remember that the bitmap.depth
parameter did not contain the value I needed, i.e, it did not
contain the total number of tags.  But maybe something has changed in
the meantime.  At any rate, if bitmap.depth does contain that value,
then your replacement is ok.

If your replacement is ok, then I guess you may want to also fix the
comments above the changes you propose.

Thanks,
Paolo

> -- 
> Jens Axboe
> 

Re: [PATCH] bfq: Fix computation of shallow depth

[Jan Kara]

On Wed 06-01-21 18:02:03, Paolo Valente wrote:
> > Il giorno 5 gen 2021, alle ore 17:29, Jens Axboe <axboe@kernel.dk> ha scritto:
> > 
> > On 1/5/21 9:21 AM, Jan Kara wrote:
> >> On Thu 10-12-20 10:44:33, Jan Kara wrote:
> >>> BFQ computes number of tags it allows to be allocated for each request type
> >>> based on tag bitmap. However it uses 1 << bitmap.shift as number of
> >>> available tags which is wrong. 'shift' is just an internal bitmap value
> >>> containing logarithm of how many bits bitmap uses in each bitmap word.
> >>> Thus number of tags allowed for some request types can be far to low.
> >>> Use proper bitmap.depth which has the number of tags instead.
> >>> 
> >>> Signed-off-by: Jan Kara <jack@suse.cz>
> >> 
> >> Ping Jens? I think it has fallen through the cracks?
> > 
> > More like waiting for Paolo to take a look. Don't mind taking it, and
> > I'll do that now, but I do expect him to review any BFQ patches being
> > sent out.
> 
> Sorry for the delay Jan.  As you know, my priority is currently to
> finalize the patches I have developed with your help; and
> unfortunately I'm way behind.  This is delaying also my review
> activity.
> 
> As for your proposal, I remember I found the right parameter rather
> empirically.  In particular, I seem to remember that the bitmap.depth
> parameter did not contain the value I needed, i.e, it did not
> contain the total number of tags.  But maybe something has changed in
> the meantime.  At any rate, if bitmap.depth does contain that value,
> then your replacement is ok.

Yes, bitmap.depth is the total number of tags AFAIK.

> If your replacement is ok, then I guess you may want to also fix the
> comments above the changes you propose.

Oh right, there's one paragraph in the comment that my patch made
redundant. I'll send a cleanup. Thanks for noticing.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR