New Software Suspend Patch for testing.

Hi.

A new 2.5 port of the 2.4 version is available on
www.sourceforge.net/projects/swsusp.

The changes from the last version are not huge; perhaps most significant is that the dynamically allocated bitmaps (in place of pageflags) are implemented. This won't mean anything to most people, but Andrew Morton, Pavel and Patrick should be happy. Note for Patrick: The driver code was using KERN_EMERGENCY to print when devices were suspended/resumed. Can we lower this to INFO (I did something alone these lines in this patch). It mucks up the nice display :>

Hopefully I'll soon find time to start bombarding Pavel, Patrick and Linus with incremental patches for merging :>. In the meantime, please give it a go.

Under 2.4, we use scripts that unload problematic drivers, switch to a text console and so on before starting the process. Hopefully this will end up being unneeded in 2.5 once the driver model is complete. In the meantime, however (especially if you get problems!), you might like to try first with a minimal load, and slowly add things. It does work fine on my Omnibook XE3! YMMV.

Brief howto:
(Assuming you've applied the patch against 2.5.66 - other versions may work too).
1. Patch, compile as normal.
2. Ensure your kernel commandline includes resume=/dev/hdaX where X is a swap partition you will be using. More than one swap partition can be used. This just specifies which will have its header used to indicate that the system is suspended and where to find the data.
3. If you want debugging info: echo 1 20 15 31 > /proc/sys/kernel/swsusp (See line 201ff of kernel/suspend.c for what the numbers mean).
4. echo 4 > /proc/acpi/sleep to start the process.
The system should save the image and powerdown.
If you choose the debugging info, you'll have to press SHIFT at the end of each step.
If you press SHIFT at other times, you can toggle this pausing on and off.
Whether you selected the debugging info or not, you can press ALT to cancel the process.
5. To resume, reboot with the same kernel. The image should be detected and reloaded without you needing to do anything special. Pausing or not is state-persistent from when you suspended.
6. You should (DV) get your system back to where it was when you started the process.

Please report success/failure/questions on the swsusp mailing list. See Florent's home page for details.

Regards,

Nigel

Nigel Cunningham
495 St Georges Road South, Hastings 4201, New Zealand

Be diligent to present yourself approved to God as a workman who does
not need to be ashamed, handling accurately the word of truth.
	-- 2 Timothy 2:14, NASB.

Hi!

Okay, lets take a look.

diff -ruN linux-2.5.66-original/arch/i386/kernel/cpu/mtrr/main.c linux-2.5.66-04/arch/i386/kernel/cpu/mtrr/main.c
@@ -35,6 +35,7 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/smp.h>
+#include <linux/suspend.h>
 
 #include <asm/mtrr.h>
 

@@ -644,6 +645,65 @@
     "write-protect",            /* 5 */
     "write-back",               /* 6 */
 };
-
+ 
+#ifdef SOFTWARE_SUSPEND_MTRR
+struct mtrr_suspend_state
+{
+     mtrr_type ltype;
+     unsigned long lbase;
+     unsigned int lsize;
+};

...

Please convert this to driver model and submit to mtrr maintainer.

diff -ruN linux-2.5.66-original/drivers/base/power.c linux-2.5.66-04/drivers/base/power.c
@@ -35,8 +35,6 @@
 	struct list_head * node;
 	int error = 0;
 
-	printk(KERN_EMERG "Suspending devices\n");
-
 	down_write(&devices_subsys.rwsem);
 	list_for_each(node,&devices_subsys.kset.list) {
 		struct device * dev = to_dev(node);

@@ -73,7 +71,7 @@
 	}
 	up_write(&devices_subsys.rwsem);
 
-	printk(KERN_EMERG "Devices Resumed\n");
+	printk(KERN_INFO "Devices Resumed\n");
 }
 
 /**

@@ -83,7 +81,7 @@
 {
 	struct list_head * entry;
 	
-	printk(KERN_EMERG "Shutting down devices\n");
+	printk(KERN_INFO "Shutting down devices\n");
 
 	down_write(&devices_subsys.rwsem);
 	list_for_each(entry,&devices_subsys.kset.list) {

I guess that submitting this as trivial is okay.

diff -ruN linux-2.5.66-original/drivers/char/vt.c linux-2.5.66-04/drivers/char/vt.c
@@ -149,13 +149,13 @@
 static void vc_init(unsigned int console, unsigned int rows,
 		    unsigned int cols, int do_clear);
 static void blank_screen(unsigned long dummy);
-static void gotoxy(int currcons, int new_x, int new_y);
+void gotoxy(int currcons, int new_x, int new_y);
 static void save_cur(int currcons);
-static void reset_terminal(int currcons, int do_clear);
+void reset_terminal(int currcons, int do_clear);
 static void con_flush_chars(struct tty_struct *tty);
 static void set_vesa_blanking(unsigned long arg);
 static void set_cursor(int currcons);
-static void hide_cursor(int currcons);
+void hide_cursor(int currcons);
 static void unblank_screen_t(unsigned long dummy);
 static void console_callback(void *ignored);
 static void __init con_init_devfs (void);

This is ugly as night. Is not there any public function (sys_write?) you could use instead?

diff -ruN linux-2.5.66-original/drivers/ide/ide-disk.c linux-2.5.66-04/drivers/ide/ide-disk.c
@@ -1515,8 +1515,6 @@
 {
 	ide_drive_t *drive = dev->driver_data;
 
-	printk("Suspending device %p\n", dev->driver_data);
-
 	/* I hope that every freeze operation from the upper levels have
 	 * already been done...
 	 */

@@ -1525,7 +1523,6 @@
 		return 0;
 
 	/* set the drive to standby */
-	printk(KERN_INFO "suspending: %s ", drive->name);
 	do_idedisk_standby(drive);
 	drive->blocked = 1;
 

@@ -1539,8 +1536,8 @@
 
 	if (level != RESUME_RESTORE_STATE)
 		return 0;
-	BUG_ON(!drive->blocked);
-	drive->blocked = 0;
+	if (drive->blocked)
+		drive->blocked--;
 	return 0;
 }
 

Please submitt this to alan, as soon as possible.

@@ -1804,7 +1801,8 @@
if ((!drive->head || drive->head > 16) && !drive->select.b.lba) {
printk(KERN_ERR "%s: INVALID GEOMETRY: %d PHYSICAL HEADS?\n",
drive->name, drive->head);
- if ((drive->id->cfs_enable_2 & 0x3000) && drive->wcache)
+ if (((drive->id->cfs_enable_2 & 0x3000) && drive->wcache) ||
+ ((drive->id->command_set_1 & 0x20) && drive->id->cfs_enable_1 & 0x20))
if (do_idedisk_flushcache(drive))
printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",
drive->name);

Is this swsusp related?

diff -ruN linux-2.5.66-original/include/linux/pagemap.h linux-2.5.66-04/include/linux/pagemap.h
@@ -8,6 +8,9 @@
 #include <linux/fs.h>
 #include <linux/list.h>
 #include <linux/highmem.h>
+#ifdef CONFIG_SOFTWARE_SUSPEND
+#include <linux/suspend.h>
+#endif
 #include <asm/uaccess.h>
 
 /*

Do not use #ifdefs around includes.

@@ -123,6 +126,8 @@
page->mapping = mapping;
page->index = index;

+ if (suspend_task)
+ last_suspend_cache_page = page;
mapping->nrpages++;
pagecache_acct(1);
}

diff -ruN linux-2.5.66-original/include/linux/suspend-debug.h linux-2.5.66-04/include/linux/suspend-debug.h
@@ -0,0 +1,61 @@
+
+#ifndef _LINUX_SWSUSP_DEBUG_H
+#define _LINUX_SWSUSP_DEBUG_H
+
+#include <linux/suspend.h>
+
+#define name_suspend "Suspend Machine:  "
+#define name_resume  "Resume Machine:   "
+#define swsusp_version "beta 19-20"
+#define name_swsusp  "Swsusp " swsusp_version ":   "
+#define console_suspend "S U S P E N D   T O   D I S K"
+#define console_resume "R E S U M E   F R O M   D I S K"

This is ugly. Are you trying to make it hard to read on purpose?

diff -ruN linux-2.5.66-original/include/linux/sysctl.h linux-2.5.66-04/include/linux/sysctl.h
@@ -129,6 +129,7 @@
 	KERN_CADPID=54,		/* int: PID of the process to notify on CAD */
 	KERN_PIDMAX=55,		/* int: PID # limit */
   	KERN_CORE_PATTERN=56,	/* string: pattern for core-file names */
+        KERN_SWSUSP=57,		/* struct: interface to configure & activate software suspension */
 };
 
 

Is sysctl being used besides debugging?

unsigned char software_suspend_enabled = 0;
+unsigned int suspend_task = 0;
+/*
+ * Poll the swsusp state every second
+ */
+#define SWSUSP_CHECK_TIMEOUT (HZ)

What is this?

/*
* XXX: We try to keep some more pages free so that I/O operations succeed
* without paging. Might this be more?
*/
-#define PAGES_FOR_IO 512
+#define PAGES_FOR_IO 512
+
+#ifdef DEBUG_DEFAULT
+int currentstage = 0;
+int swsusp_state[5] = {0, /* when set to 1 swsusp_mainloop activates software_suspend (2.4 only)
+ bit 0: off = normal state, on = suspend required
+ bit 1: aborting suspend
+ */

Please kill 2.4 stuff from 2.5 patch.

- if (mode == MARK_SWAP_RESUME) {
- if (!memcmp("S1",cur->swh.magic.magic,2))
- memcpy(cur->swh.magic.magic,"SWAP-SPACE",10);
- else if (!memcmp("S2",cur->swh.magic.magic,2))
- memcpy(cur->swh.magic.magic,"SWAPSPACE2",10);
- else printk("%sUnable to find suspended-data signature (%.10s - misspelled?\n",
- name_resume, cur->swh.magic.magic);
- } else {
- if ((!memcmp("SWAP-SPACE",cur->swh.magic.magic,10)))
- memcpy(cur->swh.magic.magic,"S1SUSP....",10);
- else if ((!memcmp("SWAPSPACE2",cur->swh.magic.magic,10)))
- memcpy(cur->swh.magic.magic,"S2SUSP....",10);
- else panic("\nSwapspace is not swapspace (%.10s)\n", cur->swh.magic.magic);
- cur->link.next = prev; /* prev is the first/last swap page of the resume area */
- /* link.next lies *no more* in last 4/8 bytes of magic */
+ switch(mode) {
+ case MARK_SWAP_RESUME:
+ if (!memcmp("1R",cur.pointer->swh.magic.magic,2))
+ memcpy(cur.pointer->swh.magic.magic,"SWAP-SPACE",10);
+ else if (!memcmp("2R",cur.pointer->swh.magic.magic,2))
+ memcpy(cur.pointer->swh.magic.magic,"SWAPSPACE2",10);
+ else printk(name_resume "Unable to find suspended-data signature (%.10s - misspelled?\n",
+ cur.pointer->swh.magic.magic);
+ break;

I guess this is nicer than previous code, good.

Snip. Sorry, have to go.
Pavel

When do you have heart between your knees?

Hi.

Okee doke.

I didn't know about sys_write. I'll take a look at it.

Okay.

Yes. Under 2.4, some people found that if the writeback cache isn't flushed before we powerdown, the image isn't completely saved. This was the fix (the first test is the original one, the second is based upon hdparm's tests for writeback cache).

Okay.

Nope. Just cut and paste from what was already in. I didn't like it much either, but haven't changed it (this isn't _all_ my work).

Yes. The main non-debugging thing that occurs to me right now is that you can specify the image size you would like swsusp to aim for.

Used in the 2.4 version for kswsusp daemon. It provides another way to start the process. I agree with you that we probably don't want to implement this in 2.5 - I just forgot about removing it (I'm not claiming that this code is perfectly cleaned up!)

Yes, I will.

This is Florent's, I believe. I actually think the header stuff is still ugly, but haven't gotten around to cleaning it up yet.

Thanks for the suggestions on how to do a better job. I know I'm not finished - I just wanted to get it tested to start with.

Regards,

Nigel

Nigel Cunningham
495 St Georges Road South, Hastings 4201, New Zealand

Be diligent to present yourself approved to God as a workman who does
not need to be ashamed, handling accurately the word of truth.
	-- 2 Timothy 2:14, NASB.

Hi!

But this looks like you are fixing generic bug, which could make kernel do something very wrong even without swsusp, right? If so, submit it to Alan, ASAP.

Sorry, I figured out it might be easier if I give you at least some comments before you start splitting patch up -- it should be easier for you this way.

Pavel

When do you have heart between your knees?

I'll grab the older ATA spec and take a look. The other changes are broken, but this one looks quite beliavable

Hi!

@@ -536,6 +519,9 @@
struct page *page;
int i;
int cold;
+#if CONFIG_SOFTWARE_SUSPEND
+ static unsigned int loopcount;
+#endif

if (wait)
might_sleep();
@@ -589,7 +575,23 @@

/* here we're in the low on memory slow path */

+#if CONFIG_SOFTWARE_SUSPEND
+ loopcount=0;
+#endif
rebalance:
+#ifdef CONFIG_SOFTWARE_SUSPEND
+ if(gfp_mask & __GFP_FAST) {
+/* when using memeat, we ask for all pages that are really free.

+   5 calls to reschedule should be sufficient to recall all of them since
+   when a page can be found, it is after only one reschedule.
+   Actually I consider this as a bug of alloc_pages, since allocating a
+   page should not hang in an endless loop when it is clear that no
+   memory is available (cbd) */	  

+ loopcount++;
+ if(loopcount > 5)
+ return NULL;
+ }
+#endif
if ((current->flags & (PF_MEMALLOC | PF_MEMDIE)) && !in_interrupt()) {
/* go through the zonelist yet again, ignoring mins */ for (i = 0; zones[i] != NULL; i++) {

Again this is *really really* ugly. Why do you need it? 2.5. has special function for freeing memory.

Pavel

When do you have heart between your knees?

I have a (fairly large) request:

Would you mind updating Documentation/swsusp.txt (and moving it to Documentation/power/swsusp.txt)?

This will accomplish a few things:

- Make the documentation readable.

- Explain exactly how swsusp works internally.

- Explain how to use it.

- Explain what the current dependencies and bugs are.

- Explain what the cryptic debug values above are.

Frankly, the code is a mess and difficult to follow. It's also poorly commented. I spent the time about a month ago unwinding and deciphering it. Unfortunately, my work conflicts heavily with the work that you're doing, and the lack of documentation describing the intent of the code makes it difficult to make sane judgements of it.

Thanks,

-pat

Hi.

Okay. I've been meaning to update it.

One more problem is that you're also trying to follow a moving target :>. In response to Pavel's last request, I'm preparing to work toward using a linked list for the meta information (to allow unlimited image size), and at the request of others, compressing (zlib) the saved pages to make the image faster to save/load.

As to the layout and documentation, I hate them too. I had to keep things as similar as possible to existing code to make merging easier, but now that I'm merged with the 2.4 code, perhaps I can do some cleanups.

Regards,

Nigel

The issue is not following a moving target. It's reading poorly maintained and documented code.

If I were you, I would work first at cleaning up the code, then work on adding features to it. There were many opportunities I found for improvement when reworking the code, plus it became orders of magnitude easier to follow.

-pat