swsusp with highmem, testing wanted

swsusp with highmem, testing wanted

[Pavel Machek]

Hi!

If you have machine with >=1GB of RAM, do you think you could test
this patch? [I'd like to hear about successes, too; perhaps send it
privately].

							Pavel

--- clean.2.5/kernel/power/swsusp.c	2004-03-11 18:11:26.000000000 +0100
+++ linux-himem-swsusp/kernel/power/swsusp.c	2004-03-25 00:53:56.000000000 +0100
@@ -61,6 +61,7 @@
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
 #include <linux/console.h>
+#include <linux/highmem.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -362,7 +363,69 @@
 	return 0;
 }
 
+struct highmem_page {
+	char *data;
+	struct page *page;
+	struct highmem_page *next;
+};
+
+struct highmem_page *highmem_copy = NULL;
+
 /* if pagedir_p != NULL it also copies the counted pages */
+static int save_highmem(void)
+{
+	int pfn;
+	struct page *page;
+	int chunk_size;
+
+	for (pfn = 0; pfn < max_pfn; pfn++) {
+		struct highmem_page *save;
+		void *kaddr;
+
+		page = pfn_to_page(pfn);
+
+		if (!PageHighMem(page))
+			continue;
+		if (PageReserved(page)) {
+			printk("highmem reserved page?!\n");
+			BUG();
+		}
+		if ((chunk_size=is_head_of_free_region(page))!=0) {
+			pfn += chunk_size - 1;
+			continue;
+		}
+		save = kmalloc(sizeof(struct highmem_page), GFP_ATOMIC);
+		if (!save)
+			panic("Not enough memory");
+		save->next = highmem_copy;
+		save->page = page;
+		save->data = get_zeroed_page(GFP_ATOMIC);
+		if (!save->data)
+			panic("Not enough memory");
+		kaddr = kmap_atomic(page, KM_USER0);
+		memcpy(save->data, kaddr, PAGE_SIZE);
+		kunmap_atomic(kaddr, KM_USER0);
+		highmem_copy = save;
+	}
+	return 0;
+}
+
+static int restore_highmem(void)
+{
+	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(save->data);
+		kfree(save);
+	}
+	return 0;
+}
+
 static int count_and_copy_data_pages(struct pbe *pagedir_p)
 {
 	int chunk_size;
@@ -378,7 +441,7 @@
 	for (pfn = 0; pfn < max_pfn; pfn++) {
 		page = pfn_to_page(pfn);
 		if (PageHighMem(page))
-			panic("Swsusp not supported on highmem boxes. Send 1GB of RAM to <pavel@ucw.cz> and try again ;-).");
+			continue;
 
 		if (!PageReserved(page)) {
 			if (PageNosave(page))
@@ -413,6 +476,7 @@
 	return nr_copy_pages;
 }
 
+
 static void free_suspend_pagedir(unsigned long this_pagedir)
 {
 	struct page *page;
@@ -492,10 +556,12 @@
 	struct sysinfo i;
 	unsigned int nr_needed_pages = 0;
 
-	drain_local_pages();
-
 	pagedir_nosave = NULL;
-	printk( "/critical section: Counting pages to copy" );
+	printk( "/critical section: Handling highmem" );
+	save_highmem();
+
+	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;
 	
@@ -603,6 +669,11 @@
 
 	PRINTK( "Freeing prev allocated pagedir\n" );
 	free_suspend_pagedir((unsigned long) pagedir_save);
+
+	printk( "Restoring highmem\n" );
+	restore_highmem();
+	printk("done, devices\n");
+
 	device_power_up();
 	spin_unlock_irq(&suspend_pagedir_lock);
 	device_resume();

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: swsusp with highmem, testing wanted

[Benjamin Herrenschmidt]

On Thu, 2004-03-25 at 10:57, Pavel Machek wrote:
> Hi!
> 
> If you have machine with >=1GB of RAM, do you think you could test
> this patch? [I'd like to hear about successes, too; perhaps send it
> privately].

Ugh ? If I understand things properly, you are copying all of highmem
down to lowmem ? Hrm... I'm afraid in lots of case you'll run out
of lowmem in the process. Actually, we should be able to use highmem
even for snapshotting since the disk IO can then be directly be done
from highmem pages...

(Though kmapp'ing/unmapping each page will be slow as hell)

Ben.

Re: swsusp with highmem, testing wanted

[Benjamin Herrenschmidt]

> I'd need to do atomic copy. (Unless someone can guarantee that during
> writing to disk, no highmem page is going to be changed...)
> 
> "copy back" during resume is done in assembly, and I'd rather not
> dealed with highmem there.

Can you make that an option ? The PPC version runs in real mode and
can perfectly copy highmem pages (with small tweaks maybe)

> OTOH, if it possible to guarantee that highmem pages do not change
> during reads/writes to disk, I might be able to get away without this
> copy.

I also think we free too much memory btw (and spend too much time
trying to free memory). Have you looked at some of Nigel stuffs in
swsusp2 ? There may be good ideas to borrow... 

Ben.

Re: swsusp with highmem, testing wanted

[Pavel Machek]

Hi!

> > I'd need to do atomic copy. (Unless someone can guarantee that during
> > writing to disk, no highmem page is going to be changed...)
> > 
> > "copy back" during resume is done in assembly, and I'd rather not
> > dealed with highmem there.
> 
> Can you make that an option ? The PPC version runs in real mode and
> can perfectly copy highmem pages (with small tweaks maybe)

What is real mode on PPC? I do not have PPC here, I guess you'd have
to do that.

> > OTOH, if it possible to guarantee that highmem pages do not change
> > during reads/writes to disk, I might be able to get away without this
> > copy.
> 
> I also think we free too much memory btw (and spend too much time
> trying to free memory). Have you looked at some of Nigel stuffs in
> swsusp2 ? There may be good ideas to borrow... 

Yes, swsusp2 is faster. It is also 10x more code. We could probably
stop freeing as soon as half of memory is free; OTOH if memory is
disk cache, it might be faster to drop it than write to swap, then
read back [swsusp2 shows its not usually the case, through].
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: swsusp with highmem, testing wanted

[Nigel Cunningham]

Hi.

On Fri, 2004-03-26 at 10:59, Pavel Machek wrote:
> > I also think we free too much memory btw (and spend too much time
> > trying to free memory). Have you looked at some of Nigel stuffs in
> > swsusp2 ? There may be good ideas to borrow... 
> 
> Yes, swsusp2 is faster. It is also 10x more code. We could probably
> stop freeing as soon as half of memory is free; OTOH if memory is
> disk cache, it might be faster to drop it than write to swap, then
> read back [swsusp2 shows its not usually the case, through].

10x more code is true, but we also need to ask, how much of that is more
functionality? How much is debugging code (that can be removed)? How
much is comments?

10x implies there's needless bloat and that the two are otherwise
equivalent. That's simply not true.

Regards,

Nigel
-- 
Nigel Cunningham
C/- Westminster Presbyterian Church Belconnen
61 Templeton Street, Cook, ACT 2614.
+61 (2) 6251 7727(wk); +61 (2) 6253 0250 (home)

Evolution (n): A hypothetical process whereby infinitely improbable events occur 
with alarming frequency, order arises from chaos, and no one is given credit.

Re: swsusp with highmem, testing wanted

[Pavel Machek]

Hi!

> On Fri, 2004-03-26 at 10:59, Pavel Machek wrote:
> > > I also think we free too much memory btw (and spend too much time
> > > trying to free memory). Have you looked at some of Nigel stuffs in
> > > swsusp2 ? There may be good ideas to borrow... 
> > 
> > Yes, swsusp2 is faster. It is also 10x more code. We could probably
> > stop freeing as soon as half of memory is free; OTOH if memory is
> > disk cache, it might be faster to drop it than write to swap, then
> > read back [swsusp2 shows its not usually the case, through].
> 
> 10x more code is true, but we also need to ask, how much of that is more
> functionality? How much is debugging code (that can be removed)? How
> much is comments?

Do you think you could strip down features + debugging etc so that
swsusp2 is only, say, 3x bigger than swsusp1? It would certainly make
merging easier.

> 10x implies there's needless bloat and that the two are otherwise
> equivalent. That's simply not true.

If I implied that I should appologize. (Sorry.) swsusp2 *has* more
features, many of them make it faster.
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: swsusp with highmem, testing wanted

[Nigel Cunningham]

Hi.

On Fri, 2004-03-26 at 11:54, Pavel Machek wrote:
> > 10x more code is true, but we also need to ask, how much of that is more
> > functionality? How much is debugging code (that can be removed)? How
> > much is comments?
> 
> Do you think you could strip down features + debugging etc so that
> swsusp2 is only, say, 3x bigger than swsusp1? It would certainly make
> merging easier.

Well, I'll certainly clean up the debugging code. I know that much of it
isn't needed any more. I'll try not to remove comments though :> (I know
it's not simple and straightforward to understand how it works, so I
want to comment it as well as I can. As I said the other day, I don't
intend to disappear into the wild blue yonder, but I don't know what the
future will bring).

> > 10x implies there's needless bloat and that the two are otherwise
> > equivalent. That's simply not true.
> 
> If I implied that I should appologize. (Sorry.) swsusp2 *has* more
> features, many of them make it faster.

No offense taken. I just wanted to make it clear we're not comparing
apples with apples here.

Regards,

Nigel
-- 
Nigel Cunningham
C/- Westminster Presbyterian Church Belconnen
61 Templeton Street, Cook, ACT 2614.
+61 (2) 6251 7727(wk); +61 (2) 6253 0250 (home)

Evolution (n): A hypothetical process whereby infinitely improbable events occur 
with alarming frequency, order arises from chaos, and no one is given credit.

Re: swsusp with highmem, testing wanted

[Benjamin Herrenschmidt]

On Fri, 2004-03-26 at 09:59, Pavel Machek wrote:
> Hi!
> 
> > > I'd need to do atomic copy. (Unless someone can guarantee that during
> > > writing to disk, no highmem page is going to be changed...)
> > > 
> > > "copy back" during resume is done in assembly, and I'd rather not
> > > dealed with highmem there.
> > 
> > Can you make that an option ? The PPC version runs in real mode and
> > can perfectly copy highmem pages (with small tweaks maybe)
> 
> What is real mode on PPC? I do not have PPC here, I guess you'd have
> to do that.

MMU OFF, access to entire physical memory. This will not work on
things like pSeries with hypervisor or iSeries, but I could deal with
that if/when needed. I know that x86 with more than 4Gb cannot access
the entire RAM in a linear way though, dunno what other facilities
you have outside of kmap then. But leave the door open to archs that
can do it ;)
 
