linux-kernel - Re: [patch, rfc: 1/2] sched, hotplug: safe use of rq->migration_thread and find_busiest

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:	Fri, 25 Jul 2008 13:52:17 +0200
From:	"Dmitry Adamushko" <dmitry.adamushko@...il.com>
To:	"Peter Zijlstra" <a.p.zijlstra@...llo.nl>
Cc:	"Ingo Molnar" <mingo@...e.hu>, LKML <linux-kernel@...r.kernel.org>,
	"Steven Rostedt" <rostedt@...dmis.org>,
	"Thomas Gleixner" <tglx@...utronix.de>
Subject: Re: [patch, rfc: 1/2] sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()

2008/7/25 Peter Zijlstra <a.p.zijlstra@...llo.nl>:
> On Fri, 2008-07-25 at 00:11 +0200, Dmitry Adamushko wrote:
>> From: Dmitry Adamushko <dmitry.adamushko@...il.com>
>> Subject: sched, hotplug: safe use of rq->migration_thread
>> and find_busiest_queue()
>>
>> ---
>>
>>     sched, hotplug: safe use of rq->migration_thread and find_busiest_queue()
>>
>>     (1) make usre rq->migration_thread is valid when we access it in set_cpus_allowed_ptr()
>>     after releasing the rq-lock;
>>
>>     (2) in load_balance() and load_balance_idle()
>>
>>     ensure that we don't get 'busiest' which can disappear as a result of cpu_down()
>>     while we are manipulating it. For this goal, we choose 'busiest' only amongst
>>     'cpu_active_map' cpus.
>>
>>     load_balance() and load_balance_idle() get called with preemption being disabled
>>     so synchronize_sched() in cpu_down() should get us synced.
>>
>>     IOW, as soon as synchronize_sched() has been done in cpu_down(cpu), the run-queue for
>>     can't be manipulated/accessed by the load-balancer.
>>
>>     Signed-off-by: Dmitry Adamushko <dmitry.adamushko@...il.com>
>
> Acked-by: Peter Zijlstra <a.p.zijlstra@...llo.nl>

Thanks.

>
>> diff --git a/kernel/sched.c b/kernel/sched.c
>> index 6acf749..b4ccc8b 100644
>> --- a/kernel/sched.c
>> +++ b/kernel/sched.c
>> @@ -3409,7 +3409,14 @@ static int load_balance(int this_cpu, struct rq *this_rq,
>>       struct rq *busiest;
>>       unsigned long flags;
>>
>> -     cpus_setall(*cpus);
>> +     /*
>> +      * Ensure that we don't get 'busiest' which can disappear
>> +      * as a result of cpu_down() while we are manipulating it.
>> +      *
>> +      * load_balance() gets called with preemption being disabled
>> +      * so synchronize_sched() in cpu_down() should get us synced.
>> +      */
>> +     *cpus = cpu_active_map;
>
> This is going to be painful on -rt... there it can be preempted. I guess
> we can put get_online_cpus() around it or something..

I've considered using get_online_cpus() for a moment but dropped this
idea exactly because I thought it would harm us latency-wise.
cpu_down() and cpu_up() may take quite a long time to complete and
load_balance() && load_balance_idle() would need to wait all this
time. And they both are kind of generic (primary) scheduler
operations.

but yea, my scheme relies on the fact that load_balance() &&
load_balance_idle() are atomic one way or another wrt. cpu_clear() +
synchronize_sched() in cpu_down().

[ speculating here ] I'd rather add an additional mechanism which
would be light-weight for load_balance() and add
synch_this_mechanism() (alike to synchonise_sched()) in cpu_down() as
perhaps we don't care that much on how fast the later one is.


-- 
Best regards,
Dmitry Adamushko
--
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/