* Re: [PATCH] bfq: don't check active group if bfq.weight is not changed
2021-01-14 12:24 ` [PATCH] bfq: don't check active group if bfq.weight is not changed Yu Kuai
@ 2021-01-15 13:35 ` kernel test robot
2021-01-15 13:35 ` kernel test robot
2021-01-22 9:46 ` Paolo Valente
2 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-01-15 13:35 UTC (permalink / raw)
To: Yu Kuai, tj, axboe, paolo.valente
Cc: kbuild-all, cgroups, linux-block, linux-kernel, yukuai3, yi.zhang
[-- Attachment #1: Type: text/plain, Size: 2014 bytes --]
Hi Yu,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on block/for-next]
[also build test ERROR on v5.11-rc3 next-20210115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Yu-Kuai/bfq-don-t-check-active-group-if-bfq-weight-is-not-changed/20210115-112031
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/2a2ab6f73f0608cec85e1f15254edc78a75d0366
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yu-Kuai/bfq-don-t-check-active-group-if-bfq-weight-is-not-changed/20210115-112031
git checkout 2a2ab6f73f0608cec85e1f15254edc78a75d0366
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: block/bfq-cgroup.o: in function `bfq_pd_init':
>> bfq-cgroup.c:(.text+0x2f0): undefined reference to `__bad_cmpxchg'
arm-linux-gnueabi-ld: block/bfq-cgroup.o: in function `bfq_io_set_weight_legacy':
bfq-cgroup.c:(.text+0x448): undefined reference to `__bad_cmpxchg'
arm-linux-gnueabi-ld: block/bfq-cgroup.o: in function `bfq_io_set_weight':
bfq-cgroup.c:(.text+0x1390): undefined reference to `__bad_cmpxchg'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 77904 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] bfq: don't check active group if bfq.weight is not changed
2021-01-14 12:24 ` [PATCH] bfq: don't check active group if bfq.weight is not changed Yu Kuai
2021-01-15 13:35 ` kernel test robot
@ 2021-01-15 13:35 ` kernel test robot
2021-01-22 9:46 ` Paolo Valente
2 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-01-15 13:35 UTC (permalink / raw)
To: Yu Kuai, tj, axboe, paolo.valente
Cc: kbuild-all, cgroups, linux-block, linux-kernel, yukuai3, yi.zhang
[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]
Hi Yu,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on block/for-next]
[also build test ERROR on v5.11-rc3 next-20210115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Yu-Kuai/bfq-don-t-check-active-group-if-bfq-weight-is-not-changed/20210115-112031
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/2a2ab6f73f0608cec85e1f15254edc78a75d0366
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yu-Kuai/bfq-don-t-check-active-group-if-bfq-weight-is-not-changed/20210115-112031
git checkout 2a2ab6f73f0608cec85e1f15254edc78a75d0366
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "__bad_cmpxchg" [block/bfq.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 78467 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] bfq: don't check active group if bfq.weight is not changed
2021-01-14 12:24 ` [PATCH] bfq: don't check active group if bfq.weight is not changed Yu Kuai
2021-01-15 13:35 ` kernel test robot
2021-01-15 13:35 ` kernel test robot
@ 2021-01-22 9:46 ` Paolo Valente
2 siblings, 0 replies; 4+ messages in thread
From: Paolo Valente @ 2021-01-22 9:46 UTC (permalink / raw)
To: Yu Kuai; +Cc: Tejun Heo, axboe, cgroups, linux-block, linux-kernel, yi.zhang
> Il giorno 14 gen 2021, alle ore 13:24, Yu Kuai <yukuai3@huawei.com> 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@huawei.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread