MIME-Version: 1.0
In-Reply-To: <20161208140933.GF5462@codeblueprint.co.uk>
References: <1480088073-11642-1-git-send-email-vincent.guittot@linaro.org>
 <1480088073-11642-3-git-send-email-vincent.guittot@linaro.org>
 <CAE40pde4VRH8LfRWMX3Vfq5pRoysUB6UuHTzN+VrcKMdSNW0uA@mail.gmail.com>
 <20161203214707.GI20785@codeblueprint.co.uk> <20161205092735.GA9161@linaro.org>
 <20161205133546.GN20785@codeblueprint.co.uk> <20161208140933.GF5462@codeblueprint.co.uk>
From: Vincent Guittot <vincent.guittot@linaro.org>
Date: Thu, 8 Dec 2016 15:33:29 +0100
Message-ID: <CAKfTPtC0_WuEsOzkTG8VD1bqDfSAgg7SO5mOKzSjGGb1SaiPqA@mail.gmail.com>
Subject: Re: [PATCH 2/2 v2] sched: use load_avg for selecting idlest group
To: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Brendan Gregg <brendan.d.gregg@gmail.com>,
        Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Morten Rasmussen <Morten.Rasmussen@arm.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Wanpeng Li <kernellwp@gmail.com>, Yuyang Du <yuyang.du@intel.com>,
        Mike Galbraith <umgwanakikbuti@gmail.com>,
        Mel Gorman <mgorman@techsingularity.net>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3593
Lines: 75

On 8 December 2016 at 15:09, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Mon, 05 Dec, at 01:35:46PM, Matt Fleming wrote:
>> On Mon, 05 Dec, at 10:27:36AM, Vincent Guittot wrote:
>> >
>> > Hi Matt,
>> >
>> > Thanks for the results.
>> >
>> > During the review, it has been pointed out by Morten that the test condition
>> > (100*this_avg_load < imbalance_scale*min_avg_load) makes more sense than
>> > (100*min_avg_load > imbalance_scale*this_avg_load). But i see lower
>> > performances with this change. Coud you run tests with the change below on
>> > top of the patchset ?
>> >
>> > ---
>> >  kernel/sched/fair.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> > index e8d1ae7..0129fbb 100644
>> > --- a/kernel/sched/fair.c
>> > +++ b/kernel/sched/fair.c
>> > @@ -5514,7 +5514,7 @@ find_idlest_group(struct sched_domain *sd, struct task_struct *p,
>> >     if (!idlest ||
>> >         (min_runnable_load > (this_runnable_load + imbalance)) ||
>> >         ((this_runnable_load < (min_runnable_load + imbalance)) &&
>> > -                   (100*min_avg_load > imbalance_scale*this_avg_load)))
>> > +                   (100*this_avg_load < imbalance_scale*min_avg_load)))
>> >             return NULL;
>> >     return idlest;
>> >  }
>>
>> Queued for testing.
>
> Most of the machines didn't notice the difference with this new patch.
> However, I did come across one test that showed a negative change,

OK so you don't see perf impact between the 2 test condition except
for the machine below
So IMHO, we should use the new test condition which tries to keep task
local if there is no other CPU that is obviously less loaded.

I'm going to prepare a new version of the patchset and apply all comments

Thanks
Vincent

>
>
> hackbench-thread-pipes
>                         4.9.0-rc6             4.9.0-rc6             4.9.0-rc6             4.9.0-rc6
>                         tip-sched      fix-fig-for-fork               fix-sig   fix-fig-for-fork-v2
> Amean    1       0.1266 (  0.00%)      0.1269 ( -0.23%)      0.1287 ( -1.69%)      0.1357 ( -7.22%)
> Amean    4       0.4989 (  0.00%)      0.5174 ( -3.72%)      0.5251 ( -5.27%)      0.5117 ( -2.58%)
> Amean    7       0.8510 (  0.00%)      0.8517 ( -0.08%)      0.8964 ( -5.34%)      0.8801 ( -3.42%)
> Amean    12      1.0699 (  0.00%)      1.0484 (  2.00%)      1.0147 (  5.15%)      1.0759 ( -0.56%)
> Amean    21      1.2816 (  0.00%)      1.2140 (  5.27%)      1.1879 (  7.31%)      1.2414 (  3.13%)
> Amean    30      1.4440 (  0.00%)      1.4003 (  3.03%)      1.3969 (  3.26%)      1.4057 (  2.65%)
> Amean    48      1.5773 (  0.00%)      1.5983 ( -1.33%)      1.3984 ( 11.34%)      1.5624 (  0.94%)
> Amean    79      2.2343 (  0.00%)      2.3066 ( -3.24%)      2.0053 ( 10.25%)      2.2630 ( -1.29%)
> Amean    96      2.6736 (  0.00%)      2.4313 (  9.06%)      2.4181 (  9.55%)      2.4717 (  7.55%)
>
>            4.9.0-rc6   4.9.0-rc6   4.9.0-rc6   4.9.0-rc6
>            tip-schedfix-fig-for-fork     fix-sigfix-fig-for-fork-v2
> User          129.53      128.64      127.70      131.00
> System       1784.54     1744.21     1654.08     1744.00
> Elapsed        92.07       90.44       86.95       91.00
>
> Looking at the 48 and 79 groups rows for mean there's a noticeable
> drop off in performance of ~10%, which should be outside of the noise
> for this test. This is a 2 socket, 4 NUMA node (yes, really), 24 cpus
> AMD opteron circa 2010.
>
> Given the age of this machine, I don't think it's worth holding up the
> patch.