| Message ID | 20201210094433.25491-1-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bfq: Fix computation of shallow depth | expand |
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 >
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.
> 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 >
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
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++)
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(-)