[PATCH 0/4] swsusp: more cleanups

[PATCH 0/4] swsusp: more cleanups

[Rafael J. Wysocki]

Hi,

The following series of patches consists of some nonessential cleanups
for swsusp.  Still, if there are no objections, please consider them for
including in 2.6.15.

The patches are against 2.6.14-rc4-mm1 with the

swsusp-cleanups.patch
swsusp-remove-unneccessary-includes.patch

applied.

Greetings,
Rafael

[PATCH 1/4] swsusp: get rid of unnecessary wrapper function

[Rafael J. Wysocki]

The following patch merges two functions in a trivial way.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Index: linux-2.6.14-rc4-mm1/kernel/power/snapshot.c
===================================================================
--- linux-2.6.14-rc4-mm1.orig/kernel/power/snapshot.c	2005-10-17 23:28:36.000000000 +0200
+++ linux-2.6.14-rc4-mm1/kernel/power/snapshot.c	2005-10-17 23:28:40.000000000 +0200
@@ -384,7 +384,7 @@
 	return 0;
 }
 
-static int suspend_prepare_image(void)
+asmlinkage int swsusp_save(void)
 {
 	int error;
 
@@ -433,9 +433,3 @@
 	printk("swsusp: critical section/: done (%d pages copied)\n", nr_copy_pages );
 	return 0;
 }
-
-
-asmlinkage int swsusp_save(void)
-{
-	return suspend_prepare_image();
-}

Re: [PATCH 1/4] swsusp: get rid of unnecessary wrapper function

[Pavel Machek]

Hi!

> The following patch merges two functions in a trivial way.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

ACK.
							Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

[PATCH 2/4] swsusp: clean up resume error path

[Rafael J. Wysocki]

The following patch removes an incorrect call to restore_highmem() from
the resume error path (there's no saved highmem in that case) and makes
swsusp touch the softlockup watchdog if there's no error (currently it only
touches the watchdog if an error occurs).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Index: linux-2.6.14-rc4-mm1/kernel/power/swsusp.c
===================================================================
--- linux-2.6.14-rc4-mm1.orig/kernel/power/swsusp.c	2005-10-17 23:28:34.000000000 +0200
+++ linux-2.6.14-rc4-mm1/kernel/power/swsusp.c	2005-10-17 23:28:47.000000000 +0200
@@ -604,6 +604,7 @@
 	/* Restore control flow magically appears here */
 	restore_processor_state();
 	restore_highmem();
+	touch_softlockup_watchdog();
 	device_power_up();
 	local_irq_enable();
 	return error;
@@ -628,7 +629,6 @@
 	 */
 	swsusp_free();
 	restore_processor_state();
-	restore_highmem();
 	touch_softlockup_watchdog();
 	device_power_up();
 	local_irq_enable();

Re: [PATCH 2/4] swsusp: clean up resume error path

[Pavel Machek]

Hi!

> The following patch removes an incorrect call to restore_highmem() from
> the resume error path (there's no saved highmem in that case) and makes
> swsusp touch the softlockup watchdog if there's no error (currently it only
> touches the watchdog if an error occurs).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Index: linux-2.6.14-rc4-mm1/kernel/power/swsusp.c
> ===================================================================
> --- linux-2.6.14-rc4-mm1.orig/kernel/power/swsusp.c	2005-10-17 23:28:34.000000000 +0200
> +++ linux-2.6.14-rc4-mm1/kernel/power/swsusp.c	2005-10-17 23:28:47.000000000 +0200
> @@ -628,7 +629,6 @@
>  	 */
>  	swsusp_free();
>  	restore_processor_state();
> -	restore_highmem();
>  	touch_softlockup_watchdog();
>  	device_power_up();
>  	local_irq_enable();

I don't like this one. restore_highmem() does freeing of allocated
pages. If swsusp_arch_suspend() fails in specific way, I suspect it
could leak highmem.

								Pavel


-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [PATCH 2/4] swsusp: clean up resume error path

[Rafael J. Wysocki]

Hi,

