[patch V2 0/2] mm/memory_hotplug: Cure potential deadlocks vs. cpu hotplug lock

[patch V2 0/2] mm/memory_hotplug: Cure potential deadlocks vs. cpu hotplug lock

[Thomas Gleixner]

Andrey reported a potential deadlock with the memory hotplug lock and the
cpu hotplug lock.

The following series addresses this by reworking the memory hotplug locking
and fixing up the potential deadlock scenarios.

Applies against Linus head. All preliminaries are merged there already

Thanks,

	tglx
---
 include/linux/swap.h |    1 
 mm/memory_hotplug.c  |   89 ++++++++-------------------------------------------
 mm/page_alloc.c      |    2 -
 mm/swap.c            |   11 ++++--
 4 files changed, 25 insertions(+), 78 deletions(-)



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

[patch V2 1/2] mm: swap: Provide lru_add_drain_all_cpuslocked()

[Thomas Gleixner]

[-- Attachment #1: mm--swap--Provide-lru_add_drain_cpuslocked--.patch --]
[-- Type: text/plain, Size: 2501 bytes --]

The rework of the cpu hotplug locking unearthed potential deadlocks with
the memory hotplug locking code.

The solution for these is to rework the memory hotplug locking code as well
and take the cpu hotplug lock before the memory hotplug lock in
mem_hotplug_begin(), but this will cause a recursive locking of the cpu
hotplug lock when the memory hotplug code calls lru_add_drain_all().

Split out the inner workings of lru_add_drain_all() into
lru_add_drain_all_cpuslocked() so this function can be invoked from the
memory hotplug code with the cpu hotplug lock held.

Reported-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: linux-mm@kvack.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
---
 include/linux/swap.h |    1 +
 mm/swap.c            |   11 ++++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -277,6 +277,7 @@ extern void mark_page_accessed(struct pa
 extern void lru_add_drain(void);
 extern void lru_add_drain_cpu(int cpu);
 extern void lru_add_drain_all(void);
+extern void lru_add_drain_all_cpuslocked(void);
 extern void rotate_reclaimable_page(struct page *page);
 extern void deactivate_file_page(struct page *page);
 extern void mark_page_lazyfree(struct page *page);
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -687,7 +687,7 @@ static void lru_add_drain_per_cpu(struct
 
 static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
 
-void lru_add_drain_all(void)
+void lru_add_drain_all_cpuslocked(void)
 {
 	static DEFINE_MUTEX(lock);
 	static struct cpumask has_work;
@@ -701,7 +701,6 @@ void lru_add_drain_all(void)
 		return;
 
 	mutex_lock(&lock);
-	get_online_cpus();
 	cpumask_clear(&has_work);
 
 	for_each_online_cpu(cpu) {
@@ -721,10 +720,16 @@ void lru_add_drain_all(void)
 	for_each_cpu(cpu, &has_work)
 		flush_work(&per_cpu(lru_add_drain_work, cpu));
 
-	put_online_cpus();
 	mutex_unlock(&lock);
 }
 
+void lru_add_drain_all(void)
+{
+	get_online_cpus();
+	lru_add_drain_all_cpuslocked();
+	put_online_cpus();
+}
+
 /**
  * release_pages - batched put_page()
  * @pages: array of pages to release


--
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 V2 1/2] mm: swap: Provide lru_add_drain_all_cpuslocked()

[Michal Hocko]

On Tue 04-07-17 11:32:33, Thomas Gleixner wrote:
> The rework of the cpu hotplug locking unearthed potential deadlocks with
> the memory hotplug locking code.
> 
> The solution for these is to rework the memory hotplug locking code as well
> and take the cpu hotplug lock before the memory hotplug lock in
> mem_hotplug_begin(), but this will cause a recursive locking of the cpu
> hotplug lock when the memory hotplug code calls lru_add_drain_all().
> 
> Split out the inner workings of lru_add_drain_all() into
> lru_add_drain_all_cpuslocked() so this function can be invoked from the
> memory hotplug code with the cpu hotplug lock held.

You have added callers in the later patch in the series AFAICS which
is OK but I think it would be better to have them in this patch
already. Nothing earth shattering (maybe a rebase artifact).

> Reported-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: linux-mm@kvack.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  include/linux/swap.h |    1 +
>  mm/swap.c            |   11 ++++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -277,6 +277,7 @@ extern void mark_page_accessed(struct pa
>  extern void lru_add_drain(void);
>  extern void lru_add_drain_cpu(int cpu);
>  extern void lru_add_drain_all(void);
> +extern void lru_add_drain_all_cpuslocked(void);
>  extern void rotate_reclaimable_page(struct page *page);
>  extern void deactivate_file_page(struct page *page);
>  extern void mark_page_lazyfree(struct page *page);
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -687,7 +687,7 @@ static void lru_add_drain_per_cpu(struct
>  
>  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
>  
> -void lru_add_drain_all(void)
> +void lru_add_drain_all_cpuslocked(void)
>  {
>  	static DEFINE_MUTEX(lock);
>  	static struct cpumask has_work;
> @@ -701,7 +701,6 @@ void lru_add_drain_all(void)
>  		return;
>  
>  	mutex_lock(&lock);
> -	get_online_cpus();
>  	cpumask_clear(&has_work);
>  
>  	for_each_online_cpu(cpu) {
> @@ -721,10 +720,16 @@ void lru_add_drain_all(void)
>  	for_each_cpu(cpu, &has_work)
>  		flush_work(&per_cpu(lru_add_drain_work, cpu));
>  
> -	put_online_cpus();
>  	mutex_unlock(&lock);
>  }
>  
> +void lru_add_drain_all(void)
> +{
> +	get_online_cpus();
> +	lru_add_drain_all_cpuslocked();
> +	put_online_cpus();
> +}
> +
>  /**
>   * release_pages - batched put_page()
>   * @pages: array of pages to release
> 

-- 
Michal Hocko
SUSE Labs

--
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 V2 1/2] mm: swap: Provide lru_add_drain_all_cpuslocked()

[Thomas Gleixner]

On Tue, 4 Jul 2017, Michal Hocko wrote:
> On Tue 04-07-17 11:32:33, Thomas Gleixner wrote:
> > The rework of the cpu hotplug locking unearthed potential deadlocks with
> > the memory hotplug locking code.
> > 
> > The solution for these is to rework the memory hotplug locking code as well
> > and take the cpu hotplug lock before the memory hotplug lock in
> > mem_hotplug_begin(), but this will cause a recursive locking of the cpu
> > hotplug lock when the memory hotplug code calls lru_add_drain_all().
> > 
> > Split out the inner workings of lru_add_drain_all() into
> > lru_add_drain_all_cpuslocked() so this function can be invoked from the
> > memory hotplug code with the cpu hotplug lock held.
> 
> You have added callers in the later patch in the series AFAICS which
> is OK but I think it would be better to have them in this patch
> already. Nothing earth shattering (maybe a rebase artifact).

The requirement for changing that comes with the extra hotplug locking in
mem_hotplug_begin(). That is required to establish the proper lock order
and then causes the recursive locking in the next patch. Adding the caller
here would be wrong, because then lru_add_drain_all_cpuslocked() would be
called unprotected. Hens and eggs as usual :)

Thanks,

	tglx

--
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 V2 1/2] mm: swap: Provide lru_add_drain_all_cpuslocked()

[Michal Hocko]

On Tue 04-07-17 14:48:56, Thomas Gleixner wrote:
> On Tue, 4 Jul 2017, Michal Hocko wrote:
> > On Tue 04-07-17 11:32:33, Thomas Gleixner wrote:
> > > The rework of the cpu hotplug locking unearthed potential deadlocks with
> > > the memory hotplug locking code.
> > > 
> > > The solution for these is to rework the memory hotplug locking code as well
> > > and take the cpu hotplug lock before the memory hotplug lock in
> > > mem_hotplug_begin(), but this will cause a recursive locking of the cpu
> > > hotplug lock when the memory hotplug code calls lru_add_drain_all().
> > > 
> > > Split out the inner workings of lru_add_drain_all() into
> > > lru_add_drain_all_cpuslocked() so this function can be invoked from the
> > > memory hotplug code with the cpu hotplug lock held.
> > 
> > You have added callers in the later patch in the series AFAICS which
> > is OK but I think it would be better to have them in this patch
> > already. Nothing earth shattering (maybe a rebase artifact).
> 
> The requirement for changing that comes with the extra hotplug locking in
> mem_hotplug_begin(). That is required to establish the proper lock order
> and then causes the recursive locking in the next patch. Adding the caller
> here would be wrong, because then lru_add_drain_all_cpuslocked() would be
> called unprotected. Hens and eggs as usual :)

Yeah, you are right. My bad I should have noticed that.
-- 
Michal Hocko
SUSE Labs

--
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 V2 1/2] mm: swap: Provide lru_add_drain_all_cpuslocked()

[Vlastimil Babka]

On 07/04/2017 11:32 AM, Thomas Gleixner wrote:
> The rework of the cpu hotplug locking unearthed potential deadlocks with
> the memory hotplug locking code.
> 
> The solution for these is to rework the memory hotplug locking code as well
> and take the cpu hotplug lock before the memory hotplug lock in
> mem_hotplug_begin(), but this will cause a recursive locking of the cpu
> hotplug lock when the memory hotplug code calls lru_add_drain_all().
> 
> Split out the inner workings of lru_add_drain_all() into
> lru_add_drain_all_cpuslocked() so this function can be invoked from the
> memory hotplug code with the cpu hotplug lock held.
> 
> Reported-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: linux-mm@kvack.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

A question below.

> ---
>  include/linux/swap.h |    1 +
>  mm/swap.c            |   11 ++++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -277,6 +277,7 @@ extern void mark_page_accessed(struct pa
>  extern void lru_add_drain(void);
>  extern void lru_add_drain_cpu(int cpu);
>  extern void lru_add_drain_all(void);
> +extern void lru_add_drain_all_cpuslocked(void);
>  extern void rotate_reclaimable_page(struct page *page);
>  extern void deactivate_file_page(struct page *page);
>  extern void mark_page_lazyfree(struct page *page);
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -687,7 +687,7 @@ static void lru_add_drain_per_cpu(struct
>  
>  static DEFINE_PER_CPU(struct work_struct, lru_add_drain_work);
>  
> -void lru_add_drain_all(void)
> +void lru_add_drain_all_cpuslocked(void)
>  {
>  	static DEFINE_MUTEX(lock);
>  	static struct cpumask has_work;
> @@ -701,7 +701,6 @@ void lru_add_drain_all(void)
>  		return;
>  
>  	mutex_lock(&lock);
> -	get_online_cpus();

Is there a an assertion check that we are locked, that could be put in
e.g. VM_WARN_ON_ONCE()?

>  	cpumask_clear(&has_work);
>  
>  	for_each_online_cpu(cpu) {
> @@ -721,10 +720,16 @@ void lru_add_drain_all(void)
>  	for_each_cpu(cpu, &has_work)
>  		flush_work(&per_cpu(lru_add_drain_work, cpu));
>  
> -	put_online_cpus();
>  	mutex_unlock(&lock);
>  }
>  
> +void lru_add_drain_all(void)
> +{
> +	get_online_cpus();
> +	lru_add_drain_all_cpuslocked();
> +	put_online_cpus();
> +}
> +
>  /**
>   * release_pages - batched put_page()
>   * @pages: array of pages to release
> 
> 

--
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 V2 1/2] mm: swap: Provide lru_add_drain_all_cpuslocked()

[Thomas Gleixner]

On Tue, 4 Jul 2017, Vlastimil Babka wrote:
> >  
> > -void lru_add_drain_all(void)
> > +void lru_add_drain_all_cpuslocked(void)
> >  {
> >  	static DEFINE_MUTEX(lock);
> >  	static struct cpumask has_work;
> > @@ -701,7 +701,6 @@ void lru_add_drain_all(void)
> >  		return;
> >  
> >  	mutex_lock(&lock);
> > -	get_online_cpus();
> 
> Is there a an assertion check that we are locked, that could be put in
> e.g. VM_WARN_ON_ONCE()?

There is a lockdep assertion lockdep_assert_cpus_held() which could be
used. Forgot to add it.

Thanks,

	tglx


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

[patch V2 2/2] mm/memory-hotplug: Switch locking to a percpu rwsem

[Thomas Gleixner]

[-- Attachment #1: mmmemory-hotplug_Switch_locking_to_a_percpu_rwsem.patch --]
[-- Type: text/plain, Size: 5671 bytes --]

Andrey reported a potential deadlock with the memory hotplug lock and the
cpu hotplug lock.

The reason is that memory hotplug takes the memory hotplug lock and then
calls stop_machine() which calls get_online_cpus(). That's the reverse lock
order to get_online_cpus(); get_online_mems(); in mm/slub_common.c

The problem has been there forever. The reason why this was never reported
is that the cpu hotplug locking had this homebrewn recursive reader writer
semaphore construct which due to the recursion evaded the full lock dep
coverage. The memory hotplug code copied that construct verbatim and
therefor has similar issues.

Three steps to fix this:

1) Convert the memory hotplug locking to a per cpu rwsem so the potential
   issues get reported proper by lockdep.

2) Lock the online cpus in mem_hotplug_begin() before taking the memory
   hotplug rwsem and use stop_machine_cpuslocked() in the page_alloc code
   and use to avoid recursive locking.

3) The cpu hotpluck locking in #2 causes a recursive locking of the cpu
   hotplug lock via __offline_pages() -> lru_add_drain_all(). Solve this by
   invoking lru_add_drain_all_cpuslocked() instead.

Reported-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: linux-mm@kvack.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
---
 mm/memory_hotplug.c |   89 ++++++++--------------------------------------------
 mm/page_alloc.c     |    2 -
 2 files changed, 16 insertions(+), 75 deletions(-)

--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -52,32 +52,17 @@ static void generic_online_page(struct p
 static online_page_callback_t online_page_callback = generic_online_page;
 static DEFINE_MUTEX(online_page_callback_lock);
 
-/* The same as the cpu_hotplug lock, but for memory hotplug. */
-static struct {
-	struct task_struct *active_writer;
-	struct mutex lock; /* Synchronizes accesses to refcount, */
-	/*
-	 * Also blocks the new readers during
-	 * an ongoing mem hotplug operation.
-	 */
-	int refcount;
+DEFINE_STATIC_PERCPU_RWSEM(mem_hotplug_lock);
 
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-	struct lockdep_map dep_map;
-#endif
-} mem_hotplug = {
-	.active_writer = NULL,
-	.lock = __MUTEX_INITIALIZER(mem_hotplug.lock),
-	.refcount = 0,
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-	.dep_map = {.name = "mem_hotplug.lock" },
-#endif
-};
+void get_online_mems(void)
+{
+	percpu_down_read(&mem_hotplug_lock);
+}
 
-/* Lockdep annotations for get/put_online_mems() and mem_hotplug_begin/end() */
-#define memhp_lock_acquire_read() lock_map_acquire_read(&mem_hotplug.dep_map)
-#define memhp_lock_acquire()      lock_map_acquire(&mem_hotplug.dep_map)
-#define memhp_lock_release()      lock_map_release(&mem_hotplug.dep_map)
+void put_online_mems(void)
+{
+	percpu_up_read(&mem_hotplug_lock);
+}
 
 #ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
 bool memhp_auto_online;
@@ -97,60 +82,16 @@ static int __init setup_memhp_default_st
 }
 __setup("memhp_default_state=", setup_memhp_default_state);
 
-void get_online_mems(void)
-{
-	might_sleep();
-	if (mem_hotplug.active_writer == current)
-		return;
-	memhp_lock_acquire_read();
-	mutex_lock(&mem_hotplug.lock);
-	mem_hotplug.refcount++;
-	mutex_unlock(&mem_hotplug.lock);
-
-}
-
-void put_online_mems(void)
-{
-	if (mem_hotplug.active_writer == current)
-		return;
-	mutex_lock(&mem_hotplug.lock);
-
-	if (WARN_ON(!mem_hotplug.refcount))
-		mem_hotplug.refcount++; /* try to fix things up */
-
-	if (!--mem_hotplug.refcount && unlikely(mem_hotplug.active_writer))
-		wake_up_process(mem_hotplug.active_writer);
-	mutex_unlock(&mem_hotplug.lock);
-	memhp_lock_release();
-
-}
-
-/* Serializes write accesses to mem_hotplug.active_writer. */
-static DEFINE_MUTEX(memory_add_remove_lock);
-
 void mem_hotplug_begin(void)
 {
-	mutex_lock(&memory_add_remove_lock);
-
-	mem_hotplug.active_writer = current;
-
-	memhp_lock_acquire();
-	for (;;) {
-		mutex_lock(&mem_hotplug.lock);
-		if (likely(!mem_hotplug.refcount))
-			break;
-		__set_current_state(TASK_UNINTERRUPTIBLE);
-		mutex_unlock(&mem_hotplug.lock);
-		schedule();
-	}
+	cpus_read_lock();
+	percpu_down_write(&mem_hotplug_lock);
 }
 
 void mem_hotplug_done(void)
 {
-	mem_hotplug.active_writer = NULL;
-	mutex_unlock(&mem_hotplug.lock);
-	memhp_lock_release();
-	mutex_unlock(&memory_add_remove_lock);
+	percpu_up_write(&mem_hotplug_lock);
+	cpus_read_unlock();
 }
 
 /* add this memory to iomem resource */
@@ -1919,7 +1860,7 @@ static int __ref __offline_pages(unsigne
 		goto failed_removal;
 	ret = 0;
 	if (drain) {
-		lru_add_drain_all();
+		lru_add_drain_all_cpuslocked();
 		cond_resched();
 		drain_all_pages(zone);
 	}
@@ -1940,7 +1881,7 @@ static int __ref __offline_pages(unsigne
 		}
 	}
 	/* drain all zone's lru pagevec, this is asynchronous... */
-	lru_add_drain_all();
+	lru_add_drain_all_cpuslocked();
 	yield();
 	/* drain pcp pages, this is synchronous. */
 	drain_all_pages(zone);
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5216,7 +5216,7 @@ void __ref build_all_zonelists(pg_data_t
 #endif
 		/* we have to stop all cpus to guarantee there is no user
 		   of zonelist */
-		stop_machine(__build_all_zonelists, pgdat, NULL);
+		stop_machine_cpuslocked(__build_all_zonelists, pgdat, NULL);
 		/* cpuset refresh routine should be here */
 	}
 	vm_total_pages = nr_free_pagecache_pages();


--
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 V2 2/2] mm/memory-hotplug: Switch locking to a percpu rwsem

[Michal Hocko]

On Tue 04-07-17 11:32:34, Thomas Gleixner wrote:
> Andrey reported a potential deadlock with the memory hotplug lock and the
> cpu hotplug lock.
> 
> The reason is that memory hotplug takes the memory hotplug lock and then
> calls stop_machine() which calls get_online_cpus(). That's the reverse lock
> order to get_online_cpus(); get_online_mems(); in mm/slub_common.c
> 
> The problem has been there forever. The reason why this was never reported
> is that the cpu hotplug locking had this homebrewn recursive reader writer
> semaphore construct which due to the recursion evaded the full lock dep
> coverage. The memory hotplug code copied that construct verbatim and
> therefor has similar issues.
> 
> Three steps to fix this:
> 
> 1) Convert the memory hotplug locking to a per cpu rwsem so the potential
>    issues get reported proper by lockdep.
> 
> 2) Lock the online cpus in mem_hotplug_begin() before taking the memory
>    hotplug rwsem and use stop_machine_cpuslocked() in the page_alloc code
>    and use to avoid recursive locking.
> 
> 3) The cpu hotpluck locking in #2 causes a recursive locking of the cpu
>    hotplug lock via __offline_pages() -> lru_add_drain_all(). Solve this by
>    invoking lru_add_drain_all_cpuslocked() instead.
> 
> Reported-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: linux-mm@kvack.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/memory_hotplug.c |   89 ++++++++--------------------------------------------
>  mm/page_alloc.c     |    2 -
>  2 files changed, 16 insertions(+), 75 deletions(-)
> 
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -52,32 +52,17 @@ static void generic_online_page(struct p
>  static online_page_callback_t online_page_callback = generic_online_page;
>  static DEFINE_MUTEX(online_page_callback_lock);
>  
> -/* The same as the cpu_hotplug lock, but for memory hotplug. */
> -static struct {
> -	struct task_struct *active_writer;
> -	struct mutex lock; /* Synchronizes accesses to refcount, */
> -	/*
> -	 * Also blocks the new readers during
> -	 * an ongoing mem hotplug operation.
> -	 */
> -	int refcount;
> +DEFINE_STATIC_PERCPU_RWSEM(mem_hotplug_lock);
>  
> -#ifdef CONFIG_DEBUG_LOCK_ALLOC
> -	struct lockdep_map dep_map;
> -#endif
> -} mem_hotplug = {
> -	.active_writer = NULL,
> -	.lock = __MUTEX_INITIALIZER(mem_hotplug.lock),
> -	.refcount = 0,
> -#ifdef CONFIG_DEBUG_LOCK_ALLOC
> -	.dep_map = {.name = "mem_hotplug.lock" },
> -#endif
> -};
> +void get_online_mems(void)
> +{
> +	percpu_down_read(&mem_hotplug_lock);
> +}
>  
> -/* Lockdep annotations for get/put_online_mems() and mem_hotplug_begin/end() */
> -#define memhp_lock_acquire_read() lock_map_acquire_read(&mem_hotplug.dep_map)
> -#define memhp_lock_acquire()      lock_map_acquire(&mem_hotplug.dep_map)
> -#define memhp_lock_release()      lock_map_release(&mem_hotplug.dep_map)
> +void put_online_mems(void)
> +{
> +	percpu_up_read(&mem_hotplug_lock);
> +}
>  
>  #ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
>  bool memhp_auto_online;
> @@ -97,60 +82,16 @@ static int __init setup_memhp_default_st
>  }
>  __setup("memhp_default_state=", setup_memhp_default_state);
>  
> -void get_online_mems(void)
> -{
> -	might_sleep();
> -	if (mem_hotplug.active_writer == current)
> -		return;
> -	memhp_lock_acquire_read();
> -	mutex_lock(&mem_hotplug.lock);
> -	mem_hotplug.refcount++;
> -	mutex_unlock(&mem_hotplug.lock);
> -
> -}
> -
> -void put_online_mems(void)
> -{
> -	if (mem_hotplug.active_writer == current)
> -		return;
> -	mutex_lock(&mem_hotplug.lock);
> -
> -	if (WARN_ON(!mem_hotplug.refcount))
> -		mem_hotplug.refcount++; /* try to fix things up */
> -
> -	if (!--mem_hotplug.refcount && unlikely(mem_hotplug.active_writer))
> -		wake_up_process(mem_hotplug.active_writer);
> -	mutex_unlock(&mem_hotplug.lock);
> -	memhp_lock_release();
> -
> -}
> -
> -/* Serializes write accesses to mem_hotplug.active_writer. */
> -static DEFINE_MUTEX(memory_add_remove_lock);
> -
>  void mem_hotplug_begin(void)
>  {
> -	mutex_lock(&memory_add_remove_lock);
> -
> -	mem_hotplug.active_writer = current;
> -
> -	memhp_lock_acquire();
> -	for (;;) {
> -		mutex_lock(&mem_hotplug.lock);
> -		if (likely(!mem_hotplug.refcount))
> -			break;
> -		__set_current_state(TASK_UNINTERRUPTIBLE);
> -		mutex_unlock(&mem_hotplug.lock);
> -		schedule();
> -	}
> +	cpus_read_lock();
> +	percpu_down_write(&mem_hotplug_lock);
>  }
>  
>  void mem_hotplug_done(void)
>  {
> -	mem_hotplug.active_writer = NULL;
> -	mutex_unlock(&mem_hotplug.lock);
> -	memhp_lock_release();
> -	mutex_unlock(&memory_add_remove_lock);
> +	percpu_up_write(&mem_hotplug_lock);
> +	cpus_read_unlock();
>  }
>  
>  /* add this memory to iomem resource */
> @@ -1919,7 +1860,7 @@ static int __ref __offline_pages(unsigne
>  		goto failed_removal;
>  	ret = 0;
>  	if (drain) {
> -		lru_add_drain_all();
> +		lru_add_drain_all_cpuslocked();
>  		cond_resched();
>  		drain_all_pages(zone);
>  	}
> @@ -1940,7 +1881,7 @@ static int __ref __offline_pages(unsigne
>  		}
>  	}
>  	/* drain all zone's lru pagevec, this is asynchronous... */
> -	lru_add_drain_all();
> +	lru_add_drain_all_cpuslocked();
>  	yield();
>  	/* drain pcp pages, this is synchronous. */
>  	drain_all_pages(zone);
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5216,7 +5216,7 @@ void __ref build_all_zonelists(pg_data_t
>  #endif
>  		/* we have to stop all cpus to guarantee there is no user
>  		   of zonelist */
> -		stop_machine(__build_all_zonelists, pgdat, NULL);
> +		stop_machine_cpuslocked(__build_all_zonelists, pgdat, NULL);
>  		/* cpuset refresh routine should be here */
>  	}
>  	vm_total_pages = nr_free_pagecache_pages();
> 

