[PATCH -mm 0/2]: SMP-safe freezer

[PATCH -mm 0/2]: SMP-safe freezer

[Rafael J. Wysocki]

Hi,

The following two patches are designed to make the task freezer SMP-safe.

The first patch is a controversial one, as Pavel doesn't like it, but I have
no other idea how the freezing of stopped tasks could be handled to avoid
the problems that it has now.  I did my best to explain everything in the
changelog.

The second patch is basically needed to make the freezer avoid races with
other tasks when modifying the other tasks' flags.

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

[PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Currently, if a task is stopped (ie. it's in the TASK_STOPPED state), it is
considered by the freezer as unfreezeable.  However, there may be a race
between the freezer and the delivery of the continuation signal to the task
resulting in the task running after we have finished freezing other tasks.
This, in turn, may lead to undesirable effects up to and including a
corruption of data.

To prevent this from happening we first need to make the freezer consider
stopped tasks as freezeable.  For this purpose we need to make freezeable()
stop returning 0 for these tasks.  We must remember, however, that the
stopped tasks need not receive the continuation signal before thaw_processes()
is called, so as soon as PF_FREEZE is set for them try_to_freeze_tasks()
should stop counting them as the ones to wait for.  Additionally, if there's a
traced task (ie. a task in the TASK_TRACED state) the parent of which has
PF_FREEZE set and is stopped, try_to_freeze_tasks() should not wait for it.
Moreover, if there are some stopped tasks that haven't received the continuation
signal before thaw_processes() is called, we must clear PF_FREEZE for them so
that they don't go to the refrigerator when it's no longer desirable.

Still if a stopped tasks receives the continuation signal after the freezer
has returned, it should immediately go to the refrigerator.  For this reason
we need to place the call to try_to_freeze() in get_signal_to_deliver() right
after the relock label.

It is also reasonable to change the error paths so that the names of stopped
or traced tasks which have not frozen are not reported to the user, because in
the majority of cases they would be false positives.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/process.c |   36 ++++++++++++++++++++++++++++++------
 kernel/signal.c        |    2 +-
 2 files changed, 31 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,6 +80,11 @@ 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);
@@ -103,9 +107,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 +155,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 +192,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 +216,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 +230,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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Pavel Machek]

Hi!

> Currently, if a task is stopped (ie. it's in the TASK_STOPPED state), it is
> considered by the freezer as unfreezeable.  However, there may be a race
> between the freezer and the delivery of the continuation signal to the task
> resulting in the task running after we have finished freezing other tasks.
> This, in turn, may lead to undesirable effects up to and including a
> corruption of data.
> 
> To prevent this from happening we first need to make the freezer consider
> stopped tasks as freezeable.  For this purpose we need to make freezeable()
> stop returning 0 for these tasks.  We must remember, however, that the
> stopped tasks need not receive the continuation signal before thaw_processes()
> is called, so as soon as PF_FREEZE is set for them try_to_freeze_tasks()
> should stop counting them as the ones to wait for.  Additionally, if there's a
> traced task (ie. a task in the TASK_TRACED state) the parent of which has
> PF_FREEZE set and is stopped, try_to_freeze_tasks() should not wait for it.
> Moreover, if there are some stopped tasks that haven't received the continuation
> signal before thaw_processes() is called, we must clear PF_FREEZE for them so
> that they don't go to the refrigerator when it's no longer desirable.
> 
> Still if a stopped tasks receives the continuation signal after the freezer
> has returned, it should immediately go to the refrigerator.  For this reason
> we need to place the call to try_to_freeze() in get_signal_to_deliver() right
> after the relock label.
> 
> It is also reasonable to change the error paths so that the names of stopped
> or traced tasks which have not frozen are not reported to the user, because in
> the majority of cases they would be false positives.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Okay, lets go ahead with this one. (ACK) I still think we should be
able to find some simpler solution, I'll take a look.
									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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Pavel Machek]

Hi!

> Currently, if a task is stopped (ie. it's in the TASK_STOPPED state), it is
> considered by the freezer as unfreezeable.  However, there may be a race
> between the freezer and the delivery of the continuation signal to the task
> resulting in the task running after we have finished freezing other tasks.
> This, in turn, may lead to undesirable effects up to and including a
> corruption of data.
> 
> To prevent this from happening we first need to make the freezer consider
> stopped tasks as freezeable.  For this purpose we need to make freezeable()
> stop returning 0 for these tasks.  We must remember, however, that the
> stopped tasks need not receive the continuation signal before thaw_processes()
> is called, so as soon as PF_FREEZE is set for them try_to_freeze_tasks()
> should stop counting them as the ones to wait for.  Additionally, if there's a
> traced task (ie. a task in the TASK_TRACED state) the parent of which has
> PF_FREEZE set and is stopped, try_to_freeze_tasks() should not wait for it.
> Moreover, if there are some stopped tasks that haven't received the continuation
> signal before thaw_processes() is called, we must clear PF_FREEZE for them so
> that they don't go to the refrigerator when it's no longer desirable.

Actually, what do you think about this patch? It removes special
handling of TASK_TRACED, and should do the trick, too...
								Pavel

diff --git a/kernel/power/process.c b/kernel/power/process.c
index 7bcc976..d56e494 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -26,8 +26,7 @@ static inline int freezeable(struct task
 	    (p->flags & PF_NOFREEZE) ||
 	    (p->exit_state == EXIT_ZOMBIE) ||
 	    (p->exit_state == EXIT_DEAD) ||
-	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)) ||
-	    (p->state == TASK_STOPPED))
+	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)))
 		return 0;
 	return 1;
 }
diff --git a/kernel/signal.c b/kernel/signal.c
index 9a61944..e305ad1 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1702,7 +1702,9 @@ finish_stop(int stop_count)
 		read_unlock(&tasklist_lock);
 	}
 
-	schedule();
+	do {
+		schedule();
+	} while (try_to_freeze());
 	/*
 	 * Now we don't run again until continued.
 	 */


-- 
(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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Tuesday, 5 December 2006 11:34, Pavel Machek wrote:
> Hi!
> 
> > Currently, if a task is stopped (ie. it's in the TASK_STOPPED state), it is
> > considered by the freezer as unfreezeable.  However, there may be a race
> > between the freezer and the delivery of the continuation signal to the task
> > resulting in the task running after we have finished freezing other tasks.
> > This, in turn, may lead to undesirable effects up to and including a
> > corruption of data.
> > 
> > To prevent this from happening we first need to make the freezer consider
> > stopped tasks as freezeable.  For this purpose we need to make freezeable()
> > stop returning 0 for these tasks.  We must remember, however, that the
> > stopped tasks need not receive the continuation signal before thaw_processes()
> > is called, so as soon as PF_FREEZE is set for them try_to_freeze_tasks()
> > should stop counting them as the ones to wait for.  Additionally, if there's a
> > traced task (ie. a task in the TASK_TRACED state) the parent of which has
> > PF_FREEZE set and is stopped, try_to_freeze_tasks() should not wait for it.
> > Moreover, if there are some stopped tasks that haven't received the continuation
> > signal before thaw_processes() is called, we must clear PF_FREEZE for them so
> > that they don't go to the refrigerator when it's no longer desirable.
> 
> Actually, what do you think about this patch? It removes special
> handling of TASK_TRACED, and should do the trick, too...

Well, I don't think so, ....

> 								Pavel
> 
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 7bcc976..d56e494 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -26,8 +26,7 @@ static inline int freezeable(struct task
>  	    (p->flags & PF_NOFREEZE) ||
>  	    (p->exit_state == EXIT_ZOMBIE) ||
>  	    (p->exit_state == EXIT_DEAD) ||
> -	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)) ||
> -	    (p->state == TASK_STOPPED))
> +	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)))
>  		return 0;
>  	return 1;
>  }
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9a61944..e305ad1 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1702,7 +1702,9 @@ finish_stop(int stop_count)
>  		read_unlock(&tasklist_lock);
>  	}
>  
> -	schedule();
> +	do {
> +		schedule();
> +	} while (try_to_freeze());
>  	/*
>  	 * Now we don't run again until continued.
>  	 */

... because if you want try_to_freeze() here to trigger, then the task in
question should have PF_FREEZE set, but we don't set it anywhere.
[Actually I think this particular change is equivalent to moving the
try_to_freeze() in get_signal_to_deliver() to after the relock label. :-)]

And the special handling of traced tasks is needed to clear TIF_SIGPENDING
for a task that had been told to freeze before it's parent froze.

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Pavel Machek]

Hi!

> > Actually, what do you think about this patch? It removes special
> > handling of TASK_TRACED, and should do the trick, too...
> 
> Well, I don't think so, ....

> > @@ -1702,7 +1702,9 @@ finish_stop(int stop_count)
> >  		read_unlock(&tasklist_lock);
> >  	}
> >  
> > -	schedule();
> > +	do {
> > +		schedule();
> > +	} while (try_to_freeze());
> >  	/*
> >  	 * Now we don't run again until continued.
> >  	 */
> 
> ... because if you want try_to_freeze() here to trigger, then the task in
> question should have PF_FREEZE set, but we don't set it anywhere.

?? We set it to all the processes, so stopped processes get it,
too...??
							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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Tuesday, 5 December 2006 15:13, Pavel Machek wrote:
> Hi!
> 
> > > Actually, what do you think about this patch? It removes special
> > > handling of TASK_TRACED, and should do the trick, too...
> > 
> > Well, I don't think so, ....
> 
> > > @@ -1702,7 +1702,9 @@ finish_stop(int stop_count)
> > >  		read_unlock(&tasklist_lock);
> > >  	}
> > >  
> > > -	schedule();
> > > +	do {
> > > +		schedule();
> > > +	} while (try_to_freeze());
> > >  	/*
> > >  	 * Now we don't run again until continued.
> > >  	 */
> > 
> > ... because if you want try_to_freeze() here to trigger, then the task in
> > question should have PF_FREEZE set, but we don't set it anywhere.
> 
> ?? We set it to all the processes, so stopped processes get it,
> too...??

No, we don't.  We only set it for the processes for which freezeable() returns 1.

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Pavel Machek]

Hi!