On Tuesday, 18 of October 2005 01:47, Pavel Machek wrote:
> Hi!
> 
> > The following patch removes an incorrect call to restore_highmem() from
> > the resume error path (there's no saved highmem in that case) and makes
> > swsusp touch the softlockup watchdog if there's no error (currently it only
> > touches the watchdog if an error occurs).
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Index: linux-2.6.14-rc4-mm1/kernel/power/swsusp.c
> > ===================================================================
> > --- linux-2.6.14-rc4-mm1.orig/kernel/power/swsusp.c	2005-10-17 23:28:34.000000000 +0200
> > +++ linux-2.6.14-rc4-mm1/kernel/power/swsusp.c	2005-10-17 23:28:47.000000000 +0200
> > @@ -628,7 +629,6 @@
> >  	 */
> >  	swsusp_free();
> >  	restore_processor_state();
> > -	restore_highmem();
> >  	touch_softlockup_watchdog();
> >  	device_power_up();
> >  	local_irq_enable();
> 
> I don't like this one. restore_highmem() does freeing of allocated
> pages. If swsusp_arch_suspend() fails in specific way, I suspect it
> could leak highmem.

The pages to be freed are only allocated in suspend_prepare_image()
(now swsusp_save()), which is on suspend, and this is the resume
error path.

The boot kernel that performs the resume does not save highmem,
so it need not and IMO it should not call restore_highmem() in the
error path (if nothing more, it's misleading).  OTOH if the resume
succeeds, restore_highmem() will be called from swsusp_suspend().

Gretings,
Rafael

Re: [PATCH 2/4] swsusp: clean up resume error path

[Pavel Machek]

Hi!

> > I don't like this one. restore_highmem() does freeing of allocated
> > pages. If swsusp_arch_suspend() fails in specific way, I suspect it
> > could leak highmem.
> 
> The pages to be freed are only allocated in suspend_prepare_image()
> (now swsusp_save()), which is on suspend, and this is the resume
> error path.

Ok, yes, you seem to be right. I was probably confused by my own
code. ACK on the patch (if it helps you).

								Pavel

-- 
Boycott Kodak -- for their patent abuse against Java.

[PATCH 3/4] swsusp: two simplifications

[Rafael J. Wysocki]

The following patch simplifies the progress meter in disk.c:free_some_memory()
and makes disk.c:pm_suspend_disk() call device_resume() explicitly in the
suspend path.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Index: linux-2.6.14-rc4-mm1/kernel/power/disk.c
===================================================================
--- linux-2.6.14-rc4-mm1.orig/kernel/power/disk.c	2005-10-17 23:28:33.000000000 +0200
+++ linux-2.6.14-rc4-mm1/kernel/power/disk.c	2005-10-17 23:28:52.000000000 +0200
@@ -92,10 +92,7 @@
 	printk("Freeing memory...  ");
 	while ((tmp = shrink_all_memory(10000))) {
 		pages += tmp;
-		printk("\b%c", p[i]);
-		i++;
-		if (i > 3)
-			i = 0;
+		printk("\b%c", p[i++ % 4]);
 	}
 	printk("\bdone (%li pages freed)\n", pages);
 }
