Re: [PATCH] bfq: don't check active group if bfq.weight is not changed

[Paolo Valente]

> Il giorno 14 gen 2021, alle ore 13:24, Yu Kuai <yukuai3@xxxxxxxxxx> ha scritto:
> 
> Now the group scheduling in BFQ depends on the check of active group,
> but in most cases group scheduling is not used and the checking
> of active group will cause bfq_asymmetric_scenario() and its caller
> bfq_better_to_idle() to always return true, so the throughput
> will be impacted if the workload doesn't need idle (e.g. random rw)
> 
> To fix that, adding check in bfq_io_set_weight_legacy() and
> bfq_pd_init() to check whether or not group scheduling is used
> (a non-default weight is used). If not, there is no need
> to check active group.
> 

Hi,
I do like the goal you want to attain.  Still, I see a problem with
your proposal.  Consider two groups, say A and B.  Suppose that both
have the same, default weight.  Yet, group A generates large I/O
requests, while group B generates small requests.  With your change,
idling would not be performed.  This would cause group A to steal
bandwidth to group B, in proportion to how large its requests are
compared with those of group B.

As a possible solution, maybe we would need also a varied_rq_size
flag, similar to the varied_weights flag?

Thoughts?

Thanks for your contribution,
Paolo

> Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx>
> ---
> block/bfq-cgroup.c  | 14 ++++++++++++--
> block/bfq-iosched.c |  8 +++-----
> block/bfq-iosched.h | 19 +++++++++++++++++++
> 3 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
> index b791e2041e49..b4ac42c4bd9f 100644
> --- a/block/bfq-cgroup.c
> +++ b/block/bfq-cgroup.c
> @@ -505,12 +505,18 @@ static struct blkcg_policy_data *bfq_cpd_alloc(gfp_t gfp)
> 	return &bgd->pd;
> }
> 
> +static inline int bfq_dft_weight(void)
> +{
> +	return cgroup_subsys_on_dfl(io_cgrp_subsys) ?
> +	       CGROUP_WEIGHT_DFL : BFQ_WEIGHT_LEGACY_DFL;
> +
> +}
> +
> static void bfq_cpd_init(struct blkcg_policy_data *cpd)
> {
> 	struct bfq_group_data *d = cpd_to_bfqgd(cpd);
> 
> -	d->weight = cgroup_subsys_on_dfl(io_cgrp_subsys) ?
> -		CGROUP_WEIGHT_DFL : BFQ_WEIGHT_LEGACY_DFL;
> +	d->weight = bfq_dft_weight();
> }
> 
> static void bfq_cpd_free(struct blkcg_policy_data *cpd)
> @@ -554,6 +560,9 @@ static void bfq_pd_init(struct blkg_policy_data *pd)
> 	bfqg->bfqd = bfqd;
> 	bfqg->active_entities = 0;
> 	bfqg->rq_pos_tree = RB_ROOT;
> +
> +	if (entity->new_weight != bfq_dft_weight())
> +		bfqd_enable_active_group_check(bfqd);
> }
> 
> static void bfq_pd_free(struct blkg_policy_data *pd)
> @@ -1013,6 +1022,7 @@ static void bfq_group_set_weight(struct bfq_group *bfqg, u64 weight, u64 dev_wei
> 		 */
> 		smp_wmb();
> 		bfqg->entity.prio_changed = 1;
> +		bfqd_enable_active_group_check(bfqg->bfqd);
> 	}
> }
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 9e4eb0fc1c16..1b695de1df95 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -699,11 +699,8 @@ static bool bfq_asymmetric_scenario(struct bfq_data *bfqd,
> 		(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 ||
> +	       bfqd_has_active_group(bfqd);
> }
> 
> /*
> @@ -6472,6 +6469,7 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e)
> 
> 	bfqd->queue_weights_tree = RB_ROOT_CACHED;
> 	bfqd->num_groups_with_pending_reqs = 0;
> +	bfqd->check_active_group = false;
> 
> 	INIT_LIST_HEAD(&bfqd->active_list);
> 	INIT_LIST_HEAD(&bfqd->idle_list);
> diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
> index 703895224562..216509013012 100644
> --- a/block/bfq-iosched.h
> +++ b/block/bfq-iosched.h
> @@ -524,6 +524,8 @@ struct bfq_data {
> 
> 	/* true if the device is non rotational and performs queueing */
> 	bool nonrot_with_queueing;
> +	/* true if need to check num_groups_with_pending_reqs */
> +	bool check_active_group;
> 
> 	/*
> 	 * Maximum number of requests in driver in the last
> @@ -1066,6 +1068,17 @@ static inline void bfq_pid_to_str(int pid, char *str, int len)
> }
> 
> #ifdef CONFIG_BFQ_GROUP_IOSCHED
> +static inline void bfqd_enable_active_group_check(struct bfq_data *bfqd)
> +{
> +	cmpxchg_relaxed(&bfqd->check_active_group, false, true);
> +}
> +
> +static inline bool bfqd_has_active_group(struct bfq_data *bfqd)
> +{
> +	return bfqd->check_active_group &&
> +	       bfqd->num_groups_with_pending_reqs > 0;
> +}
> +
> struct bfq_group *bfqq_group(struct bfq_queue *bfqq);
> 
> #define bfq_log_bfqq(bfqd, bfqq, fmt, args...)	do {			\
> @@ -1085,6 +1098,12 @@ struct bfq_group *bfqq_group(struct bfq_queue *bfqq);
> } while (0)
> 
> #else /* CONFIG_BFQ_GROUP_IOSCHED */
> +static inline void bfqd_enable_active_group_check(struct bfq_data *bfqd) {}
> +
> +static inline bool bfqd_has_active_group(struct bfq_data *bfqd)
> +{
> +	return false;
> +}
> 
> #define bfq_log_bfqq(bfqd, bfqq, fmt, args...) do {	\
> 	char pid_str[MAX_PID_STR_LENGTH];	\
> -- 
> 2.25.4
>