On Wed, 17 Jun 2020 at 06:50, Valentin Schneider <valentin.schnei...@arm.com> wrote: > > > On 16/06/20 17:48, peter.pu...@linaro.org wrote: > > From: Peter Puhov <peter.pu...@linaro.org> > > We tested this patch with following benchmarks: > > perf bench -f simple sched pipe -l 4000000 > > perf bench -f simple sched messaging -l 30000 > > perf bench -f simple mem memset -s 3GB -l 15 -f default > > perf bench -f simple futex wake -s -t 640 -w 1 > > sysbench cpu --threads=8 --cpu-max-prime=10000 run > > sysbench memory --memory-access-mode=rnd --threads=8 run > > sysbench threads --threads=8 run > > sysbench mutex --mutex-num=1 --threads=8 run > > hackbench --loops 20000 > > hackbench --pipe --threads --loops 20000 > > hackbench --pipe --threads --loops 20000 --datasize 4096 > > > > and found some performance improvements in: > > sysbench threads > > sysbench mutex > > perf bench futex wake > > and no regressions in others. > > > > One nitpick for the results of those: condensing them in a table form would > make them more reader-friendly. Perhaps something like: > > | Benchmark | Metric | Lower is better? | BASELINE | SERIES | DELTA | > |------------------+----------+------------------+----------+--------+-------| > | Sysbench threads | # events | No | 45526 | 56567 | +24% | > | Sysbench mutex | ... | | | | | > > If you want to include more stats for each benchmark, you could have one table > per (e.g. see [1]) - it'd still be a more readable form (or so I believe). > > [1]: > https://lore.kernel.org/lkml/20200206191957.12325-1-valentin.schnei...@arm.com/ > Good point. I will reformat test results.
> > --- > > kernel/sched/fair.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > index 02f323b85b6d..abcbdf80ee75 100644 > > --- a/kernel/sched/fair.c > > +++ b/kernel/sched/fair.c > > @@ -8662,8 +8662,14 @@ static bool update_pick_idlest(struct sched_group > > *idlest, > > > > case group_has_spare: > > /* Select group with most idle CPUs */ > > - if (idlest_sgs->idle_cpus >= sgs->idle_cpus) > > + if (idlest_sgs->idle_cpus > sgs->idle_cpus) > > return false; > > + > > + /* Select group with lowest group_util */ > > + if (idlest_sgs->idle_cpus == sgs->idle_cpus && > > + idlest_sgs->group_util <= sgs->group_util) > > + return false; > > + > > break; > > } > > update_sd_pick_busiest() uses the group's nr_running instead. You mention > in the changelog that using nr_running is a possible alternative, did you > try benchmarking that and seeing how it compares to using group_util? > > I think it would be nice to keep pick_busiest() and pick_idlest() aligned > wherever possible/sensible. > I agree with you. > Also, there can be cases where one group has a few "big" tasks and another > has a handful more "small" tasks. Say something like > > sgs_a->group_util = U > sgs_a->sum_nr_running = N > > sgs_b->group_util = U*4/3 > sgs_b->sum_nr_running = N*2/3 > > (sgs_b has more util per task, i.e. bigger tasks on average) > > Given that we're in the 'group_has_spare' case, I would think picking the > group with the lesser amount of running tasks would make sense. Though I > guess you can find pathological cases where the util per task difference is > huge and we should look at util first... I will re-run the tests with logic based on sum_nr_running and post results. Thank you for suggestions.
