[PATCH] ARM : sparsemem: Crashes on ARM platform when sparsemem enabled in linux-2.6.​35.13 due to pfn_valid(​) and pfn_valid_​within().

[PATCH] ARM : sparsemem: Crashes on ARM platform when sparsemem enabled in linux-2.6.​35.13 due to pfn_valid(​) and pfn_valid_​within().

[Kautuk Consul]

Hi,

On my ARM machine, I have linux-2.6.35.13 installed and the total
kernel memory is not aligned to the section size SECTION_SIZE_BITS.

I observe kernel crashes in the following 3 scenarios:
i)    When we do a "cat /proc/pagetypeinfo": This happens because the
pfn_valid() macro is not able to detect invalid PFNs in the loop in
vmstat.c: pagetypeinfo_showblockcount_print().
ii)    When we do "echo xxxx > /proc/vm/sys/min_free_kbytes": This
happens because the pfn_valid() macro is not able to detect invalid
PFNs in page_alloc.c: setup_zone_migrate_reserve().
iii)   When I try to copy a really huge file: This happens because the
CONFIG_HOLES_IN_ZONE config option is not set.
       The code then crashes in the VM_BUG_ON in loop in
move_freepages() as pfn_valid_within() did not compile correctly to
pfn_valid().

This patch is a combination of :
a)  Back-ported changes of the patch from Will Deacon found at:
http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=commit;h=7b7bf499f79de3f6c85a340c8453a78789523f85
b) Addition of the CONFIG_HOLES_IN_ZONE config option to
arch/arm/Kconfig in order to prevent crashes in move_freepages()
when/if the total kernel memory is not aligned to SECTION_SIZE_BITS.
This also leads to
proper compilation of the pfn_valid_within() macro which otherwise
will always return 1 to the caller.

Apologies for the last patch which I sent to these mailing lists in
improper format.

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Kautuk Consul <consul.kautuk@gmail.com>
---
 arch/arm/include/asm/page.h |    2 +-
 arch/arm/Kconfig            |    6 ++++++
 arch/arm/mm/init.c          |    4 +++-
 include/linux/mmzone.h      |    2 ++
 4 files changed, 12 insertions(+), 2 deletions(-)

diff -uprN a/arch/arm/include/asm/page.h b/arch/arm/include/asm/page.h
--- a/arch/arm/include/asm/page.h       2011-08-02 10:04:54.917207995 +0530
+++ b/arch/arm/include/asm/page.h       2011-08-02 10:14:45.464208248 +0530
@@ -195,7 +195,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 -uprN a/arch/arm/Kconfig b/arch/arm/Kconfig
--- a/arch/arm/Kconfig  2011-08-02 10:04:55.607207996 +0530
+++ b/arch/arm/Kconfig  2011-08-02 10:13:56.461207994 +0530
@@ -1265,6 +1265,12 @@ config ARCH_SPARSEMEM_DEFAULT
 config ARCH_SELECT_MEMORY_MODEL
       def_bool ARCH_DISCONTIGMEM_ENABLE && ARCH_SPARSEMEM_ENABLE

+config ARCH_PROVIDES_PFN_VALID
+       def_bool ARCH_HAS_HOLES_MEMORYMODEL || !SPARSEMEM
+
+config HOLES_IN_ZONE
+       def_bool ARCH_HAS_HOLES_MEMORYMODEL || SPARSEMEM
+
 config NODES_SHIFT
       int
       default "4" if ARCH_LH7A40X
diff -uprN a/arch/arm/mm/init.c b/arch/arm/mm/init.c
--- a/arch/arm/mm/init.c        2011-08-02 10:04:55.492207995 +0530
+++ b/arch/arm/mm/init.c        2011-08-02 10:29:47.408208131 +0530
@@ -325,7 +325,7 @@ static void __init bootmem_free_node(int
       free_area_init_node(node, zone_size, min, zhole_size);
 }

