Freezer fixes for 2.6.24

PATCH In review Rafael J. Wysocki 3 participants 10 messages Nov 21, 2007 – Nov 22, 2007

Subsystems (3)

VFS Char and misc drivers Apm driver

lore

Share & tools

lore

Rafael J. Wysocki

author

2007-11-21 01:44 UTC #0 lore raw

cover Freezer fixes for 2.6.24

Hi Len,

The following two patches are intended to fix 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, but unfortunately I can't test it. The problem is a bit academic, though, since no one has reported this problem so far, so apparently this driver is not very popular. ;-)

Please apply.

Thanks,
Rafael

Signature · 1 line

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

Rafael J. Wysocki

author

2007-11-21 01:50 UTC #1 lore raw

PATCH 1/2 Freezer: Fix s2disk resume from initrd

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 carreid 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);

Nigel Cunningham

2007-11-21 02:22 UTC #2 lore raw

Re: [PATCH 1/2] Freezer: Fix s2disk resume from initrd

Hi.

Just a typo...

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

Quoted 2 lines · On Wednesday 21 November 2007 12:50:17 Rafael J. Wysocki wrote:

init/do_mounts_initrd.c +2 -0

+	/*
+	 * In case that a resume from disk is carreid out by linuxrc or one of

carried

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

Quoted 1 line

init/do_mounts_initrd.c +1 -0

+	 * its children, we need to tell the freezer not to wait for us

(final full stop also missing)

Regards,

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 19:20 UTC #3 lore raw

Re: [PATCH 1/2] Freezer: Fix s2disk resume from initrd

Quoted 6 lines · Nigel Cunningham

Hi.
Just a typo...
On Wednesday 21 November 2007 12:50:17 Rafael J. Wysocki wrote:

Quoted 8 lines

init/do_mounts_initrd.c static void __init handle_initrd(void) · line 59
Quoted 2 lines
init/do_mounts_initrd.c +2 -0
+	/*
+	 * In case that a resume from disk is carreid out by linuxrc or one of
carried
init/do_mounts_initrd.c static void __init handle_initrd(void) · line 60
Quoted 1 line
init/do_mounts_initrd.c +1 -0
+	 * its children, we need to tell the freezer not to wait for us
(final full stop also missing)

Thanks, I'll fix the comment in the next iteration.

Rafael

Rafael J. Wysocki

author

2007-11-21 01:53 UTC #4 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-21 07:51 UTC #5 lore raw

In reply to Rafael J. Wysocki

Quoted 2 lines · Rafael J. Wysocki

The second one is intended to fix the APM emulation breakage caused before 2.6.23, but unfortunately I can't test it.

Why not ?

Just hack Kconfig to be able to select this driver on x86 architecture and it should work fine, no ?

I'll give it a test today.

Franck

Rafael J. Wysocki

author

2007-11-21 19:21 UTC #6 lore raw

Quoted 8 lines · Franck Bui-Huu

Quoted 2 lines · Rafael J. Wysocki
The second one is intended to fix the APM emulation breakage caused before 2.6.23, but unfortunately I can't test it.
Why not ?
Just hack Kconfig to be able to select this driver on x86 architecture and it should work fine, no ?

Well, I'm afraid I have no suitable userland here ...

Quoted 1 line

I'll give it a test today.

OK, thanks!

Rafael

Franck Bui-Huu

2007-11-22 13:05 UTC #7 lore raw

Quoted 11 lines · Rafael J. Wysocki

Quoted 7 lines · Franck Bui-Huu
Quoted 2 lines · Rafael J. Wysocki
The second one is intended to fix the APM emulation breakage caused before 2.6.23, but unfortunately I can't test it.
Why not ?
Just hack Kconfig to be able to select this driver on x86 architecture and it should work fine, no ?
Well, I'm afraid I have no suitable userland here ...

I don't have either.

Anyway I just verified that the current code is broken. I put 2 processes waiting for a suspend evetn by reading the /dev/apm_bios device and a third process that does the suspend event by doing an ioctl() on the apm device. The system doesn't suspend (the kernel dumped a list of the current tasks, instead of printing "Freezing of tasks failed after..." BTW) and eventually the third process exits with an incorrect value: -EINTR.

Applying your patch makes the previous test case work.

I also discovered that s2ram doesn't work on my laptop and had to use standby suspend. I'll try to find out why.

Franck

Rafael J. Wysocki

author

2007-11-22 16:10 UTC #8 lore raw

Quoted 23 lines · Franck Bui-Huu

Quoted 11 lines · Rafael J. Wysocki
Quoted 7 lines · Franck Bui-Huu
Quoted 2 lines · Rafael J. Wysocki
The second one is intended to fix the APM emulation breakage caused before 2.6.23, but unfortunately I can't test it.
Why not ?
Just hack Kconfig to be able to select this driver on x86 architecture and it should work fine, no ?
Well, I'm afraid I have no suitable userland here ...
I don't have either.
Anyway I just verified that the current code is broken. I put 2 processes waiting for a suspend evetn by reading the /dev/apm_bios device and a third process that does the suspend event by doing an ioctl() on the apm device. The system doesn't suspend (the kernel dumped a list of the current tasks, instead of printing "Freezing of tasks failed after..." BTW) and eventually the third process exits with an incorrect value: -EINTR.

Quoted 1 line

Applying your patch makes the previous test case work.

IOW, the patch helps?

Quoted 2 lines

I also discovered that s2ram doesn't work on my laptop and had to use standby suspend. I'll try to find out why.

Yes, please.

Thanks,
Rafael

Franck Bui-Huu

2007-11-22 20:29 UTC #9 lore raw

Quoted 9 lines · Rafael J. Wysocki

Quoted 8 lines · Franck Bui-Huu
Anyway I just verified that the current code is broken. I put 2 processes waiting for a suspend evetn by reading the /dev/apm_bios device and a third process that does the suspend event by doing an ioctl() on the apm device. The system doesn't suspend (the kernel dumped a list of the current tasks, instead of printing "Freezing of tasks failed after..." BTW) and eventually the third process exits with an incorrect value: -EINTR.

Quoted 3 lines

Quoted 1 line
Applying your patch makes the previous test case work.
IOW, the patch helps?

yes.

Franck