Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1575081 > unrolled thread
| Started by | Dmitry Vyukov <dvyukov@google.com> |
|---|---|
| First post | 2017-02-06 20:20 +0100 |
| Last post | 2017-02-07 23:40 +0100 |
| Articles | 20 on this page of 57 — 9 participants |
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.
Re: mm: deadlock between get_online_cpus/pcpu_alloc Dmitry Vyukov <dvyukov@google.com> - 2017-02-06 20:20 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Mel Gorman <mgorman@techsingularity.net> - 2017-02-06 23:10 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 09:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Vlastimil Babka <vbabka@suse.cz> - 2017-02-07 10:30 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Mel Gorman <mgorman@techsingularity.net> - 2017-02-07 10:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 11:00 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Mel Gorman <mgorman@techsingularity.net> - 2017-02-07 11:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Mel Gorman <mgorman@techsingularity.net> - 2017-02-07 12:20 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Mel Gorman <mgorman@techsingularity.net> - 2017-02-07 10:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Vlastimil Babka <vbabka@suse.cz> - 2017-02-07 11:00 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 11:10 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Mel Gorman <mgorman@techsingularity.net> - 2017-02-07 11:30 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 11:40 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Mel Gorman <mgorman@techsingularity.net> - 2017-02-07 12:40 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 12:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Vlastimil Babka <vbabka@suse.cz> - 2017-02-07 13:00 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 13:10 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 13:40 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Vlastimil Babka <vbabka@suse.cz> - 2017-02-07 13:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 13:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Vlastimil Babka <vbabka@suse.cz> - 2017-02-07 15:00 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Mel Gorman <mgorman@techsingularity.net> - 2017-02-07 15:00 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 15:20 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 16:40 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Mel Gorman <mgorman@techsingularity.net> - 2017-02-07 17:30 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 17:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Christoph Lameter <cl@linux.com> - 2017-02-07 18:00 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Thomas Gleixner <tglx@linutronix.de> - 2017-02-07 23:30 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-08 08:40 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Thomas Gleixner <tglx@linutronix.de> - 2017-02-08 13:10 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-08 13:30 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Mel Gorman <mgorman@techsingularity.net> - 2017-02-08 13:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Mel Gorman <mgorman@techsingularity.net> - 2017-02-08 15:10 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Peter Zijlstra <peterz@infradead.org> - 2017-02-08 18:00 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Thomas Gleixner <tglx@linutronix.de> - 2017-02-08 15:10 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Christoph Lameter <cl@linux.com> - 2017-02-08 16:30 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Christoph Lameter <cl@linux.com> - 2017-02-08 17:30 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Thomas Gleixner <tglx@linutronix.de> - 2017-02-08 19:40 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Christoph Lameter <cl@linux.com> - 2017-02-09 04:20 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Thomas Gleixner <tglx@linutronix.de> - 2017-02-09 12:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Christoph Lameter <cl@linux.com> - 2017-02-09 15:10 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Thomas Gleixner <tglx@linutronix.de> - 2017-02-09 16:40 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Christoph Lameter <cl@linux.com> - 2017-02-09 16:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Thomas Gleixner <tglx@linutronix.de> - 2017-02-09 17:20 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Christoph Lameter <cl@linux.com> - 2017-02-09 18:30 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Thomas Gleixner <tglx@linutronix.de> - 2017-02-09 18:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-09 20:20 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Christoph Lameter <cl@linux.com> - 2017-02-10 19:10 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-08 18:40 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Christoph Lameter <cl@linux.com> - 2017-02-08 16:10 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Tejun Heo <tj@kernel.org> - 2017-02-07 18:10 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 21:40 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Mel Gorman <mgorman@techsingularity.net> - 2017-02-07 14:10 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 14:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-02-07 12:30 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Michal Hocko <mhocko@kernel.org> - 2017-02-07 09:50 +0100
Re: mm: deadlock between get_online_cpus/pcpu_alloc Thomas Gleixner <tglx@linutronix.de> - 2017-02-07 23:40 +0100
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | Vlastimil Babka <vbabka@suse.cz> |
|---|---|
| Date | 2017-02-07 15:00 +0100 |
| Message-ID | <t8etk-3Av-9@gated-at.bofh.it> |
| In reply to | #1575637 |
On 02/07/2017 01:48 PM, Michal Hocko wrote: > On Tue 07-02-17 13:43:39, Vlastimil Babka wrote: > [...] >> > Anyway, shouldn't be it sufficient to disable preemption >> > on drain_local_pages_wq? The CPU hotplug callback will not preempt us >> > and so we cannot work on the same cpus, right? >> >> I thought the problem here was that the callback races with the work item >> that has been migrated to a different cpu. Once we are not working on the >> local cpu, disabling preempt/irq's won't help? > > If the worker is racing with the callback than only one of can run on a > _particular_ cpu. So they cannot race. Or am I missing something? Ah I forgot that migrated work item will in fact run on local cpu. So looks like nobody should race with the callback indeed (assuming that when the callback is called, the cpu in question already isn't executing workqueue workers).
[toc] | [prev] | [next] | [standalone]
| From | Mel Gorman <mgorman@techsingularity.net> |
|---|---|
| Date | 2017-02-07 15:00 +0100 |
| Message-ID | <t8etk-3Av-19@gated-at.bofh.it> |
| In reply to | #1575625 |
On Tue, Feb 07, 2017 at 01:37:08PM +0100, Michal Hocko wrote: > > You cannot put sleepable lock inside the preempt disbaled section... > > We can make it a spinlock right? > > Scratch that! For some reason I thought that cpu notifiers are run in an > atomic context. Now that I am checking the code again it turns out I was > wrong. __cpu_notify uses __raw_notifier_call_chain so this is not an > atomic context. Indeed. > Anyway, shouldn't be it sufficient to disable preemption > on drain_local_pages_wq? That would be sufficient for a hot-removed CPU moving the drain request to another CPU and avoiding any scheduling events. > The CPU hotplug callback will not preempt us > and so we cannot work on the same cpus, right? > I don't see a specific guarantee that it cannot be preempted and it would depend on an the exact cpu hotplug implementation which is subject to quite a lot of change. Hence, the mutex provides a guantee that the hot-removed CPU teardown cannot run on the same CPU as a workqueue drain running on a CPU it was not originally scheduled for. -- Mel Gorman SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-02-07 15:20 +0100 |
| Message-ID | <t8eMG-3Xq-9@gated-at.bofh.it> |
| In reply to | #1575757 |
On Tue 07-02-17 13:58:46, Mel Gorman wrote: > On Tue, Feb 07, 2017 at 01:37:08PM +0100, Michal Hocko wrote: [...] > > Anyway, shouldn't be it sufficient to disable preemption > > on drain_local_pages_wq? > > That would be sufficient for a hot-removed CPU moving the drain request > to another CPU and avoiding any scheduling events. > > > The CPU hotplug callback will not preempt us > > and so we cannot work on the same cpus, right? > > > > I don't see a specific guarantee that it cannot be preempted and it > would depend on an the exact cpu hotplug implementation which is subject > to quite a lot of change. But we do not care about the whole cpu hotplug code. The only part we really do care about is the race inside drain_pages_zone and that will run in an atomic context on the specific CPU. You are absolutely right that using the mutex is safe as well but the hotplug path is already littered with locks and adding one more to the picture doesn't sound great to me. So I would really like to not use a lock if that is possible and safe (with a big fat comment of course). -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-02-07 16:40 +0100 |
| Message-ID | <t8g25-4G5-11@gated-at.bofh.it> |
| In reply to | #1575762 |
On Tue 07-02-17 15:19:11, Michal Hocko wrote:
> On Tue 07-02-17 13:58:46, Mel Gorman wrote:
> > On Tue, Feb 07, 2017 at 01:37:08PM +0100, Michal Hocko wrote:
> [...]
> > > Anyway, shouldn't be it sufficient to disable preemption
> > > on drain_local_pages_wq?
> >
> > That would be sufficient for a hot-removed CPU moving the drain request
> > to another CPU and avoiding any scheduling events.
> >
> > > The CPU hotplug callback will not preempt us
> > > and so we cannot work on the same cpus, right?
> > >
> >
> > I don't see a specific guarantee that it cannot be preempted and it
> > would depend on an the exact cpu hotplug implementation which is subject
> > to quite a lot of change.
>
> But we do not care about the whole cpu hotplug code. The only part we
> really do care about is the race inside drain_pages_zone and that will
> run in an atomic context on the specific CPU.
>
> You are absolutely right that using the mutex is safe as well but the
> hotplug path is already littered with locks and adding one more to the
> picture doesn't sound great to me. So I would really like to not use a
> lock if that is possible and safe (with a big fat comment of course).
And with the full changelog. I hope I haven't missed anything this time.
---
From 8c6af3116520251cc4ec2213f0a4ed2544bb4365 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Tue, 7 Feb 2017 16:08:35 +0100
Subject: [PATCH] mm, page_alloc: do not depend on cpu hotplug locks inside the
allocator
Dmitry has reported the following lockdep splat
[<ffffffff81571db1>] lock_acquire+0x2a1/0x630 kernel/locking/lockdep.c:3753
[<ffffffff8436697e>] __mutex_lock_common kernel/locking/mutex.c:521 [inline]
[<ffffffff8436697e>] mutex_lock_nested+0x24e/0xff0 kernel/locking/mutex.c:621
[<ffffffff818f07ea>] pcpu_alloc+0xbda/0x1280 mm/percpu.c:896
[<ffffffff818f0ee4>] __alloc_percpu+0x24/0x30 mm/percpu.c:1075
[<ffffffff816543e3>] smpcfd_prepare_cpu+0x73/0xd0 kernel/smp.c:44
[<ffffffff814240b4>] cpuhp_invoke_callback+0x254/0x1480 kernel/cpu.c:136
[<ffffffff81425821>] cpuhp_up_callbacks+0x81/0x2a0 kernel/cpu.c:493
[<ffffffff81427bf3>] _cpu_up+0x1e3/0x2a0 kernel/cpu.c:1057
[<ffffffff81427d23>] do_cpu_up+0x73/0xa0 kernel/cpu.c:1087
[<ffffffff81427d68>] cpu_up+0x18/0x20 kernel/cpu.c:1095
[<ffffffff854ede84>] smp_init+0xe9/0xee kernel/smp.c:564
[<ffffffff85482f81>] kernel_init_freeable+0x439/0x690 init/main.c:1010
[<ffffffff84357083>] kernel_init+0x13/0x180 init/main.c:941
[<ffffffff84377baa>] ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:433
cpu_hotplug_begin
cpu_hotplug.lock
pcpu_alloc
pcpu_alloc_mutex
[<ffffffff81423012>] get_online_cpus+0x62/0x90 kernel/cpu.c:248
[<ffffffff8185fcf8>] drain_all_pages+0xf8/0x710 mm/page_alloc.c:2385
[<ffffffff81865e5d>] __alloc_pages_direct_reclaim mm/page_alloc.c:3440 [inline]
[<ffffffff81865e5d>] __alloc_pages_slowpath+0x8fd/0x2370 mm/page_alloc.c:3778
[<ffffffff818681c5>] __alloc_pages_nodemask+0x8f5/0xc60 mm/page_alloc.c:3980
[<ffffffff818ed0c1>] __alloc_pages include/linux/gfp.h:426 [inline]
[<ffffffff818ed0c1>] __alloc_pages_node include/linux/gfp.h:439 [inline]
[<ffffffff818ed0c1>] alloc_pages_node include/linux/gfp.h:453 [inline]
[<ffffffff818ed0c1>] pcpu_alloc_pages mm/percpu-vm.c:93 [inline]
[<ffffffff818ed0c1>] pcpu_populate_chunk+0x1e1/0x900 mm/percpu-vm.c:282
[<ffffffff818f0a11>] pcpu_alloc+0xe01/0x1280 mm/percpu.c:998
[<ffffffff818f0eb7>] __alloc_percpu_gfp+0x27/0x30 mm/percpu.c:1062
[<ffffffff817d25b2>] bpf_array_alloc_percpu kernel/bpf/arraymap.c:34 [inline]
[<ffffffff817d25b2>] array_map_alloc+0x532/0x710 kernel/bpf/arraymap.c:99
[<ffffffff817ba034>] find_and_alloc_map kernel/bpf/syscall.c:34 [inline]
[<ffffffff817ba034>] map_create kernel/bpf/syscall.c:188 [inline]
[<ffffffff817ba034>] SYSC_bpf kernel/bpf/syscall.c:870 [inline]
[<ffffffff817ba034>] SyS_bpf+0xd64/0x2500 kernel/bpf/syscall.c:827
[<ffffffff84377941>] entry_SYSCALL_64_fastpath+0x1f/0xc2
pcpu_alloc
pcpu_alloc_mutex
drain_all_pages
get_online_cpus
cpu_hotplug.lock
[<ffffffff81427876>] cpu_hotplug_begin+0x206/0x2e0 kernel/cpu.c:304
[<ffffffff81427ada>] _cpu_up+0xca/0x2a0 kernel/cpu.c:1011
[<ffffffff81427d23>] do_cpu_up+0x73/0xa0 kernel/cpu.c:1087
[<ffffffff81427d68>] cpu_up+0x18/0x20 kernel/cpu.c:1095
[<ffffffff854ede84>] smp_init+0xe9/0xee kernel/smp.c:564
[<ffffffff85482f81>] kernel_init_freeable+0x439/0x690 init/main.c:1010
[<ffffffff84357083>] kernel_init+0x13/0x180 init/main.c:941
[<ffffffff84377baa>] ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:433
cpu_hotplug_begin
cpu_hotplug.lock
Pulling cpu hotplug locks inside the page allocator is just too
dangerous. Let's remove the dependency by dropping get_online_cpus()
from drain_all_pages. This is not so simple though because now we do not
have a protection against cpu hotplug which means 2 things:
- the work item might be executed on a different cpu in worker
from unbound pool so it doesn't run on pinned on the cpu
- we have to make sure that we do not race with page_alloc_cpu_dead
calling drain_pages_zone
Disabling preemption in drain_local_pages_wq will solve the first
problem drain_local_pages will determine its local CPU from the WQ
context which will be stable after that point, page_alloc_cpu_dead
is pinned to the CPU already. The later condition is achieved
by disabling IRQs in drain_pages_zone.
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/page_alloc.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c3358d4f7932..b6411816787a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2343,7 +2343,16 @@ void drain_local_pages(struct zone *zone)
static void drain_local_pages_wq(struct work_struct *work)
{
+ /*
+ * drain_all_pages doesn't use proper cpu hotplug protection so
+ * we can race with cpu offline when the WQ can move this from
+ * a cpu pinned worker to an unbound one. We can operate on a different
+ * cpu which is allright but we also have to make sure to not move to
+ * a different one.
+ */
+ preempt_disable();
drain_local_pages(NULL);
+ preempt_enable();
}
/*
@@ -2379,12 +2388,6 @@ void drain_all_pages(struct zone *zone)
}
/*
- * As this can be called from reclaim context, do not reenter reclaim.
- * An allocation failure can be handled, it's simply slower
- */
- 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
@@ -2423,7 +2426,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);
}
--
2.11.0
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Mel Gorman <mgorman@techsingularity.net> |
|---|---|
| Date | 2017-02-07 17:30 +0100 |
| Message-ID | <t8gOt-5dy-7@gated-at.bofh.it> |
| In reply to | #1575807 |
On Tue, Feb 07, 2017 at 04:34:59PM +0100, Michal Hocko wrote: > > But we do not care about the whole cpu hotplug code. The only part we > > really do care about is the race inside drain_pages_zone and that will > > run in an atomic context on the specific CPU. > > > > You are absolutely right that using the mutex is safe as well but the > > hotplug path is already littered with locks and adding one more to the > > picture doesn't sound great to me. So I would really like to not use a > > lock if that is possible and safe (with a big fat comment of course). > > And with the full changelog. I hope I haven't missed anything this time. > --- > From 8c6af3116520251cc4ec2213f0a4ed2544bb4365 Mon Sep 17 00:00:00 2001 > From: Michal Hocko <mhocko@suse.com> > Date: Tue, 7 Feb 2017 16:08:35 +0100 > Subject: [PATCH] mm, page_alloc: do not depend on cpu hotplug locks inside the > allocator > > <SNIP> > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Signed-off-by: Michal Hocko <mhocko@suse.com> Not that I can think of. It's almost identical to the diff I posted with the exception of the mutex in the cpu hotplug teardown path. I agree that in the current implementation that it should be unnecessary even if I thought it would be more robust against any other hotplug churn. Acked-by: Mel Gorman <mgorman@techsingularity.net> -- Mel Gorman SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-02-07 17:50 +0100 |
| Message-ID | <t8h7Q-5ki-11@gated-at.bofh.it> |
| In reply to | #1575853 |
On Tue 07-02-17 16:22:24, Mel Gorman wrote: > On Tue, Feb 07, 2017 at 04:34:59PM +0100, Michal Hocko wrote: > > > But we do not care about the whole cpu hotplug code. The only part we > > > really do care about is the race inside drain_pages_zone and that will > > > run in an atomic context on the specific CPU. > > > > > > You are absolutely right that using the mutex is safe as well but the > > > hotplug path is already littered with locks and adding one more to the > > > picture doesn't sound great to me. So I would really like to not use a > > > lock if that is possible and safe (with a big fat comment of course). > > > > And with the full changelog. I hope I haven't missed anything this time. > > --- > > From 8c6af3116520251cc4ec2213f0a4ed2544bb4365 Mon Sep 17 00:00:00 2001 > > From: Michal Hocko <mhocko@suse.com> > > Date: Tue, 7 Feb 2017 16:08:35 +0100 > > Subject: [PATCH] mm, page_alloc: do not depend on cpu hotplug locks inside the > > allocator > > > > <SNIP> > > > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > Not that I can think of. It's almost identical to the diff I posted with > the exception of the mutex in the cpu hotplug teardown path. I agree that > in the current implementation that it should be unnecessary even if I > thought it would be more robust against any other hotplug churn. I am always nervous when seeing hotplug locks being used in low level code. It has bitten us several times already and those deadlocks are quite hard to spot when reviewing the code and very rare to hit so they tend to live for a long time. > Acked-by: Mel Gorman <mgorman@techsingularity.net> Thanks! I will wait for Tejun to confirm my assumptions are correct and post the patch to Andrew if there are no further problems spotted. Btw. this will also get rid of another lockdep report which seem to be false possitive though http://lkml.kernel.org/r/20170203145548.GC19325@dhcp22.suse.cz -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Christoph Lameter <cl@linux.com> |
|---|---|
| Date | 2017-02-07 18:00 +0100 |
| Message-ID | <t8hhw-5nU-23@gated-at.bofh.it> |
| In reply to | #1575870 |
On Tue, 7 Feb 2017, Michal Hocko wrote: > I am always nervous when seeing hotplug locks being used in low level > code. It has bitten us several times already and those deadlocks are > quite hard to spot when reviewing the code and very rare to hit so they > tend to live for a long time. Yep. Hotplug events are pretty significant. Using stop_machine_XXXX() etc would be advisable and that would avoid the taking of locks and get rid of all the ocmplexity, reduce the code size and make the overall system much more reliable. Thomas?
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-02-07 23:30 +0100 |
| Message-ID | <t8mqS-iJ-7@gated-at.bofh.it> |
| In reply to | #1575888 |
On Tue, 7 Feb 2017, Christoph Lameter wrote: > On Tue, 7 Feb 2017, Michal Hocko wrote: > > > I am always nervous when seeing hotplug locks being used in low level > > code. It has bitten us several times already and those deadlocks are > > quite hard to spot when reviewing the code and very rare to hit so they > > tend to live for a long time. > > Yep. Hotplug events are pretty significant. Using stop_machine_XXXX() etc > would be advisable and that would avoid the taking of locks and get rid of all the > ocmplexity, reduce the code size and make the overall system much more > reliable. Huch? stop_machine() is horrible and heavy weight. Don't go there, there must be simpler solutions than that. Thanks, tglx
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-02-08 08:40 +0100 |
| Message-ID | <t8v19-5P4-93@gated-at.bofh.it> |
| In reply to | #1576122 |
On Tue 07-02-17 23:25:17, Thomas Gleixner wrote: > On Tue, 7 Feb 2017, Christoph Lameter wrote: > > On Tue, 7 Feb 2017, Michal Hocko wrote: > > > > > I am always nervous when seeing hotplug locks being used in low level > > > code. It has bitten us several times already and those deadlocks are > > > quite hard to spot when reviewing the code and very rare to hit so they > > > tend to live for a long time. > > > > Yep. Hotplug events are pretty significant. Using stop_machine_XXXX() etc > > would be advisable and that would avoid the taking of locks and get rid of all the > > ocmplexity, reduce the code size and make the overall system much more > > reliable. > > Huch? stop_machine() is horrible and heavy weight. Don't go there, there > must be simpler solutions than that. Absolutely agreed. We are in the page allocator path so using the stop_machine* is just ridiculous. And, in fact, there is a much simpler solution [1] [1] http://lkml.kernel.org/r/20170207201950.20482-1-mhocko@kernel.org -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-02-08 13:10 +0100 |
| Message-ID | <t8zep-a8-5@gated-at.bofh.it> |
| In reply to | #1576315 |
On Wed, 8 Feb 2017, Michal Hocko wrote: > On Tue 07-02-17 23:25:17, Thomas Gleixner wrote: > > On Tue, 7 Feb 2017, Christoph Lameter wrote: > > > On Tue, 7 Feb 2017, Michal Hocko wrote: > > > > > > > I am always nervous when seeing hotplug locks being used in low level > > > > code. It has bitten us several times already and those deadlocks are > > > > quite hard to spot when reviewing the code and very rare to hit so they > > > > tend to live for a long time. > > > > > > Yep. Hotplug events are pretty significant. Using stop_machine_XXXX() etc > > > would be advisable and that would avoid the taking of locks and get rid of all the > > > ocmplexity, reduce the code size and make the overall system much more > > > reliable. > > > > Huch? stop_machine() is horrible and heavy weight. Don't go there, there > > must be simpler solutions than that. > > Absolutely agreed. We are in the page allocator path so using the > stop_machine* is just ridiculous. And, in fact, there is a much simpler > solution [1] > > [1] http://lkml.kernel.org/r/20170207201950.20482-1-mhocko@kernel.org Well, yes. It's simple, but from an RT point of view I really don't like it as we have to fix it up again. On RT we solved the problem of the page allocator differently which allows us to do drain_all_pages() from the caller CPU as a side effect. That's interesting not only for RT, it's also interesting for NOHZ FULL scenarios because you don't inflict the work on the other CPUs. https://git.kernel.org/cgit/linux/kernel/git/rt/linux-rt-devel.git/commit/?h=linux-4.9.y-rt-rebase&id=d577a017da694e29a06af057c517f2a7051eb305 That uses local locks (an RT speciality which compile away into preempt/irq disable/enable when RT is disabled). Works like a charm :) Thanks, tglx
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-02-08 13:30 +0100 |
| Message-ID | <t8zxM-h3-21@gated-at.bofh.it> |
| In reply to | #1576487 |
On Wed 08-02-17 13:02:07, Thomas Gleixner wrote: > On Wed, 8 Feb 2017, Michal Hocko wrote: [...] > > [1] http://lkml.kernel.org/r/20170207201950.20482-1-mhocko@kernel.org > > Well, yes. It's simple, but from an RT point of view I really don't like > it as we have to fix it up again. I thought that preempt_disable would turn into migrate_disable or something like that which shouldn't cause too much trouble. Or am I missing something? Which part of the patch is so RT unfriendly? -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Mel Gorman <mgorman@techsingularity.net> |
|---|---|
| Date | 2017-02-08 13:50 +0100 |
| Message-ID | <t8zR8-oe-33@gated-at.bofh.it> |
| In reply to | #1576487 |
On Wed, Feb 08, 2017 at 01:02:07PM +0100, Thomas Gleixner wrote: > On Wed, 8 Feb 2017, Michal Hocko wrote: > > On Tue 07-02-17 23:25:17, Thomas Gleixner wrote: > > > On Tue, 7 Feb 2017, Christoph Lameter wrote: > > > > On Tue, 7 Feb 2017, Michal Hocko wrote: > > > > > > > > > I am always nervous when seeing hotplug locks being used in low level > > > > > code. It has bitten us several times already and those deadlocks are > > > > > quite hard to spot when reviewing the code and very rare to hit so they > > > > > tend to live for a long time. > > > > > > > > Yep. Hotplug events are pretty significant. Using stop_machine_XXXX() etc > > > > would be advisable and that would avoid the taking of locks and get rid of all the > > > > ocmplexity, reduce the code size and make the overall system much more > > > > reliable. > > > > > > Huch? stop_machine() is horrible and heavy weight. Don't go there, there > > > must be simpler solutions than that. > > > > Absolutely agreed. We are in the page allocator path so using the > > stop_machine* is just ridiculous. And, in fact, there is a much simpler > > solution [1] > > > > [1] http://lkml.kernel.org/r/20170207201950.20482-1-mhocko@kernel.org > > Well, yes. It's simple, but from an RT point of view I really don't like > it as we have to fix it up again. > > On RT we solved the problem of the page allocator differently which allows > us to do drain_all_pages() from the caller CPU as a side effect. That's > interesting not only for RT, it's also interesting for NOHZ FULL scenarios > because you don't inflict the work on the other CPUs. > > https://git.kernel.org/cgit/linux/kernel/git/rt/linux-rt-devel.git/commit/?h=linux-4.9.y-rt-rebase&id=d577a017da694e29a06af057c517f2a7051eb305 > It may be worth noting that patches in Andrew's tree no longer disable interrupts in the per-cpu allocator and now per-cpu draining will be from workqueue context. The reasoning was due to the overhead of the page allocator with figures included. Interrupts will bypass the per-cpu allocator and use the irq-safe zone->lock to allocate from the core. It'll collide with the RT patch. Primary patch of interest is http://www.ozlabs.org/~akpm/mmots/broken-out/mm-page_alloc-only-use-per-cpu-allocator-for-irq-safe-requests.patch The draining from workqueue context may be a problem for RT but one option would be to move the drain to only drain for high-order pages after direct reclaim combined with only draining for order-0 if __alloc_pages_may_oom is about to be called. -- Mel Gorman SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Mel Gorman <mgorman@techsingularity.net> |
|---|---|
| Date | 2017-02-08 15:10 +0100 |
| Message-ID | <t8B6x-1k4-3@gated-at.bofh.it> |
| In reply to | #1576520 |
On Wed, Feb 08, 2017 at 02:23:19PM +0100, Thomas Gleixner wrote: > On Wed, 8 Feb 2017, Mel Gorman wrote: > > It may be worth noting that patches in Andrew's tree no longer disable > > interrupts in the per-cpu allocator and now per-cpu draining will > > be from workqueue context. The reasoning was due to the overhead of > > the page allocator with figures included. Interrupts will bypass the > > per-cpu allocator and use the irq-safe zone->lock to allocate from > > the core. It'll collide with the RT patch. Primary patch of interest is > > http://www.ozlabs.org/~akpm/mmots/broken-out/mm-page_alloc-only-use-per-cpu-allocator-for-irq-safe-requests.patch > > Yeah, we'll sort that out once it hits Linus tree and we move RT forward. > Though I have once complaint right away: > > + preempt_enable_no_resched(); > > This is a nono, even in mainline. You effectively disable a preemption > point. > This came up during review on whether it should or shouldn't be a preemption point. Initially it was preempt_enable() but a preemption point didn't exist before, the reviewer pushed for it and as it was the allocator fast path that was unlikely to need a reschedule or preempt, I made the change. I can alter it before it hits mainline if you say RT is going to have an issue with it. > > The draining from workqueue context may be a problem for RT but one > > option would be to move the drain to only drain for high-order pages > > after direct reclaim combined with only draining for order-0 if > > __alloc_pages_may_oom is about to be called. > > Why would the draining from workqueue context be an issue on RT? > It probably isn't. The latency of the operation is likely longer than an IPI was but given the context it occurs in, I severely doubted it mattered. I couldn't think of a reason why it would matter to RT but there was no harm double checking. -- Mel Gorman SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2017-02-08 18:00 +0100 |
| Message-ID | <t8DL3-2L3-3@gated-at.bofh.it> |
| In reply to | #1576594 |
On Wed, Feb 08, 2017 at 02:03:32PM +0000, Mel Gorman wrote: > > Yeah, we'll sort that out once it hits Linus tree and we move RT forward. > > Though I have once complaint right away: > > > > + preempt_enable_no_resched(); > > > > This is a nono, even in mainline. You effectively disable a preemption > > point. > > > > This came up during review on whether it should or shouldn't be a preemption > point. Initially it was preempt_enable() but a preemption point didn't > exist before, the reviewer pushed for it and as it was the allocator fast > path that was unlikely to need a reschedule or preempt, I made the change. Not relevant. The only acceptable use of preempt_enable_no_resched() is if the next statement is a schedule() variant.
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-02-08 15:10 +0100 |
| Message-ID | <t8B6x-1k4-5@gated-at.bofh.it> |
| In reply to | #1576520 |
On Wed, 8 Feb 2017, Mel Gorman wrote: > It may be worth noting that patches in Andrew's tree no longer disable > interrupts in the per-cpu allocator and now per-cpu draining will > be from workqueue context. The reasoning was due to the overhead of > the page allocator with figures included. Interrupts will bypass the > per-cpu allocator and use the irq-safe zone->lock to allocate from > the core. It'll collide with the RT patch. Primary patch of interest is > http://www.ozlabs.org/~akpm/mmots/broken-out/mm-page_alloc-only-use-per-cpu-allocator-for-irq-safe-requests.patch Yeah, we'll sort that out once it hits Linus tree and we move RT forward. Though I have once complaint right away: + preempt_enable_no_resched(); This is a nono, even in mainline. You effectively disable a preemption point. > The draining from workqueue context may be a problem for RT but one > option would be to move the drain to only drain for high-order pages > after direct reclaim combined with only draining for order-0 if > __alloc_pages_may_oom is about to be called. Why would the draining from workqueue context be an issue on RT? Thanks, tglx
[toc] | [prev] | [next] | [standalone]
| From | Christoph Lameter <cl@linux.com> |
|---|---|
| Date | 2017-02-08 16:30 +0100 |
| Message-ID | <t8ClX-20B-3@gated-at.bofh.it> |
| In reply to | #1576315 |
On Wed, 8 Feb 2017, Michal Hocko wrote: > > Huch? stop_machine() is horrible and heavy weight. Don't go there, there > > must be simpler solutions than that. > > Absolutely agreed. We are in the page allocator path so using the > stop_machine* is just ridiculous. And, in fact, there is a much simpler > solution [1] That is nonsense. stop_machine would be used when adding removing a processor. There would be no need to synchronize when looping over active cpus anymore. get_online_cpus() etc would be removed from the hot path since the cpu masks are guaranteed to be stable.
[toc] | [prev] | [next] | [standalone]
| From | Christoph Lameter <cl@linux.com> |
|---|---|
| Date | 2017-02-08 17:30 +0100 |
| Message-ID | <t8Di1-2Aw-11@gated-at.bofh.it> |
| In reply to | #1576646 |
On Wed, 8 Feb 2017, Michal Hocko wrote: > I have no idea what you are trying to say and how this is related to the > deadlock we are discussing here. We certainly do not need to add > stop_machine the problem. And yeah, dropping get_online_cpus was > possible after considering all fallouts. This is not the first time get_online_cpus() causes problems due to the need to support hotplug for processors. Hotplugging is not happening frequently (which is low balling it. Actually the frequency of the hotplug events on almost all systems is zero) so the constant check is a useless overhead and causes trouble for development. In particular get_online_cpus() is often needed in sections that need to hold locks. So lets get rid of it. The severity, frequency and rarity of processor hotplug events would justify only allowing adding and removal of processors through the stop_machine_xx mechanism. With that in place the processor masks can be used without synchronization and the locking issues all over the kernel would become simpler. It is likely that this will even improve the hotplug code because the easier form of synchronization (you have a piece of code that executed while the OS is in stop state) would allow to make more significant changes to the software environment. F.e. one could think about removing memory segments as well as maybe per cpu segments.
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-02-08 19:40 +0100 |
| Message-ID | <t8FjQ-3Oi-17@gated-at.bofh.it> |
| In reply to | #1576687 |
On Wed, 8 Feb 2017, Christoph Lameter wrote: > On Wed, 8 Feb 2017, Michal Hocko wrote: > > > I have no idea what you are trying to say and how this is related to the > > deadlock we are discussing here. We certainly do not need to add > > stop_machine the problem. And yeah, dropping get_online_cpus was > > possible after considering all fallouts. > > This is not the first time get_online_cpus() causes problems due to the > need to support hotplug for processors. Hotplugging is not happening > frequently (which is low balling it. Actually the frequency of the hotplug > events on almost all systems is zero) so the constant check is a useless > overhead and causes trouble for development. In particular There is a world outside yours. Hotplug is actually used frequently for power purposes in some scenarios. > get_online_cpus() is often needed in sections that need to hold locks. > > So lets get rid of it. The severity, frequency and rarity of processor > hotplug events would justify only allowing adding and removal of > processors through the stop_machine_xx mechanism. With that in place the > processor masks can be used without synchronization and the locking issues > all over the kernel would become simpler. > > It is likely that this will even improve the hotplug code because the > easier form of synchronization (you have a piece of code that executed > while the OS is in stop state) would allow to make more significant > changes to the software environment. F.e. one could think about removing > memory segments as well as maybe per cpu segments. It will improve nothing. The stop machine context is extremly limited and you cannot do complex things there at all. Not to talk about the inability of taking a simple mutex which would immediately deadlock the machine. stop machine is the last resort for things which need to be done atomically and that operation can be done in a very restricted context. And everything complex needs to be done _before_ that in normal context. Hot unplug already uses stop machine for the final removal of the outgoing CPU, but that's definitely not the place where you can do anything complex like page management. If you can prepare the outgoing cpu work during the cpu offline phase and then just flip a bit in the stop machine part, then this might work, but anything else is just handwaving and proliferation of wet dreams. Thanks, tglx
[toc] | [prev] | [next] | [standalone]
| From | Christoph Lameter <cl@linux.com> |
|---|---|
| Date | 2017-02-09 04:20 +0100 |
| Message-ID | <t8Nr3-BF-5@gated-at.bofh.it> |
| In reply to | #1576815 |
On Wed, 8 Feb 2017, Thomas Gleixner wrote: > There is a world outside yours. Hotplug is actually used frequently for > power purposes in some scenarios. The usual case does not inolve hotplug. > It will improve nothing. The stop machine context is extremly limited and > you cannot do complex things there at all. Not to talk about the inability > of taking a simple mutex which would immediately deadlock the machine. You do not need to do complex things. Basically flipping some cpu mask bits will do it. stop machine ensures that code is not executing on the processors when the bits are flipped. That will ensure that there is no need to do any get_online_cpu() nastiness in critical VM paths since we are guaranteed not to be executing them. > And everything complex needs to be done _before_ that in normal > context. Hot unplug already uses stop machine for the final removal of the > outgoing CPU, but that's definitely not the place where you can do anything > complex like page management. If it already does that then why do we still need get_online_cpu()? We do not do anything like page management. Why would we? We just need to ensure that nothing is executing when the bits are flipped. If that is the case then the get_online_cpu(0 calls are unecessary because the bit flipping simply cannot occur in these functions. There is nothing to serialize against. > If you can prepare the outgoing cpu work during the cpu offline phase and > then just flip a bit in the stop machine part, then this might work, but > anything else is just handwaving and proliferation of wet dreams. Fine with that.
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-02-09 12:50 +0100 |
| Message-ID | <t8VoC-5CX-13@gated-at.bofh.it> |
| In reply to | #1577279 |
On Wed, 8 Feb 2017, Christoph Lameter wrote: > On Wed, 8 Feb 2017, Thomas Gleixner wrote: > > > There is a world outside yours. Hotplug is actually used frequently for > > power purposes in some scenarios. > > The usual case does not inolve hotplug. We do not care about your definition of "usual". The kernel serves _ALL_ use cases. > > It will improve nothing. The stop machine context is extremly limited and > > you cannot do complex things there at all. Not to talk about the inability > > of taking a simple mutex which would immediately deadlock the machine. > > You do not need to do complex things. Basically flipping some cpu mask > bits will do it. stop machine ensures that code is not > executing on the processors when the bits are flipped. That will ensure > that there is no need to do any get_online_cpu() nastiness in critical VM > paths since we are guaranteed not to be executing them. And how does that solve the problem at hand? Not at all: CPU 0 CPU 1 for_each_online_cpu(cpu) ==> cpu = 1 stop_machine() set_cpu_online(1, false) queue_work(cpu1) Thanks, tglx
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web