> > Currently, if a task is stopped (ie. it's in the TASK_STOPPED state), it is
> > considered by the freezer as unfreezeable.  However, there may be a race
> > between the freezer and the delivery of the continuation signal to the task
> > resulting in the task running after we have finished freezing other tasks.
> > This, in turn, may lead to undesirable effects up to and including a
> > corruption of data.
> > 
> > To prevent this from happening we first need to make the freezer consider
> > stopped tasks as freezeable.  For this purpose we need to make freezeable()
> > stop returning 0 for these tasks.  We must remember, however, that the
> > stopped tasks need not receive the continuation signal before thaw_processes()
> > is called, so as soon as PF_FREEZE is set for them try_to_freeze_tasks()
> > should stop counting them as the ones to wait for.  Additionally, if there's a
> > traced task (ie. a task in the TASK_TRACED state) the parent of which has
> > PF_FREEZE set and is stopped, try_to_freeze_tasks() should not wait for it.
> > Moreover, if there are some stopped tasks that haven't received the continuation
> > signal before thaw_processes() is called, we must clear PF_FREEZE for them so
> > that they don't go to the refrigerator when it's no longer desirable.
> 
> Actually, what do you think about this patch? It removes special
> handling of TASK_TRACED, and should do the trick, too...

I was surprised, but the patch seems to work okay. Can you replace
your 1/2 with this one, and see what breaks?

								Pavel

> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 7bcc976..d56e494 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -26,8 +26,7 @@ static inline int freezeable(struct task
>  	    (p->flags & PF_NOFREEZE) ||
>  	    (p->exit_state == EXIT_ZOMBIE) ||
>  	    (p->exit_state == EXIT_DEAD) ||
> -	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)) ||
> -	    (p->state == TASK_STOPPED))
> +	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)))
>  		return 0;
>  	return 1;
>  }
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9a61944..e305ad1 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1702,7 +1702,9 @@ finish_stop(int stop_count)
>  		read_unlock(&tasklist_lock);
>  	}
>  
> -	schedule();
> +	do {
> +		schedule();
> +	} while (try_to_freeze());
>  	/*
>  	 * Now we don't run again until continued.
>  	 */
> 
> 

-- 
(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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Tuesday, 5 December 2006 12:24, Pavel Machek wrote:
> Hi!
> 
> > > Currently, if a task is stopped (ie. it's in the TASK_STOPPED state), it is
> > > considered by the freezer as unfreezeable.  However, there may be a race
> > > between the freezer and the delivery of the continuation signal to the task
> > > resulting in the task running after we have finished freezing other tasks.
> > > This, in turn, may lead to undesirable effects up to and including a
> > > corruption of data.
> > > 
> > > To prevent this from happening we first need to make the freezer consider
> > > stopped tasks as freezeable.  For this purpose we need to make freezeable()
> > > stop returning 0 for these tasks.  We must remember, however, that the
> > > stopped tasks need not receive the continuation signal before thaw_processes()
> > > is called, so as soon as PF_FREEZE is set for them try_to_freeze_tasks()
> > > should stop counting them as the ones to wait for.  Additionally, if there's a
> > > traced task (ie. a task in the TASK_TRACED state) the parent of which has
> > > PF_FREEZE set and is stopped, try_to_freeze_tasks() should not wait for it.
> > > Moreover, if there are some stopped tasks that haven't received the continuation
> > > signal before thaw_processes() is called, we must clear PF_FREEZE for them so
> > > that they don't go to the refrigerator when it's no longer desirable.
> > 
> > Actually, what do you think about this patch? It removes special
> > handling of TASK_TRACED, and should do the trick, too...
> 
> I was surprised, but the patch seems to work okay. Can you replace
> your 1/2 with this one, and see what breaks?

I don't think anything will _visibly_ break, because (1) even if the traced
task has TIF_SIGPENDING set unnecessarily, it will just notice there is no
real signal to handle and continue and (2) the race between the delivery of
the continuation signal and the freezer is damn hard to trigger (still I think
I can wirte some artificial code that would trigger this, although it would
involve a kernel thread sending SIGCONT to a user space process - provided
it's permissible ;-)).

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Pavel Machek]

Hi!

> > > Actually, what do you think about this patch? It removes special
> > > handling of TASK_TRACED, and should do the trick, too...
> > 
> > I was surprised, but the patch seems to work okay. Can you replace
> > your 1/2 with this one, and see what breaks?
> 
> I don't think anything will _visibly_ break, because (1) even if the traced
> task has TIF_SIGPENDING set unnecessarily, it will just notice there is no
> real signal to handle and continue and 

We are generating spurious wakeups, but as you noticed, that should be
okay.

>(2) the race between the delivery of
> the continuation signal and the freezer is damn hard to trigger (still I think
> I can wirte some artificial code that would trigger this, although it would
> involve a kernel thread sending SIGCONT to a user space process - provided
> it's permissible ;-)).

It is not really permissible, but I do not see how it would hurt...

With that patch, we put TASK_STOPPED tasks into refrigerator, along
with everyone else. SIGCONT is *not* going to help them out of
refrigerator.

We also ignore TASK_STOPPED now, so we'll not proceed until all
stopped tasks are in refrigerator.

Now... I'm not 100% sure I got the ptrace() cases right (and did not
test that)... but TASK_STOPPED cases look pretty trivial to me now.

							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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Tuesday, 5 December 2006 15:12, Pavel Machek wrote:
> Hi!
> 
> > > > Actually, what do you think about this patch? It removes special
> > > > handling of TASK_TRACED, and should do the trick, too...
> > > 
> > > I was surprised, but the patch seems to work okay. Can you replace
> > > your 1/2 with this one, and see what breaks?
> > 
> > I don't think anything will _visibly_ break, because (1) even if the traced
> > task has TIF_SIGPENDING set unnecessarily, it will just notice there is no
> > real signal to handle and continue and 
> 
> We are generating spurious wakeups, but as you noticed, that should be
> okay.

So we can move the check to freezeable().  Fine.

> >(2) the race between the delivery of
> > the continuation signal and the freezer is damn hard to trigger (still I think
> > I can wirte some artificial code that would trigger this, although it would
> > involve a kernel thread sending SIGCONT to a user space process - provided
> > it's permissible ;-)).
> 
> It is not really permissible, but I do not see how it would hurt...
> 
> With that patch, we put TASK_STOPPED tasks into refrigerator, along
> with everyone else. SIGCONT is *not* going to help them out of
> refrigerator.
> 
> We also ignore TASK_STOPPED now, so we'll not proceed until all
> stopped tasks are in refrigerator.
> 
> Now... I'm not 100% sure I got the ptrace() cases right (and did not
> test that)... but TASK_STOPPED cases look pretty trivial to me now.

Please note that currently (and with your patch) freezeable() returns 0 for
TASK_STOPPED, so we don't set PF_FREEZE for them and my patch changes
_exactly_ that and adds some minor (optional) modifications to the error
paths.

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Tuesday, 5 December 2006 15:26, Rafael J. Wysocki wrote:
> On Tuesday, 5 December 2006 15:12, Pavel Machek wrote:
> > Hi!
> > 
> > > > > Actually, what do you think about this patch? It removes special
> > > > > handling of TASK_TRACED, and should do the trick, too...
> > > > 
> > > > I was surprised, but the patch seems to work okay. Can you replace
> > > > your 1/2 with this one, and see what breaks?
> > > 
> > > I don't think anything will _visibly_ break, because (1) even if the traced
> > > task has TIF_SIGPENDING set unnecessarily, it will just notice there is no
> > > real signal to handle and continue and 
> > 
> > We are generating spurious wakeups, but as you noticed, that should be
> > okay.
> 
> So we can move the check to freezeable().  Fine.

Oops, not so.  We have to reset PF_FREEZE for it too and freezeable() doesn't
do that.

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Pavel Machek]

Hi!

> > >(2) the race between the delivery of
> > > the continuation signal and the freezer is damn hard to trigger (still I think
> > > I can wirte some artificial code that would trigger this, although it would
> > > involve a kernel thread sending SIGCONT to a user space process - provided
> > > it's permissible ;-)).
> > 
> > It is not really permissible, but I do not see how it would hurt...
> > 
> > With that patch, we put TASK_STOPPED tasks into refrigerator, along
> > with everyone else. SIGCONT is *not* going to help them out of
> > refrigerator.
> > 
> > We also ignore TASK_STOPPED now, so we'll not proceed until all
> > stopped tasks are in refrigerator.
> > 
> > Now... I'm not 100% sure I got the ptrace() cases right (and did not
> > test that)... but TASK_STOPPED cases look pretty trivial to me now.
> 
> Please note that currently (and with your patch) freezeable() returns 0 for

I cerainly wanted freezable to return 1 for TASK_STOPPED... and I
think this part of my patch should be doing it...?


diff --git a/kernel/power/process.c b/kernel/power/process.c
index 7bcc976..d56e494 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c                                 
@@ -26,8 +26,7 @@ static inline int freezeable(struct task
            (p->flags & PF_NOFREEZE) ||
            (p->exit_state == EXIT_ZOMBIE) ||
            (p->exit_state == EXIT_DEAD) ||
-           ((p->exit_state == TASK_TRACED) && frozen(p->parent)) ||
-           (p->state == TASK_STOPPED))
+           ((p->exit_state == TASK_TRACED) && frozen(p->parent)))
                return 0;
        return 1;
 }


> TASK_STOPPED, so we don't set PF_FREEZE for them and my patch changes
> _exactly_ that and adds some minor (optional) modifications to the error
> paths.


-- 
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Tuesday, 5 December 2006 16:27, Pavel Machek wrote:
> Hi!
> 
> > > >(2) the race between the delivery of
> > > > the continuation signal and the freezer is damn hard to trigger (still I think
> > > > I can wirte some artificial code that would trigger this, although it would
> > > > involve a kernel thread sending SIGCONT to a user space process - provided
> > > > it's permissible ;-)).
> > > 
> > > It is not really permissible, but I do not see how it would hurt...
> > > 
> > > With that patch, we put TASK_STOPPED tasks into refrigerator, along
> > > with everyone else. SIGCONT is *not* going to help them out of
> > > refrigerator.
> > > 
> > > We also ignore TASK_STOPPED now, so we'll not proceed until all
> > > stopped tasks are in refrigerator.
> > > 
> > > Now... I'm not 100% sure I got the ptrace() cases right (and did not
> > > test that)... but TASK_STOPPED cases look pretty trivial to me now.
> > 
> > Please note that currently (and with your patch) freezeable() returns 0 for
> 
> I cerainly wanted freezable to return 1 for TASK_STOPPED... and I
> think this part of my patch should be doing it...?
> 
> 
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 7bcc976..d56e494 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c                                 
> @@ -26,8 +26,7 @@ static inline int freezeable(struct task
>             (p->flags & PF_NOFREEZE) ||
>             (p->exit_state == EXIT_ZOMBIE) ||
>             (p->exit_state == EXIT_DEAD) ||
> -           ((p->exit_state == TASK_TRACED) && frozen(p->parent)) ||
> -           (p->state == TASK_STOPPED))
> +           ((p->exit_state == TASK_TRACED) && frozen(p->parent)))
>                 return 0;
>         return 1;
>  }

