Thread (60 messages) 60 messages, 9 authors, 2017-02-10

Re: mm: deadlock between get_online_cpus/pcpu_alloc

From: Mel Gorman <hidden>
Date: 2017-02-07 11:34:39
Also in: lkml
Subsystem: memory management, memory management - page allocator, the rest · Maintainers: Andrew Morton, Vlastimil Babka, Linus Torvalds

On Tue, Feb 07, 2017 at 11:35:52AM +0100, Michal Hocko wrote:
On Tue 07-02-17 10:28:09, Mel Gorman wrote:
quoted
On Tue, Feb 07, 2017 at 10:49:28AM +0100, Vlastimil Babka wrote:
quoted
On 02/07/2017 10:43 AM, Mel Gorman wrote:
quoted
If I'm reading this right, a hot-remove will set the pool POOL_DISASSOCIATED
and unbound. A workqueue queued for draining get migrated during hot-remove
and a drain operation will execute twice on a CPU -- one for what was
queued and a second time for the CPU it was migrated from. It should still
work with flush_work which doesn't appear to block forever if an item
got migrated to another workqueue. The actual drain workqueue function is
using the CPU ID it's currently running on so it shouldn't get confused.
Is the worker that will process this migrated workqueue also guaranteed
to be pinned to a cpu for the whole work, though? drain_local_pages()
needs that guarantee.
It should be by running on a workqueue handler bound to that CPU (queued
on wq->cpu_pwqs in __queue_work)
Are you sure? The comment in kernel/workqueue.c says
         * While DISASSOCIATED, the cpu may be offline and all workers have
         * %WORKER_UNBOUND set and concurrency management disabled, and may
         * be executing on any CPU.  The pool behaves as an unbound one.

I might be misreadig but an unbound pool can be handled by workers which
are not pinned on any cpu AFAIU.
Right. The unbind operation can set a mask that is any allowable CPU and
the final process_work is not done in a context that prevents
preemption.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3b93879990fd..7af165d308c4 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2342,7 +2342,14 @@ void drain_local_pages(struct zone *zone)
 
 static void drain_local_pages_wq(struct work_struct *work)
 {
+	/*
+	 * Ordinarily a drain operation is bound to a CPU but may be unbound
+	 * after a CPU hotplug operation so it's necessary to disable
+	 * preemption for the drain to stabilise the CPU ID.
+	 */
+	preempt_disable();
 	drain_local_pages(NULL);
+	preempt_enable_no_resched();
 }
 
 /*
@@ -2377,13 +2384,10 @@ void drain_all_pages(struct zone *zone)
 		mutex_lock(&pcpu_drain_mutex);
 	}
 
-	get_online_cpus();
-
 	/*
-	 * We don't care about racing with CPU hotplug event
-	 * as offline notification will cause the notified
-	 * cpu to drain that CPU pcps and on_each_cpu_mask
-	 * disables preemption as part of its processing
+	 * We don't care about racing with CPU hotplug event as offline
+	 * notification will cause the notified cpu to drain that CPU pcps
+	 * and it is serialised against here via pcpu_drain_mutex.
 	 */
 	for_each_online_cpu(cpu) {
 		struct per_cpu_pageset *pcp;
@@ -2418,7 +2422,6 @@ void drain_all_pages(struct zone *zone)
 	for_each_cpu(cpu, &cpus_with_pcps)
 		flush_work(per_cpu_ptr(&pcpu_drain, cpu));
 
-	put_online_cpus();
 	mutex_unlock(&pcpu_drain_mutex);
 }
 
@@ -6711,7 +6714,16 @@ static int page_alloc_cpu_dead(unsigned int cpu)
 {
 
 	lru_add_drain_cpu(cpu);
+
+	/*
+	 * A per-cpu drain via a workqueue from drain_all_pages can be
+	 * rescheduled onto an unrelated CPU. That allows the hotplug
+	 * operation and the drain to potentially race on the same
+	 * CPU. Serialise hotplug versus drain using pcpu_drain_mutex
+	 */
+	mutex_lock(&pcpu_drain_mutex);
 	drain_pages(cpu);
+	mutex_unlock(&pcpu_drain_mutex);
 
 	/*
 	 * Spill the event counters of the dead processor
-- 
Mel Gorman
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>
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help