-- 
Michal Hocko
SUSE Labs

--
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 V2 2/2] mm/memory-hotplug: Switch locking to a percpu rwsem

[Vlastimil Babka]

On 07/04/2017 11:32 AM, Thomas Gleixner wrote:
> Andrey reported a potential deadlock with the memory hotplug lock and the
> cpu hotplug lock.
> 
> The reason is that memory hotplug takes the memory hotplug lock and then
> calls stop_machine() which calls get_online_cpus(). That's the reverse lock
> order to get_online_cpus(); get_online_mems(); in mm/slub_common.c
> 
> The problem has been there forever. The reason why this was never reported
> is that the cpu hotplug locking had this homebrewn recursive reader writer
> semaphore construct which due to the recursion evaded the full lock dep
> coverage. The memory hotplug code copied that construct verbatim and
> therefor has similar issues.
> 
> Three steps to fix this:
> 
> 1) Convert the memory hotplug locking to a per cpu rwsem so the potential
>    issues get reported proper by lockdep.
> 
> 2) Lock the online cpus in mem_hotplug_begin() before taking the memory
>    hotplug rwsem and use stop_machine_cpuslocked() in the page_alloc code
>    and use to avoid recursive locking.

     ^ s/and use // ?

> 
> 3) The cpu hotpluck locking in #2 causes a recursive locking of the cpu
>    hotplug lock via __offline_pages() -> lru_add_drain_all(). Solve this by
>    invoking lru_add_drain_all_cpuslocked() instead.
> 
> Reported-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: linux-mm@kvack.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/memory_hotplug.c |   89 ++++++++--------------------------------------------
>  mm/page_alloc.c     |    2 -
>  2 files changed, 16 insertions(+), 75 deletions(-)

