[PATCH v2 0/4] optimize the bfq queue idle judgment

[PATCH v2 0/4] optimize the bfq queue idle judgment

[Yu Kuai]

Chagnes in V2:
 - as suggested by Paolo, add support to track if root_group have any
 pending requests, and use that to handle the situation when only one
 group is activated while root group doesn't have any pending requests.
 - modify commit message in patch 2

Yu Kuai (4):
  block, bfq: add support to track if root_group have any pending
    requests
  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 | 69 ++++++++++++++++++++++++++++++++++++++-------
 block/bfq-iosched.h | 29 +++++++++++++++++--
 block/bfq-wf2q.c    | 37 +++++++++++++++---------
 3 files changed, 110 insertions(+), 25 deletions(-)

-- 
2.31.1

[PATCH v2 1/4] block, bfq: add support to track if root_group have any pending requests

[Yu Kuai]

Add a new member in bfq_data to track number of queues that are in
root_group with any pending requests. This will be used in next patch
to optmize queue idle judgment when root_group doesn't have any
pending requests.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/bfq-iosched.c |  8 +++++++-
 block/bfq-iosched.h | 13 +++++++++++--
 block/bfq-wf2q.c    | 37 ++++++++++++++++++++++++-------------
 3 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 727955918563..7c6b412f9a9c 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -859,8 +859,14 @@ void __bfq_weights_tree_remove(struct bfq_data *bfqd,
 void bfq_weights_tree_remove(struct bfq_data *bfqd,
 			     struct bfq_queue *bfqq)
 {
-	struct bfq_entity *entity = bfqq->entity.parent;
+	struct bfq_entity *entity = &bfqq->entity;
+
+	if (entity->in_groups_with_pending_reqs) {
+		entity->in_groups_with_pending_reqs = false;
+		bfqd->num_queues_with_pending_reqs_in_root--;
+	}
 
+	entity = entity->parent;
 	for_each_entity(entity) {
 		struct bfq_sched_data *sd = entity->my_sched_data;
 
diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
index 99c2a3cb081e..610769214f72 100644
--- a/block/bfq-iosched.h
+++ b/block/bfq-iosched.h
@@ -195,7 +195,12 @@ struct bfq_entity {
 	/* flag, set to request a weight, ioprio or ioprio_class change  */
 	int prio_changed;
 
-	/* flag, set if the entity is counted in groups_with_pending_reqs */
+	/*
+	 * If entity represents bfq_group, this flag will set if the group is
+	 * not root_group and have any pending requests; If entity represents
+	 * bfq_queue, this flag will set if the queue is in root_group and have
+	 * any pending requests.
+	 */
 	bool in_groups_with_pending_reqs;
 
 	/* last child queue of entity created (for non-leaf entities) */
@@ -539,7 +544,11 @@ struct bfq_data {
 	 * with no request waiting for completion.
 	 */
 	unsigned int num_groups_with_pending_reqs;
-
+	/*
+	 * number of queues that are in root_group with at least one request
+	 * waiting for completion.
+	 */
+	unsigned int num_queues_with_pending_reqs_in_root;
 	/*
 	 * Per-class (RT, BE, IDLE) number of bfq_queues containing
 	 * requests (including the queue in service, even if it is
diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
index 7a462df71f68..188c8f907219 100644
--- a/block/bfq-wf2q.c
+++ b/block/bfq-wf2q.c
@@ -946,6 +946,29 @@ static void bfq_update_fin_time_enqueue(struct bfq_entity *entity,
 	bfq_active_insert(st, entity);
 }
 
+static void bfq_update_groups_with_pending_reqs(struct bfq_entity *entity)
+{
+#ifdef CONFIG_BFQ_GROUP_IOSCHED
+	struct bfq_queue *bfqq = bfq_entity_to_bfqq(entity);
+
+	if (bfqq) {
+		if (!entity->parent && !entity->in_groups_with_pending_reqs) {
+			entity->in_groups_with_pending_reqs = true;
+			bfqq->bfqd->num_queues_with_pending_reqs_in_root++;
+		}
+	} else {
+		if (!entity->in_groups_with_pending_reqs) {
+			struct bfq_group *bfqg =
+				container_of(entity, struct bfq_group, entity);
+			struct bfq_data *bfqd = bfqg->bfqd;
+
+			entity->in_groups_with_pending_reqs = true;
+			bfqd->num_groups_with_pending_reqs++;
+		}
+	}
+#endif
+}
+
 /**
  * __bfq_activate_entity - handle activation of entity.
  * @entity: the entity being activated.
@@ -999,19 +1022,7 @@ static void __bfq_activate_entity(struct bfq_entity *entity,
 		entity->on_st_or_in_serv = true;
 	}
 
-#ifdef CONFIG_BFQ_GROUP_IOSCHED
-	if (!bfq_entity_to_bfqq(entity)) { /* bfq_group */
-		struct bfq_group *bfqg =
-			container_of(entity, struct bfq_group, entity);
-		struct bfq_data *bfqd = bfqg->bfqd;
-
-		if (!entity->in_groups_with_pending_reqs) {
-			entity->in_groups_with_pending_reqs = true;
-			bfqd->num_groups_with_pending_reqs++;
-		}
-	}
-#endif
-
+	bfq_update_groups_with_pending_reqs(entity);
 	bfq_update_fin_time_enqueue(entity, st, backshifted);
 }
 
-- 
2.31.1

Re: [PATCH v2 1/4] block, bfq: add support to track if root_group have any pending requests

[Paolo Valente]

> Il giorno 6 ago 2021, alle ore 04:08, Yu Kuai <yukuai3@huawei.com> ha scritto:
> 
> Add a new member in bfq_data to track number of queues that are in
> root_group with any pending requests.

maybe modify the last part of the sentence as: ... and that have some pending request

> This will be used in next patch
> to optmize queue idle judgment when root_group doesn't have any
> pending requests.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
> block/bfq-iosched.c |  8 +++++++-
> block/bfq-iosched.h | 13 +++++++++++--
> block/bfq-wf2q.c    | 37 ++++++++++++++++++++++++-------------
> 3 files changed, 42 insertions(+), 16 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 727955918563..7c6b412f9a9c 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -859,8 +859,14 @@ void __bfq_weights_tree_remove(struct bfq_data *bfqd,
> void bfq_weights_tree_remove(struct bfq_data *bfqd,
> 			     struct bfq_queue *bfqq)
> {
> -	struct bfq_entity *entity = bfqq->entity.parent;
> +	struct bfq_entity *entity = &bfqq->entity;
> +
> +	if (entity->in_groups_with_pending_reqs) {
> +		entity->in_groups_with_pending_reqs = false;
> +		bfqd->num_queues_with_pending_reqs_in_root--;

Here you cross the comment "The decrement of
num_groups_with_pending_reqs is not performed immediately upon ...".

Find a way to
- move that comment up, and to make it correct for this slightly
  different decrement
- leave a correct comment (probably shorter) in the original position

> +	}
> 
> +	entity = entity->parent;
> 	for_each_entity(entity) {
> 		struct bfq_sched_data *sd = entity->my_sched_data;
> 
> diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
> index 99c2a3cb081e..610769214f72 100644
> --- a/block/bfq-iosched.h
> +++ b/block/bfq-iosched.h
> @@ -195,7 +195,12 @@ struct bfq_entity {
> 	/* flag, set to request a weight, ioprio or ioprio_class change  */
> 	int prio_changed;
> 
> -	/* flag, set if the entity is counted in groups_with_pending_reqs */
> +	/*
> +	 * If entity represents bfq_group, this flag will set

will be set?

> if the group is
> +	 * not root_group and have

has

> any pending requests; If entity represents

one more nit: use lowercase after semicolon

> +	 * bfq_queue, this flag will set

will be set

> if the queue is in root_group and have
> +	 * any pending requests.
> +	 */
> 	bool in_groups_with_pending_reqs;
> 

The name of the above field follows from the fact that entity is
counted in groups_with_pending_reqs.  You change this fact, because,
in your patch, a queue is not counted in groups_with_pending_reqs.
But you leave the same name. This creates confusion.


> 	/* last child queue of entity created (for non-leaf entities) */
> @@ -539,7 +544,11 @@ struct bfq_data {
> 	 * with no request waiting for completion.
> 	 */
> 	unsigned int num_groups_with_pending_reqs;
> -
> +	/*
> +	 * number of queues that are in root_group with at least one request
> +	 * waiting for completion.

please link somehow this comment to the long comment that comes before it

> +	 */
> +	unsigned int num_queues_with_pending_reqs_in_root;

Why using two counters?  I mean, couldn't you simply count also the
root group in num_groups_with_pending_reqs?

> 	/*
> 	 * Per-class (RT, BE, IDLE) number of bfq_queues containing
> 	 * requests (including the queue in service, even if it is
> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
> index 7a462df71f68..188c8f907219 100644
> --- a/block/bfq-wf2q.c
> +++ b/block/bfq-wf2q.c
> @@ -946,6 +946,29 @@ static void bfq_update_fin_time_enqueue(struct bfq_entity *entity,
> 	bfq_active_insert(st, entity);
> }
> 
> +static void bfq_update_groups_with_pending_reqs(struct bfq_entity *entity)
> +{
> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
> +	struct bfq_queue *bfqq = bfq_entity_to_bfqq(entity);
> +

why do you introduce an extra variable bfqq, instead of doing as in
the original version of the code?  In addition, you remove the comment
/* bfq_group */

Thanks,
Paolo

> +	if (bfqq) {
> +		if (!entity->parent && !entity->in_groups_with_pending_reqs) {
> +			entity->in_groups_with_pending_reqs = true;
> +			bfqq->bfqd->num_queues_with_pending_reqs_in_root++;
> +		}
> +	} else {
> +		if (!entity->in_groups_with_pending_reqs) {
> +			struct bfq_group *bfqg =
> +				container_of(entity, struct bfq_group, entity);
> +			struct bfq_data *bfqd = bfqg->bfqd;
> +
> +			entity->in_groups_with_pending_reqs = true;
> +			bfqd->num_groups_with_pending_reqs++;
> +		}
> +	}
> +#endif
> +}
> +
> /**
>  * __bfq_activate_entity - handle activation of entity.
>  * @entity: the entity being activated.
> @@ -999,19 +1022,7 @@ static void __bfq_activate_entity(struct bfq_entity *entity,
> 		entity->on_st_or_in_serv = true;
> 	}
> 
> -#ifdef CONFIG_BFQ_GROUP_IOSCHED
> -	if (!bfq_entity_to_bfqq(entity)) { /* bfq_group */
> -		struct bfq_group *bfqg =
> -			container_of(entity, struct bfq_group, entity);
> -		struct bfq_data *bfqd = bfqg->bfqd;
> -
> -		if (!entity->in_groups_with_pending_reqs) {
> -			entity->in_groups_with_pending_reqs = true;
> -			bfqd->num_groups_with_pending_reqs++;
> -		}
> -	}
> -#endif
> -
> +	bfq_update_groups_with_pending_reqs(entity);
> 	bfq_update_fin_time_enqueue(entity, st, backshifted);
> }
> 
> -- 
> 2.31.1
> 

Re: [PATCH v2 1/4] block, bfq: add support to track if root_group have any pending requests

[yukuai (C)]

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:
>>
>> Add a new member in bfq_data to track number of queues that are in
>> root_group with any pending requests.
> 
> maybe modify the last part of the sentence as: ... and that have some pending request
> 
>> This will be used in next patch
>> to optmize queue idle judgment when root_group doesn't have any
>> pending requests.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>> block/bfq-iosched.c |  8 +++++++-
>> block/bfq-iosched.h | 13 +++++++++++--
>> block/bfq-wf2q.c    | 37 ++++++++++++++++++++++++-------------
>> 3 files changed, 42 insertions(+), 16 deletions(-)
>>
>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>> index 727955918563..7c6b412f9a9c 100644
>> --- a/block/bfq-iosched.c
>> +++ b/block/bfq-iosched.c
>> @@ -859,8 +859,14 @@ void __bfq_weights_tree_remove(struct bfq_data *bfqd,
>> void bfq_weights_tree_remove(struct bfq_data *bfqd,
>> 			     struct bfq_queue *bfqq)
>> {
>> -	struct bfq_entity *entity = bfqq->entity.parent;
>> +	struct bfq_entity *entity = &bfqq->entity;
>> +
>> +	if (entity->in_groups_with_pending_reqs) {
>> +		entity->in_groups_with_pending_reqs = false;
>> +		bfqd->num_queues_with_pending_reqs_in_root--;
> 
> Here you cross the comment "The decrement of
> num_groups_with_pending_reqs is not performed immediately upon ...".
> 
> Find a way to
> - move that comment up, and to make it correct for this slightly
>    different decrement
> - leave a correct comment (probably shorter) in the original position
> 
>> +	}
>>
>> +	entity = entity->parent;
>> 	for_each_entity(entity) {
>> 		struct bfq_sched_data *sd = entity->my_sched_data;
>>
>> diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
>> index 99c2a3cb081e..610769214f72 100644
>> --- a/block/bfq-iosched.h
>> +++ b/block/bfq-iosched.h
>> @@ -195,7 +195,12 @@ struct bfq_entity {
>> 	/* flag, set to request a weight, ioprio or ioprio_class change  */
>> 	int prio_changed;
>>
>> -	/* flag, set if the entity is counted in groups_with_pending_reqs */
>> +	/*
>> +	 * If entity represents bfq_group, this flag will set
> 
> will be set?
> 
>> if the group is
>> +	 * not root_group and have
> 
> has
> 
>> any pending requests; If entity represents
> 
> one more nit: use lowercase after semicolon
> 
>> +	 * bfq_queue, this flag will set
> 
> will be set
> 
>> if the queue is in root_group and have
>> +	 * any pending requests.
>> +	 */
>> 	bool in_groups_with_pending_reqs;
>>
> 
> The name of the above field follows from the fact that entity is
> counted in groups_with_pending_reqs.  You change this fact, because,
> in your patch, a queue is not counted in groups_with_pending_reqs.
> But you leave the same name. This creates confusion.
> 
> 
>> 	/* last child queue of entity created (for non-leaf entities) */
>> @@ -539,7 +544,11 @@ struct bfq_data {
>> 	 * with no request waiting for completion.
>> 	 */
>> 	unsigned int num_groups_with_pending_reqs;
>> -
>> +	/*
>> +	 * number of queues that are in root_group with at least one request
>> +	 * waiting for completion.
> 
> please link somehow this comment to the long comment that comes before it
> 
>> +	 */
>> +	unsigned int num_queues_with_pending_reqs_in_root;
> 
> Why using two counters?  I mean, couldn't you simply count also the
> root group in num_groups_with_pending_reqs?

Hi, Paolo

Thanks for taking time reviewing these patches

I was doing this too complicated, while counting root group into
num_groups_with_pending_reqs is much easier. I'll do this in next
iteration.

Thanks
Yu Kuai

> 
>> 	/*
>> 	 * Per-class (RT, BE, IDLE) number of bfq_queues containing
>> 	 * requests (including the queue in service, even if it is
>> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
>> index 7a462df71f68..188c8f907219 100644
>> --- a/block/bfq-wf2q.c
>> +++ b/block/bfq-wf2q.c
>> @@ -946,6 +946,29 @@ static void bfq_update_fin_time_enqueue(struct bfq_entity *entity,
>> 	bfq_active_insert(st, entity);
>> }
>>
>> +static void bfq_update_groups_with_pending_reqs(struct bfq_entity *entity)
>> +{
>> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
>> +	struct bfq_queue *bfqq = bfq_entity_to_bfqq(entity);
>> +
> 
> why do you introduce an extra variable bfqq, instead of doing as in
> the original version of the code?  In addition, you remove the comment
> /* bfq_group */
> 
> Thanks,
> Paolo
> 
>> +	if (bfqq) {
>> +		if (!entity->parent && !entity->in_groups_with_pending_reqs) {
>> +			entity->in_groups_with_pending_reqs = true;
>> +			bfqq->bfqd->num_queues_with_pending_reqs_in_root++;
>> +		}
>> +	} else {
>> +		if (!entity->in_groups_with_pending_reqs) {
>> +			struct bfq_group *bfqg =
>> +				container_of(entity, struct bfq_group, entity);
>> +			struct bfq_data *bfqd = bfqg->bfqd;
>> +
>> +			entity->in_groups_with_pending_reqs = true;
>> +			bfqd->num_groups_with_pending_reqs++;
>> +		}
>> +	}
>> +#endif
>> +}
>> +
>> /**
>>   * __bfq_activate_entity - handle activation of entity.
>>   * @entity: the entity being activated.
>> @@ -999,19 +1022,7 @@ static void __bfq_activate_entity(struct bfq_entity *entity,
>> 		entity->on_st_or_in_serv = true;
>> 	}
>>
>> -#ifdef CONFIG_BFQ_GROUP_IOSCHED
>> -	if (!bfq_entity_to_bfqq(entity)) { /* bfq_group */
>> -		struct bfq_group *bfqg =
>> -			container_of(entity, struct bfq_group, entity);
>> -		struct bfq_data *bfqd = bfqg->bfqd;
>> -
>> -		if (!entity->in_groups_with_pending_reqs) {
>> -			entity->in_groups_with_pending_reqs = true;
>> -			bfqd->num_groups_with_pending_reqs++;
>> -		}
>> -	}
>> -#endif
>> -
>> +	bfq_update_groups_with_pending_reqs(entity);
>> 	bfq_update_fin_time_enqueue(entity, st, backshifted);
>> }
>>
>> -- 
>> 2.31.1
>>
> 
> .
> 

[PATCH v2 2/4] block, bfq: do not idle if only one cgroup is activated

[Yu Kuai]

If only one group is activated, there is no need to guarantee the same
share of the throughput of queues in the same group.

If CONFIG_BFQ_GROUP_IOSCHED is enabled, there is no need to check
'varied_queue_weights' and 'multiple_classes_busy':
1) num_groups_with_pending_reqs = 0, idle is not needed
2) num_groups_with_pending_reqs = 1
   - if root group have any pending requests, idle is needed
   - if root group is idle, idle is not needed
3) num_groups_with_pending_reqs > 1, idle is needed

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 | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 7c6b412f9a9c..a780205a1be4 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 are one active group at most(incluing root_group).
+ * 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,26 @@ 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;
+
+	if (bfqd->num_groups_with_pending_reqs &&
+	    bfqd->num_queues_with_pending_reqs_in_root)
+		return true;
+
+	/*
+	 * Reach here means only one group(incluing root group) has pending
+	 * requests, thus it's safe to return.
+	 */
+	return false;
+#endif
+
+	smallest_weight = bfqq &&
 		bfqq->weight_counter &&
 		bfqq->weight_counter ==
 		container_of(
@@ -729,21 +750,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 v2 2/4] block, bfq: do not idle if only one cgroup is activated

[Paolo Valente]

> Il giorno 6 ago 2021, alle ore 04:08, Yu Kuai <yukuai3@huawei.com> ha scritto:
> 
> If only one group is activated, there is no need to guarantee the same
> share of the throughput of queues in the same group.
> 
> If CONFIG_BFQ_GROUP_IOSCHED is enabled, there is no need to check
> 'varied_queue_weights' and 'multiple_classes_busy':
> 1) num_groups_with_pending_reqs = 0, idle is not needed
> 2) num_groups_with_pending_reqs = 1
>   - if root group have any pending requests, idle is needed
>   - if root group is idle, idle is not needed
> 3) num_groups_with_pending_reqs > 1, idle is needed
> 
> 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 | 35 ++++++++++++++++++++++++++---------
> 1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 7c6b412f9a9c..a780205a1be4 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 are one active group at most(incluing root_group).

there are -> there is
incluing -> including
add a space before left parenthesis

> + * If the last condition is false, there is no need to guarantee the,

remove comma

> + * same share of the throughput of queues in the same group.

Actually, I would not add this extra comment on the last condition at all.

>  * 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,26 @@ 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;
> +
> +	if (bfqd->num_groups_with_pending_reqs &&
> +	    bfqd->num_queues_with_pending_reqs_in_root)
> +		return true;
> +
> +	/*
> +	 * Reach here means only one group(incluing root group) has pending
> +	 * requests, thus it's safe to return.
> +	 */
> +	return false;
> +#endif
> +
> +	smallest_weight = bfqq &&
> 		bfqq->weight_counter &&
> 		bfqq->weight_counter ==
> 		container_of(
> @@ -729,21 +750,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

Why do you make these extensive changes, while you can leave all the
function unchanged and just modify the above condition to something
like

|| bfqd->num_groups_with_pending_reqs > 1
|| (bfqd->num_groups_with_pending_reqs && bfqd->num_queues_with_pending_reqs_in_root)

In addition, I still wonder whether you can simply add also the root
group to bfqd->num_groups_with_pending_reqs (when the root group is
active).  This would make the design much cleaner.

Thanks,
Paolo

> -#endif
> -		;
> +	return varied_queue_weights || multiple_classes_busy;
> }
> 
> /*
> -- 
> 2.31.1
> 

Re: [PATCH v2 2/4] block, bfq: do not idle if only one cgroup is activated

[yukuai (C)]

On 2021/08/27 1:00, Paolo Valente wrote:
> 
> Why do you make these extensive changes, while you can leave all the
> function unchanged and just modify the above condition to something
> like
> 
> || bfqd->num_groups_with_pending_reqs > 1
> || (bfqd->num_groups_with_pending_reqs && bfqd->num_queues_with_pending_reqs_in_root)

Hi, Paolo

I was thinking that if CONFIG_BFQ_GROUP_IOSCHED is enabled, there is no
need to caculate smallest_weight, varied_queue_weights, and
multiple_classes_busy:

If we count root group into num_groups_with_pending_reqs
  - If num_groups_with_pending_reqs <= 1, idle is not needed
  - If num_groups_with_pending_reqs  > 1, idle is needed

Thus such changes can save some additional overhead.

Thanks
Yu Kuai

> 
> In addition, I still wonder whether you can simply add also the root
> group to bfqd->num_groups_with_pending_reqs (when the root group is
> active).  This would make the design much cleaner.
> 
> Thanks,
> Paolo
> 
>> -#endif
>> -		;
>> +	return varied_queue_weights || multiple_classes_busy;
>> }
>>
>> /*
>> -- 
>> 2.31.1
>>
> 
> .
> 

Re: [PATCH v2 2/4] block, bfq: do not idle if only one cgroup is activated

[Paolo Valente]

> Il giorno 2 set 2021, alle ore 15:31, yukuai (C) <yukuai3@huawei.com> ha scritto:
> 
> On 2021/08/27 1:00, Paolo Valente wrote:
>> Why do you make these extensive changes, while you can leave all the
>> function unchanged and just modify the above condition to something
>> like
>> || bfqd->num_groups_with_pending_reqs > 1
>> || (bfqd->num_groups_with_pending_reqs && bfqd->num_queues_with_pending_reqs_in_root)
> 
> Hi, Paolo
> 
> I was thinking that if CONFIG_BFQ_GROUP_IOSCHED is enabled, there is no
> need to caculate smallest_weight, varied_queue_weights, and
> multiple_classes_busy:
> 
> If we count root group into num_groups_with_pending_reqs
> - If num_groups_with_pending_reqs <= 1, idle is not needed

Unfortunately, if active queues have different weights or belong to
different classes, then idling is needed to preserve per-queue
bandwidths.

Thanks,
Paolo

> - If num_groups_with_pending_reqs  > 1, idle is needed
> 
> Thus such changes can save some additional overhead.
> 
> Thanks
> Yu Kuai
> 
>> In addition, I still wonder whether you can simply add also the root
>> group to bfqd->num_groups_with_pending_reqs (when the root group is
>> active).  This would make the design much cleaner.
>> Thanks,
>> Paolo
>>> -#endif
>>> -		;
>>> +	return varied_queue_weights || multiple_classes_busy;
>>> }
>>> 
>>> /*
>>> -- 
>>> 2.31.1
>>> 
>> .

Re: [PATCH v2 2/4] block, bfq: do not idle if only one cgroup is activated

[yukuai (C)]

On 2021/09/07 17:10, Paolo Valente wrote:
> 
> 
>> Il giorno 2 set 2021, alle ore 15:31, yukuai (C) <yukuai3@huawei.com> ha scritto:
>>
>> On 2021/08/27 1:00, Paolo Valente wrote:
>>> Why do you make these extensive changes, while you can leave all the
>>> function unchanged and just modify the above condition to something
>>> like
>>> || bfqd->num_groups_with_pending_reqs > 1
>>> || (bfqd->num_groups_with_pending_reqs && bfqd->num_queues_with_pending_reqs_in_root)
>>
>> Hi, Paolo
>>
>> I was thinking that if CONFIG_BFQ_GROUP_IOSCHED is enabled, there is no
>> need to caculate smallest_weight, varied_queue_weights, and
>> multiple_classes_busy:
>>
>> If we count root group into num_groups_with_pending_reqs
>> - If num_groups_with_pending_reqs <= 1, idle is not needed
> 
> Unfortunately, if active queues have different weights or belong to
> different classes, then idling is needed to preserve per-queue
> bandwidths.
> 
> Thanks,
> Paolo

Hi, Paolo

It's right, if num_groups_with_pending_reqs == 1, multiple_classes_busy
should be checked, while smallest_weight and varied_queue_weights can
be skipped.

Thanks
Kuai

> .
> 

[PATCH v2 3/4] 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 a780205a1be4..7df3fc0ef4ef 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -4936,6 +4936,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;
@@ -5019,6 +5033,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 610769214f72..9ab498839e7c 100644
--- a/block/bfq-iosched.h
+++ b/block/bfq-iosched.h
@@ -777,6 +777,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

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

[Paolo Valente]

> Il giorno 6 ago 2021, alle ore 04:08, Yu Kuai <yukuai3@huawei.com> ha scritto:
> 
> If bfq keep

keeps

> dispatching requests with same size, the following
> information are

is stored

> 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 a780205a1be4..7df3fc0ef4ef 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -4936,6 +4936,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;
> @@ -5019,6 +5033,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 610769214f72..9ab498839e7c 100644
> --- a/block/bfq-iosched.h
> +++ b/block/bfq-iosched.h
> @@ -777,6 +777,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

keeps ... stores

> +	 * 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

keeps ... stores

Thanks,
Paolo

> +	 * time when the first request was dispatched.
> +	 */
> +	unsigned long dispatch_time;
> +#endif
> };
> 
> enum bfqq_state_flags {
> -- 
> 2.31.1
> 

[PATCH v2 4/4] 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 | 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

Re: [PATCH v2 4/4] block, bfq: consider request size in bfq_asymmetric_scenario()

[Paolo Valente]

> 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
> 

Re: [PATCH v2 4/4] block, bfq: consider request size in bfq_asymmetric_scenario()

[yukuai (C)]

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.

Re: [PATCH v2 4/4] block, bfq: consider request size in bfq_asymmetric_scenario()

[Paolo Valente]

> 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.

Re: [PATCH v2 4/4] block, bfq: consider request size in bfq_asymmetric_scenario()

[yukuai (C)]

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

Re: [PATCH v2 0/4] optimize the bfq queue idle judgment

[yukuai (C)]

On 2021/08/06 10:08, Yu Kuai wrote:
> Chagnes in V2:
>   - as suggested by Paolo, add support to track if root_group have any
>   pending requests, and use that to handle the situation when only one
>   group is activated while root group doesn't have any pending requests.
>   - modify commit message in patch 2
> 
> Yu Kuai (4):
>    block, bfq: add support to track if root_group have any pending
>      requests
>    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 | 69 ++++++++++++++++++++++++++++++++++++++-------
>   block/bfq-iosched.h | 29 +++++++++++++++++--
>   block/bfq-wf2q.c    | 37 +++++++++++++++---------
>   3 files changed, 110 insertions(+), 25 deletions(-)
> 


ping ...

Re: [PATCH v2 0/4] optimize the bfq queue idle judgment

[yukuai (C)]

On 2021/08/14 10:34, yukuai (C) wrote:
> On 2021/08/06 10:08, Yu Kuai wrote:
>> Chagnes in V2:
>>   - as suggested by Paolo, add support to track if root_group have any
>>   pending requests, and use that to handle the situation when only one
>>   group is activated while root group doesn't have any pending requests.
>>   - modify commit message in patch 2
>>
>> Yu Kuai (4):
>>    block, bfq: add support to track if root_group have any pending
>>      requests
>>    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 | 69 ++++++++++++++++++++++++++++++++++++++-------
>>   block/bfq-iosched.h | 29 +++++++++++++++++--
>>   block/bfq-wf2q.c    | 37 +++++++++++++++---------
>>   3 files changed, 110 insertions(+), 25 deletions(-)
>>
> 
> 
> ping ...
> .
> 

friendly ping ...

Re: [PATCH v2 0/4] optimize the bfq queue idle judgment

[Paolo Valente]

> Il giorno 6 ago 2021, alle ore 04:08, Yu Kuai <yukuai3@huawei.com> ha scritto:
> 
> Chagnes in V2:
> - as suggested by Paolo, add support to track if root_group have any
> pending requests, and use that to handle the situation when only one
> group is activated while root group doesn't have any pending requests.
> - modify commit message in patch 2
> 

Hi,
sorry for my delay.  I've just finished reviewing your new patches.
As I already said, I like the improvement you are making.  Now the way
you make it also seems effective to me.  I still have a design
concern.  You will find it in my detailed comments on your patches.

Thanks
Paolo

> Yu Kuai (4):
>  block, bfq: add support to track if root_group have any pending
>    requests
>  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 | 69 ++++++++++++++++++++++++++++++++++++++-------
> block/bfq-iosched.h | 29 +++++++++++++++++--
> block/bfq-wf2q.c    | 37 +++++++++++++++---------
> 3 files changed, 110 insertions(+), 25 deletions(-)
> 
> -- 
> 2.31.1
>