On Tue, 7 Jun 2011 10:03:13 +0100
Mel Gorman [off-list ref] wrote:

On Tue, Jun 07, 2011 at 09:57:08AM +0900, KAMEZAWA Hiroyuki wrote:

quoted

On Mon, 6 Jun 2011 14:45:19 -0700
Andrew Morton [off-list ref] wrote:

quoted

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 ;)

quoted

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.

quoted

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.

quoted

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.
==

quoted

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 <redacted>

---
 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@kvack.org.  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@kvack.org"> email@kvack.org </a>