Nice! Glad to see the crazy code go.

--
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 V2 2/2] mm/memory-hotplug: Switch locking to a percpu rwsem

[Thomas Gleixner]

On Tue, 4 Jul 2017, Vlastimil Babka wrote:

> On 07/04/2017 11:32 AM, Thomas Gleixner wrote:
> > Andrey reported a potential deadlock with the memory hotplug lock and the
> > cpu hotplug lock.
> > 
> > The reason is that memory hotplug takes the memory hotplug lock and then
> > calls stop_machine() which calls get_online_cpus(). That's the reverse lock
> > order to get_online_cpus(); get_online_mems(); in mm/slub_common.c
> > 
> > The problem has been there forever. The reason why this was never reported
> > is that the cpu hotplug locking had this homebrewn recursive reader writer
> > semaphore construct which due to the recursion evaded the full lock dep
> > coverage. The memory hotplug code copied that construct verbatim and
> > therefor has similar issues.
> > 
> > Three steps to fix this:
> > 
> > 1) Convert the memory hotplug locking to a per cpu rwsem so the potential
> >    issues get reported proper by lockdep.
> > 
> > 2) Lock the online cpus in mem_hotplug_begin() before taking the memory
> >    hotplug rwsem and use stop_machine_cpuslocked() in the page_alloc code
> >    and use to avoid recursive locking.
> 
>      ^ s/and use // ?