Ah, I didn't notice, sorry.

You should add
(p->exit_state == TASK_TRACED && p->parent->state == TASK_STOPPED) here
too, I think.

Also, have you verified that stopped tasks don't go to the refrigerator after
thaw_processes() is called?

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

Okay, I have replaced my [1/2] with the patch below ...

On Tuesday, 5 December 2006 11:34, Pavel Machek wrote:
> Hi!
> 
> > Currently, if a task is stopped (ie. it's in the TASK_STOPPED state), it is
> > considered by the freezer as unfreezeable.  However, there may be a race
> > between the freezer and the delivery of the continuation signal to the task
> > resulting in the task running after we have finished freezing other tasks.
> > This, in turn, may lead to undesirable effects up to and including a
> > corruption of data.
> > 
> > To prevent this from happening we first need to make the freezer consider
> > stopped tasks as freezeable.  For this purpose we need to make freezeable()
> > stop returning 0 for these tasks.  We must remember, however, that the
> > stopped tasks need not receive the continuation signal before thaw_processes()
> > is called, so as soon as PF_FREEZE is set for them try_to_freeze_tasks()
> > should stop counting them as the ones to wait for.  Additionally, if there's a
> > traced task (ie. a task in the TASK_TRACED state) the parent of which has
> > PF_FREEZE set and is stopped, try_to_freeze_tasks() should not wait for it.
> > Moreover, if there are some stopped tasks that haven't received the continuation
> > signal before thaw_processes() is called, we must clear PF_FREEZE for them so
> > that they don't go to the refrigerator when it's no longer desirable.
> 
> Actually, what do you think about this patch? It removes special
> handling of TASK_TRACED, and should do the trick, too...
> 								Pavel
> 
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 7bcc976..d56e494 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -26,8 +26,7 @@ static inline int freezeable(struct task
>  	    (p->flags & PF_NOFREEZE) ||
>  	    (p->exit_state == EXIT_ZOMBIE) ||
>  	    (p->exit_state == EXIT_DEAD) ||
> -	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)) ||
> -	    (p->state == TASK_STOPPED))
> +	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)))

... with the exception that I haven't added the last line, since there's some
code in try_to_freeze_tasks() that does the same and better ...

>  		return 0;
>  	return 1;
>  }
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9a61944..e305ad1 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1702,7 +1702,9 @@ finish_stop(int stop_count)
>  		read_unlock(&tasklist_lock);
>  	}
>  
> -	schedule();
> +	do {
> +		schedule();
> +	} while (try_to_freeze());
>  	/*
>  	 * Now we don't run again until continued.
>  	 */
> 
> 

... and it fails to freeze processes if there's a stopped task (to verify,
run vi, press ^Z, and try to suspend).

It happens because we shouldn't count the stopped task as freezeable any
more after we've set PF_FREEZE for it and we can fix that by adding

	if (p->state == TASK_STOPPED && freezing(p))
		continue;

to the main loop in try_to_freeze_tasks().  Then we obtain the appended patch.

<tests again>

Now the stopped task doesn't prevent us from freezing processes.  So far so
good.  [To be continued.]

Greetings,
Rafael


Index: linux-2.6.19-rc6-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/kernel/power/process.c	2006-12-05 21:10:00.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/power/process.c	2006-12-05 21:56:57.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,6 +102,9 @@ 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)) {
Index: linux-2.6.19-rc6-mm2/kernel/signal.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/kernel/signal.c	2006-12-05 21:10:00.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/signal.c	2006-12-05 21:11:31.000000000 +0100
@@ -1829,7 +1829,9 @@ finish_stop(int stop_count)
 		read_unlock(&tasklist_lock);
 	}
 
-	schedule();
+	do {
+		schedule();
+	} while (try_to_freeze());
 	/*
 	 * Now we don't run again until continued.
 	 */

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

On Tuesday, 5 December 2006 22:03, Rafael J. Wysocki wrote:
> Okay, I have replaced my [1/2] with the patch below ...
> 
> On Tuesday, 5 December 2006 11:34, Pavel Machek wrote:
<--snip--> 
> ... and it fails to freeze processes if there's a stopped task (to verify,
> run vi, press ^Z, and try to suspend).
> 
> It happens because we shouldn't count the stopped task as freezeable any
> more after we've set PF_FREEZE for it and we can fix that by adding
> 
> 	if (p->state == TASK_STOPPED && freezing(p))
> 		continue;
> 
> to the main loop in try_to_freeze_tasks().  Then we obtain the appended patch.
> 
> <tests again>
> 
> Now the stopped task doesn't prevent us from freezing processes.  So far so
> good.

Unfortunately, after the resume the task that was stopped before the suspend
goes to the refrigerator as soon as it receives the continuation signal (to verify,
run vi, press ^Z, suspend, resume, type fg and look at the output of ps ax -
on another terminal, becase this one will be unuseable ;-)).

Hm.  It looks like we have to clear the freezing request for the stopped task
before we thaw the other processes, but there may be more stopped tasks,
so we need to add something like this:

	read_lock(&tasklist_lock);
	do_each_thread(g, p) {
		if (p->state == TASK_STOPPED && freezing(p))
			cancel_freezing(p);
	} while_each_thread(g, p);
	read_unlock(&tasklist_lock);

to thaw_processes().  It's cleaner to add it as an inline function, though,
and once we've done it, we get the appended patch.

<tests again>

Well, now the task that was stopped before the suspend doesn't go to the
refrigerator when it's received the continuation signal after the resume.
Good, but is it sufficient?  [To be continued.]


Index: linux-2.6.19-rc6-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/kernel/power/process.c	2006-12-05 21:10:00.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/power/process.c	2006-12-05 22:28:56.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,6 +102,9 @@ 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)) {
@@ -185,6 +187,18 @@ int freeze_processes(void)
 	return 0;
 }
 
+static inline void release_stopped_tasks(void)
+{
+	struct task_struct *g, *p;
+
+	read_lock(&tasklist_lock);
+	do_each_thread(g, p) {
+		if (p->state == TASK_STOPPED && 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;
@@ -207,6 +221,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	2006-12-05 21:10:00.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/signal.c	2006-12-05 21:11:31.000000000 +0100
@@ -1829,7 +1829,9 @@ finish_stop(int stop_count)
 		read_unlock(&tasklist_lock);
 	}
 
-	schedule();
+	do {
+		schedule();
+	} while (try_to_freeze());
 	/*
 	 * Now we don't run again until continued.
 	 */

-------------------------------------------------------------------------
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] [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

On Tuesday, 5 December 2006 22:26, Rafael J. Wysocki wrote:
> On Tuesday, 5 December 2006 22:03, Rafael J. Wysocki wrote:
> > Okay, I have replaced my [1/2] with the patch below ...
> > 
> > On Tuesday, 5 December 2006 11:34, Pavel Machek wrote:
<--snip-->
> 
> Well, now the task that was stopped before the suspend doesn't go to the
> refrigerator when it's received the continuation signal after the resume.
> Good, but is it sufficient?

Well, almost.

Namely, if you suspend with a stopped vi and you look at the dmesg output
after the resume, there will be something like this:

Restarting tasks ... <4> Strange, vi not stopped

which means that thaw_tasks() is confused by the lurking stopped task.  We can
fix that by checking if the task in question is not stopped before printing
the message in thaw_tasks() and we get this:

 kernel/power/process.c |   21 ++++++++++++++++++---
 kernel/signal.c        |    4 +++-
 2 files changed, 21 insertions(+), 4 deletions(-)

Index: linux-2.6.19-rc6-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/kernel/power/process.c	2006-12-05 21:10:00.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/power/process.c	2006-12-05 22:44:45.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,6 +102,9 @@ 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)) {
@@ -185,6 +187,18 @@ int freeze_processes(void)
 	return 0;
 }
 
+static inline void release_stopped_tasks(void)
+{
+	struct task_struct *g, *p;
+
+	read_lock(&tasklist_lock);
+	do_each_thread(g, p) {
+		if (p->state == TASK_STOPPED && 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,7 +211,7 @@ static void thaw_tasks(int thaw_user_spa
 		if (is_user_space(p) == !thaw_user_space)
 			continue;
 
-		if (!thaw_process(p))
+		if (!thaw_process(p) && p->state != TASK_STOPPED)
 			printk(KERN_WARNING " Strange, %s not stopped\n",
 				p->comm );
 	} while_each_thread(g, p);
@@ -207,6 +221,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	2006-12-05 21:10:00.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/signal.c	2006-12-05 21:11:31.000000000 +0100
@@ -1829,7 +1829,9 @@ finish_stop(int stop_count)
 		read_unlock(&tasklist_lock);
 	}
 
-	schedule();
+	do {
+		schedule();
+	} while (try_to_freeze());
 	/*
 	 * Now we don't run again until continued.
 	 */

But now it looks almost like my [1/2], doesn't it? ;-)

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

Re: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Pavel Machek]

Hi!

> > Actually, what do you think about this patch? It removes special
> > handling of TASK_TRACED, and should do the trick, too...
> > 
> > diff --git a/kernel/power/process.c b/kernel/power/process.c
> > index 7bcc976..d56e494 100644
> > --- a/kernel/power/process.c
> > +++ b/kernel/power/process.c
> > @@ -26,8 +26,7 @@ static inline int freezeable(struct task
> >  	    (p->flags & PF_NOFREEZE) ||
> >  	    (p->exit_state == EXIT_ZOMBIE) ||
> >  	    (p->exit_state == EXIT_DEAD) ||
> > -	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)) ||
> > -	    (p->state == TASK_STOPPED))
> > +	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)))
> 
> ... with the exception that I haven't added the last line, since there's some
> code in try_to_freeze_tasks() that does the same and better ...

