[PATCH v4 0/2] optimize the bfq queue idle judgment

[PATCH v4 0/2] optimize the bfq queue idle judgment

[Yu Kuai]

Changes in V4:
 - fix a compile warning when CONFIG_BLK_CGROUP is not enabled.

Changes in V3:
 - Instead of tracking each queue in root group, tracking root group
 directly just like non-root group does.
 - remove patch 3,4 from these series.

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 (2):
  block, bfq: counted root group into 'num_groups_with_pending_reqs'
  block, bfq: do not idle if only one cgroup is activated

 block/bfq-iosched.c | 40 ++++++++++++++++++++++++++++--------
 block/bfq-wf2q.c    | 50 +++++++++++++++++++++++++++++++++------------
 2 files changed, 69 insertions(+), 21 deletions(-)

-- 
2.31.1

[PATCH v4 1/2] block, bfq: counted root group into 'num_groups_with_pending_reqs'

[Yu Kuai]

'num_groups_with_pending_reqs' represents how many groups that are
not root group and have pending requests. This patch also counted
root group into 'num_groups_with_pending_reqs'.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/bfq-iosched.c | 36 ++++++++++++++++++++++++++------
 block/bfq-wf2q.c    | 50 +++++++++++++++++++++++++++++++++------------
 2 files changed, 67 insertions(+), 19 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index fec18118dc30..d251735383f7 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -852,6 +852,16 @@ void __bfq_weights_tree_remove(struct bfq_data *bfqd,
 	bfq_put_queue(bfqq);
 }
 
