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>