Re: swsusp update: supports discontingmem/highmem

From: Pavel Machek
Date: Tue Apr 06 2004 - 07:18:38 EST

Hi!

> > It makes swsusp behave correctly
> > w.r.t. discontingmem, and adds highmem handling
>
> Some of those ENOMEM panics in save_highmem_zone() look like they might
> need proper handling instead?

Done.

> The 256 byte automatic array in read_swapfiles() may bring you a visit from
> the stack space police, although I don't think it's really a problem. 256
> bytes for a pathname may be a bit excessive though.

Marked static, its only called once anyway.

> Please send me an update patch sometime which makes all the new code go
> away again if !CONFIG_HIGHMEM.

Done; plus I added some warnings to swsusp.S (based on discussion with
wli). Relative to previous diff, please apply.
Pavel

--- tmp/linux/arch/i386/power/swsusp.S 2003-09-28 22:05:30.000000000 +0200
+++ linux/arch/i386/power/swsusp.S 2004-04-06 13:25:42.000000000 +0200
@@ -1,6 +1,13 @@
.text

-/* Originally gcc generated, modified by hand */
+/* Originally gcc generated, modified by hand
+ *
+ * This may not use any stack, nor any variable that is not "NoSave":
+ *
+ * Its rewriting one kernel image with another. What is stack in "old"
+ * image could very well be data page in "new" image, and overwriting
+ * your own stack under you is bad idea.
+ */

#include <linux/linkage.h>
#include <asm/segment.h>
--- tmp/linux/kernel/power/swsusp.c 2004-04-06 13:26:39.000000000 +0200
+++ linux/kernel/power/swsusp.c 2004-04-06 13:07:46.000000000 +0200
@@ -225,7 +225,7 @@
static void read_swapfiles(void) /* This is called before saving image */
{
int i, len;
- char buff[sizeof(resume_file)], *sname;
+ static char buff[sizeof(resume_file)], *sname;

len=strlen(resume_file);
root_swap = 0xFFFF;
@@ -366,6 +366,7 @@
return 0;
}

+#ifdef CONFIG_HIGHMEM
struct highmem_page {
char *data;
struct page *page;
@@ -374,7 +375,7 @@

struct highmem_page *highmem_copy = NULL;

-static void save_highmem_zone(struct zone *zone)
+static int save_highmem_zone(struct zone *zone)
{
unsigned long zone_pfn;
for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
@@ -384,7 +385,7 @@
unsigned long pfn = zone_pfn + zone->zone_start_pfn;
int chunk_size;

- if (!(pfn%200))
+ if (!(pfn%1000))
printk(".");
if (!pfn_valid(pfn))
continue;
@@ -397,7 +398,7 @@
*/
if (PageReserved(page)) {
printk("highmem reserved page?!\n");
- BUG();
+ continue;
}
if ((chunk_size = is_head_of_free_region(page))) {
pfn += chunk_size - 1;
@@ -406,26 +407,33 @@
}
save = kmalloc(sizeof(struct highmem_page), GFP_ATOMIC);
if (!save)
- panic("Not enough memory");
+ return -ENOMEM;
save->next = highmem_copy;
save->page = page;
save->data = (void *) get_zeroed_page(GFP_ATOMIC);
- if (!save->data)
- panic("Not enough memory");
+ 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 void save_highmem(void)
+static int save_highmem(void)
{
struct zone *zone;
+ int res = 0;
for_each_zone(zone) {
if (is_highmem(zone))
- save_highmem_zone(zone);
+ res = save_highmem_zone(zone);
+ if (res)
+ return res;
}
+ return 0;
}

static int restore_highmem(void)
@@ -443,6 +451,7 @@
}
return 0;
}
+#endif

static int pfn_is_nosave(unsigned long pfn)
{
@@ -460,7 +469,7 @@
struct page *page;
unsigned long pfn = zone_pfn + zone->zone_start_pfn;

- if (!(pfn%200))
+ if (!(pfn%1000))
printk(".");
if (!pfn_valid(pfn))
continue;
@@ -548,7 +557,7 @@

while(nr_copy_pages--) {
p->address = get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
- if(!p->address) {
+ if (!p->address) {
free_suspend_pagedir((unsigned long) pagedir);
return NULL;
}
@@ -589,10 +598,17 @@
unsigned int nr_needed_pages = 0;

pagedir_nosave = NULL;
- printk( "/critical section: Handling highmem" );
- save_highmem();
+ printk( "/critical section: ");
+#ifdef CONFIG_HIGHMEM
+ printk( "handling highmem" );
+ if (save_highmem()) {
+ printk(KERN_CRIT "%sNot enough free pages for highmem\n", name_suspend);
+ return -ENOMEM;
+ }
+ printk(", ");
+#endif

- printk(", counting pages to copy" );
+ printk("counting pages to copy" );
drain_local_pages();
nr_copy_pages = count_and_copy_data_pages(NULL);
nr_needed_pages = nr_copy_pages + PAGES_FOR_IO;
@@ -602,23 +618,22 @@
printk(KERN_CRIT "%sCouldn't get enough free pages, on %d pages short\n",
name_suspend, nr_needed_pages-nr_free_pages());
root_swap = 0xFFFF;
- return 1;
+ return -ENOMEM;
}
si_swapinfo(&i); /* FIXME: si_swapinfo(&i) returns all swap devices information.
We should only consider resume_device. */
if (i.freeswap < nr_needed_pages) {
printk(KERN_CRIT "%sThere's not enough swap space available, on %ld pages short\n",
name_suspend, nr_needed_pages-i.freeswap);
- return 1;
+ return -ENOSPC;
}

PRINTK( "Alloc pagedir\n" );
pagedir_save = pagedir_nosave = create_suspend_pagedir(nr_copy_pages);
- if(!pagedir_nosave) {
- /* Shouldn't happen */
- printk(KERN_CRIT "%sCouldn't allocate enough pages\n",name_suspend);
- panic("Really should not happen");
- return 1;
+ if (!pagedir_nosave) {
+ /* Pagedir is big, one-chunk allocation. It is easily possible for this allocation to fail */
+ printk(KERN_CRIT "%sCouldn't allocate continuous pagedir\n", name_suspend);
+ return -ENOMEM;
}
nr_copy_pages_check = nr_copy_pages;
pagedir_order_check = pagedir_order;
@@ -702,8 +717,10 @@
PRINTK( "Freeing prev allocated pagedir\n" );
free_suspend_pagedir((unsigned long) pagedir_save);

+#ifdef CONFIG_HIGHMEM
printk( "Restoring highmem\n" );
restore_highmem();
+#endif
printk("done, devices\n");

device_power_up();

--
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/