On Wed, 8 Jun 2011 12:15:11 +0200
Johannes Weiner [off-list ref] wrote:

On Wed, Jun 08, 2011 at 05:45:05PM +0900, KAMEZAWA Hiroyuki wrote:

quoted

@@ -196,7 +195,11 @@ static int __meminit init_section_page_cgroup(unsigned long pfn)
 		pc = base + index;
 		init_page_cgroup(pc, nr);
 	}
-
+	/*
+	 * Even if passed 'pfn' is not aligned to section, we need to align
+	 * it to section boundary because of SPARSEMEM pfn calculation.
+	 */
+	pfn = pfn & ~(PAGES_PER_SECTION - 1);

PAGE_SECTION_MASK?

will use it.

quoted

 	section->page_cgroup = base - pfn;
 	total_usage += table_size;
 	return 0;

@@ -228,7 +231,7 @@ int __meminit online_page_cgroup(unsigned long start_pfn,
 	for (pfn = start; !fail && pfn < end; pfn += PAGES_PER_SECTION) {
 		if (!pfn_present(pfn))
 			continue;
-		fail = init_section_page_cgroup(pfn);
+		fail = init_section_page_cgroup(pfn, nid);

AFAICS, nid can be -1 in the hotplug callbacks when there is a new
section added to a node that already has memory, and then the
allocation will fall back to numa_node_id().

Ah, thank you for pointing out.

So I think we either need to trust start_pfn has valid mem map backing
it (ARM has no memory hotplug support) and use pfn_to_nid(start_pfn),
or find another way to the right node, no?

memory hotplug itself does nid = page_to_nid(pfn_to_page(pfn))..
so we can assume memmap of start_pfn will be valid..

quoted

@@ -285,14 +288,36 @@ void __init page_cgroup_init(void)
 {
 	unsigned long pfn;
 	int fail = 0;
+	int nid;
 
 	if (mem_cgroup_disabled())
 		return;
 
-	for (pfn = 0; !fail && pfn < max_pfn; pfn += PAGES_PER_SECTION) {
-		if (!pfn_present(pfn))
-			continue;
-		fail = init_section_page_cgroup(pfn);
+	for_each_node_state(nid, N_HIGH_MEMORY) {
+		unsigned long start_pfn, end_pfn;
+
+		start_pfn = NODE_DATA(nid)->node_start_pfn;
+		end_pfn = start_pfn + NODE_DATA(nid)->node_spanned_pages;
+		/*
+		 * Because we cannot trust page->flags of page out of node
+		 * boundary, we skip pfn < start_pfn.
+		 */
+		for (pfn = start_pfn;
+		     !fail && (pfn < end_pfn);
+		     pfn = ALIGN(pfn + 1, PAGES_PER_SECTION)) {

If we don't bother to align the pfn on the first iteration, I don't
think we should for subsequent iterations.  init_section_page_cgroup()
has to be able to cope anyway.  How about

	pfn += PAGES_PER_SECTION

instead?

I thought of that but it means (pfn < end_pfn) goes wrong.
If pfn is not aligned.

                   pfn-------->end_pfn----------->pfn+PAGES_PER_SECTION

pages in [pfn..end_pfn) will not be handled. But I'd like to think about
this in the next version.

Thank you for review.

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>