-#ifndef CONFIG_SPARSEMEM
+#ifdef CONFIG_ARCH_PROVIDES_PFN_VALID
 int pfn_valid(unsigned long pfn)
 {
       struct meminfo *mi = &meminfo;
@@ -345,7 +345,9 @@ int pfn_valid(unsigned long pfn)
       return 0;
 }
 EXPORT_SYMBOL(pfn_valid);
+#endif

+#ifndef CONFIG_SPARSEMEM
 static void arm_memory_present(struct meminfo *mi, int node)
 {
 }
diff -uprN a/include/linux/mmzone.h b/include/linux/mmzone.h
--- a/include/linux/mmzone.h    2011-08-02 10:04:48.998207995 +0530
+++ b/include/linux/mmzone.h    2011-08-02 10:16:49.007207996 +0530
@@ -1058,12 +1058,14 @@ static inline struct mem_section *__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)
 {
--

Thanks,
Kautuk.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] ARM : sparsemem: Crashes on ARM platform when sparsemem enabled in linux-2.6.???35.13 due to pfn_valid(???) and pfn_valid_???within().

[Mel Gorman]

On Tue, Aug 02, 2011 at 01:36:49PM +0530, Kautuk Consul wrote:
> Hi,
> 
> On my ARM machine, I have linux-2.6.35.13 installed and the total
> kernel memory is not aligned to the section size SECTION_SIZE_BITS.
> 
> I observe kernel crashes in the following 3 scenarios:
> i)    When we do a "cat /proc/pagetypeinfo": This happens because the
> pfn_valid() macro is not able to detect invalid PFNs in the loop in
> vmstat.c: pagetypeinfo_showblockcount_print().
> ii)    When we do "echo xxxx > /proc/vm/sys/min_free_kbytes": This
> happens because the pfn_valid() macro is not able to detect invalid
> PFNs in page_alloc.c: setup_zone_migrate_reserve().
> iii)   When I try to copy a really huge file: This happens because the
> CONFIG_HOLES_IN_ZONE config option is not set.
>        The code then crashes in the VM_BUG_ON in loop in
> move_freepages() as pfn_valid_within() did not compile correctly to
> pfn_valid().
> 
> This patch is a combination of :
> a)  Back-ported changes of the patch from Will Deacon found at:
> http://git.kernel.org/?p=linux/kernel/git/stable/linux-3.0.y.git;a=commit;h=7b7bf499f79de3f6c85a340c8453a78789523f85
> b) Addition of the CONFIG_HOLES_IN_ZONE config option to
> arch/arm/Kconfig in order to prevent crashes in move_freepages()
> when/if the total kernel memory is not aligned to SECTION_SIZE_BITS.
> This also leads to
> proper compilation of the pfn_valid_within() macro which otherwise
> will always return 1 to the caller.
> 

Ok, for -stable backports, it is required that each patch is a backport
of a single commit. This is to ensure that there are no accidental
forks or fixes that get lost. If this patch is a combination of two
patches, then the expected format for review would be in three mails

Mail 1: [Patch 0/2] A leader explaining why the series is required. The
	information you have above is fine

Mail 2: [Patch 1/2] would be commit 
        [7b7bf499: ARM: 6913/1: sparsemem: allow pfn_valid to be
        overridden when using SPARSEMEM]

	The only difference between that commit and your commit would
	be that it's being sent to stable and at the beginning of the
	mail you should have

	"commit: 7b7bf499f79de3f6c85a340c8453a78789523f85"

	This is to the history of the patch is clear at a glance

Mail 3: [Patch 2/2] would be whatever commit introduces
	CONFIG_HOLES_IN_ZONE. If this is not in mainline already,
	it should be merged to mainline before backporting.

See Documentation/stable_kernel_rules.txt .

In this format for example, I would not even have to review patch 1/2
properly other than checking it's equivalent to the mainstream patch
that I already ack'd. It'd probably have been picked up by now :/ .

I don't know about patch 2 yet because I didn't go to the effort of
applying this patch, reverting the upstream commit and examining the
remainder.

I know this appears awkward but review bandwidth is extremely limited
and it's important that fixes do not get lost.

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>