Re: [PATCH v2] sched/fair: fix 1 task per CPU

[Vincent Guittot]

On Thu, 13 Dec 2018 at 11:44, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Mon, Sep 10, 2018 at 04:43:09PM +0200, Vincent Guittot wrote:
> > When CPUs have different capacity because of RT/DL tasks or
> > micro-architecture or max frequency differences, there are situation where
> > the imbalance is not correctly set to migrate waiting task on the idle CPU.
> >
> > The UC uses the force_balance case:
> >
> >       if (env->idle != CPU_NOT_IDLE && group_has_capacity(env, local) &&
> >           busiest->group_no_capacity)
> >               goto force_balance;
> >
> > But calculate_imbalance fails to set the right amount of load to migrate
> > a task because of the special condition:
> >
> >   busiest->avg_load <= sds->avg_load || local->avg_load >= sds->avg_load)
> >
> > Add in fix_small_imbalance, this special case that triggered the force
> > balance in order to make sure that the amount of load to migrate will be
> > enough.
>
> So I think this patch is going in the wrong direction for a number of
> reasons:
>
>  - we'd like to get rid of fix_small_imbalance(), and this adds to it;
>
>  - the whole load_per_task stuff is terminally broken, it _cannot_ work
>    right.
>
>
> What I've suggested in the past is parameterizing the load balancer and
> picking different criteria to balance on:

This patch is clearly a fix of the current implementation.
What you suggest below makes sense but implies a significant rework in
the calculate_imbalance and the load_balancer in general and will need
more time to reach a stable state.
Nevertheless, I will have a look at that

I imagine that your feedback for https://lkml.org/lkml/2018/10/2/283
will be the same ?

Vincent

>
>  - nr_running ; if there are idle CPUs around
>  - utilization ; if there's idle time (u<1)
>  - weight
>
> And I suppose you're hitting one of the nr_running cases here.
>
> > Signed-off-by: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> > ---
> >  kernel/sched/fair.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 309c93f..72bc5e8 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -8048,6 +8048,20 @@ void fix_small_imbalance(struct lb_env *env, struct sd_lb_stats *sds)
> >       local = &sds->local_stat;
> >       busiest = &sds->busiest_stat;
> >
> > +     /*
> > +      * There is available capacity in local group and busiest group is
> > +      * overloaded but calculate_imbalance can't compute the amount of load
> > +      * to migrate because load_avg became meaningless due to asymetric
> > +      * capacity between groups. In such case, we only want to migrate at
> > +      * least one tasks of the busiest group and rely of the average load
> > +      * per task to ensure the migration.
> > +      */
> > +     if (env->idle != CPU_NOT_IDLE && group_has_capacity(env, local) &&
> > +         busiest->group_no_capacity) {
> > +             env->imbalance = busiest->load_per_task;
> > +             return;
> > +     }
> > +
> >       if (!local->sum_nr_running)
> >               local->load_per_task = cpu_avg_load_per_task(env->dst_cpu);
> >       else if (busiest->load_per_task > local->load_per_task)
> > --
> > 2.7.4
> >