> Yes, swsusp2 is faster. It is also 10x more code. We could probably
> stop freeing as soon as half of memory is free; OTOH if memory is
> disk cache, it might be faster to drop it than write to swap, then
> read back [swsusp2 shows its not usually the case, through].
> 								Pavel
-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>

Re: swsusp with highmem, testing wanted

[Jeff Chua]

On Thu, 25 Mar 2004, Pavel Machek wrote:

> If you have machine with >=1GB of RAM, do you think you could test
> this patch? [I'd like to hear about successes, too; perhaps send it
> privately].

It works! 1GB of RAM. Highmem enabled.


Thanks,
Jeff

Re: swsusp with highmem, testing wanted

[Pavel Machek]

Hi!

> > I actually ran it on real 2GB machine, and it seemed to do the trick,
> > unless "too much" memory was full.
> 
> Well your patch really looked nothing more than a nasty hack, since it
> has known and very real failures. Why do you need to copy all highmem
> down to low mem? That cannot _ever_ work reliably?!

Because it is only solution I know that does not require rewriting half
the kernel or rewriting all the block drivers. (see how swsusp already
does copy of lowmem).

Having special "poll" mode for block drivers might do the trick, but thats lot of work.

Which operations are allowed to access highmem? Can I rely on
block device read/write not accessing highmem?

