linux-kernel - Re: [PATCH] sched: fix env->src

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] [day] [month] [year] [list]

Message-ID: <CAKfTPtAS_-sDiNr=putbZaX1+Fg795yrU06LgXtgfXuT3wn-+w@mail.gmail.com>
Date:	Thu, 14 Feb 2013 14:37:06 +0100
From:	Vincent Guittot <vincent.guittot@...aro.org>
To:	Paul Turner <pjt@...gle.com>
Cc:	linux-kernel@...r.kernel.org, linaro-dev@...ts.linaro.org,
	peterz@...radead.org, mingo@...nel.org
Subject: Re: [PATCH] sched: fix env->src_cpu for active migration

On 13 February 2013 21:03, Paul Turner <pjt@...gle.com> wrote:
> On Tue, Feb 12, 2013 at 5:19 AM, Vincent Guittot
> <vincent.guittot@...aro.org> wrote:
>> need_active_balance uses env->src_cpu which is set only if there is more
>> than 1 task on the run queue.
>> We must set the src_cpu field unconditionnally
>> otherwise the test "env->src_cpu > env->dst_cpu" will always fail if there is
>> only 1 task on the run queue
>>
>> Signed-off-by: Vincent Guittot <vincent.guittot@...aro.org>
>> ---
>>  kernel/sched/fair.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 81fa536..32938ea 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -5044,6 +5044,10 @@ redo:
>>
>
> Aside:  Oh dear, it looks like in all the re-factoring here
> update_h_load() has escaped rq->lock?
>
> load_balance()
> ...
>                 update_h_load(env.src_cpu);
> more_balance:
>                 local_irq_save(flags);
>                 double_rq_lock(env.dst_rq, busiest);
>
>
>
>>         ld_moved = 0;
>>         lb_iterations = 1;
>> +
>> +       env.src_cpu   = busiest->cpu;
>> +       env.src_rq    = busiest;
>> +
>
> Hmm -- But shouldn't find_busiest_queue return NULL in this case?
>
> We're admittedly racing but we should have:
>   busiest->nr_running == 1
>   wl = rq->load.weight > env->imbalance since imbalance < (wl -
> this_rq->load.weight / 2)

check_asym_packing overwrites the env->imbalance with (sds->max_load *
sds->busiest->sgp->power / SCHED_POWER_SCALE) so fbq can return a
queue

>
> This would seem specialized to the case when we either
>   A) race (fbq is openly racy)
>   B) have no capacity
>
> Admittedly when we do race current case this is probably a biased
> coinflip depending on whatever was on the stack (src_cpu is
> uninitialized); it's also super easy for this to later become a crash
> if someone tries to dereference src_rq so we should do something.
>
> The case this seems most important for (and something we should add an
> explicit comment regarding) is that we want this case specifically for
> CPU_NEW_IDLE to move a single runnable-task to a lower numbered
> idle-cpu index in the SD_ASYM case (although I suspect we need to push
> this up to fbq also to actually find it...)

The update of imbalance should be enough

>
> In the !SD_ASYM case we'd probably also want to  re-check busiest
> nr_running in need_active_balance (or probably better alternative
> re-arrange the checks) since this is going to potentially now move a
> single large task needlessly to an already loaded rq in balance-failed
> case.

yes, that could be an interesting add-on

>
>
>>         if (busiest->nr_running > 1) {
>>                 /*
>>                  * Attempt to move tasks. If find_busiest_group has found
>> @@ -5052,8 +5056,6 @@ redo:
>>                  * correctly treated as an imbalance.
>>                  */
>>                 env.flags |= LBF_ALL_PINNED;
>> -               env.src_cpu   = busiest->cpu;
>> -               env.src_rq    = busiest;
>>                 env.loop_max  = min(sysctl_sched_nr_migrate, busiest->nr_running);
>>
>>                 update_h_load(env.src_cpu);
>> --
>> 1.7.9.5
>>
>> --
>> 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/
--
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/