[PATCH] ARM: Fix page counting in mem_init and show_mem

[PATCH] ARM: Fix page counting in mem_init and show_mem

[Michael Spang]

The code in mem_init & show_mem to count page usage has two issues:

1. It assumes the memory map for a bank is contiguous. The sparsemem
   memory model partitions the memory map into sections, which may not
   be contiguous. They are usually contiguous due only to allocation
   order. Avoid this by using pfn_to_page for each page.

   If the memory map is not contiguous the pointer math works out
   badly and crashes the system.

2. A memory bank may have holes. Some regions may be removed using
   memblock_remove, and will not have valid page stucts. The code
   should not access the page structs for such pages. Avoid this by
   skipping pages that fail pfn_valid().

   If the memory map has holes, the free & total page counts are
   wrong.

Signed-off-by: Michael Spang <spang@chromium.org>
---
 arch/arm/mm/init.c |   40 ++++++++++++++++++++++------------------
 1 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index c21d06c..97d811a 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -101,16 +101,19 @@ void show_mem(unsigned int filter)
 
 	for_each_bank (i, mi) {
 		struct membank *bank = &mi->bank[i];
-		unsigned int pfn1, pfn2;
-		struct page *page, *end;
+		unsigned int start, end, pfn;
 
-		pfn1 = bank_pfn_start(bank);
-		pfn2 = bank_pfn_end(bank);
+		start = bank_pfn_start(bank);
+		end = bank_pfn_end(bank);
 
-		page = pfn_to_page(pfn1);
-		end  = pfn_to_page(pfn2 - 1) + 1;
+		for (pfn = start; pfn < end; pfn++) {
+			struct page *page;
+
+			if (!pfn_valid(pfn))
+				continue;
+
+			page = pfn_to_page(pfn);
 
-		do {
 			total++;
 			if (PageReserved(page))
 				reserved++;
@@ -122,8 +125,7 @@ void show_mem(unsigned int filter)
 				free++;
 			else
 				shared += page_count(page) - 1;
-			page++;
-		} while (page < end);
+		}
 	}
 
 	printk("%d pages of RAM\n", total);
@@ -619,22 +621,24 @@ void __init mem_init(void)
 
 	for_each_bank(i, &meminfo) {
 		struct membank *bank = &meminfo.bank[i];
-		unsigned int pfn1, pfn2;
-		struct page *page, *end;
+		unsigned int start, end, pfn;
 
-		pfn1 = bank_pfn_start(bank);
-		pfn2 = bank_pfn_end(bank);
+		start = bank_pfn_start(bank);
+		end = bank_pfn_end(bank);
 
-		page = pfn_to_page(pfn1);
-		end  = pfn_to_page(pfn2 - 1) + 1;
+		for (pfn = start; pfn < end; pfn++) {
+			struct page *page;
+
+			if (!pfn_valid(pfn))
+				continue;
+
+			page = pfn_to_page(pfn);
 
-		do {
 			if (PageReserved(page))
 				reserved_pages++;
 			else if (!page_count(page))
 				free_pages++;
-			page++;
-		} while (page < end);
+		}
 	}
 
 	/*
-- 
1.7.7.3

[PATCH] ARM: Fix page counting in mem_init and show_mem

[Russell King - ARM Linux]

On Mon, Oct 22, 2012 at 09:34:51PM -0400, Michael Spang wrote:
>  	for_each_bank (i, mi) {
>  		struct membank *bank = &mi->bank[i];
> -		unsigned int pfn1, pfn2;
> -		struct page *page, *end;
> +		unsigned int start, end, pfn;
>  
> -		pfn1 = bank_pfn_start(bank);
> -		pfn2 = bank_pfn_end(bank);
> +		start = bank_pfn_start(bank);
> +		end = bank_pfn_end(bank);
>  
> -		page = pfn_to_page(pfn1);
> -		end  = pfn_to_page(pfn2 - 1) + 1;
> +		for (pfn = start; pfn < end; pfn++) {
> +			struct page *page;
> +
> +			if (!pfn_valid(pfn))
> +				continue;

This is not a very good fix; what this means is that we end up calling
pfn_valid() for each and every page in the system, and as pfn_valid()
may not be a simple test (but a search) we should avoid that when we're
iterating over all pages in the system.

Firstly, the mem blank information is assumed from the very beginning
to be aligned with the sparsemem split-up.  This comes from the previous
discontiguous implementation where this was an absolute requirement.  We
continue to require that.

Secondly, if you're worred about the stolen memory, then we need to be
iterating over the memblock information instead of the membank information.
This is slightly more complex because memblock will merge neighbouring
regions into one contiguous entry - and this needs to be split up here.
This is why I persisted with the membank stuff here as that _should_
already be appropriately split.

In the long run though, moving to memblock and dealing better with the
split memory maps (rather than looking up each and every page using
pfn_to_page()) is the right way to go.

[PATCH] ARM: Fix page counting in mem_init and show_mem

[Michael Spang]

On Thu, Nov 29, 2012 at 8:08 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Oct 22, 2012 at 09:34:51PM -0400, Michael Spang wrote:
>>       for_each_bank (i, mi) {
>>               struct membank *bank = &mi->bank[i];
>> -             unsigned int pfn1, pfn2;
>> -             struct page *page, *end;
>> +             unsigned int start, end, pfn;
>>
>> -             pfn1 = bank_pfn_start(bank);
>> -             pfn2 = bank_pfn_end(bank);
>> +             start = bank_pfn_start(bank);
>> +             end = bank_pfn_end(bank);
>>
>> -             page = pfn_to_page(pfn1);
>> -             end  = pfn_to_page(pfn2 - 1) + 1;
>> +             for (pfn = start; pfn < end; pfn++) {
>> +                     struct page *page;
>> +
>> +                     if (!pfn_valid(pfn))
>> +                             continue;
>
> This is not a very good fix; what this means is that we end up calling
> pfn_valid() for each and every page in the system, and as pfn_valid()
> may not be a simple test (but a search) we should avoid that when we're
> iterating over all pages in the system.
>
> Firstly, the mem blank information is assumed from the very beginning
> to be aligned with the sparsemem split-up.  This comes from the previous
> discontiguous implementation where this was an absolute requirement.  We
> continue to require that.

Little confused here.

On my system, there are 2 membanks and 8 sparsemem sections.
Obviously, the banks have been further divided into sections by
sparsemem. My problem occurs because this code assumes there's a
single struct page array for the whole bank, when really there are
multiple.

Each struct page array is allocated in a separate call to bootmem.
It's disastrous if bootmem can't allocate them contiguously. This
happens on one of my devices with certain kernel options.

>
> Secondly, if you're worred about the stolen memory, then we need to be
> iterating over the memblock information instead of the membank information.
> This is slightly more complex because memblock will merge neighbouring
> regions into one contiguous entry - and this needs to be split up here.
> This is why I persisted with the membank stuff here as that _should_
> already be appropriately split.
>
> In the long run though, moving to memblock and dealing better with the
> split memory maps (rather than looking up each and every page using
> pfn_to_page()) is the right way to go.

Thanks,
Michael