swsusp: do not use higher order memory allocations on suspend

kernel/power/swsusp.c +0 -1

@@ -373,15 +377,22 @@
 
 static int write_pagedir(void)
 {
-	unsigned long addr = (unsigned long)pagedir_nosave;

Quoted 13 lines

kernel/power/swsusp.c +4 -3

@@ -373,15 +377,22 @@
 
 static int write_pagedir(void)
 {
-	unsigned long addr = (unsigned long)pagedir_nosave;
 	int error = 0;
-	int n = SUSPEND_PD_PAGES(nr_copy_pages);
-	int i;
+	unsigned n = 0;
+	struct pbe * pbe;
+
+	printk( "Writing pagedir ...");
 

Review line 387

Quoted 2 lines

(quoted diff) +2 -0

+
+	pr_debug("\b\b\bdone (%u pages)\n", n);

Just cosmetic:
Why do you use pr_debug here instead of printk like you did above?

The following patch is (yet) an(other) attempt to eliminate the need for using higher order memory allocations on suspend. It accomplishes this by replacing the array of page backup entries with a list, so it is only necessary to allocate individual memory pages.

I have noticed that the suspend code in swsusp is actually independent of the resume code and vice versa, so it is not necessary to change them both at once.

Therefore I have decided, for now, to only modify the suspend part which is more straightforward. The resume code is almost unaffected by this patch (the only thing changed in this code is a BUG_ON() that is removed).

The patch is against 2.6.11-rc2. It has been tested and quite thoroughly debugged on x86-64. Still, if anyone could test it on another architecture, I'd be grateful. Of course comments, suggestions etc. are welcome.

Hi!

I'll do some testing later.

include/linux/suspend.h line 20

Quoted 12 lines

diff -Nru linux-2.6.11-rc2-orig/include/linux/suspend.h linux-2.6.11-rc2/include/linux/suspend.h

include/linux/suspend.h +2 -3

@@ -16,11 +16,20 @@
 	unsigned long address;		/* address of the copy */
 	unsigned long orig_address;	/* original address of page */
 	swp_entry_t swap_address;	
-	swp_entry_t dummy;		/* we need scratch space at 
-					 * end of page (see link, diskpage)
-					 */
+
+	struct pbe *next;

It is still used as a scratch space at end of page, please leave the comment.

Quoted 5 lines

include/linux/suspend.h +1 -1

 	printk( "Writing data to swap (%d pages)...     ", nr_copy_pages );
-	for (i = 0; i < nr_copy_pages && !error; i++) {
+	for_each_pbe(p, pagedir_nosave) {
 		if (!(i%mod))
 			printk( "\b\b\b\b%3d%%", i / mod );

include/linux/suspend.h

Quoted 5 lines

include/linux/suspend.h +3 -2

-		error = write_page((pagedir_nosave+i)->address,
-					  &((pagedir_nosave+i)->swap_address));
+		error = write_page(p->address, &(p->swap_address));
+		if (error)
+			goto Exit;

Can you do return error instead? Goto is ugly. Or just break. Same in next chunks.

include/linux/suspend.h

Quoted 8 lines

include/linux/suspend.h +3 -4

-
-static void calc_order(void)
+static int calc_nr(int nr_copy)
 {
-	int diff = 0;
-	int order = 0;
+	int extra = 0;
+	int mod = (nr_copy % PBES_PER_PAGE) ? 1 : 0;

!!( ) is usually used for this.

Quoted 12 lines

static inline void free_pagedir(struct pbe *pblist)

{
        struct pbe *pbe;

        while (pblist) {
                pbe = pblist + PB_PAGE_SKIP;
                pblist = pbe->next;
                free_page((unsigned long)pbe);
        }
        pr_debug("free_pagedir(): done\n");
}

Quoted 2 lines

Should not you free_page(pblist) instead? This passes address in middle of page to free_page, that seems wrong.

Certainly. It should be something like that:

while (pblist) {
	pbe = pblist;
	pblist = (pbe + PB_PAGE_SKIP)->next;
	free_page((unsigned long)pbe);
}

Hi!

Quoted 15 lines

Quoted 12 lines

static inline void free_pagedir(struct pbe *pblist)

{
        struct pbe *pbe;

        while (pblist) {
                pbe = pblist + PB_PAGE_SKIP;
                pblist = pbe->next;
                free_page((unsigned long)pbe);
        }
        pr_debug("free_pagedir(): done\n");
}

Quoted 2 lines

Should not you free_page(pblist) instead? This passes address in middle of page to free_page, that seems wrong.

Quoted 7 lines

Certainly. It should be something like that:

while (pblist) {
	pbe = pblist;
	pblist = (pbe + PB_PAGE_SKIP)->next;
	free_page((unsigned long)pbe);
}

Hmm, I see, my "fix" leaks one page of memory during each suspend... I've fixed it properly now and will eventually propagete it back.

This should be right..

        while (pblist) {
                pbe = (pblist + PB_PAGE_SKIP)->next;
                free_page((unsigned long)pblist);
                pblist = pbe;
        }