[PATCH] arm: mm: Don't free prohibited memmap entries

[PATCH] arm: mm: Don't free prohibited memmap entries

[Michael Bohan]

From: Michael Bohan <mbohan@quicinc.com>

The VM subsystem assumes that there are valid memmap entries to
the bank end aligned to MAX_ORDER_NR_PAGES. It will try and read
these page structs, and so we cannot free any memmap entries that
it may inspect.

Signed-off-by: Michael Bohan <mbohan@quicinc.com>
---
 arch/arm/mm/init.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 83db12a..dcd04bf 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -532,28 +532,27 @@ 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];
 
 		bank_start = bank_pfn_start(bank);
-		if (bank_start < prev_bank_end) {
-			printk(KERN_ERR "MEM: unordered memory banks.  "
-				"Not freeing memmap.\n");
-			break;
-		}
 
 		/*
 		 * If we had a previous bank, and there is a space
 		 * between the current bank and the previous, free it.
 		 */
-		if (prev_bank_end && prev_bank_end != bank_start)
+		if (prev_bank_end && prev_bank_end < bank_start)
 			free_memmap(node, prev_bank_end, bank_start);
 
-		prev_bank_end = bank_pfn_end(bank);
+		/*
+		 * Align up here since the VM subsystem insists that the
+		 * memmap entries are valid from the bank end aligned to
+		 * MAX_ORDER_NR_PAGES.
+		 */
+		prev_bank_end = ALIGN(bank_pfn_end(bank), MAX_ORDER_NR_PAGES);
 	}
 }
 
-- 
1.6.2.3

[PATCH] arm: mm: Don't free prohibited memmap entries

[Russell King - ARM Linux]

On Mon, Apr 05, 2010 at 03:34:21PM -0700, Michael Bohan wrote:
> From: Michael Bohan <mbohan@quicinc.com>
> 
> The VM subsystem assumes that there are valid memmap entries to
> the bank end aligned to MAX_ORDER_NR_PAGES. It will try and read
> these page structs, and so we cannot free any memmap entries that
> it may inspect.

1. are you enabling ARCH_HAS_HOLES_MEMORYMODEL ?
2. where does it try to access these page structs without trying
   pfn_valid() to check whether a page struct exists first?

[PATCH] arm: mm: Don't free prohibited memmap entries

[Michael Bohan]

On 4/6/2010 3:08 PM, Russell King - ARM Linux wrote:
> 1. are you enabling ARCH_HAS_HOLES_MEMORYMODEL ?
>    

Yes, although this does not impact the problem I'm dealing with.  That 
option is only used for /proc/pagetypeinfo currently.  It would be good 
if we could consolidate ARCH_HAS_HOLES_MEMORYMODEL and HOLES_IN_ZONE, 
but that may be out of scope for this change.

> 2. where does it try to access these page structs without trying
>     pfn_valid() to check whether a page struct exists first?
>    

The specific piece of code that is causing crashes in my scenario is in 
vm/page_alloc.c:move_freepages(), called from move_freepages_block().  
The code in move_freepages_block aligns the end_pfn to the closest page 
block, which may take us to invalid memmap entries.

The macro that conditionally saves us in this case is the 
pfn_valid_within(), called from move_freepages().  If HOLES_IN_ZONE is 
configured, this option calls down to pfn_valid() to make sure the page 
has a valid memmap entry.  There are likely other cases where this is an 
issue as well that I haven't run into.

Michael

[PATCH] arm: mm: Don't free prohibited memmap entries

[Russell King - ARM Linux]

On Tue, Apr 06, 2010 at 05:33:17PM -0700, Michael Bohan wrote:
> On 4/6/2010 3:08 PM, Russell King - ARM Linux wrote:
>> 1. are you enabling ARCH_HAS_HOLES_MEMORYMODEL ?
>>    
>
> Yes, although this does not impact the problem I'm dealing with.  That  
> option is only used for /proc/pagetypeinfo currently.  It would be good  
> if we could consolidate ARCH_HAS_HOLES_MEMORYMODEL and HOLES_IN_ZONE,  
> but that may be out of scope for this change.
>
>> 2. where does it try to access these page structs without trying
>>     pfn_valid() to check whether a page struct exists first?
>>    
>
> The specific piece of code that is causing crashes in my scenario is in  
> vm/page_alloc.c:move_freepages(), called from move_freepages_block().   
> The code in move_freepages_block aligns the end_pfn to the closest page  
> block, which may take us to invalid memmap entries.
>
> The macro that conditionally saves us in this case is the  
> pfn_valid_within(), called from move_freepages().  If HOLES_IN_ZONE is  
> configured, this option calls down to pfn_valid() to make sure the page  
> has a valid memmap entry.  There are likely other cases where this is an  
> issue as well that I haven't run into.