> > What wories me is 
> > 
> > +               kaddr = kmap_atomic(page, KM_USER0);
> > +               memcpy(save->data, kaddr, PAGE_SIZE);
> > +               kunmap_atomic(kaddr, KM_USER0);
> > 
> > : am I allowed to use KM_USER0, or should I get new KM_constant just
> > for me?
> 
> KM_USER0 should be fine.

Thanks.
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: swsusp with highmem, testing wanted

[Jens Axboe]

On Thu, Mar 25 2004, Pavel Machek wrote:
> Hi!
> 
> > > I actually ran it on real 2GB machine, and it seemed to do the trick,
> > > unless "too much" memory was full.
> > 
> > Well your patch really looked nothing more than a nasty hack, since it
> > has known and very real failures. Why do you need to copy all highmem
> > down to low mem? That cannot _ever_ work reliably?!
> 
> Because it is only solution I know that does not require rewriting half
> the kernel or rewriting all the block drivers. (see how swsusp already
> does copy of lowmem).

I don't understand, why would you need to rewrite block drivers?! Either
way, your patch surely is a bad idea no matter what way you look at it.

> Having special "poll" mode for block drivers might do the trick, but
> thats lot of work.

Maybe I'm missing something, but why doesn't the regular io paths work?

> Which operations are allowed to access highmem? Can I rely on
> block device read/write not accessing highmem?

