Re: [PATCH 0/6] mm: make movable onlining suck less

[Michal Hocko]

On Mon 03-04-17 14:58:30, Reza Arbab wrote:
> On Mon, Apr 03, 2017 at 01:55:46PM +0200, Michal Hocko wrote:
> >Anyting? I would really appreciate a feedback from IBM and Futjitsu guys
> >who have shaped this code last few years. Also Igor and Vitaly seem to be
> >using memory hotplug in virtualized environments. I do not expect they
> >would see a huge advantage of the rework but I would appreciate to give it
> >some testing to catch any potential regressions.
> 
> Sorry for the delayed reply.
> 
> With this set, I'm able to "online_movable" blocks in ascending order.
> 
> However, I am seeing a regression. When adding memory to a memoryless node,
> it shows up in node 0 instead. I'm digging to see if I can help narrow down
> where things go wrong.

OK, I guess I know what is going on here. online_pages relies on
pfn_to_nid(pfn) to return a proper node. But we are doing
page_to_nid(pfn_to_page(__pfn_to_nid_pfn)) so we rely on the page being
properly initialized. Damn, I should have noticed that. There are two
ways around that. Either the __add_section stores the nid into the
struct page and make page_to_nid reliable or store it somewhere else
(ideally into the memblock). The first is easier (let's do it for now)
but longterm we do not want to rely on the struct page at all I believe.

Does the following help?
---
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index b9dc1c4e26c3..0e21b9f67c9d 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -309,14 +309,19 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn)
 
 	/*
 	 * Make all the pages reserved so that nobody will stumble over half
-	 * initialized state.
+	 * initialized state. 
+	 * FIXME: We also have to associate it with a node because pfn_to_node
+	 * relies on having page with the proper node.
 	 */
 	for (i = 0; i < PAGES_PER_SECTION; i++) {
 		unsigned long pfn = phys_start_pfn + i;
+		struct page *page;
 		if (!pfn_valid(pfn))
 			continue;
 
-		SetPageReserved(pfn_to_page(phys_start_pfn + i));
+		page = pfn_to_page(pfn);
+		set_page_node(page, nid);
+		SetPageReserved(page);
 	}
 
 	return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
-- 
Michal Hocko
SUSE Labs