+static inline void
+bfq_clear_group_with_pending_reqs(struct bfq_data *bfqd,
+				  struct bfq_entity *entity)
+{
+	if (entity->in_groups_with_pending_reqs) {
+		entity->in_groups_with_pending_reqs = false;
+		bfqd->num_groups_with_pending_reqs--;
+	}
+}
+
 /*
  * Invoke __bfq_weights_tree_remove on bfqq and decrement the number
  * of active groups for each queue's inactive parent entity.
@@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
 			     struct bfq_queue *bfqq)
 {
 	struct bfq_entity *entity = bfqq->entity.parent;
+	struct bfq_sched_data *sd;
+
+	/*
+	 * If the bfq queue is in root group, the decrement of
+	 * num_groups_with_pending_reqs is performed immediately upon the
+	 * deactivation of entity.
+	 */
+	if (!entity) {
+		entity = &bfqd->root_group->entity;
+		sd = entity->my_sched_data;
+
+		if (!sd->in_service_entity)
+			bfq_clear_group_with_pending_reqs(bfqd, entity);
+
+		return;
+	}
 
 	for_each_entity(entity) {
-		struct bfq_sched_data *sd = entity->my_sched_data;
+		sd = entity->my_sched_data;
 
 		if (sd->next_in_service || sd->in_service_entity) {
 			/*
@@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
 		}
 
 		/*
-		 * The decrement of num_groups_with_pending_reqs is
+		 * If the bfq queue is not in root group,
+		 * the decrement of num_groups_with_pending_reqs is
 		 * not performed immediately upon the deactivation of
 		 * entity, but it is delayed to when it also happens
 		 * that the first leaf descendant bfqq of entity gets
@@ -889,10 +916,7 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
 		 * needed. See the comments on
 		 * num_groups_with_pending_reqs for details.
 		 */
-		if (entity->in_groups_with_pending_reqs) {
-			entity->in_groups_with_pending_reqs = false;
-			bfqd->num_groups_with_pending_reqs--;
-		}
+		bfq_clear_group_with_pending_reqs(bfqd, entity);
 	}
 
 	/*
diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
index b74cc0da118e..3e9e672aa302 100644
--- a/block/bfq-wf2q.c
+++ b/block/bfq-wf2q.c
@@ -945,6 +945,42 @@ static void bfq_update_fin_time_enqueue(struct bfq_entity *entity,
 
 	bfq_active_insert(st, entity);
 }
+#ifdef CONFIG_BFQ_GROUP_IOSCHED
+static inline void
+bfq_set_group_with_pending_reqs(struct bfq_data *bfqd,
+				struct bfq_entity *entity)
+{
+	if (!entity->in_groups_with_pending_reqs) {
+		entity->in_groups_with_pending_reqs = true;
+		bfqd->num_groups_with_pending_reqs++;
+	}
+}
+
+static void bfq_update_groups_with_pending_reqs(struct bfq_entity *entity)
+{
+	struct bfq_queue *bfqq = bfq_entity_to_bfqq(entity);
+
+	if (bfqq) {
+		/*
+		 * If the entity represents bfq_queue, and the queue belongs to
+		 * root cgroup.
+		 */
+		if (!entity->parent)
+			bfq_set_group_with_pending_reqs(bfqq->bfqd,
+				&bfqq->bfqd->root_group->entity);
+	} else {
+		/* If the entity represents bfq_group. */
+		struct bfq_group *bfqg =
+			container_of(entity, struct bfq_group, entity);
+		struct bfq_data *bfqd = bfqg->bfqd;
+
+		bfq_set_group_with_pending_reqs(bfqd, entity);
+	}
+}
+#else
+#define bfq_update_groups_with_pending_reqs(entity) \
+	do {} while (0)
+#endif
 
 /**
  * __bfq_activate_entity - handle activation of entity.
@@ -999,19 +1035,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 v4 2/2] 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.

Test procedure:
run "fio -numjobs=1 -ioengine=psync -bs=4k -direct=1 -rw=randread..."
multiple times in the same cgroup.

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. This is because bfq_queue can
be merged in this situation.

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

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index d251735383f7..8d94f511bee8 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -709,7 +709,7 @@ 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.
  * 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.
@@ -741,7 +741,7 @@ static bool bfq_asymmetric_scenario(struct bfq_data *bfqd,
 
 	return varied_queue_weights || multiple_classes_busy
 #ifdef CONFIG_BFQ_GROUP_IOSCHED
-	       || bfqd->num_groups_with_pending_reqs > 0
+	       || bfqd->num_groups_with_pending_reqs > 1
 #endif
 		;
 }
-- 
2.31.1

Re: [PATCH v4 1/2] block, bfq: counted root group into 'num_groups_with_pending_reqs'

[Paolo Valente]

> Il giorno 14 ott 2021, alle ore 03:45, Yu Kuai <yukuai3@huawei.com> ha scritto:
> 
> 'num_groups_with_pending_reqs' represents how many groups that are
> not root group and have pending requests. This patch also counted
> root group into 'num_groups_with_pending_reqs'.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
> block/bfq-iosched.c | 36 ++++++++++++++++++++++++++------
> block/bfq-wf2q.c    | 50 +++++++++++++++++++++++++++++++++------------
> 2 files changed, 67 insertions(+), 19 deletions(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index fec18118dc30..d251735383f7 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -852,6 +852,16 @@ void __bfq_weights_tree_remove(struct bfq_data *bfqd,
> 	bfq_put_queue(bfqq);
> }
> 
> +static inline void
> +bfq_clear_group_with_pending_reqs(struct bfq_data *bfqd,
> +				  struct bfq_entity *entity)
> +{
> +	if (entity->in_groups_with_pending_reqs) {
> +		entity->in_groups_with_pending_reqs = false;
> +		bfqd->num_groups_with_pending_reqs--;
> +	}
> +}
> +
> /*
>  * Invoke __bfq_weights_tree_remove on bfqq and decrement the number
>  * of active groups for each queue's inactive parent entity.
> @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
> 			     struct bfq_queue *bfqq)
> {
> 	struct bfq_entity *entity = bfqq->entity.parent;
> +	struct bfq_sched_data *sd;
> +
> +	/*
> +	 * If the bfq queue is in root group, the decrement of
> +	 * num_groups_with_pending_reqs is performed immediately upon the
> +	 * deactivation of entity.
> +	 */
> +	if (!entity) {
> +		entity = &bfqd->root_group->entity;
> +		sd = entity->my_sched_data;
> +
> +		if (!sd->in_service_entity)
> +			bfq_clear_group_with_pending_reqs(bfqd, entity);
> +
> +		return;
> +	}
> 
> 	for_each_entity(entity) {
> -		struct bfq_sched_data *sd = entity->my_sched_data;
> +		sd = entity->my_sched_data;
> 
> 		if (sd->next_in_service || sd->in_service_entity) {
> 			/*
> @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
> 		}
> 
> 		/*
> -		 * The decrement of num_groups_with_pending_reqs is
> +		 * If the bfq queue is not in root group,
> +		 * the decrement of num_groups_with_pending_reqs is


I'm sorry if I didn't notice this before, but why do you postpone the
decrement only for queues not in root group?  If I'm not missing
anything, the active (i.e., with pending reqs) state of the root group
is to be computed as that of ay other group.

Thanks,
Paolo

> 		 * not performed immediately upon the deactivation of
> 		 * entity, but it is delayed to when it also happens
> 		 * that the first leaf descendant bfqq of entity gets
> @@ -889,10 +916,7 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
> 		 * needed. See the comments on
> 		 * num_groups_with_pending_reqs for details.
> 		 */
> -		if (entity->in_groups_with_pending_reqs) {
> -			entity->in_groups_with_pending_reqs = false;
> -			bfqd->num_groups_with_pending_reqs--;
> -		}
> +		bfq_clear_group_with_pending_reqs(bfqd, entity);
> 	}
> 
> 	/*
> diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
> index b74cc0da118e..3e9e672aa302 100644
> --- a/block/bfq-wf2q.c
> +++ b/block/bfq-wf2q.c
> @@ -945,6 +945,42 @@ static void bfq_update_fin_time_enqueue(struct bfq_entity *entity,
> 
> 	bfq_active_insert(st, entity);
> }
> +#ifdef CONFIG_BFQ_GROUP_IOSCHED
> +static inline void
> +bfq_set_group_with_pending_reqs(struct bfq_data *bfqd,
> +				struct bfq_entity *entity)
> +{
> +	if (!entity->in_groups_with_pending_reqs) {
> +		entity->in_groups_with_pending_reqs = true;
> +		bfqd->num_groups_with_pending_reqs++;
> +	}
> +}
> +
> +static void bfq_update_groups_with_pending_reqs(struct bfq_entity *entity)
> +{
> +	struct bfq_queue *bfqq = bfq_entity_to_bfqq(entity);
> +
> +	if (bfqq) {
> +		/*
> +		 * If the entity represents bfq_queue, and the queue belongs to
> +		 * root cgroup.
> +		 */
> +		if (!entity->parent)
> +			bfq_set_group_with_pending_reqs(bfqq->bfqd,
> +				&bfqq->bfqd->root_group->entity);
> +	} else {
> +		/* If the entity represents bfq_group. */
> +		struct bfq_group *bfqg =
> +			container_of(entity, struct bfq_group, entity);
> +		struct bfq_data *bfqd = bfqg->bfqd;
> +
> +		bfq_set_group_with_pending_reqs(bfqd, entity);
> +	}
> +}
> +#else
> +#define bfq_update_groups_with_pending_reqs(entity) \
> +	do {} while (0)
> +#endif
> 
> /**
>  * __bfq_activate_entity - handle activation of entity.
> @@ -999,19 +1035,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 v4 1/2] block, bfq: counted root group into 'num_groups_with_pending_reqs'

[yukuai (C)]

On 2021/10/20 16:51, Paolo Valente wrote:

>> @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
>> 			     struct bfq_queue *bfqq)
>> {
>> 	struct bfq_entity *entity = bfqq->entity.parent;
>> +	struct bfq_sched_data *sd;
>> +
>> +	/*
>> +	 * If the bfq queue is in root group, the decrement of
>> +	 * num_groups_with_pending_reqs is performed immediately upon the
>> +	 * deactivation of entity.
>> +	 */
>> +	if (!entity) {
>> +		entity = &bfqd->root_group->entity;
>> +		sd = entity->my_sched_data;
>> +
>> +		if (!sd->in_service_entity)
>> +			bfq_clear_group_with_pending_reqs(bfqd, entity);
>> +
>> +		return;
>> +	}
>>
>> 	for_each_entity(entity) {
>> -		struct bfq_sched_data *sd = entity->my_sched_data;
>> +		sd = entity->my_sched_data;
>>
>> 		if (sd->next_in_service || sd->in_service_entity) {
>> 			/*
>> @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
>> 		}
>>
>> 		/*
>> -		 * The decrement of num_groups_with_pending_reqs is
>> +		 * If the bfq queue is not in root group,
>> +		 * the decrement of num_groups_with_pending_reqs is
> 
> 
> I'm sorry if I didn't notice this before, but why do you postpone the
> decrement only for queues not in root group?  If I'm not missing
> anything, the active (i.e., with pending reqs) state of the root group
> is to be computed as that of ay other group.

Hi, Paolo

I thought if queue is in root group, then bfqq->entity.parent is NULL,
and such case is handled above, which is separate from previous
implementation for queues that are not in root group.

Is this the wrong way to handle root group?

Thanks,
Kuai

Re: [PATCH v4 1/2] block, bfq: counted root group into 'num_groups_with_pending_reqs'

[Paolo Valente]

> Il giorno 20 ott 2021, alle ore 11:20, yukuai (C) <yukuai3@huawei.com> ha scritto:
> 
> On 2021/10/20 16:51, Paolo Valente wrote:
> 
>>> @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
>>> 			     struct bfq_queue *bfqq)
>>> {
>>> 	struct bfq_entity *entity = bfqq->entity.parent;
>>> +	struct bfq_sched_data *sd;
>>> +
>>> +	/*
>>> +	 * If the bfq queue is in root group, the decrement of
>>> +	 * num_groups_with_pending_reqs is performed immediately upon the
>>> +	 * deactivation of entity.
>>> +	 */
>>> +	if (!entity) {
>>> +		entity = &bfqd->root_group->entity;
>>> +		sd = entity->my_sched_data;
>>> +
>>> +		if (!sd->in_service_entity)
>>> +			bfq_clear_group_with_pending_reqs(bfqd, entity);
>>> +
>>> +		return;
>>> +	}
>>> 
>>> 	for_each_entity(entity) {
>>> -		struct bfq_sched_data *sd = entity->my_sched_data;
>>> +		sd = entity->my_sched_data;
>>> 
>>> 		if (sd->next_in_service || sd->in_service_entity) {
>>> 			/*
>>> @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
>>> 		}
>>> 
>>> 		/*
>>> -		 * The decrement of num_groups_with_pending_reqs is
>>> +		 * If the bfq queue is not in root group,
>>> +		 * the decrement of num_groups_with_pending_reqs is
>> I'm sorry if I didn't notice this before, but why do you postpone the
>> decrement only for queues not in root group?  If I'm not missing
>> anything, the active (i.e., with pending reqs) state of the root group
>> is to be computed as that of ay other group.
> 
> Hi, Paolo
> 
> I thought if queue is in root group, then bfqq->entity.parent is NULL,
> and such case is handled above, which is separate from previous
> implementation for queues that are not in root group.
> 
> Is this the wrong way to handle root group?
> 

I think that, if we want to count also the root group among the active
ones, then the logic for tagging the root group as active must be the
same as the other groups. Or am I missing something?

Thanks,
Paolo

> Thanks,
> Kuai

Re: [PATCH v4 1/2] block, bfq: counted root group into 'num_groups_with_pending_reqs'

[yukuai (C)]

On 2021/10/20 17:29, Paolo Valente wrote:
> 
> 
>> Il giorno 20 ott 2021, alle ore 11:20, yukuai (C) <yukuai3@huawei.com> ha scritto:
>>
>> On 2021/10/20 16:51, Paolo Valente wrote:
>>
>>>> @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
>>>> 			     struct bfq_queue *bfqq)
>>>> {
>>>> 	struct bfq_entity *entity = bfqq->entity.parent;
>>>> +	struct bfq_sched_data *sd;
>>>> +
>>>> +	/*
>>>> +	 * If the bfq queue is in root group, the decrement of
>>>> +	 * num_groups_with_pending_reqs is performed immediately upon the
>>>> +	 * deactivation of entity.
>>>> +	 */
>>>> +	if (!entity) {
>>>> +		entity = &bfqd->root_group->entity;
>>>> +		sd = entity->my_sched_data;
>>>> +
>>>> +		if (!sd->in_service_entity)
>>>> +			bfq_clear_group_with_pending_reqs(bfqd, entity);
>>>> +
>>>> +		return;
>>>> +	}
>>>>
>>>> 	for_each_entity(entity) {
>>>> -		struct bfq_sched_data *sd = entity->my_sched_data;
>>>> +		sd = entity->my_sched_data;
>>>>
>>>> 		if (sd->next_in_service || sd->in_service_entity) {
>>>> 			/*
>>>> @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
>>>> 		}
>>>>
>>>> 		/*
>>>> -		 * The decrement of num_groups_with_pending_reqs is
>>>> +		 * If the bfq queue is not in root group,
>>>> +		 * the decrement of num_groups_with_pending_reqs is
>>> I'm sorry if I didn't notice this before, but why do you postpone the
>>> decrement only for queues not in root group?  If I'm not missing
>>> anything, the active (i.e., with pending reqs) state of the root group
>>> is to be computed as that of ay other group.
>>
>> Hi, Paolo
>>
>> I thought if queue is in root group, then bfqq->entity.parent is NULL,
>> and such case is handled above, which is separate from previous
>> implementation for queues that are not in root group.
>>
>> Is this the wrong way to handle root group?
>>
> 
> I think that, if we want to count also the root group among the active
> ones, then the logic for tagging the root group as active must be the
> same as the other groups. Or am I missing something?

Hi, Paolo

Currently, if queue is in root group, bfqq->entity.parent is NULL, and
this makes it hard to keep the same logic.

Can we store root_group->my_entity to bfqq->entity.parent if the queue
is in root group?

Thanks,
Kuai

Re: [PATCH v4 1/2] block, bfq: counted root group into 'num_groups_with_pending_reqs'

[Paolo Valente]

> Il giorno 20 ott 2021, alle ore 11:38, yukuai (C) <yukuai3@huawei.com> ha scritto:
> 
> On 2021/10/20 17:29, Paolo Valente wrote:
>>> Il giorno 20 ott 2021, alle ore 11:20, yukuai (C) <yukuai3@huawei.com> ha scritto:
>>> 
>>> On 2021/10/20 16:51, Paolo Valente wrote:
>>> 
>>>>> @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
>>>>> 			     struct bfq_queue *bfqq)
>>>>> {
>>>>> 	struct bfq_entity *entity = bfqq->entity.parent;
>>>>> +	struct bfq_sched_data *sd;
>>>>> +
>>>>> +	/*
>>>>> +	 * If the bfq queue is in root group, the decrement of
>>>>> +	 * num_groups_with_pending_reqs is performed immediately upon the
>>>>> +	 * deactivation of entity.
>>>>> +	 */
>>>>> +	if (!entity) {
>>>>> +		entity = &bfqd->root_group->entity;
>>>>> +		sd = entity->my_sched_data;
>>>>> +
>>>>> +		if (!sd->in_service_entity)
>>>>> +			bfq_clear_group_with_pending_reqs(bfqd, entity);
>>>>> +
>>>>> +		return;
>>>>> +	}
>>>>> 
>>>>> 	for_each_entity(entity) {
>>>>> -		struct bfq_sched_data *sd = entity->my_sched_data;
>>>>> +		sd = entity->my_sched_data;
>>>>> 
>>>>> 		if (sd->next_in_service || sd->in_service_entity) {
>>>>> 			/*
>>>>> @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
>>>>> 		}
>>>>> 
>>>>> 		/*
>>>>> -		 * The decrement of num_groups_with_pending_reqs is
>>>>> +		 * If the bfq queue is not in root group,
>>>>> +		 * the decrement of num_groups_with_pending_reqs is
>>>> I'm sorry if I didn't notice this before, but why do you postpone the
>>>> decrement only for queues not in root group?  If I'm not missing
>>>> anything, the active (i.e., with pending reqs) state of the root group
>>>> is to be computed as that of ay other group.
>>> 
>>> Hi, Paolo
>>> 
>>> I thought if queue is in root group, then bfqq->entity.parent is NULL,
>>> and such case is handled above, which is separate from previous
>>> implementation for queues that are not in root group.
>>> 
>>> Is this the wrong way to handle root group?
>>> 
>> I think that, if we want to count also the root group among the active
>> ones, then the logic for tagging the root group as active must be the
>> same as the other groups. Or am I missing something?
> 
> Hi, Paolo
> 
> Currently, if queue is in root group, bfqq->entity.parent is NULL, and
> this makes it hard to keep the same logic.
> 
> Can we store root_group->my_entity to bfqq->entity.parent if the queue
> is in root group?
> 

Any sensible implementation is ok for me.  Usually, stuff for root
group is in the bfqd.

Thanks,
Paolo

> Thanks,
> Kuai

Re: [PATCH v4 1/2] block, bfq: counted root group into 'num_groups_with_pending_reqs'

[yukuai (C)]

On 2021/10/20 17:43, Paolo Valente wrote:
> 
> 
>> Il giorno 20 ott 2021, alle ore 11:38, yukuai (C) <yukuai3@huawei.com> ha scritto:
>>
>> On 2021/10/20 17:29, Paolo Valente wrote:
>>>> Il giorno 20 ott 2021, alle ore 11:20, yukuai (C) <yukuai3@huawei.com> ha scritto:
>>>>
>>>> On 2021/10/20 16:51, Paolo Valente wrote:
>>>>
>>>>>> @@ -860,9 +870,25 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
>>>>>> 			     struct bfq_queue *bfqq)
>>>>>> {
>>>>>> 	struct bfq_entity *entity = bfqq->entity.parent;
>>>>>> +	struct bfq_sched_data *sd;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If the bfq queue is in root group, the decrement of
>>>>>> +	 * num_groups_with_pending_reqs is performed immediately upon the
>>>>>> +	 * deactivation of entity.
>>>>>> +	 */
>>>>>> +	if (!entity) {
>>>>>> +		entity = &bfqd->root_group->entity;
>>>>>> +		sd = entity->my_sched_data;
>>>>>> +
>>>>>> +		if (!sd->in_service_entity)
>>>>>> +			bfq_clear_group_with_pending_reqs(bfqd, entity);
>>>>>> +
>>>>>> +		return;
>>>>>> +	}
>>>>>>
>>>>>> 	for_each_entity(entity) {
>>>>>> -		struct bfq_sched_data *sd = entity->my_sched_data;
>>>>>> +		sd = entity->my_sched_data;
>>>>>>
>>>>>> 		if (sd->next_in_service || sd->in_service_entity) {
>>>>>> 			/*
>>>>>> @@ -880,7 +906,8 @@ void bfq_weights_tree_remove(struct bfq_data *bfqd,
>>>>>> 		}
>>>>>>
>>>>>> 		/*
>>>>>> -		 * The decrement of num_groups_with_pending_reqs is
>>>>>> +		 * If the bfq queue is not in root group,
>>>>>> +		 * the decrement of num_groups_with_pending_reqs is
>>>>> I'm sorry if I didn't notice this before, but why do you postpone the
>>>>> decrement only for queues not in root group?  If I'm not missing
>>>>> anything, the active (i.e., with pending reqs) state of the root group
>>>>> is to be computed as that of ay other group.
>>>>
>>>> Hi, Paolo
>>>>
>>>> I thought if queue is in root group, then bfqq->entity.parent is NULL,
>>>> and such case is handled above, which is separate from previous
>>>> implementation for queues that are not in root group.
>>>>
>>>> Is this the wrong way to handle root group?
>>>>
>>> I think that, if we want to count also the root group among the active
>>> ones, then the logic for tagging the root group as active must be the
>>> same as the other groups. Or am I missing something?
>>
>> Hi, Paolo
>>
>> Currently, if queue is in root group, bfqq->entity.parent is NULL, and
>> this makes it hard to keep the same logic.
>>
>> Can we store root_group->my_entity to bfqq->entity.parent if the queue
>> is in root group?
>>
> 
> Any sensible implementation is ok for me.  Usually, stuff for root
> group is in the bfqd.
> 

I'll try to do implement that way,

Thanks,
Kuai

> Thanks,
> Paolo
> 
>> Thanks,
>> Kuai
> 
> .
>