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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Apr 2017 09:23:29 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Reza Arbab <arbab@...ux.vnet.ibm.com>,
        Gary Hade <garyhade@...ibm.com>
Cc:     linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        Mel Gorman <mgorman@...e.de>, Vlastimil Babka <vbabka@...e.cz>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Yasuaki Ishimatsu <yasu.isimatu@...il.com>,
        Tang Chen <tangchen@...fujitsu.com>, qiuxishi@...wei.com,
        Kani Toshimitsu <toshi.kani@....com>, slaoub@...il.com,
        Joonsoo Kim <js1304@...il.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Zhang Zhen <zhenzhang.zhang@...wei.com>,
        David Rientjes <rientjes@...gle.com>,
        Daniel Kiper <daniel.kiper@...cle.com>,
        Igor Mammedov <imammedo@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Chris Metcalf <cmetcalf@...lanox.com>,
        Dan Williams <dan.j.williams@...il.com>,
        Heiko Carstens <heiko.carstens@...ibm.com>,
        Lai Jiangshan <laijs@...fujitsu.com>,
        Martin Schwidefsky <schwidefsky@...ibm.com>
Subject: Re: [PATCH 0/6] mm: make movable onlining suck less

[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