| Message ID | 1350956091-5276-1-git-send-email-spang@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <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
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); + } } /*
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(-)