On Mon, Jan 18, 2021 at 08:55:03PM +0800, Li, Aubrey wrote: > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > index 12e08da90024..6c0f841e9e75 100644 > > --- a/kernel/sched/fair.c > > +++ b/kernel/sched/fair.c > > @@ -6006,6 +6006,14 @@ static inline int find_idlest_cpu(struct > > sched_domain *sd, struct task_struct *p > > return new_cpu; > > } > > > > +static inline int __select_idle_cpu(struct task_struct *p, int core, > > struct cpumask *cpus) > > Sorry if I missed anything, why p and cpus are needed here? >
They are not needed. The original code was matching the calling pattern for select_idle_core() which needs p and cpus to check if sibling CPUs are allowed. > > @@ -6135,7 +6147,7 @@ static int select_idle_cpu(struct task_struct *p, > > struct sched_domain *sd, int t > > > > cpumask_and(cpus, sched_domain_span(sd), p->cpus_ptr); > > > > - if (sched_feat(SIS_PROP)) { > > + if (sched_feat(SIS_PROP) && !smt) { > > Is it possible the system does have a idle core, but I still don't want to > scan the entire llc domain? > This version is matching historical behaviour. To limit the scan for cores, select_idle_core() would need to obey SIS_PROP and that patch was dropped as it introduced regressions. It would only be considered once SIS_PROP had better metrics for limiting the depth of the search. > > u64 avg_cost, avg_idle, span_avg; > > > > /* > > @@ -6159,16 +6171,29 @@ 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; > > It looks like nr only makes sense when smt = false now, can it be moved into > else branch below? > It can. I expect the saving to be marginal and it will need to move back when/if select_idle_core() obeys SIS_PROP. > > - 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; > > What if the last idle core is selected here, should we set_idle_cores false > before return? > We'd have to check what bits were still set in the cpus mask and determine if they represent an idle core. I severely doubt it would be worth the cost given that the availability of idle cores can change at any instant. -- Mel Gorman SUSE Labs