[PATCH] mm: might_sleep warning

[PATCH] mm: might_sleep warning

[Pavel Tatashin]

Robot reported this issue:
https://lkml.org/lkml/2018/2/27/851

That is introduced by:
mm: initialize pages on demand during boot

The problem is caused by changing static branch value within spin lock.
Spin lock disables preemption, and changing static branch value takes
mutex lock in its path, and thus may sleep.

The fix is to add another boolean variable to avoid the need to change
static branch within spinlock.

Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com>
---
 mm/page_alloc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b337a026007c..52edc6695b2b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1579,6 +1579,7 @@ static int __init deferred_init_memmap(void *data)
  * page_alloc_init_late() soon after smp_init() is complete.
  */
 static __initdata DEFINE_SPINLOCK(deferred_zone_grow_lock);
+static bool deferred_zone_grow __initdata = true;
 static DEFINE_STATIC_KEY_TRUE(deferred_pages);
 
 /*
@@ -1616,7 +1617,7 @@ deferred_grow_zone(struct zone *zone, unsigned int order)
 	 * Bail if we raced with another thread that disabled on demand
 	 * initialization.
 	 */
-	if (!static_branch_unlikely(&deferred_pages)) {
+	if (!static_branch_unlikely(&deferred_pages) || !deferred_zone_grow) {
 		spin_unlock_irqrestore(&deferred_zone_grow_lock, flags);
 		return false;
 	}
@@ -1683,10 +1684,15 @@ void __init page_alloc_init_late(void)
 	/*
 	 * We are about to initialize the rest of deferred pages, permanently
 	 * disable on-demand struct page initialization.
+	 *
+	 * Note: it is prohibited to modify static branches in non-preemptible
+	 * context. Since, spin_lock() disables preemption, we must use an
+	 * extra boolean deferred_zone_grow.
 	 */
 	spin_lock(&deferred_zone_grow_lock);
-	static_branch_disable(&deferred_pages);
+	deferred_zone_grow = false;
 	spin_unlock(&deferred_zone_grow_lock);
+	static_branch_disable(&deferred_pages);
 
 	/* There will be num_node_state(N_MEMORY) threads */
 	atomic_set(&pgdat_init_n_undone, num_node_state(N_MEMORY));
-- 
2.16.2

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

Re: [PATCH] mm: might_sleep warning

[Andrew Morton]

On Tue,  6 Mar 2018 14:20:22 -0500 Pavel Tatashin <pasha.tatashin@oracle.com> wrote:

> Robot reported this issue:
> https://lkml.org/lkml/2018/2/27/851
> 
> That is introduced by:
> mm: initialize pages on demand during boot
> 
> The problem is caused by changing static branch value within spin lock.
> Spin lock disables preemption, and changing static branch value takes
> mutex lock in its path, and thus may sleep.
> 
> The fix is to add another boolean variable to avoid the need to change
> static branch within spinlock.
> 
> ...
>
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1579,6 +1579,7 @@ static int __init deferred_init_memmap(void *data)
>   * page_alloc_init_late() soon after smp_init() is complete.
>   */
>  static __initdata DEFINE_SPINLOCK(deferred_zone_grow_lock);
> +static bool deferred_zone_grow __initdata = true;
>  static DEFINE_STATIC_KEY_TRUE(deferred_pages);
>  
>  /*
> @@ -1616,7 +1617,7 @@ deferred_grow_zone(struct zone *zone, unsigned int order)
>  	 * Bail if we raced with another thread that disabled on demand
>  	 * initialization.
>  	 */
> -	if (!static_branch_unlikely(&deferred_pages)) {
> +	if (!static_branch_unlikely(&deferred_pages) || !deferred_zone_grow) {
>  		spin_unlock_irqrestore(&deferred_zone_grow_lock, flags);
>  		return false;
>  	}
> @@ -1683,10 +1684,15 @@ void __init page_alloc_init_late(void)
>  	/*
>  	 * We are about to initialize the rest of deferred pages, permanently
>  	 * disable on-demand struct page initialization.
> +	 *
> +	 * Note: it is prohibited to modify static branches in non-preemptible
> +	 * context. Since, spin_lock() disables preemption, we must use an
> +	 * extra boolean deferred_zone_grow.
>  	 */
>  	spin_lock(&deferred_zone_grow_lock);
> -	static_branch_disable(&deferred_pages);
> +	deferred_zone_grow = false;
>  	spin_unlock(&deferred_zone_grow_lock);
> +	static_branch_disable(&deferred_pages);
>  
>  	/* There will be num_node_state(N_MEMORY) threads */
>  	atomic_set(&pgdat_init_n_undone, num_node_state(N_MEMORY));

Kinda ugly, but I can see the logic behind the decisions.

Can we instead turn deferred_zone_grow_lock into a mutex?

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

Re: [PATCH] mm: might_sleep warning

[Pavel Tatashin]

[-- Attachment #1: Type: text/plain, Size: 2910 bytes --]

[CCed everyone]

Hi Andrew,

I afraid we cannot change this spinlock to mutex
because deferred_grow_zone() might be called from an interrupt context if
interrupt thread needs to allocate memory.

Thank you,
Pavel


On Tue, Mar 6, 2018 at 3:36 PM, Andrew Morton <akpm@linux-foundation.org>
wrote:

> On Tue,  6 Mar 2018 14:20:22 -0500 Pavel Tatashin <
> pasha.tatashin@oracle.com> wrote:
>
> > Robot reported this issue:
> > https://lkml.org/lkml/2018/2/27/851
> >
> > That is introduced by:
> > mm: initialize pages on demand during boot
> >
> > The problem is caused by changing static branch value within spin lock.
> > Spin lock disables preemption, and changing static branch value takes
> > mutex lock in its path, and thus may sleep.
> >
> > The fix is to add another boolean variable to avoid the need to change
> > static branch within spinlock.
> >
> > ...
> >
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1579,6 +1579,7 @@ static int __init deferred_init_memmap(void *data)
> >   * page_alloc_init_late() soon after smp_init() is complete.
> >   */
> >  static __initdata DEFINE_SPINLOCK(deferred_zone_grow_lock);
> > +static bool deferred_zone_grow __initdata = true;
> >  static DEFINE_STATIC_KEY_TRUE(deferred_pages);
> >
> >  /*
> > @@ -1616,7 +1617,7 @@ deferred_grow_zone(struct zone *zone, unsigned int
> order)
> >        * Bail if we raced with another thread that disabled on demand
> >        * initialization.
> >        */
> > -     if (!static_branch_unlikely(&deferred_pages)) {
> > +     if (!static_branch_unlikely(&deferred_pages) ||
> !deferred_zone_grow) {
> >               spin_unlock_irqrestore(&deferred_zone_grow_lock, flags);
> >               return false;
> >       }
> > @@ -1683,10 +1684,15 @@ void __init page_alloc_init_late(void)
> >       /*
> >        * We are about to initialize the rest of deferred pages,
> permanently
> >        * disable on-demand struct page initialization.
> > +      *
> > +      * Note: it is prohibited to modify static branches in
> non-preemptible
> > +      * context. Since, spin_lock() disables preemption, we must use an
> > +      * extra boolean deferred_zone_grow.
> >        */
> >       spin_lock(&deferred_zone_grow_lock);
> > -     static_branch_disable(&deferred_pages);
> > +     deferred_zone_grow = false;
> >       spin_unlock(&deferred_zone_grow_lock);
> > +     static_branch_disable(&deferred_pages);
> >
> >       /* There will be num_node_state(N_MEMORY) threads */
> >       atomic_set(&pgdat_init_n_undone, num_node_state(N_MEMORY));
>
> Kinda ugly, but I can see the logic behind the decisions.
>
> Can we instead turn deferred_zone_grow_lock into a mutex?
>
> --
> 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>
>

[-- Attachment #2: Type: text/html, Size: 7336 bytes --]

Re: [PATCH] mm: might_sleep warning

[Andrew Morton]

On Tue, 6 Mar 2018 15:48:26 -0500 Pavel Tatashin <pasha.tatashin@oracle.com> wrote:

> On Tue, Mar 6, 2018 at 3:36 PM, Andrew Morton <akpm@linux-foundation.org>
> wrote:
> 
> > On Tue,  6 Mar 2018 14:20:22 -0500 Pavel Tatashin <
> > pasha.tatashin@oracle.com> wrote:
> >
> > >       spin_lock(&deferred_zone_grow_lock);
> > > -     static_branch_disable(&deferred_pages);
> > > +     deferred_zone_grow = false;
> > >       spin_unlock(&deferred_zone_grow_lock);
> > > +     static_branch_disable(&deferred_pages);
> > >
> > >       /* There will be num_node_state(N_MEMORY) threads */
> > >       atomic_set(&pgdat_init_n_undone, num_node_state(N_MEMORY));
> >
> > Kinda ugly, but I can see the logic behind the decisions.
> >
> > Can we instead turn deferred_zone_grow_lock into a mutex?

(top-posting repaired.  Please don't top-post).

> [CCed everyone]
> 
> Hi Andrew,
> 
> I afraid we cannot change this spinlock to mutex
> because deferred_grow_zone() might be called from an interrupt context if
> interrupt thread needs to allocate memory.
> 

OK.  But if deferred_grow_zone() can be called from interrupt then
page_alloc_init_late() should be using spin_lock_irq(), shouldn't it? 
I'm surprised that lockdep didn't detect that.


--- a/mm/page_alloc.c~mm-initialize-pages-on-demand-during-boot-fix-4-fix
+++ a/mm/page_alloc.c
@@ -1689,9 +1689,9 @@ void __init page_alloc_init_late(void)
 	 * context. Since, spin_lock() disables preemption, we must use an
 	 * extra boolean deferred_zone_grow.
 	 */
-	spin_lock(&deferred_zone_grow_lock);
+	spin_lock_irq(&deferred_zone_grow_lock);
 	deferred_zone_grow = false;
-	spin_unlock(&deferred_zone_grow_lock);
+	spin_unlock_irq(&deferred_zone_grow_lock);
 	static_branch_disable(&deferred_pages);
 
 	/* There will be num_node_state(N_MEMORY) threads */
_

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

Re: [PATCH] mm: might_sleep warning

[Pavel Tatashin]

> > > >       spin_lock(&deferred_zone_grow_lock);
> > > > -     static_branch_disable(&deferred_pages);
> > > > +     deferred_zone_grow = false;
> > > >       spin_unlock(&deferred_zone_grow_lock);
> > > > +     static_branch_disable(&deferred_pages);
> > > >
> > > >       /* There will be num_node_state(N_MEMORY) threads */
> > > >       atomic_set(&pgdat_init_n_undone, num_node_state(N_MEMORY));
> > >
> > > Kinda ugly, but I can see the logic behind the decisions.
> > >
> > > Can we instead turn deferred_zone_grow_lock into a mutex?
>
> (top-posting repaired.  Please don't top-post).
>
> > [CCed everyone]
> >
> > Hi Andrew,
> >
> > I afraid we cannot change this spinlock to mutex
> > because deferred_grow_zone() might be called from an interrupt context if
> > interrupt thread needs to allocate memory.
> >
>
> OK.  But if deferred_grow_zone() can be called from interrupt then
> page_alloc_init_late() should be using spin_lock_irq(), shouldn't it?
> I'm surprised that lockdep didn't detect that.

No, page_alloc_init_late()  cannot be called from interrupt, it is
called straight from:
kernel_init_freeable(). But, I believe deferred_grow_zone(): can be called:

get_page_from_freelist()
 _deferred_grow_zone()
   deferred_grow_zone()


>
>
>
> --- a/mm/page_alloc.c~mm-initialize-pages-on-demand-during-boot-fix-4-fix
> +++ a/mm/page_alloc.c
> @@ -1689,9 +1689,9 @@ void __init page_alloc_init_late(void)
>          * context. Since, spin_lock() disables preemption, we must use an
>          * extra boolean deferred_zone_grow.
>          */
> -       spin_lock(&deferred_zone_grow_lock);
> +       spin_lock_irq(&deferred_zone_grow_lock);
>         deferred_zone_grow = false;
> -       spin_unlock(&deferred_zone_grow_lock);
> +       spin_unlock_irq(&deferred_zone_grow_lock);
>         static_branch_disable(&deferred_pages);
>
>         /* There will be num_node_state(N_MEMORY) threads */
> _
>
> --
> 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>

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

Re: [PATCH] mm: might_sleep warning

[Andrew Morton]

On Tue, 6 Mar 2018 16:04:06 -0500 Pavel Tatashin <pasha.tatashin@oracle.com> wrote:

> > > > >       spin_lock(&deferred_zone_grow_lock);
> > > > > -     static_branch_disable(&deferred_pages);
> > > > > +     deferred_zone_grow = false;
> > > > >       spin_unlock(&deferred_zone_grow_lock);
> > > > > +     static_branch_disable(&deferred_pages);
> > > > >
> > > > >       /* There will be num_node_state(N_MEMORY) threads */
> > > > >       atomic_set(&pgdat_init_n_undone, num_node_state(N_MEMORY));
> > > >
> > > > Kinda ugly, but I can see the logic behind the decisions.
> > > >
> > > > Can we instead turn deferred_zone_grow_lock into a mutex?
> >
> > (top-posting repaired.  Please don't top-post).
> >
> > > [CCed everyone]
> > >
> > > Hi Andrew,
> > >
> > > I afraid we cannot change this spinlock to mutex
> > > because deferred_grow_zone() might be called from an interrupt context if
> > > interrupt thread needs to allocate memory.
> > >
> >
> > OK.  But if deferred_grow_zone() can be called from interrupt then
> > page_alloc_init_late() should be using spin_lock_irq(), shouldn't it?
> > I'm surprised that lockdep didn't detect that.
> 
> No, page_alloc_init_late()  cannot be called from interrupt, it is
> called straight from:
> kernel_init_freeable(). But, I believe deferred_grow_zone(): can be called:
> 
> get_page_from_freelist()
>  _deferred_grow_zone()
>    deferred_grow_zone()

That's why page_alloc_init_late() needs spin_lock_irq().  If a CPU is
holding deferred_zone_grow_lock with enabled interrupts and an
interrupt comes in on that CPU and the CPU runs deferred_grow_zone() in
its interrupt handler, we deadlock.

lockdep knows about this bug and should have reported it.

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

Re: [PATCH] mm: might_sleep warning

[Pavel Tatashin]

> That's why page_alloc_init_late() needs spin_lock_irq().  If a CPU is
> holding deferred_zone_grow_lock with enabled interrupts and an
> interrupt comes in on that CPU and the CPU runs deferred_grow_zone() in
> its interrupt handler, we deadlock.
> 
> lockdep knows about this bug and should have reported it.
> 

I see what you are saying. Yes you are correct, we need spin_lock_irq() 
in page_alloc_init_late(). I will update the patch. I am not sure why 
lockdep has not reported it. May be it is initialized after this code is 
executed?

Thank you,
Pavel

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