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 <[email protected]>
---
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
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.
On Thu, Nov 29, 2012 at 8:08 AM, Russell King - ARM Linux
<[email protected]> 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