You mean modify highmem pages, or?

-- 
Jens Axboe

Re: swsusp with highmem, testing wanted

[Pavel Machek]

Hi!

> > Having special "poll" mode for block drivers might do the trick, but
> > thats lot of work.
> 
> Maybe I'm missing something, but why doesn't the regular io paths
> work?

(Basically) you want to replace all kernel data with kernel data saved
on disk. How do you do that using normal i/o paths? If you'll read
"new" data 4KB at a time, you'll crash... because you still need "old"
data to do the reading, and "new" data may fit on same physical spot
in memory.

There are two solutions to this:

* require half of memory to be free during suspend. That way you can
read "new" data onto free spots, then cli and copy

* assume we had special "polling" ide driver that only uses memory
between 0-640KB. That way, I'd have to make sure that 0-640KB is free
during suspending, but otherwise it would work...

Do you see the problem now?

> > Which operations are allowed to access highmem? Can I rely on
> > block device read/write not accessing highmem?
> 
> You mean modify highmem pages, or?

I'd like to know this. Suppose I ask block subsystem to read from disk
into page @1.8GB. All the highmem contains trash. Will block subsystem
be able to work in this situation?
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: swsusp with highmem, testing wanted

[Jens Axboe]

On Thu, Mar 25 2004, Pavel Machek wrote:
> Hi!
> 
> > > Having special "poll" mode for block drivers might do the trick, but
> > > thats lot of work.
> > 
> > Maybe I'm missing something, but why doesn't the regular io paths
> > work?
> 
> (Basically) you want to replace all kernel data with kernel data saved
> on disk. How do you do that using normal i/o paths? If you'll read
> "new" data 4KB at a time, you'll crash... because you still need "old"
> data to do the reading, and "new" data may fit on same physical spot
> in memory.
> 
> There are two solutions to this:
> 
> * require half of memory to be free during suspend. That way you can
> read "new" data onto free spots, then cli and copy
> 
> * assume we had special "polling" ide driver that only uses memory
> between 0-640KB. That way, I'd have to make sure that 0-640KB is free
> during suspending, but otherwise it would work...
> 
> Do you see the problem now?