> >  		return 0;
> >  	return 1;
> >  }
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index 9a61944..e305ad1 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1702,7 +1702,9 @@ finish_stop(int stop_count)
> >  		read_unlock(&tasklist_lock);
> >  	}
> >  
> > -	schedule();
> > +	do {
> > +		schedule();
> > +	} while (try_to_freeze());
> >  	/*
> >  	 * Now we don't run again until continued.
> >  	 */
> > 
> > 
> 
> ... and it fails to freeze processes if there's a stopped task (to verify,
> run vi, press ^Z, and try to suspend).

Huh, I thought I've tested that. Too bad, you are right.

> It happens because we shouldn't count the stopped task as freezeable any
> more after we've set PF_FREEZE for it and we can fix that by adding
> 
> 	if (p->state == TASK_STOPPED && freezing(p))
> 		continue;

No, I'd actually like to force stopped task into refrigerator.

									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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

On Tuesday, 5 December 2006 23:19, Pavel Machek wrote:
> Hi!
> 
> > > Actually, what do you think about this patch? It removes special
> > > handling of TASK_TRACED, and should do the trick, too...
> > > 
> > > diff --git a/kernel/power/process.c b/kernel/power/process.c
> > > index 7bcc976..d56e494 100644
> > > --- a/kernel/power/process.c
> > > +++ b/kernel/power/process.c
> > > @@ -26,8 +26,7 @@ static inline int freezeable(struct task
> > >  	    (p->flags & PF_NOFREEZE) ||
> > >  	    (p->exit_state == EXIT_ZOMBIE) ||
> > >  	    (p->exit_state == EXIT_DEAD) ||
> > > -	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)) ||
> > > -	    (p->state == TASK_STOPPED))
> > > +	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)))
> > 
> > ... with the exception that I haven't added the last line, since there's some
> > code in try_to_freeze_tasks() that does the same and better ...
> 
> > >  		return 0;
> > >  	return 1;
> > >  }
> > > diff --git a/kernel/signal.c b/kernel/signal.c
> > > index 9a61944..e305ad1 100644
> > > --- a/kernel/signal.c
> > > +++ b/kernel/signal.c
> > > @@ -1702,7 +1702,9 @@ finish_stop(int stop_count)
> > >  		read_unlock(&tasklist_lock);
> > >  	}
> > >  
> > > -	schedule();
> > > +	do {
> > > +		schedule();
> > > +	} while (try_to_freeze());
> > >  	/*
> > >  	 * Now we don't run again until continued.
> > >  	 */
> > > 
> > > 
> > 
> > ... and it fails to freeze processes if there's a stopped task (to verify,
> > run vi, press ^Z, and try to suspend).
> 
> Huh, I thought I've tested that. Too bad, you are right.
> 
> > It happens because we shouldn't count the stopped task as freezeable any
> > more after we've set PF_FREEZE for it and we can fix that by adding
> > 
> > 	if (p->state == TASK_STOPPED && freezing(p))
> > 		continue;
> 
> No, I'd actually like to force stopped task into refrigerator.

Well, we'd have to send SIGCONT to them for this purpose and then stop them
back again during the resume.  Doesn't sound nice ...

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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] [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Nigel Cunningham]

Hi.

On Tue, 2006-12-05 at 23:18 +0100, Rafael J. Wysocki wrote:
> > > It happens because we shouldn't count the stopped task as freezeable any
> > > more after we've set PF_FREEZE for it and we can fix that by adding
> > > 
> > > 	if (p->state == TASK_STOPPED && freezing(p))
> > > 		continue;
> > 
> > No, I'd actually like to force stopped task into refrigerator.
> 
> Well, we'd have to send SIGCONT to them for this purpose and then stop them
> back again during the resume.  Doesn't sound nice ...

I agree with Rafael. Contingencies for handling them being restarted by
other means is one thing, but restarting tasks that were stopped is
wrong. Who knows what work they might do between the SIGCONT and
freezing that (and this is the key point) the user doesn't want them to
do?

Regards,

Nigel


-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Pavel Machek]

Hi!

> ... and it fails to freeze processes if there's a stopped task (to verify,
> run vi, press ^Z, and try to suspend).

Ok, here's better version. (Notice it only differs by one bit ;-).

Ok, something is still weird. Bash reports spurious...

[2]+  Stopped                 vi

...after resume. But I think it is right approach. We want to store
all the tasks in refrigerator.
								Pavel


diff --git a/kernel/power/process.c b/kernel/power/process.c
index 7bcc976..9849d88 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -26,8 +26,7 @@ static inline int freezeable(struct task
 	    (p->flags & PF_NOFREEZE) ||
 	    (p->exit_state == EXIT_ZOMBIE) ||
 	    (p->exit_state == EXIT_DEAD) ||
-	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)) ||
-	    (p->state == TASK_STOPPED))
+	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)))
 		return 0;
 	return 1;
 }
@@ -62,7 +61,7 @@ static inline void freeze_process(struct
 	if (!freezing(p)) {
 		freeze(p);
 		spin_lock_irqsave(&p->sighand->siglock, flags);
-		signal_wake_up(p, 0);
+		signal_wake_up(p, 1);
 		spin_unlock_irqrestore(&p->sighand->siglock, flags);
 	}
 }
diff --git a/kernel/signal.c b/kernel/signal.c
index 9a61944..e305ad1 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1702,7 +1702,9 @@ finish_stop(int stop_count)
 		read_unlock(&tasklist_lock);
 	}
 
-	schedule();
+	do {
+		schedule();
+	} while (try_to_freeze());
 	/*
 	 * Now we don't run again until continued.
 	 */

									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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Tuesday, 5 December 2006 23:36, Pavel Machek wrote:
> Hi!
> 
> > ... and it fails to freeze processes if there's a stopped task (to verify,
> > run vi, press ^Z, and try to suspend).
> 
> Ok, here's better version. (Notice it only differs by one bit ;-).
> 
> Ok, something is still weird. Bash reports spurious...
> 
> [2]+  Stopped                 vi
> 
> ...after resume.

This is because of how signal_wake_up() works, I think..

> But I think it is right approach.

Fine by me with a couple of remarks below.

> We want to store all the tasks in refrigerator.
> 								Pavel
> 
> 
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 7bcc976..9849d88 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -26,8 +26,7 @@ static inline int freezeable(struct task
>  	    (p->flags & PF_NOFREEZE) ||
>  	    (p->exit_state == EXIT_ZOMBIE) ||
>  	    (p->exit_state == EXIT_DEAD) ||
> -	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)) ||
> -	    (p->state == TASK_STOPPED))
> +	    ((p->exit_state == TASK_TRACED) && frozen(p->parent)))

This doesn't apply to -mm as well as to -stable, -git, whatever.  There's no

((p->exit_state == TASK_TRACED) && frozen(p->parent)) ||

line here in any of these kernels.  Instead, there is a check in
try_to_freeze_tasks() that does this.

>  		return 0;
>  	return 1;
>  }
> @@ -62,7 +61,7 @@ static inline void freeze_process(struct
>  	if (!freezing(p)) {
>  		freeze(p);
>  		spin_lock_irqsave(&p->sighand->siglock, flags);
> -		signal_wake_up(p, 0);
> +		signal_wake_up(p, 1);

Do we want traced tasks to get kicked here?  If not, we'd have to write our
own signal_wake_up() (no big deal, I think) or modify the existing one to be
able to kick TASK_STOPPED without kicking TASK_TRACED.

>  		spin_unlock_irqrestore(&p->sighand->siglock, flags);
>  	}
>  }
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9a61944..e305ad1 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1702,7 +1702,9 @@ finish_stop(int stop_count)
>  		read_unlock(&tasklist_lock);
>  	}
>  
> -	schedule();
> +	do {
> +		schedule();
> +	} while (try_to_freeze());
>  	/*
>  	 * Now we don't run again until continued.
>  	 */

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Tuesday, 5 December 2006 23:45, Rafael J. Wysocki wrote:
> On Tuesday, 5 December 2006 23:36, Pavel Machek wrote:
> > Hi!
> > 
> > > ... and it fails to freeze processes if there's a stopped task (to verify,
> > > run vi, press ^Z, and try to suspend).
> > 
> > Ok, here's better version. (Notice it only differs by one bit ;-).
> > 
> > Ok, something is still weird. Bash reports spurious...
> > 
> > [2]+  Stopped                 vi
> > 
> > ...after resume.
> 
> This is because of how signal_wake_up() works, I think..
> 
> > But I think it is right approach.

Okay, with the appended patch applied everything seems to work and I don't
see any undesirable side-effects.

Greetings,
Rafael


 kernel/power/process.c |    8 +++++---
 kernel/signal.c        |    4 +++-
 2 files changed, 8 insertions(+), 4 deletions(-)

Index: linux-2.6.19-rc6-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/kernel/power/process.c	2006-12-05 21:10:00.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/power/process.c	2006-12-06 00:14:21.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;
 }
@@ -63,7 +62,10 @@ static inline void freeze_process(struct
 	if (!freezing(p)) {
 		freeze(p);
 		spin_lock_irqsave(&p->sighand->siglock, flags);
-		signal_wake_up(p, 0);
+		set_tsk_thread_flag(p, TIF_SIGPENDING);
+		if (!wake_up_state(p, TASK_INTERRUPTIBLE | TASK_STOPPED))
+			kick_process(p);
+
 		spin_unlock_irqrestore(&p->sighand->siglock, flags);
 	}
 }
Index: linux-2.6.19-rc6-mm2/kernel/signal.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/kernel/signal.c	2006-12-05 21:10:00.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/signal.c	2006-12-05 21:11:31.000000000 +0100
@@ -1829,7 +1829,9 @@ finish_stop(int stop_count)
 		read_unlock(&tasklist_lock);
 	}
 
-	schedule();
+	do {
+		schedule();
+	} while (try_to_freeze());
 	/*
 	 * Now we don't run again until continued.
 	 */

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Pavel Machek]

Hi!

> > > ...after resume.
> > 
> > This is because of how signal_wake_up() works, I think..
> > 
> > > But I think it is right approach.
> 
> Okay, with the appended patch applied everything seems to work and I don't
> see any undesirable side-effects.

I promise to try it... tommorow. Looks very good to me.
									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: [Suspend-devel] [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Nigel Cunningham]

Hi guys.