@@ -177,13 +174,12 @@
 		goto Done;
 
 	if (in_suspend) {
+		device_resume();
 		pr_debug("PM: writing image.\n");
 		error = swsusp_write();
 		if (!error)
 			power_down(pm_disk_mode);
 		else {
-		/* swsusp_write can not fail in device_resume,
-		   no need to do second device_resume */
 			swsusp_free();
 			unprepare_processes();
 			return error;
Index: linux-2.6.14-rc4-mm1/kernel/power/swsusp.c
===================================================================
--- linux-2.6.14-rc4-mm1.orig/kernel/power/swsusp.c	2005-10-17 23:28:47.000000000 +0200
+++ linux-2.6.14-rc4-mm1/kernel/power/swsusp.c	2005-10-17 23:28:52.000000000 +0200
@@ -562,7 +562,7 @@
 int swsusp_write(void)
 {
 	int error;
-	device_resume();
+
 	lock_swapdevices();
 	error = write_suspend_image();
 	/* This will unlock ignored swap devices since writing is finished */

Re: [PATCH 3/4] swsusp: two simplifications

[Pavel Machek]

Hi!

> The following patch simplifies the progress meter in disk.c:free_some_memory()
> and makes disk.c:pm_suspend_disk() call device_resume() explicitly in the
> suspend path.

ACK.

> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> --- linux-2.6.14-rc4-mm1.orig/kernel/power/disk.c	2005-10-17 23:28:33.000000000 +0200
> +++ linux-2.6.14-rc4-mm1/kernel/power/disk.c	2005-10-17 23:28:52.000000000 +0200
> @@ -92,10 +92,7 @@
>  	printk("Freeing memory...  ");
>  	while ((tmp = shrink_all_memory(10000))) {
>  		pages += tmp;
> -		printk("\b%c", p[i]);
> -		i++;
> -		if (i > 3)
> -			i = 0;
> +		printk("\b%c", p[i++ % 4]);
>  	}
>  	printk("\bdone (%li pages freed)\n", pages);
>  }

This is actually not equivalent, but ok. (Equivalent would be ++i % 4
;-)

							Pavel

-- 
Boycott Kodak -- for their patent abuse against Java.

[PATCH 4/4] swsusp: reduce the use of global variables

[Rafael J. Wysocki]

The following patch reduces the use of global variables in the
snapshot-handling part of swsusp, snapshot.c.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Index: linux-2.6.14-rc4-mm1/kernel/power/snapshot.c
===================================================================
--- linux-2.6.14-rc4-mm1.orig/kernel/power/snapshot.c	2005-10-17 23:28:40.000000000 +0200
+++ linux-2.6.14-rc4-mm1/kernel/power/snapshot.c	2005-10-18 00:13:02.000000000 +0200
@@ -164,37 +164,38 @@
 	return 1;
 }
 
-static void count_data_pages(void)
+static unsigned count_data_pages(void)
 {
 	struct zone *zone;
 	unsigned long zone_pfn;
+	unsigned n;
 
-	nr_copy_pages = 0;
-
+	n = 0;
 	for_each_zone (zone) {
 		if (is_highmem(zone))
 			continue;
 		mark_free_pages(zone);
 		for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
-			nr_copy_pages += saveable(zone, &zone_pfn);
+			n += saveable(zone, &zone_pfn);
 	}
+	return n;
 }
 
-static void copy_data_pages(void)
+static void copy_data_pages(struct pbe *pblist)
 {
 	struct zone *zone;
 	unsigned long zone_pfn;
-	struct pbe *pbe = pagedir_nosave, *p;
+	struct pbe *pbe, *p;
 
-	pr_debug("copy_data_pages(): pages to copy: %d\n", nr_copy_pages);
+	pbe = pblist;
 	for_each_zone (zone) {
 		if (is_highmem(zone))
 			continue;
 		mark_free_pages(zone);
 		/* This is necessary for swsusp_free() */
-		for_each_pb_page (p, pagedir_nosave)
+		for_each_pb_page (p, pblist)
 			SetPageNosaveFree(virt_to_page(p));
-		for_each_pbe (p, pagedir_nosave)
+		for_each_pbe (p, pblist)
 			SetPageNosaveFree(virt_to_page(p->address));
 		for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
 			if (saveable(zone, &zone_pfn)) {
@@ -347,46 +348,39 @@
  *	free pages.
  */
 
-static int enough_free_mem(void)
+static int enough_free_mem(unsigned nr_pages)
 {
 	pr_debug("swsusp: available memory: %u pages\n", nr_free_pages());
-	return nr_free_pages() > (nr_copy_pages + PAGES_FOR_IO +
-		nr_copy_pages/PBES_PER_PAGE + !!(nr_copy_pages%PBES_PER_PAGE));
+	return nr_free_pages() > (nr_pages + PAGES_FOR_IO +
+		(nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE);
 }
 
 
-static int swsusp_alloc(void)
+static struct pbe *swsusp_alloc(unsigned nr_pages)
 {
-	struct pbe *p;
+	struct pbe *pblist, *p;
 
-	pagedir_nosave = NULL;
-
-	if (MAX_PBES < nr_copy_pages / PBES_PER_PAGE +
-	    !!(nr_copy_pages % PBES_PER_PAGE))
-		return -ENOSPC;
-
-	if (!(pagedir_save = alloc_pagedir(nr_copy_pages))) {
+	if (!(pblist = alloc_pagedir(nr_pages))) {
 		printk(KERN_ERR "suspend: Allocating pagedir failed.\n");
-		return -ENOMEM;
+		return NULL;
 	}
-	create_pbe_list(pagedir_save, nr_copy_pages);
-	pagedir_nosave = pagedir_save;
+	create_pbe_list(pblist, nr_pages);
 
-	for_each_pbe (p, pagedir_save) {
+	for_each_pbe (p, pblist) {
 		p->address = (unsigned long)alloc_image_page();
 		if (!p->address) {
 			printk(KERN_ERR "suspend: Allocating image pages failed.\n");
 			swsusp_free();
-			return -ENOMEM;
+			return NULL;
 		}
 	}
 
-	return 0;
+	return pblist;
 }
 
 int swsusp_save(void)
 {
-	int error;
+	unsigned nr_pages;
 
 	pr_debug("swsusp: critical section: \n");
 	if (save_highmem()) {
@@ -396,33 +390,37 @@
 	}
 
 	drain_local_pages();
-	count_data_pages();
-	printk("swsusp: Need to copy %u pages\n", nr_copy_pages);
+	nr_pages = count_data_pages();
+	printk("swsusp: Need to copy %u pages\n", nr_pages);
 
 	pr_debug("swsusp: pages needed: %u + %lu + %u, free: %u\n",
-		 nr_copy_pages,
-		 nr_copy_pages/PBES_PER_PAGE + !!(nr_copy_pages%PBES_PER_PAGE),
+		 nr_pages,
+		 (nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE,
 		 PAGES_FOR_IO, nr_free_pages());
 
-	if (!enough_free_mem()) {
+	/* This is needed because of the fixed size of swsusp_info */
+	if (MAX_PBES < (nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE)
+		return -ENOSPC;
+
+	if (!enough_free_mem(nr_pages)) {
 		printk(KERN_ERR "swsusp: Not enough free memory\n");
 		return -ENOMEM;
 	}
 
-	if (!enough_swap()) {
+	if (!enough_swap(nr_pages)) {
 		printk(KERN_ERR "swsusp: Not enough free swap\n");
 		return -ENOSPC;
 	}
 
-	error = swsusp_alloc();
-	if (error)
-		return error;
+	pagedir_nosave = swsusp_alloc(nr_pages);
+	if (!pagedir_nosave)
+		return -ENOMEM;
 
 	/* During allocating of suspend pagedir, new cold pages may appear.
 	 * Kill them.
 	 */
 	drain_local_pages();
-	copy_data_pages();
+	copy_data_pages(pagedir_nosave);
 
 	/*
 	 * End of critical section. From now on, we can write to memory,
@@ -430,6 +428,8 @@
 	 * touch swap space! Except we must write out our image of course.
 	 */
 
-	printk("swsusp: critical section/: done (%d pages copied)\n", nr_copy_pages );
+	nr_copy_pages = nr_pages;
+
+	printk("swsusp: critical section/: done (%d pages copied)\n", nr_pages);
 	return 0;
 }
Index: linux-2.6.14-rc4-mm1/kernel/power/swsusp.c
===================================================================
--- linux-2.6.14-rc4-mm1.orig/kernel/power/swsusp.c	2005-10-17 23:28:52.000000000 +0200
+++ linux-2.6.14-rc4-mm1/kernel/power/swsusp.c	2005-10-18 00:13:02.000000000 +0200
@@ -543,14 +543,14 @@
  *	We should only consider resume_device.
  */
 
-int enough_swap(void)
+int enough_swap(unsigned nr_pages)
 {
 	struct sysinfo i;
 
 	si_swapinfo(&i);
 	pr_debug("swsusp: available swap: %lu pages\n", i.freeswap);
-	return i.freeswap > (nr_copy_pages + PAGES_FOR_IO +
-		nr_copy_pages/PBES_PER_PAGE + !!(nr_copy_pages%PBES_PER_PAGE));
+	return i.freeswap > (nr_pages + PAGES_FOR_IO +
+		(nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE);
 }
 
 
Index: linux-2.6.14-rc4-mm1/kernel/power/power.h
===================================================================
--- linux-2.6.14-rc4-mm1.orig/kernel/power/power.h	2005-10-17 23:28:29.000000000 +0200
+++ linux-2.6.14-rc4-mm1/kernel/power/power.h	2005-10-18 00:13:02.000000000 +0200
@@ -69,4 +69,4 @@
 extern struct pbe * alloc_pagedir(unsigned nr_pages);
 extern void create_pbe_list(struct pbe *pblist, unsigned nr_pages);
 extern void swsusp_free(void);
-extern int enough_swap(void);
+extern int enough_swap(unsigned nr_pages);

Re: [PATCH 4/4] swsusp: reduce the use of global variables

[Pavel Machek]

> The following patch reduces the use of global variables in the
> snapshot-handling part of swsusp, snapshot.c.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

ACK.
							Pavel

-- 
Boycott Kodak -- for their patent abuse against Java.