linux-kernel - Re: [PATCH v2 02/11] sched: remove a wake

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 28 May 2014 17:25:38 +0200
From:	Vincent Guittot <vincent.guittot@...aro.org>
To:	Dietmar Eggemann <dietmar.eggemann@....com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	"npiggin@...il.com" <npiggin@...il.com>,
	Ingo Molnar <mingo@...nel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	LAK <linux-arm-kernel@...ts.infradead.org>,
	Preeti U Murthy <preeti@...ux.vnet.ibm.com>,
	Morten Rasmussen <Morten.Rasmussen@....com>,
	Mike Galbraith <efault@....de>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	"linaro-kernel@...ts.linaro.org" <linaro-kernel@...ts.linaro.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>
Subject: Re: [PATCH v2 02/11] sched: remove a wake_affine condition

On 28 May 2014 17:09, Dietmar Eggemann <dietmar.eggemann@....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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/