Re: [PATCH 3/3] sched/fair: fix unnecessary increase of balance interval

[Vincent Guittot]

On Thu, 13 Dec 2018 at 14:52, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Tue, Aug 07, 2018 at 05:56:27PM +0200, Vincent Guittot wrote:
> > +static inline bool
> > +asym_active_balance(enum cpu_idle_type idle, unsigned int flags, int dst, int src)
> >  {
> > +     if (idle != CPU_NOT_IDLE) {
> >
> >               /*
> >                * ASYM_PACKING needs to force migrate tasks from busy but
> >                * lower priority CPUs in order to pack all tasks in the
> >                * highest priority CPUs.
> >                */
> > +             if ((flags & SD_ASYM_PACKING) &&
> > +                 sched_asym_prefer(dst, src))
> > +                     return true;
> >       }
> >
> > +     return false;
> > +}
> > +
> > +static int need_active_balance(struct lb_env *env)
> > +{
> > +     struct sched_domain *sd = env->sd;
> > +
> > +
> > +     if (asym_active_balance(env->idle, sd->flags, env->dst_cpu, env->src_cpu))
> > +             return 1;
> > +
> >       /*
> >        * The dst_cpu is idle and the src_cpu CPU has only 1 CFS task.
> >        * It's worth migrating the task if the src_cpu's capacity is reduced
> > @@ -8650,7 +8660,8 @@ static int load_balance(int this_cpu, struct rq *this_rq,
> >       } else
> >               sd->nr_balance_failed = 0;
> >
> > +     if (likely(!active_balance) ||
> > +         asym_active_balance(env.idle, sd->flags, env.dst_cpu, env.src_cpu)) {
>
> Perhaps like the below?

Yes. Far more simple and readable

>
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8857,21 +8857,24 @@ static struct rq *find_busiest_queue(str
>   */
>  #define MAX_PINNED_INTERVAL    512
>
> +static inline bool
> +asym_active_balance(struct lb_env *env)
> +{
> +       /*
> +        * ASYM_PACKING needs to force migrate tasks from busy but
> +        * lower priority CPUs in order to pack all tasks in the
> +        * highest priority CPUs.
> +        */
> +       return env->idle != CPU_NOT_IDLE && (env->sd->flags & SD_ASYM_PACKING) &&
> +               sched_asym_prefer(env->dst_cpu, env->src_cpu);
> +}
> +
>  static int need_active_balance(struct lb_env *env)
>  {
>         struct sched_domain *sd = env->sd;
>
> -       if (env->idle != CPU_NOT_IDLE) {
> -
> -               /*
> -                * ASYM_PACKING needs to force migrate tasks from busy but
> -                * lower priority CPUs in order to pack all tasks in the
> -                * highest priority CPUs.
> -                */
> -               if ((sd->flags & SD_ASYM_PACKING) &&
> -                   sched_asym_prefer(env->dst_cpu, env->src_cpu))
> -                       return 1;
> -       }
> +       if (asym_active_balance(env))
> +               return 1;
>
>         /*
>          * The dst_cpu is idle and the src_cpu CPU has only 1 CFS task.
> @@ -9150,7 +9153,7 @@ static int load_balance(int this_cpu, st
>         } else
>                 sd->nr_balance_failed = 0;
>
> -       if (likely(!active_balance)) {
> +       if (likely(!active_balance) || asym_active_balance(&env)) {
>                 /* We were unbalanced, so reset the balancing interval */
>                 sd->balance_interval = sd->min_interval;
>         } else {