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

[Michal Hocko]

[Let's add Gary who as introduced this code c04fc586c1a48]

On Mon 03-04-17 15:42:13, Reza Arbab wrote:
> On Mon, Apr 03, 2017 at 10:23:38PM +0200, Michal Hocko wrote:
> >On Mon 03-04-17 14:58:30, Reza Arbab wrote:
> >>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));
> 
> Almost there. I'm seeing the memory in the correct node now, but the
> /sys/devices/system/node/nodeX/memoryY links are not being created.
> 
> I think it's tripping up here, in register_mem_sect_under_node():
> 
> 		page_nid = get_nid_for_pfn(pfn);
> 		if (page_nid < 0)
> 			continue;

Huh, this code is confusing. How can we have a memblock spanning more
nodes? If not then the loop over all sections in the memblock seem
pointless as well.  Also why do we require page_initialized() in
get_nid_for_pfn? The changelog doesn't explain that and there are no
comments that would help either.

Gary, could you clarify this please?
-- 
Michal Hocko
SUSE Labs