I see what you mean.

> > > Which operations are allowed to access highmem? Can I rely on
> > > block device read/write not accessing highmem?
> > 
> > You mean modify highmem pages, or?
> 
> I'd like to know this. Suppose I ask block subsystem to read from disk
> into page @1.8GB. All the highmem contains trash. Will block subsystem
> be able to work in this situation?

We've never enforced anything like that, so you cannot rely on it. Block
layer itself doesn't keep anything in high memory, and I cannot imagine
any drivers that do either.

-- 
Jens Axboe

Re: swsusp with highmem, testing wanted

[Pavel Machek]

Hi!

> > > > Which operations are allowed to access highmem? Can I rely on
> > > > block device read/write not accessing highmem?
> > > 
> > > You mean modify highmem pages, or?
> > 
> > I'd like to know this. Suppose I ask block subsystem to read from disk
> > into page @1.8GB. All the highmem contains trash. Will block subsystem
> > be able to work in this situation?
> 
> We've never enforced anything like that, so you cannot rely on it. Block
> layer itself doesn't keep anything in high memory, and I cannot imagine
> any drivers that do either.

Well, if it is possible to enforce that it stays that way, it should
be possible to get something more inteligent for highmem.

								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: swsusp with highmem, testing wanted

[Pavel Machek]

Hi!

> I think this kind of thing should help stabilize swsusp in the presence
> of memory holes, which can be important for embedded devices which would
> in the future find swsusp useful for power-saving purposes.

I had to apply this to compile it, I have not ran it yet.

								Pavel

--- tmp/linux/kernel/power/swsusp.c	2004-03-25 14:42:07.000000000 +0100
+++ linux/kernel/power/swsusp.c	2004-03-25 14:41:12.000000000 +0100
@@ -443,16 +443,16 @@
 
 static int pfn_is_nosave(unsigned long pfn)
 {
-	static const unsigned long nosave_begin_pfn
+	unsigned long nosave_begin_pfn
 				= __pa(&__nosave_begin) >> PAGE_SHIFT;
-	static const unsigned long nosave_end_pfn
+	unsigned long nosave_end_pfn
 				= PAGE_ALIGN(__pa(&__nosave_end)) >> PAGE_SHIFT;
 	return pfn >= nosave_begin_pfn && pfn < nosave_end_pfn;
 }
 
 static int count_and_copy_zone(struct zone *zone, struct pbe **pagedir_p)
 {
-	unsigned long zone_pfn, nr_copy_pages = 0;
+	unsigned long zone_pfn, chunk_size, nr_copy_pages = 0;
 	struct pbe *pbe = *pagedir_p;
 	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
 		struct page *page;
@@ -472,7 +472,7 @@
 		}
 		nr_copy_pages++;
 		if (!pbe)
-			continue
+			continue;
 		pbe->orig_address = page_address(page);
 		copy_page((void *)pbe->address, (void *)pbe->orig_address);
 		pbe++;
@@ -495,7 +495,7 @@
 static void free_suspend_pagedir_zone(struct zone *zone, unsigned long pagedir)
 {
 	unsigned long zone_pfn, pagedir_end, pagedir_pfn, pagedir_end_pfn;
-	pagedir_end = pagedir + PAGE_SIZE << pagedir_order;
+	pagedir_end = pagedir + (PAGE_SIZE << pagedir_order);
 	pagedir_pfn = __pa(pagedir) >> PAGE_SHIFT;
 	pagedir_end_pfn = __pa(pagedir_end) >> PAGE_SHIFT;
 	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
@@ -517,7 +517,7 @@
 	struct zone *zone;
 	for_each_zone(zone) {
 		if (!is_highmem(zone))
-			free_suspend_pagedir_zone(this_pagedir);
+			free_suspend_pagedir_zone(zone, this_pagedir);
 	}
 	free_pages(this_pagedir, pagedir_order);
 }

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: swsusp with highmem, testing wanted

