[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
