Messages in this thread | | | Subject | Re: [patch v2 1/2] sched: check for prev_cpu == this_cpu before calling wake_affine() | From | Peter Zijlstra <> | Date | Tue, 20 Apr 2010 10:46:52 +0200 |
| |
On Wed, 2010-03-31 at 16:47 -0700, Suresh Siddha wrote:
> From: Suresh Siddha <suresh.b.siddha@intel.com> > Subject: sched: fix select_idle_sibling() logic in select_task_rq_fair() > > Issues in the current select_idle_sibling() logic in select_task_rq_fair() > in the context of a task wake-up: > > a) Once we select the idle sibling, we use that domain (spanning the cpu that > the task is currently woken-up and the idle sibling that we found) in our > wake_affine() decisions. This domain is completely different from the > domain(we are supposed to use) that spans the cpu that the task currently > woken-up and the cpu where the task previously ran. > > b) We do select_idle_sibling() check only for the cpu that the task is > currently woken-up on. If select_task_rq_fair() selects the previously run > cpu for waking the task, doing a select_idle_sibling() check > for that cpu also helps and we don't do this currently. > > c) In the scenarios where the cpu that the task is woken-up is busy but > with its HT siblings are idle, we are selecting the task be woken-up > on the idle HT sibling instead of a core that it previously ran > and currently completely idle. i.e., we are not taking decisions based on > wake_affine() but directly selecting an idle sibling that can cause > an imbalance at the SMT/MC level which will be later corrected by the > periodic load balancer. > > Fix this by first going through the load imbalance calculations using > wake_affine() and once we make a decision of woken-up cpu vs previously-ran cpu, > then choose a possible idle sibling for waking up the task on. > > Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
OK, so I'm going to take this, but I had one concern, see below:
> diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c > index 49ad993..f905a4b 100644 > --- a/kernel/sched_fair.c > +++ b/kernel/sched_fair.c > @@ -1385,28 +1385,48 @@ find_idlest_cpu(struct sched_group *group, struct task_struct *p, int this_cpu) > * Try and locate an idle CPU in the sched_domain. > */ > static int > +select_idle_sibling(struct task_struct *p, int target) > { > int cpu = smp_processor_id(); > int prev_cpu = task_cpu(p); > int i; > + struct sched_domain *sd; > > /* > + * If the task is going to be woken-up on this cpu and if it is > + * already idle, then it is the right target. > */ > + if (target == cpu && !cpu_rq(cpu)->cfs.nr_running) > + return cpu; > + > + /* > + * If the task is going to be woken-up on the cpu where it previously > + * ran and if it is currently idle, then it the right target. > + */ > + if (target == prev_cpu && !cpu_rq(prev_cpu)->cfs.nr_running) > return prev_cpu; > > /* > + * Otherwise, iterate the domains and find an elegible idle cpu. > */ > + for_each_domain(target, sd) { > + if (!(sd->flags & SD_SHARE_PKG_RESOURCES)) > break; > + > + for_each_cpu_and(i, sched_domain_span(sd), &p->cpus_allowed) { > + if (!cpu_rq(i)->cfs.nr_running) { > + target = i; > + break; > + } > } > + > + /* > + * Lets stop looking for an idle sibling when we reached > + * the domain that spans the current cpu and prev_cpu. > + */ > + if (cpumask_test_cpu(cpu, sched_domain_span(sd)) && > + cpumask_test_cpu(prev_cpu, sched_domain_span(sd))) > + break; > } > > return target;
So here we keep using !cfs.nr_running to mean idle, which might not at all be true when there's real-time tasks around.
So should we be using idle_cpu(i) instead?
> @@ -1429,7 +1449,7 @@ static int select_task_rq_fair(struct task_struct *p, int sd_flag, int wake_flag > int cpu = smp_processor_id(); > int prev_cpu = task_cpu(p); > int new_cpu = cpu; > + int want_affine = 0; > int want_sd = 1; > int sync = wake_flags & WF_SYNC; > > @@ -1467,36 +1487,15 @@ static int select_task_rq_fair(struct task_struct *p, int sd_flag, int wake_flag > want_sd = 0; > } > > if (want_affine) { > /* > * If both cpu and prev_cpu are part of this domain, > * cpu is a valid SD_WAKE_AFFINE target. > */ > + if (cpumask_test_cpu(prev_cpu, sched_domain_span(tmp)) > + && (tmp->flags & SD_WAKE_AFFINE)) { > + affine_sd = tmp; > + want_affine = 0; > } > } > > @@ -1527,8 +1526,10 @@ static int select_task_rq_fair(struct task_struct *p, int sd_flag, int wake_flag > #endif > > if (affine_sd) { > + if (cpu == prev_cpu || wake_affine(affine_sd, p, sync)) > + return select_idle_sibling(p, cpu); > + else > + return select_idle_sibling(p, prev_cpu); > } > > while (sd) { > >
| |