[William Lee Irwin III]

At some point in the past, I wrote:
>> I think this kind of thing should help stabilize swsusp in the presence
>> of memory holes, which can be important for embedded devices which would
>> in the future find swsusp useful for power-saving purposes.

On Thu, Mar 25, 2004 at 10:59:19PM +0100, Pavel Machek wrote:
> I had to apply this to compile it, I have not ran it yet.

Looks like I had a gaffe or two in there. Let me know if there's any
trouble running it. The thing was meant to be equivalent to the prior
code on ia32, and to avoid some pfn <-> page conversion issues that
matter on other systems.


-- wli

Re: swsusp with highmem, testing wanted

[Pavel Machek]

Hi!

> At some point in the past, I wrote:
> >> I think this kind of thing should help stabilize swsusp in the presence
> >> of memory holes, which can be important for embedded devices which would
> >> in the future find swsusp useful for power-saving purposes.
> 
> On Thu, Mar 25, 2004 at 10:59:19PM +0100, Pavel Machek wrote:
> > I had to apply this to compile it, I have not ran it yet.
> 
> Looks like I had a gaffe or two in there. Let me know if there's any
> trouble running it. The thing was meant to be equivalent to the prior
> code on ia32, and to avoid some pfn <-> page conversion issues that
> matter on other systems.

Well, you forgot to dd chunk_size -1 to zone_pfn, too, and that took
me a while to find. Here's the final patch, and that one seems to work
okay.

								Pavel

--- tmp/linux/kernel/power/swsusp.c	2004-03-26 12:51:45.000000000 +0100
+++ linux/kernel/power/swsusp.c	2004-03-26 12:10:06.000000000 +0100
@@ -102,7 +102,7 @@
    allocated at time of resume, that travels through memory not to
    collide with anything.
 
-   Warning: this is even more evil than it seems. Pagedirs this files
+   Warning: this is even more evil than it seems. Pagedirs this file
    talks about are completely different from page directories used by
    MMU hardware.
  */
@@ -383,6 +383,9 @@
 		void *kaddr;
 		unsigned long pfn = zone_pfn + zone->zone_start_pfn;
 		int chunk_size;
+
+		if (!(pfn%200))
+			printk(".");
 		if (!pfn_valid(pfn))
 			continue;
 		page = pfn_to_page(pfn);
@@ -398,6 +401,7 @@
 		}
 		if ((chunk_size = is_head_of_free_region(page))) {
 			pfn += chunk_size - 1;
+			zone_pfn += chunk_size - 1;
 			continue;
 		}
 		save = kmalloc(sizeof(struct highmem_page), GFP_ATOMIC);
@@ -405,7 +409,7 @@
 			panic("Not enough memory");
 		save->next = highmem_copy;
 		save->page = page;
-		save->data = get_zeroed_page(GFP_ATOMIC);
+		save->data = (void *) get_zeroed_page(GFP_ATOMIC);
 		if (!save->data)
 			panic("Not enough memory");
 		kaddr = kmap_atomic(page, KM_USER0);
@@ -415,7 +419,6 @@
 	}
 }
 
-/* if pagedir_p != NULL it also copies the counted pages */
 static void save_highmem(void)
 {
 	struct zone *zone;
@@ -435,7 +438,7 @@
 		kaddr = kmap_atomic(save->page, KM_USER0);
 		memcpy(kaddr, save->data, PAGE_SIZE);
 		kunmap_atomic(kaddr, KM_USER0);
-		free_page(save->data);
+		free_page((long) save->data);
 		kfree(save);
 	}
 	return 0;
