Re: [RFC][PATCH -mm 1/5] PM: Make freeze_processes SMP-safe

["Rafael J. Wysocki" <rjw@sisk.pl>]

Hi,

On Sunday, 26 November 2006 20:48, Pavel Machek wrote:
> Hi!
> 
> > Index: linux-2.6.19-rc6-mm1/kernel/power/process.c
> > ===================================================================
> > --- linux-2.6.19-rc6-mm1.orig/kernel/power/process.c	2006-11-25 21:26:52.000000000 +0100
> > +++ linux-2.6.19-rc6-mm1/kernel/power/process.c	2006-11-26 14:17:11.000000000 +0100
> > @@ -28,8 +28,7 @@ static inline int freezeable(struct task
> >  	if ((p == current) || 
> >  	    (p->flags & PF_NOFREEZE) ||
> >  	    (p->exit_state == EXIT_ZOMBIE) ||
> > -	    (p->exit_state == EXIT_DEAD) ||
> > -	    (p->state == TASK_STOPPED))
> > +	    (p->exit_state == EXIT_DEAD))
> >  		return 0;
> >  	return 1;
> >  }
> > @@ -61,10 +60,13 @@ static inline void freeze_process(struct
> >  	unsigned long flags;
> >  
> >  	if (!freezing(p)) {
> > -		freeze(p);
> > -		spin_lock_irqsave(&p->sighand->siglock, flags);
> > -		signal_wake_up(p, 0);
> > -		spin_unlock_irqrestore(&p->sighand->siglock, flags);
> > +		rmb();
> 
> If frozen is atomic_t, do we need memory barrier?

I think so.  For example on x86-64 atomic_read() is just a read.

> > +		if (!frozen(p)) {
> > +			freeze(p);
> > +			spin_lock_irqsave(&p->sighand->siglock, flags);
> > +			signal_wake_up(p, 0);
> > +			spin_unlock_irqrestore(&p->sighand->siglock, flags);
> > +		}
> >  	}
> >  }
> >  
> > @@ -90,11 +92,12 @@ static unsigned int try_to_freeze_tasks(
> >  {
> >  	struct task_struct *g, *p;
> >  	unsigned long end_time;
> > -	unsigned int todo;
> > +	unsigned int todo, nr_stopped;
> >  
> >  	end_time = jiffies + TIMEOUT;
> >  	do {
> >  		todo = 0;
> > +		nr_stopped = 0;
> >  		read_lock(&tasklist_lock);
> >  		do_each_thread(g, p) {
> >  			if (!freezeable(p))
> > @@ -103,6 +106,10 @@ static unsigned int try_to_freeze_tasks(
> >  			if (frozen(p))
> >  				continue;
> >  
> > +			if (p->state == TASK_STOPPED) {
> > +				nr_stopped++;
> > +				continue;
> > +			}
> >  			if (p->state == TASK_TRACED &&
> >  			    (frozen(p->parent) ||
> >  			     p->parent->state == TASK_STOPPED)) {
> > @@ -128,6 +135,21 @@ static unsigned int try_to_freeze_tasks(
> >  		} while_each_thread(g, p);
> >  		read_unlock(&tasklist_lock);
> >  		yield();			/* Yield is okay here */
> > +		if (!todo) {
> > +			/* Make sure that none of the stopped processes has
> > +			 * received the continuation signal after we checked
> > +			 * last time.
> > +			 */
> 
> I do not like the counting idea; it should be simpler to just check if
> all the processes are still stopped.

I thought about that but didn't invent anything reasonable enough.

> But I'm not sure if this is enough. What if signal is being delivered
> on another CPU while freezing, still being delivered while this second
> check runs, and then SIGCONT is delivered? 

Hm, is this possible in practice?  I mean, if todo is 0 and nr_stopped doesn't
change, then there are no processes that can send the SIGCONT (unless someone
creates a kernel thread with PF_NOFREEZE that will do just that).

Anyway, for now I've no idea how to fix this properly.  Will think about it
tomorrow.

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV