Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1186778 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/3] mm, meminit: Allow early_pfn_to_nid to be used during runtime Mel Gorman <mgorman@suse.de> - 2015-07-17 14:30 +0200
    Re: [PATCH 3/3] mm, meminit: Allow early_pfn_to_nid to be used  during runtime Mel Gorman <mgorman@suse.de> - 2015-07-17 15:20 +0200
      Re: [PATCH 3/3] mm, meminit: Allow early_pfn_to_nid to be used  during runtime Peter Zijlstra <peterz@infradead.org> - 2015-07-17 15:30 +0200
        Re: [PATCH 3/3] mm, meminit: Allow early_pfn_to_nid to be used  during runtime Mel Gorman <mgorman@suse.de> - 2015-07-17 15:40 +0200
          Re: [PATCH 3/3] mm, meminit: Allow early_pfn_to_nid to be used  during runtime Peter Zijlstra <peterz@infradead.org> - 2015-07-17 16:00 +0200
    Re: [PATCH 3/3] mm, meminit: Allow early_pfn_to_nid to be used  during runtime Peter Zijlstra <peterz@infradead.org> - 2015-07-17 15:20 +0200

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

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 | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 94e2599830c2..f1e841c67b7a 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;
 
 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;
 }
 #endif
 
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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.

-- 
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Fri, Jul 17, 2015 at 02:39:13PM +0100, Mel Gorman wrote:

> 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;

Last time I tested it, an uncontended spinlock (cache hot) ran around 20
cycles, the unlock is a regular store (x86) and in single digit cycles.
I doubt modern hardware makes it go slower.

> ---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>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web