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

[Pavel Machek <pavel@ucw.cz>]

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.

> @@ -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?

>  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.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

-------------------------------------------------------------------------
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