: In kernel hibernation, suspend to both

Hi Rafael,

Here is the same thing, this time with signed off. Looks like all the memory used for hibernation is being released before power_down(), so I guess we should be good. The patch is against linux-next.

------------------------------
Enable suspend to both for in-kernel hibernation.

It is often useful to suspend to memory after hibernation image has been written to disk. If the battery runs out or power is otherwise lost, the computer will resume from the hibernated image. If not, it will resume from memory and hibernation image will be discarded.

Signed-off-by Bojan Smojver <[email protected]>

---
 Documentation/power/swsusp.txt |    5 +++++
 kernel/power/hibernate.c       |   36 ++++++++++++++++++++++++++++++++++++
 kernel/power/power.h           |    3 +++
 kernel/power/swap.c            |   28 ++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 0 deletions(-)

Documentation/power/swsusp.txt +5 -0

index ac190cf..92341b8 100644
@@ -33,6 +33,11 @@ echo shutdown > /sys/power/disk; echo disk > /sys/power/state
 
 echo platform > /sys/power/disk; echo disk > /sys/power/state
 
+. If you would like to write hibernation image to swap and then suspend
+to RAM (provided your platform supports it), you can try
+
+echo suspend > /sys/power/disk; echo disk > /sys/power/state
+
 . If you have SATA disks, you'll need recent kernels with SATA suspend
 support. For suspend and resume to work, make sure your disk drivers
 are built into kernel -- not modules. [There's way to make

kernel/power/hibernate.c +8 -0

index e09dfbf..3b5726f 100644
@@ -5,6 +5,7 @@
  * Copyright (c) 2003 Open Source Development Lab
  * Copyright (c) 2004 Pavel Machek <[email protected]>
  * Copyright (c) 2009 Rafael J. Wysocki, Novell Inc.
+ * Copyright (C) 2012 Bojan Smojver <[email protected]>
  *
  * This file is released under the GPLv2.
  */

@@ -44,6 +45,9 @@ enum {
 	HIBERNATION_PLATFORM,
 	HIBERNATION_SHUTDOWN,
 	HIBERNATION_REBOOT,
+#ifdef CONFIG_SUSPEND
+	HIBERNATION_SUSPEND,
+#endif
 	/* keep last */
 	__HIBERNATION_AFTER_LAST
 };

@@ -572,6 +576,10 @@ int hibernation_platform_enter(void)
  */
 static void power_down(void)
 {
+#ifdef CONFIG_SUSPEND
+	int error;
+#endif
+
 	switch (hibernation_mode) {
 	case HIBERNATION_REBOOT:
 		kernel_restart(NULL);

kernel/power/hibernate.c +16 -0

+		if (error) {
+			if (hibernation_ops)
+				hibernation_mode = HIBERNATION_PLATFORM;
+			else
+				hibernation_mode = HIBERNATION_SHUTDOWN;
+			power_down();
+		}
+		/*
+		 * Restore swap signature.
+		 */
+		error = swsusp_unmark();
+		if (error)
+			printk(KERN_ERR "PM: Swap will be unusable! "
+			                "Try swapon -a.\n");
+		return;
+#endif
 
 	}
 

kernel/power/hibernate.c +0 -0

 	kernel_halt();
 	/*

Quoted 1 line

+ error = suspend_devices_and_enter(PM_SUSPEND_MEM);

I can imagine running into a host of problems here, since the suspend sequence is not carried out fully, from the beginning.

For example, this will skip sending out the PM_SUSPEND_PREPARE and the PM_POST_SUSPEND notifiers. Worse, we actually send out the PM_HIBERNATION_PREPARE
and PM_POST_HIBERNATION notifiers and then do a suspend instead, underneath!

(Similar cases for the rest of the notifiers sent during suspend vs hibernation).

Don't we need to handle such things properly, in order to make suspend-to-both
work reliably?

Honest answer - I have absolutely no idea. I've seen the code of suspend-utils (i.e. user mode stuff) and it seems to me that it does exactly this. Could be wrong of course, just like many times before.

Quoted 3 lines · Bojan Smojver

Honest answer - I have absolutely no idea. I've seen the code of suspend-utils (i.e. user mode stuff) and it seems to me that it does exactly this. Could be wrong of course, just like many times before.

What makes me think that this may not be that bad is the fact that

post-resume, it will actually be hibernation code that will be unwinding

things. So like this: prepare for hibernation, create image, suspend to

memory

(equivalent to hibernation failure of some kind, really), resume

from memory, unwind from unsuccessful hibernation.

No?

This is the point where your patch gets scary - Suspend is not carried out in its fullest sense; instead you jump directly to suspend_devices_and_enter(). Luckily, most of the things that happen before this are common between suspend and hibernation. However, one thing that really stands out is the notifications: if you directly call suspend_devices_and_enter(), we end up missing the PM_SUSPEND_PREPARE notifications.

And there is no guarantee that everybody implements the same thing for both PM_SUSPEND_PREPARE and PM_HIBERNATION_PREPARE notifications. That is the reason I don't think it is safe.

Quoted 7 lines · Srivatsa S. Bhat

This is the point where your patch gets scary - Suspend is not carried out in its fullest sense; instead you jump directly to suspend_devices_and_enter(). Luckily, most of the things that happen before this are common between suspend and hibernation. However, one thing that really stands out is the notifications: if you directly call suspend_devices_and_enter(), we end up missing the PM_SUSPEND_PREPARE notifications.

I'm guessing because at the point of suspend to RAM here, we are not really doing the real, full suspend to RAM, things would be different. At this point, not the whole system is alive - most of it is frozen. Only devices required for image writing are alive. Or maybe I'm misunderstanding the hibernation process...

Quoted 3 lines

And there is no guarantee that everybody implements the same thing for both PM_SUSPEND_PREPARE and PM_HIBERNATION_PREPARE notifications. That is the reason I don't think it is safe.

OK.

So, you would do:

- prepare for hibernation (includes suspension of devices)

- resume devices involved in image writing

- write image
- prepare for suspend
- suspend to RAM
- resume from RAM
- post suspend from RAM

- unwind unsuccessful hibernation

Isn't that going to confuse devices involved in image writing even more by essentially executing post-resume twice (once for suspend to RAM, once for hibernation)?

Yes, you misunderstand it.

I'll work on redoing the patch with proper prepare to suspend to RAM.

If I'm reading the source of suspend.c correctly, this would then mean that instead of calling suspend_devices_and_enter() I should be calling pm_suspend(), correct?

This function calls enter_state(), which in turn does prepare/suspend/finish.

+ error = pm_suspend(PM_SUSPEND_MEM);

Quoted 1 line · Bojan Smojver

+ error = pm_suspend(PM_SUSPEND_MEM);

Er, that won't work. The lock is being held by the caller. OK, we'll have to make it complicated, I guess...

Totally untested...

No. That hangs my box.

This triggers a bug in workqueues code (essentially the same as the previous patch, except for sys_sync() not being done):

Alan/Srivatsa,

Coming back to the explanation of how this whole thing works, it would seem that at the point of image writing all devices are fully functional (not just some, as I mistakenly believed). However, the processes are supposed to be already frozen, right? Calling suspend_prepare(), which will essentially try to freeze the processes and kernel threads, seems like the wrong thing to do.

Did you guys mean that we should be calling pm_notifier_call_chain(PM_SUSPEND_PREPARE) only here?

Quoted 4 lines · Bojan Smojver

No. That hangs my box.

This triggers a bug in workqueues code (essentially the same as the previous patch, except for sys_sync() not being done):

Alan/Srivatsa,

Coming back to the explanation of how this whole thing works, it would seem that at the point of image writing all devices are fully functional (not just some, as I mistakenly believed). However, the processes are supposed to be already frozen, right? Calling suspend_prepare(), which will essentially try to freeze the processes and kernel threads, seems like the wrong thing to do.

Did you guys mean that we should be calling pm_notifier_call_chain(PM_SUSPEND_PREPARE) only here?

Quoted 15 lines · Bojan Smojver

Quoted 4 lines · Bojan Smojver

No. That hangs my box.

This triggers a bug in workqueues code (essentially the same as the previous patch, except for sys_sync() not being done):

Alan/Srivatsa,

Coming back to the explanation of how this whole thing works, it would seem that at the point of image writing all devices are fully functional (not just some, as I mistakenly believed). However, the processes are supposed to be already frozen, right? Calling suspend_prepare(), which will essentially try to freeze the processes and kernel threads, seems like the wrong thing to do.

Quoted 3 lines

Did you guys mean that we should be calling pm_notifier_call_chain(PM_SUSPEND_PREPARE) only here?

Exactly! And also arrange for the corresponding PM_POST_SUSPEND notification to happen at the end of suspend-to-ram stage...

Quoted 22 lines · Srivatsa S. Bhat

Quoted 18 lines · Bojan Smojver

Quoted 4 lines · Bojan Smojver

No. That hangs my box.

This triggers a bug in workqueues code (essentially the same as the previous patch, except for sys_sync() not being done):

Alan/Srivatsa,

Coming back to the explanation of how this whole thing works, it would seem that at the point of image writing all devices are fully functional (not just some, as I mistakenly believed). However, the processes are supposed to be already frozen, right? Calling suspend_prepare(), which will essentially try to freeze the processes and kernel threads, seems like the wrong thing to do.

Did you guys mean that we should be calling pm_notifier_call_chain(PM_SUSPEND_PREPARE) only here?

Quoted 2 lines

Exactly! And also arrange for the corresponding PM_POST_SUSPEND notification to happen at the end of suspend-to-ram stage...

Actually, no. The notifiers are supposed to be called when user space is available, otherwise some things will break badly (firmware loading for one example IIRC).

which I suppose is what the first Bojan's patch did, right?

Quoted 1 line · Rafael J. Wysocki

which I suppose is what the first Bojan's patch did, right?

Correct. No extra notifiers were sent, so whatever hibernation code did was it.

That patch is here:

http://marc.info/?l=linux-kernel&m=133651588229850&w=2

Quoted 1 line

http://marc.info/?l=linux-kernel&m=133651588229850&w=2

Now that 3.4 is out, just a ping to have this in the 3.5 queue.

Quoted 3 lines · Bojan Smojver

Quoted 1 line · Rafael J. Wysocki

which I suppose is what the first Bojan's patch did, right?

Quoted 6 lines

Correct. No extra notifiers were sent, so whatever hibernation code did was it.

That patch is here:

http://marc.info/?l=linux-kernel&m=133651588229850&w=2

Now that 3.4 is out, just a ping to have this in the 3.5 queue.

No, I'm not going to put that into 3.5, sorry. It can go into 3.6.

Quoted 1 line · Rafael J. Wysocki

No, I'm not going to put that into 3.5, sorry. It can go into 3.6.

May I ask why? I do not see why people should wait another few months to get this.

It's not urgent and it _can_ wait.

Quoted 1 line · Rafael J. Wysocki

It's not urgent and it _can_ wait.

OK. I'll go with "better late than never", in that case. :-)

Let me ask again, can you send the last version of that patch so that I can give it a run pls?

Sorry. That patch is here (pretty much the original with signed-off):

http://marc.info/?l=linux-kernel&m=133651588229850&w=2

Quoted 3 lines · Bojan Smojver

Sorry. That patch is here (pretty much the original with signed-off):

http://marc.info/?l=linux-kernel&m=133651588229850&w=2

Ok, I finally gave the above a run, here's the dmesg after a successful suspend-resume cycle. It looks ok except the funny characters after

"[ 380.938864] PM: Compressing and saving image data (75689 pages) ..."

I'll keep running it the next couple of days/weeks and report any instabilities.

Quoted 8 lines

Ok, I finally gave the above a run, here's the dmesg after a successful suspend-resume cycle. It looks ok except the funny characters after

"[ 380.938864] PM: Compressing and saving image data (75689 pages) ..."

I'll keep running it the next couple of days/weeks and report any instabilities.

Thanks for testing. The funny characters are backspaces printed by the progress indicator. :-)

Normal, suspend-to-disk cycle doesn't put that output in dmesg.

Quoted 1 line · Borislav Petkov

Normal, suspend-to-disk cycle doesn't put that output in dmesg.

It does, but that kernel state never gets used again in that case. The state before the image writing starts is the one used on thaw, so these messages never appear anywhere.

But yeah, I do not see why these should ever go to any dmesg output. They should just go to the screen.

Quoted 2 lines · Borislav Petkov

Let me ask again, can you send the last version of that patch so that I can give it a run pls?

Sorry. That patch is here (pretty much the original with signed-off):

http://marc.info/?l=linux-kernel&m=133651588229850&w=2

Quoted 6 lines · Bojan Smojver

Quoted 2 lines · Borislav Petkov

Let me ask again, can you send the last version of that patch so that I can give it a run pls?

Quoted 3 lines

Sorry. That patch is here (pretty much the original with signed-off):

http://marc.info/?l=linux-kernel&m=133651588229850&w=2

Queued up for 3.6 in linux-pm/linux-next.

Quoted 9 lines · Rafael J. Wysocki

Quoted 8 lines · Bojan Smojver

Quoted 2 lines · Borislav Petkov

Let me ask again, can you send the last version of that patch so that I can give it a run pls?

Sorry. That patch is here (pretty much the original with signed-off):

Quoted 3 lines

Quoted 1 line

http://marc.info/?l=linux-kernel&m=133651588229850&w=2

Queued up for 3.6 in linux-pm/linux-next.

IMHO, this one still needs a fix for the progress indicator output landing in dmesg and filling it up with \x08 chars:

http://marc.info/?l=linux-pm&m=133848160403298&w=2

Hmm. Is there any fix, though?

Quoted 1 line · Rafael J. Wysocki

Hmm. Is there any fix, though?

How do we tell printk() to not dump stuff into dmesg?

Or, how do we
otherwise just print to the console?

Quoted 5 lines · Bojan Smojver

Quoted 1 line · Rafael J. Wysocki

Hmm. Is there any fix, though?

How do we tell printk() to not dump stuff into dmesg?

You can't; that's what printk() does, by definition. If you don't want stuff to go into the dmesg buffer then don't call printk().

Quoted 2 lines

Or, how do we
otherwise just print to the console?

That's a better question. Unfortunately, I don't know the answer.

Ok, I don't know the hibernation code at all but let me give it a try:

how about teach the piece that parses

$ echo suspend > /sys/power/disk

to make a note to self that we're doing the hibrid hibernation method and then the function that prints the progress indicator to be let known
of the aforementioned self-note so that it doesn't do any output?

Purely hypothetical, of course.

Quoted 11 lines · Borislav Petkov

Ok, I don't know the hibernation code at all but let me give it a try:

how about teach the piece that parses

$ echo suspend > /sys/power/disk

to make a note to self that we're doing the hibrid hibernation method and then the function that prints the progress indicator to be let known
of the aforementioned self-note so that it doesn't do any output?

Quoted 1 line

Purely hypothetical, of course.

Yeah, we already have a variable for that. But, we can just do it simpler - paint one dot every 10% or something, without any backspaces. That will fix it for sure.

Quoted 11 lines · Borislav Petkov

Ok, I don't know the hibernation code at all but let me give it a try:

how about teach the piece that parses

$ echo suspend > /sys/power/disk

to make a note to self that we're doing the hibrid hibernation method and then the function that prints the progress indicator to be let known
of the aforementioned self-note so that it doesn't do any output?

Quoted 1 line

Purely hypothetical, of course.

Yeah, we already have a variable for that. But, we can just do it simpler - paint one dot every 10% or something, without any backspaces. That will fix it for sure.

Yeah, that will do.

Quoted 1 line · Rafael J. Wysocki

Yeah, that will do.

Something like this maybe. I didn't really test this, so Borislav, please let me know whether this is something that may works for you.

Quoted 5 lines · Bojan Smojver

Quoted 1 line · Rafael J. Wysocki

Yeah, that will do.

Something like this maybe. I didn't really test this, so Borislav, please let me know whether this is something that may works for you.

Yeah, this could probably work but it changes the progress indicator unconditionally.

I don't know what the general consensus is about the progress indicator but if you want to keep the old behavior too, you probably need to carve out the printk functionality from save_image() and pass flags to the new function which printks different output depending on how it is.

If we don't want the current output with how many pages it needs to use on swap, then we can do the dots.

Frankly, I don't have a preference.

So, I think printing a status line every 10% would be fine.

Quoted 1 line · Rafael J. Wysocki

So, I think printing a status line every 10% would be fine.

My patch prints a dot every 10%, which is not on a separate line. Did you want me print a full line instead or are you saying dots are fine?

PS. Yeah, the current printout with \b is a hack and I do not particularly like it either. Proper way would be with plymouth or something like that, but I got no joy with that on that list. One day, maybe...

I think I'd prefer a full status line every 10%. Or even maybe every 20%.

Quoted 2 lines · Rafael J. Wysocki

I think I'd prefer a full status line every 10%. Or even maybe every 20%.

Here is one that does a line every 10%:

which, on the whole, is nice and compact and still contains the whole info.

Quoted 2 lines · Borislav Petkov

which, on the whole, is nice and compact and still contains the whole info.

Yeah, that was the deal with the dots too. But, Rafael suggested we do lines (and I see his point). This way any other lines printed in the meantime (I have seen that happen) will also print properly. Ditto that last line - 100% is still not quite finished. :-)

I'd use a slightly different status line, though. What about:

"PM: Image saving progress: %d%%"

Then, if something is printed in between those lines, it'll still be clear
what they mean.

Quoted 1 line

PS. Yeah, the current printout with \b is a hack and I do not

particularly

Quoted 1 line

like it either. Proper way would be with plymouth or something like

that, but

Quoted 1 line

I got no joy with that on that list. One day, maybe...

I'm not sure what you mean?

Quoted 12 lines · Srivatsa S. Bhat

Quoted 1 line

+ error = suspend_devices_and_enter(PM_SUSPEND_MEM);

I can imagine running into a host of problems here, since the suspend sequence is not carried out fully, from the beginning.

For example, this will skip sending out the PM_SUSPEND_PREPARE and the PM_POST_SUSPEND notifiers. Worse, we actually send out the PM_HIBERNATION_PREPARE
and PM_POST_HIBERNATION notifiers and then do a suspend instead, underneath!

Quoted 6 lines

(Similar cases for the rest of the notifiers sent during suspend vs hibernation).

Don't we need to handle such things properly, in order to make suspend-to-both
work reliably?

Honest answer - I have absolutely no idea. I've seen the code of suspend-utils (i.e. user mode stuff) and it seems to me that it does exactly this. Could be wrong of course, just like many times before.

Rafael?

Quoted 5 lines · Bojan Smojver

Quoted 4 lines · Srivatsa S. Bhat

Quoted 1 line

+ error = suspend_devices_and_enter(PM_SUSPEND_MEM);

I can imagine running into a host of problems here, since the

suspend

Quoted 3 lines

Quoted 3 lines

sequence is not carried out fully, from the beginning.

For example, this will skip sending out the PM_SUSPEND_PREPARE and

the

Quoted 12 lines

Quoted 11 lines

PM_POST_SUSPEND notifiers. Worse, we actually send out the PM_HIBERNATION_PREPARE
and PM_POST_HIBERNATION notifiers and then do a suspend instead, underneath!

(Similar cases for the rest of the notifiers sent during suspend vs hibernation).

Don't we need to handle such things properly, in order to make suspend-to-both
work reliably?

Quoted 5 lines

Honest answer - I have absolutely no idea. I've seen the code of suspend-utils (i.e. user mode stuff) and it seems to me that it does exactly this. Could be wrong of course, just like many times before.

Rafael?

Sorry, that has fallen out of my radar somehow.

Srivatsa is right, we should generally pay attention to those details.

I think we should generally use a different "prepare" notification for the
save-image-and-suspend case.

Thanks,
Rafael

Quoted 37 lines · Rafael J. Wysocki

Quoted 5 lines · Bojan Smojver

Quoted 4 lines · Srivatsa S. Bhat

Quoted 1 line

+ error = suspend_devices_and_enter(PM_SUSPEND_MEM);

I can imagine running into a host of problems here, since the

suspend

Quoted 3 lines

Quoted 3 lines

sequence is not carried out fully, from the beginning.

For example, this will skip sending out the PM_SUSPEND_PREPARE and

the

Quoted 12 lines

Quoted 11 lines

PM_POST_SUSPEND notifiers. Worse, we actually send out the PM_HIBERNATION_PREPARE
and PM_POST_HIBERNATION notifiers and then do a suspend instead, underneath!

(Similar cases for the rest of the notifiers sent during suspend vs hibernation).

Don't we need to handle such things properly, in order to make suspend-to-both
work reliably?

Quoted 5 lines

Honest answer - I have absolutely no idea. I've seen the code of suspend-utils (i.e. user mode stuff) and it seems to me that it does exactly this. Could be wrong of course, just like many times before.

Rafael?

Sorry, that has fallen out of my radar somehow.

Srivatsa is right, we should generally pay attention to those details.

I think we should generally use a different "prepare" notification for the
save-image-and-suspend case.

Quoted 2 lines

Thanks,
Rafael

OK, I will try to rework then, if that is the case.

What I don't understand is this: should the hibernation fail for some reason, we would get the same hibernation code unwind that failure, right?

So, a suspend after the image write will be just one long "failure", after which hibernation code has to unwind again. No?

Quoted 1 line

So, a suspend after the image write will be just one long "failure",

after

Quoted 1 line

which hibernation code has to unwind again. No?

Hmm. Good question. It should be like this I think, although there may be some corner cases lurking.

Quoted 6 lines · Rafael J. Wysocki

Quoted 1 line

So, a suspend after the image write will be just one long "failure",

after

Quoted 1 line

which hibernation code has to unwind again. No?

Hmm. Good question. It should be like this I think, although there may be some corner cases lurking.

AFAICT, the user space suspend to both works this way.

If I understand thing correctly, at the point of suspend to memory, we are not really running a full system any more. All the processes have been frozen and they've been told that hibernation is in progress. Ditto devices, minus the ones we use to write to swap. So, when hibernation code unwinds post resume from memory, it would be doing the correct thing.

Srivatsa, does that make sense to you?

But does it carry out suspend-to-ram in full, or does it skip the PM_SUSPEND_PREPARE notifications?

See my concerns about the preparation stage for suspend, in my other mail as well. I would have been less worried if we did things in full:
1. prepare for hibernation
2. create image
3. prepare for suspend
4. do suspend
5. do resume
6. unwind from hibernation code (like a failed hibernation).

Still no guarantees, but somewhat better.
(consider what will happen if some code expects completion of an operation,
successful or otherwise, before starting another.. and hence is unprepared
for something like:
hibernation prepare -> suspend prepare -> post suspend -> post hibernation)

If we don't rework this thing carefully via a special notification for save-image-and-suspend like what Rafael suggested, and instead try to go
with existing stuff as it is, I would definitely suggest putting this feature
under something like CONFIG_EXPERIMENTAL ;-)