[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20080725224100.GB16433@in.ibm.com>
Date: Fri, 25 Jul 2008 18:41:00 -0400
From: Gautham R Shenoy <ego@...ibm.com>
To: Dmitry Adamushko <dmitry.adamushko@...il.com>
Cc: Ingo Molnar <mingo@...e.hu>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [patch, rfc: 2/2] sched, hotplug: ensure a task is on the
valid cpu after set_cpus_allowed_ptr()
On Fri, Jul 25, 2008 at 12:15:30AM +0200, Dmitry Adamushko wrote:
>
> From: Dmitry Adamushko <dmitry.adamushko@...il.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.
Hi Dmitry,
Wasn't set_cpus_allowed_ptr() created from set_cpus_allowed() code
path ?
that means this bug (if it really is one....) has been around for quite
some time now!!
>
> 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.
>
> Signed-off-by: Dmitry Adamushko <dmitry.adamushko@...il.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);
>
>
> --
> 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/
>
--
Thanks and Regards
gautham
--
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/
Powered by blists - more mailing lists