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 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.

---

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().




>
>>     Signed-off-by: Dmitry Adamushko <dmitry.adamushko@gmail.com>
>>
>> diff --git a/kernel/sched.c b/kernel/sched.c
>> index b4ccc8b..c3bd78a 100644
>> --- a/kernel/sched.c
>> +++ b/kernel/sched.c
>> @@ -5774,21 +5774,23 @@ int set_cpus_allowed_ptr(struct task_struct *p, const cpumask_t *new_mask)
>>       }
>>
>>       /* Can the task run on the task's current CPU? If so, we're done */
>> -     if (cpu_isset(task_cpu(p), *new_mask))
>> -             goto out;
>> +     while (!cpu_isset(task_cpu(p), p->cpus_allowed)) {
>> +             int cpu = any_online_cpu(p->cpus_allowed);
>>
>> -     if (migrate_task(p, any_online_cpu(*new_mask), &req)) {
>> -             /* Need to wait for migration thread (might exit: take ref). */
>> -             struct task_struct *mt = rq->migration_thread;
>> +             if (migrate_task(p, cpu, &req)) {
>> +                     /* Need to wait for migration thread (might exit: take ref). */
>> +                     struct task_struct *mt = rq->migration_thread;
>>
>> -             get_task_struct(mt);
>> -             task_rq_unlock(rq, &flags);
>> -             wake_up_process(mt);
>> -             put_task_struct(mt);
>> +                     get_task_struct(mt);
>> +                     task_rq_unlock(rq, &flags);
>> +                     wake_up_process(mt);
>> +                     put_task_struct(mt);
>>
>> -             wait_for_completion(&req.done);
>> -             tlb_migrate_finish(p->mm);
>> -             return 0;
>> +                     wait_for_completion(&req.done);
>> +                     tlb_migrate_finish(p->mm);
>> +
>> +                     rq = task_rq_lock(p, &flags);
>> +             }
>>       }
>>  out:
>>       task_rq_unlock(rq, &flags);
>>
>>
>
>



-- 
Best regards,
Dmitry Adamushko