[RFC][PATCH -mm] PM: Change ordering of suspend and resume code

[RFC][PATCH -mm] PM: Change ordering of suspend and resume code

[Rafael J. Wysocki]

Hi,

The discussion in a recent thread on Linux-PM has indicated that it's
necessary to call pm_ops->finish() before devce_resume(),
but enable_nonboot_cpus() has to be called before pm_ops->finish()
(cf. http://lists.osdl.org/pipermail/linux-pm/2006-November/004164.html).
For consistency, it seems reasonable to call disable_nonboot_cpus() after
device_suspend().

This way the suspend code will remain symmetrical with respect to the resume
code and it may allow us to speed up things in the future by suspending and
resuming devices and/or saving the suspend image in many threads, but for this
purpose we first need to make freeze_processes() SMP-safe.

The following series of patches makes freeze_processes() SMP-safe and
reorders the suspend and resume code so that nonboot CPUs are disabled
after devices have been suspended and enabled before the devices are
resumed.  It also causes pm_ops->finish() to be called after
enable_nonboot_cpus() wherever necessary.

The first four patches have been tested on two different x86-64 SMP boxes and
they don't seem to break anything.  Still, if anyone can test them on some other
SMP boxes (especially on i386 ones), please do so and tell me if there are
any problems.

The last patch is untested.

All of the patches are against 2.6.19-rc6-mm1.

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

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

[Rafael J. Wysocki]

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.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/freezer.h |   10 +++++-----
 include/linux/sched.h   |    4 +++-
 2 files changed, 8 insertions(+), 6 deletions(-)

Index: linux-2.6.19-rc6-mm1/include/linux/freezer.h
===================================================================
--- linux-2.6.19-rc6-mm1.orig/include/linux/freezer.h
+++ linux-2.6.19-rc6-mm1/include/linux/freezer.h
@@ -14,16 +14,15 @@ static inline int frozen(struct task_str
  */
 static inline int freezing(struct task_struct *p)
 {
-	return p->flags & PF_FREEZE;
+	return !!p->freezing;
 }
 
 /*
  * Request that a process be frozen
- * FIXME: SMP problem. We may not modify other process' flags!
  */
 static inline void freeze(struct task_struct *p)
 {
-	p->flags |= PF_FREEZE;
+	p->freezing = 1;
 }
 
 /*
@@ -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;
 }
 
 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
@@ -1161,7 +1164,6 @@ static inline void put_task_struct(struc
 #define PF_MEMALLOC	0x00000800	/* Allocating memory */
 #define PF_FLUSHER	0x00001000	/* responsible for disk writeback */
 #define PF_USED_MATH	0x00002000	/* if unset the fpu must be initialized before use */
-#define PF_FREEZE	0x00004000	/* this task is being frozen for suspend now */
 #define PF_NOFREEZE	0x00008000	/* this thread should not be frozen */
 #define PF_FROZEN	0x00010000	/* frozen for system suspend */
 #define PF_FSTRANS	0x00020000	/* inside a filesystem transaction */


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

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

[Pavel Machek]

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

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

[Rafael J. Wysocki]

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

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

[Rafael J. Wysocki]

Hi,

On Sunday, 26 November 2006 11:02, Rafael J. Wysocki wrote:
> 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.

Patch with atomic_t follows.

The atomic_set(..., 0) are used to avoid the (theoretical) situation in which
freezing might be decreased twice in a row and I've decided to explicitly
initialize freezing in fork.c for clarity.

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/freezer.h |   10 +++++-----
 include/linux/sched.h   |    4 +++-
 kernel/fork.c           |    4 ++++
 3 files changed, 12 insertions(+), 6 deletions(-)

Index: linux-2.6.19-rc6-mm1/include/linux/freezer.h
===================================================================
--- linux-2.6.19-rc6-mm1.orig/include/linux/freezer.h	2006-11-26 11:31:39.000000000 +0100
+++ linux-2.6.19-rc6-mm1/include/linux/freezer.h	2006-11-26 11:52:29.000000000 +0100
@@ -14,16 +14,15 @@ static inline int frozen(struct task_str
  */
 static inline int freezing(struct task_struct *p)
 {
-	return p->flags & PF_FREEZE;
+	return !!atomic_read(&p->freezing);
 }
 
 /*
  * Request that a process be frozen
- * FIXME: SMP problem. We may not modify other process' flags!
  */
 static inline void freeze(struct task_struct *p)
 {
-	p->flags |= PF_FREEZE;
+	atomic_inc(&p->freezing);
 }
 
 /*
@@ -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;
+	atomic_set(&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;
+	atomic_set(&p->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	2006-11-26 11:31:39.000000000 +0100
+++ linux-2.6.19-rc6-mm1/include/linux/sched.h	2006-11-26 11:33:29.000000000 +0100
@@ -1065,6 +1065,9 @@ struct task_struct {
 #ifdef	CONFIG_TASK_DELAY_ACCT
 	struct task_delay_info *delays;
 #endif
+#ifdef CONFIG_PM
+	atomic_t freezing;	/* if set, we should be freezing for suspend */
+#endif
 #ifdef CONFIG_FAULT_INJECTION
 	int make_it_fail;
 #endif
@@ -1161,7 +1164,6 @@ static inline void put_task_struct(struc
 #define PF_MEMALLOC	0x00000800	/* Allocating memory */
 #define PF_FLUSHER	0x00001000	/* responsible for disk writeback */
 #define PF_USED_MATH	0x00002000	/* if unset the fpu must be initialized before use */
-#define PF_FREEZE	0x00004000	/* this task is being frozen for suspend now */
 #define PF_NOFREEZE	0x00008000	/* this thread should not be frozen */
 #define PF_FROZEN	0x00010000	/* frozen for system suspend */
 #define PF_FSTRANS	0x00020000	/* inside a filesystem transaction */
Index: linux-2.6.19-rc6-mm1/kernel/fork.c
===================================================================
--- linux-2.6.19-rc6-mm1.orig/kernel/fork.c	2006-11-25 21:26:52.000000000 +0100
+++ linux-2.6.19-rc6-mm1/kernel/fork.c	2006-11-26 11:45:00.000000000 +0100
@@ -1097,6 +1097,10 @@ static struct task_struct *copy_process(
 	p->blocked_on = NULL; /* not blocked yet */
 #endif
 
+#ifdef CONFIG_PM
+	atomic_set(&p->freezing, 0);
+#endif
+
 	p->tgid = p->pid;
 	if (clone_flags & CLONE_THREAD)
 		p->tgid = current->tgid;

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

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

[Rafael J. Wysocki]

Hi,

On Sunday, 26 November 2006 12:15, Rafael J. Wysocki wrote:
> On Sunday, 26 November 2006 11:02, Rafael J. Wysocki wrote:
> > On Sunday, 26 November 2006 08:47, Pavel Machek wrote:
<--snip--> 
> > Okay, I'll use atomic_t.
> 
> Patch with atomic_t follows.
> 
> The atomic_set(..., 0) are used to avoid the (theoretical) situation in which
> freezing might be decreased twice in a row and I've decided to explicitly
> initialize freezing in fork.c for clarity.

I've just realized that there is a race in freeze_process() where we should
check if the proces hasn't been frozen already (the process may have entered
the refrigerator and reset 'freezing' after we checked its PF_FROZEN flag,
so we have to check if PF_FROZEN is set for the process before we
set 'freezing' for it).  Since an explicit memory barrier is needed here, it
seems reasonable to add one in frozen_process() as well.

Also, it seems to me that stopped processes require special treatment.
Namely, one or more of them might receive the continuation signal after
we check their states in try_to_freeze_tasks() so we should make sure this
hasn't happened before we break the main loop in there.

Revised patch follows.

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/freezer.h |   11 ++++++-----
 include/linux/sched.h   |    4 +++-
 kernel/fork.c           |    4 ++++
 kernel/power/process.c  |   36 +++++++++++++++++++++++++++++-------
 4 files changed, 42 insertions(+), 13 deletions(-)

Index: linux-2.6.19-rc6-mm1/include/linux/freezer.h
===================================================================
--- linux-2.6.19-rc6-mm1.orig/include/linux/freezer.h	2006-11-26 11:31:39.000000000 +0100
+++ linux-2.6.19-rc6-mm1/include/linux/freezer.h	2006-11-26 14:07:03.000000000 +0100
@@ -14,16 +14,15 @@ static inline int frozen(struct task_str
  */
 static inline int freezing(struct task_struct *p)
 {
-	return p->flags & PF_FREEZE;
+	return !!atomic_read(&p->freezing);
 }
 
 /*
  * Request that a process be frozen
- * FIXME: SMP problem. We may not modify other process' flags!
  */
 static inline void freeze(struct task_struct *p)
 {
-	p->flags |= PF_FREEZE;
+	atomic_inc(&p->freezing);
 }
 
 /*
@@ -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;
+	atomic_set(&p->freezing, 0);
 }
 
 /*
@@ -52,7 +51,9 @@ 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;
+	wmb();
+	atomic_set(&p->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	2006-11-26 11:31:39.000000000 +0100
+++ linux-2.6.19-rc6-mm1/include/linux/sched.h	2006-11-26 11:33:29.000000000 +0100
@@ -1065,6 +1065,9 @@ struct task_struct {
 #ifdef	CONFIG_TASK_DELAY_ACCT
 	struct task_delay_info *delays;
 #endif
+#ifdef CONFIG_PM
+	atomic_t freezing;	/* if set, we should be freezing for suspend */
+#endif
 #ifdef CONFIG_FAULT_INJECTION
 	int make_it_fail;
 #endif
@@ -1161,7 +1164,6 @@ static inline void put_task_struct(struc
 #define PF_MEMALLOC	0x00000800	/* Allocating memory */
 #define PF_FLUSHER	0x00001000	/* responsible for disk writeback */
 #define PF_USED_MATH	0x00002000	/* if unset the fpu must be initialized before use */
-#define PF_FREEZE	0x00004000	/* this task is being frozen for suspend now */
 #define PF_NOFREEZE	0x00008000	/* this thread should not be frozen */
 #define PF_FROZEN	0x00010000	/* frozen for system suspend */
 #define PF_FSTRANS	0x00020000	/* inside a filesystem transaction */
Index: linux-2.6.19-rc6-mm1/kernel/fork.c
===================================================================
--- linux-2.6.19-rc6-mm1.orig/kernel/fork.c	2006-11-25 21:26:52.000000000 +0100
+++ linux-2.6.19-rc6-mm1/kernel/fork.c	2006-11-26 11:45:00.000000000 +0100
@@ -1097,6 +1097,10 @@ static struct task_struct *copy_process(
 	p->blocked_on = NULL; /* not blocked yet */
 #endif
 
+#ifdef CONFIG_PM
+	atomic_set(&p->freezing, 0);
+#endif
+
 	p->tgid = p->pid;
 	if (clone_flags & CLONE_THREAD)
 		p->tgid = current->tgid;
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(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.
+			 */
+			todo = nr_stopped;
+			nr_stopped = 0;
+			read_lock(&tasklist_lock);
+			do_each_thread(g, p) {
+				if (p->state == TASK_STOPPED)
+					nr_stopped++;
+			} while_each_thread(g, p);
+			read_unlock(&tasklist_lock);
+			todo -= nr_stopped;
+		}
 		if (todo && time_after(jiffies, end_time))
 			break;
 	} while (todo);

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

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

[Pavel Machek]

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?

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

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

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

[Rafael J. Wysocki]

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

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

[Pavel Machek]

Hi!

> > If frozen is atomic_t, do we need memory barrier?
> 
> I think so.  For example on x86-64 atomic_read() is just a read.

I'm not sure, but for x86-64 barriers are nops, anyway, IIRC.

> > > @@ -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).

No, SIGCONT was sent before freezing even started, and for some reason
takes long time on other CPU.

[Of course it is going to be quite hard to hit that race _in practice_
and mdelay(1000) before check would solve it for practical
purposes...?]

> Anyway, for now I've no idea how to fix this properly.  Will think about it
> tomorrow.
								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

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

[Alan Stern]

On Mon, 27 Nov 2006, Pavel Machek wrote:

> Hi!
> 
> > > If frozen is atomic_t, do we need memory barrier?
> > 
> > I think so.  For example on x86-64 atomic_read() is just a read.
> 
> I'm not sure, but for x86-64 barriers are nops, anyway, IIRC.

atomic_read() does not include its own memory barrier.  If you want one, 
you have to write it yourself.  At least, that's how I interpret 
Documentation/atomic_ops.txt.

Alan Stern


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

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

[Rafael J. Wysocki]

On Monday, 27 November 2006 00:28, Pavel Machek wrote:
> Hi!
> 
> > > If frozen is atomic_t, do we need memory barrier?
> > 
> > I think so.  For example on x86-64 atomic_read() is just a read.
> 
> I'm not sure, but for x86-64 barriers are nops, anyway, IIRC.

Well, last time I checked they were "lfence" and "sfence", at least on SMP.

Greetings,
Rafael

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

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

[Pavel Machek]

Hi!

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

Sorry for one more change, but


/* set thread flags in other task's structures
 * - see asm/thread_info.h for TIF_xxxx flags available
 */
static inline void set_tsk_thread_flag(struct task_struct *tsk, int
flag)
{
        set_ti_thread_flag(task_thread_info(tsk), flag);
}

...could we use set_tsk_thread_flag and friends to avoid enlarging
task struct?
								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

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

[Rafael J. Wysocki]

On Monday, 27 November 2006 11:50, Pavel Machek wrote:
> Hi!
> 
> > > > @@ -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.
> 
> Sorry for one more change, but
> 
> 
> /* set thread flags in other task's structures
>  * - see asm/thread_info.h for TIF_xxxx flags available
>  */
> static inline void set_tsk_thread_flag(struct task_struct *tsk, int
> flag)
> {
>         set_ti_thread_flag(task_thread_info(tsk), flag);
> }
> 
> ...could we use set_tsk_thread_flag and friends to avoid enlarging
> task struct?

_Of_ _course_.  [Heh, I didn't even know something like this existed. ;-)]

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

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

[Pavel Machek]

On Mon 2006-11-27 21:02:38, Rafael J. Wysocki wrote:
> On Monday, 27 November 2006 11:50, Pavel Machek wrote:
> > Hi!
> > 
> > > > > @@ -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.
> > 
> > Sorry for one more change, but
> > 
> > 
> > /* set thread flags in other task's structures
> >  * - see asm/thread_info.h for TIF_xxxx flags available
> >  */
> > static inline void set_tsk_thread_flag(struct task_struct *tsk, int
> > flag)
> > {
> >         set_ti_thread_flag(task_thread_info(tsk), flag);
> > }
> > 
> > ...could we use set_tsk_thread_flag and friends to avoid enlarging
> > task struct?
> 
> _Of_ _course_.  [Heh, I didn't even know something like this existed. ;-)]

Well, of course I did not know, too.... I was just searching for
atomic_t semantics when I hit this by accident ;-).
								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

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

[Rafael J. Wysocki]

Hi,

On Monday, 27 November 2006 00:09, Rafael J. Wysocki wrote:
> On Sunday, 26 November 2006 20:48, Pavel Machek wrote:
<--snip-->
> > > @@ -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.

As far as this particular problem is concerned, I think there are two possible
solutions.

One of them would be do disable the delivery of continuation signals before
we start freezing processes, but I don't know how to do this exactly so that
it's not racy.  Also it would be quite intrusive.

The other one may be something along with the lines of the appended patch.

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/process.c |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Index: linux-2.6.19-rc6-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/kernel/power/process.c	2006-11-28 22:49:30.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/power/process.c	2006-11-29 00:10:05.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;
 }
@@ -103,9 +102,13 @@ static unsigned int try_to_freeze_tasks(
 			if (frozen(p))
 				continue;
 
+			if (p->state == TASK_STOPPED && freezing(p))
+				continue;
+
 			if (p->state == TASK_TRACED &&
 			    (frozen(p->parent) ||
-			     p->parent->state == TASK_STOPPED)) {
+			     (p->parent->state == TASK_STOPPED &&
+			      freezing(p->parent)))) {
 				cancel_freezing(p);
 				continue;
 			}
@@ -185,6 +188,18 @@ int freeze_processes(void)
 	return 0;
 }
 
+static void release_stopped_tasks(void)
+{
+	struct task_struct *g, *p;
+
+	read_lock(&tasklist_lock);
+	do_each_thread(g, p) {
+		if (p->state == TASK_STOPPED)
+			cancel_freezing(p);
+	} while_each_thread(g, p);
+	read_unlock(&tasklist_lock);
+}
+
 static void thaw_tasks(int thaw_user_space)
 {
 	struct task_struct *g, *p;
@@ -207,6 +222,7 @@ static void thaw_tasks(int thaw_user_spa
 void thaw_processes(void)
 {
 	printk("Restarting tasks ... ");
+	release_stopped_tasks();
 	thaw_tasks(FREEZER_KERNEL_THREADS);
 	thaw_tasks(FREEZER_USER_SPACE);
 	schedule();

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

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

[Pavel Machek]

Hi!

> > > 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.
> 
> As far as this particular problem is concerned, I think there are two possible
> solutions.
> 
> One of them would be do disable the delivery of continuation signals before
> we start freezing processes, but I don't know how to do this exactly so that
> it's not racy.  Also it would be quite intrusive.
> 
> The other one may be something along with the lines of the appended patch.

There has to be a better solution. Stopped tasks are suspended
somewhere in kernel, right? One try_to_freeze() and problem should be
solved, in regular way, and without tricks...?

								Pavel

> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  kernel/power/process.c |   22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6.19-rc6-mm2/kernel/power/process.c
> ===================================================================
> --- linux-2.6.19-rc6-mm2.orig/kernel/power/process.c	2006-11-28 22:49:30.000000000 +0100
> +++ linux-2.6.19-rc6-mm2/kernel/power/process.c	2006-11-29 00:10:05.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;
>  }
> @@ -103,9 +102,13 @@ static unsigned int try_to_freeze_tasks(
>  			if (frozen(p))
>  				continue;
>  
> +			if (p->state == TASK_STOPPED && freezing(p))
> +				continue;
> +
>  			if (p->state == TASK_TRACED &&
>  			    (frozen(p->parent) ||
> -			     p->parent->state == TASK_STOPPED)) {
> +			     (p->parent->state == TASK_STOPPED &&
> +			      freezing(p->parent)))) {
>  				cancel_freezing(p);
>  				continue;
>  			}
> @@ -185,6 +188,18 @@ int freeze_processes(void)
>  	return 0;
>  }
>  
> +static void release_stopped_tasks(void)
> +{
> +	struct task_struct *g, *p;
> +
> +	read_lock(&tasklist_lock);
> +	do_each_thread(g, p) {
> +		if (p->state == TASK_STOPPED)
> +			cancel_freezing(p);
> +	} while_each_thread(g, p);
> +	read_unlock(&tasklist_lock);
> +}
> +
>  static void thaw_tasks(int thaw_user_space)
>  {
>  	struct task_struct *g, *p;
> @@ -207,6 +222,7 @@ static void thaw_tasks(int thaw_user_spa
>  void thaw_processes(void)
>  {
>  	printk("Restarting tasks ... ");
> +	release_stopped_tasks();
>  	thaw_tasks(FREEZER_KERNEL_THREADS);
>  	thaw_tasks(FREEZER_USER_SPACE);
>  	schedule();
> 
> -------------------------------------------------------------------------
> 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
> _______________________________________________
> Suspend-devel mailing list
> Suspend-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/suspend-devel

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

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

[Rafael J. Wysocki]

On Thursday, 30 November 2006 00:55, Pavel Machek wrote:
> Hi!
> 
> > > > 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.
> > 
> > As far as this particular problem is concerned, I think there are two possible
> > solutions.
> > 
> > One of them would be do disable the delivery of continuation signals before
> > we start freezing processes, but I don't know how to do this exactly so that
> > it's not racy.  Also it would be quite intrusive.
> > 
> > The other one may be something along with the lines of the appended patch.
> 
> There has to be a better solution. Stopped tasks are suspended
> somewhere in kernel, right? One try_to_freeze() and problem should be
> solved, in regular way, and without tricks...?

Why?  _This_ is a regular way, IMHO.

The problem is that stopped tasks aren't actually running (obviously) so they
_can't_ execute try_to_freeze() until someone sends them a signal.  However,
once they actually have received the signal, we want them to freeze, so we
must tell them to do so.  Still, if they don't receive the signal, we want them
to stay stopped (IOW, the freezer by itself should not wake them up).

Therefore, we need to call freeze_process() for the stopped tasks too, but
this alone won't wake them up (which is good).  Now if someone wakes them
up in the meantime, they will go to the refrigerator thanks to the
try_to_freeze() in get_signal_to_deliver() (they will think they have a signal
to handle, because we have called freeze_process() for them).  _But_ if
anyone doesn't wake them up, we must cancel the freezing of them so they
don't go freezing when they receive the continuation signal at some point
in the future.

Of course after we have called freeze_process() for a stopped tasks, we
shouldn't count that task as freezeable, because it may not be woken up by
anyone (which is _very_likely BTW).

IMO there's no simpler way of doing this and anything else would be racy.

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

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

[Rafael J. Wysocki]

Hi,

On Thursday, 30 November 2006 01:21, Rafael J. Wysocki wrote:
> On Thursday, 30 November 2006 00:55, Pavel Machek wrote:
> > Hi!
> > 
> > > > > 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.
> > > 
> > > As far as this particular problem is concerned, I think there are two possible
> > > solutions.
> > > 
> > > One of them would be do disable the delivery of continuation signals before
> > > we start freezing processes, but I don't know how to do this exactly so that
> > > it's not racy.  Also it would be quite intrusive.
> > > 
> > > The other one may be something along with the lines of the appended patch.
> > 
> > There has to be a better solution. Stopped tasks are suspended
> > somewhere in kernel, right? One try_to_freeze() and problem should be
> > solved, in regular way, and without tricks...?
> 
> Why?  _This_ is a regular way, IMHO.
> 
> The problem is that stopped tasks aren't actually running (obviously) so they
> _can't_ execute try_to_freeze() until someone sends them a signal.  However,
> once they actually have received the signal, we want them to freeze, so we
> must tell them to do so.  Still, if they don't receive the signal, we want them
> to stay stopped (IOW, the freezer by itself should not wake them up).

<--snip-->

In fact, I really mean that if we want a process to go to the refrigerator, we
have to set PF_FREEZE for it (otherwise try_to_freeze() won't do anytning).
Thus because we want stopped processes to go to the refrigerator once they
have received the continuation signal, we have to set PF_FREEZE for them,
so we should call either freeze_process() or just freeze() for them.

Now once we have set PF_FREEZE for a stopped process, we shouldn't count
it as freezeable any more, because we can't do anything more with it.
Moreover, if the process hasn't received the continuation signal before we
call freeze_processes(), PF_FREEZE set will still be set for it, so we have to
clear it (otherwise the process would go to the refrigerator as soon as it
receives the continuation signal).

Now the question remains if we should call the entire freeze_process() or just
freeze() for stopped tasks and I think it really doesn't matter.  Still, since we
call recalc_sigpending() in the refrigerator, I think it's reasonable to use
freeze_process() in this case (less lines of code).

Additionally, we can move the try_to_freeze() in get_signal_to_deliver() so
that processes receiving continuation signals are frozen immediately rather
than some time later, but this doesn't really change the rest of the patch
(which follows - untested for now, but I'll test it later today).

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/process.c |   41 +++++++++++++++++++++++++++++++++++------
 kernel/signal.c        |    2 +-
 2 files changed, 36 insertions(+), 7 deletions(-)

Index: linux-2.6.19-rc6-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/kernel/power/process.c
+++ linux-2.6.19-rc6-mm2/kernel/power/process.c
@@ -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;
 }
@@ -81,11 +80,21 @@ static void cancel_freezing(struct task_
 	}
 }
 
+static inline int stopped_and_freezing(struct task_struct *p)
+{
+	return p->state == TASK_STOPPED && freezing(p);
+}
+
 static inline int is_user_space(struct task_struct *p)
 {
 	return p->mm && !(p->flags & PF_BORROWED_MM);
 }
 
+static inline int unfrozen_stopped_or_traced(struct task_struct *p)
+{
+	return ;
+}
+
 static unsigned int try_to_freeze_tasks(int freeze_user_space)
 {
 	struct task_struct *g, *p;
@@ -103,9 +112,11 @@ static unsigned int try_to_freeze_tasks(
 			if (frozen(p))
 				continue;
 
-			if (p->state == TASK_TRACED &&
-			    (frozen(p->parent) ||
-			     p->parent->state == TASK_STOPPED)) {
+			if (stopped_and_freezing(p))
+				continue;
+
+			if (p->state == TASK_TRACED && (frozen(p->parent) ||
+			    stopped_and_freezing(p->parent))) {
 				cancel_freezing(p);
 				continue;
 			}
@@ -149,7 +160,8 @@ static unsigned int try_to_freeze_tasks(
 			if (is_user_space(p) == !freeze_user_space)
 				continue;
 
-			if (freezeable(p) && !frozen(p))
+			if (freezeable(p) && !frozen(p) &&
+			    p->state != TASK_STOPPED && p->state != TASK_TRACED)
 				printk(KERN_ERR " %s\n", p->comm);
 
 			cancel_freezing(p);
@@ -185,6 +197,18 @@ int freeze_processes(void)
 	return 0;
 }
 
+static void release_stopped_tasks(void)
+{
+	struct task_struct *g, *p;
+
+	read_lock(&tasklist_lock);
+	do_each_thread(g, p) {
+		if (stopped_and_freezing(p))
+			cancel_freezing(p);
+	} while_each_thread(g, p);
+	read_unlock(&tasklist_lock);
+}
+
 static void thaw_tasks(int thaw_user_space)
 {
 	struct task_struct *g, *p;
@@ -197,6 +221,10 @@ static void thaw_tasks(int thaw_user_spa
 		if (is_user_space(p) == !thaw_user_space)
 			continue;
 
+		if (!frozen(p) &&
+		    (p->state == TASK_STOPPED || p->state == TASK_TRACED))
+			continue;
+
 		if (!thaw_process(p))
 			printk(KERN_WARNING " Strange, %s not stopped\n",
 				p->comm );
@@ -207,6 +235,7 @@ static void thaw_tasks(int thaw_user_spa
 void thaw_processes(void)
 {
 	printk("Restarting tasks ... ");
+	release_stopped_tasks();
 	thaw_tasks(FREEZER_KERNEL_THREADS);
 	thaw_tasks(FREEZER_USER_SPACE);
 	schedule();
Index: linux-2.6.19-rc6-mm2/kernel/signal.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/kernel/signal.c
+++ linux-2.6.19-rc6-mm2/kernel/signal.c
@@ -1937,9 +1937,9 @@ int get_signal_to_deliver(siginfo_t *inf
 	sigset_t *mask = &current->blocked;
 	int signr = 0;
 
+relock:
 	try_to_freeze();
 
-relock:
 	spin_lock_irq(&current->sighand->siglock);
 	for (;;) {
 		struct k_sigaction *ka;

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

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

[Alan Stern]

On Thu, 30 Nov 2006, Rafael J. Wysocki wrote:

> In fact, I really mean that if we want a process to go to the refrigerator, we
> have to set PF_FREEZE for it (otherwise try_to_freeze() won't do anytning).
> Thus because we want stopped processes to go to the refrigerator once they
> have received the continuation signal, we have to set PF_FREEZE for them,
> so we should call either freeze_process() or just freeze() for them.
> 
> Now once we have set PF_FREEZE for a stopped process, we shouldn't count
> it as freezeable any more, because we can't do anything more with it.
> Moreover, if the process hasn't received the continuation signal before we
> call freeze_processes(), PF_FREEZE set will still be set for it, so we have to
> clear it (otherwise the process would go to the refrigerator as soon as it
> receives the continuation signal).

I haven't followed the patches, but this obviously is a very tricky issue.  
A stopped process might be waiting for a signal or event that can be sent
only by another process which is already frozen -- in which case the
stopped process is itself effectively frozen without your doing anything
to it.

On the other hand, a stopped process might be waiting for a signal that 
can be sent by an unfreezable process -- and the stopped process might be 
holding a lock which is needed by some other unfreezable process, so you 
need to allow it to run long enough to release the lock before freezing 
it.

Unless you can somehow rule out this scenario (unfreezable process waiting 
for resource held by unfrozen but stopped and freezable process), I don't 
see how this approach can be made to work.

Alan Stern


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

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

[Rafael J. Wysocki]

On Thursday, 30 November 2006 16:43, Alan Stern wrote:
> On Thu, 30 Nov 2006, Rafael J. Wysocki wrote:
> 
> > In fact, I really mean that if we want a process to go to the refrigerator, we
> > have to set PF_FREEZE for it (otherwise try_to_freeze() won't do anytning).
> > Thus because we want stopped processes to go to the refrigerator once they
> > have received the continuation signal, we have to set PF_FREEZE for them,
> > so we should call either freeze_process() or just freeze() for them.
> > 
> > Now once we have set PF_FREEZE for a stopped process, we shouldn't count
> > it as freezeable any more, because we can't do anything more with it.
> > Moreover, if the process hasn't received the continuation signal before we
> > call freeze_processes(), PF_FREEZE set will still be set for it, so we have to
> > clear it (otherwise the process would go to the refrigerator as soon as it
> > receives the continuation signal).
> 
> I haven't followed the patches, but this obviously is a very tricky issue.

Yes.

> A stopped process might be waiting for a signal or event that can be sent
> only by another process which is already frozen -- in which case the
> stopped process is itself effectively frozen without your doing anything
> to it.

Yes.

> On the other hand, a stopped process might be waiting for a signal that 
> can be sent by an unfreezable process -- and the stopped process might be 
> holding a lock which is needed by some other unfreezable process, so you 
> need to allow it to run long enough to release the lock before freezing 
> it.

Theoretically, yes.  But does it happen?  Moreover, even if it does, why can't
we hold the unfreezeable process until the lock is released?

> Unless you can somehow rule out this scenario (unfreezable process waiting 
> for resource held by unfrozen but stopped and freezable process), I don't 
> see how this approach can be made to work.

Well, currently we're treating all stopped processes as non-freezeable.  This
also is theoretically wrong, because, for example, the continuation signal may
be delivered to a stopped userland proces after we have frozen all of the
other userland processes, so we won't be freezing this one and it'll be
allowed to run when we are saving the image, which is generally dangerous.
I also haven't seen it happening, though.

Thus there are two theoretical issues on the table, but I think the
probability of the second one actually happening is a bit greater.

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

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

[Rafael J. Wysocki]

On Thursday, 30 November 2006 17:04, Rafael J. Wysocki wrote:
> On Thursday, 30 November 2006 16:43, Alan Stern wrote:
> > On Thu, 30 Nov 2006, Rafael J. Wysocki wrote:
> > 
> > > In fact, I really mean that if we want a process to go to the refrigerator, we
> > > have to set PF_FREEZE for it (otherwise try_to_freeze() won't do anytning).
> > > Thus because we want stopped processes to go to the refrigerator once they
> > > have received the continuation signal, we have to set PF_FREEZE for them,
> > > so we should call either freeze_process() or just freeze() for them.
> > > 
> > > Now once we have set PF_FREEZE for a stopped process, we shouldn't count
> > > it as freezeable any more, because we can't do anything more with it.
> > > Moreover, if the process hasn't received the continuation signal before we
> > > call freeze_processes(), PF_FREEZE set will still be set for it, so we have to
> > > clear it (otherwise the process would go to the refrigerator as soon as it
> > > receives the continuation signal).
> > 
> > I haven't followed the patches, but this obviously is a very tricky issue.
> 
> Yes.
> 
> > A stopped process might be waiting for a signal or event that can be sent
> > only by another process which is already frozen -- in which case the
> > stopped process is itself effectively frozen without your doing anything
> > to it.
> 
> Yes.
> 
> > On the other hand, a stopped process might be waiting for a signal that 
> > can be sent by an unfreezable process -- and the stopped process might be 
> > holding a lock which is needed by some other unfreezable process, so you 
> > need to allow it to run long enough to release the lock before freezing 
> > it.
> 
> Theoretically, yes.  But does it happen?  Moreover, even if it does, why can't
> we hold the unfreezeable process until the lock is released?
> 
> > Unless you can somehow rule out this scenario (unfreezable process waiting 
> > for resource held by unfrozen but stopped and freezable process), I don't 
> > see how this approach can be made to work.

Hm, in fact I think that if it happens, then this is a bug.

Namely, the freezeable task can only acquire the lock when it's running.  Thus
it would have to acquire the lock before it's stopped.  Consequently, it could
be preempted and subsequently frozen after it has acquired the lock and
_before_ it's stopped.

Now from the unfreezeable task's point of view it doesn't matter if the
freezeable task acquired the lock and has been frozen before being stopped
or if it acquired the lock and was stopped and then has been woken up and
frozen, since in both cases the final outcome is the same: the freezeable task
is frozen and holds the lock, so the unfreezeable task cannot continue
running.

Thus I think the question is if _any_ freezeable task, be it stopped or not,
can hold a lock that's needed by an unfreezeable task, and I think the answer
is, generally, 'no, it can't', because otherwise it might block the
unfreezeable task (if it's frozen while holding the lock).

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

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

[Alan Stern]

On Thu, 30 Nov 2006, Rafael J. Wysocki wrote:

> > > On the other hand, a stopped process might be waiting for a signal that 
> > > can be sent by an unfreezable process -- and the stopped process might be 
> > > holding a lock which is needed by some other unfreezable process, so you 
> > > need to allow it to run long enough to release the lock before freezing 
> > > it.
> > 
> > Theoretically, yes.  But does it happen?  Moreover, even if it does, why can't
> > we hold the unfreezeable process until the lock is released?
> > 
> > > Unless you can somehow rule out this scenario (unfreezable process waiting 
> > > for resource held by unfrozen but stopped and freezable process), I don't 
> > > see how this approach can be made to work.
> 
> Hm, in fact I think that if it happens, then this is a bug.
> 
> Namely, the freezeable task can only acquire the lock when it's running.  Thus
> it would have to acquire the lock before it's stopped.  Consequently, it could
> be preempted and subsequently frozen after it has acquired the lock and
> _before_ it's stopped.
> 
> Now from the unfreezeable task's point of view it doesn't matter if the
> freezeable task acquired the lock and has been frozen before being stopped
> or if it acquired the lock and was stopped and then has been woken up and
> frozen, since in both cases the final outcome is the same: the freezeable task
> is frozen and holds the lock, so the unfreezeable task cannot continue
> running.
> 
> Thus I think the question is if _any_ freezeable task, be it stopped or not,
> can hold a lock that's needed by an unfreezeable task, and I think the answer
> is, generally, 'no, it can't', because otherwise it might block the
> unfreezeable task (if it's frozen while holding the lock).

Well, certainly the answer is "It shouldn't".  :-)

I don't know any examples where an unfreezable task waits for a resource 
which might be held by a frozen task, PROVIDED that tasks don't get frozen 
at a bad spot.

Here's what I mean.  usb-storage's kernel thread is unfreezable, because
it might be needed for reading or writing a memory image to a swap region.  
If there's an I/O error then usb-storage will try to issue a USB port
reset, for which it needs to acquire the USB device's lock.

Now various other tasks may acquire that lock, and they may even stop
while holding it.  However they should never get frozen while holding the
lock -- which means they shouldn't get frozen at arbitrary times merely
because they are stopped.  They are careful to call try_to_freeze() only
at times when they don't hold any locks.

Alan Stern


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

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

[Rafael J. Wysocki]

On Thursday, 30 November 2006 23:34, Alan Stern wrote:
> On Thu, 30 Nov 2006, Rafael J. Wysocki wrote:
> 
> > > > On the other hand, a stopped process might be waiting for a signal that 
> > > > can be sent by an unfreezable process -- and the stopped process might be 
> > > > holding a lock which is needed by some other unfreezable process, so you 
> > > > need to allow it to run long enough to release the lock before freezing 
> > > > it.
> > > 
> > > Theoretically, yes.  But does it happen?  Moreover, even if it does, why can't
> > > we hold the unfreezeable process until the lock is released?
> > > 
> > > > Unless you can somehow rule out this scenario (unfreezable process waiting 
> > > > for resource held by unfrozen but stopped and freezable process), I don't 
> > > > see how this approach can be made to work.
> > 
> > Hm, in fact I think that if it happens, then this is a bug.
> > 
> > Namely, the freezeable task can only acquire the lock when it's running.  Thus
> > it would have to acquire the lock before it's stopped.  Consequently, it could
> > be preempted and subsequently frozen after it has acquired the lock and
> > _before_ it's stopped.
> > 
> > Now from the unfreezeable task's point of view it doesn't matter if the
> > freezeable task acquired the lock and has been frozen before being stopped
> > or if it acquired the lock and was stopped and then has been woken up and
> > frozen, since in both cases the final outcome is the same: the freezeable task
> > is frozen and holds the lock, so the unfreezeable task cannot continue
> > running.
> > 
> > Thus I think the question is if _any_ freezeable task, be it stopped or not,
> > can hold a lock that's needed by an unfreezeable task, and I think the answer
> > is, generally, 'no, it can't', because otherwise it might block the
> > unfreezeable task (if it's frozen while holding the lock).
> 
> Well, certainly the answer is "It shouldn't".  :-)
> 
> I don't know any examples where an unfreezable task waits for a resource 
> which might be held by a frozen task, PROVIDED that tasks don't get frozen 
> at a bad spot.
> 
> Here's what I mean.  usb-storage's kernel thread is unfreezable, because
> it might be needed for reading or writing a memory image to a swap region.  
> If there's an I/O error then usb-storage will try to issue a USB port
> reset, for which it needs to acquire the USB device's lock.
> 
> Now various other tasks may acquire that lock, and they may even stop
> while holding it.  However they should never get frozen while holding the
> lock -- which means they shouldn't get frozen at arbitrary times merely
> because they are stopped.  They are careful to call try_to_freeze() only
> at times when they don't hold any locks.

This means they are kernel threads, so they won't be entering
get_signal_to_deliver(), will they?

If they don't enter get_signal_to_deliver(), they can only be frozen where
they explicitly call try_to_freeze().

Greetings,
Rafael

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

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

[Alan Stern]

On Thu, 30 Nov 2006, Rafael J. Wysocki wrote:

> > Here's what I mean.  usb-storage's kernel thread is unfreezable, because
> > it might be needed for reading or writing a memory image to a swap region.  
> > If there's an I/O error then usb-storage will try to issue a USB port
> > reset, for which it needs to acquire the USB device's lock.
> > 
> > Now various other tasks may acquire that lock, and they may even stop
> > while holding it.  However they should never get frozen while holding the
> > lock -- which means they shouldn't get frozen at arbitrary times merely
> > because they are stopped.  They are careful to call try_to_freeze() only
> > at times when they don't hold any locks.
> 
> This means they are kernel threads, so they won't be entering
> get_signal_to_deliver(), will they?

Some of them are kernel threads and some of them are user threads.  Only 
the kernel threads call try_to_freeze().

> If they don't enter get_signal_to_deliver(), they can only be frozen where
> they explicitly call try_to_freeze().

What about the user threads?

Alan Stern


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

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

[Rafael J. Wysocki]

On Friday, 1 December 2006 15:56, Alan Stern wrote:
> On Thu, 30 Nov 2006, Rafael J. Wysocki wrote:
> 
> > > Here's what I mean.  usb-storage's kernel thread is unfreezable, because
> > > it might be needed for reading or writing a memory image to a swap region.  
> > > If there's an I/O error then usb-storage will try to issue a USB port
> > > reset, for which it needs to acquire the USB device's lock.
> > > 
> > > Now various other tasks may acquire that lock, and they may even stop
> > > while holding it.  However they should never get frozen while holding the
> > > lock -- which means they shouldn't get frozen at arbitrary times merely
> > > because they are stopped.  They are careful to call try_to_freeze() only
> > > at times when they don't hold any locks.
> > 
> > This means they are kernel threads, so they won't be entering
> > get_signal_to_deliver(), will they?
> 
> Some of them are kernel threads and some of them are user threads.  Only 
> the kernel threads call try_to_freeze().
> 
> > If they don't enter get_signal_to_deliver(), they can only be frozen where
> > they explicitly call try_to_freeze().
> 
> What about the user threads?

We send fake signals to them, so they enter get_signal_to_deliver() and they
call try_to_freeze() from there.

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

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

[Alan Stern]

On Fri, 1 Dec 2006, Rafael J. Wysocki wrote:

> On Friday, 1 December 2006 15:56, Alan Stern wrote:
> > On Thu, 30 Nov 2006, Rafael J. Wysocki wrote:
> > 
> > > > Here's what I mean.  usb-storage's kernel thread is unfreezable, because
> > > > it might be needed for reading or writing a memory image to a swap region.  
> > > > If there's an I/O error then usb-storage will try to issue a USB port
> > > > reset, for which it needs to acquire the USB device's lock.
> > > > 
> > > > Now various other tasks may acquire that lock, and they may even stop
> > > > while holding it.  However they should never get frozen while holding the
> > > > lock -- which means they shouldn't get frozen at arbitrary times merely
> > > > because they are stopped.  They are careful to call try_to_freeze() only
> > > > at times when they don't hold any locks.
> > > 
> > > This means they are kernel threads, so they won't be entering
> > > get_signal_to_deliver(), will they?
> > 
> > Some of them are kernel threads and some of them are user threads.  Only 
> > the kernel threads call try_to_freeze().
> > 
> > > If they don't enter get_signal_to_deliver(), they can only be frozen where
> > > they explicitly call try_to_freeze().
> > 
> > What about the user threads?
> 
> We send fake signals to them, so they enter get_signal_to_deliver() and they
> call try_to_freeze() from there.

Well then, see, you're doing exactly what you said should never happen --
you are freezing a user thread while it might be holding a lock that the
unfreezable usb-storage thread needs.

Alan Stern


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

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

[Rafael J. Wysocki]

On Friday, 1 December 2006 22:17, Alan Stern wrote:
> On Fri, 1 Dec 2006, Rafael J. Wysocki wrote:
> 
> > On Friday, 1 December 2006 15:56, Alan Stern wrote:
> > > On Thu, 30 Nov 2006, Rafael J. Wysocki wrote:
> > > 
> > > > > Here's what I mean.  usb-storage's kernel thread is unfreezable, because
> > > > > it might be needed for reading or writing a memory image to a swap region.  
> > > > > If there's an I/O error then usb-storage will try to issue a USB port
> > > > > reset, for which it needs to acquire the USB device's lock.
> > > > > 
> > > > > Now various other tasks may acquire that lock, and they may even stop
> > > > > while holding it.  However they should never get frozen while holding the
> > > > > lock -- which means they shouldn't get frozen at arbitrary times merely
> > > > > because they are stopped.  They are careful to call try_to_freeze() only
> > > > > at times when they don't hold any locks.
> > > > 
> > > > This means they are kernel threads, so they won't be entering
> > > > get_signal_to_deliver(), will they?
> > > 
> > > Some of them are kernel threads and some of them are user threads.  Only 
> > > the kernel threads call try_to_freeze().
> > > 
> > > > If they don't enter get_signal_to_deliver(), they can only be frozen where
> > > > they explicitly call try_to_freeze().
> > > 
> > > What about the user threads?
> > 
> > We send fake signals to them, so they enter get_signal_to_deliver() and they
> > call try_to_freeze() from there.
> 
> Well then, see, you're doing exactly what you said should never happen --
> you are freezing a user thread while it might be holding a lock that the
> unfreezable usb-storage thread needs.

Well, the current code does exactly the same - it freezes userland tasks
by sending them fake signals (and there's no way to check if such a process
holds any locks at that time).  Moreover, it's been doing that from day one
and my patch doesn't change that.

If you have an unfreezeable process that depends on locks that may be held
by userland tasks, then _this_ is a bug, IMHO.

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

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

[Alan Stern]

On Fri, 1 Dec 2006, Rafael J. Wysocki wrote:

> Well, the current code does exactly the same - it freezes userland tasks
> by sending them fake signals (and there's no way to check if such a process
> holds any locks at that time).  Moreover, it's been doing that from day one
> and my patch doesn't change that.

Obviously the scenario I described is very unlikely.  It would depend on 
a user program doing exactly the wrong thing at the wrong time, _and_ 
using a USB mass storage device for swapping, _and_ getting an I/O error 
while reading or writing the memory image.

> If you have an unfreezeable process that depends on locks that may be held
> by userland tasks, then _this_ is a bug, IMHO.

In this case it isn't absolute dependence.  If usb-storage isn't able to
obtain the lock it needs, then it times out after 1 second and simply
doesn't reset the device.

Still, you would have your work cut out trying to find all the places in
the kernel where an unfreezable process takes a lock which might be held
by a user process.  For instance, I doubt that all the workqueue threads
could pass this test.

Alan Stern


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

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

[Rafael J. Wysocki]

On Friday, 1 December 2006 23:07, Alan Stern wrote:
> On Fri, 1 Dec 2006, Rafael J. Wysocki wrote:
> 
> > Well, the current code does exactly the same - it freezes userland tasks
> > by sending them fake signals (and there's no way to check if such a process
> > holds any locks at that time).  Moreover, it's been doing that from day one
> > and my patch doesn't change that.
> 
> Obviously the scenario I described is very unlikely.  It would depend on 
> a user program doing exactly the wrong thing at the wrong time, _and_ 
> using a USB mass storage device for swapping, _and_ getting an I/O error 
> while reading or writing the memory image.
> 
> > If you have an unfreezeable process that depends on locks that may be held
> > by userland tasks, then _this_ is a bug, IMHO.
> 
> In this case it isn't absolute dependence.  If usb-storage isn't able to
> obtain the lock it needs, then it times out after 1 second and simply
> doesn't reset the device.

Well, I think as long as the system can recover, it's all fine.

> Still, you would have your work cut out trying to find all the places in
> the kernel where an unfreezable process takes a lock which might be held
> by a user process.  For instance, I doubt that all the workqueue threads
> could pass this test.

Fortunately such problems have never been reported, so we can postpone the
hunt for these places a little. ;-)  But seriously, they are potentially dangerous
with the current code, so in fact we'll need to have a look at them at some
point in the future.

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

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

[Pavel Machek]

Hi!

> > We send fake signals to them, so they enter get_signal_to_deliver() and they
> > call try_to_freeze() from there.
> 
> Well then, see, you're doing exactly what you said should never happen --
> you are freezing a user thread while it might be holding a lock that the
> unfreezable usb-storage thread needs.


I do not think it is okay to call get_signal_to_deliver with *any*
lock held... Or do you have example when that is broken?
							Pavel
-- 
Thanks for all the (sleeping) penguins.

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

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

[Alan Stern]

On Sat, 2 Dec 2006, Pavel Machek wrote:

> Hi!
> 
> > > We send fake signals to them, so they enter get_signal_to_deliver() and they
> > > call try_to_freeze() from there.
> > 
> > Well then, see, you're doing exactly what you said should never happen --
> > you are freezing a user thread while it might be holding a lock that the
> > unfreezable usb-storage thread needs.
> 
> 
> I do not think it is okay to call get_signal_to_deliver with *any*
> lock held... Or do you have example when that is broken?

Is get_signal_to_deliver() ever called at any time other than when the CPU 
is about to switch back into user mode?  If not then there shouldn't be 
any problem.  The routines I was talking about earlier hold the USB device 
locks while running in kernel mode, but they release them before returning 
to user mode.

Alan Stern


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

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

[Rafael J. Wysocki]

On Saturday, 2 December 2006 16:39, Alan Stern wrote:
> On Sat, 2 Dec 2006, Pavel Machek wrote:
> 
> > Hi!
> > 
> > > > We send fake signals to them, so they enter get_signal_to_deliver() and they
> > > > call try_to_freeze() from there.
> > > 
> > > Well then, see, you're doing exactly what you said should never happen --
> > > you are freezing a user thread while it might be holding a lock that the
> > > unfreezable usb-storage thread needs.
> > 
> > 
> > I do not think it is okay to call get_signal_to_deliver with *any*
> > lock held... Or do you have example when that is broken?
> 
> Is get_signal_to_deliver() ever called at any time other than when the CPU 
> is about to switch back into user mode?

No, I don't think so.  There's a check in do_signal() that only allows it to
call get_signal_to_deliver() in user mode.

> If not then there shouldn't be any problem.  The routines I was talking
> about earlier hold the USB device locks while running in kernel mode, but
> they release them before returning to user mode.

Then everything is correct, I think.

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

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

[Rafael J. Wysocki]

On Thursday, 30 November 2006 16:07, Rafael J. Wysocki wrote:
> On Thursday, 30 November 2006 01:21, Rafael J. Wysocki wrote:
> > On Thursday, 30 November 2006 00:55, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > > > 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.
> > > > 
> > > > As far as this particular problem is concerned, I think there are two possible
> > > > solutions.
> > > > 
> > > > One of them would be do disable the delivery of continuation signals before
> > > > we start freezing processes, but I don't know how to do this exactly so that
> > > > it's not racy.  Also it would be quite intrusive.
> > > > 
> > > > The other one may be something along with the lines of the appended patch.
> > > 
> > > There has to be a better solution. Stopped tasks are suspended
> > > somewhere in kernel, right? One try_to_freeze() and problem should be
> > > solved, in regular way, and without tricks...?
> > 
> > Why?  _This_ is a regular way, IMHO.
> > 
> > The problem is that stopped tasks aren't actually running (obviously) so they
> > _can't_ execute try_to_freeze() until someone sends them a signal.  However,
> > once they actually have received the signal, we want them to freeze, so we
> > must tell them to do so.  Still, if they don't receive the signal, we want them
> > to stay stopped (IOW, the freezer by itself should not wake them up).
> 
> <--snip-->
> 
> In fact, I really mean that if we want a process to go to the refrigerator, we
> have to set PF_FREEZE for it (otherwise try_to_freeze() won't do anytning).
> Thus because we want stopped processes to go to the refrigerator once they
> have received the continuation signal, we have to set PF_FREEZE for them,
> so we should call either freeze_process() or just freeze() for them.
> 
> Now once we have set PF_FREEZE for a stopped process, we shouldn't count
> it as freezeable any more, because we can't do anything more with it.
> Moreover, if the process hasn't received the continuation signal before we
> call freeze_processes(), PF_FREEZE set will still be set for it, so we have to
> clear it (otherwise the process would go to the refrigerator as soon as it
> receives the continuation signal).
> 
> Now the question remains if we should call the entire freeze_process() or just
> freeze() for stopped tasks and I think it really doesn't matter.  Still, since we
> call recalc_sigpending() in the refrigerator, I think it's reasonable to use
> freeze_process() in this case (less lines of code).
> 
> Additionally, we can move the try_to_freeze() in get_signal_to_deliver() so
> that processes receiving continuation signals are frozen immediately rather
> than some time later, but this doesn't really change the rest of the patch
> (which follows - untested for now, but I'll test it later today).

Now tested and it doesn't break anything, at least.

Greetings,
Rafael

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

[Pavel Machek]

Hi!

> > Okay, I'll use atomic_t.
> 
> Patch with atomic_t follows.
> 
> The atomic_set(..., 0) are used to avoid the (theoretical) situation in which
> freezing might be decreased twice in a row and I've decided to explicitly
> initialize freezing in fork.c for clarity.

Looks okay to me, but I'm not sure if Andrew/someone will not have a
problem with task_struct getting bigger. Can you push this one early
and separately?

								Pavel

> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  include/linux/freezer.h |   10 +++++-----
>  include/linux/sched.h   |    4 +++-
>  kernel/fork.c           |    4 ++++
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> Index: linux-2.6.19-rc6-mm1/include/linux/freezer.h
> ===================================================================
> --- linux-2.6.19-rc6-mm1.orig/include/linux/freezer.h	2006-11-26 11:31:39.000000000 +0100
> +++ linux-2.6.19-rc6-mm1/include/linux/freezer.h	2006-11-26 11:52:29.000000000 +0100
> @@ -14,16 +14,15 @@ static inline int frozen(struct task_str
>   */
>  static inline int freezing(struct task_struct *p)
>  {
> -	return p->flags & PF_FREEZE;
> +	return !!atomic_read(&p->freezing);
>  }
>  
>  /*
>   * Request that a process be frozen
> - * FIXME: SMP problem. We may not modify other process' flags!
>   */
>  static inline void freeze(struct task_struct *p)
>  {
> -	p->flags |= PF_FREEZE;
> +	atomic_inc(&p->freezing);
>  }
>  
>  /*
> @@ -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;
> +	atomic_set(&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;
> +	atomic_set(&p->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	2006-11-26 11:31:39.000000000 +0100
> +++ linux-2.6.19-rc6-mm1/include/linux/sched.h	2006-11-26 11:33:29.000000000 +0100
> @@ -1065,6 +1065,9 @@ struct task_struct {
>  #ifdef	CONFIG_TASK_DELAY_ACCT
>  	struct task_delay_info *delays;
>  #endif
> +#ifdef CONFIG_PM
> +	atomic_t freezing;	/* if set, we should be freezing for suspend */
> +#endif
>  #ifdef CONFIG_FAULT_INJECTION
>  	int make_it_fail;
>  #endif
> @@ -1161,7 +1164,6 @@ static inline void put_task_struct(struc
>  #define PF_MEMALLOC	0x00000800	/* Allocating memory */
>  #define PF_FLUSHER	0x00001000	/* responsible for disk writeback */
>  #define PF_USED_MATH	0x00002000	/* if unset the fpu must be initialized before use */
> -#define PF_FREEZE	0x00004000	/* this task is being frozen for suspend now */
>  #define PF_NOFREEZE	0x00008000	/* this thread should not be frozen */
>  #define PF_FROZEN	0x00010000	/* frozen for system suspend */
>  #define PF_FSTRANS	0x00020000	/* inside a filesystem transaction */
> Index: linux-2.6.19-rc6-mm1/kernel/fork.c
> ===================================================================
> --- linux-2.6.19-rc6-mm1.orig/kernel/fork.c	2006-11-25 21:26:52.000000000 +0100
> +++ linux-2.6.19-rc6-mm1/kernel/fork.c	2006-11-26 11:45:00.000000000 +0100
> @@ -1097,6 +1097,10 @@ static struct task_struct *copy_process(
>  	p->blocked_on = NULL; /* not blocked yet */
>  #endif
>  
> +#ifdef CONFIG_PM
> +	atomic_set(&p->freezing, 0);
> +#endif
> +
>  	p->tgid = p->pid;
>  	if (clone_flags & CLONE_THREAD)
>  		p->tgid = current->tgid;
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/linux-pm

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

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

[Luca]

On 11/26/06, Pavel Machek <pavel@ucw.cz> wrote:
===================================================================
> > --- 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.

What about using lock_task_sighand()? This should protect us against
->flags manipulation due to signals without the need of an extra
field.

Luca

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

[RFC][PATCH -mm 2/5] swsusp: Change code ordering in disk.c

[Rafael J. Wysocki]

Change the ordering of code in kernel/power/disk.c so that
device_suspend() is called before disable_nonboot_cpus() and
platform_finish() is called after enable_nonboot_cpus() and before
device_resume().

The changes here only affect the built-in swsusp.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/cpu.c        |    2 
 kernel/power/disk.c |  110 ++++++++++++++++++++++++++--------------------------
 2 files changed, 58 insertions(+), 54 deletions(-)

Index: linux-2.6.19-rc6-mm1/kernel/cpu.c
===================================================================
--- linux-2.6.19-rc6-mm1.orig/kernel/cpu.c
+++ linux-2.6.19-rc6-mm1/kernel/cpu.c
@@ -318,6 +318,8 @@ void enable_nonboot_cpus(void)
 	mutex_lock(&cpu_add_remove_lock);
 	cpu_hotplug_disabled = 0;
 	mutex_unlock(&cpu_add_remove_lock);
+	if (cpus_empty(frozen_cpus))
+		return;
 
 	printk("Enabling non-boot CPUs ...\n");
 	for_each_cpu_mask(cpu, frozen_cpus) {
Index: linux-2.6.19-rc6-mm1/kernel/power/disk.c
===================================================================
--- linux-2.6.19-rc6-mm1.orig/kernel/power/disk.c
+++ linux-2.6.19-rc6-mm1/kernel/power/disk.c
@@ -89,52 +89,26 @@ static inline void platform_finish(void)
 	}
 }
 
+static void unprepare_processes(void)
+{
+	thaw_processes();
+	pm_restore_console();
+}
+
 static int prepare_processes(void)
 {
 	int error = 0;
 
 	pm_prepare_console();
-
-	error = disable_nonboot_cpus();
-	if (error)
-		goto enable_cpus;
-
 	if (freeze_processes()) {
 		error = -EBUSY;
-		goto thaw;
-	}
-
-	if (pm_disk_mode == PM_DISK_TESTPROC) {
-		printk("swsusp debug: Waiting for 5 seconds.\n");
-		mdelay(5000);
-		goto thaw;
-	}
-
-	error = platform_prepare();
-	if (error)
-		goto thaw;
-
-	/* Free memory before shutting down devices. */
-	if (!(error = swsusp_shrink_memory()))
+	} else if (pm_disk_mode != PM_DISK_TESTPROC) {
 		return 0;
-
-	platform_finish();
- thaw:
-	thaw_processes();
- enable_cpus:
-	enable_nonboot_cpus();
-	pm_restore_console();
+	}
+	unprepare_processes();
 	return error;
 }
 
-static void unprepare_processes(void)
-{
-	platform_finish();
-	thaw_processes();
-	enable_nonboot_cpus();
-	pm_restore_console();
-}
-
 /**
  *	pm_suspend_disk - The granpappy of hibernation power management.
  *
@@ -152,29 +126,45 @@ int pm_suspend_disk(void)
 	if (error)
 		return error;
 
-	if (pm_disk_mode == PM_DISK_TESTPROC)
+	if (pm_disk_mode == PM_DISK_TESTPROC) {
+		printk("swsusp debug: Waiting for 5 seconds.\n");
+		mdelay(5000);
+		goto Thaw;
+	}
+	/* Free memory before shutting down devices. */
+	error = swsusp_shrink_memory();
+	if (error)
+		goto Thaw;
+
+	error = platform_prepare();
+	if (error)
 		goto Thaw;
 
 	suspend_console();
 	error = device_suspend(PMSG_FREEZE);
 	if (error) {
-		resume_console();
-		printk("Some devices failed to suspend\n");
-		goto Thaw;
+		printk(KERN_ERR "PM: Some devices failed to suspend\n");
+		goto Resume_devices;
 	}
+	error = disable_nonboot_cpus();
+	if (error)
+		goto Enable_cpus;
 
 	if (pm_disk_mode == PM_DISK_TEST) {
 		printk("swsusp debug: Waiting for 5 seconds.\n");
 		mdelay(5000);
-		goto Done;
+		goto Enable_cpus;
 	}
 
 	pr_debug("PM: snapshotting memory.\n");
 	in_suspend = 1;
-	if ((error = swsusp_suspend()))
-		goto Done;
+	error = swsusp_suspend();
+	if (error)
+		goto Enable_cpus;
 
 	if (in_suspend) {
+		enable_nonboot_cpus();
+		platform_finish();
 		device_resume();
 		resume_console();
 		pr_debug("PM: writing image.\n");
@@ -190,7 +180,10 @@ int pm_suspend_disk(void)
 	}
 
 	swsusp_free();
- Done:
+ Enable_cpus:
+	enable_nonboot_cpus();
+ Resume_devices:
+	platform_finish();
 	device_resume();
 	resume_console();
  Thaw:
@@ -239,41 +232,50 @@ static int software_resume(void)
 
 	pr_debug("PM: Checking swsusp image.\n");
 
-	if ((error = swsusp_check()))
+	error = swsusp_check();
+	if (error)
 		goto Done;
 
 	pr_debug("PM: Preparing processes for restore.\n");
 
-	if ((error = prepare_processes())) {
+	error = prepare_processes();
+	if (error) {
 		swsusp_close();
 		goto Done;
 	}
 
 	pr_debug("PM: Reading swsusp image.\n");
 
-	if ((error = swsusp_read())) {
+	error = swsusp_read();
+	if (error) {
 		swsusp_free();
 		goto Thaw;
 	}
 
 	pr_debug("PM: Preparing devices for restore.\n");
 
-	suspend_console();
-	if ((error = device_suspend(PMSG_PRETHAW))) {
-		resume_console();
-		printk("Some devices failed to suspend\n");
+	error = platform_prepare();
+	if (error) {
 		swsusp_free();
 		goto Thaw;
 	}
+	suspend_console();
+	error = device_suspend(PMSG_PRETHAW);
+	if (error)
+		goto Free;
 
-	mb();
+	error = disable_nonboot_cpus();
+	if (!error)
+		swsusp_resume();
 
-	pr_debug("PM: Restoring saved image.\n");
-	swsusp_resume();
-	pr_debug("PM: Restore failed, recovering.n");
+	enable_nonboot_cpus();
+ Free:
+	swsusp_free();
+	platform_finish();
 	device_resume();
 	resume_console();
  Thaw:
+	printk(KERN_ERR "PM: Restore failed, recovering.\n");
 	unprepare_processes();
  Done:
 	/* For success case, the suspend path will release the lock */


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

[RFC][PATCH -mm 3/5] swsusp: Change code ordering in user.c

[Rafael J. Wysocki]

Change the ordering of code in kernel/power/user.c so that device_suspend()
is called before disable_nonboot_cpus() and device_resume() is called after
enable_nonboot_cpus().

The changes here only affect the userland interface of swsusp.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/user.c |   92 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 58 insertions(+), 34 deletions(-)

Index: linux-2.6.19-rc6-mm1/kernel/power/user.c
===================================================================
--- linux-2.6.19-rc6-mm1.orig/kernel/power/user.c
+++ linux-2.6.19-rc6-mm1/kernel/power/user.c
@@ -122,6 +122,59 @@ static ssize_t snapshot_write(struct fil
 	return res;
 }
 
+static int snapshot_suspend(void)
+{
+	int error;
+
+	mutex_lock(&pm_mutex);
+	/* Free memory before shutting down devices. */
+	error = swsusp_shrink_memory();
+	if (error)
+		goto Finish;
+
+	suspend_console();
+	error = device_suspend(PMSG_FREEZE);
+	if (error)
+		goto Resume_devices;
+
+	error = disable_nonboot_cpus();
+	if (!error) {
+		in_suspend = 1;
+		error = swsusp_suspend();
+	}
+	enable_nonboot_cpus();
+ Resume_devices:
+	device_resume();
+	resume_console();
+ Finish:
+	mutex_unlock(&pm_mutex);
+	return error;
+}
+
+static int snapshot_restore(void)
+{
+	int error;
+
+	mutex_lock(&pm_mutex);
+	pm_prepare_console();
+	suspend_console();
+	error = device_suspend(PMSG_PRETHAW);
+	if (error)
+		goto Resume_devices;
+
+	error = disable_nonboot_cpus();
+	if (!error)
+		error = swsusp_resume();
+
+	enable_nonboot_cpus();
+ Resume_devices:
+	device_resume();
+	resume_console();
+	pm_restore_console();
+	mutex_unlock(&pm_mutex);
+	return error;
+}
+
 static int snapshot_ioctl(struct inode *inode, struct file *filp,
                           unsigned int cmd, unsigned long arg)
 {
@@ -145,14 +198,9 @@ static int snapshot_ioctl(struct inode *
 		if (data->frozen)
 			break;
 		mutex_lock(&pm_mutex);
-		error = disable_nonboot_cpus();
-		if (!error) {
-			error = freeze_processes();
-			if (error) {
-				thaw_processes();
-				enable_nonboot_cpus();
-				error = -EBUSY;
-			}
+		if (freeze_processes()) {
+			thaw_processes();
+			error = -EBUSY;
 		}
 		mutex_unlock(&pm_mutex);
 		if (!error)
@@ -164,7 +212,6 @@ static int snapshot_ioctl(struct inode *
 			break;
 		mutex_lock(&pm_mutex);
 		thaw_processes();
-		enable_nonboot_cpus();
 		mutex_unlock(&pm_mutex);
 		data->frozen = 0;
 		break;
@@ -174,20 +221,7 @@ static int snapshot_ioctl(struct inode *
 			error = -EPERM;
 			break;
 		}
-		mutex_lock(&pm_mutex);
-		/* Free memory before shutting down devices. */
-		error = swsusp_shrink_memory();
-		if (!error) {
-			suspend_console();
-			error = device_suspend(PMSG_FREEZE);
-			if (!error) {
-				in_suspend = 1;
-				error = swsusp_suspend();
-				device_resume();
-			}
-			resume_console();
-		}
-		mutex_unlock(&pm_mutex);
+		error = snapshot_suspend();
 		if (!error)
 			error = put_user(in_suspend, (unsigned int __user *)arg);
 		if (!error)
@@ -201,17 +235,7 @@ static int snapshot_ioctl(struct inode *
 			error = -EPERM;
 			break;
 		}
-		mutex_lock(&pm_mutex);
-		pm_prepare_console();
-		suspend_console();
-		error = device_suspend(PMSG_PRETHAW);
-		if (!error) {
-			error = swsusp_resume();
-			device_resume();
-		}
-		resume_console();
-		pm_restore_console();
-		mutex_unlock(&pm_mutex);
+		error = snapshot_restore();
 		break;
 
 	case SNAPSHOT_FREE:

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

[RFC][PATCH -mm 4/5] swsusp: Add PLATFORM_SNAPSHOT and PLATFORM_RESTORE ioctls

[Rafael J. Wysocki]

Since pm_ops->finish() has to be called after enable_nonboot_cpus() and before
device_resume(), from the userland interface perspective it should be treated
as a part of the atomic suspend and resume operations.  For this reason we
need two additional ioctls to be called instead of ATOMIC_SNAPSHOT and
ATOMIC_RESTORE, respectively, when the platform suspend mode is to be used.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/power.h |    4 ++
 kernel/power/user.c  |   71 +++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 64 insertions(+), 11 deletions(-)

Index: linux-2.6.19-rc6-mm1/kernel/power/power.h
===================================================================
--- linux-2.6.19-rc6-mm1.orig/kernel/power/power.h
+++ linux-2.6.19-rc6-mm1/kernel/power/power.h
@@ -133,7 +133,9 @@ struct resume_swap_area {
 #define SNAPSHOT_PMOPS			_IOW(SNAPSHOT_IOC_MAGIC, 12, unsigned int)
 #define SNAPSHOT_SET_SWAP_AREA		_IOW(SNAPSHOT_IOC_MAGIC, 13, \
 							struct resume_swap_area)
-#define SNAPSHOT_IOC_MAXNR	13
+#define SNAPSHOT_PLATFORM_SNAPSHOT	_IOW(SNAPSHOT_IOC_MAGIC, 14, unsigned int)
+#define SNAPSHOT_PLATFORM_RESTORE	_IO(SNAPSHOT_IOC_MAGIC, 15)
+#define SNAPSHOT_IOC_MAXNR	15
 
 #define PMOPS_PREPARE	1
 #define PMOPS_ENTER	2
Index: linux-2.6.19-rc6-mm1/kernel/power/user.c
===================================================================
--- linux-2.6.19-rc6-mm1.orig/kernel/power/user.c
+++ linux-2.6.19-rc6-mm1/kernel/power/user.c
@@ -122,7 +122,23 @@ static ssize_t snapshot_write(struct fil
 	return res;
 }
 
-static int snapshot_suspend(void)
+static inline int platform_prepare(void)
+{
+	int error = 0;
+
+	if (pm_ops && pm_ops->prepare)
+		error = pm_ops->prepare(PM_SUSPEND_DISK);
+
+	return error;
+}
+
+static inline void platform_finish(void)
+{
+	if (pm_ops && pm_ops->finish)
+		pm_ops->finish(PM_SUSPEND_DISK);
+}
+
+static int snapshot_suspend(int platform_mode)
 {
 	int error;
 
@@ -132,6 +148,11 @@ static int snapshot_suspend(void)
 	if (error)
 		goto Finish;
 
+	if (platform_mode) {
+		error = platform_prepare();
+		if (error)
+			goto Finish;
+	}
 	suspend_console();
 	error = device_suspend(PMSG_FREEZE);
 	if (error)
@@ -144,6 +165,9 @@ static int snapshot_suspend(void)
 	}
 	enable_nonboot_cpus();
  Resume_devices:
+	if (platform_mode)
+		platform_finish();
+
 	device_resume();
 	resume_console();
  Finish:
@@ -151,12 +175,17 @@ static int snapshot_suspend(void)
 	return error;
 }
 
-static int snapshot_restore(void)
+static int snapshot_restore(int platform_mode)
 {
 	int error;
 
 	mutex_lock(&pm_mutex);
 	pm_prepare_console();
+	if (platform_mode) {
+		error = platform_prepare();
+		if (error)
+			goto Finish;
+	}
 	suspend_console();
 	error = device_suspend(PMSG_PRETHAW);
 	if (error)
@@ -168,8 +197,12 @@ static int snapshot_restore(void)
 
 	enable_nonboot_cpus();
  Resume_devices:
+	if (platform_mode)
+		platform_finish();
+
 	device_resume();
 	resume_console();
+ Finish:
 	pm_restore_console();
 	mutex_unlock(&pm_mutex);
 	return error;
@@ -221,7 +254,7 @@ static int snapshot_ioctl(struct inode *
 			error = -EPERM;
 			break;
 		}
-		error = snapshot_suspend();
+		error = snapshot_suspend(0);
 		if (!error)
 			error = put_user(in_suspend, (unsigned int __user *)arg);
 		if (!error)
@@ -235,7 +268,7 @@ static int snapshot_ioctl(struct inode *
 			error = -EPERM;
 			break;
 		}
-		error = snapshot_restore();
+		error = snapshot_restore(0);
 		break;
 
 	case SNAPSHOT_FREE:
@@ -345,9 +378,7 @@ static int snapshot_ioctl(struct inode *
 		switch (arg) {
 
 		case PMOPS_PREPARE:
-			if (pm_ops->prepare) {
-				error = pm_ops->prepare(PM_SUSPEND_DISK);
-			}
+			error = platform_prepare();
 			break;
 
 		case PMOPS_ENTER:
@@ -356,9 +387,7 @@ static int snapshot_ioctl(struct inode *
 			break;
 
 		case PMOPS_FINISH:
-			if (pm_ops && pm_ops->finish) {
-				pm_ops->finish(PM_SUSPEND_DISK);
-			}
+			platform_finish();
 			break;
 
 		default:
@@ -399,6 +428,28 @@ static int snapshot_ioctl(struct inode *
 		}
 		break;
 
+	case SNAPSHOT_PLATFORM_SNAPSHOT:
+		if (data->mode != O_RDONLY || !data->frozen  || data->ready) {
+			error = -EPERM;
+			break;
+		}
+		error = snapshot_suspend(1);
+		if (!error)
+			error = put_user(in_suspend, (unsigned int __user *)arg);
+		if (!error)
+			data->ready = 1;
+		break;
+
+	case SNAPSHOT_PLATFORM_RESTORE:
+		snapshot_write_finalize(&data->handle);
+		if (data->mode != O_WRONLY || !data->frozen ||
+		    !snapshot_image_loaded(&data->handle)) {
+			error = -EPERM;
+			break;
+		}
+		error = snapshot_restore(1);
+		break;
+
 	default:
 		error = -ENOTTY;
 

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

Re: [linux-pm] [RFC][PATCH -mm 4/5] swsusp: Add PLATFORM_SNAPSHOT and PLATFORM_RESTORE ioctls

[Pavel Machek]

Hi!

> Since pm_ops->finish() has to be called after enable_nonboot_cpus() and before
> device_resume(), from the userland interface perspective it should be treated
> as a part of the atomic suspend and resume operations.  For this reason we
> need two additional ioctls to be called instead of ATOMIC_SNAPSHOT and
> ATOMIC_RESTORE, respectively, when the platform suspend mode is to be used.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  kernel/power/power.h |    4 ++
>  kernel/power/user.c  |   71 +++++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 64 insertions(+), 11 deletions(-)
> 
> Index: linux-2.6.19-rc6-mm1/kernel/power/power.h
> ===================================================================
> --- linux-2.6.19-rc6-mm1.orig/kernel/power/power.h
> +++ linux-2.6.19-rc6-mm1/kernel/power/power.h
> @@ -133,7 +133,9 @@ struct resume_swap_area {
>  #define SNAPSHOT_PMOPS			_IOW(SNAPSHOT_IOC_MAGIC, 12, unsigned int)
>  #define SNAPSHOT_SET_SWAP_AREA		_IOW(SNAPSHOT_IOC_MAGIC, 13, \
>  							struct resume_swap_area)
> -#define SNAPSHOT_IOC_MAXNR	13
> +#define SNAPSHOT_PLATFORM_SNAPSHOT	_IOW(SNAPSHOT_IOC_MAGIC, 14, unsigned int)
> +#define SNAPSHOT_PLATFORM_RESTORE	_IO(SNAPSHOT_IOC_MAGIC, 15)
> +#define SNAPSHOT_IOC_MAXNR	15

Maybe we should add SNAPSHOT_WITH_FLAGS (uint)
                    RESTORE_WITH_FLAGS (uint)

with one of those flags being USE_PLATFORM_MODE? We may need more
flags in future...

...or maybe have ioctl(fd, USE_PLATFORM_MODE)?
									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

Re: [linux-pm] [RFC][PATCH -mm 4/5] swsusp: Add PLATFORM_SNAPSHOT and PLATFORM_RESTORE ioctls

[Rafael J. Wysocki]

Hi,

On Sunday, 26 November 2006 20:51, Pavel Machek wrote:
> Hi!
> 
> > Since pm_ops->finish() has to be called after enable_nonboot_cpus() and before
> > device_resume(), from the userland interface perspective it should be treated
> > as a part of the atomic suspend and resume operations.  For this reason we
> > need two additional ioctls to be called instead of ATOMIC_SNAPSHOT and
> > ATOMIC_RESTORE, respectively, when the platform suspend mode is to be used.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  kernel/power/power.h |    4 ++
> >  kernel/power/user.c  |   71 +++++++++++++++++++++++++++++++++++++++++++--------
> >  2 files changed, 64 insertions(+), 11 deletions(-)
> > 
> > Index: linux-2.6.19-rc6-mm1/kernel/power/power.h
> > ===================================================================
> > --- linux-2.6.19-rc6-mm1.orig/kernel/power/power.h
> > +++ linux-2.6.19-rc6-mm1/kernel/power/power.h
> > @@ -133,7 +133,9 @@ struct resume_swap_area {
> >  #define SNAPSHOT_PMOPS			_IOW(SNAPSHOT_IOC_MAGIC, 12, unsigned int)
> >  #define SNAPSHOT_SET_SWAP_AREA		_IOW(SNAPSHOT_IOC_MAGIC, 13, \
> >  							struct resume_swap_area)
> > -#define SNAPSHOT_IOC_MAXNR	13
> > +#define SNAPSHOT_PLATFORM_SNAPSHOT	_IOW(SNAPSHOT_IOC_MAGIC, 14, unsigned int)
> > +#define SNAPSHOT_PLATFORM_RESTORE	_IO(SNAPSHOT_IOC_MAGIC, 15)
> > +#define SNAPSHOT_IOC_MAXNR	15
> 
> Maybe we should add SNAPSHOT_WITH_FLAGS (uint)
>                     RESTORE_WITH_FLAGS (uint)
> 
> with one of those flags being USE_PLATFORM_MODE? We may need more
> flags in future...

Sure, we can do that.

> ...or maybe have ioctl(fd, USE_PLATFORM_MODE)?

No.  At least I'm not going to implement it. ;-)

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

Re: [linux-pm] [RFC][PATCH -mm 4/5] swsusp: Add PLATFORM_SNAPSHOT and PLATFORM_RESTORE ioctls

[Pavel Machek]

Hi!

> > Maybe we should add SNAPSHOT_WITH_FLAGS (uint)
> >                     RESTORE_WITH_FLAGS (uint)
> > 
> > with one of those flags being USE_PLATFORM_MODE? We may need more
> > flags in future...
> 
> Sure, we can do that.

Okay, that works for me ;-).

> > ...or maybe have ioctl(fd, USE_PLATFORM_MODE)?
> 
> No.  At least I'm not going to implement it. ;-)

-ELAZY.

								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

Re: [linux-pm] [RFC][PATCH -mm 4/5] swsusp: Add PLATFORM_SNAPSHOT and PLATFORM_RESTORE ioctls

[Pavel Machek]

On Mon 2006-11-27 00:29:37, Pavel Machek wrote:
> Hi!
> 
> > > Maybe we should add SNAPSHOT_WITH_FLAGS (uint)
> > >                     RESTORE_WITH_FLAGS (uint)
> > > 
> > > with one of those flags being USE_PLATFORM_MODE? We may need more
> > > flags in future...
> > 
> > Sure, we can do that.
> 
> Okay, that works for me ;-).
> 
> > > ...or maybe have ioctl(fd, USE_PLATFORM_MODE)?
> > 
> > No.  At least I'm not going to implement it. ;-)
> 
> -ELAZY.

(Of course this meant "Pavel is too lazy, so he'll take interface
Rafael _is_ willing to implement").
									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

[RFC][PATCH -mm 5/5] PM: Change code ordering in main.c

[Rafael J. Wysocki]

Change the ordering of code in kernel/power/main.c so that device_suspend()
is called before disable_nonboot_cpus() and pm_ops->finish() is called after
enable_nonboot_cpus() and before device_resume().

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/main.c |   34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

Index: linux-2.6.19-rc6-mm1/kernel/power/main.c
===================================================================
--- linux-2.6.19-rc6-mm1.orig/kernel/power/main.c
+++ linux-2.6.19-rc6-mm1/kernel/power/main.c
@@ -43,6 +43,11 @@ void pm_set_ops(struct pm_ops * ops)
 	mutex_unlock(&pm_mutex);
 }
 
+static inline void pm_finish(suspend_state_t state)
+{
+	if (pm_ops->finish)
+		pm_ops->finish(state);
+}
 
 /**
  *	suspend_prepare - Do prep work before entering low-power state.
@@ -63,10 +68,6 @@ static int suspend_prepare(suspend_state
 
 	pm_prepare_console();
 
-	error = disable_nonboot_cpus();
-	if (error)
-		goto Enable_cpu;
-
 	if (freeze_processes()) {
 		error = -EAGAIN;
 		goto Thaw;
@@ -88,18 +89,22 @@ static int suspend_prepare(suspend_state
 	}
 
 	suspend_console();
-	if ((error = device_suspend(PMSG_SUSPEND))) {
+	error = device_suspend(PMSG_SUSPEND);
+	if (error) {
 		printk(KERN_ERR "Some devices failed to suspend\n");
-		goto Finish;
+		goto Resume_devices;
 	}
-	return 0;
- Finish:
-	if (pm_ops->finish)
-		pm_ops->finish(state);
+	error = disable_nonboot_cpus();
+	if (!error)
+		return 0;
+
+	enable_nonboot_cpus();
+ Resume_devices:
+	pm_finish(state);
+	device_resume();
+	resume_console();
  Thaw:
 	thaw_processes();
- Enable_cpu:
-	enable_nonboot_cpus();
 	pm_restore_console();
 	return error;
 }
@@ -134,12 +139,11 @@ int suspend_enter(suspend_state_t state)
 
 static void suspend_finish(suspend_state_t state)
 {
+	enable_nonboot_cpus();
+	pm_finish(state);
 	device_resume();
 	resume_console();
 	thaw_processes();
-	enable_nonboot_cpus();
-	if (pm_ops && pm_ops->finish)
-		pm_ops->finish(state);
 	pm_restore_console();
 }
 


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

Re: [RFC][PATCH -mm] PM: Change ordering of suspend and resume code

[Pavel Machek]

Hi!

> The discussion in a recent thread on Linux-PM has indicated that it's
> necessary to call pm_ops->finish() before devce_resume(),
> but enable_nonboot_cpus() has to be called before pm_ops->finish()
> (cf. http://lists.osdl.org/pipermail/linux-pm/2006-November/004164.html).
> For consistency, it seems reasonable to call disable_nonboot_cpus() after
> device_suspend().
> 
> This way the suspend code will remain symmetrical with respect to the resume
> code and it may allow us to speed up things in the future by suspending and
> resuming devices and/or saving the suspend image in many threads, but for this
> purpose we first need to make freeze_processes() SMP-safe.

I do not think first patch is enough to make it SMP-safe, sorry.

> The following series of patches makes freeze_processes() SMP-safe and
> reorders the suspend and resume code so that nonboot CPUs are disabled
> after devices have been suspended and enabled before the devices are
> resumed.  It also causes pm_ops->finish() to be called after
> enable_nonboot_cpus() wherever necessary.
> 
> The first four patches have been tested on two different x86-64 SMP boxes and
> they don't seem to break anything.  Still, if anyone can test them on some other
> SMP boxes (especially on i386 ones), please do so and tell me if there are
> any problems.
> 
> The last patch is untested.

I guess I should fix s2ram enough that it works for you... What is the
primary notebook you are using?
									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

Re: [RFC][PATCH -mm] PM: Change ordering of suspend and resume code

[Rafael J. Wysocki]

Hi,

On Sunday, 26 November 2006 08:44, Pavel Machek wrote:
> Hi!
> 
> > The discussion in a recent thread on Linux-PM has indicated that it's
> > necessary to call pm_ops->finish() before devce_resume(),
> > but enable_nonboot_cpus() has to be called before pm_ops->finish()
> > (cf. http://lists.osdl.org/pipermail/linux-pm/2006-November/004164.html).
> > For consistency, it seems reasonable to call disable_nonboot_cpus() after
> > device_suspend().
> > 
> > This way the suspend code will remain symmetrical with respect to the resume
> > code and it may allow us to speed up things in the future by suspending and
> > resuming devices and/or saving the suspend image in many threads, but for this
> > purpose we first need to make freeze_processes() SMP-safe.
> 
> I do not think first patch is enough to make it SMP-safe, sorry.

So, what is the scenario in which it will fail?

> > The following series of patches makes freeze_processes() SMP-safe and
> > reorders the suspend and resume code so that nonboot CPUs are disabled
> > after devices have been suspended and enabled before the devices are
> > resumed.  It also causes pm_ops->finish() to be called after
> > enable_nonboot_cpus() wherever necessary.
> > 
> > The first four patches have been tested on two different x86-64 SMP boxes and
> > they don't seem to break anything.  Still, if anyone can test them on some other
> > SMP boxes (especially on i386 ones), please do so and tell me if there are
> > any problems.
> > 
> > The last patch is untested.
> 
> I guess I should fix s2ram enough that it works for you... What is the
> primary notebook you are using?

HPC nx6325.  Someone has reported he's managed to get s2ram working on it,
but it looks like his machine is slightly different to this one ...

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

Re: [RFC][PATCH -mm] PM: Change ordering of suspend and resume code

[Pavel Machek]

Hi!

> > > SMP boxes (especially on i386 ones), please do so and tell me if there are
> > > any problems.
> > > 
> > > The last patch is untested.
> > 
> > I guess I should fix s2ram enough that it works for you... What is the
> > primary notebook you are using?
> 
> HPC nx6325.  Someone has reported he's managed to get s2ram working on it,
> but it looks like his machine is slightly different to this one ...

Is there some way to identify your model? Can you post s2ram strings?
Stefan, do you think we could get machine similar-enough to Rafael's?
									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

Re: [RFC][PATCH -mm] PM: Change ordering of suspend and resume code

[Rafael J. Wysocki]

Hi,

On Sunday, 26 November 2006 22:31, Pavel Machek wrote:
> Hi!
> 
> > > > SMP boxes (especially on i386 ones), please do so and tell me if there are
> > > > any problems.
> > > > 
> > > > The last patch is untested.
> > > 
> > > I guess I should fix s2ram enough that it works for you... What is the
> > > primary notebook you are using?
> > 
> > HPC nx6325.  Someone has reported he's managed to get s2ram working on it,
> > but it looks like his machine is slightly different to this one ...
> 
> Is there some way to identify your model? Can you post s2ram strings?
> Stefan, do you think we could get machine similar-enough to Rafael's?

Well, I'm not user it's worth the effort.  The BIOS/ACPI seems to be
terminally broken on this box and I'll need quite some time to look at it
closely, but not in the nearest future.

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

Re: [RFC][PATCH -mm] PM: Change ordering of suspend and resume code

[Stefan Seyfried]

On Sun, Nov 26, 2006 at 10:31:50PM +0100, Pavel Machek wrote:
> Hi!
> > HPC nx6325.  Someone has reported he's managed to get s2ram working on it,
> > but it looks like his machine is slightly different to this one ...
> 
> Is there some way to identify your model? Can you post s2ram strings?
> Stefan, do you think we could get machine similar-enough to Rafael's?

I have a nx6325 here and will look at it (but it might take some time).
-- 
Stefan Seyfried
QA / R&D Team Mobile Devices        |              "Any ideas, John?"
SUSE LINUX Products GmbH, Nürnberg  | "Well, surrounding them's out." 

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