lkml.org 
[lkml]   [2020]   [Jun]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] sched/fair: update_pick_idlest() Select group with lowest group_util when idle_cpus are equal
On Wed, 17 Jun 2020 at 06:50, Valentin Schneider
<valentin.schneider@arm.com> wrote:
>
>
> On 16/06/20 17:48, peter.puhov@linaro.org wrote:
> > From: Peter Puhov <peter.puhov@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.schneider@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.

\
 
 \ /
  Last update: 2020-06-17 16:54    [W:0.073 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site