Freezer fixes for 2.6.24 (rev. 2)

PATCH In review Rafael J. Wysocki 4 participants 9 messages Nov 21, 2007 – Nov 25, 2007

Subsystems (3)

VFS Char and misc drivers Apm driver

lore

Share & tools

lore

Rafael J. Wysocki

author

2007-11-21 22:22 UTC #0 lore raw

cover Freezer fixes for 2.6.24 (rev. 2)

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

Signature · 1 line

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

Rafael J. Wysocki

author

2007-11-21 22:25 UTC #1 lore raw

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

From: Rafael J. Wysocki <[email protected]>

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 <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Nigel Cunningham <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Chris Friedhoff <[email protected]>

---
 init/do_mounts_initrd.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

init/do_mounts_initrd.c +9 -3

Index: 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);

Pavel Machek

2007-11-22 09:50 UTC #2 lore raw

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

Hi!

Quoted 4 lines

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.

Quoted 13 lines

Index: linux-2.6/init/do_mounts_initrd.c
===================================================================

init/do_mounts_initrd.c +5 -0

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

init/do_mounts_initrd.c static void __init handle_initrd(void) · line 69

Quoted 10 lines

init/do_mounts_initrd.c +4 -3

+
 	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

Signature · 2 lines

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

Rafael J. Wysocki

author

2007-11-22 16:09 UTC #3 lore raw

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

Quoted 27 lines · Pavel Machek

Hi!
Quoted 4 lines
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.
Quoted 17 lines
Index: linux-2.6/init/do_mounts_initrd.c
===================================================================
init/do_mounts_initrd.c +6 -1
@@ -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)) {

Quoted 8 lines

init/do_mounts_initrd.c static void __init handle_initrd(void) · line 69
Quoted 6 lines
init/do_mounts_initrd.c +3 -2
-			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 ...

Quoted 1 line

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

Pavel Machek

2007-11-23 12:21 UTC #4 lore raw

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

Hi!

Quoted 18 lines

Quoted 9 lines

init/do_mounts_initrd.c +6 -1

+	/*
+	 * 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)) {

Quoted 8 lines

init/do_mounts_initrd.c static void __init handle_initrd(void) · line 69
Quoted 6 lines
init/do_mounts_initrd.c +3 -2
-			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).

Quoted 8 lines

Well, the old code plain doesn't work ...
Quoted 1 line
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

Signature · 2 lines

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

Nigel Cunningham

2007-11-25 23:10 UTC #5 lore raw

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

In reply to Rafael J. Wysocki

Hi.

Quoted 3 lines · On Thursday 22 November 2007 09:25:32 Rafael J. Wysocki wrote:

From: Rafael J. Wysocki <[email protected]>
Add appropriate freezer annotations to handle_initrd(), so that it's

possible

Quoted 31 lines

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 <[email protected]>
Cc Pavel Machek <[email protected]>
Cc Nigel Cunningham <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Chris Friedhoff <[email protected]>
---
 init/do_mounts_initrd.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
Index: linux-2.6/init/do_mounts_initrd.c
===================================================================
init/do_mounts_initrd.c +6 -0
@@ -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)

init/do_mounts_initrd.c static void __init handle_initrd(void) · line 72

Quoted 10 lines

init/do_mounts_initrd.c +3 -3

-		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

Signature · 2 lines

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

Rafael J. Wysocki

author

2007-11-21 22:26 UTC #6 lore raw

PATCH 2/2 Freezer: Fix APM emulation breakage

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

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 <[email protected]> for pointing out the problem.

Signed-off-by Rafael J. Wysocki <[email protected]>

Cc Pavel Machek <[email protected]>

Cc Franck Bui-Huu <[email protected]>

Cc Nigel Cunningham <[email protected]>

---
 drivers/char/apm-emulation.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

drivers/char/apm-emulation.c +8 -7

Index: 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;

Franck Bui-Huu

2007-11-22 20:33 UTC #7 lore raw

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

Quoted 9 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
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

Rafael J. Wysocki

author

2007-11-22 21:29 UTC #8 lore raw

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

Quoted 11 lines · Franck Bui-Huu

Quoted 9 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
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().

Quoted 2 lines

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