On 28 May 2014 17:09, Dietmar Eggemann <dietmar.eggem...@arm.com> wrote: > Hi Vincent & Peter, > > On 28/05/14 07:49, Vincent Guittot wrote: > [...] >> >> Nick, >> >> While doing some rework on the wake affine part of the scheduler, i >> failed to catch the use case that takes advantage of a condition that >> you added some while ago with the commit >> a3f21bce1fefdf92a4d1705e888d390b10f3ac6f >> >> Could you help us to clarify the 2 first lines of the test that you added ? >> + if ((tl <= load && >> + tl + target_load(cpu, idx) <= >> SCHED_LOAD_SCALE) || >> + 100*(tl + SCHED_LOAD_SCALE) <= >> imbalance*load) { >> >> Regards, >> Vincent >>> >>>> >>>>>> commit a3f21bce1fefdf92a4d1705e888d390b10f3ac6f >>>>>> >>>>>> >>>>>> + if ((tl <= load && >>>>>> + tl + target_load(cpu, idx) <= >>>>>> SCHED_LOAD_SCALE) || >>>>>> + 100*(tl + SCHED_LOAD_SCALE) <= >>>>>> imbalance*load) { >>>>>> >>>>>> >>>>>> So back when the code got introduced, it read: >>>>>> >>>>>> target_load(prev_cpu, idx) - sync*SCHED_LOAD_SCALE < >>>>>> source_load(this_cpu, idx) && >>>>>> target_load(prev_cpu, idx) - sync*SCHED_LOAD_SCALE + >>>>>> target_load(this_cpu, idx) < SCHED_LOAD_SCALE >>>>>> > > Shouldn't this be > > target_load(this_cpu, idx) - sync*SCHED_LOAD_SCALE <= source_load(prev_cpu, > idx) && > target_load(this_cpu, idx) - sync*SCHED_LOAD_SCALE + target_load(prev_cpu, > idx) <= SCHED_LOAD_SCALE
yes, there was a typo mistake in Peter's explanation > > "[PATCH] sched: implement smpnice" (2dd73a4f09beacadde827a032cf15fd8b1fa3d48) > mentions that SCHED_LOAD_BALANCE (IMHO, should be SCHED_LOAD_SCALE) represents > the load contribution of a single task. So I read the second part as if > the sum of the load of this_cpu and prev_cpu is smaller or equal to the > (maximal) load contribution (maximal possible effect) of a single task. > > There is even a comment in "[PATCH] sched: tweak affine wakeups" > (a3f21bce1fefdf92a4d1705e888d390b10f3ac6f) in try_to_wake_up() when > SCHED_LOAD_SCALE gets subtracted from tl = this_load = > target_load(this_cpu, idx): > > + * If sync wakeup then subtract the (maximum possible) > + * effect of the currently running task from the load > + * of the current CPU: > > "[PATCH] sched: implement smpnice" then replaces SCHED_LOAD_SCALE w/ > > +static inline unsigned long cpu_avg_load_per_task(int cpu) > +{ > + runqueue_t *rq = cpu_rq(cpu); > + unsigned long n = rq->nr_running; > + > + return n ? rq->raw_weighted_load / n : SCHED_LOAD_SCALE; > > -- Dietmar > >>>>>> So while the first line makes some sense, the second line is still >>>>>> somewhat challenging. >>>>>> >>>>>> I read the second line something like: if there's less than one full >>>>>> task running on the combined cpus. >>>>> >>>>> ok. your explanation makes sense >>>> >>>> Maybe, its still slightly weird :-) >>>> >>>>>> > [...] > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/