Re: [patch, rfc: 2/2] sched, hotplug: ensure a task is on the valid cpu after set_cpus_allowed_ptr()

["Dmitry Adamushko" <dmitry.adamushko@gmail.com>]

2008/7/25 Peter Zijlstra <a.p.zijlstra@chello.nl>:
> On Fri, 2008-07-25 at 15:20 +0200, Dmitry Adamushko wrote:
>> 2008/7/25 Peter Zijlstra <a.p.zijlstra@chello.nl>:
>> > On Fri, 2008-07-25 at 00:15 +0200, Dmitry Adamushko wrote:
>> >>
>> >> From: Dmitry Adamushko <dmitry.adamushko@gmail.com>
>> >> Subject: sched, hotplug: ensure a task is on the valid cpu after
>> >> set_cpus_allowed_ptr()
>> >>
>> >> ---
>> >>     sched, hotplug: ensure a task is on the valid cpu after set_cpus_allowed_ptr()
>> >>
>> >>     The 'new_mask' may not include task_cpu(p) so we migrate 'p' on another 'cpu'.
>> >>     In case it can't be placed on this 'cpu' immediately, we submit a request
>> >>     to the migration thread and wait for its completion.
>> >>
>> >>     Now, by the moment this request gets handled by the migration_thread,
>> >>     'cpu' may well be offline/non-active. As a result, 'p' continues
>> >>     running on its old cpu which is not in the 'new_mask'.
>> >>
>> >>     Fix it: ensure 'p' ends up on a valid cpu.
>> >>
>> >>     Theoreticaly (but unlikely), we may get an endless loop if someone cpu_down()'s
>> >>     a new cpu we have choosen on each iteration.
>> >>
>> >>     Alternatively, we may introduce a special type of request to migration_thread,
>> >>     namely "move_to_any_allowed_cpu" (e.g. by specifying dest_cpu == -1).
>> >>
>> >>     Note, any_active_cpu() instead of any_online_cpu() would be better here.
>> >
>> > Hrmm,.. this is all growing into something of a mess.. defeating the
>> > whole purpose of introducing that cpu_active_map stuff.
>> >
>> > Would the suggested SRCU logic simplify all this?
>>
>> Ah, wait a second.
>>
>> sched_setaffinity() -> set_cpus_allowed_ptr() is ok vs. cpu_down() as
>> it does use get_online_cpus(). So none of the cpus can become offline
>> while we are in set_cpus_allowed_ptr().
>>
>> but there are numerous calls to set_cpus_allowed_ptr() from other
>> places and not all of them seem to call get_online_cpus()...
>>
>> yeah, I should check this issue again..
>>
>> btw., indeed all these different sync. cases are a bit of mess.
>
> Will ponder it a bit more, but my brain can't seem to let go of SRCU
> now..

I like it too.

> I'll go concentrate on making the swap-over-nfs patches prettier,
> maybe that will induce a brainwave ;-)

what's about task-migration over NFS? ;-)


>> btw., I was wondering about this change:
>>
>> ba42059fbd0aa1ac91b582412b5fedb1258f241f
>>
>> sched: hrtick_enabled() should use cpu_active()
>>
>> Peter pointed out that hrtick_enabled() should use cpu_active().
>
> What exactly were you wondering about?
>
> It seemed a good idea to stop starting hrtimers before we migrate them
> to another cpu (one of the things done later in cpu_down), thereby
> avoiding spurious fires on remote cpus.
>

Yeah, I thought that it's likely cpu_down() related.

I looked at it from the point of cpu_up(), e.g. a cpu is online ->
tasks get queued and start running (while cpu is still _not_ active
for a while). So when they get enqueued first time, hrtick_enabled()
wil give 0 and hr-timer won't be used.

Actually, cpu_active_map has already broken expectations/assumptions -
http://lkml.org/lkml/2008/7/24/260 (in case you have missed it). But
this particular "microcode"s behavior is really bad, I think.


-- 
Best regards,
Dmitry Adamushko