[PATCH 0/3] optimize the queue idle judgment

[PATCH 0/3] optimize the queue idle judgment

[Yu Kuai]

bfqq might plug I/O dispatch when it remains temporarily empty while
being served, this will benefit for both sequence io bandwidth and relative
sync io control.

This path set tries to extend the two special cases that idle is not
needed, and can get better bandwidth.

1) only one group is activated.
2) when more than one groups are activated, all queues are issuring
requests with same size.

Yu Kuai (3):
  block, bfq: do not idle if only one cgroup is activated
  block, bfq: add support to record request size information
  block, bfq: consider request size in bfq_asymmetric_scenario()

 block/bfq-iosched.c | 50 +++++++++++++++++++++++++++++++++++++--------
 block/bfq-iosched.h | 16 +++++++++++++++
 2 files changed, 57 insertions(+), 9 deletions(-)

-- 
2.31.1

[PATCH 1/3] block, bfq: do not idle if only one cgroup is activated

[Yu Kuai]

If only one group is activated, specifically
'bfqd->num_groups_with_pending_reqs == 1', there is no need to guarantee
the same share of the throughput of queues in the same group.

Thus change the condition from '> 0' to '> 1' in
bfq_asymmetric_scenario(). By the way, if 'num_groups_with_pending_reqs'
is greater than 1, there is no need to check 'varied_queue_weights' and
'multiple_classes_busy', thus move the judgement forward.

Test procedure:
run "fio -numjobs=1 -ioengine=psync -bs=4k -direct=1 -rw=randread..." multiple
times in the same 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) |

By the way, if I test with "fio -numjobs=1/2/4/8 ...", test result is
the same with or without this patch.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/bfq-iosched.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 727955918563..2768a4c1cc45 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -709,7 +709,9 @@ bfq_pos_tree_add_move(struct bfq_data *bfqd, struct bfq_queue *bfqq)
  * much easier to maintain the needed state:
  * 1) all active queues have the same weight,
  * 2) all active queues belong to the same I/O-priority class,
- * 3) there are no active groups.
+ * 3) there is one active group at most.
+ * If the last condition is false, there is no need to guarantee the
+ * same share of the throughput of queues in the same group.
  * In particular, the last condition is always true if hierarchical
  * support or the cgroups interface are not enabled, thus no state
  * needs to be maintained in this case.
