On Tue, 7 Jun 2011 10:03:13 +0100
Mel Gorman <mgorman@xxxxxxx> wrote:
> On Tue, Jun 07, 2011 at 09:57:08AM +0900, KAMEZAWA Hiroyuki wrote:
> > On Mon, 6 Jun 2011 14:45:19 -0700
> > Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > > Hopefully he can test this one for us as well, thanks.
> > >
> >
> > A patch with better description (of mine) is here.
> > Anyway, I felt I needed a fix for ARM special case.
> >
> > ==
> > fix-init-page_cgroup-for-sparsemem-taking-care-of-broken-page-flags.patch
> > Even with SPARSEMEM, there are some magical memmap.
> >
>
> Who wants to introduce SPARSEMEM_MAGICAL?
>
ARM guys ;)
> > If a Node is not aligned to SECTION, memmap of pfn which is out of
> > Node's range is not initialized. And page->flags contains 0.
> >
>
> This is tangential but it might be worth introducing
> CONFIG_DEBUG_MEMORY_MODEL that WARN_ONs page->flag == 0 in
> pfn_to_page() to catch some accesses outside node boundaries. Not for
> this bug though.
>
Hmm, buf if zone == 0 && section == 0 && nid == 0, page->flags is 0.
> > If Node(0) doesn't exist, NODE_DATA(pfn_to_nid(pfn)) causes error.
> >
>
> Well, not in itself. It causes a bug when we try allocate memory
> from node 0 but there is a subtle performance bug here as well. For
> unaligned nodes, the cgroup information can be allocated from node
> 0 instead of node-local.
>
> > In another case, for example, ARM frees memmap which is never be used
> > even under SPARSEMEM. In that case, page->flags will contain broken
> > value.
> >
>
> Again, not as such. In that case, struct page is not valid memory
> at all.
Hmm, IIUC, ARM's code frees memmap by free_bootmem().....so, memory used
for 'struct page' is valid and can access (but it's not struct page.)
If my English sounds strange, I'm sorry. Hm
How about this ?
==
In another case, for example, ARM frees memmap which is never be used
and reuse memory for memmap for other purpose. So, in that case,
a page got by pfn_to_page(pfn) may not a struct page.
==
>
> > This patch does a strict check on nid which is obtained by
> > pfn_to_page() and use proper NID for page_cgroup allocation.
> >
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> >
> > ---
> > mm/page_cgroup.c | 36 +++++++++++++++++++++++++++++++++++-
> > 1 file changed, 35 insertions(+), 1 deletion(-)
> >
> > Index: linux-3.0-rc1/mm/page_cgroup.c
> > ===================================================================
> > --- linux-3.0-rc1.orig/mm/page_cgroup.c
> > +++ linux-3.0-rc1/mm/page_cgroup.c
> > @@ -168,6 +168,7 @@ static int __meminit init_section_page_c
> > struct mem_section *section;
> > unsigned long table_size;
> > unsigned long nr;
> > + unsigned long tmp;
> > int nid, index;
> >
> > nr = pfn_to_section_nr(pfn);
> > @@ -175,8 +176,41 @@ static int __meminit init_section_page_c
> >
> > if (section->page_cgroup)
> > return 0;
> > + /*
> > + * check Node-ID. Because we get 'pfn' which is obtained by calculation,
> > + * the pfn may "not exist" or "alreay freed". Even if pfn_valid() returns
> > + * true, page->flags may contain broken value and pfn_to_nid() returns
> > + * bad value.
> > + * (See CONFIG_ARCH_HAS_HOLES_MEMORYMODEL and ARM's free_memmap())
> > + * So, we need to do careful check, here.
> > + */
>
> You don't really need to worry about ARM here as long as you stay
> within node boundaries and you only care about the first valid page
> in the node. Why not lookup NODE_DATA(nid) and make sure start and
> end are within the node boundaries?
>
I thought ARM's code just takes care of MAX_ORDER alignment..and doesn't
take care of making holes in a zone/node. Am I wrong ?
== arch/arm/mm/init.c===
for_each_bank(i, mi) {
struct membank *bank = &mi->bank[i];
bank_start = bank_pfn_start(bank);
#ifdef CONFIG_SPARSEMEM
/*
* Take care not to free memmap entries that don't exist
* due to SPARSEMEM sections which aren't present.
*/
bank_start = min(bank_start,
ALIGN(prev_bank_end, PAGES_PER_SECTION));
#endif
/*
* 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)
free_memmap(prev_bank_end, bank_start);
/*
* 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);
}
===
ARM frees memmap for holes between valid memory bank.
Do you mean this one "memory bank" represents a node finally ?
Thanks,
-Kame
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>