[PATCH 2/2] Freezer: Fix APM emulation breakage

[PATCH 2/2] Freezer: Fix APM emulation breakage

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

The APM emulation is currently broken as a result of commit
831441862956fffa17b9801db37e6ea1650b0f69
"Freezer: make kernel threads nonfreezable by default"
that removed the PF_NOFREEZE annotations from apm_ioctl() without adding
the appropriate freezer hooks.  Fix it and remove the unnecessary variable flags
from apm_ioctl().

Special thanks to Franck Bui-Huu <vagabon.xyz@gmail.com> for pointing out the
problem.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Franck Bui-Huu <vagabon.xyz@gmail.com>
Cc: Nigel Cunningham <nigel@nigel.suspend2.net>
---
 drivers/char/apm-emulation.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/char/apm-emulation.c
===================================================================
--- linux-2.6.orig/drivers/char/apm-emulation.c
+++ linux-2.6/drivers/char/apm-emulation.c
@@ -295,7 +295,6 @@ static int
 apm_ioctl(struct inode * inode, struct file *filp, u_int cmd, u_long arg)
 {
 	struct apm_user *as = filp->private_data;
-	unsigned long flags;
 	int err = -EINVAL;
 
 	if (!as->suser || !as->writer)
@@ -331,10 +330,16 @@ apm_ioctl(struct inode * inode, struct f
 			 * Wait for the suspend/resume to complete.  If there
 			 * are pending acknowledges, we wait here for them.
 			 */
-			flags = current->flags;
+			freezer_do_not_count();
 
 			wait_event(apm_suspend_waitqueue,
 				   as->suspend_state == SUSPEND_DONE);
+
+			/*
+			 * Since we are waiting until the suspend is done, the
+			 * try_to_freeze() in freezer_count() will not trigger
+			 */
+			freezer_count();
 		} else {
 			as->suspend_state = SUSPEND_WAIT;
 			mutex_unlock(&state_lock);
@@ -362,14 +367,10 @@ apm_ioctl(struct inode * inode, struct f
 			 * Wait for the suspend/resume to complete.  If there
 			 * are pending acknowledges, we wait here for them.
 			 */
-			flags = current->flags;
-
-			wait_event_interruptible(apm_suspend_waitqueue,
+			wait_event_freezable(apm_suspend_waitqueue,
 					 as->suspend_state == SUSPEND_DONE);
 		}
 
-		current->flags = flags;
-
 		mutex_lock(&state_lock);
 		err = as->suspend_result;
 		as->suspend_state = SUSPEND_NONE;

[PATCH 0/2] Freezer fixes for 2.6.24 (rev. 2)

[Rafael J. Wysocki]

Hi Len,

This is the second iteration of the patches are intended to fix two recent
regressions related to the freezer.

The first one fixes http://bugzilla.kernel.org/show_bug.cgi?id=9345 and has
been tested by the reporter.

The second one is intended to fix the APM emulation breakage caused before
2.6.23.

Please apply.

Thanks,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

[PATCH 1/2] Freezer: Fix s2disk resume from initrd (rev. 2)

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Add appropriate freezer annotations to handle_initrd(), so that it's possible
to resume from disk from an initrd.

This patch fixes Bug #9345: http://bugzilla.kernel.org/show_bug.cgi?id=9345

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Nigel Cunningham <nigel@suspend2.net>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Chris Friedhoff <chris@friedhoff.org>
---
 init/do_mounts_initrd.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Index: linux-2.6/init/do_mounts_initrd.c
===================================================================
--- linux-2.6.orig/init/do_mounts_initrd.c
+++ linux-2.6/init/do_mounts_initrd.c
@@ -55,12 +55,18 @@ static void __init handle_initrd(void)
 	sys_mount(".", "/", NULL, MS_MOVE, NULL);
 	sys_chroot(".");
 
+	/*
+	 * In case that a resume from disk is carried out by linuxrc or one of
+	 * its children, we need to tell the freezer not to wait for us.
+	 */
+	current->flags |= PF_FREEZER_SKIP;
+
 	pid = kernel_thread(do_linuxrc, "/linuxrc", SIGCHLD);
 	if (pid > 0)
-		while (pid != sys_wait4(-1, NULL, 0, NULL)) {
-			try_to_freeze();
+		while (pid != sys_wait4(-1, NULL, 0, NULL))
 			yield();
-		}
+
+	current->flags &= ~PF_FREEZER_SKIP;
 
 	/* move initrd to rootfs' /old */
 	sys_fchdir(old_fd);

[PATCH 2/2] Freezer: Fix APM emulation breakage

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

The APM emulation is currently broken as a result of commit
831441862956fffa17b9801db37e6ea1650b0f69
"Freezer: make kernel threads nonfreezable by default"
that removed the PF_NOFREEZE annotations from apm_ioctl() without adding
the appropriate freezer hooks.  Fix it and remove the unnecessary variable flags
from apm_ioctl().

Special thanks to Franck Bui-Huu <vagabon.xyz@gmail.com> for pointing out the
problem.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Franck Bui-Huu <vagabon.xyz@gmail.com>
Cc: Nigel Cunningham <nigel@nigel.suspend2.net>
---
 drivers/char/apm-emulation.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/char/apm-emulation.c
===================================================================
--- linux-2.6.orig/drivers/char/apm-emulation.c
+++ linux-2.6/drivers/char/apm-emulation.c
@@ -295,7 +295,6 @@ static int
 apm_ioctl(struct inode * inode, struct file *filp, u_int cmd, u_long arg)
 {
 	struct apm_user *as = filp->private_data;
-	unsigned long flags;
 	int err = -EINVAL;
 
 	if (!as->suser || !as->writer)
@@ -331,10 +330,16 @@ apm_ioctl(struct inode * inode, struct f
 			 * Wait for the suspend/resume to complete.  If there
 			 * are pending acknowledges, we wait here for them.
 			 */
-			flags = current->flags;
+			freezer_do_not_count();
 
 			wait_event(apm_suspend_waitqueue,
 				   as->suspend_state == SUSPEND_DONE);
+
+			/*
+			 * Since we are waiting until the suspend is done, the
+			 * try_to_freeze() in freezer_count() will not trigger
+			 */
+			freezer_count();
 		} else {
 			as->suspend_state = SUSPEND_WAIT;
 			mutex_unlock(&state_lock);
@@ -362,14 +367,10 @@ apm_ioctl(struct inode * inode, struct f
 			 * Wait for the suspend/resume to complete.  If there
 			 * are pending acknowledges, we wait here for them.
 			 */
-			flags = current->flags;
-
-			wait_event_interruptible(apm_suspend_waitqueue,
+			wait_event_freezable(apm_suspend_waitqueue,
 					 as->suspend_state == SUSPEND_DONE);
 		}
 
-		current->flags = flags;
-
 		mutex_lock(&state_lock);
 		err = as->suspend_result;
 		as->suspend_state = SUSPEND_NONE;

Re: [PATCH 1/2] Freezer: Fix s2disk resume from initrd (rev. 2)

[Pavel Machek]

Hi!

> Add appropriate freezer annotations to handle_initrd(), so that it's possible
> to resume from disk from an initrd.
> 
> This patch fixes Bug #9345: http://bugzilla.kernel.org/show_bug.cgi?id=9345


Unfortunately, bugzilla does not help me in understanding this.

> Index: linux-2.6/init/do_mounts_initrd.c
> ===================================================================
> --- linux-2.6.orig/init/do_mounts_initrd.c
> +++ linux-2.6/init/do_mounts_initrd.c
> @@ -55,12 +55,18 @@ static void __init handle_initrd(void)
>  	sys_mount(".", "/", NULL, MS_MOVE, NULL);
>  	sys_chroot(".");
>  
> +	/*
> +	 * In case that a resume from disk is carried out by linuxrc or one of
> +	 * its children, we need to tell the freezer not to wait for us.
> +	 */
> +	current->flags |= PF_FREEZER_SKIP;
> +
>  	pid = kernel_thread(do_linuxrc, "/linuxrc", SIGCHLD);
>  	if (pid > 0)
> -		while (pid != sys_wait4(-1, NULL, 0, NULL)) {
> -			try_to_freeze();
> +		while (pid != sys_wait4(-1, NULL, 0, NULL))
>  			yield();
> -		}
> +
> +	current->flags &= ~PF_FREEZER_SKIP;

New code should work (but it is uglier than the old one). What was
wrong with the old one? Refrigerator should make sys_wait4() return,
and that it turn should put handle_initrd() into refrigerator...

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

Re: [PATCH 1/2] Freezer: Fix s2disk resume from initrd (rev. 2)

[Rafael J. Wysocki]

On Thursday, 22 of November 2007, Pavel Machek wrote:
> Hi!
> 
> > Add appropriate freezer annotations to handle_initrd(), so that it's possible
> > to resume from disk from an initrd.
> > 
> > This patch fixes Bug #9345: http://bugzilla.kernel.org/show_bug.cgi?id=9345
> 
> 
> Unfortunately, bugzilla does not help me in understanding this.
> 
> > Index: linux-2.6/init/do_mounts_initrd.c
> > ===================================================================
> > --- linux-2.6.orig/init/do_mounts_initrd.c
> > +++ linux-2.6/init/do_mounts_initrd.c
> > @@ -55,12 +55,18 @@ static void __init handle_initrd(void)
> >  	sys_mount(".", "/", NULL, MS_MOVE, NULL);
> >  	sys_chroot(".");
> >  
> > +	/*
> > +	 * In case that a resume from disk is carried out by linuxrc or one of
> > +	 * its children, we need to tell the freezer not to wait for us.
> > +	 */
> > +	current->flags |= PF_FREEZER_SKIP;
> > +
> >  	pid = kernel_thread(do_linuxrc, "/linuxrc", SIGCHLD);
> >  	if (pid > 0)
> > -		while (pid != sys_wait4(-1, NULL, 0, NULL)) {
> > -			try_to_freeze();
> > +		while (pid != sys_wait4(-1, NULL, 0, NULL))
> >  			yield();
> > -		}
> > +
> > +	current->flags &= ~PF_FREEZER_SKIP;
> 
> New code should work (but it is uglier than the old one).

Well, the old code plain doesn't work ...

> What was wrong with the old one? Refrigerator should make sys_wait4() return,

It doesn't, because the task as no mm and is therefore considered as a kernel
thread and not sent a signal.

This _really_ is an exceptional case.

Greetings,
Rafael

Re: [PATCH 2/2] Freezer: Fix APM emulation breakage

[Franck Bui-Huu]

Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The APM emulation is currently broken as a result of commit
> 831441862956fffa17b9801db37e6ea1650b0f69
> "Freezer: make kernel threads nonfreezable by default"
> that removed the PF_NOFREEZE annotations from apm_ioctl() without adding
> the appropriate freezer hooks.  Fix it and remove the unnecessary variable flags
> from apm_ioctl().
> 

This patch should probably go to the 2.6.23 stable tree as
well, shouldn't it ?

		Franck

Re: [PATCH 2/2] Freezer: Fix APM emulation breakage

[Rafael J. Wysocki]

On Thursday, 22 of November 2007, Franck Bui-Huu wrote:
> Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The APM emulation is currently broken as a result of commit
> > 831441862956fffa17b9801db37e6ea1650b0f69
> > "Freezer: make kernel threads nonfreezable by default"
> > that removed the PF_NOFREEZE annotations from apm_ioctl() without adding
> > the appropriate freezer hooks.  Fix it and remove the unnecessary variable flags
> > from apm_ioctl().
> > 
> 
> This patch should probably go to the 2.6.23 stable tree as
> well, shouldn't it ?

Yes, it should, but first it needs to get to the Linus' tree.

Greetings,
Rafael

Re: [PATCH 1/2] Freezer: Fix s2disk resume from initrd (rev. 2)

[Pavel Machek]

Hi!

> > > +	/*
> > > +	 * In case that a resume from disk is carried out by linuxrc or one of
> > > +	 * its children, we need to tell the freezer not to wait for us.
> > > +	 */
> > > +	current->flags |= PF_FREEZER_SKIP;
> > > +
> > >  	pid = kernel_thread(do_linuxrc, "/linuxrc", SIGCHLD);
> > >  	if (pid > 0)
> > > -		while (pid != sys_wait4(-1, NULL, 0, NULL)) {
> > > -			try_to_freeze();
> > > +		while (pid != sys_wait4(-1, NULL, 0, NULL))
> > >  			yield();
> > > -		}
> > > +
> > > +	current->flags &= ~PF_FREEZER_SKIP;
> > 
> > New code should work (but it is uglier than the old one).
> 
> Well, the old code plain doesn't work ...
> 
> > What was wrong with the old one? Refrigerator should make sys_wait4() return,
> 
> It doesn't, because the task as no mm and is therefore considered as a kernel
> thread and not sent a signal.
> 
> This _really_ is an exceptional case.

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

Re: [PATCH 1/2] Freezer: Fix s2disk resume from initrd (rev. 2)

[Nigel Cunningham]

Hi.

On Thursday 22 November 2007 09:25:32 Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Add appropriate freezer annotations to handle_initrd(), so that it's 
possible
> to resume from disk from an initrd.
> 
> This patch fixes Bug #9345: http://bugzilla.kernel.org/show_bug.cgi?id=9345
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Nigel Cunningham <nigel@suspend2.net>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Chris Friedhoff <chris@friedhoff.org>
> ---
>  init/do_mounts_initrd.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/init/do_mounts_initrd.c
> ===================================================================
> --- linux-2.6.orig/init/do_mounts_initrd.c
> +++ linux-2.6/init/do_mounts_initrd.c
> @@ -55,12 +55,18 @@ static void __init handle_initrd(void)
>  	sys_mount(".", "/", NULL, MS_MOVE, NULL);
>  	sys_chroot(".");
>  
> +	/*
> +	 * In case that a resume from disk is carried out by linuxrc or one of
> +	 * its children, we need to tell the freezer not to wait for us.
> +	 */
> +	current->flags |= PF_FREEZER_SKIP;
> +
>  	pid = kernel_thread(do_linuxrc, "/linuxrc", SIGCHLD);
>  	if (pid > 0)
> -		while (pid != sys_wait4(-1, NULL, 0, NULL)) {
> -			try_to_freeze();
> +		while (pid != sys_wait4(-1, NULL, 0, NULL))
>  			yield();
> -		}
> +
> +	current->flags &= ~PF_FREEZER_SKIP;
>  
>  	/* move initrd to rootfs' /old */
>  	sys_fchdir(old_fd);
> 
> 

Ack. This fixes the issue for me, too.

Nigel
-- 
See http://www.tuxonice.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.