Date: Wed, 30 Nov 2016 14:24:25 +0000
From: Morten Rasmussen <morten.rasmussen@arm.com>
To: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Matt Fleming <matt@codeblueprint.co.uk>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Wanpeng Li <kernellwp@gmail.com>, Yuyang Du <yuyang.du@intel.com>,
        Mike Galbraith <umgwanakikbuti@gmail.com>
Subject: Re: [PATCH 2/2 v2] sched: use load_avg for selecting idlest group
Message-ID: <20161130142425.GF1716@e105550-lin.cambridge.arm.com>
References: <1480088073-11642-1-git-send-email-vincent.guittot@linaro.org>
 <1480088073-11642-3-git-send-email-vincent.guittot@linaro.org>
 <20161130124912.GD1716@e105550-lin.cambridge.arm.com>
 <CAKfTPtBMeL9Omvj+KzL2KAhTH8rjz5BzPypaj6DmCXn0ykZpWg@mail.gmail.com>
 <CAKfTPtAavCw+bYXOd1f6V7SfWjn3xSJNVcR3Za9eOMP_GFHd3w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAKfTPtAavCw+bYXOd1f6V7SfWjn3xSJNVcR3Za9eOMP_GFHd3w@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2566
Lines: 52

On Wed, Nov 30, 2016 at 02:54:00PM +0100, Vincent Guittot wrote:
> On 30 November 2016 at 14:49, Vincent Guittot
> <vincent.guittot@linaro.org> wrote:
> > On 30 November 2016 at 13:49, Morten Rasmussen <morten.rasmussen@arm.com> wrote:
> >> On Fri, Nov 25, 2016 at 04:34:33PM +0100, Vincent Guittot wrote:
> >>> find_idlest_group() only compares the runnable_load_avg when looking for
> >>> the least loaded group. But on fork intensive use case like hackbench
> >
> > [snip]
> >
> >>> +                             min_avg_load = avg_load;
> >>> +                             idlest = group;
> >>> +                     } else if ((runnable_load < (min_runnable_load + imbalance)) &&
> >>> +                                     (100*min_avg_load > imbalance_scale*avg_load)) {
> >>> +                             /*
> >>> +                              * The runnable loads are close so we take
> >>> +                              * into account blocked load through avg_load
> >>> +                              *  which is blocked + runnable load
> >>> +                              */
> >>> +                             min_avg_load = avg_load;
> >>>                               idlest = group;
> >>>                       }
> >>>
> >>> @@ -5470,13 +5495,16 @@ find_idlest_group(struct sched_domain *sd, struct task_struct *p,
> >>>               goto no_spare;
> >>>
> >>>       if (this_spare > task_util(p) / 2 &&
> >>> -         imbalance*this_spare > 100*most_spare)
> >>> +         imbalance_scale*this_spare > 100*most_spare)
> >>>               return NULL;
> >>>       else if (most_spare > task_util(p) / 2)
> >>>               return most_spare_sg;
> >>>
> >>>  no_spare:
> >>> -     if (!idlest || 100*this_load < imbalance*min_load)
> >>> +     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)))
> >>
> >> I don't get why you have imbalance_scale applied to this_avg_load and
> >> not min_avg_load. IIUC, you end up preferring non-local groups?
> >
> > In fact, I have keep the same condition that is used when looping the group.
> > You're right that we should prefer local rq if avg_load are close and
> > test the condition
> > (100*this_avg_load > imbalance_scale*min_avg_load) instead
> 
> Of course the correct condition is
>  (100*this_avg_load < imbalance_scale*min_avg_load)

Agreed, I should have read the entire thread before replying :-)