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

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

Hi,

On Sunday, 26 November 2006 08:47, Pavel Machek wrote:
> Hi!
> 
> > Currently, the PF_FREEZE process flag is used to indicate that the process
> > should enter the refrigerator as soon as possible.  Unfortunately it is set by
> > the freezer while the process may be changing its flags for another reason
> > and this may lead to a race between the freezer and the process itself.
> > 
> > This problem may be solved by introducing an additional member, called (for
> > example) 'freezing', into task_struct which will only be used to indicate that
> > the process should enter the refrigerator.  Then, if the 'freezing' member of
> > task_struct is reset by the process itself only after it has entered the
> > refrigerator, the modifications of it will be guaranteed to occur at different
> > times, because the freezer can only set it before the process enters the
> > refrigerator.  Thus the code will be SMP-safe even though no explicit locking
> > is used.
> 
> I do not think we can go without locking here.

Why exactly?

> > @@ -31,7 +30,7 @@ static inline void freeze(struct task_st
> >   */
> >  static inline void do_not_freeze(struct task_struct *p)
> >  {
> > -	p->flags &= ~PF_FREEZE;
> > +	p->freezing = 0;
> >  }
> >  
> >  /*
> > @@ -52,7 +51,8 @@ static inline int thaw_process(struct ta
> >   */
> >  static inline void frozen_process(struct task_struct *p)
> >  {
> > -	p->flags = (p->flags & ~PF_FREEZE) | PF_FROZEN;
> > +	p->flags |= PF_FROZEN;
> > +	p->freezing = 0;
> >  }
> 
> Is mb() needed between |= and freezing = 0?

Hm, I'm not sure.  Are there architectures on which memory writes can be
reordered?
 
> >  extern void refrigerator(void);
> > Index: linux-2.6.19-rc6-mm1/include/linux/sched.h
> > ===================================================================
> > --- linux-2.6.19-rc6-mm1.orig/include/linux/sched.h
> > +++ linux-2.6.19-rc6-mm1/include/linux/sched.h
> > @@ -1065,6 +1065,9 @@ struct task_struct {
> >  #ifdef	CONFIG_TASK_DELAY_ACCT
> >  	struct task_delay_info *delays;
> >  #endif
> > +#ifdef CONFIG_PM
> > +	int freezing;		/* if set, we should be freezing for suspend */
> > +#endif
> >  #ifdef CONFIG_FAULT_INJECTION
> >  	int make_it_fail;
> >  #endif
> 
> It is int, imagine machine that can't do 32-bit atomic access (only
> does 64 bits). On such beast (alpha? something stranger?) this will
> clobber make_it_fail field, sometimes.
> 
> OTOH on i386 normal instructions can be used. But that's okay, we
> should just use atomic_t here. Should be as fast on i386/x86-64, and
> still safe.

Okay, I'll use atomic_t.

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