On Wed, 2006-12-06 at 00:45 +0100, Pavel Machek wrote:
> Hi!
> 
> > > > ...after resume.
> > > 
> > > This is because of how signal_wake_up() works, I think..
> > > 
> > > > But I think it is right approach.
> > 
> > Okay, with the appended patch applied everything seems to work and I don't
> > see any undesirable side-effects.
> 
> I promise to try it... tommorow. Looks very good to me.

Just to say sorry for my lack of interaction / feedback. Too busy with
other stuff at the moment, but I'm trying to keep track :)

Nigel

Re: [Suspend-devel] [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

On Wednesday, 6 December 2006 00:45, Pavel Machek wrote:
> Hi!
> 
> > > > ...after resume.
> > > 
> > > This is because of how signal_wake_up() works, I think..
> > > 
> > > > But I think it is right approach.
> > 
> > Okay, with the appended patch applied everything seems to work and I don't
> > see any undesirable side-effects.
> 
> I promise to try it... tommorow. Looks very good to me.

Unfortunately there's one problem with it.

To reproduce it I run "gdb /bin/cat", execute "run" in gdb and press ^Z twice
to stop both processes.  Next I suspend and resume and run "fg" to get the gdb
back.and press "Enter" to get the prompt.  Then, it turns out that the
terminal echo doesn't work and "Enter" doesn't make it go to the next line.
I can recover from this state by typing "fg+Enter" (with no echo) so that
/bin/cat gets the continuation signal and it restores the terminal settings,
apparently.

This means, however, that with this patch the behavior of a process (gdb)
after the resume may be different to its normal behavior, which is wrong.

With my original [1/2] this problem doesn't appear (ie. after the resume gdb
behaves normally).  Thus I think that, although this patch is much more
elegant, my original [1/2] is a safer solution, because we can say exactly
what it does.

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

Re: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Pavel Machek]

Hi!

> > > > > ...after resume.
> > > > 
> > > > This is because of how signal_wake_up() works, I think..
> > > > 
> > > > > But I think it is right approach.
> > > 
> > > Okay, with the appended patch applied everything seems to work and I don't
> > > see any undesirable side-effects.
> > 
> > I promise to try it... tommorow. Looks very good to me.
> 
> Unfortunately there's one problem with it.

Well, IIRC it still had the 'bash reports vi stopped twice' problem.

> To reproduce it I run "gdb /bin/cat", execute "run" in gdb and press ^Z twice
> to stop both processes.  Next I suspend and resume and run "fg" to get the gdb
> back.and press "Enter" to get the prompt.  Then, it turns out that the
> terminal echo doesn't work and "Enter" doesn't make it go to the next line.
> I can recover from this state by typing "fg+Enter" (with no echo) so that
> /bin/cat gets the continuation signal and it restores the terminal settings,
> apparently.

I wonder if we should start a test suite ;-).

> This means, however, that with this patch the behavior of a process (gdb)
> after the resume may be different to its normal behavior, which is wrong.

Yep.

> With my original [1/2] this problem doesn't appear (ie. after the resume gdb
> behaves normally).  Thus I think that, although this patch is much more
> elegant, my original [1/2] is a safer solution, because we can say exactly
> what it does.

Original 1/2 indeed is 'safer'... but it is also too ugly to live.
Getting this to work should not be that hard, and I'd prefer not to
add more tricky code to already-tricky process.c.

In the meantime, perhaps we want 2/2 merged? That one was
safe&obvious.
							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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Friday, 8 December 2006 12:21, Pavel Machek wrote:
> Hi!
> 
> > > > > > ...after resume.
> > > > > 
> > > > > This is because of how signal_wake_up() works, I think..
> > > > > 
> > > > > > But I think it is right approach.
> > > > 
> > > > Okay, with the appended patch applied everything seems to work and I don't
> > > > see any undesirable side-effects.
> > > 
> > > I promise to try it... tommorow. Looks very good to me.
> > 
> > Unfortunately there's one problem with it.
> 
> Well, IIRC it still had the 'bash reports vi stopped twice' problem.

It doesn't do that for me, but ...

> > To reproduce it I run "gdb /bin/cat", execute "run" in gdb and press ^Z twice
> > to stop both processes.  Next I suspend and resume and run "fg" to get the gdb
> > back.and press "Enter" to get the prompt.  Then, it turns out that the
> > terminal echo doesn't work and "Enter" doesn't make it go to the next line.
> > I can recover from this state by typing "fg+Enter" (with no echo) so that
> > /bin/cat gets the continuation signal and it restores the terminal settings,
> > apparently.
> 
> I wonder if we should start a test suite ;-).
> 
> > This means, however, that with this patch the behavior of a process (gdb)
> > after the resume may be different to its normal behavior, which is wrong.
> 
> Yep.
> 
> > With my original [1/2] this problem doesn't appear (ie. after the resume gdb
> > behaves normally).  Thus I think that, although this patch is much more
> > elegant, my original [1/2] is a safer solution, because we can say exactly
> > what it does.
> 
> Original 1/2 indeed is 'safer'... but it is also too ugly to live.

Well, I don't agree with that. :-)

> Getting this to work should not be that hard,

But it would involve messing up with the scheduler, no?

> and I'd prefer not to add more tricky code to already-tricky process.c.
> 
> In the meantime, perhaps we want 2/2 merged? That one was
> safe&obvious.

Sort of.  Except I still don't know which architectures are supposed to use
the freezer ...

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Friday, 8 December 2006 12:49, Rafael J. Wysocki wrote:
> On Friday, 8 December 2006 12:21, Pavel Machek wrote:
> > Hi!
> > 
> > > > > > > ...after resume.
> > > > > > 
> > > > > > This is because of how signal_wake_up() works, I think..
> > > > > > 
> > > > > > > But I think it is right approach.
> > > > > 
> > > > > Okay, with the appended patch applied everything seems to work and I don't
> > > > > see any undesirable side-effects.
> > > > 
> > > > I promise to try it... tommorow. Looks very good to me.
> > > 
> > > Unfortunately there's one problem with it.
> > 
> > Well, IIRC it still had the 'bash reports vi stopped twice' problem.
> 
> It doesn't do that for me, but ...
> 
> > > To reproduce it I run "gdb /bin/cat", execute "run" in gdb and press ^Z twice
> > > to stop both processes.  Next I suspend and resume and run "fg" to get the gdb
> > > back.and press "Enter" to get the prompt.  Then, it turns out that the
> > > terminal echo doesn't work and "Enter" doesn't make it go to the next line.
> > > I can recover from this state by typing "fg+Enter" (with no echo) so that
> > > /bin/cat gets the continuation signal and it restores the terminal settings,
> > > apparently.
> > 
> > I wonder if we should start a test suite ;-).
> > 
> > > This means, however, that with this patch the behavior of a process (gdb)
> > > after the resume may be different to its normal behavior, which is wrong.
> > 
> > Yep.
> > 
> > > With my original [1/2] this problem doesn't appear (ie. after the resume gdb
> > > behaves normally).  Thus I think that, although this patch is much more
> > > elegant, my original [1/2] is a safer solution, because we can say exactly
> > > what it does.
> > 
> > Original 1/2 indeed is 'safer'... but it is also too ugly to live.
> 
> Well, I don't agree with that. :-)
> 
> > Getting this to work should not be that hard,
> 
> But it would involve messing up with the scheduler, no?
> 
> > and I'd prefer not to add more tricky code to already-tricky process.c.

BTW, I think the alternative is even more tricky than [1/2] itself, although
the trickiness is not clearly visible in this case.

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Friday, 8 December 2006 12:49, Rafael J. Wysocki wrote:
> On Friday, 8 December 2006 12:21, Pavel Machek wrote:
> > Hi!
> > 
> > > > > > > ...after resume.
> > > > > > 
> > > > > > This is because of how signal_wake_up() works, I think..
> > > > > > 
> > > > > > > But I think it is right approach.
> > > > > 
> > > > > Okay, with the appended patch applied everything seems to work and I don't
> > > > > see any undesirable side-effects.
> > > > 
> > > > I promise to try it... tommorow. Looks very good to me.
> > > 
> > > Unfortunately there's one problem with it.
> > 
> > Well, IIRC it still had the 'bash reports vi stopped twice' problem.
> 
> It doesn't do that for me, but ...
> 
> > > To reproduce it I run "gdb /bin/cat", execute "run" in gdb and press ^Z twice
> > > to stop both processes.  Next I suspend and resume and run "fg" to get the gdb
> > > back.and press "Enter" to get the prompt.  Then, it turns out that the
> > > terminal echo doesn't work and "Enter" doesn't make it go to the next line.
> > > I can recover from this state by typing "fg+Enter" (with no echo) so that
> > > /bin/cat gets the continuation signal and it restores the terminal settings,
> > > apparently.
> > 
> > I wonder if we should start a test suite ;-).
> > 
> > > This means, however, that with this patch the behavior of a process (gdb)
> > > after the resume may be different to its normal behavior, which is wrong.
> > 
> > Yep.

Okay, I think I know what to do so that it works.  The above symptoms are not
present with the appended patch.

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/process.c |   12 ++++++------
 kernel/signal.c        |    4 +++-
 2 files changed, 9 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	2006-12-08 21:15:18.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/power/process.c	2006-12-08 21:47:23.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,9 +60,12 @@ static inline void freeze_process(struct
 	unsigned long flags;
 
 	if (!freezing(p)) {
+		if (p->state == TASK_STOPPED)
+			force_sig_specific(SIGSTOP, p);
+
 		freeze(p);
 		spin_lock_irqsave(&p->sighand->siglock, flags);
-		signal_wake_up(p, 0);
+		signal_wake_up(p, p->state == TASK_STOPPED);
 		spin_unlock_irqrestore(&p->sighand->siglock, flags);
 	}
 }
@@ -103,9 +105,7 @@ 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 (p->state == TASK_TRACED && frozen(p->parent)) {
 				cancel_freezing(p);
 				continue;
 			}
Index: linux-2.6.19-rc6-mm2/kernel/signal.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/kernel/signal.c	2006-12-08 21:15:18.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/signal.c	2006-12-08 21:15:30.000000000 +0100
@@ -1829,7 +1829,9 @@ finish_stop(int stop_count)
 		read_unlock(&tasklist_lock);
 	}
 
