[PATCH 1/3] arm: mm: Update mem bank ordering comment to reflect code

[PATCH 1/3] arm: mm: Update mem bank ordering comment to reflect code

[Michael Bohan]

From: Michael Bohan <mbohan@quicinc.com>

The memory banks are now sorted in bootmem_init() -- thus this
code no longer needs fixing.

Change-Id: Idc20f9c34bef416544081c3b5cf33fe120c63759
Signed-off-by: Michael Bohan <mbohan@quicinc.com>
Signed-off-by: Michael Bohan <mbohan@codeaurora.org>
---
 arch/arm/mm/init.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 7829cb5..ab4890f 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -531,9 +531,8 @@ static void __init free_unused_memmap_node(int node, struct meminfo *mi)
 	unsigned int i;
 
 	/*
-	 * [FIXME] This relies on each bank being in address order.  This
-	 * may not be the case, especially if the user has provided the
-	 * information on the command line.
+	 * This relies on each bank being in address order. The banks
+	 * are sorted previously in bootmem_init().
 	 */
 	for_each_nodebank(i, mi, node) {
 		struct membank *bank = &mi->bank[i];
-- 
1.6.2.3

[PATCH 2/3] arm: Add config option for HOLES_IN_ZONE

[Michael Bohan]

From: Michael Bohan <mbohan@quicinc.com>

This option is for FLATMEM configurations with multiple memory
banks that have end addresses not aligned to MAX_ORDER_NR_PAGES.

To save memory, ARM frees memmap regions for pfns associated with
gaps in the address map. When the VM code migrates pages, it will
assume that the memmap entries are valid.

This option adds extra checking with pfn_valid() to
ensure the memmap entries are valid. It may adversely impact
performance.

Change-Id: I48d8afb69db25b1f44d698f1a7da106911b4be1c
Signed-off-by: Michael Bohan <mbohan@quicinc.com>
Signed-off-by: Michael Bohan <mbohan@codeaurora.org>
---
 arch/arm/Kconfig |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index c5408bf..8dadce9 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1233,6 +1233,20 @@ config OABI_COMPAT
 config ARCH_HAS_HOLES_MEMORYMODEL
 	bool
 
+config HOLES_IN_ZONE
+	bool "Memory map has holes ending within MAX_ORDER_NR_PAGES"
+	default n
+	depends on FLATMEM
+	help
+	  Say 'y' here if you have multiple memory banks where the bank
+	  end addresses coresponding to the holes are not MAX_ORDER_NR_PAGES
+	  aligned. Failure to enable this in those circumstances will result
+	  in the VM code assuming ownership of pages with no memmap memory
+	  allocated for them. This option could impact performance as it
+	  performs extra checks in the VM code. As alternative, consider
+	  aligning end addresses to MAX_ORDER_NR_PAGES or changing MAX_ORDER
+	  to compensate for your alignment requirements.
+
 # Discontigmem is deprecated
 config ARCH_DISCONTIGMEM_ENABLE
 	bool
-- 
1.6.2.3

[PATCH 3/3] arm: mm: Add alignment check for memory banks in FLATMEM

[Michael Bohan]

From: Michael Bohan <mbohan@quicinc.com>

In FLATMEM configurations, the VM code assumes that memory bank
end addresses in a hole are MAX_ORDER_NR_PAGES aligned if
CONFIG_HOLES_IN_ZONE is not enabled. The crash scenarios are hard
to debug, so let's not proceed if we aren't configured right.

Change-Id: I1494d7bf124e4ec85996af9ae81b394a3322ec6b
Signed-off-by: Michael Bohan <mbohan@quicinc.com>
Signed-off-by: Michael Bohan <mbohan@codeaurora.org>
---
 arch/arm/mm/init.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index ab4890f..3fee7d4 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -254,6 +254,24 @@ static void __init bootmem_init_node(int node, struct meminfo *mi,
 	for_each_nodebank(i, mi, node) {
 		struct membank *bank = &mi->bank[i];
 
+#if defined(CONFIG_FLATMEM) && !defined(CONFIG_HOLES_IN_ZONE)
+		/*
+		 * The VM code assumes that hole end addresses are aligned if
+		 * CONFIG_HOLES_IN_ZONE is not enabled. This results in
+		 * panics since we free unused memmap entries on ARM.
+		 * This check shouldn't be necessary for the last bank's end
+		 * address, since the VM code accounts for the total zone size.
+		 */
+		if ((i < (mi->nr_banks - 1)) &&
+		    (bank_pfn_end(bank) & (MAX_ORDER_NR_PAGES - 1))) {
+			pr_err("Memory bank[%d] not aligned to 0x%x bytes.\n"
+			       "\tMake bank end address align with MAX_ORDER\n"
+			       "\tor enable option CONFIG_HOLES_IN_ZONE.\n",
+			       i, __pfn_to_phys(MAX_ORDER_NR_PAGES));
+			BUG();
+		}
+#endif
+
 		if (!bank->highmem)
 			map_memory_bank(bank);
 	}
-- 
1.6.2.3

[PATCH 2/3] arm: Add config option for HOLES_IN_ZONE

[Mel Gorman]

On Thu, Apr 01, 2010 at 05:20:48PM -0700, Michael Bohan wrote:
> From: Michael Bohan <mbohan@quicinc.com>
> 
> This option is for FLATMEM configurations with multiple memory
> banks that have end addresses not aligned to MAX_ORDER_NR_PAGES.
> 
> To save memory, ARM frees memmap regions for pfns associated with
> gaps in the address map. When the VM code migrates pages, it will
> assume that the memmap entries are valid.
> 
> This option adds extra checking with pfn_valid() to
> ensure the memmap entries are valid. It may adversely impact
> performance.
> 

It will impact performance although not necessarily in a way that's easy to
detect because the biggest impact is on page-allocator-intensive-loads.

Have you managed to trigger this problem? I am assuming you are not
referring to page migration as such but more likely when pages shuffle
between lists in move_freepages(). To trigger a problem there, I think
you would have to have a memory layout that had an
max-order-unaligned-hole in the middle of a zone. Is that happening?

If so, another possibility you may want to consider (if you haven't
already) is to ensure in free_memmap() that if holes are being punched
in the middle of a zone that they are only max-order aligned. This would
avoid corruption problems without taking a performance hit.

> Change-Id: I48d8afb69db25b1f44d698f1a7da106911b4be1c
> Signed-off-by: Michael Bohan <mbohan@quicinc.com>
> Signed-off-by: Michael Bohan <mbohan@codeaurora.org>
> ---
>  arch/arm/Kconfig |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index c5408bf..8dadce9 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1233,6 +1233,20 @@ config OABI_COMPAT
>  config ARCH_HAS_HOLES_MEMORYMODEL
>  	bool
>  
> +config HOLES_IN_ZONE
> +	bool "Memory map has holes ending within MAX_ORDER_NR_PAGES"
> +	default n
> +	depends on FLATMEM
> +	help
> +	  Say 'y' here if you have multiple memory banks where the bank
> +	  end addresses coresponding to the holes are not MAX_ORDER_NR_PAGES
> +	  aligned. Failure to enable this in those circumstances will result
> +	  in the VM code assuming ownership of pages with no memmap memory
> +	  allocated for them. This option could impact performance as it
> +	  performs extra checks in the VM code. As alternative, consider
> +	  aligning end addresses to MAX_ORDER_NR_PAGES or changing MAX_ORDER
> +	  to compensate for your alignment requirements.
> +
>  # Discontigmem is deprecated
>  config ARCH_DISCONTIGMEM_ENABLE
>  	bool
> -- 
> 1.6.2.3
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

[PATCH 2/3] arm: Add config option for HOLES_IN_ZONE

[Michael Bohan]

On 4/2/2010 2:56 AM, Mel Gorman wrote:
> Have you managed to trigger this problem? I am assuming you are not
> referring to page migration as such but more likely when pages shuffle
> between lists in move_freepages(). To trigger a problem there, I think
> you would have to have a memory layout that had an
> max-order-unaligned-hole in the middle of a zone. Is that happening?
>    

Yes, that is exactly what's happening.  I communed with you some time 
ago regarding this problem.  I see crashes in move_freepages_block() 
without this patch.

> If so, another possibility you may want to consider (if you haven't
> already) is to ensure in free_memmap() that if holes are being punched
> in the middle of a zone that they are only max-order aligned. This would
> avoid corruption problems without taking a performance hit.
>    

I did consider this.  The only problem here is that for each unaligned 
memory hole, we waste some more amount of memory (8, 16, or 24KB) 
assuming 1 MB aligned end addresses and MAX_ORDER == 11.  That doesn't 
sound that bad, but there's always the chance that somebody has a lot of 
holes in their memory map.    I suppose it's not truly 'wasted' in the 
sense that it's necessary for correctness; and I agree that it may be 
better than taking a run-time performance hit.

As you suggested, it's not easy to quantify what sort of performance hit 
we're taking.  Without solid data, it might be safer to go your 
suggested route of freeing only legitimate memmap entries.  I will test 
on my end that this does indeed keep the allocator happy (eg. no 
crashes) and does not cause any bad observable side effects.

I guess the only questions for linux-arm-kernel is whether they would be 
okay with freeing a little less memmap in the case of unaligned bank end 
addresses, and whether they think this warrants an info message to the 
user during bootup that they should consider aligning them to save a 
little memory.

Thanks,
Michael