| Message ID | 20210806020826.1407257-5-yukuai3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | optimize the bfq queue idle judgment | expand |
> Il giorno 6 ago 2021, alle ore 04:08, Yu Kuai <yukuai3@huawei.com> ha scritto: > > There is a special case when bfq do not need to idle when more than > one groups is active: > Unfortunately, there is a misunderstanding here. If more than one group is active, then idling is not needed only if a lot of symmetry conditions also hold: - all active groups have the same weight - all active groups contain the same number of active queues - all active queues have the same weight - all active queues belong to the same I/O-priority class - all dispatched requests have the same size Similarly, if only one group is active, then idling is not needed only if the above last three conditions hold. The current logic, including your changes up to your previous patch, is simply ignoring the last condition above. So, unfortunately, your extra information about varied request size should be used in the opposite way than how you propose to use it. Thanks, Paolo > 1) all active queues have the same weight, > 2) all active queues have the same request size. > 3) all active queues belong to the same I/O-priority class, > > Each time a request is dispatched, bfq can switch in service queue > safely, since the throughput of each active queue is guaranteed to > be equivalent. > > Test procedure: > run "fio -numjobs=1 -ioengine=psync -bs=4k -direct=1 -rw=randread..." in > different cgroup(not root). > > Test result: total bandwidth(Mib/s) > | total jobs | before this patch | after this patch | > | ---------- | ----------------- | --------------------- | > | 1 | 33.8 | 33.8 | > | 2 | 33.8 | 65.4 (32.7 each job) | > | 4 | 33.8 | 106.8 (26.7 each job) | > | 8 | 33.8 | 126.4 (15.8 each job) | > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > block/bfq-iosched.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 7df3fc0ef4ef..e5a07bd1fd84 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -268,6 +268,16 @@ static struct kmem_cache *bfq_pool; > */ > #define BFQ_RATE_SHIFT 16 > > +/* > + * 1) bfq keep dispatching requests with same size for at least one second. > + * 2) bfq dispatch at lease 1024 requests > + * > + * We think bfq are dispatching request with same size if the above two > + * conditions hold true. > + */ > +#define VARIED_REQUEST_SIZE(bfqd) ((bfqd)->dispatch_count < 1024 ||\ > + time_before(jiffies, (bfqd)->dispatch_time + HZ)) > + > /* > * When configured for computing the duration of the weight-raising > * for interactive queues automatically (see the comments at the > @@ -724,7 +734,8 @@ static bool bfq_asymmetric_scenario(struct bfq_data *bfqd, > bool multiple_classes_busy; > > #ifdef CONFIG_BFQ_GROUP_IOSCHED > - if (bfqd->num_groups_with_pending_reqs > 1) > + if (bfqd->num_groups_with_pending_reqs > 1 && > + VARIED_REQUEST_SIZE(bfqd)) > return true; > > if (bfqd->num_groups_with_pending_reqs && > -- > 2.31.1 >
On 2021/08/27 1:00, Paolo Valente wrote: > > >> Il giorno 6 ago 2021, alle ore 04:08, Yu Kuai <yukuai3@huawei.com> ha scritto: >> >> There is a special case when bfq do not need to idle when more than >> one groups is active: >> > > Unfortunately, there is a misunderstanding here. If more than one > group is active, then idling is not needed only if a lot of symmetry > conditions also hold: > - all active groups have the same weight > - all active groups contain the same number of active queues Hi, Paolo I didn't think of this contition. It's seems that if we want to idle when more than one group is active, there are two additional conditions: - all dispatched requests have the same size - all active groups contain the same number of active queues Thus we still need to track how many queues are active in each group. The conditions seems to be too much, do you think is it worth it to add support to idle when more than one group is active? Thanks Kuai > - all active queues have the same weight > - all active queues belong to the same I/O-priority class > - all dispatched requests have the same size > > Similarly, if only one group is active, then idling is not needed only > if the above last three conditions hold. > > The current logic, including your changes up to your previous patch, > is simply ignoring the last condition above. > > So, unfortunately, your extra information about varied request size > should be used in the opposite way than how you propose to use it.
> Il giorno 7 set 2021, alle ore 13:29, yukuai (C) <yukuai3@huawei.com> ha scritto: > > On 2021/08/27 1:00, Paolo Valente wrote: >>> Il giorno 6 ago 2021, alle ore 04:08, Yu Kuai <yukuai3@huawei.com> ha scritto: >>> >>> There is a special case when bfq do not need to idle when more than >>> one groups is active: >>> >> Unfortunately, there is a misunderstanding here. If more than one >> group is active, then idling is not needed only if a lot of symmetry >> conditions also hold: >> - all active groups have the same weight >> - all active groups contain the same number of active queues > > Hi, Paolo > > I didn't think of this contition. > > It's seems that if we want to idle when more than one group is active, > there are two additional conditions: > > - all dispatched requests have the same size > - all active groups contain the same number of active queues > Also the weights and the I/O priorities of the queues inside the groups needs to be controlled, unfortunately. > Thus we still need to track how many queues are active in each group. > The conditions seems to be too much, do you think is it worth it to > add support to idle when more than one group is active? > I think I see your point. The problem is that these states are dynamic. So, if we suspend tracking all the above information while more than one group is active, then we are with no state in case only one group remains active. Thanks, Paolo > Thanks > Kuai > >> - all active queues have the same weight >> - all active queues belong to the same I/O-priority class >> - all dispatched requests have the same size >> Similarly, if only one group is active, then idling is not needed only >> if the above last three conditions hold. >> The current logic, including your changes up to your previous patch, >> is simply ignoring the last condition above. >> So, unfortunately, your extra information about varied request size >> should be used in the opposite way than how you propose to use it.
On 2021/09/15 15:36, Paolo Valente wrote: > > >> Il giorno 7 set 2021, alle ore 13:29, yukuai (C) <yukuai3@huawei.com> ha scritto: >> >> On 2021/08/27 1:00, Paolo Valente wrote: >>>> Il giorno 6 ago 2021, alle ore 04:08, Yu Kuai <yukuai3@huawei.com> ha scritto: >>>> >>>> There is a special case when bfq do not need to idle when more than >>>> one groups is active: >>>> >>> Unfortunately, there is a misunderstanding here. If more than one >>> group is active, then idling is not needed only if a lot of symmetry >>> conditions also hold: >>> - all active groups have the same weight >>> - all active groups contain the same number of active queues >> >> Hi, Paolo >> >> I didn't think of this contition. >> >> It's seems that if we want to idle when more than one group is active, >> there are two additional conditions: >> >> - all dispatched requests have the same size >> - all active groups contain the same number of active queues >> > > Also the weights and the I/O priorities of the queues inside the > groups needs to be controlled, unfortunately. > >> Thus we still need to track how many queues are active in each group. >> The conditions seems to be too much, do you think is it worth it to >> add support to idle when more than one group is active? >> > > I think I see your point. The problem is that these states are > dynamic. So, if we suspend tracking all the above information while > more than one group is active, then we are with no state in case only > one group remains active. Hi, Paolo In this case, I'll drop the last two patches in the next iteration. Thanks, Kuai
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 7df3fc0ef4ef..e5a07bd1fd84 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -268,6 +268,16 @@ static struct kmem_cache *bfq_pool; */ #define BFQ_RATE_SHIFT 16 +/* + * 1) bfq keep dispatching requests with same size for at least one second. + * 2) bfq dispatch at lease 1024 requests + * + * We think bfq are dispatching request with same size if the above two + * conditions hold true. + */ +#define VARIED_REQUEST_SIZE(bfqd) ((bfqd)->dispatch_count < 1024 ||\ + time_before(jiffies, (bfqd)->dispatch_time + HZ)) + /* * When configured for computing the duration of the weight-raising * for interactive queues automatically (see the comments at the @@ -724,7 +734,8 @@ static bool bfq_asymmetric_scenario(struct bfq_data *bfqd, bool multiple_classes_busy; #ifdef CONFIG_BFQ_GROUP_IOSCHED - if (bfqd->num_groups_with_pending_reqs > 1) + if (bfqd->num_groups_with_pending_reqs > 1 && + VARIED_REQUEST_SIZE(bfqd)) return true; if (bfqd->num_groups_with_pending_reqs &&
There is a special case when bfq do not need to idle when more than one groups is active: 1) all active queues have the same weight, 2) all active queues have the same request size. 3) all active queues belong to the same I/O-priority class, Each time a request is dispatched, bfq can switch in service queue safely, since the throughput of each active queue is guaranteed to be equivalent. Test procedure: run "fio -numjobs=1 -ioengine=psync -bs=4k -direct=1 -rw=randread..." in different cgroup(not root). Test result: total bandwidth(Mib/s) | total jobs | before this patch | after this patch | | ---------- | ----------------- | --------------------- | | 1 | 33.8 | 33.8 | | 2 | 33.8 | 65.4 (32.7 each job) | | 4 | 33.8 | 106.8 (26.7 each job) | | 8 | 33.8 | 126.4 (15.8 each job) | Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- block/bfq-iosched.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)