Skip to contentSkip to conversation

swsusp: rework swsusp_suspend

PATCH In review Rafael J. Wysocki 3 participants 4 messages Oct 31, 2005 – Nov 1, 2005

Subsystems (2)
Suspend to RAM Hibernation
lore
Share & tools
lore

Rafael J. Wysocki

author
#0 lore raw

PATCH 1/1 swsusp: rework swsusp_suspend

Hi,

The following patch makes only the functions in swsusp.c call functions in snapshot.c and not both ways. It also moves the check for available swap out of swsusp_suspend() which is necessary for separating the swap-handling functions in swsusp from the core code.

Basically, it moves the code without changing its functionality.

Please consider for applying.

Greetings,
Rafael

Signed-off-by Rafael J. Wysocki <[email protected]>
 kernel/power/power.h    |    2 
 kernel/power/snapshot.c |  102 ------------------------------
 kernel/power/swsusp.c   |  158 ++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 128 insertions(+), 134 deletions(-)
kernel/power/snapshot.c +0 -102
Index: linux-2.6.14-git3/kernel/power/snapshot.c
===================================================================
@@ -33,98 +33,6 @@
 
 #include "power.h"
 
-#ifdef CONFIG_HIGHMEM
-struct highmem_page {
-	char *data;
-	struct page *page;
-	struct highmem_page *next;
-};
-
-static struct highmem_page *highmem_copy;
-
-static int save_highmem_zone(struct zone *zone)
-{
-	unsigned long zone_pfn;
-	mark_free_pages(zone);
-	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
-		struct page *page;
-		struct highmem_page *save;
-		void *kaddr;
-		unsigned long pfn = zone_pfn + zone->zone_start_pfn;
-
-		if (!(pfn%1000))
-			printk(".");
-		if (!pfn_valid(pfn))
-			continue;
-		page = pfn_to_page(pfn);
-		/*
-		 * This condition results from rvmalloc() sans vmalloc_32()
-		 * and architectural memory reservations. This should be
-		 * corrected eventually when the cases giving rise to this
-		 * are better understood.
-		 */
-		if (PageReserved(page)) {
-			printk("highmem reserved page?!\n");
-			continue;
-		}
-		BUG_ON(PageNosave(page));
-		if (PageNosaveFree(page))
-			continue;
-		save = kmalloc(sizeof(struct highmem_page), GFP_ATOMIC);
-		if (!save)
-			return -ENOMEM;
-		save->next = highmem_copy;
-		save->page = page;
-		save->data = (void *) get_zeroed_page(GFP_ATOMIC);
-		if (!save->data) {
-			kfree(save);
-			return -ENOMEM;
-		}
-		kaddr = kmap_atomic(page, KM_USER0);
-		memcpy(save->data, kaddr, PAGE_SIZE);
-		kunmap_atomic(kaddr, KM_USER0);
-		highmem_copy = save;
-	}
-	return 0;
-}
-
-
-static int save_highmem(void)
-{
-	struct zone *zone;
-	int res = 0;
-
-	pr_debug("swsusp: Saving Highmem\n");
-	for_each_zone (zone) {
-		if (is_highmem(zone))
-			res = save_highmem_zone(zone);
-		if (res)
-			return res;
-	}
-	return 0;
-}
-
-int restore_highmem(void)
-{
-	printk("swsusp: Restoring Highmem\n");
-	while (highmem_copy) {
-		struct highmem_page *save = highmem_copy;
-		void *kaddr;
-		highmem_copy = save->next;
-
-		kaddr = kmap_atomic(save->page, KM_USER0);
-		memcpy(kaddr, save->data, PAGE_SIZE);
-		kunmap_atomic(kaddr, KM_USER0);
-		free_page((long) save->data);
-		kfree(save);
-	}
-	return 0;
-}
-#else
-static int save_highmem(void) { return 0; }
-int restore_highmem(void) { return 0; }
-#endif /* CONFIG_HIGHMEM */
-
 
 static int pfn_is_nosave(unsigned long pfn)
 {

@@ -383,11 +291,6 @@
 	unsigned nr_pages;
 
 	pr_debug("swsusp: critical section: \n");
-	if (save_highmem()) {
-		printk(KERN_CRIT "swsusp: Not enough free pages for highmem\n");
-		restore_highmem();
-		return -ENOMEM;
-	}
 
 	drain_local_pages();
 	nr_pages = count_data_pages();

@@ -407,11 +310,6 @@
 		return -ENOMEM;
 	}
 
-	if (!enough_swap(nr_pages)) {
-		printk(KERN_ERR "swsusp: Not enough free swap\n");
-		return -ENOSPC;
-	}
-
 	pagedir_nosave = swsusp_alloc(nr_pages);
 	if (!pagedir_nosave)
 		return -ENOMEM;
kernel/power/swsusp.c +128 -30

221 lines not on this page · raw message

