RE: Regarding patch "mm: meminit: make __early_pfn_to_nid SMP-safe and introduce meminit_pfn_in_nid"

RE: Regarding patch "mm: meminit: make __early_pfn_to_nid SMP-safe and introduce meminit_pfn_in_nid"

[Alex Ng (LIS)]

> -----Original Message-----
> From: Mel Gorman [mailto:mgorman@suse.de]
> Sent: Wednesday, July 15, 2015 7:51 AM
> To: Waiman Long <waiman.long@hp.com>
> Cc: Alex Ng (LIS) <alexng@microsoft.com>; Robin Holt
> <robinmholt@gmail.com>; Nate Zimmer <nzimmer@sgi.com>; Dave Hansen
> <dave.hansen@intel.com>; Scott Norton <scott.norton@hp.com>; Luck,
> Tony <tony.luck@intel.com>; Ingo Molnar <mingo@elte.hu>; H. Peter Anvin
> <hpa@zytor.com>; Thomas Gleixner <tglx@linutronix.de>
> Subject: Re: Regarding patch "mm: meminit: make __early_pfn_to_nid SMP-
> safe and introduce meminit_pfn_in_nid"
> 
> On Wed, Jul 15, 2015 at 10:07:00AM -0400, Waiman Long wrote:
> > On 07/15/2015 09:40 AM, Mel Gorman wrote:
> > >On Wed, Jul 15, 2015 at 01:02:34AM +0000, Alex Ng (LIS) wrote:
> > >>Hello,
> > >>
> > >>Just wanted to bring to your attention an issue regarding commit
> 8a942fdea560d4ac0e9d9fabcd5201ad20e0c382<https://git.kernel.org/cgit/lin
> ux/kernel/git/torvalds/linux.git/commit/mm/page_alloc.c?id=8a942fdea560
> d4ac0e9d9fabcd5201ad20e0c382>. After this commit, calls to
> early_pfn_to_nid() are resulting in panic due to checking whether system is
> in early boot state. Such calls are happening after system has booted; and
> were working before this commit.
> > >>
> > >>One such case is in memory hot plug scenario; which results in below call
> trace. The function vmemmap_verify() calls early_pfn_to_nid() after system
> has booted. Should these calls be updated to use the SMP-safe version?
> > >>
> > >Yes but if there is more than one I missed then it's awkward. I missed the
> > >hotplug case when making early_pfn_to_nid() crash if it was not during
> > >boot. I suspect that hotplug just happened to work because it's under a
> > >giant mutex and cannot race.
> > >
> > >I didn't even build test this but can you try it? It makes early_pfn_to_nid()
> > >usable when the system is up and running by using a lock once races are
> > >possible.
> > >
> > >
> > >diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > >index 94e2599830c2..91dd0c78a427 100644
> > >--- a/mm/page_alloc.c
> > >+++ b/mm/page_alloc.c
> > >@@ -982,21 +982,26 @@ static void __init
> __free_pages_boot_core(struct page *page,
> > >
> > >  #if defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID) || \
> > >  	defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP)
> > >-/* Only safe to use early in boot when initialisation is single-threaded */
> > >+
> > >  static struct mminit_pfnnid_cache early_pfnnid_cache __meminitdata;
> > >+DEFINE_SPINLOCK(early_pfn_lock);
> >
> > I think the lock should be static and preferably moved inside
> > early_pfn_to_nid() if it is the only function that uses it.
> 
> Agreed. Lets hear first if it addresses Alex problem first and if so,
> I'll apply your feedback and put a changelog on it. There are now at least
> three follow-on patches that I'm waiting for feedback on.
> 
> --
> Mel Gorman
> SUSE Labs

Tried this patch and it addresses the hotplug issue. It's no longer crashing.

Thanks,
Alex Ng

Re: Regarding patch "mm: meminit: make __early_pfn_to_nid SMP-safe and introduce meminit_pfn_in_nid"

[Mel Gorman]

On Wed, Jul 15, 2015 at 10:45:11PM +0000, Alex Ng (LIS) wrote:
> > Agreed. Lets hear first if it addresses Alex problem first and if so,
> > I'll apply your feedback and put a changelog on it. There are now at least
> > three follow-on patches that I'm waiting for feedback on.
> > 
> > --
> > Mel Gorman
> > SUSE Labs
> 
> Tried this patch and it addresses the hotplug issue. It's no longer crashing.
> 

Excellent, thanks for that Alex. I've taken the liberty of adding the
following.

Reported-and-tested-by: Alex Ng <alexng@microsoft.com>

I'm hoping to get feedback on another two meminit patch and do all the
fixes at once but if it's still quiet tomorrow then I'll send them all on
to Andrew.

-- 
Mel Gorman
SUSE Labs