Ooops, yes.

Thanks,

	tglx

--
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 V2 2/2] mm/memory-hotplug: Switch locking to a percpu rwsem

[Davidlohr Bueso]

On Tue, 04 Jul 2017, Thomas Gleixner wrote:

>Andrey reported a potential deadlock with the memory hotplug lock and the
>cpu hotplug lock.
>
>The reason is that memory hotplug takes the memory hotplug lock and then
>calls stop_machine() which calls get_online_cpus(). That's the reverse lock
>order to get_online_cpus(); get_online_mems(); in mm/slub_common.c
>
>The problem has been there forever. The reason why this was never reported
>is that the cpu hotplug locking had this homebrewn recursive reader writer
>semaphore construct which due to the recursion evaded the full lock dep
>coverage. The memory hotplug code copied that construct verbatim and
>therefor has similar issues.
>
>Three steps to fix this:
>
>1) Convert the memory hotplug locking to a per cpu rwsem so the potential
>   issues get reported proper by lockdep.

I particularly like how the mem hotplug is well suited for pcpu-rwsem.
As a side effect you end up optimizing get/put_online_mems() at the cost
of more overhead for the actual hotplug operation, which is rare and of less
performance importance.

Thanks,
Davidlohr

--
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 V2 2/2] mm/memory-hotplug: Switch locking to a percpu rwsem

