Skip to contentSkip to conversation

Free swap suspend from depending upon PageReserved.

PATCH In review Nigel Cunningham 3 participants 5 messages Oct 5, 2005

Subsystems (3)
Memory management Suspend to RAM Hibernation
lore
Share & tools
lore

Nigel Cunningham

author
#0 lore raw

PATCH 1/1 Free swap suspend from depending upon PageReserved.

Hi.

Here's the patch we've previously discussed, which removes the dependancy of swap suspend on PageReserved.

Regards,

Nigel

 arch/i386/mm/init.c   |   36 ++++++++++++++++++++++++++++--------
 kernel/power/swsusp.c |    7 +++----
 mm/bootmem.c          |    4 ++++
 3 files changed, 35 insertions(+), 12 deletions(-)
arch/i386/mm/init.c +28 -8
diff -ruNp 3120-e820-table-support.patch-old/arch/i386/mm/init.c 3120-e820-table-support.patch-new/arch/i386/mm/init.c
@@ -27,6 +27,7 @@
 #include <linux/slab.h>
 #include <linux/proc_fs.h>
 #include <linux/efi.h>
+#include <linux/suspend.h>
 
 #include <asm/processor.h>
 #include <asm/system.h>

@@ -270,11 +271,14 @@ void __init one_highpage_init(struct pag
 {
 	if (page_is_ram(pfn) && !(bad_ppro && page_kills_ppro(pfn))) {
 		ClearPageReserved(page);
+		ClearPageNosave(page);
 		set_page_count(page, 1);
 		__free_page(page);
 		totalhigh_pages++;
-	} else
+	} else {
 		SetPageReserved(page);
+		SetPageNosave(page);
+	}
 }
 
 #ifdef CONFIG_NUMA

@@ -353,7 +357,7 @@ static void __init pagetable_init (void)
 #endif
 }
 
-#ifdef CONFIG_SOFTWARE_SUSPEND
+#ifdef CONFIG_PM
 /*
  * Swap suspend & friends need this for resume because things like the intel-agp
  * driver might have split up a kernel 4MB mapping.

@@ -540,6 +544,7 @@ void __init mem_init(void)
 	int codesize, reservedpages, datasize, initsize;
 	int tmp;
 	int bad_ppro;
+	void * addr;
 
 #ifdef CONFIG_FLATMEM
 	if (!mem_map)

@@ -570,12 +575,25 @@ void __init mem_init(void)
 	totalram_pages += free_all_bootmem();
 
 	reservedpages = 0;
-	for (tmp = 0; tmp < max_low_pfn; tmp++)
-		/*
-		 * Only count reserved RAM pages
-		 */
-		if (page_is_ram(tmp) && PageReserved(pfn_to_page(tmp)))
-			reservedpages++;
+	addr = __va(0);
+	for (tmp = 0; tmp < max_low_pfn; tmp++, addr += PAGE_SIZE) {
+		if (page_is_ram(tmp)) {
+			/*
+			 * Only count reserved RAM pages
+			 */
+			if (PageReserved(mem_map+tmp))
+				reservedpages++;
+			/*
+			 * Mark nosave pages
+			 */
+			if (addr >= (void *)&__nosave_begin && addr < (void *)&__nosave_end)
+				SetPageNosave(mem_map+tmp);
+		} else
+			/*
+			 * Non-RAM pages are always nosave
+			 */
+			SetPageNosave(mem_map+tmp);
+	}
 
 	set_highmem_pages_init(bad_ppro);
 