Well, there's two ways to look at this - either you should ensure
that memory is available up to MAX_ORDER_NR_PAGES, possibly reducing
this number of that's necessary to achive this.

Or, we need to have ARCH_HAS_HOLES_MEMORYMODEL needs to select
HOLES_IN_ZONE so we get proper checking of PFNs - maybe conditional on
MSM.  I think most users of ARCH_HAS_HOLES_MEMORYMODEL align memory to
a power-of-two amount of memory, and so MAX_ORDER_NR_PAGES doesn't
cause them a problem.

[PATCH] arm: mm: Don't free prohibited memmap entries

[Michael Bohan]

On 4/12/2010 2:11 PM, Russell King - ARM Linux wrote:
> On Tue, Apr 06, 2010 at 05:33:17PM -0700, Michael Bohan wrote:
>    
>> On 4/6/2010 3:08 PM, Russell King - ARM Linux wrote:
>>      
>>> 1. are you enabling ARCH_HAS_HOLES_MEMORYMODEL ?
>>>
>>>        
>> Yes, although this does not impact the problem I'm dealing with.  That
>> option is only used for /proc/pagetypeinfo currently.  It would be good
>> if we could consolidate ARCH_HAS_HOLES_MEMORYMODEL and HOLES_IN_ZONE,
>> but that may be out of scope for this change.
>>
>>      
>>> 2. where does it try to access these page structs without trying
>>>      pfn_valid() to check whether a page struct exists first?
>>>
>>>        
>> The specific piece of code that is causing crashes in my scenario is in
>> vm/page_alloc.c:move_freepages(), called from move_freepages_block().
>> The code in move_freepages_block aligns the end_pfn to the closest page
>> block, which may take us to invalid memmap entries.
>>
>> The macro that conditionally saves us in this case is the
>> pfn_valid_within(), called from move_freepages().  If HOLES_IN_ZONE is
>> configured, this option calls down to pfn_valid() to make sure the page
>> has a valid memmap entry.  There are likely other cases where this is an
>> issue as well that I haven't run into.
>>      
> Well, there's two ways to look at this - either you should ensure
> that memory is available up to MAX_ORDER_NR_PAGES, possibly reducing
> this number of that's necessary to achive this.
>
> Or, we need to have ARCH_HAS_HOLES_MEMORYMODEL needs to select
> HOLES_IN_ZONE so we get proper checking of PFNs - maybe conditional on
> MSM.  I think most users of ARCH_HAS_HOLES_MEMORYMODEL align memory to
> a power-of-two amount of memory, and so MAX_ORDER_NR_PAGES doesn't
> cause them a problem.
>    

The solution proposed in this patch does not include HOLES_IN_ZONE or 
ARCH_HAS_HOLES_MEMORYMODEL.  I proposed HOLES_IN_ZONE in a previous 
patch, but per Mel's suggestion here we simply don't free any necessary 
memmap entries that are required by the VM subsystem.  This has the 
following benefits:

-No extra run time overhead.
-No extra memory used for platforms with memory hole end addresses 
aligned to MAX_ORDER_NR_PAGES.  This means that there should be no 
impact at all for users who are not currently running into this 
problem.  For users that don't align to MAX_ORDER_NR_PAGES, there would 
be a loss of 8KB, 16KB, or 24KB of memory per memory hole (assuming 
MAX_ORDER == 11), depending on where their hole end address lands.  This 
is somewhat undesirable, but better than losing the full 1MB, 2MB, or 
3MB, respectfully, that would occur to achieve alignment.  Some 
platforms are not flexible with respect to their memory map.

I'd also like to point out that this problem is not limited to MSM.  Any 
platform in the future that changes its memory map in such a fashion 
will likely run into this bug.

Thanks,
Michael