[Davidlohr Bueso]

On Tue, 04 Jul 2017, Davidlohr Bueso wrote:

>As a side effect you end up optimizing get/put_online_mems() at the cost
>of more overhead for the actual hotplug operation, which is rare and of less
>performance importance.

So nm this, the reader side actually gets _more_ expensive with pcpu-rwsems
due to at least two full barriers for each get/put operation.

Thanks,
Davidlohr

--
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 V2 2/2] mm/memory-hotplug: Switch locking to a percpu rwsem

[Thomas Gleixner]

On Tue, 4 Jul 2017, Davidlohr Bueso wrote:
> On Tue, 04 Jul 2017, Davidlohr Bueso wrote:
> 
> > As a side effect you end up optimizing get/put_online_mems() at the cost
> > of more overhead for the actual hotplug operation, which is rare and of less
> > performance importance.
> 
> So nm this, the reader side actually gets _more_ expensive with pcpu-rwsems
> due to at least two full barriers for each get/put operation.

Compared to a mutex_lock/unlock() pair on a global 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 V2 2/2] mm/memory-hotplug: Switch locking to a percpu rwsem

[Davidlohr Bueso]

On Tue, 04 Jul 2017, Thomas Gleixner wrote:

>On Tue, 4 Jul 2017, Davidlohr Bueso wrote:
>> On Tue, 04 Jul 2017, Davidlohr Bueso wrote:
>>
>> > As a side effect you end up optimizing get/put_online_mems() at the cost
>> > of more overhead for the actual hotplug operation, which is rare and of less
>> > performance importance.
>>
>> So nm this, the reader side actually gets _more_ expensive with pcpu-rwsems
>> due to at least two full barriers for each get/put operation.
>
>Compared to a mutex_lock/unlock() pair on a global mutex ....