@@ -443,37 +446,41 @@
 
 static int pfn_is_nosave(unsigned long pfn)
 {
-	static const unsigned long nosave_begin_pfn
-				= __pa(&__nosave_begin) >> PAGE_SHIFT;
-	static const unsigned long nosave_end_pfn
-				= PAGE_ALIGN(__pa(&__nosave_end)) >> PAGE_SHIFT;
-	return pfn >= nosave_begin_pfn && pfn < nosave_end_pfn;
+	unsigned long nosave_begin_pfn = __pa(&__nosave_begin) >> PAGE_SHIFT;
+	unsigned long nosave_end_pfn = PAGE_ALIGN(__pa(&__nosave_end)) >> PAGE_SHIFT;
+	return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
 }
 
+/* if *pagedir_p != NULL it also copies the counted pages */
 static int count_and_copy_zone(struct zone *zone, struct pbe **pagedir_p)
 {
-	unsigned long zone_pfn, nr_copy_pages = 0;
+	unsigned long zone_pfn, chunk_size, nr_copy_pages = 0;
 	struct pbe *pbe = *pagedir_p;
 	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
 		struct page *page;
 		unsigned long pfn = zone_pfn + zone->zone_start_pfn;
+
+		if (!(pfn%200))
+			printk(".");
 		if (!pfn_valid(pfn))
 			continue;
 		page = pfn_to_page(pfn);
 		BUG_ON(PageReserved(page) && PageNosave(page));
 		if (PageNosave(page))
 			continue;
-		else if (PageReserved(page) && pfn_is_nosave(pfn)) {
+		if (PageReserved(page) && pfn_is_nosave(pfn)) {
 			PRINTK("[nosave pfn 0x%lx]", pfn);
 			continue;
-		} else if ((chunk_size = is_head_of_free_region(page))) {
+		}
+		if ((chunk_size = is_head_of_free_region(page))) {
 			pfn += chunk_size - 1;
+			zone_pfn += chunk_size - 1;
 			continue;
 		}
 		nr_copy_pages++;
 		if (!pbe)
-			continue
-		pbe->orig_address = page_address(page);
+			continue;
+		pbe->orig_address = (long) page_address(page);
 		copy_page((void *)pbe->address, (void *)pbe->orig_address);
 		pbe++;
 	}
@@ -495,7 +502,7 @@
 static void free_suspend_pagedir_zone(struct zone *zone, unsigned long pagedir)
 {
 	unsigned long zone_pfn, pagedir_end, pagedir_pfn, pagedir_end_pfn;
-	pagedir_end = pagedir + PAGE_SIZE << pagedir_order;
+	pagedir_end = pagedir + (PAGE_SIZE << pagedir_order);
 	pagedir_pfn = __pa(pagedir) >> PAGE_SHIFT;
 	pagedir_end_pfn = __pa(pagedir_end) >> PAGE_SHIFT;
 	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
@@ -517,7 +524,7 @@
 	struct zone *zone;
 	for_each_zone(zone) {
 		if (!is_highmem(zone))
-			free_suspend_pagedir_zone(this_pagedir);
+			free_suspend_pagedir_zone(zone, this_pagedir);
 	}
 	free_pages(this_pagedir, pagedir_order);
 }


-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: swsusp with highmem, testing wanted

[William Lee Irwin III]

At some point in the past, I wrote:
>> Looks like I had a gaffe or two in there. Let me know if there's any
>> trouble running it. The thing was meant to be equivalent to the prior
>> code on ia32, and to avoid some pfn <-> page conversion issues that
>> matter on other systems.

On Fri, Mar 26, 2004 at 01:08:57PM +0100, Pavel Machek wrote:
> Well, you forgot to dd chunk_size -1 to zone_pfn, too, and that took
> me a while to find. Here's the final patch, and that one seems to work
> okay.

Looks like more than one or two. Sorry if I ended up burning up time on
your end. But thanks for taking the changes and fixing them up.


-- wli