@@ -674,6 +692,7 @@ void free_initmem(void)
 	addr = (unsigned long)(&__init_begin);
 	for (; addr < (unsigned long)(&__init_end); addr += PAGE_SIZE) {
 		ClearPageReserved(virt_to_page(addr));
+		ClearPageNosave(virt_to_page(addr));
 		set_page_count(virt_to_page(addr), 1);
 		memset((void *)addr, 0xcc, PAGE_SIZE);
 		free_page(addr);

@@ -689,6 +708,7 @@ void free_initrd_mem(unsigned long start
 		printk (KERN_INFO "Freeing initrd memory: %ldk freed\n", (end - start) >> 10);
 	for (; start < end; start += PAGE_SIZE) {
 		ClearPageReserved(virt_to_page(start));
+		ClearPageNosave(virt_to_page(start));
 		set_page_count(virt_to_page(start), 1);
 		free_page(start);
 		totalram_pages++;
kernel/power/swsusp.c +3 -4
diff -ruNp 3120-e820-table-support.patch-old/kernel/power/swsusp.c 3120-e820-table-support.patch-new/kernel/power/swsusp.c
@@ -672,13 +672,12 @@ static int saveable(struct zone * zone, 
 		return 0;
 
 	page = pfn_to_page(pfn);
-	BUG_ON(PageReserved(page) && PageNosave(page));
-	if (PageNosave(page))
-		return 0;
-	if (PageReserved(page) && pfn_is_nosave(pfn)) {
+	if (pfn_is_nosave(pfn)) {
 		pr_debug("[nosave pfn 0x%lx]", pfn);
 		return 0;
 	}
+	if (PageNosave(page))
+		return 0;
 	if (PageNosaveFree(page))
 		return 0;
mm/bootmem.c +4 -0
diff -ruNp 3120-e820-table-support.patch-old/mm/bootmem.c 3120-e820-table-support.patch-new/mm/bootmem.c
@@ -291,12 +291,14 @@ static unsigned long __init free_all_boo
 			page = pfn_to_page(pfn);
 			count += BITS_PER_LONG;
 			__ClearPageReserved(page);
+			ClearPageNosave(page);
 			order = ffs(BITS_PER_LONG) - 1;
 			set_page_refs(page, order);
 			for (j = 1; j < BITS_PER_LONG; j++) {
 				if (j + 16 < BITS_PER_LONG)
 					prefetchw(page + j + 16);
 				__ClearPageReserved(page + j);
+				ClearPageNosave(page + j);
 			}
 			__free_pages(page, order);
 			i += BITS_PER_LONG;

@@ -309,6 +311,7 @@ static unsigned long __init free_all_boo
 				if (v & m) {
 					count++;
 					__ClearPageReserved(page);
+					ClearPageNosave(page);
 					set_page_refs(page, 0);
 					__free_page(page);
 				}

@@ -329,6 +332,7 @@ static unsigned long __init free_all_boo
 	for (i = 0; i < ((bdata->node_low_pfn-(bdata->node_boot_start >> PAGE_SHIFT))/8 + PAGE_SIZE-1)/PAGE_SIZE; i++,page++) {
 		count++;
 		__ClearPageReserved(page);
+		ClearPageNosave(page);
 		set_page_count(page, 1);
 		__free_page(page);
 	}
#1 lore raw

Hi!

Quoted 2 lines

Here's the patch we've previously discussed, which removes the dependancy of swap suspend on PageReserved.

This ends up in Linus' changelog, so "we've previously discussed" is not okay here. Missing signed-off. What is benefit of this?

swsusp part looks okay, but will Andrew like the generic part? I guess I'd prefer to postpone this one (unless we are last user of PageReserved) -- I do not see too big benefit and there's potential for breakage.

arch/i386/mm/init.c static void __init pagetable_init (void) · line 363

Quoted 9 lines
arch/i386/mm/init.c +1 -1
@@ -353,7 +357,7 @@ static void __init pagetable_init (void)
 #endif
 }
 
-#ifdef CONFIG_SOFTWARE_SUSPEND
+#ifdef CONFIG_PM
 /*
  * Swap suspend & friends need this for resume because things like the intel-agp
  * driver might have split up a kernel 4MB mapping.

This is wrong, right? It

arch/i386/mm/init.c void __init mem_init(void) · line 547

Quoted 5 lines
arch/i386/mm/init.c +1 -0
@@ -540,6 +544,7 @@ void __init mem_init(void)
 	int codesize, reservedpages, datasize, initsize;
 	int tmp;
 	int bad_ppro;
+	void * addr;

Please make it void *addr;.
Pavel

Signature · 1 line
if you have sharp zaurus hardware you don't need... you know my address
#2 lore raw

Hi,

Quoted 5 lines · Pavel Machek

Hi!

Quoted 2 lines

Here's the patch we've previously discussed, which removes the dependancy of swap suspend on PageReserved.

Quoted 7 lines

This ends up in Linus' changelog, so "we've previously discussed" is not okay here. Missing signed-off. What is benefit of this?

swsusp part looks okay, but will Andrew like the generic part? I guess I'd prefer to postpone this one (unless we are last user of PageReserved) -- I do not see too big benefit and there's potential for breakage.

Basically, what it does is to make swsusp avoid saving (and restoring) non-RAM pages (like the ISA hole, BIOS etc.). I think it is a nice thing to do and it does not hurt anyone (it only clears and/or sets PG_nosave at some places). However, if we decide to do this for i386, it should also be done for x86-64.

Greetings,
Rafael

author
#4 lore raw
In reply to Pavel Machek

Hi.

Quoted 7 lines · Pavel Machek

Hi!

Quoted 2 lines

Here's the patch we've previously discussed, which removes the dependancy of swap suspend on PageReserved.

This ends up in Linus' changelog, so "we've previously discussed" is not okay here. Missing signed-off. What is benefit of this?

I wasn't seeking to get it merged immediately, but was seeking comments.

Quoted 4 lines

swsusp part looks okay, but will Andrew like the generic part? I guess I'd prefer to postpone this one (unless we are last user of PageReserved) -- I do not see too big benefit and there's potential for breakage.

There have already been patches to remove work toward removing PageReserved. If swap suspend isn't the last user, it won't be far from it.

Quoted 11 lines

arch/i386/mm/init.c static void __init pagetable_init (void) · line 363

Quoted 9 lines
arch/i386/mm/init.c +1 -1
@@ -353,7 +357,7 @@ static void __init pagetable_init (void)
 #endif
 }
 
-#ifdef CONFIG_SOFTWARE_SUSPEND
+#ifdef CONFIG_PM
 /*
  * Swap suspend & friends need this for resume because things like the intel-agp
  * driver might have split up a kernel 4MB mapping.

This is wrong, right? It

No. I use it too. From your perspective though, I suppose it is wrong.

Quoted 7 lines

arch/i386/mm/init.c void __init mem_init(void) · line 547

Quoted 5 lines
arch/i386/mm/init.c +1 -0
@@ -540,6 +544,7 @@ void __init mem_init(void)
 	int codesize, reservedpages, datasize, initsize;
 	int tmp;
 	int bad_ppro;
+	void * addr;

Please make it void *addr;.

Ok.

If noone else suggests changes, I'll properly submit it.

Regards,

Nigel

Quoted 1 line

Pavel

author
#3 lore raw
In reply to Rafael J. Wysocki

Hi.

Quoted 16 lines · Rafael J. Wysocki

Hi,

Quoted 5 lines · Pavel Machek

Hi!

Quoted 2 lines

Here's the patch we've previously discussed, which removes the dependancy of swap suspend on PageReserved.

Quoted 7 lines

This ends up in Linus' changelog, so "we've previously discussed" is not okay here. Missing signed-off. What is benefit of this?

swsusp part looks okay, but will Andrew like the generic part? I guess I'd prefer to postpone this one (unless we are last user of PageReserved) -- I do not see too big benefit and there's potential for breakage.

Quoted 5 lines

Basically, what it does is to make swsusp avoid saving (and restoring) non-RAM pages (like the ISA hole, BIOS etc.). I think it is a nice thing to do and it does not hurt anyone (it only clears and/or sets PG_nosave at some places). However, if we decide to do this for i386, it should also be done for x86-64.

True. I wasn't thinking about others arches, and should have. I'll modify the patch and seek to repost today.

Regards,

Nigel

Quoted 2 lines

Greetings,
Rafael