Re: [PATCH 4/4] sched/fair: Merge select_idle_core/cpu()

[Vincent Guittot]

On Wed, 27 Jan 2021 at 13:02, Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Jan 27, 2021 at 11:43:22AM +0100, Vincent Guittot wrote:
> > > @@ -6149,18 +6161,31 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, int t
> > >         }
> > >
> > >         for_each_cpu_wrap(cpu, cpus, target) {
> > > -               if (!--nr)
> > > -                       return -1;
> > > -               if (available_idle_cpu(cpu) || sched_idle_cpu(cpu))
> > > -                       break;
> > > +               if (smt) {
> > > +                       i = select_idle_core(p, cpu, cpus, &idle_cpu);
> > > +                       if ((unsigned int)i < nr_cpumask_bits)
> > > +                               return i;
> > > +
> > > +               } else {
> > > +                       if (!--nr)
> > > +                               return -1;
> > > +                       i = __select_idle_cpu(cpu);
> >
> > you should use idle_cpu directly instead of this intermediate i variable
> >
> > +                       idle_cpu = __select_idle_cpu(cpu);
> > +                       if ((unsigned int)idle_cpu < nr_cpumask_bits)
> > +                               break;
> >
> > Apart ths small comment above, the patch looks good to me and I
> > haven't any performance regression anymore
> >
>
> It's matching the code sequence in the SMT block. If we are going to make
> that change, then go the full way with this?
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 52a650aa2108..01e40e36c386 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6129,7 +6129,7 @@ static inline int select_idle_core(struct task_struct *p, int core, struct cpuma
>  static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, int target)
>  {
>         struct cpumask *cpus = this_cpu_cpumask_var_ptr(select_idle_mask);
> -       int i, cpu, idle_cpu = -1, nr = INT_MAX;
> +       int cpu, idle_cpu = -1, nr = INT_MAX;
>         bool smt = test_idle_cores(target, false);
>         int this = smp_processor_id();
>         struct sched_domain *this_sd;
> @@ -6162,18 +6162,16 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, int t
>
>         for_each_cpu_wrap(cpu, cpus, target) {
>                 if (smt) {
> -                       i = select_idle_core(p, cpu, cpus, &idle_cpu);
> -                       if ((unsigned int)i < nr_cpumask_bits)
> -                               return i;
> +                       idle_cpu = select_idle_core(p, cpu, cpus, &idle_cpu);

but how do you differentiate idle core (return value) and an idle cpu
in the core set in &idle_cpu

You will return as soon as a cpu is idle and before testing all cores

> +                       if ((unsigned int)idle_cpu < nr_cpumask_bits)
> +                               return idle_cpu;
>
>                 } else {
>                         if (!--nr)
>                                 return -1;
> -                       i = __select_idle_cpu(cpu);
> -                       if ((unsigned int)i < nr_cpumask_bits) {
> -                               idle_cpu = i;
> +                       idle_cpu = __select_idle_cpu(cpu);
> +                       if ((unsigned int)idle_cpu < nr_cpumask_bits)
>                                 break;
> -                       }
>                 }
>         }
>
>
> --
> Mel Gorman
> SUSE Labs