Re: [RFC PATCH] cpuset: fix cpuset_cpus_allowed_fallback() don't update tsk->rt.nr_cpus_allowed

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Mon, May 02, 2011 at 07:42:40PM +0900, KOSAKI Motohiro wrote:
> > On 04/28, KOSAKI Motohiro wrote:
> > >
> > > Oleg, Peter,
> > >
> > > I apologize if I misunderstand a code.
> > 
> > Heh, I bet you understand it better than me ;)
> > 
> > > index f20eb8f..42dcbdc 100644
> > > --- a/include/linux/cpuset.h
> > > +++ b/include/linux/cpuset.h
> > > @@ -147,6 +147,7 @@ static inline void cpuset_cpus_allowed(struct task_struct *p,
> > >  static inline int cpuset_cpus_allowed_fallback(struct task_struct *p)
> > >  {
> > >  	cpumask_copy(&p->cpus_allowed, cpu_possible_mask);
> > > +	p->rt.nr_cpus_allowed = cpumask_weight(&p->cpus_allowed);
> > >  	return cpumask_any(cpu_active_mask);
> > >  }
> > >
> > > diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> > > index 1ceeb04..6e5bbe8 100644
> > > --- a/kernel/cpuset.c
> > > +++ b/kernel/cpuset.c
> > > @@ -2220,6 +2220,7 @@ int cpuset_cpus_allowed_fallback(struct task_struct *tsk)
> > >  		cpumask_copy(&tsk->cpus_allowed, cpu_possible_mask);
> > >  		cpu = cpumask_any(cpu_active_mask;
> > >  	}
> > > +	tsk->rt.nr_cpus_allowed = cpumask_weight(&tsk->cpus_allowed);
> > 
> > I think you are right...
> > 
> > But, don't we need sched_class->set_cpus_allowed() in this case? Only for
> > consistency, iiuc currently it is not needed because the task is not active.
> > 
> > IOW, perhaps cpuset_cpus_allowed_fallback() should do
> > 
> > 
> > 	if (p->sched_class->set_cpus_allowed)
> > 		p->sched_class->set_cpus_allowed(p, cpu_possible_mask);
> > 	else {
> > 		cpumask_copy(&p->cpus_allowed, cpu_possible_mask);
> > 		p->rt.nr_cpus_allowed = cpumask_weight(cpu_possible_mask);
> > 	}
> > 
> > ?
> > 
> > If yes, probably the new do_set_cpus_allowed(p, mask) helper makes sense,
> > it can be used by set_cpus_allowed_ptr() too.
> 
> Absolutely. I have very similar patch. but I though we should keep them
> separated. The reasons are two.
> 
> 1. To keep simple one line patch may help to reduce a backport guy's headache.
> 2. now we have 6 tsk->cpu_allowed writer.
> 
> 1) sched_rt.c: set_cpus_allowed_rt()		no problem. it's shceduler.
> 2) sched.c: set_cpus_allowed_ptr()		ditto.
> 3) sched.c: init_idle()				no lock, but no competitor. that's init.
> 4) kthread.c kthread_bind()			no lock, but no competitor. kthread haven't started yet
> 5) cpuset.c: cpuset_cpus_allowed_fallback() 	p->pi_lock held
> 6) arch/bfin/../process.c: bfin_clone()		crazy. but I alread sent a patch to blackfin folks.
> 
> ok, (2)-(5) can use do_set_cpus_allowed(). but It slightly large refactoring.
> 
> But, Hmmm...
> I've found my patch have one issue. Sigh. Recently RCU sub system introduced
> rcuc FIFO kthread and it start to run before secondary cpu is up. then,
> cpuset_cpus_allowed_fallback set its cpus_allowed to cpu_possible_mask and
> kernel will crash.

If there is something different that I should be doing with the rcuc FIFO
kthread, please do let me know!

							Thanx, Paul

> Will fix it too.
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/