kernel/power/power.h +0 -2
Index: linux-2.6.14-git3/kernel/power/power.h
===================================================================
@@ -65,8 +65,6 @@
 extern asmlinkage int swsusp_arch_suspend(void);
 extern asmlinkage int swsusp_arch_resume(void);
 
-extern int restore_highmem(void);
 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(unsigned nr_pages);
#1 lore raw

Hi!

Quoted 4 lines

The following patch makes only the functions in swsusp.c call functions in snapshot.c and not both ways. It also moves the check for available swap out of swsusp_suspend() which is necessary for separating the swap-handling functions in swsusp from the core code.

Moving highmem handling code is not neccessary for this goal, right? Please keep it in place.

(You being x86-64 person, I can understand you don't want it in snapshot.c ;-)

Pavel

Signature · 1 line
Thanks, Sharp!
author
#2 lore raw

Hi,

Quoted 8 lines · Pavel Machek

Hi!

Quoted 4 lines

The following patch makes only the functions in swsusp.c call functions in snapshot.c and not both ways. It also moves the check for available swap out of swsusp_suspend() which is necessary for separating the swap-handling functions in swsusp from the core code.

Moving highmem handling code is not neccessary for this goal, right?

No, it is a side-effect.

Quoted 1 line

Please keep it in place.

OK (although I don't see why you are insisting on this).

Quoted 2 lines

(You being x86-64 person, I can understand you don't want it in snapshot.c ;-)

This isn't related to my hardware preferences. ;-)

The highmem-handling functions have nothing to do with the snapshot functionality, because the highmem is _really_ saved _before_ creating the snapshot and restored _after_ it has beed created.

Clearly restore_highmem() is only called from swsusp_suspend() and swsusp_resume() which are in swsusp.c. If CONFIG_HIGHMEM
is unset, it is defined as a non-static empty function which bloats the kernel as Ingo Oeser has already noticed. Therefore if CONFIG_HIGHMEM is unset, restore_highmem() should be declared as a static empty function in swsusp.c (this declaration should not be placed in power.h as it causes some compiler warnings to appear for files that do not use restore_highmem()). By analogy it should be defined as a static function is CONFIG_HIGHMEM is set either.

OTOH save_highmem() is actually symmetrical to restore_highmem(), so it should be called from the same function from which restore_highmem() is called, and this function is swsusp_suspend() (the call to restore_highmem() from swsusp_resume() is unnecessary, but you refused to let it go).

Anyway the appended patch contains only changes that you have already agreed with, except for the "#ifdef CONFIG_HIGHMEM" thing in swsusp.c which originally was in snapshot.c (and bloated the kernel).

Greetings,
Rafael

Signed-off-by Rafael J. Wysocki <[email protected]>
 kernel/power/power.h    |    2 
 kernel/power/snapshot.c |  102 ------------------------------
 kernel/power/swsusp.c   |  158 ++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 128 insertions(+), 134 deletions(-)
kernel/power/snapshot.c +2 -17
Index: linux-2.6.14-git4/kernel/power/snapshot.c
===================================================================
@@ -88,8 +88,7 @@
 	return 0;
 }
 
-
-static int save_highmem(void)
+int save_highmem(void)
 {
 	struct zone *zone;
 	int res = 0;

@@ -120,11 +119,7 @@
 	}
 	return 0;
 }
-#else
-static int save_highmem(void) { return 0; }
-int restore_highmem(void) { return 0; }
-#endif /* CONFIG_HIGHMEM */
-
+#endif
 
 static int pfn_is_nosave(unsigned long pfn)
 {

@@ -383,11 +378,6 @@
 	unsigned nr_pages;
 
 	pr_debug("swsusp: critical section: \n");
-	if (save_highmem()) {
-		printk(KERN_CRIT "swsusp: Not enough free pages for highmem\n");
-		restore_highmem();
-		return -ENOMEM;
-	}
 
 	drain_local_pages();
 	nr_pages = count_data_pages();

@@ -407,11 +397,6 @@
 		return -ENOMEM;
 	}
 
-	if (!enough_swap(nr_pages)) {
-		printk(KERN_ERR "swsusp: Not enough free swap\n");
-		return -ENOSPC;
-	}
-
 	pagedir_nosave = swsusp_alloc(nr_pages);
 	if (!pagedir_nosave)
 		return -ENOMEM;
kernel/power/swsusp.c +45 -30

138 lines not on this page · raw message

kernel/power/power.h +0 -2
Index: linux-2.6.14-git4/kernel/power/power.h
===================================================================
@@ -65,8 +65,6 @@
 extern asmlinkage int swsusp_arch_suspend(void);
 extern asmlinkage int swsusp_arch_resume(void);
 
-extern int restore_highmem(void);
 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(unsigned nr_pages);
#3 lore raw

Hi!

Quoted 3 lines

Anyway the appended patch contains only changes that you have already agreed with, except for the "#ifdef CONFIG_HIGHMEM" thing in swsusp.c which originally was in snapshot.c (and bloated the kernel).

ACK.
Pavel

Signature · 1 line
Thanks, Sharp!