[PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[Will Deacon]

In commit eb33575c ("[ARM] Double check memmap is actually valid with a
memmap has unexpected holes V2"), a new function, memmap_valid_within,
was introduced to mmzone.h so that holes in the memmap which pass
pfn_valid in SPARSEMEM configurations can be detected and avoided.

The fix to this problem checks that the pfn <-> page linkages are
correct by calculating the page for the pfn and then checking that
page_to_pfn on that page returns the original pfn. Unfortunately, in
SPARSEMEM configurations, this results in reading from the page flags to
determine the correct section. Since the memmap here has been freed,
junk is read from memory and the check is no longer robust.

In the best case, reading from /proc/pagetypeinfo will give you the
wrong answer. In the worst case, you get SEGVs, Kernel OOPses and hung
CPUs.

This patch allows architectures to provide their own pfn_valid function
instead of using the default implementation used by sparsemem. The
architecture-specific version is aware of the memmap state and will
return false when passed a pfn for a freed page within a valid section.

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Mel Gorman <mgorman@suse.de>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/Kconfig            |    3 +++
 arch/arm/include/asm/page.h |    2 +-
 arch/arm/mm/init.c          |    4 +++-
 include/linux/mmzone.h      |    2 ++
 4 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 377a7a5..d6cfc9c 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1520,6 +1520,9 @@ config ARCH_SPARSEMEM_DEFAULT
 config ARCH_SELECT_MEMORY_MODEL
 	def_bool ARCH_SPARSEMEM_ENABLE
 
+config ARCH_PROVIDES_PFN_VALID
+	def_bool ARCH_HAS_HOLES_MEMORYMODEL || !SPARSEMEM
+
 config HIGHMEM
 	bool "High Memory Support (EXPERIMENTAL)"
 	depends on MMU && EXPERIMENTAL
diff --git a/arch/arm/include/asm/page.h b/arch/arm/include/asm/page.h
index f51a695..8702233 100644
--- a/arch/arm/include/asm/page.h
+++ b/arch/arm/include/asm/page.h
@@ -197,7 +197,7 @@ typedef unsigned long pgprot_t;
 
 typedef struct page *pgtable_t;
 
-#ifndef CONFIG_SPARSEMEM
+#ifdef CONFIG_ARCH_PROVIDES_PFN_VALID
 extern int pfn_valid(unsigned long);
 #endif
 
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index e591513..d425b36 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -252,13 +252,15 @@ static void __init arm_bootmem_free(unsigned long min, unsigned long max_low,
 	free_area_init_node(0, zone_size, min, zhole_size);
 }
 
-#ifndef CONFIG_SPARSEMEM
+#ifdef CONFIG_ARCH_PROVIDES_PFN_VALID
 int pfn_valid(unsigned long pfn)
 {
 	return memblock_is_memory(pfn << PAGE_SHIFT);
 }
 EXPORT_SYMBOL(pfn_valid);
+#endif
 
+#ifndef CONFIG_SPARSEMEM
 static void arm_memory_present(void)
 {
 }
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index e56f835..72225dd 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1053,12 +1053,14 @@ static inline struct mem_section *__pfn_to_section(unsigned long pfn)
 	return __nr_to_section(pfn_to_section_nr(pfn));
 }
 
+#ifndef CONFIG_ARCH_PROVIDES_PFN_VALID
 static inline int pfn_valid(unsigned long pfn)
 {
 	if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
 		return 0;
 	return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
 }
+#endif
 
 static inline int pfn_present(unsigned long pfn)
 {
-- 
1.7.0.4

[PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[Will Deacon]

In commit eb33575c ("[ARM] Double check memmap is actually valid with a
memmap has unexpected holes V2"), a new function, memmap_valid_within,
was introduced to mmzone.h so that holes in the memmap which pass
pfn_valid in SPARSEMEM configurations can be detected and avoided.

The fix to this problem checks that the pfn <-> page linkages are
correct by calculating the page for the pfn and then checking that
page_to_pfn on that page returns the original pfn. Unfortunately, in
SPARSEMEM configurations, this results in reading from the page flags to
determine the correct section. Since the memmap here has been freed,
junk is read from memory and the check is no longer robust.

In the best case, reading from /proc/pagetypeinfo will give you the
wrong answer. In the worst case, you get SEGVs, Kernel OOPses and hung
CPUs.

This patch allows architectures to provide their own pfn_valid function
instead of using the default implementation used by sparsemem. The
architecture-specific version is aware of the memmap state and will
return false when passed a pfn for a freed page within a valid section.

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Mel Gorman <mgorman@suse.de>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/Kconfig            |    3 +++
 arch/arm/include/asm/page.h |    2 +-
 arch/arm/mm/init.c          |    4 +++-
 include/linux/mmzone.h      |    2 ++
 4 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 377a7a5..d6cfc9c 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1520,6 +1520,9 @@ config ARCH_SPARSEMEM_DEFAULT
 config ARCH_SELECT_MEMORY_MODEL
 	def_bool ARCH_SPARSEMEM_ENABLE
 
+config ARCH_PROVIDES_PFN_VALID
+	def_bool ARCH_HAS_HOLES_MEMORYMODEL || !SPARSEMEM
+
 config HIGHMEM
 	bool "High Memory Support (EXPERIMENTAL)"
 	depends on MMU && EXPERIMENTAL
diff --git a/arch/arm/include/asm/page.h b/arch/arm/include/asm/page.h
index f51a695..8702233 100644
--- a/arch/arm/include/asm/page.h
+++ b/arch/arm/include/asm/page.h
@@ -197,7 +197,7 @@ typedef unsigned long pgprot_t;
 
 typedef struct page *pgtable_t;
 
-#ifndef CONFIG_SPARSEMEM
+#ifdef CONFIG_ARCH_PROVIDES_PFN_VALID
 extern int pfn_valid(unsigned long);
 #endif
 
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index e591513..d425b36 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -252,13 +252,15 @@ static void __init arm_bootmem_free(unsigned long min, unsigned long max_low,
 	free_area_init_node(0, zone_size, min, zhole_size);
 }
 
-#ifndef CONFIG_SPARSEMEM
+#ifdef CONFIG_ARCH_PROVIDES_PFN_VALID
 int pfn_valid(unsigned long pfn)
 {
 	return memblock_is_memory(pfn << PAGE_SHIFT);
 }
 EXPORT_SYMBOL(pfn_valid);
+#endif
 
+#ifndef CONFIG_SPARSEMEM
 static void arm_memory_present(void)
 {
 }
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index e56f835..72225dd 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1053,12 +1053,14 @@ static inline struct mem_section *__pfn_to_section(unsigned long pfn)
 	return __nr_to_section(pfn_to_section_nr(pfn));
 }
 
+#ifndef CONFIG_ARCH_PROVIDES_PFN_VALID
 static inline int pfn_valid(unsigned long pfn)
 {
 	if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
 		return 0;
 	return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
 }
+#endif
 
 static inline int pfn_present(unsigned long pfn)
 {
-- 
1.7.0.4

Re: [PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[Mel Gorman]

On Wed, May 18, 2011 at 05:03:59PM +0100, Will Deacon wrote:
> In commit eb33575c ("[ARM] Double check memmap is actually valid with a
> memmap has unexpected holes V2"), a new function, memmap_valid_within,
> was introduced to mmzone.h so that holes in the memmap which pass
> pfn_valid in SPARSEMEM configurations can be detected and avoided.
> 
> The fix to this problem checks that the pfn <-> page linkages are
> correct by calculating the page for the pfn and then checking that
> page_to_pfn on that page returns the original pfn. Unfortunately, in
> SPARSEMEM configurations, this results in reading from the page flags to
> determine the correct section. Since the memmap here has been freed,
> junk is read from memory and the check is no longer robust.
> 
> In the best case, reading from /proc/pagetypeinfo will give you the
> wrong answer. In the worst case, you get SEGVs, Kernel OOPses and hung
> CPUs.
> 
> This patch allows architectures to provide their own pfn_valid function
> instead of using the default implementation used by sparsemem. The
> architecture-specific version is aware of the memmap state and will
> return false when passed a pfn for a freed page within a valid section.
> 
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Mel Gorman <mgorman@suse.de>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

I don't have an ARM machine to test on and I'm not particularly
sensitive to the requirements of ARM so I'm not the best reviewer. If
this passes tests, I see little problem with it other than the
architecture-specific pfn_valid is slower than the sparsemem equivalent
and the cache footprint is probably higher as memblock_is_memory
is searching a list of blocks. If this problem is exclusive to
reading /proc/pagetypeinfo, you might want to consider only using
memblock_is_memory in that case. Otherwise, functionally it looks like
it should work.

> ---
>  arch/arm/Kconfig            |    3 +++
>  arch/arm/include/asm/page.h |    2 +-
>  arch/arm/mm/init.c          |    4 +++-
>  include/linux/mmzone.h      |    2 ++
>  4 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 377a7a5..d6cfc9c 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1520,6 +1520,9 @@ config ARCH_SPARSEMEM_DEFAULT
>  config ARCH_SELECT_MEMORY_MODEL
>  	def_bool ARCH_SPARSEMEM_ENABLE
>  
> +config ARCH_PROVIDES_PFN_VALID
> +	def_bool ARCH_HAS_HOLES_MEMORYMODEL || !SPARSEMEM
> +
>  config HIGHMEM
>  	bool "High Memory Support (EXPERIMENTAL)"
>  	depends on MMU && EXPERIMENTAL
> diff --git a/arch/arm/include/asm/page.h b/arch/arm/include/asm/page.h
> index f51a695..8702233 100644
> --- a/arch/arm/include/asm/page.h
> +++ b/arch/arm/include/asm/page.h
> @@ -197,7 +197,7 @@ typedef unsigned long pgprot_t;
>  
>  typedef struct page *pgtable_t;
>  
> -#ifndef CONFIG_SPARSEMEM
> +#ifdef CONFIG_ARCH_PROVIDES_PFN_VALID
>  extern int pfn_valid(unsigned long);
>  #endif
>  
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index e591513..d425b36 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -252,13 +252,15 @@ static void __init arm_bootmem_free(unsigned long min, unsigned long max_low,
>  	free_area_init_node(0, zone_size, min, zhole_size);
>  }
>  
> -#ifndef CONFIG_SPARSEMEM
> +#ifdef CONFIG_ARCH_PROVIDES_PFN_VALID
>  int pfn_valid(unsigned long pfn)
>  {
>  	return memblock_is_memory(pfn << PAGE_SHIFT);
>  }
>  EXPORT_SYMBOL(pfn_valid);
> +#endif
>  
> +#ifndef CONFIG_SPARSEMEM
>  static void arm_memory_present(void)
>  {
>  }
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index e56f835..72225dd 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1053,12 +1053,14 @@ static inline struct mem_section *__pfn_to_section(unsigned long pfn)
>  	return __nr_to_section(pfn_to_section_nr(pfn));
>  }
>  
> +#ifndef CONFIG_ARCH_PROVIDES_PFN_VALID
>  static inline int pfn_valid(unsigned long pfn)
>  {
>  	if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
>  		return 0;
>  	return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
>  }
> +#endif
>  
>  static inline int pfn_present(unsigned long pfn)
>  {
> -- 
> 1.7.0.4
> 

-- 
Mel Gorman
SUSE Labs

[PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[Mel Gorman]

On Wed, May 18, 2011 at 05:03:59PM +0100, Will Deacon wrote:
> In commit eb33575c ("[ARM] Double check memmap is actually valid with a
> memmap has unexpected holes V2"), a new function, memmap_valid_within,
> was introduced to mmzone.h so that holes in the memmap which pass
> pfn_valid in SPARSEMEM configurations can be detected and avoided.
> 
> The fix to this problem checks that the pfn <-> page linkages are
> correct by calculating the page for the pfn and then checking that
> page_to_pfn on that page returns the original pfn. Unfortunately, in
> SPARSEMEM configurations, this results in reading from the page flags to
> determine the correct section. Since the memmap here has been freed,
> junk is read from memory and the check is no longer robust.
> 
> In the best case, reading from /proc/pagetypeinfo will give you the
> wrong answer. In the worst case, you get SEGVs, Kernel OOPses and hung
> CPUs.
> 
> This patch allows architectures to provide their own pfn_valid function
> instead of using the default implementation used by sparsemem. The
> architecture-specific version is aware of the memmap state and will
> return false when passed a pfn for a freed page within a valid section.
> 
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Mel Gorman <mgorman@suse.de>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

I don't have an ARM machine to test on and I'm not particularly
sensitive to the requirements of ARM so I'm not the best reviewer. If
this passes tests, I see little problem with it other than the
architecture-specific pfn_valid is slower than the sparsemem equivalent
and the cache footprint is probably higher as memblock_is_memory
is searching a list of blocks. If this problem is exclusive to
reading /proc/pagetypeinfo, you might want to consider only using
memblock_is_memory in that case. Otherwise, functionally it looks like
it should work.

> ---
>  arch/arm/Kconfig            |    3 +++
>  arch/arm/include/asm/page.h |    2 +-
>  arch/arm/mm/init.c          |    4 +++-
>  include/linux/mmzone.h      |    2 ++
>  4 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 377a7a5..d6cfc9c 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1520,6 +1520,9 @@ config ARCH_SPARSEMEM_DEFAULT
>  config ARCH_SELECT_MEMORY_MODEL
>  	def_bool ARCH_SPARSEMEM_ENABLE
>  
> +config ARCH_PROVIDES_PFN_VALID
> +	def_bool ARCH_HAS_HOLES_MEMORYMODEL || !SPARSEMEM
> +
>  config HIGHMEM
>  	bool "High Memory Support (EXPERIMENTAL)"
>  	depends on MMU && EXPERIMENTAL
> diff --git a/arch/arm/include/asm/page.h b/arch/arm/include/asm/page.h
> index f51a695..8702233 100644
> --- a/arch/arm/include/asm/page.h
> +++ b/arch/arm/include/asm/page.h
> @@ -197,7 +197,7 @@ typedef unsigned long pgprot_t;
>  
>  typedef struct page *pgtable_t;
>  
> -#ifndef CONFIG_SPARSEMEM
> +#ifdef CONFIG_ARCH_PROVIDES_PFN_VALID
>  extern int pfn_valid(unsigned long);
>  #endif
>  
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index e591513..d425b36 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -252,13 +252,15 @@ static void __init arm_bootmem_free(unsigned long min, unsigned long max_low,
>  	free_area_init_node(0, zone_size, min, zhole_size);
>  }
>  
> -#ifndef CONFIG_SPARSEMEM
> +#ifdef CONFIG_ARCH_PROVIDES_PFN_VALID
>  int pfn_valid(unsigned long pfn)
>  {
>  	return memblock_is_memory(pfn << PAGE_SHIFT);
>  }
>  EXPORT_SYMBOL(pfn_valid);
> +#endif
>  
> +#ifndef CONFIG_SPARSEMEM
>  static void arm_memory_present(void)
>  {
>  }
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index e56f835..72225dd 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1053,12 +1053,14 @@ static inline struct mem_section *__pfn_to_section(unsigned long pfn)
>  	return __nr_to_section(pfn_to_section_nr(pfn));
>  }
>  
> +#ifndef CONFIG_ARCH_PROVIDES_PFN_VALID
>  static inline int pfn_valid(unsigned long pfn)
>  {
>  	if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
>  		return 0;
>  	return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
>  }
> +#endif
>  
>  static inline int pfn_present(unsigned long pfn)
>  {
> -- 
> 1.7.0.4
> 

-- 
Mel Gorman
SUSE Labs

RE: [PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[H Hartley Sweeten]

On Wednesday, May 18, 2011 9:04 AM, Will Deacon wrote:
>
> In commit eb33575c ("[ARM] Double check memmap is actually valid with a
> memmap has unexpected holes V2"), a new function, memmap_valid_within,
> was introduced to mmzone.h so that holes in the memmap which pass
> pfn_valid in SPARSEMEM configurations can be detected and avoided.
>
> The fix to this problem checks that the pfn <-> page linkages are
> correct by calculating the page for the pfn and then checking that
> page_to_pfn on that page returns the original pfn. Unfortunately, in
> SPARSEMEM configurations, this results in reading from the page flags to
> determine the correct section. Since the memmap here has been freed,
> junk is read from memory and the check is no longer robust.
>
> In the best case, reading from /proc/pagetypeinfo will give you the
> wrong answer. In the worst case, you get SEGVs, Kernel OOPses and hung
> CPUs.
>
> This patch allows architectures to provide their own pfn_valid function
> instead of using the default implementation used by sparsemem. The
> architecture-specific version is aware of the memmap state and will
> return false when passed a pfn for a freed page within a valid section.
>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Mel Gorman <mgorman@suse.de>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

I tested this on an EP93xx based system which uses ARCH_HAS_HOLES_MEMORYMODEL.
The EP9307A has 64MB of memory that appears as two 32MB blocks at addresses
0xc0000000 and 0xc4000000.  Currently the EP93xx uses a Flat Memory model and
the hole used to cause a Kernel OOPs before commit e80d6a24 ("[ARM] Skip memory
holes in FLATMEM when reading /proc/pagetypeinfo"), which is where I think this
all started.

Without your patch I get this when reading /proc/pagetypeinfo:

# cat /proc/pagetypeinfo
Page block order: 10
Pages per block:  1024

Free pages count per migrate type at order       0      1      2      3      4      5      6      7      8      9     10
Node    0, zone   Normal, type    Unmovable      3      4      3      0      2      2      4      5      2      2      3
Node    0, zone   Normal, type  Reclaimable      2      0      0      0      1      0      0      1      0      1      0
Node    0, zone   Normal, type      Movable      0      3      4      1      1      1      0      1      0      2      4
Node    0, zone   Normal, type      Reserve      1      1      1      2      0      0      0      0      0      0      0
Node    0, zone   Normal, type      Isolate      0      0      0      0      0      0      0      0      0      0      0

Number of blocks type     Unmovable  Reclaimable      Movable      Reserve      Isolate
Node 0, zone   Normal            8            1            6            1            0

After your patch I get this:

# cat /proc/pagetypeinfo
Page block order: 10
Pages per block:  1024

Free pages count per migrate type at order       0      1      2      3      4      5      6      7      8      9     10
Node    0, zone   Normal, type    Unmovable      1      0      2      0      1      3      4      3      3      2      3
Node    0, zone   Normal, type  Reclaimable      0      1      0      0      1      0      0      1      0      1      0
Node    0, zone   Normal, type      Movable      1      1      1      1      0      0      1      1      0      2      4
Node    0, zone   Normal, type      Reserve      1      1      1      2      0      0      0      0      0      0      0
Node    0, zone   Normal, type      Isolate      0      0      0      0      0      0      0      0      0      0      0

Number of blocks type     Unmovable  Reclaimable      Movable      Reserve      Isolate
Node 0, zone   Normal            8            1            6            1            0

I'm not sure what the output "should" be, but the patch does not seem to
cause any issues.  So feel free to add:

Tested-by: H Hartley Sweeten <hsweeten@visionengravers.com>

[PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[H Hartley Sweeten]

On Wednesday, May 18, 2011 9:04 AM, Will Deacon wrote:
>
> In commit eb33575c ("[ARM] Double check memmap is actually valid with a
> memmap has unexpected holes V2"), a new function, memmap_valid_within,
> was introduced to mmzone.h so that holes in the memmap which pass
> pfn_valid in SPARSEMEM configurations can be detected and avoided.
>
> The fix to this problem checks that the pfn <-> page linkages are
> correct by calculating the page for the pfn and then checking that
> page_to_pfn on that page returns the original pfn. Unfortunately, in
> SPARSEMEM configurations, this results in reading from the page flags to
> determine the correct section. Since the memmap here has been freed,
> junk is read from memory and the check is no longer robust.
>
> In the best case, reading from /proc/pagetypeinfo will give you the
> wrong answer. In the worst case, you get SEGVs, Kernel OOPses and hung
> CPUs.
>
> This patch allows architectures to provide their own pfn_valid function
> instead of using the default implementation used by sparsemem. The
> architecture-specific version is aware of the memmap state and will
> return false when passed a pfn for a freed page within a valid section.
>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Mel Gorman <mgorman@suse.de>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

I tested this on an EP93xx based system which uses ARCH_HAS_HOLES_MEMORYMODEL.
The EP9307A has 64MB of memory that appears as two 32MB blocks at addresses
0xc0000000 and 0xc4000000.  Currently the EP93xx uses a Flat Memory model and
the hole used to cause a Kernel OOPs before commit e80d6a24 ("[ARM] Skip memory
holes in FLATMEM when reading /proc/pagetypeinfo"), which is where I think this
all started.

Without your patch I get this when reading /proc/pagetypeinfo:

# cat /proc/pagetypeinfo
Page block order: 10
Pages per block:  1024

Free pages count per migrate type at order       0      1      2      3      4      5      6      7      8      9     10
Node    0, zone   Normal, type    Unmovable      3      4      3      0      2      2      4      5      2      2      3
Node    0, zone   Normal, type  Reclaimable      2      0      0      0      1      0      0      1      0      1      0
Node    0, zone   Normal, type      Movable      0      3      4      1      1      1      0      1      0      2      4
Node    0, zone   Normal, type      Reserve      1      1      1      2      0      0      0      0      0      0      0
Node    0, zone   Normal, type      Isolate      0      0      0      0      0      0      0      0      0      0      0

Number of blocks type     Unmovable  Reclaimable      Movable      Reserve      Isolate
Node 0, zone   Normal            8            1            6            1            0

After your patch I get this:

# cat /proc/pagetypeinfo
Page block order: 10
Pages per block:  1024

Free pages count per migrate type at order       0      1      2      3      4      5      6      7      8      9     10
Node    0, zone   Normal, type    Unmovable      1      0      2      0      1      3      4      3      3      2      3
Node    0, zone   Normal, type  Reclaimable      0      1      0      0      1      0      0      1      0      1      0
Node    0, zone   Normal, type      Movable      1      1      1      1      0      0      1      1      0      2      4
Node    0, zone   Normal, type      Reserve      1      1      1      2      0      0      0      0      0      0      0
Node    0, zone   Normal, type      Isolate      0      0      0      0      0      0      0      0      0      0      0

Number of blocks type     Unmovable  Reclaimable      Movable      Reserve      Isolate
Node 0, zone   Normal            8            1            6            1            0

I'm not sure what the output "should" be, but the patch does not seem to
cause any issues.  So feel free to add:

Tested-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Re: [PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[Will Deacon]

Hi Mel,

Thanks for looking at this.

On Wed, 2011-05-18 at 17:59 +0100, Mel Gorman wrote:
> On Wed, May 18, 2011 at 05:03:59PM +0100, Will Deacon wrote:
> > In commit eb33575c ("[ARM] Double check memmap is actually valid with a
> > memmap has unexpected holes V2"), a new function, memmap_valid_within,
> > was introduced to mmzone.h so that holes in the memmap which pass
> > pfn_valid in SPARSEMEM configurations can be detected and avoided.
> >
> > The fix to this problem checks that the pfn <-> page linkages are
> > correct by calculating the page for the pfn and then checking that
> > page_to_pfn on that page returns the original pfn. Unfortunately, in
> > SPARSEMEM configurations, this results in reading from the page flags to
> > determine the correct section. Since the memmap here has been freed,
> > junk is read from memory and the check is no longer robust.
> >
> > In the best case, reading from /proc/pagetypeinfo will give you the
> > wrong answer. In the worst case, you get SEGVs, Kernel OOPses and hung
> > CPUs.
> >
> > This patch allows architectures to provide their own pfn_valid function
> > instead of using the default implementation used by sparsemem. The
> > architecture-specific version is aware of the memmap state and will
> > return false when passed a pfn for a freed page within a valid section.
> >
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Mel Gorman <mgorman@suse.de>
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> I don't have an ARM machine to test on and I'm not particularly
> sensitive to the requirements of ARM so I'm not the best reviewer. If
> this passes tests, I see little problem with it other than the
> architecture-specific pfn_valid is slower than the sparsemem equivalent
> and the cache footprint is probably higher as memblock_is_memory
> is searching a list of blocks. 

Yes, it is slower than just checking to see if the sparsemem section is
valid but that is the price you pay for partially populated sections. At
the end of the day, we're just falling back to the pfn_valid definition
that is used when !CONFIG_SPARSEMEM.

> If this problem is exclusive to
> reading /proc/pagetypeinfo, you might want to consider only using
> memblock_is_memory in that case. Otherwise, functionally it looks like
> it should work.

I initially thought it was exclusive to that operation, but it turns out
the problem is more far-reaching as pfn_valid is used by things like the
ioremap code to ensure that we don't remap normal memory.

> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index e56f835..72225dd 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -1053,12 +1053,14 @@ static inline struct mem_section *__pfn_to_section(unsigned long pfn)
> >       return __nr_to_section(pfn_to_section_nr(pfn));
> >  }
> > 
> > +#ifndef CONFIG_ARCH_PROVIDES_PFN_VALID
> >  static inline int pfn_valid(unsigned long pfn)
> >  {
> >       if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
> >               return 0;
> >       return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
> >  }
> > +#endif
> > 
> >  static inline int pfn_present(unsigned long pfn)
> >  {

Can I add your Ack for the changes to mmzone.h please?

Thanks,

Will

[PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[Will Deacon]

Hi Mel,

Thanks for looking at this.

On Wed, 2011-05-18 at 17:59 +0100, Mel Gorman wrote:
> On Wed, May 18, 2011 at 05:03:59PM +0100, Will Deacon wrote:
> > In commit eb33575c ("[ARM] Double check memmap is actually valid with a
> > memmap has unexpected holes V2"), a new function, memmap_valid_within,
> > was introduced to mmzone.h so that holes in the memmap which pass
> > pfn_valid in SPARSEMEM configurations can be detected and avoided.
> >
> > The fix to this problem checks that the pfn <-> page linkages are
> > correct by calculating the page for the pfn and then checking that
> > page_to_pfn on that page returns the original pfn. Unfortunately, in
> > SPARSEMEM configurations, this results in reading from the page flags to
> > determine the correct section. Since the memmap here has been freed,
> > junk is read from memory and the check is no longer robust.
> >
> > In the best case, reading from /proc/pagetypeinfo will give you the
> > wrong answer. In the worst case, you get SEGVs, Kernel OOPses and hung
> > CPUs.
> >
> > This patch allows architectures to provide their own pfn_valid function
> > instead of using the default implementation used by sparsemem. The
> > architecture-specific version is aware of the memmap state and will
> > return false when passed a pfn for a freed page within a valid section.
> >
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Mel Gorman <mgorman@suse.de>
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> I don't have an ARM machine to test on and I'm not particularly
> sensitive to the requirements of ARM so I'm not the best reviewer. If
> this passes tests, I see little problem with it other than the
> architecture-specific pfn_valid is slower than the sparsemem equivalent
> and the cache footprint is probably higher as memblock_is_memory
> is searching a list of blocks. 

Yes, it is slower than just checking to see if the sparsemem section is
valid but that is the price you pay for partially populated sections. At
the end of the day, we're just falling back to the pfn_valid definition
that is used when !CONFIG_SPARSEMEM.

> If this problem is exclusive to
> reading /proc/pagetypeinfo, you might want to consider only using
> memblock_is_memory in that case. Otherwise, functionally it looks like
> it should work.

I initially thought it was exclusive to that operation, but it turns out
the problem is more far-reaching as pfn_valid is used by things like the
ioremap code to ensure that we don't remap normal memory.

> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index e56f835..72225dd 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -1053,12 +1053,14 @@ static inline struct mem_section *__pfn_to_section(unsigned long pfn)
> >       return __nr_to_section(pfn_to_section_nr(pfn));
> >  }
> > 
> > +#ifndef CONFIG_ARCH_PROVIDES_PFN_VALID
> >  static inline int pfn_valid(unsigned long pfn)
> >  {
> >       if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
> >               return 0;
> >       return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
> >  }
> > +#endif
> > 
> >  static inline int pfn_present(unsigned long pfn)
> >  {

Can I add your Ack for the changes to mmzone.h please?

Thanks,

Will

RE: [PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[Will Deacon]

On Wed, 2011-05-18 at 19:53 +0100, H Hartley Sweeten wrote:
> On Wednesday, May 18, 2011 9:04 AM, Will Deacon wrote:
> >
> > In commit eb33575c ("[ARM] Double check memmap is actually valid with a
> > memmap has unexpected holes V2"), a new function, memmap_valid_within,
> > was introduced to mmzone.h so that holes in the memmap which pass
> > pfn_valid in SPARSEMEM configurations can be detected and avoided.

[...]

> I tested this on an EP93xx based system which uses ARCH_HAS_HOLES_MEMORYMODEL.
> The EP9307A has 64MB of memory that appears as two 32MB blocks at addresses
> 0xc0000000 and 0xc4000000.  Currently the EP93xx uses a Flat Memory model and
> the hole used to cause a Kernel OOPs before commit e80d6a24 ("[ARM] Skip memory
> holes in FLATMEM when reading /proc/pagetypeinfo"), which is where I think this
> all started.
> 
[...]

> I'm not sure what the output "should" be, but the patch does not seem to
> cause any issues.  So feel free to add:
> 
> Tested-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Thanks for that. It's good to know that this patch doesn't break
non-sparsemem platforms with whacky flatmem configurations :)

Will

[PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[Will Deacon]

On Wed, 2011-05-18 at 19:53 +0100, H Hartley Sweeten wrote:
> On Wednesday, May 18, 2011 9:04 AM, Will Deacon wrote:
> >
> > In commit eb33575c ("[ARM] Double check memmap is actually valid with a
> > memmap has unexpected holes V2"), a new function, memmap_valid_within,
> > was introduced to mmzone.h so that holes in the memmap which pass
> > pfn_valid in SPARSEMEM configurations can be detected and avoided.

[...]

> I tested this on an EP93xx based system which uses ARCH_HAS_HOLES_MEMORYMODEL.
> The EP9307A has 64MB of memory that appears as two 32MB blocks at addresses
> 0xc0000000 and 0xc4000000.  Currently the EP93xx uses a Flat Memory model and
> the hole used to cause a Kernel OOPs before commit e80d6a24 ("[ARM] Skip memory
> holes in FLATMEM when reading /proc/pagetypeinfo"), which is where I think this
> all started.
> 
[...]

> I'm not sure what the output "should" be, but the patch does not seem to
> cause any issues.  So feel free to add:
> 
> Tested-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Thanks for that. It's good to know that this patch doesn't break
non-sparsemem platforms with whacky flatmem configurations :)

Will

Re: [PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[Mel Gorman]

On Thu, May 19, 2011 at 09:55:17AM +0100, Will Deacon wrote:
> Hi Mel,
> 
> Thanks for looking at this.
> 
> On Wed, 2011-05-18 at 17:59 +0100, Mel Gorman wrote:
> > On Wed, May 18, 2011 at 05:03:59PM +0100, Will Deacon wrote:
> > > In commit eb33575c ("[ARM] Double check memmap is actually valid with a
> > > memmap has unexpected holes V2"), a new function, memmap_valid_within,
> > > was introduced to mmzone.h so that holes in the memmap which pass
> > > pfn_valid in SPARSEMEM configurations can be detected and avoided.
> > >
> > > The fix to this problem checks that the pfn <-> page linkages are
> > > correct by calculating the page for the pfn and then checking that
> > > page_to_pfn on that page returns the original pfn. Unfortunately, in
> > > SPARSEMEM configurations, this results in reading from the page flags to
> > > determine the correct section. Since the memmap here has been freed,
> > > junk is read from memory and the check is no longer robust.
> > >
> > > In the best case, reading from /proc/pagetypeinfo will give you the
> > > wrong answer. In the worst case, you get SEGVs, Kernel OOPses and hung
> > > CPUs.
> > >
> > > This patch allows architectures to provide their own pfn_valid function
> > > instead of using the default implementation used by sparsemem. The
> > > architecture-specific version is aware of the memmap state and will
> > > return false when passed a pfn for a freed page within a valid section.
> > >
> > > Cc: Russell King <linux@arm.linux.org.uk>
> > > Cc: Mel Gorman <mgorman@suse.de>
> > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > 
> > I don't have an ARM machine to test on and I'm not particularly
> > sensitive to the requirements of ARM so I'm not the best reviewer. If
> > this passes tests, I see little problem with it other than the
> > architecture-specific pfn_valid is slower than the sparsemem equivalent
> > and the cache footprint is probably higher as memblock_is_memory
> > is searching a list of blocks. 
> 
> Yes, it is slower than just checking to see if the sparsemem section is
> valid but that is the price you pay for partially populated sections. At
> the end of the day, we're just falling back to the pfn_valid definition
> that is used when !CONFIG_SPARSEMEM.
> 

Ok.

> > If this problem is exclusive to
> > reading /proc/pagetypeinfo, you might want to consider only using
> > memblock_is_memory in that case. Otherwise, functionally it looks like
> > it should work.
> 
> I initially thought it was exclusive to that operation, but it turns out
> the problem is more far-reaching as pfn_valid is used by things like the
> ioremap code to ensure that we don't remap normal memory.
> 

That would justify it. Might want to stick that into the changelog
because we'll forget it and someone will "fix" it :)

> > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > > index e56f835..72225dd 100644
> > > --- a/include/linux/mmzone.h
> > > +++ b/include/linux/mmzone.h
> > > @@ -1053,12 +1053,14 @@ static inline struct mem_section *__pfn_to_section(unsigned long pfn)
> > >       return __nr_to_section(pfn_to_section_nr(pfn));
> > >  }
> > > 
> > > +#ifndef CONFIG_ARCH_PROVIDES_PFN_VALID
> > >  static inline int pfn_valid(unsigned long pfn)
> > >  {
> > >       if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
> > >               return 0;
> > >       return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
> > >  }
> > > +#endif
> > > 
> > >  static inline int pfn_present(unsigned long pfn)
> > >  {
> 
> Can I add your Ack for the changes to mmzone.h please?
> 

Minor nit on the name but it'd be nice if it was simile to
CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID as they are both related to the
memory model. Whether you do it or not in a v2, I'll ack the mmzone.h
change;

Acked-by: Mel Gorman <mgorman@suse.de>

-- 
Mel Gorman
SUSE Labs

[PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[Mel Gorman]

On Thu, May 19, 2011 at 09:55:17AM +0100, Will Deacon wrote:
> Hi Mel,
> 
> Thanks for looking at this.
> 
> On Wed, 2011-05-18 at 17:59 +0100, Mel Gorman wrote:
> > On Wed, May 18, 2011 at 05:03:59PM +0100, Will Deacon wrote:
> > > In commit eb33575c ("[ARM] Double check memmap is actually valid with a
> > > memmap has unexpected holes V2"), a new function, memmap_valid_within,
> > > was introduced to mmzone.h so that holes in the memmap which pass
> > > pfn_valid in SPARSEMEM configurations can be detected and avoided.
> > >
> > > The fix to this problem checks that the pfn <-> page linkages are
> > > correct by calculating the page for the pfn and then checking that
> > > page_to_pfn on that page returns the original pfn. Unfortunately, in
> > > SPARSEMEM configurations, this results in reading from the page flags to
> > > determine the correct section. Since the memmap here has been freed,
> > > junk is read from memory and the check is no longer robust.
> > >
> > > In the best case, reading from /proc/pagetypeinfo will give you the
> > > wrong answer. In the worst case, you get SEGVs, Kernel OOPses and hung
> > > CPUs.
> > >
> > > This patch allows architectures to provide their own pfn_valid function
> > > instead of using the default implementation used by sparsemem. The
> > > architecture-specific version is aware of the memmap state and will
> > > return false when passed a pfn for a freed page within a valid section.
> > >
> > > Cc: Russell King <linux@arm.linux.org.uk>
> > > Cc: Mel Gorman <mgorman@suse.de>
> > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > 
> > I don't have an ARM machine to test on and I'm not particularly
> > sensitive to the requirements of ARM so I'm not the best reviewer. If
> > this passes tests, I see little problem with it other than the
> > architecture-specific pfn_valid is slower than the sparsemem equivalent
> > and the cache footprint is probably higher as memblock_is_memory
> > is searching a list of blocks. 
> 
> Yes, it is slower than just checking to see if the sparsemem section is
> valid but that is the price you pay for partially populated sections. At
> the end of the day, we're just falling back to the pfn_valid definition
> that is used when !CONFIG_SPARSEMEM.
> 

Ok.

> > If this problem is exclusive to
> > reading /proc/pagetypeinfo, you might want to consider only using
> > memblock_is_memory in that case. Otherwise, functionally it looks like
> > it should work.
> 
> I initially thought it was exclusive to that operation, but it turns out
> the problem is more far-reaching as pfn_valid is used by things like the
> ioremap code to ensure that we don't remap normal memory.
> 

That would justify it. Might want to stick that into the changelog
because we'll forget it and someone will "fix" it :)

> > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > > index e56f835..72225dd 100644
> > > --- a/include/linux/mmzone.h
> > > +++ b/include/linux/mmzone.h
> > > @@ -1053,12 +1053,14 @@ static inline struct mem_section *__pfn_to_section(unsigned long pfn)
> > >       return __nr_to_section(pfn_to_section_nr(pfn));
> > >  }
> > > 
> > > +#ifndef CONFIG_ARCH_PROVIDES_PFN_VALID
> > >  static inline int pfn_valid(unsigned long pfn)
> > >  {
> > >       if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
> > >               return 0;
> > >       return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
> > >  }
> > > +#endif
> > > 
> > >  static inline int pfn_present(unsigned long pfn)
> > >  {
> 
> Can I add your Ack for the changes to mmzone.h please?
> 

Minor nit on the name but it'd be nice if it was simile to
CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID as they are both related to the
memory model. Whether you do it or not in a v2, I'll ack the mmzone.h
change;

Acked-by: Mel Gorman <mgorman@suse.de>

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[Will Deacon]

On Thu, 2011-05-19 at 10:23 +0100, Mel Gorman wrote:
> > > If this problem is exclusive to
> > > reading /proc/pagetypeinfo, you might want to consider only using
> > > memblock_is_memory in that case. Otherwise, functionally it looks like
> > > it should work.
> >
> > I initially thought it was exclusive to that operation, but it turns out
> > the problem is more far-reaching as pfn_valid is used by things like the
> > ioremap code to ensure that we don't remap normal memory.
> >
> 
> That would justify it. Might want to stick that into the changelog
> because we'll forget it and someone will "fix" it :)

Good thinking, the last thing we want is having this optimised away!
I've added a statement mentioning ioremap.

> > > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > > > index e56f835..72225dd 100644
> > > > --- a/include/linux/mmzone.h
> > > > +++ b/include/linux/mmzone.h
> > > > @@ -1053,12 +1053,14 @@ static inline struct mem_section *__pfn_to_section(unsigned long pfn)
> > > >       return __nr_to_section(pfn_to_section_nr(pfn));
> > > >  }
> > > >
> > > > +#ifndef CONFIG_ARCH_PROVIDES_PFN_VALID
> > > >  static inline int pfn_valid(unsigned long pfn)
> > > >  {
> > > >       if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
> > > >               return 0;
> > > >       return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
> > > >  }
> > > > +#endif
> > > >
> > > >  static inline int pfn_present(unsigned long pfn)
> > > >  {
> >
> > Can I add your Ack for the changes to mmzone.h please?
> >
> 
> Minor nit on the name but it'd be nice if it was simile to
> CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID as they are both related to the
> memory model. Whether you do it or not in a v2, I'll ack the mmzone.h
> change;

Sure, I didn't notice the other CONFIG option.
CONFIG_HAVE_ARCH_PFN_VALID it is.

> Acked-by: Mel Gorman <mgorman@suse.de>

Cracking, thanks for that. I won't bother with a v2 as the changes are
trivial and I plan to merge this via Russell anyway.

Will

[PATCH] ARM: sparsemem: allow pfn_valid to be overridden when using SPARSEMEM

[Will Deacon]

On Thu, 2011-05-19 at 10:23 +0100, Mel Gorman wrote:
> > > If this problem is exclusive to
> > > reading /proc/pagetypeinfo, you might want to consider only using
> > > memblock_is_memory in that case. Otherwise, functionally it looks like
> > > it should work.
> >
> > I initially thought it was exclusive to that operation, but it turns out
> > the problem is more far-reaching as pfn_valid is used by things like the
> > ioremap code to ensure that we don't remap normal memory.
> >
> 
> That would justify it. Might want to stick that into the changelog
> because we'll forget it and someone will "fix" it :)

Good thinking, the last thing we want is having this optimised away!
I've added a statement mentioning ioremap.

> > > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > > > index e56f835..72225dd 100644
> > > > --- a/include/linux/mmzone.h
> > > > +++ b/include/linux/mmzone.h
> > > > @@ -1053,12 +1053,14 @@ static inline struct mem_section *__pfn_to_section(unsigned long pfn)
> > > >       return __nr_to_section(pfn_to_section_nr(pfn));
> > > >  }
> > > >
> > > > +#ifndef CONFIG_ARCH_PROVIDES_PFN_VALID
> > > >  static inline int pfn_valid(unsigned long pfn)
> > > >  {
> > > >       if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
> > > >               return 0;
> > > >       return valid_section(__nr_to_section(pfn_to_section_nr(pfn)));
> > > >  }
> > > > +#endif
> > > >
> > > >  static inline int pfn_present(unsigned long pfn)
> > > >  {
> >
> > Can I add your Ack for the changes to mmzone.h please?
> >
> 
> Minor nit on the name but it'd be nice if it was simile to
> CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID as they are both related to the
> memory model. Whether you do it or not in a v2, I'll ack the mmzone.h
> change;

Sure, I didn't notice the other CONFIG option.
CONFIG_HAVE_ARCH_PFN_VALID it is.

> Acked-by: Mel Gorman <mgorman@suse.de>

Cracking, thanks for that. I won't bother with a v2 as the changes are
trivial and I plan to merge this via Russell anyway.

Will