Re: [PATCH 1/1] hotplug cpu: migrate a task within its cpuset

[Oleg Nesterov <oleg@tv-sign.ru>]

On 05/23, Cliff Wickman wrote:
> 
> On Thu, May 24, 2007 at 01:29:02AM +0400, Oleg Nesterov wrote:
> > Cliff Wickman wrote:
> > >
> > > - * NOTE: interrupts should be disabled by the caller
> > > + * NOTE: interrupts are not disabled by the caller
> > >   */
> > >  static void move_task_off_dead_cpu(int dead_cpu, struct task_struct *p)
> > >  {
> > > @@ -5008,6 +5008,17 @@ restart:
> > >  	if (dest_cpu == NR_CPUS)
> > >  		dest_cpu = any_online_cpu(p->cpus_allowed);
> > >
> > > +	/* try to stay on the same cpuset */
> > > +	if (dest_cpu == NR_CPUS) {
> > > +		/*
> > > +		 * Call to cpuset_cpus_allowed may sleep, so we depend
> > > +		 * on move_task_off_dead_cpu() being called in a non-critical
> > > +		 * region.
> > > +		 */
> > > +		p->cpus_allowed = cpuset_cpus_allowed(p);
> > > +		dest_cpu = any_online_cpu(p->cpus_allowed);
> > > +	}
> > 
> > I know nothing about cpuset.c, a _very_ naive question.
> 
> Paul Jackson is the cpuset guru.

Hopefully Paul can help us...

> > Do we really need task_lock() (used by cpuset_cpus_allowed) here ?
> 
> According to Paul's comment in kernel/cpuset.c
>  * It is ok to first take manage_sem, then nest callback_sem.  We also
>  * require taking task_lock() when dereferencing a tasks cpuset pointer.
> So I'm afraid it is not safe to call guarantee_online_cpus(tsk->cpuset, &mask);
> without it.  Could the task not be exiting?

But how task_lock() can help? cpuset_exit() doesn't take it, it changes ->cpuset
lockless. However, it sets ->cpuset = &top_cpuset, and the comment says:

	* Don't even think about derefencing 'cs' after the cpuset use count
	* goes to zero, except inside a critical section guarded by manage_mutex
	* or callback_mutex.
	     ^^^^^^^^^^^^^^
So, perhaps cpuset_lock() should be enough.

(That said, looking at cpuset_exit() I can't understand why callback_mutex is
 enough. If it is not, cpuset_cpus_allowed() is not safe either).

> > If not, probably we can make this simpler. CPU_DEAD takes cpuset_lock(),
> > move_task_off_dead_cpu() uses guarantee_online_cpus() which doesn't sleep,
> > so we don't need other changes.
> > 
> > Possible?
> > 
> > If not, this patch should also change migrate_dead(), it still calls
> > move_task_off_dead_cpu() with irqs disabled, no?
> 
> Right, the lock is released but I indeed didn't reenable irqs.
> How would you suggest doing that?  The irq state was saved in local
> variable "flags" back in migration_call().

migration_call(CPU_DEAD) is called with irqs enabled.

Oleg.