Messages in this thread | | | From | Vincent Guittot <> | Date | Wed, 27 Jan 2021 14:07:50 +0100 | Subject | Re: [PATCH 4/4] sched/fair: Merge select_idle_core/cpu() |
| |
On Wed, 27 Jan 2021 at 13:02, Mel Gorman <mgorman@techsingularity.net> 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
| |