Revert "bfq: Fix computation of shallow depth"

This reverts commit 6d4d273588378c65915acaf7b2ee74e9dd9c130a.

bfq.limit_depth passes word_depths[] as shallow_depth down to sbitmap core sbitmap_get_shallow, which uses just the number to limit the scan depth of each bitmap word, formula:
scan_percentage_for_each_word = shallow_depth / (1 << sbimap->shift) * 100%

That means the comments's percentiles 50%, 75%, 18%, 37% of bfq are correct. But after commit patch 'bfq: Fix computation of shallow depth', we use sbitmap.depth instead, as a example in following case:

sbitmap.depth = 256, map_nr = 4, shift = 6; sbitmap_word.depth = 64. The resulsts of computed bfqd->word_depths[] are {128, 192, 48, 96}, and three of the numbers exceed core dirver's 'sbitmap_word.depth=64' limit nothing. Do we really don't want limit depth for such workloads, or we just want to bump up the percentiles to 100%?

Please correct me if I miss something, thanks.

Signed-off-by Lin Feng <[email protected]>

---
  block/bfq-iosched.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

block/bfq-iosched.c +4 -4

index 9e4eb0fc1c16..9e81d1052091 100644
@@ -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(bt->sb.depth >> 1, 1U);
+	bfqd->word_depths[0][0] = max((1U << bt->sb.shift) >> 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((bt->sb.depth * 3) >> 2, 1U);
+	bfqd->word_depths[0][1] = max(((1U << bt->sb.shift) * 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((bt->sb.depth * 3) >> 4, 1U);
+	bfqd->word_depths[1][0] = max(((1U << bt->sb.shift) * 3) >> 4, 1U);
  	/* no more than ~37% of tags for sync writes (~20% extra tags) */
-	bfqd->word_depths[1][1] = max((bt->sb.depth * 6) >> 4, 1U);
+	bfqd->word_depths[1][1] = max(((1U << bt->sb.shift) * 6) >> 4, 1U);
 
  	for (i = 0; i < 2; i++)
  		for (j = 0; j < 2; j++)

Il giorno 1 feb 2021, alle ore 08:32, Lin Feng <[email protected]> ha scritto:

Hi, it seems that this patch was blocked by linux mailist servers, so ping again.

Based on https://patchwork.kernel.org/project/linux-block/patch/[email protected]/,
it looks like we have made a consensus about bfqd->word_depths[2][2]'s changing, so now the computation codes for bfq's word_depths array are not necessary and one variable is enough.

But IMHO async depth limitation for slow drivers is essential, which is what we always did in cfq age.

Quoted 48 lines

This reverts commit 6d4d273588378c65915acaf7b2ee74e9dd9c130a. bfq.limit_depth passes word_depths[] as shallow_depth down to sbitmap core sbitmap_get_shallow, which uses just the number to limit the scan depth of each bitmap word, formula:
scan_percentage_for_each_word = shallow_depth / (1 << sbimap->shift) * 100% That means the comments's percentiles 50%, 75%, 18%, 37% of bfq are correct. But after commit patch 'bfq: Fix computation of shallow depth', we use sbitmap.depth instead, as a example in following case:
sbitmap.depth = 256, map_nr = 4, shift = 6; sbitmap_word.depth = 64. The resulsts of computed bfqd->word_depths[] are {128, 192, 48, 96}, and three of the numbers exceed core dirver's 'sbitmap_word.depth=64' limit nothing. Do we really don't want limit depth for such workloads, or we just want to bump up the percentiles to 100%?
Please correct me if I miss something, thanks.

Signed-off-by Lin Feng <[email protected]>

---
 block/bfq-iosched.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

block/bfq-iosched.c +4 -4

index 9e4eb0fc1c16..9e81d1052091 100644
@@ -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(bt->sb.depth >> 1, 1U);
+	bfqd->word_depths[0][0] = max((1U << bt->sb.shift) >> 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((bt->sb.depth * 3) >> 2, 1U);
+	bfqd->word_depths[0][1] = max(((1U << bt->sb.shift) * 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((bt->sb.depth * 3) >> 4, 1U);
+	bfqd->word_depths[1][0] = max(((1U << bt->sb.shift) * 3) >> 4, 1U);
 	/* no more than ~37% of tags for sync writes (~20% extra tags) */
-	bfqd->word_depths[1][1] = max((bt->sb.depth * 6) >> 4, 1U);
+	bfqd->word_depths[1][1] = max(((1U << bt->sb.shift) * 6) >> 4, 1U);
   	for (i = 0; i < 2; i++)
 		for (j = 0; j < 2; j++)

Il giorno 29 gen 2021, alle ore 12:18, Lin Feng <[email protected]> ha scritto:

This reverts commit 6d4d273588378c65915acaf7b2ee74e9dd9c130a.

bfq.limit_depth passes word_depths[] as shallow_depth down to sbitmap core sbitmap_get_shallow, which uses just the number to limit the scan depth of each bitmap word, formula:
scan_percentage_for_each_word = shallow_depth / (1 << sbimap->shift) * 100%

That means the comments's percentiles 50%, 75%, 18%, 37% of bfq are correct. But after commit patch 'bfq: Fix computation of shallow depth', we use sbitmap.depth instead, as a example in following case:

sbitmap.depth = 256, map_nr = 4, shift = 6; sbitmap_word.depth = 64. The resulsts of computed bfqd->word_depths[] are {128, 192, 48, 96}, and three of the numbers exceed core dirver's 'sbitmap_word.depth=64' limit nothing. Do we really don't want limit depth for such workloads, or we just want to bump up the percentiles to 100%?

Please correct me if I miss something, thanks.

Signed-off-by Lin Feng <[email protected]>

---
block/bfq-iosched.c | 8 ++++----

1 file changed, 4 insertions(+), 4 deletions(-)

block/bfq-iosched.c +4 -4

index 9e4eb0fc1c16..9e81d1052091 100644
@@ -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(bt->sb.depth >> 1, 1U);
+	bfqd->word_depths[0][0] = max((1U << bt->sb.shift) >> 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((bt->sb.depth * 3) >> 2, 1U);
+	bfqd->word_depths[0][1] = max(((1U << bt->sb.shift) * 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((bt->sb.depth * 3) >> 4, 1U);
+	bfqd->word_depths[1][0] = max(((1U << bt->sb.shift) * 3) >> 4, 1U);
 	/* no more than ~37% of tags for sync writes (~20% extra tags) */
-	bfqd->word_depths[1][1] = max((bt->sb.depth * 6) >> 4, 1U);
+	bfqd->word_depths[1][1] = max(((1U << bt->sb.shift) * 6) >> 4, 1U);
 
 	for (i = 0; i < 2; i++)
 		for (j = 0; j < 2; j++)

Signature · 1 line

2.25.4

This reverts commit 6d4d273588378c65915acaf7b2ee74e9dd9c130a.

bfq.limit_depth passes word_depths[] as shallow_depth down to sbitmap core sbitmap_get_shallow, which uses just the number to limit the scan depth of each bitmap word, formula:
scan_percentage_for_each_word = shallow_depth / (1 << sbimap->shift) * 100%

That means the comments's percentiles 50%, 75%, 18%, 37% of bfq are correct. But after commit patch 'bfq: Fix computation of shallow depth', we use sbitmap.depth instead, as a example in following case:

sbitmap.depth = 256, map_nr = 4, shift = 6; sbitmap_word.depth = 64. The resulsts of computed bfqd->word_depths[] are {128, 192, 48, 96}, and three of the numbers exceed core dirver's 'sbitmap_word.depth=64' limit nothing. Do we really don't want limit depth for such workloads, or we just want to bump up the percentiles to 100%?

Please correct me if I miss something, thanks.

Signed-off-by Lin Feng <[email protected]>

---
 block/bfq-iosched.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

block/bfq-iosched.c +4 -4

index 9e4eb0fc1c16..9e81d1052091 100644
@@ -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(bt->sb.depth >> 1, 1U);
+	bfqd->word_depths[0][0] = max((1U << bt->sb.shift) >> 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((bt->sb.depth * 3) >> 2, 1U);
+	bfqd->word_depths[0][1] = max(((1U << bt->sb.shift) * 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((bt->sb.depth * 3) >> 4, 1U);
+	bfqd->word_depths[1][0] = max(((1U << bt->sb.shift) * 3) >> 4, 1U);
 	/* no more than ~37% of tags for sync writes (~20% extra tags) */
-	bfqd->word_depths[1][1] = max((bt->sb.depth * 6) >> 4, 1U);
+	bfqd->word_depths[1][1] = max(((1U << bt->sb.shift) * 6) >> 4, 1U);
 
 	for (i = 0; i < 2; i++)
 		for (j = 0; j < 2; j++)

Signature · 1 line

2.25.4

Hello!

Quoted 6 lines · Lin Feng

This reverts commit 6d4d273588378c65915acaf7b2ee74e9dd9c130a.

bfq.limit_depth passes word_depths[] as shallow_depth down to sbitmap core sbitmap_get_shallow, which uses just the number to limit the scan depth of each bitmap word, formula:
scan_percentage_for_each_word = shallow_depth / (1 << sbimap->shift) * 100%

Looking at sbitmap_get_shallow() again more carefully, I agree that I misunderstood how shallow_depth argument gets used and the original code was correct and I broke it. Thanks for spotting this!

What I didn't notice is that shallow_depth indeed gets used for each bitmap word separately and not for bitmap as a whole. I'd say this could use some more documentation but that's unrelated to your revert. So feel free to add:

Reviewed-by Jan Kara <[email protected]>

Quoted 14 lines · Jan Kara

Hello!

Quoted 6 lines · Lin Feng

This reverts commit 6d4d273588378c65915acaf7b2ee74e9dd9c130a.

bfq.limit_depth passes word_depths[] as shallow_depth down to sbitmap core sbitmap_get_shallow, which uses just the number to limit the scan depth of each bitmap word, formula:
scan_percentage_for_each_word = shallow_depth / (1 << sbimap->shift) * 100%

Looking at sbitmap_get_shallow() again more carefully, I agree that I misunderstood how shallow_depth argument gets used and the original code was correct and I broke it. Thanks for spotting this!

Quoted 5 lines

What I didn't notice is that shallow_depth indeed gets used for each bitmap word separately and not for bitmap as a whole. I'd say this could use some more documentation but that's unrelated to your revert. So feel free to add:

Reviewed-by Jan Kara <[email protected]>

I don't have the original patch (neither directly nor in the archive), so I had to hand-apply it. In any case, applied for 5.11, thanks.

Hi all,

Quoted 15 lines · Jens Axboe

Quoted 14 lines · Jan Kara

Hello!

Quoted 6 lines · Lin Feng

This reverts commit 6d4d273588378c65915acaf7b2ee74e9dd9c130a.

bfq.limit_depth passes word_depths[] as shallow_depth down to sbitmap core sbitmap_get_shallow, which uses just the number to limit the scan depth of each bitmap word, formula:
scan_percentage_for_each_word = shallow_depth / (1 << sbimap->shift) * 100%

Looking at sbitmap_get_shallow() again more carefully, I agree that I misunderstood how shallow_depth argument gets used and the original code was correct and I broke it. Thanks for spotting this!

Quoted 9 lines

Quoted 5 lines

What I didn't notice is that shallow_depth indeed gets used for each bitmap word separately and not for bitmap as a whole. I'd say this could use some more documentation but that's unrelated to your revert. So feel free to add:

Reviewed-by Jan Kara <[email protected]>

I don't have the original patch (neither directly nor in the archive), so I had to hand-apply it. In any case, applied for 5.11, thanks.

Take a look at linux-block.git tree, the hand-applied commit for this patch is broken, the following changing line is left out:

(quoted diff) +1 -1

-	bfqd->word_depths[0][1] = max((bt->sb.depth * 3) >> 2, 1U);
+	bfqd->word_depths[0][1] = max(((1U << bt->sb.shift) * 3) >> 2, 1U);

Sorry for making troubles to you, I will resend this patch with tiny commit log typo fix(sbimap -> sbitmap) and attaching Jan's Reviewed-by, also thanks his time for reviewing.

Hope this time lkml server will not block my patch.