Ah, right, I was thrown off the:

    if (mem_hotplug.active_writer == current)
       return;

checks, which is only true within hotplug_begin/end. So normally we'd take
the lock, which was my first impression. Sorry for the noise.

Thanks,
Davidlohr

--
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 V2 0/2] mm/memory_hotplug: Cure potential deadlocks vs. cpu hotplug lock

[Andrew Morton]

On Tue, 04 Jul 2017 11:32:32 +0200 Thomas Gleixner <tglx@linutronix.de> wrote:

> Andrey reported a potential deadlock with the memory hotplug lock and the
> cpu hotplug lock.
> 
> The following series addresses this by reworking the memory hotplug locking
> and fixing up the potential deadlock scenarios.

Do you think we should squeeze this into 4.13-rc1, or can we afford to
take the more cautious route?

--
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 V2 0/2] mm/memory_hotplug: Cure potential deadlocks vs. cpu hotplug lock

[Thomas Gleixner]

On Wed, 5 Jul 2017, Andrew Morton wrote:
> On Tue, 04 Jul 2017 11:32:32 +0200 Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > Andrey reported a potential deadlock with the memory hotplug lock and the
> > cpu hotplug lock.
> > 
> > The following series addresses this by reworking the memory hotplug locking
> > and fixing up the potential deadlock scenarios.
> 
> Do you think we should squeeze this into 4.13-rc1, or can we afford to
> take the more cautious route?

The deadlocks are real and the lockdep splats are triggering on Linus head,
so it should go into 4.13-rc1 if possible.

Thanks,

	tglx

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