@@ -717,7 +719,16 @@ bfq_pos_tree_add_move(struct bfq_data *bfqd, struct bfq_queue *bfqq)
 static bool bfq_asymmetric_scenario(struct bfq_data *bfqd,
 				   struct bfq_queue *bfqq)
 {
-	bool smallest_weight = bfqq &&
+	bool smallest_weight;
+	bool varied_queue_weights;
+	bool multiple_classes_busy;
+
+#ifdef CONFIG_BFQ_GROUP_IOSCHED
+	if (bfqd->num_groups_with_pending_reqs > 1)
+		return true;
+#endif
+
+	smallest_weight = bfqq &&
 		bfqq->weight_counter &&
 		bfqq->weight_counter ==
 		container_of(
@@ -729,21 +740,17 @@ static bool bfq_asymmetric_scenario(struct bfq_data *bfqd,
 	 * For queue weights to differ, queue_weights_tree must contain
 	 * at least two nodes.
 	 */
-	bool varied_queue_weights = !smallest_weight &&
+	varied_queue_weights = !smallest_weight &&
 		!RB_EMPTY_ROOT(&bfqd->queue_weights_tree.rb_root) &&
 		(bfqd->queue_weights_tree.rb_root.rb_node->rb_left ||
 		 bfqd->queue_weights_tree.rb_root.rb_node->rb_right);
 
-	bool multiple_classes_busy =
+	multiple_classes_busy =
 		(bfqd->busy_queues[0] && bfqd->busy_queues[1]) ||
 		(bfqd->busy_queues[0] && bfqd->busy_queues[2]) ||
 		(bfqd->busy_queues[1] && bfqd->busy_queues[2]);
 
-	return varied_queue_weights || multiple_classes_busy
-#ifdef CONFIG_BFQ_GROUP_IOSCHED
-	       || bfqd->num_groups_with_pending_reqs > 0
-#endif
-		;
+	return varied_queue_weights || multiple_classes_busy;
 }
 
 /*
-- 
2.31.1

Re: [PATCH 1/3] block, bfq: do not idle if only one cgroup is activated

[Paolo Valente]

> Il giorno 14 lug 2021, alle ore 11:45, Yu Kuai <yukuai3@huawei.com> ha scritto:
> 
> If only one group is activated, specifically
> 'bfqd->num_groups_with_pending_reqs == 1', there is no need to guarantee
> the same share of the throughput of queues in the same group.
> 
> Thus change the condition from '> 0' to '> 1' in
> bfq_asymmetric_scenario().

I see your point, and I agree with your goal.  Yet, your change seems
not to suffer from the following problem.

In addition to the groups that are created explicitly, there is the
implicit root group.  So, when bfqd->num_groups_with_pending_reqs ==
1, there may be both active processes in the root group and active
processes in the only group created explicitly.  In this case, idling
is needed to preserve service guarantees.

Probably your idea should be improved by making sure that there is
pending I/O only from either the root group or the explicit group.

Thanks,
Paolo

> By the way, if 'num_groups_with_pending_reqs'
> is greater than 1, there is no need to check 'varied_queue_weights' and
> 'multiple_classes_busy', thus move the judgement forward.
> 
> Test procedure:
> run "fio -numjobs=1 -ioengine=psync -bs=4k -direct=1 -rw=randread..." multiple
> times in the same 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) |
> 
> By the way, if I test with "fio -numjobs=1/2/4/8 ...", test result is
> the same with or without this patch.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
> block/bfq-iosched.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 727955918563..2768a4c1cc45 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -709,7 +709,9 @@ bfq_pos_tree_add_move(struct bfq_data *bfqd, struct bfq_queue *bfqq)
>  * much easier to maintain the needed state:
>  * 1) all active queues have the same weight,
>  * 2) all active queues belong to the same I/O-priority class,
> - * 3) there are no active groups.
> + * 3) there is one active group at most.
> + * If the last condition is false, there is no need to guarantee the
> + * same share of the throughput of queues in the same group.
>  * In particular, the last condition is always true if hierarchical
>  * support or the cgroups interface are not enabled, thus no state
>  * needs to be maintained in this case.
> @@ -717,7 +719,16 @@ bfq_pos_tree_add_move(struct bfq_data *bfqd, struct bfq_queue *bfqq)
> static bool bfq_asymmetric_scenario(struct bfq_data *bfqd,
> 				   struct bfq_queue *bfqq)
> {
> -	bool smallest_weight = bfqq &&
> +	bool smallest_weight;
> +	bool varied_queue_weights;
> +	bool multiple_classes_busy;
> +
> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
> +	if (bfqd->num_groups_with_pending_reqs > 1)
> +		return true;
> +#endif
> +
> +	smallest_weight = bfqq &&
> 		bfqq->weight_counter &&
> 		bfqq->weight_counter ==
> 		container_of(
> @@ -729,21 +740,17 @@ static bool bfq_asymmetric_scenario(struct bfq_data *bfqd,
> 	 * For queue weights to differ, queue_weights_tree must contain
> 	 * at least two nodes.
> 	 */
> -	bool varied_queue_weights = !smallest_weight &&
> +	varied_queue_weights = !smallest_weight &&
> 		!RB_EMPTY_ROOT(&bfqd->queue_weights_tree.rb_root) &&
> 		(bfqd->queue_weights_tree.rb_root.rb_node->rb_left ||
> 		 bfqd->queue_weights_tree.rb_root.rb_node->rb_right);
> 
> -	bool multiple_classes_busy =
> +	multiple_classes_busy =
> 		(bfqd->busy_queues[0] && bfqd->busy_queues[1]) ||
> 		(bfqd->busy_queues[0] && bfqd->busy_queues[2]) ||
> 		(bfqd->busy_queues[1] && bfqd->busy_queues[2]);
> 
> -	return varied_queue_weights || multiple_classes_busy
> -#ifdef CONFIG_BFQ_GROUP_IOSCHED
> -	       || bfqd->num_groups_with_pending_reqs > 0
> -#endif
> -		;
> +	return varied_queue_weights || multiple_classes_busy;
> }
> 
> /*
> -- 
> 2.31.1
> 

Re: [PATCH 1/3] block, bfq: do not idle if only one cgroup is activated

[yukuai (C)]

On 2021/07/24 15:12, Paolo Valente wrote:
> 
> 
>> Il giorno 14 lug 2021, alle ore 11:45, Yu Kuai <yukuai3@huawei.com> ha scritto:
>>
>> If only one group is activated, specifically
>> 'bfqd->num_groups_with_pending_reqs == 1', there is no need to guarantee
>> the same share of the throughput of queues in the same group.
>>
>> Thus change the condition from '> 0' to '> 1' in
>> bfq_asymmetric_scenario().
> 
> I see your point, and I agree with your goal.  Yet, your change seems
> not to suffer from the following problem.
> 
> In addition to the groups that are created explicitly, there is the
> implicit root group.  So, when bfqd->num_groups_with_pending_reqs ==
> 1, there may be both active processes in the root group and active
> processes in the only group created explicitly.  In this case, idling
> is needed to preserve service guarantees.
> 
> Probably your idea should be improved by making sure that there is
> pending I/O only from either the root group or the explicit group.
> 
> Thanks,
> Paolo
> 

Hi,

Thanks for you advice, will do this in the next iteration.

Best regards,
Kuai

Re: [PATCH 1/3] block, bfq: do not idle if only one cgroup is activated

[yukuai (C)]

On 2021/07/24 15:12, Paolo Valente wrote:
> 
> 
>> Il giorno 14 lug 2021, alle ore 11:45, Yu Kuai <yukuai3@huawei.com> ha scritto:
>>
>> If only one group is activated, specifically
>> 'bfqd->num_groups_with_pending_reqs == 1', there is no need to guarantee
>> the same share of the throughput of queues in the same group.
>>
>> Thus change the condition from '> 0' to '> 1' in
>> bfq_asymmetric_scenario().
> 
> I see your point, and I agree with your goal.  Yet, your change seems
> not to suffer from the following problem.
> 
> In addition to the groups that are created explicitly, there is the
> implicit root group.  So, when bfqd->num_groups_with_pending_reqs ==
> 1, there may be both active processes in the root group and active
> processes in the only group created explicitly.  In this case, idling
> is needed to preserve service guarantees.
> 
> Probably your idea should be improved by making sure that there is
> pending I/O only from either the root group or the explicit group.
> 
> Thanks,
> Paolo


Hi, Paolo

I'm trying to add support to judge if root group have pending rqs, the
implementation involve setting and clearing the busy state.

I'm thinking about setting busy in __bfq_activate_entity() if
bfq_entity_to_bfqq() return valid bfqq, however I'm not sure where to
clear the busy state.

On the other hand, do you think the way I record rq size info in patch 2
is OK? If so, I can do this the similar way: say that root group doesn't
have any pending requests if bfq haven't dispatch rq from root group for
a period of time.

Thanks
Kuai

Re: [PATCH 1/3] block, bfq: do not idle if only one cgroup is activated

[Paolo Valente]

> Il giorno 31 lug 2021, alle ore 09:10, yukuai (C) <yukuai3@huawei.com> ha scritto:
> 
> On 2021/07/24 15:12, Paolo Valente wrote:
>>> Il giorno 14 lug 2021, alle ore 11:45, Yu Kuai <yukuai3@huawei.com> ha scritto:
>>> 
>>> If only one group is activated, specifically
>>> 'bfqd->num_groups_with_pending_reqs == 1', there is no need to guarantee
>>> the same share of the throughput of queues in the same group.
>>> 
>>> Thus change the condition from '> 0' to '> 1' in
>>> bfq_asymmetric_scenario().
>> I see your point, and I agree with your goal.  Yet, your change seems
>> not to suffer from the following problem.
>> In addition to the groups that are created explicitly, there is the
>> implicit root group.  So, when bfqd->num_groups_with_pending_reqs ==
>> 1, there may be both active processes in the root group and active
>> processes in the only group created explicitly.  In this case, idling
>> is needed to preserve service guarantees.
>> Probably your idea should be improved by making sure that there is
>> pending I/O only from either the root group or the explicit group.
>> Thanks,
>> Paolo
> 
> 
> Hi, Paolo
> 

Hi

> I'm trying to add support to judge if root group have pending rqs, the
> implementation involve setting and clearing the busy state.
> 

I wouldn't use the busy state, as it does not take in-flight requests
into account.  For I/O control, the latter are as important as the
ones still queued in the scheduler.  For this reason, I take in-flight
requests into account when counting
bfqd->num_groups_with_pending_reqs.

See, e.g., this

	if (!bfqq->dispatched && !bfq_bfqq_busy(bfqq)) {
		...
		bfq_weights_tree_remove(bfqd, bfqq);
	}

in bfq_completed_request.

I would replicate the same logic in deciding whether the root group
has pending I/O.


> I'm thinking about setting busy in __bfq_activate_entity() if
> bfq_entity_to_bfqq() return valid bfqq, however I'm not sure where to
> clear the busy state.
> 
> On the other hand, do you think the way I record rq size info in patch 2
> is OK?

First, let's see what you reply to my suggestion above.

Thanks,
Paolo

>  If so, I can do this the similar way: say that root group doesn't
> have any pending requests if bfq haven't dispatch rq from root group for
> a period of time.
> 
> Thanks
> Kuai

Re: [PATCH 1/3] block, bfq: do not idle if only one cgroup is activated

[yukuai (C)]

On 2021/08/03 15:07, Paolo Valente wrote:
> 
> 
>> Il giorno 31 lug 2021, alle ore 09:10, yukuai (C) <yukuai3@huawei.com> ha scritto:
>>
>> On 2021/07/24 15:12, Paolo Valente wrote:
>>>> Il giorno 14 lug 2021, alle ore 11:45, Yu Kuai <yukuai3@huawei.com> ha scritto:
>>>>
>>>> If only one group is activated, specifically
>>>> 'bfqd->num_groups_with_pending_reqs == 1', there is no need to guarantee
>>>> the same share of the throughput of queues in the same group.
>>>>
>>>> Thus change the condition from '> 0' to '> 1' in
>>>> bfq_asymmetric_scenario().
>>> I see your point, and I agree with your goal.  Yet, your change seems
>>> not to suffer from the following problem.
>>> In addition to the groups that are created explicitly, there is the
>>> implicit root group.  So, when bfqd->num_groups_with_pending_reqs ==
>>> 1, there may be both active processes in the root group and active
>>> processes in the only group created explicitly.  In this case, idling
>>> is needed to preserve service guarantees.
>>> Probably your idea should be improved by making sure that there is
>>> pending I/O only from either the root group or the explicit group.
>>> Thanks,
>>> Paolo
>>
>>
>> Hi, Paolo
>>
> 
> Hi
> 
>> I'm trying to add support to judge if root group have pending rqs, the
>> implementation involve setting and clearing the busy state.
>>
> 
> I wouldn't use the busy state, as it does not take in-flight requests
> into account.  For I/O control, the latter are as important as the
> ones still queued in the scheduler.  For this reason, I take in-flight
> requests into account when counting
> bfqd->num_groups_with_pending_reqs.
> 
> See, e.g., this
> 
> 	if (!bfqq->dispatched && !bfq_bfqq_busy(bfqq)) {
> 		...
> 		bfq_weights_tree_remove(bfqd, bfqq);
> 	}
> 
> in bfq_completed_request.
> 
> I would replicate the same logic in deciding whether the root group
> has pending I/O.
> 

Hi, Paolo

Thanks for your advice, I'll send a new patchset soon.

Kuai

[PATCH 2/3] block, bfq: add support to record request size information

[Yu Kuai]

If bfq keep dispatching requests with same size, the following
information are stored if CONFIG_BFQ_GROUP_IOSCHED is enabled:

1) the size
2) the count of requests
3) when the first request was dispatched

These will be used in later patch to support concurrent sync
io in such situation.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/bfq-iosched.c | 15 +++++++++++++++
 block/bfq-iosched.h | 16 ++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 2768a4c1cc45..e5a1093ec30a 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -4920,6 +4920,20 @@ static bool bfq_has_work(struct blk_mq_hw_ctx *hctx)
 		bfq_tot_busy_queues(bfqd) > 0;
 }
 
+static void bfq_update_dispatch_size_info(struct bfq_data *bfqd,
+					  unsigned int size)
+{
+#ifdef CONFIG_BFQ_GROUP_IOSCHED
+	if (bfqd->dispatch_size == size) {
+		bfqd->dispatch_count++;
+	} else {
+		bfqd->dispatch_size = size;
+		bfqd->dispatch_count = 1;
+		bfqd->dispatch_time = jiffies;
+	}
+#endif
+}
+
 static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
 {
 	struct bfq_data *bfqd = hctx->queue->elevator->elevator_data;
@@ -5003,6 +5017,7 @@ static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
 		bfqd->rq_in_driver++;
 start_rq:
 		rq->rq_flags |= RQF_STARTED;
+		bfq_update_dispatch_size_info(bfqd, blk_rq_bytes(rq));
 	}
 exit:
 	return rq;
diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
index 99c2a3cb081e..4b9d95447a50 100644
--- a/block/bfq-iosched.h
+++ b/block/bfq-iosched.h
@@ -768,6 +768,22 @@ struct bfq_data {
 	 * function)
 	 */
 	unsigned int word_depths[2][2];
+
+#ifdef CONFIG_BFQ_GROUP_IOSCHED
+	/* the size of last dispatched request */
+	unsigned int dispatch_size;
+	/*
+	 * If bfq keep dispatching requests with same size, this store the
+	 * count of requests. We use unsigned long here, so we don't care
+	 * about overflow.
+	 */
+	unsigned long dispatch_count;
+	/*
+	 * If bfq keep dispatching requests with same size, this store the
+	 * time when the first request was dispatched.
+	 */
+	unsigned long dispatch_time;
+#endif
 };
 
 enum bfqq_state_flags {
-- 
2.31.1

[PATCH 3/3] block, bfq: consider request size in bfq_asymmetric_scenario()

[Yu Kuai]

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 | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index e5a1093ec30a..b78fe8a1537e 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -268,6 +268,15 @@ 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 +733,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;
 #endif
 
-- 
2.31.1

Re: [PATCH 0/3] optimize the queue idle judgment

[yukuai (C)]

On 2021/07/14 17:45, Yu Kuai wrote:
> bfqq might plug I/O dispatch when it remains temporarily empty while
> being served, this will benefit for both sequence io bandwidth and relative
> sync io control.
> 
> This path set tries to extend the two special cases that idle is not
> needed, and can get better bandwidth.
> 
> 1) only one group is activated.
> 2) when more than one groups are activated, all queues are issuring
> requests with same size.
> 
> Yu Kuai (3):
>    block, bfq: do not idle if only one cgroup is activated
>    block, bfq: add support to record request size information
>    block, bfq: consider request size in bfq_asymmetric_scenario()
> 
>   block/bfq-iosched.c | 50 +++++++++++++++++++++++++++++++++++++--------
>   block/bfq-iosched.h | 16 +++++++++++++++
>   2 files changed, 57 insertions(+), 9 deletions(-)
> 

ping ...