Re: [PATCH 3/3] mm, meminit: Allow early_pfn_to_nid to be used during runtime

[Mel Gorman <mgorman@suse.de>]

On Fri, Jul 17, 2015 at 03:29:22PM +0200, Peter Zijlstra wrote:
> On Fri, Jul 17, 2015 at 02:17:29PM +0100, Mel Gorman wrote:
> > On Fri, Jul 17, 2015 at 03:12:32PM +0200, Peter Zijlstra wrote:
> > > On Fri, Jul 17, 2015 at 01:22:04PM +0100, Mel Gorman wrote:
> > > >  int __meminit early_pfn_to_nid(unsigned long pfn)
> > > >  {
> > > > +	static DEFINE_SPINLOCK(early_pfn_lock);
> > > >  	int nid;
> > > >  
> > > > -	/* The system will behave unpredictably otherwise */
> > > > -	BUG_ON(system_state != SYSTEM_BOOTING);
> > > > +	/* Avoid locking overhead during boot but hotplug must lock */
> > > > +	if (system_state != SYSTEM_BOOTING)
> > > > +		spin_lock(&early_pfn_lock);
> > > >  
> > > >  	nid = __early_pfn_to_nid(pfn, &early_pfnnid_cache);
> > > > -	if (nid >= 0)
> > > > -		return nid;
> > > > -	/* just returns 0 */
> > > > -	return 0;
> > > > +	if (nid < 0)
> > > > +		nid = 0;
> > > > +
> > > > +	if (system_state != SYSTEM_BOOTING)
> > > > +		spin_unlock(&early_pfn_lock);
> > > > +
> > > > +	return nid;
> > > >  }
> > > 
> > > Why the conditional locking?
> > 
> > Unnecessary during boot when it's inherently serialised. The point of
> > the deferred initialisation was to boot as quickly as possible.
> 
> Sure, but does it make a measurable difference?

I'm don't know and no longer have access to the necessary machine to test
any more. You make a reasonable point and I would be surprised if it was
noticable. On the other hand, conditional locking is evil and the patch
reflected my thinking at the time "we don't need locks during boot". It's
the type of thinking that should be backed with figures if it was to be
used at all so lets go with;

---8<---
mm, meminit: Allow early_pfn_to_nid to be used during runtime v2

early_pfn_to_nid historically was inherently not SMP safe but only
used during boot which is inherently single threaded or during hotplug
which is protected by a giant mutex. With deferred memory initialisation
there was a thread-safe version introduced and the early_pfn_to_nid
would trigger a BUG_ON if used unsafely. Memory hotplug hit that check.
This patch makes early_pfn_to_nid introduces a lock to make it safe to
use during hotplug.

Reported-and-tested-by: Alex Ng <alexng@microsoft.com>
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 mm/page_alloc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 94e2599830c2..93316f3bcecb 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -982,21 +982,21 @@ 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;
 
 int __meminit early_pfn_to_nid(unsigned long pfn)
 {
+	static DEFINE_SPINLOCK(early_pfn_lock);
 	int nid;
 
-	/* The system will behave unpredictably otherwise */
-	BUG_ON(system_state != SYSTEM_BOOTING);
-
+	spin_lock(&early_pfn_lock);
 	nid = __early_pfn_to_nid(pfn, &early_pfnnid_cache);
-	if (nid >= 0)
-		return nid;
-	/* just returns 0 */
-	return 0;
+	if (nid < 0)
+		nid = 0;
+	spin_unlock(&early_pfn_lock);
+
+	return nid;
 }
 #endif
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>