-	schedule();
+	do {
+		schedule();
+	} while (try_to_freeze());
 	/*
 	 * Now we don't run again until continued.
 	 */

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Pavel Machek]

Hi!

> > > I wonder if we should start a test suite ;-).
> > > 
> > > > This means, however, that with this patch the behavior of a process (gdb)
> > > > after the resume may be different to its normal behavior, which is wrong.
> > > 
> > > Yep.
> 
> Okay, I think I know what to do so that it works.  The above symptoms are not
> present with the appended patch.

Looks good to me. Thanks for you work!
									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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Saturday, 9 December 2006 00:36, Pavel Machek wrote:
> Hi!
> 
> > > > I wonder if we should start a test suite ;-).
> > > > 
> > > > > This means, however, that with this patch the behavior of a process (gdb)
> > > > > after the resume may be different to its normal behavior, which is wrong.
> > > > 
> > > > Yep.
> > 
> > Okay, I think I know what to do so that it works.  The above symptoms are not
> > present with the appended patch.
> 
> Looks good to me. Thanks for you work!

Well, there's still a race possible in there, if SIGCONT comes after we have
forced the SIGSTOP and before we call signal_wake_up().

I think we should force the SIGSTOP and call signal_wake_up() without
releasing the lock.  I'll try to do something along these lines later today.

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Saturday, 9 December 2006 16:35, Rafael J. Wysocki wrote:
> On Saturday, 9 December 2006 00:36, Pavel Machek wrote:
> > Hi!
> > 
> > > > > I wonder if we should start a test suite ;-).
> > > > > 
> > > > > > This means, however, that with this patch the behavior of a process (gdb)
> > > > > > after the resume may be different to its normal behavior, which is wrong.
> > > > > 
> > > > > Yep.
> > > 
> > > Okay, I think I know what to do so that it works.  The above symptoms are not
> > > present with the appended patch.
> > 
> > Looks good to me. Thanks for you work!
> 
> Well, there's still a race possible in there, if SIGCONT comes after we have
> forced the SIGSTOP and before we call signal_wake_up().
> 
> I think we should force the SIGSTOP and call signal_wake_up() without
> releasing the lock.  I'll try to do something along these lines later today.

I think something like the appended patch will do.

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/sched.h  |    1 +
 kernel/power/process.c |   15 ++++++++-------
 kernel/signal.c        |   16 +++++++++++++++-
 3 files changed, 24 insertions(+), 8 deletions(-)

Index: linux-2.6.19-rc6-mm2/kernel/power/process.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/kernel/power/process.c	2006-12-08 23:20:35.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/power/process.c	2006-12-10 00:37:24.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,9 +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);
+		freeze(p);
+		if (p->state == TASK_STOPPED) {
+			force_sigstop_and_wake_up(p);
+		} else {
+			signal_wake_up(p, 0);
+		}
 		spin_unlock_irqrestore(&p->sighand->siglock, flags);
 	}
 }
@@ -103,9 +106,7 @@ 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 (p->state == TASK_TRACED && frozen(p->parent)) {
 				cancel_freezing(p);
 				continue;
 			}
Index: linux-2.6.19-rc6-mm2/kernel/signal.c
===================================================================
--- linux-2.6.19-rc6-mm2.orig/kernel/signal.c	2006-12-08 23:20:35.000000000 +0100
+++ linux-2.6.19-rc6-mm2/kernel/signal.c	2006-12-10 00:43:39.000000000 +0100
@@ -962,6 +962,18 @@ force_sig_specific(int sig, struct task_
 }
 
 /*
+ * Force a SIGSTOP and make the task wake up.
+ *
+ * This is needed for the freezing of stopped tasks, because we want them to
+ * enter the refrigerator and be stopped again immediately after leaving it.
+ */
+void force_sigstop_and_wake_up(struct task_struct *t)
+{
+	specific_send_sig_info(SIGSTOP, SEND_SIG_FORCED, t);
+	signal_wake_up(t, 1);
+}
+
+/*
  * Test if P wants to take SIG.  After we've checked all threads with this,
  * it's equivalent to finding no threads not blocking SIG.  Any threads not
  * blocking SIG were ruled out because they are not running and already
@@ -1829,7 +1841,9 @@ finish_stop(int stop_count)
 		read_unlock(&tasklist_lock);
 	}
 
-	schedule();
+	do {
+		schedule();
+	} while (try_to_freeze());
 	/*
 	 * Now we don't run again until continued.
 	 */
Index: linux-2.6.19-rc6-mm2/include/linux/sched.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/linux/sched.h	2006-12-10 00:20:07.000000000 +0100
+++ linux-2.6.19-rc6-mm2/include/linux/sched.h	2006-12-10 00:42:44.000000000 +0100
@@ -1350,6 +1350,7 @@ extern int kill_pg_info(int, struct sigi
 extern void do_notify_parent(struct task_struct *, int);
 extern void force_sig(int, struct task_struct *);
 extern void force_sig_specific(int, struct task_struct *);
+extern void force_sigstop_and_wake_up(struct task_struct *t);
 extern int send_sig(int, struct task_struct *, int);
 extern void zap_other_threads(struct task_struct *p);
 extern int kill_pg(pid_t, int, int);

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Sunday, 10 December 2006 01:27, Rafael J. Wysocki wrote:
> On Saturday, 9 December 2006 16:35, Rafael J. Wysocki wrote:
> > On Saturday, 9 December 2006 00:36, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > > > I wonder if we should start a test suite ;-).
> > > > > > 
> > > > > > > This means, however, that with this patch the behavior of a process (gdb)
> > > > > > > after the resume may be different to its normal behavior, which is wrong.
> > > > > > 
> > > > > > Yep.
> > > > 
> > > > Okay, I think I know what to do so that it works.  The above symptoms are not
> > > > present with the appended patch.
> > > 
> > > Looks good to me. Thanks for you work!
> > 
> > Well, there's still a race possible in there, if SIGCONT comes after we have
> > forced the SIGSTOP and before we call signal_wake_up().
> > 
> > I think we should force the SIGSTOP and call signal_wake_up() without
> > releasing the lock.  I'll try to do something along these lines later today.
> 
> I think something like the appended patch will do.
> 
> Greetings,
> Rafael
> 
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  include/linux/sched.h  |    1 +
>  kernel/power/process.c |   15 ++++++++-------
>  kernel/signal.c        |   16 +++++++++++++++-
>  3 files changed, 24 insertions(+), 8 deletions(-)
> 
> Index: linux-2.6.19-rc6-mm2/kernel/power/process.c
> ===================================================================
> --- linux-2.6.19-rc6-mm2.orig/kernel/power/process.c	2006-12-08 23:20:35.000000000 +0100
> +++ linux-2.6.19-rc6-mm2/kernel/power/process.c	2006-12-10 00:37:24.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,9 +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);
> +		freeze(p);
> +		if (p->state == TASK_STOPPED) {
> +			force_sigstop_and_wake_up(p);
> +		} else {
> +			signal_wake_up(p, 0);
> +		}
>  		spin_unlock_irqrestore(&p->sighand->siglock, flags);
>  	}
>  }

Ah, freeze(p) need not be under the lock.

Besides, I'm now thinking the previous version is also correct, because even
if SIGCONT comes after we have forced SIGSTOP, it will remove the SIGSTOP
from the queue and the task's state will change to TASK_RUNNING, so the
next signal_wake_up() will do what it should.

I prefer that one, because it's shorter and doesn't affect sched.h. ;-)

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Pavel Machek]

Hi!

> Ah, freeze(p) need not be under the lock.
> 
> Besides, I'm now thinking the previous version is also correct, because even
> if SIGCONT comes after we have forced SIGSTOP, it will remove the SIGSTOP
> from the queue and the task's state will change to TASK_RUNNING, so the
> next signal_wake_up() will do what it should.
> 
> I prefer that one, because it's shorter and doesn't affect sched.h. ;-)

Okay... consider your latest patches ACKed. (But it would be nice to
make them wait in -mm to 2.6.21, I believe this needs testing).
								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: [PATCH -mm 1/2]: PM: Fix handling of stopped tasks

[Rafael J. Wysocki]

Hi,

On Friday, 8 December 2006 12:49, Rafael J. Wysocki wrote:
> On Friday, 8 December 2006 12:21, Pavel Machek wrote:
> > Hi!
> > 
<--snip--> 
> > In the meantime, perhaps we want 2/2 merged? That one was
> > safe&obvious.
> 
> Sort of.  Except I still don't know which architectures are supposed to use
> the freezer ...

I think it's reasonable to define TIF_FREEZE for the architectures that
include kernel/power/Kconfig into their Kconfigs. :-)

