[PATCH] [MIPS] Hibernation: only save pages in system ram

[PATCH] [MIPS] Hibernation: only save pages in system ram

[Wu Zhangjin]

From: Wu Zhangjin <wuzj@lemote.com>

when using hibernation(STD) with CONFIG_FLATMEM in linux-mips-64bit, it
fails for the current mips-specific hibernation implementation save the
pages in all of the memory space(except the nosave section) and make
there will be not enough memory left to the STD task itself, and then
fail. in reality, we only need to save the pages in system rams.

here is the reason why it fail:

kernel/power/snapshot.c:

static void mark_nosave_pages(struct memory_bitmap *bm)
{
		...
		if (pfn_valid(pfn)) {
			...
		}
}

arch/mips/include/asm/page.h:

	...
	#ifdef CONFIG_FLATMEM

	#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)

	#elif defined(CONFIG_SPARSEMEM)

	/* pfn_valid is defined in linux/mmzone.h */
	...

we can rewrite pfn_valid(pfn) to fix this problem, but I really do not
want to touch such a widely-used macro, so, I used another solution:

static struct page *saveable_page(struct zone *zone, unsigned long pfn)
{
	...
	if ( .... pfn_is_nosave(pfn)
		return NULL;
	...
}

and pfn_is_nosave is implemented in arch/mips/power/cpu.c, so, hacking
this one is better.

Signed-off-by: Wu Zhangjin <wuzj@lemote.com>
---
 arch/mips/power/cpu.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/arch/mips/power/cpu.c b/arch/mips/power/cpu.c
index 7995df4..ef472e3 100644
--- a/arch/mips/power/cpu.c
+++ b/arch/mips/power/cpu.c
@@ -10,6 +10,7 @@
 #include <asm/suspend.h>
 #include <asm/fpu.h>
 #include <asm/dsp.h>
+#include <asm/bootinfo.h>
 
 static u32 saved_status;
 struct pt_regs saved_regs;
@@ -34,10 +35,26 @@ void restore_processor_state(void)
 		restore_dsp(current);
 }
 
+int pfn_in_system_ram(unsigned long pfn)
+{
+	int i;
+
+	for (i = 0; i < boot_mem_map.nr_map; i++) {
+		if (boot_mem_map.map[i].type == BOOT_MEM_RAM) {
+			if ((pfn >= (boot_mem_map.map[i].addr >> PAGE_SHIFT)) &&
+				((pfn) < ((boot_mem_map.map[i].addr +
+					boot_mem_map.map[i].size) >> PAGE_SHIFT)))
+				return 1;
+		}
+	}
+	return 0;
+}
+
 int pfn_is_nosave(unsigned long pfn)
 {
 	unsigned long nosave_begin_pfn = PFN_DOWN(__pa(&__nosave_begin));
 	unsigned long nosave_end_pfn = PFN_UP(__pa(&__nosave_end));
 
-	return	(pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
+	return ((pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn))
+			|| !pfn_in_system_ram(pfn);
 }
-- 
1.6.0.4

Re: [PATCH] [MIPS] Hibernation: only save pages in system ram

[Pavel Machek]

> arch/mips/include/asm/page.h:
> 
> 	...
> 	#ifdef CONFIG_FLATMEM
> 
> 	#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
> 
> 	#elif defined(CONFIG_SPARSEMEM)
> 
> 	/* pfn_valid is defined in linux/mmzone.h */
> 	...
> 
> we can rewrite pfn_valid(pfn) to fix this problem, but I really do not
> want to touch such a widely-used macro, so, I used another solution:

Well, fixing pfn_valid() is the right way to go. It makes mips behave
like other architectures. Please do that.

> Signed-off-by: Wu Zhangjin <wuzj@lemote.com>

NAK.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] [MIPS] Hibernation: only save pages in system ram

[Ralf Baechle]

On Tue, Jun 30, 2009 at 10:52:50PM +0800, Wu Zhangjin wrote:

> From: Wu Zhangjin <wuzj@lemote.com>
> 
> when using hibernation(STD) with CONFIG_FLATMEM in linux-mips-64bit, it
> fails for the current mips-specific hibernation implementation save the
> pages in all of the memory space(except the nosave section) and make
> there will be not enough memory left to the STD task itself, and then
> fail. in reality, we only need to save the pages in system rams.
> 
> here is the reason why it fail:
> 
> kernel/power/snapshot.c:
> 
> static void mark_nosave_pages(struct memory_bitmap *bm)
> {
> 		...
> 		if (pfn_valid(pfn)) {
> 			...
> 		}
> }
> 
> arch/mips/include/asm/page.h:
> 
> 	...
> 	#ifdef CONFIG_FLATMEM
> 
> 	#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
> 
> 	#elif defined(CONFIG_SPARSEMEM)
> 
> 	/* pfn_valid is defined in linux/mmzone.h */
> 	...
> 
> we can rewrite pfn_valid(pfn) to fix this problem, but I really do not
> want to touch such a widely-used macro, so, I used another solution:
> 
> static struct page *saveable_page(struct zone *zone, unsigned long pfn)
> {
> 	...
> 	if ( .... pfn_is_nosave(pfn)
> 		return NULL;
> 	...
> }
> 
> and pfn_is_nosave is implemented in arch/mips/power/cpu.c, so, hacking
> this one is better.

No - pfn_valid() is broken, so it should be fixed.  Commit
752fbeb2e3555c0d236e992f1195fd7ce30e728d introduced the breakage.  It
seemed to assume that the valid range for PFNs doesn't start at 0 but
some higher number but got that entirely wrong..

#define ARCH_PFN_OFFSET         PFN_UP(PHYS_OFFSET)
#define pfn_valid(pfn)         ((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)

works nicely when PHYS_OFFSET is 0 - as for most MIPS systems and goes
horribly wrong otherwise.  So I suggest below patch.

  Ralf

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

 arch/mips/include/asm/page.h |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/arch/mips/include/asm/page.h b/arch/mips/include/asm/page.h
index dc0eaa7..96a14a4 100644
--- a/arch/mips/include/asm/page.h
+++ b/arch/mips/include/asm/page.h
@@ -165,7 +165,14 @@ typedef struct { unsigned long pgprot; } pgprot_t;
 
 #ifdef CONFIG_FLATMEM
 
-#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
+#define pfn_valid(pfn)							\
+({									\
+	unsigned long __pfn = (pfn);					\
+	/* avoid <linux/bootmem.h> include hell */			\
+	extern unsigned long min_low_pfn;				\
+									\
+	__pfn >= min_low_pfn && __pfn < max_mapnr;			\
+})
 
 #elif defined(CONFIG_SPARSEMEM)
 

Re: [PATCH] [MIPS] Hibernation: only save pages in system ram

[Rafael J. Wysocki]

On Tuesday 30 June 2009, Wu Zhangjin wrote:
> From: Wu Zhangjin <wuzj@lemote.com>
> 
> when using hibernation(STD) with CONFIG_FLATMEM in linux-mips-64bit, it
> fails for the current mips-specific hibernation implementation save the
> pages in all of the memory space(except the nosave section) and make
> there will be not enough memory left to the STD task itself, and then
> fail. in reality, we only need to save the pages in system rams.
> 
> here is the reason why it fail:
> 
> kernel/power/snapshot.c:
> 
> static void mark_nosave_pages(struct memory_bitmap *bm)
> {
> 		...
> 		if (pfn_valid(pfn)) {
> 			...
> 		}
> }
> 
> arch/mips/include/asm/page.h:
> 
> 	...
> 	#ifdef CONFIG_FLATMEM
> 
> 	#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
> 
> 	#elif defined(CONFIG_SPARSEMEM)
> 
> 	/* pfn_valid is defined in linux/mmzone.h */
> 	...
> 
> we can rewrite pfn_valid(pfn) to fix this problem, but I really do not
> want to touch such a widely-used macro, so, I used another solution:
> 
> static struct page *saveable_page(struct zone *zone, unsigned long pfn)
> {
> 	...
> 	if ( .... pfn_is_nosave(pfn)
> 		return NULL;
> 	...
> }
> 
> and pfn_is_nosave is implemented in arch/mips/power/cpu.c, so, hacking
> this one is better.

Not really.

Could that be handled with the help of register_nosave_region() or
register_nosave_region_late() instead?

Best,
Rafael


> Signed-off-by: Wu Zhangjin <wuzj@lemote.com>
> ---
>  arch/mips/power/cpu.c |   19 ++++++++++++++++++-
>  1 files changed, 18 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/mips/power/cpu.c b/arch/mips/power/cpu.c
> index 7995df4..ef472e3 100644
> --- a/arch/mips/power/cpu.c
> +++ b/arch/mips/power/cpu.c
> @@ -10,6 +10,7 @@
>  #include <asm/suspend.h>
>  #include <asm/fpu.h>
>  #include <asm/dsp.h>
> +#include <asm/bootinfo.h>
>  
>  static u32 saved_status;
>  struct pt_regs saved_regs;
> @@ -34,10 +35,26 @@ void restore_processor_state(void)
>  		restore_dsp(current);
>  }
>  
> +int pfn_in_system_ram(unsigned long pfn)
> +{
> +	int i;
> +
> +	for (i = 0; i < boot_mem_map.nr_map; i++) {
> +		if (boot_mem_map.map[i].type == BOOT_MEM_RAM) {
> +			if ((pfn >= (boot_mem_map.map[i].addr >> PAGE_SHIFT)) &&
> +				((pfn) < ((boot_mem_map.map[i].addr +
> +					boot_mem_map.map[i].size) >> PAGE_SHIFT)))
> +				return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
>  int pfn_is_nosave(unsigned long pfn)
>  {
>  	unsigned long nosave_begin_pfn = PFN_DOWN(__pa(&__nosave_begin));
>  	unsigned long nosave_end_pfn = PFN_UP(__pa(&__nosave_end));
>  
> -	return	(pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
> +	return ((pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn))
> +			|| !pfn_in_system_ram(pfn);
>  }

Re: [PATCH] [MIPS] Hibernation: only save pages in system ram

[Wu Zhangjin]

On Fri, 2009-07-03 at 00:22 +0100, Ralf Baechle wrote:
> On Tue, Jun 30, 2009 at 10:52:50PM +0800, Wu Zhangjin wrote:
> 
> > From: Wu Zhangjin <wuzj@lemote.com>
> > 
> > when using hibernation(STD) with CONFIG_FLATMEM in linux-mips-64bit, it
> > fails for the current mips-specific hibernation implementation save the
> > pages in all of the memory space(except the nosave section) and make
> > there will be not enough memory left to the STD task itself, and then
> > fail. in reality, we only need to save the pages in system rams.
> > 
> > here is the reason why it fail:
> > 
> > kernel/power/snapshot.c:
> > 
> > static void mark_nosave_pages(struct memory_bitmap *bm)
> > {
> > 		...
> > 		if (pfn_valid(pfn)) {
> > 			...
> > 		}
> > }
> > 
> > arch/mips/include/asm/page.h:
> > 
> > 	...
> > 	#ifdef CONFIG_FLATMEM
> > 
> > 	#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
> > 
> > 	#elif defined(CONFIG_SPARSEMEM)
> > 
> > 	/* pfn_valid is defined in linux/mmzone.h */
> > 	...
> > 
> > we can rewrite pfn_valid(pfn) to fix this problem, but I really do not
> > want to touch such a widely-used macro, so, I used another solution:
> > 
> > static struct page *saveable_page(struct zone *zone, unsigned long pfn)
> > {
> > 	...
> > 	if ( .... pfn_is_nosave(pfn)
> > 		return NULL;
> > 	...
> > }
> > 
> > and pfn_is_nosave is implemented in arch/mips/power/cpu.c, so, hacking
> > this one is better.
> 
> No - pfn_valid() is broken, so it should be fixed.  Commit
> 752fbeb2e3555c0d236e992f1195fd7ce30e728d introduced the breakage.  It
> seemed to assume that the valid range for PFNs doesn't start at 0 but
> some higher number but got that entirely wrong..
> 
> #define ARCH_PFN_OFFSET         PFN_UP(PHYS_OFFSET)
> #define pfn_valid(pfn)         ((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
> 
> works nicely when PHYS_OFFSET is 0 - as for most MIPS systems and goes
> horribly wrong otherwise.  So I suggest below patch.

Just test it with the latest master branch of linux-mips on fulong2e,
Your patch works well, thanks!

(BTW: of course, the ide-relative bug is also there even with Bart's
patch, so, currently, I just revert this part of the ide patch: "ide:
improve handling of Power Management requests" to test your patch: 

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index d5f3c77..8c4608c 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -468,12 +468,12 @@ void do_ide_request(struct request_queue *q)
                ide_hwif_t *prev_port;
 repeat:
                prev_port = hwif->host->cur_port;
-
+/*
                if (drive->dev_flags & IDE_DFLAG_BLOCKED)
                        rq = hwif->rq;
                else
                        WARN_ON_ONCE(hwif->rq);
-
+*/
                if (drive->dev_flags & IDE_DFLAG_SLEEPING &&
                    time_after(drive->sleep, jiffies)) {
                        ide_unlock_port(hwif);

So, I will try to test Bart's patch with linux-next, I'm cloning it
currently! perhaps there is something missing in my git repo or the
linux-mips git repo? because i can not apply bart's patch neither to my
git repo nor to the master branch of linux-mips git repo directly, even
if pulled linux-next and Davie's ide-2.6 in.
)

Regards,
Wu Zhangjin

> 
>   Ralf
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> 
>  arch/mips/include/asm/page.h |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/mips/include/asm/page.h b/arch/mips/include/asm/page.h
> index dc0eaa7..96a14a4 100644
> --- a/arch/mips/include/asm/page.h
> +++ b/arch/mips/include/asm/page.h
> @@ -165,7 +165,14 @@ typedef struct { unsigned long pgprot; } pgprot_t;
>  
>  #ifdef CONFIG_FLATMEM
>  
> -#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
> +#define pfn_valid(pfn)							\
> +({									\
> +	unsigned long __pfn = (pfn);					\
> +	/* avoid <linux/bootmem.h> include hell */			\
> +	extern unsigned long min_low_pfn;				\
> +									\
> +	__pfn >= min_low_pfn && __pfn < max_mapnr;			\
> +})
>  
>  #elif defined(CONFIG_SPARSEMEM)
>  

Re: [PATCH] [MIPS] Hibernation: only save pages in system ram

[Wu Zhangjin]

On Fri, 2009-07-03 at 11:21 +0800, Wu Zhangjin wrote:
> On Fri, 2009-07-03 at 00:22 +0100, Ralf Baechle wrote:
> > On Tue, Jun 30, 2009 at 10:52:50PM +0800, Wu Zhangjin wrote:
> > 
> > > From: Wu Zhangjin <wuzj@lemote.com>
> > > 
> > > when using hibernation(STD) with CONFIG_FLATMEM in linux-mips-64bit, it
> > > fails for the current mips-specific hibernation implementation save the
> > > pages in all of the memory space(except the nosave section) and make
> > > there will be not enough memory left to the STD task itself, and then
> > > fail. in reality, we only need to save the pages in system rams.
> > > 
> > > here is the reason why it fail:
> > > 
> > > kernel/power/snapshot.c:
> > > 
> > > static void mark_nosave_pages(struct memory_bitmap *bm)
> > > {
> > > 		...
> > > 		if (pfn_valid(pfn)) {
> > > 			...
> > > 		}
> > > }
> > > 
> > > arch/mips/include/asm/page.h:
> > > 
> > > 	...
> > > 	#ifdef CONFIG_FLATMEM
> > > 
> > > 	#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
> > > 
> > > 	#elif defined(CONFIG_SPARSEMEM)
> > > 
> > > 	/* pfn_valid is defined in linux/mmzone.h */
> > > 	...
> > > 
> > > we can rewrite pfn_valid(pfn) to fix this problem, but I really do not
> > > want to touch such a widely-used macro, so, I used another solution:
> > > 
> > > static struct page *saveable_page(struct zone *zone, unsigned long pfn)
> > > {
> > > 	...
> > > 	if ( .... pfn_is_nosave(pfn)
> > > 		return NULL;
> > > 	...
> > > }
> > > 
> > > and pfn_is_nosave is implemented in arch/mips/power/cpu.c, so, hacking
> > > this one is better.
> > 
> > No - pfn_valid() is broken, so it should be fixed.  Commit
> > 752fbeb2e3555c0d236e992f1195fd7ce30e728d introduced the breakage.  It
> > seemed to assume that the valid range for PFNs doesn't start at 0 but
> > some higher number but got that entirely wrong..
> > 
> > #define ARCH_PFN_OFFSET         PFN_UP(PHYS_OFFSET)
> > #define pfn_valid(pfn)         ((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
> > 
> > works nicely when PHYS_OFFSET is 0 - as for most MIPS systems and goes
> > horribly wrong otherwise.  So I suggest below patch.
> 
> Just test it with the latest master branch of linux-mips on fulong2e,
> Your patch works well, thanks!
> 
> (BTW: of course, the ide-relative bug is also there even with Bart's
> patch, so, currently, I just revert this part of the ide patch: "ide:
> improve handling of Power Management requests" to test your patch: 
> 
> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> index d5f3c77..8c4608c 100644
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -468,12 +468,12 @@ void do_ide_request(struct request_queue *q)
>                 ide_hwif_t *prev_port;
>  repeat:
>                 prev_port = hwif->host->cur_port;
> -
> +/*
>                 if (drive->dev_flags & IDE_DFLAG_BLOCKED)
>                         rq = hwif->rq;
>                 else
>                         WARN_ON_ONCE(hwif->rq);
> -
> +*/
>                 if (drive->dev_flags & IDE_DFLAG_SLEEPING &&
>                     time_after(drive->sleep, jiffies)) {
>                         ide_unlock_port(hwif);
> 
> So, I will try to test Bart's patch with linux-next, I'm cloning it
> currently! perhaps there is something missing in my git repo or the
> linux-mips git repo? because i can not apply bart's patch neither to 
> git repo nor to the master branch of linux-mips git repo directly, even
> if pulled linux-next and Davie's ide-2.6 in.

Sorry, I have missed something in Bart's patch, Can apply it now, so
please ignore the above complaint.

but unfortunately, Bart's patch also not fix the problem of STD, I have
reported my test result in another E-mail thread: "[Bug #13663] suspend
to ram regression (IDE related)", so, please ignore this thread, thanks!

Regards,
Wu Zhangjin

> )
> 
> Regards,
> Wu Zhangjin
> 
> > 
> >   Ralf
> > 
> > Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> > 
> >  arch/mips/include/asm/page.h |    9 ++++++++-
> >  1 files changed, 8 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/mips/include/asm/page.h b/arch/mips/include/asm/page.h
> > index dc0eaa7..96a14a4 100644
> > --- a/arch/mips/include/asm/page.h
> > +++ b/arch/mips/include/asm/page.h
> > @@ -165,7 +165,14 @@ typedef struct { unsigned long pgprot; } pgprot_t;
> >  
> >  #ifdef CONFIG_FLATMEM
> >  
> > -#define pfn_valid(pfn)		((pfn) >= ARCH_PFN_OFFSET && (pfn) < max_mapnr)
> > +#define pfn_valid(pfn)							\
> > +({									\
> > +	unsigned long __pfn = (pfn);					\
> > +	/* avoid <linux/bootmem.h> include hell */			\
> > +	extern unsigned long min_low_pfn;				\
> > +									\
> > +	__pfn >= min_low_pfn && __pfn < max_mapnr;			\
> > +})
> >  
> >  #elif defined(CONFIG_SPARSEMEM)
> >