The appended patch does this (applies on top of
http://lists.osdl.org/pipermail/linux-pm/2006-December/004307.html).

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/asm-arm/thread_info.h     |    2 ++
 include/asm-frv/thread_info.h     |    2 ++
 include/asm-i386/thread_info.h    |    2 ++
 include/asm-ia64/thread_info.h    |    2 ++
 include/asm-powerpc/thread_info.h |    2 ++
 include/asm-sh/thread_info.h      |    2 ++
 include/asm-x86_64/thread_info.h  |    2 ++
 include/linux/freezer.h           |   11 ++++++-----
 include/linux/sched.h             |    1 -
 kernel/power/process.c            |   17 ++++++++++-------
 10 files changed, 30 insertions(+), 13 deletions(-)

Index: linux-2.6.19-rc6-mm2/include/asm-arm/thread_info.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/asm-arm/thread_info.h
+++ linux-2.6.19-rc6-mm2/include/asm-arm/thread_info.h
@@ -137,6 +137,7 @@ extern void iwmmxt_task_switch(struct th
 #define TIF_POLLING_NRFLAG	16
 #define TIF_USING_IWMMXT	17
 #define TIF_MEMDIE		18
+#define TIF_FREEZE		19
 
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
 #define _TIF_SIGPENDING		(1 << TIF_SIGPENDING)
@@ -144,6 +145,7 @@ extern void iwmmxt_task_switch(struct th
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_USING_IWMMXT	(1 << TIF_USING_IWMMXT)
+#define _TIF_FREEZE		(1 << TIF_FREEZE)
 
 /*
  * Change these and you break ASM code in entry-common.S
Index: linux-2.6.19-rc6-mm2/include/asm-ia64/thread_info.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/asm-ia64/thread_info.h
+++ linux-2.6.19-rc6-mm2/include/asm-ia64/thread_info.h
@@ -88,6 +88,7 @@ struct thread_info {
 #define TIF_MEMDIE		17
 #define TIF_MCA_INIT		18	/* this task is processing MCA or INIT */
 #define TIF_DB_DISABLED		19	/* debug trap disabled for fsyscall */
+#define TIF_FREEZE		20	/* is freezing for suspend */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
@@ -98,6 +99,7 @@ struct thread_info {
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_MCA_INIT		(1 << TIF_MCA_INIT)
 #define _TIF_DB_DISABLED	(1 << TIF_DB_DISABLED)
+#define _TIF_FREEZE		(1 << TIF_FREEZE)
 
 /* "work to do on user-return" bits */
 #define TIF_ALLWORK_MASK	(_TIF_NOTIFY_RESUME|_TIF_SIGPENDING|_TIF_NEED_RESCHED|_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT)
Index: linux-2.6.19-rc6-mm2/include/asm-sh/thread_info.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/asm-sh/thread_info.h
+++ linux-2.6.19-rc6-mm2/include/asm-sh/thread_info.h
@@ -106,6 +106,7 @@ static inline struct thread_info *curren
 #define TIF_USEDFPU		16	/* FPU was used by this task this quantum (SMP) */
 #define TIF_POLLING_NRFLAG	17	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE		18
+#define TIF_FREEZE		19
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
@@ -114,6 +115,7 @@ static inline struct thread_info *curren
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
 #define _TIF_USEDFPU		(1<<TIF_USEDFPU)
 #define _TIF_POLLING_NRFLAG	(1<<TIF_POLLING_NRFLAG)
+#define _TIF_FREEZE		(1<<TIF_FREEZE)
 
 #define _TIF_WORK_MASK		0x000000FE	/* work to do on interrupt/exception return */
 #define _TIF_ALLWORK_MASK	0x000000FF	/* work to do on any return to u-space */
Index: linux-2.6.19-rc6-mm2/include/asm-frv/thread_info.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/asm-frv/thread_info.h
+++ linux-2.6.19-rc6-mm2/include/asm-frv/thread_info.h
@@ -116,6 +116,7 @@ register struct thread_info *__current_t
 #define TIF_RESTORE_SIGMASK	6	/* restore signal mask in do_signal() */
 #define TIF_POLLING_NRFLAG	16	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE		17	/* OOM killer killed process */
+#define TIF_FREEZE		18	/* freezing for suspend */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
@@ -125,6 +126,7 @@ register struct thread_info *__current_t
 #define _TIF_IRET		(1 << TIF_IRET)
 #define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
+#define _TIF_FREEZE		(1 << TIF_FREEZE)
 
 #define _TIF_WORK_MASK		0x0000FFFE	/* work to do on interrupt/exception return */
 #define _TIF_ALLWORK_MASK	0x0000FFFF	/* work to do on any return to u-space */
Index: linux-2.6.19-rc6-mm2/include/asm-i386/thread_info.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/asm-i386/thread_info.h
+++ linux-2.6.19-rc6-mm2/include/asm-i386/thread_info.h
@@ -134,6 +134,7 @@ static inline struct thread_info *curren
 #define TIF_MEMDIE		16
 #define TIF_DEBUG		17	/* uses debug registers */
 #define TIF_IO_BITMAP		18	/* uses I/O bitmap */
+#define TIF_FREEZE		19	/* is freezing for suspend */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
@@ -147,6 +148,7 @@ static inline struct thread_info *curren
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
 #define _TIF_DEBUG		(1<<TIF_DEBUG)
 #define _TIF_IO_BITMAP		(1<<TIF_IO_BITMAP)
+#define _TIF_FREEZE		(1<<TIF_FREEZE)
 
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK \
Index: linux-2.6.19-rc6-mm2/include/asm-powerpc/thread_info.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/asm-powerpc/thread_info.h
+++ linux-2.6.19-rc6-mm2/include/asm-powerpc/thread_info.h
@@ -122,6 +122,7 @@ static inline struct thread_info *curren
 #define TIF_RESTOREALL		12	/* Restore all regs (implies NOERROR) */
 #define TIF_NOERROR		14	/* Force successful syscall return */
 #define TIF_RESTORE_SIGMASK	15	/* Restore signal mask in do_signal */
+#define TIF_FREEZE		16	/* Freezing for suspend */
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -138,6 +139,7 @@ static inline struct thread_info *curren
 #define _TIF_RESTOREALL		(1<<TIF_RESTOREALL)
 #define _TIF_NOERROR		(1<<TIF_NOERROR)
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
+#define _TIF_FREEZE		(1<<TIF_FREEZE)
 #define _TIF_SYSCALL_T_OR_A	(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT|_TIF_SECCOMP)
 
 #define _TIF_USER_WORK_MASK	(_TIF_NOTIFY_RESUME | _TIF_SIGPENDING | \
Index: linux-2.6.19-rc6-mm2/include/asm-x86_64/thread_info.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/asm-x86_64/thread_info.h
+++ linux-2.6.19-rc6-mm2/include/asm-x86_64/thread_info.h
@@ -122,6 +122,7 @@ static inline struct thread_info *stack_
 #define TIF_MEMDIE		20
 #define TIF_DEBUG		21	/* uses debug registers */
 #define TIF_IO_BITMAP		22	/* uses I/O bitmap */
+#define TIF_FREEZE		23	/* is freezing for suspend */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
@@ -137,6 +138,7 @@ static inline struct thread_info *stack_
 #define _TIF_ABI_PENDING	(1<<TIF_ABI_PENDING)
 #define _TIF_DEBUG		(1<<TIF_DEBUG)
 #define _TIF_IO_BITMAP		(1<<TIF_IO_BITMAP)
+#define _TIF_FREEZE		(1<<TIF_FREEZE)
 
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK \
Index: linux-2.6.19-rc6-mm2/include/linux/freezer.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/linux/freezer.h
+++ linux-2.6.19-rc6-mm2/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 test_tsk_thread_flag(p, TIF_FREEZE);
 }
 
 /*
  * 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;
+	set_tsk_thread_flag(p, TIF_FREEZE);
 }
 
 /*
@@ -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;
+	clear_tsk_thread_flag(p, TIF_FREEZE);
 }
 
 /*
@@ -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();
+	clear_tsk_thread_flag(p, TIF_FREEZE);
 }
 
 extern void refrigerator(void);
Index: linux-2.6.19-rc6-mm2/include/linux/sched.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/linux/sched.h
+++ linux-2.6.19-rc6-mm2/include/linux/sched.h
@@ -1161,7 +1161,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-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
@@ -60,13 +60,16 @@ static inline void freeze_process(struct
 	unsigned long flags;
 
 	if (!freezing(p)) {
-		if (p->state == TASK_STOPPED)
-			force_sig_specific(SIGSTOP, p);
-
-		freeze(p);
-		spin_lock_irqsave(&p->sighand->siglock, flags);
-		signal_wake_up(p, p->state == TASK_STOPPED);
-		spin_unlock_irqrestore(&p->sighand->siglock, flags);
+		rmb();
+		if (!frozen(p)) {
+			if (p->state == TASK_STOPPED)
+				force_sig_specific(SIGSTOP, p);
+
+			freeze(p);
+			spin_lock_irqsave(&p->sighand->siglock, flags);
+			signal_wake_up(p, p->state == TASK_STOPPED);
+			spin_unlock_irqrestore(&p->sighand->siglock, flags);
+		}
 	}
 }
 

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

[PATCH -mm 2/2]: PM: SMP-safe freezer

[Rafael J. Wysocki]

Currently, to tell a task that it should go to the refrigerator, we set the
PF_FREEZE flag for it and send a fake signal to it.  Unfortunately there are
two SMP-related problems with this approach.  First, a task running on another
CPU may be updating its flags while the freezer attempts to set PF_FREEZE
for it and this may leave the task's flags in an inconsistent state.  Second,
there is a potential race between freeze_process() and refrigerator() in
which freeze_process() running on one CPU is reading a task's PF_FREEZE
flag while refrigerator() running on another CPU has just set PF_FROZEN
for the same task and attempts to reset PF_FREEZE for it.  If refrigerator
wins the race, freeze_process() will state that PF_FREEZE hasn't been set
for the task and will set it unnecessarily, so the task will go to the
refrigerator once again after it's been thawed.

To solve first of these problems we need to stop using PF_FROZEN to tell
tasks that they should go to the refrigerator.  Instead, we can introduce
a special TIF_*** flag and use it for this purpose, since it is allowed to
change the other tasks' TIF_*** flags and there are special calls for it.

To avoid the freeze_process()-refrigerator() race we can make freeze_process()
to always check the task's PF_FROZEN flag after it's read its "freeze" flag.
We should also make sure that refrigerator() will always reset the task's
"freeze" flag after it's set PF_FROZEN for it.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/asm-frv/thread_info.h     |    2 ++
 include/asm-i386/thread_info.h    |    2 ++
 include/asm-powerpc/thread_info.h |    2 ++
 include/asm-x86_64/thread_info.h  |    2 ++
 include/linux/freezer.h           |   11 ++++++-----
 include/linux/sched.h             |    1 -
 kernel/power/process.c            |   11 +++++++----
 7 files changed, 21 insertions(+), 10 deletions(-)

Index: linux-2.6.19-rc6-mm2/include/asm-frv/thread_info.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/asm-frv/thread_info.h
+++ linux-2.6.19-rc6-mm2/include/asm-frv/thread_info.h
@@ -116,6 +116,7 @@ register struct thread_info *__current_t
 #define TIF_RESTORE_SIGMASK	6	/* restore signal mask in do_signal() */
 #define TIF_POLLING_NRFLAG	16	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE		17	/* OOM killer killed process */
+#define TIF_FREEZE		18	/* freezing for suspend */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)
@@ -125,6 +126,7 @@ register struct thread_info *__current_t
 #define _TIF_IRET		(1 << TIF_IRET)
 #define _TIF_RESTORE_SIGMASK	(1 << TIF_RESTORE_SIGMASK)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
+#define _TIF_FREEZE		(1 << TIF_FREEZE)
 
 #define _TIF_WORK_MASK		0x0000FFFE	/* work to do on interrupt/exception return */
 #define _TIF_ALLWORK_MASK	0x0000FFFF	/* work to do on any return to u-space */
Index: linux-2.6.19-rc6-mm2/include/asm-i386/thread_info.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/asm-i386/thread_info.h
+++ linux-2.6.19-rc6-mm2/include/asm-i386/thread_info.h
@@ -134,6 +134,7 @@ static inline struct thread_info *curren
 #define TIF_MEMDIE		16
 #define TIF_DEBUG		17	/* uses debug registers */
 #define TIF_IO_BITMAP		18	/* uses I/O bitmap */
+#define TIF_FREEZE		19	/* is freezing for suspend */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
@@ -147,6 +148,7 @@ static inline struct thread_info *curren
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
 #define _TIF_DEBUG		(1<<TIF_DEBUG)
 #define _TIF_IO_BITMAP		(1<<TIF_IO_BITMAP)
+#define _TIF_FREEZE		(1<<TIF_FREEZE)
 
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK \
Index: linux-2.6.19-rc6-mm2/include/asm-powerpc/thread_info.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/asm-powerpc/thread_info.h
+++ linux-2.6.19-rc6-mm2/include/asm-powerpc/thread_info.h
@@ -122,6 +122,7 @@ static inline struct thread_info *curren
 #define TIF_RESTOREALL		12	/* Restore all regs (implies NOERROR) */
 #define TIF_NOERROR		14	/* Force successful syscall return */
 #define TIF_RESTORE_SIGMASK	15	/* Restore signal mask in do_signal */
+#define TIF_FREEZE		16	/* Freezing for suspend */
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -138,6 +139,7 @@ static inline struct thread_info *curren
 #define _TIF_RESTOREALL		(1<<TIF_RESTOREALL)
 #define _TIF_NOERROR		(1<<TIF_NOERROR)
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
+#define _TIF_FREEZE		(1<<TIF_FREEZE)
 #define _TIF_SYSCALL_T_OR_A	(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT|_TIF_SECCOMP)
 
 #define _TIF_USER_WORK_MASK	(_TIF_NOTIFY_RESUME | _TIF_SIGPENDING | \
Index: linux-2.6.19-rc6-mm2/include/asm-x86_64/thread_info.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/asm-x86_64/thread_info.h
+++ linux-2.6.19-rc6-mm2/include/asm-x86_64/thread_info.h
@@ -122,6 +122,7 @@ static inline struct thread_info *stack_
 #define TIF_MEMDIE		20
 #define TIF_DEBUG		21	/* uses debug registers */
 #define TIF_IO_BITMAP		22	/* uses I/O bitmap */
+#define TIF_FREEZE		23	/* is freezing for suspend */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1<<TIF_NOTIFY_RESUME)
@@ -137,6 +138,7 @@ static inline struct thread_info *stack_
 #define _TIF_ABI_PENDING	(1<<TIF_ABI_PENDING)
 #define _TIF_DEBUG		(1<<TIF_DEBUG)
 #define _TIF_IO_BITMAP		(1<<TIF_IO_BITMAP)
+#define _TIF_FREEZE		(1<<TIF_FREEZE)
 
 /* work to do on interrupt/exception return */
 #define _TIF_WORK_MASK \
Index: linux-2.6.19-rc6-mm2/include/linux/freezer.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/linux/freezer.h
+++ linux-2.6.19-rc6-mm2/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 test_tsk_thread_flag(p, TIF_FREEZE);
 }
 
 /*
  * 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;
+	set_tsk_thread_flag(p, TIF_FREEZE);
 }
 
 /*
@@ -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;
+	clear_tsk_thread_flag(p, TIF_FREEZE);
 }
 
 /*
@@ -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();
+	clear_tsk_thread_flag(p, TIF_FREEZE);
 }
 
 extern void refrigerator(void);
Index: linux-2.6.19-rc6-mm2/include/linux/sched.h
===================================================================
--- linux-2.6.19-rc6-mm2.orig/include/linux/sched.h
+++ linux-2.6.19-rc6-mm2/include/linux/sched.h
@@ -1161,7 +1161,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-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
@@ -60,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);
+		}
 	}
 }
 

-------------------------------------------------------------------------
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: [PATCH -mm 2/2]: PM: SMP-safe freezer

[Pavel Machek]

Hi!

> To solve first of these problems we need to stop using PF_FROZEN to tell
> tasks that they should go to the refrigerator.  Instead, we can introduce
> a special TIF_*** flag and use it for this purpose, since it is allowed to
> change the other tasks' TIF_*** flags and there are special calls for it.
> 
> To avoid the freeze_process()-refrigerator() race we can make freeze_process()
> to always check the task's PF_FROZEN flag after it's read its "freeze" flag.
> We should also make sure that refrigerator() will always reset the task's
> "freeze" flag after it's set PF_FROZEN for it.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

ACK.
								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: [PATCH -mm 2/2]: PM: SMP-safe freezer

[Rafael J. Wysocki]

On Monday, 4 December 2006 11:30, Pavel Machek wrote:
> Hi!
> 
> > To solve first of these problems we need to stop using PF_FROZEN to tell
> > tasks that they should go to the refrigerator.  Instead, we can introduce
> > a special TIF_*** flag and use it for this purpose, since it is allowed to
> > change the other tasks' TIF_*** flags and there are special calls for it.
> > 
> > To avoid the freeze_process()-refrigerator() race we can make freeze_process()
> > to always check the task's PF_FROZEN flag after it's read its "freeze" flag.
> > We should also make sure that refrigerator() will always reset the task's
> > "freeze" flag after it's set PF_FROZEN for it.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> ACK.

But I think I'll need to add TIF_FROZEN for all architectures, because suspend
to RAM is supposed to work on all of them, isn't it?

Also, this patch is only sufficient with [1/2] or an alternative solution?

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: [PATCH -mm 2/2]: PM: SMP-safe freezer

[Pavel Machek]

Hi!

> > > To solve first of these problems we need to stop using PF_FROZEN to tell
> > > tasks that they should go to the refrigerator.  Instead, we can introduce
> > > a special TIF_*** flag and use it for this purpose, since it is allowed to
> > > change the other tasks' TIF_*** flags and there are special calls for it.
> > > 
> > > To avoid the freeze_process()-refrigerator() race we can make freeze_process()
> > > to always check the task's PF_FROZEN flag after it's read its "freeze" flag.
> > > We should also make sure that refrigerator() will always reset the task's
> > > "freeze" flag after it's set PF_FROZEN for it.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > ACK.
> 
> But I think I'll need to add TIF_FROZEN for all architectures, because suspend
> to RAM is supposed to work on all of them, isn't it?

Well, yes, it should be added, but no, I do not think s2ram works on
that many machines.

> Also, this patch is only sufficient with [1/2] or an alternative solution?

I haven't finished looking at that one. I suspect it is okay, but I'm
not sure it is enough.
							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: [PATCH -mm 2/2]: PM: SMP-safe freezer

[Rafael J. Wysocki]

Hi,

On Monday, 4 December 2006 20:44, Pavel Machek wrote:
> Hi!
> 
> > > > To solve first of these problems we need to stop using PF_FROZEN to tell
> > > > tasks that they should go to the refrigerator.  Instead, we can introduce
> > > > a special TIF_*** flag and use it for this purpose, since it is allowed to
> > > > change the other tasks' TIF_*** flags and there are special calls for it.
> > > > 
> > > > To avoid the freeze_process()-refrigerator() race we can make freeze_process()
> > > > to always check the task's PF_FROZEN flag after it's read its "freeze" flag.
> > > > We should also make sure that refrigerator() will always reset the task's
> > > > "freeze" flag after it's set PF_FROZEN for it.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > ACK.
> > 
> > But I think I'll need to add TIF_FROZEN for all architectures, because suspend
> > to RAM is supposed to work on all of them, isn't it?
> 
> Well, yes, it should be added, but no, I do not think s2ram works on
> that many machines.

So, do you think I should add any more architectures (which ones?) or leave it
as is?

> > Also, this patch is only sufficient with [1/2] or an alternative solution?
> 
> I haven't finished looking at that one. I suspect it is okay, but I'm
> not sure it is enough.

Well, I think I haven't overlooked anything, but of course that's always
possible. ;-)

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

-------------------------------------------------------------------------
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] [PATCH -mm 2/2]: PM: SMP-safe freezer

[David Brownell]

[ off $SUBJECT ]

On Monday 04 December 2006 11:44 am, Pavel Machek wrote:
> > But I think I'll need to add TIF_FROZEN for all architectures, because suspend
> > to RAM is supposed to work on all of them, isn't it?
> 
> Well, yes, it should be added, but no, I do not think s2ram works on
> that many machines.

Userspace "s2ram" != PM_SUSPEND_MEM ("suspend-to-RAM") though.

I'm not even sure there's a globally acceptable definition of
what PM_SUSPEND_MEM indicates, beyond the fact that one expects
it saves more power than PM_SUSPEND_STANDBY.

Yes, the Documentation/power/states.txt regurgitates ACPI spec text.
But lots of non-ACPI systems don't have any reason to do things
like ACPI says.  And in terms of latency, I've seen a lot of that
be due to the PM framework taking so long to walk the device tree,
with no substantial difference between "standby" and "str" costs.

-------------------------------------------------------------------------
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] [PATCH -mm 2/2]: PM: SMP-safe freezer

[Rafael J. Wysocki]

On Tuesday, 5 December 2006 06:43, David Brownell wrote:
> [ off $SUBJECT ]
> 
> On Monday 04 December 2006 11:44 am, Pavel Machek wrote:
> > > But I think I'll need to add TIF_FROZEN for all architectures, because suspend
> > > to RAM is supposed to work on all of them, isn't it?
> > 
> > Well, yes, it should be added, but no, I do not think s2ram works on
> > that many machines.
> 
> Userspace "s2ram" != PM_SUSPEND_MEM ("suspend-to-RAM") though.
> 
> I'm not even sure there's a globally acceptable definition of
> what PM_SUSPEND_MEM indicates, beyond the fact that one expects
> it saves more power than PM_SUSPEND_STANDBY.
> 
> Yes, the Documentation/power/states.txt regurgitates ACPI spec text.
> But lots of non-ACPI systems don't have any reason to do things
> like ACPI says.  And in terms of latency, I've seen a lot of that
> be due to the PM framework taking so long to walk the device tree,
> with no substantial difference between "standby" and "str" costs.

Well, in this particular case the question is only which architectures will
actually use the freezer. :-)

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King