Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1575081 > unrolled thread

Re: mm: deadlock between get_online_cpus/pcpu_alloc

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.

Contents

  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 1 of 3  [1] 2 3  Next page →

#1575081 — Re: mm: deadlock between get_online_cpus/pcpu_alloc

On Mon, Jan 30, 2017 at 4:48 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Sun, Jan 29, 2017 at 6:22 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
>> On 29.1.2017 13:44, Dmitry Vyukov wrote:
>>> Hello,
>>>
>>> I've got the following deadlock report while running syzkaller fuzzer
>>> on f37208bc3c9c2f811460ef264909dfbc7f605a60:
>>>
>>> [ INFO: possible circular locking dependency detected ]
>>> 4.10.0-rc5-next-20170125 #1 Not tainted
>>> -------------------------------------------------------
>>> syz-executor3/14255 is trying to acquire lock:
>>>  (cpu_hotplug.dep_map){++++++}, at: [<ffffffff814271c7>]
>>> get_online_cpus+0x37/0x90 kernel/cpu.c:239
>>>
>>> but task is already holding lock:
>>>  (pcpu_alloc_mutex){+.+.+.}, at: [<ffffffff81937fee>]
>>> pcpu_alloc+0xbfe/0x1290 mm/percpu.c:897
>>>
>>> which lock already depends on the new lock.
>>
>> I suspect the dependency comes from recent changes in drain_all_pages(). They
>> were later redone (for other reasons, but nice to have another validation) in
>> the mmots patch [1], which AFAICS is not yet in mmotm and thus linux-next. Could
>> you try if it helps?
>
> It happened only once on linux-next, so I can't verify the fix. But I
> will watch out for other occurrences.


Unfortunately it does not seem to help.
Fuzzer now runs on 510948533b059f4f5033464f9f4a0c32d4ab0c08 of
mmotm/auto-latest
(git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git):

commit 510948533b059f4f5033464f9f4a0c32d4ab0c08
Date:   Thu Feb 2 10:08:47 2017 +0100
    mmotm: userfaultfd-non-cooperative-add-event-for-memory-unmaps-fix

The commit you referenced is already there:

commit 806b158031ca0b4714e775898396529a758ebc2c
Date:   Thu Feb 2 08:53:16 2017 +0100
    mm, page_alloc: use static global work_struct for draining per-cpu pages

But I still got:

[ INFO: possible circular locking dependency detected ]
4.9.0 #6 Not tainted
-------------------------------------------------------
syz-executor1/8199 is trying to acquire lock:
 (cpu_hotplug.dep_map){++++++}, at: [<ffffffff81422fe7>]
get_online_cpus+0x37/0x90 kernel/cpu.c:246
but task is already holding lock:
 (pcpu_alloc_mutex){+.+.+.}, at: [<ffffffff818f07ea>]
pcpu_alloc+0xbda/0x1280 mm/percpu.c:896
which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

       [  403.953319] [<ffffffff8156fc29>] validate_chain
kernel/locking/lockdep.c:2265 [inline]
       [  403.953319] [<ffffffff8156fc29>]
__lock_acquire+0x2149/0x3430 kernel/locking/lockdep.c:3338
       [  403.961232] [<ffffffff81571db1>] lock_acquire+0x2a1/0x630
kernel/locking/lockdep.c:3753
       [  403.968788] [<ffffffff8436697e>] __mutex_lock_common
kernel/locking/mutex.c:521 [inline]
       [  403.968788] [<ffffffff8436697e>]
mutex_lock_nested+0x24e/0xff0 kernel/locking/mutex.c:621
       [  403.976782] [<ffffffff818f07ea>] pcpu_alloc+0xbda/0x1280
mm/percpu.c:896
       [  403.984266] [<ffffffff818f0ee4>] __alloc_percpu+0x24/0x30
mm/percpu.c:1075
       [  403.991873] [<ffffffff816543e3>]
smpcfd_prepare_cpu+0x73/0xd0 kernel/smp.c:44
       [  403.999799] [<ffffffff814240b4>]
cpuhp_invoke_callback+0x254/0x1480 kernel/cpu.c:136
       [  404.008253] [<ffffffff81425821>]
cpuhp_up_callbacks+0x81/0x2a0 kernel/cpu.c:493
       [  404.016365] [<ffffffff81427bf3>] _cpu_up+0x1e3/0x2a0 kernel/cpu.c:1057
       [  404.023507] [<ffffffff81427d23>] do_cpu_up+0x73/0xa0 kernel/cpu.c:1087
       [  404.030647] [<ffffffff81427d68>] cpu_up+0x18/0x20 kernel/cpu.c:1095
       [  404.037523] [<ffffffff854ede84>] smp_init+0xe9/0xee kernel/smp.c:564
       [  404.044559] [<ffffffff85482f81>]
kernel_init_freeable+0x439/0x690 init/main.c:1010
       [  404.052811] [<ffffffff84357083>] kernel_init+0x13/0x180
init/main.c:941
       [  404.060198] [<ffffffff84377baa>] ret_from_fork+0x2a/0x40
arch/x86/entry/entry_64.S:433

       [  404.072827] [<ffffffff8156fc29>] validate_chain
kernel/locking/lockdep.c:2265 [inline]
       [  404.072827] [<ffffffff8156fc29>]
__lock_acquire+0x2149/0x3430 kernel/locking/lockdep.c:3338
       [  404.080733] [<ffffffff81571db1>] lock_acquire+0x2a1/0x630
kernel/locking/lockdep.c:3753
       [  404.088311] [<ffffffff8436697e>] __mutex_lock_common
kernel/locking/mutex.c:521 [inline]
       [  404.088311] [<ffffffff8436697e>]
mutex_lock_nested+0x24e/0xff0 kernel/locking/mutex.c:621
       [  404.096318] [<ffffffff81427876>]
cpu_hotplug_begin+0x206/0x2e0 kernel/cpu.c:304
       [  404.104321] [<ffffffff81427ada>] _cpu_up+0xca/0x2a0 kernel/cpu.c:1011
       [  404.111357] [<ffffffff81427d23>] do_cpu_up+0x73/0xa0 kernel/cpu.c:1087
       [  404.118480] [<ffffffff81427d68>] cpu_up+0x18/0x20 kernel/cpu.c:1095
       [  404.125360] [<ffffffff854ede84>] smp_init+0xe9/0xee kernel/smp.c:564
       [  404.132393] [<ffffffff85482f81>]
kernel_init_freeable+0x439/0x690 init/main.c:1010
       [  404.140668] [<ffffffff84357083>] kernel_init+0x13/0x180
init/main.c:941
       [  404.148079] [<ffffffff84377baa>] ret_from_fork+0x2a/0x40
arch/x86/entry/entry_64.S:433

       [  404.160977] [<ffffffff8156976d>] check_prev_add
kernel/locking/lockdep.c:1828 [inline]
       [  404.160977] [<ffffffff8156976d>]
check_prevs_add+0xa8d/0x1c00 kernel/locking/lockdep.c:1938
       [  404.168898] [<ffffffff8156fc29>] validate_chain
kernel/locking/lockdep.c:2265 [inline]
       [  404.168898] [<ffffffff8156fc29>]
__lock_acquire+0x2149/0x3430 kernel/locking/lockdep.c:3338
       [  404.176844] [<ffffffff81571db1>] lock_acquire+0x2a1/0x630
kernel/locking/lockdep.c:3753
       [  404.184416] [<ffffffff81423012>] get_online_cpus+0x62/0x90
kernel/cpu.c:248
       [  404.192103] [<ffffffff8185fcf8>] drain_all_pages+0xf8/0x710
mm/page_alloc.c:2385
       [  404.199880] [<ffffffff81865e5d>]
__alloc_pages_direct_reclaim mm/page_alloc.c:3440 [inline]
       [  404.199880] [<ffffffff81865e5d>]
__alloc_pages_slowpath+0x8fd/0x2370 mm/page_alloc.c:3778
       [  404.208406] [<ffffffff818681c5>]
__alloc_pages_nodemask+0x8f5/0xc60 mm/page_alloc.c:3980
       [  404.216851] [<ffffffff818ed0c1>] __alloc_pages
include/linux/gfp.h:426 [inline]
       [  404.216851] [<ffffffff818ed0c1>] __alloc_pages_node
include/linux/gfp.h:439 [inline]
       [  404.216851] [<ffffffff818ed0c1>] alloc_pages_node
include/linux/gfp.h:453 [inline]
       [  404.216851] [<ffffffff818ed0c1>] pcpu_alloc_pages
mm/percpu-vm.c:93 [inline]
       [  404.216851] [<ffffffff818ed0c1>]
pcpu_populate_chunk+0x1e1/0x900 mm/percpu-vm.c:282
       [  404.225015] [<ffffffff818f0a11>] pcpu_alloc+0xe01/0x1280
mm/percpu.c:998
       [  404.232482] [<ffffffff818f0eb7>]
__alloc_percpu_gfp+0x27/0x30 mm/percpu.c:1062
       [  404.240389] [<ffffffff817d25b2>] bpf_array_alloc_percpu
kernel/bpf/arraymap.c:34 [inline]
       [  404.240389] [<ffffffff817d25b2>] array_map_alloc+0x532/0x710
kernel/bpf/arraymap.c:99
       [  404.248224] [<ffffffff817ba034>] find_and_alloc_map
kernel/bpf/syscall.c:34 [inline]
       [  404.248224] [<ffffffff817ba034>] map_create
kernel/bpf/syscall.c:188 [inline]
       [  404.248224] [<ffffffff817ba034>] SYSC_bpf
kernel/bpf/syscall.c:870 [inline]
       [  404.248224] [<ffffffff817ba034>] SyS_bpf+0xd64/0x2500
kernel/bpf/syscall.c:827
       [  404.255434] [<ffffffff84377941>] entry_SYSCALL_64_fastpath+0x1f/0xc2

other info that might help us debug this:

Chain exists of:
 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(pcpu_alloc_mutex);
                               lock(cpu_hotplug.lock);
                               lock(pcpu_alloc_mutex);
  lock(cpu_hotplug.dep_map);

 *** DEADLOCK ***

2 locks held by syz-executor1/8199:
 #0:  (pcpu_alloc_mutex){+.+.+.}, at: [<ffffffff818f07ea>]
pcpu_alloc+0xbda/0x1280 mm/percpu.c:896
 #1:  (pcpu_drain_mutex){+.+...}, at: [<ffffffff8185fcd7>]
drain_all_pages+0xd7/0x710 mm/page_alloc.c:2375

stack backtrace:
CPU: 0 PID: 8199 Comm: syz-executor1 Not tainted 4.9.0 #6
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
 ffff88017ea4e118 ffffffff8234d0df ffffffff00000000 1ffff1002fd49bb6
 ffffed002fd49bae 0000000041b58ab3 ffffffff84b38180 ffffffff8234cdf1
 ffffffff84b00510 ffffffff81560170 ffff88018ab02200 0000000041b58ab3
Call Trace:
 [<ffffffff8234d0df>] __dump_stack lib/dump_stack.c:15 [inline]
 [<ffffffff8234d0df>] dump_stack+0x2ee/0x3ef lib/dump_stack.c:51
 [<ffffffff815673e7>] print_circular_bug+0x307/0x3b0
kernel/locking/lockdep.c:1202
 [<ffffffff8156976d>] check_prev_add kernel/locking/lockdep.c:1828 [inline]
 [<ffffffff8156976d>] check_prevs_add+0xa8d/0x1c00 kernel/locking/lockdep.c:1938
 [<ffffffff8156fc29>] validate_chain kernel/locking/lockdep.c:2265 [inline]
 [<ffffffff8156fc29>] __lock_acquire+0x2149/0x3430 kernel/locking/lockdep.c:3338
 [<ffffffff81571db1>] lock_acquire+0x2a1/0x630 kernel/locking/lockdep.c:3753
 [<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
syz-executor1: page allocation failure: order:0,
mode:0x14001c2(GFP_KERNEL|__GFP_HIGHMEM|__GFP_COLD), nodemask=(null)
syz-executor1 cpuset=/ mems_allowed=0
CPU: 0 PID: 8199 Comm: syz-executor1 Not tainted 4.9.0 #6
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
 ffff88017ea4eb80 ffffffff8234d0df ffffffff00000000 1ffff1002fd49d03
 ffffed002fd49cfb 0000000041b58ab3 ffffffff84b38180 ffffffff8234cdf1
 0000000000000282 ffffffff84fd53c0 ffff8801dae65b38 ffff88017ea4e7b8
Call Trace:
 [<ffffffff8234d0df>] __dump_stack lib/dump_stack.c:15 [inline]
 [<ffffffff8234d0df>] dump_stack+0x2ee/0x3ef lib/dump_stack.c:51
 [<ffffffff8186530f>] warn_alloc+0x21f/0x360 mm/page_alloc.c:3126
 [<ffffffff818671f8>] __alloc_pages_slowpath+0x1c98/0x2370 mm/page_alloc.c:3890
 [<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

[toc] | [next] | [standalone]

#1575189

On Mon, Feb 06, 2017 at 08:13:35PM +0100, Dmitry Vyukov wrote:
> On Mon, Jan 30, 2017 at 4:48 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> > On Sun, Jan 29, 2017 at 6:22 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
> >> On 29.1.2017 13:44, Dmitry Vyukov wrote:
> >>> Hello,
> >>>
> >>> I've got the following deadlock report while running syzkaller fuzzer
> >>> on f37208bc3c9c2f811460ef264909dfbc7f605a60:
> >>>
> >>> [ INFO: possible circular locking dependency detected ]
> >>> 4.10.0-rc5-next-20170125 #1 Not tainted
> >>> -------------------------------------------------------
> >>> syz-executor3/14255 is trying to acquire lock:
> >>>  (cpu_hotplug.dep_map){++++++}, at: [<ffffffff814271c7>]
> >>> get_online_cpus+0x37/0x90 kernel/cpu.c:239
> >>>
> >>> but task is already holding lock:
> >>>  (pcpu_alloc_mutex){+.+.+.}, at: [<ffffffff81937fee>]
> >>> pcpu_alloc+0xbfe/0x1290 mm/percpu.c:897
> >>>
> >>> which lock already depends on the new lock.
> >>
> >> I suspect the dependency comes from recent changes in drain_all_pages(). They
> >> were later redone (for other reasons, but nice to have another validation) in
> >> the mmots patch [1], which AFAICS is not yet in mmotm and thus linux-next. Could
> >> you try if it helps?
> >
> > It happened only once on linux-next, so I can't verify the fix. But I
> > will watch out for other occurrences.
> 
> Unfortunately it does not seem to help.

I'm a little stuck on how to best handle this. get_online_cpus() can
halt forever if the hotplug operation is holding the mutex when calling
pcpu_alloc. One option would be to add a try_get_online_cpus() helper which
trylocks the mutex. However, given that drain is so unlikely to actually
make that make a difference when racing against parallel allocations,
I think this should be acceptable.

Any objections?

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3b93879990fd..a3192447e906 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3432,7 +3432,17 @@ __alloc_pages_direct_reclaim(gfp_t gfp_mask, unsigned int order,
 	 */
 	if (!page && !drained) {
 		unreserve_highatomic_pageblock(ac, false);
-		drain_all_pages(NULL);
+
+		/*
+		 * Only drain from contexts allocating for user allocations.
+		 * Kernel allocations could be holding a CPU hotplug-related
+		 * mutex, particularly hot-add allocating per-cpu structures
+		 * while hotplug-related mutex's are held which would prevent
+		 * get_online_cpus ever returning.
+		 */
+		if (gfp_mask & __GFP_HARDWALL)
+			drain_all_pages(NULL);
+
 		drained = true;
 		goto retry;
 	}

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575485

On Mon 06-02-17 22:05:30, Mel Gorman wrote:
> On Mon, Feb 06, 2017 at 08:13:35PM +0100, Dmitry Vyukov wrote:
> > On Mon, Jan 30, 2017 at 4:48 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> > > On Sun, Jan 29, 2017 at 6:22 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
> > >> On 29.1.2017 13:44, Dmitry Vyukov wrote:
> > >>> Hello,
> > >>>
> > >>> I've got the following deadlock report while running syzkaller fuzzer
> > >>> on f37208bc3c9c2f811460ef264909dfbc7f605a60:
> > >>>
> > >>> [ INFO: possible circular locking dependency detected ]
> > >>> 4.10.0-rc5-next-20170125 #1 Not tainted
> > >>> -------------------------------------------------------
> > >>> syz-executor3/14255 is trying to acquire lock:
> > >>>  (cpu_hotplug.dep_map){++++++}, at: [<ffffffff814271c7>]
> > >>> get_online_cpus+0x37/0x90 kernel/cpu.c:239
> > >>>
> > >>> but task is already holding lock:
> > >>>  (pcpu_alloc_mutex){+.+.+.}, at: [<ffffffff81937fee>]
> > >>> pcpu_alloc+0xbfe/0x1290 mm/percpu.c:897
> > >>>
> > >>> which lock already depends on the new lock.
> > >>
> > >> I suspect the dependency comes from recent changes in drain_all_pages(). They
> > >> were later redone (for other reasons, but nice to have another validation) in
> > >> the mmots patch [1], which AFAICS is not yet in mmotm and thus linux-next. Could
> > >> you try if it helps?
> > >
> > > It happened only once on linux-next, so I can't verify the fix. But I
> > > will watch out for other occurrences.
> > 
> > Unfortunately it does not seem to help.
> 
> I'm a little stuck on how to best handle this. get_online_cpus() can
> halt forever if the hotplug operation is holding the mutex when calling
> pcpu_alloc. One option would be to add a try_get_online_cpus() helper which
> trylocks the mutex. However, given that drain is so unlikely to actually
> make that make a difference when racing against parallel allocations,
> I think this should be acceptable.
> 
> Any objections?
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 3b93879990fd..a3192447e906 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3432,7 +3432,17 @@ __alloc_pages_direct_reclaim(gfp_t gfp_mask, unsigned int order,
>  	 */
>  	if (!page && !drained) {
>  		unreserve_highatomic_pageblock(ac, false);
> -		drain_all_pages(NULL);
> +
> +		/*
> +		 * Only drain from contexts allocating for user allocations.
> +		 * Kernel allocations could be holding a CPU hotplug-related
> +		 * mutex, particularly hot-add allocating per-cpu structures
> +		 * while hotplug-related mutex's are held which would prevent
> +		 * get_online_cpus ever returning.
> +		 */
> +		if (gfp_mask & __GFP_HARDWALL)
> +			drain_all_pages(NULL);
> +

This wouldn't work AFAICS. If you look at the lockdep splat, the path
which reverses the locking order (takes pcpu_alloc_mutex prior to
cpu_hotplug.lock is bpf_array_alloc_percpu which is GFP_USER and thus
__GFP_HARDWALL.

I believe we shouldn't pull any dependency on the hotplug locks inside
the allocator. This is just too fragile! Can we simply drop the
get_online_cpus()? Why do we need it, anyway? Say we are racing with the
cpu offlining. I have to check the code but my impression was that WQ
code will ignore the cpu requested by the work item when the cpu is
going offline. If the offline happens while the worker function already
executes then it has to wait as we run with preemption disabled so we
should be safe here. Or am I missing something obvious?

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575508

On 02/07/2017 09:48 AM, Michal Hocko wrote:
> On Mon 06-02-17 22:05:30, Mel Gorman wrote:
>>> Unfortunately it does not seem to help.
>>
>> I'm a little stuck on how to best handle this. get_online_cpus() can
>> halt forever if the hotplug operation is holding the mutex when calling
>> pcpu_alloc. One option would be to add a try_get_online_cpus() helper which
>> trylocks the mutex. However, given that drain is so unlikely to actually
>> make that make a difference when racing against parallel allocations,
>> I think this should be acceptable.
>>
>> Any objections?
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 3b93879990fd..a3192447e906 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3432,7 +3432,17 @@ __alloc_pages_direct_reclaim(gfp_t gfp_mask, unsigned int order,
>>  	 */
>>  	if (!page && !drained) {
>>  		unreserve_highatomic_pageblock(ac, false);
>> -		drain_all_pages(NULL);
>> +
>> +		/*
>> +		 * Only drain from contexts allocating for user allocations.
>> +		 * Kernel allocations could be holding a CPU hotplug-related
>> +		 * mutex, particularly hot-add allocating per-cpu structures
>> +		 * while hotplug-related mutex's are held which would prevent
>> +		 * get_online_cpus ever returning.
>> +		 */
>> +		if (gfp_mask & __GFP_HARDWALL)
>> +			drain_all_pages(NULL);
>> +
> 
> This wouldn't work AFAICS. If you look at the lockdep splat, the path
> which reverses the locking order (takes pcpu_alloc_mutex prior to
> cpu_hotplug.lock is bpf_array_alloc_percpu which is GFP_USER and thus
> __GFP_HARDWALL.
> 
> I believe we shouldn't pull any dependency on the hotplug locks inside
> the allocator. This is just too fragile! Can we simply drop the
> get_online_cpus()? Why do we need it, anyway? Say we are racing with the

It was added after I noticed in review that queue_work_on() has a
comment that caller must ensure that cpu can't go away, and wondered
about it. Also noted that a similar lru_add_drain_all() does it too.

> cpu offlining. I have to check the code but my impression was that WQ
> code will ignore the cpu requested by the work item when the cpu is
> going offline. If the offline happens while the worker function already
> executes then it has to wait as we run with preemption disabled so we
> should be safe here. Or am I missing something obvious?

Tejun suggested an alternative solution to avoiding get_online_cpus() in
this thread:
https://lkml.kernel.org/r/<20170123170329.GA7820@htj.duckdns.org>

[toc] | [prev] | [next] | [standalone]

#1575517

On Tue, Feb 07, 2017 at 10:23:31AM +0100, Vlastimil Babka wrote:
> 
> > cpu offlining. I have to check the code but my impression was that WQ
> > code will ignore the cpu requested by the work item when the cpu is
> > going offline. If the offline happens while the worker function already
> > executes then it has to wait as we run with preemption disabled so we
> > should be safe here. Or am I missing something obvious?
> 
> Tejun suggested an alternative solution to avoiding get_online_cpus() in
> this thread:
> https://lkml.kernel.org/r/<20170123170329.GA7820@htj.duckdns.org>

I was hoping it would not really be necessary to sync against the cpu
offline callback if we know we don't really have to guarantee we run on
the correct CPU.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575538

On Tue 07-02-17 10:23:31, Vlastimil Babka wrote:
> On 02/07/2017 09:48 AM, Michal Hocko wrote:
> > On Mon 06-02-17 22:05:30, Mel Gorman wrote:
> >>> Unfortunately it does not seem to help.
> >>
> >> I'm a little stuck on how to best handle this. get_online_cpus() can
> >> halt forever if the hotplug operation is holding the mutex when calling
> >> pcpu_alloc. One option would be to add a try_get_online_cpus() helper which
> >> trylocks the mutex. However, given that drain is so unlikely to actually
> >> make that make a difference when racing against parallel allocations,
> >> I think this should be acceptable.
> >>
> >> Any objections?
> >>
> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> >> index 3b93879990fd..a3192447e906 100644
> >> --- a/mm/page_alloc.c
> >> +++ b/mm/page_alloc.c
> >> @@ -3432,7 +3432,17 @@ __alloc_pages_direct_reclaim(gfp_t gfp_mask, unsigned int order,
> >>  	 */
> >>  	if (!page && !drained) {
> >>  		unreserve_highatomic_pageblock(ac, false);
> >> -		drain_all_pages(NULL);
> >> +
> >> +		/*
> >> +		 * Only drain from contexts allocating for user allocations.
> >> +		 * Kernel allocations could be holding a CPU hotplug-related
> >> +		 * mutex, particularly hot-add allocating per-cpu structures
> >> +		 * while hotplug-related mutex's are held which would prevent
> >> +		 * get_online_cpus ever returning.
> >> +		 */
> >> +		if (gfp_mask & __GFP_HARDWALL)
> >> +			drain_all_pages(NULL);
> >> +
> > 
> > This wouldn't work AFAICS. If you look at the lockdep splat, the path
> > which reverses the locking order (takes pcpu_alloc_mutex prior to
> > cpu_hotplug.lock is bpf_array_alloc_percpu which is GFP_USER and thus
> > __GFP_HARDWALL.
> > 
> > I believe we shouldn't pull any dependency on the hotplug locks inside
> > the allocator. This is just too fragile! Can we simply drop the
> > get_online_cpus()? Why do we need it, anyway? Say we are racing with the
> 
> It was added after I noticed in review that queue_work_on() has a
> comment that caller must ensure that cpu can't go away, and wondered
> about it.

Ohh, I haven't noticed the comment. Thanks for pointing it out. I still
do not see what would a missing get_online_cpus mean for queuing.

> Also noted that a similar lru_add_drain_all() does it too.
> 
> > cpu offlining. I have to check the code but my impression was that WQ
> > code will ignore the cpu requested by the work item when the cpu is
> > going offline. If the offline happens while the worker function already
> > executes then it has to wait as we run with preemption disabled so we
> > should be safe here. Or am I missing something obvious?
> 
> Tejun suggested an alternative solution to avoiding get_online_cpus() in
> this thread:
> https://lkml.kernel.org/r/<20170123170329.GA7820@htj.duckdns.org>

OK, so we have page_alloc_cpu_notify which also does drain_pages so all
we have to do to make sure they do not race is to synchronize there.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575556

On Tue, Feb 07, 2017 at 10:23:31AM +0100, Vlastimil Babka wrote:
> > cpu offlining. I have to check the code but my impression was that WQ
> > code will ignore the cpu requested by the work item when the cpu is
> > going offline. If the offline happens while the worker function already
> > executes then it has to wait as we run with preemption disabled so we
> > should be safe here. Or am I missing something obvious?
> 
> Tejun suggested an alternative solution to avoiding get_online_cpus() in
> this thread:
> https://lkml.kernel.org/r/<20170123170329.GA7820@htj.duckdns.org>

But it would look like the following as it could be serialised against
pcpu_drain_mutex as the cpu hotplug teardown callback is allowed to sleep.

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3b93879990fd..8cd8b1bbe00c 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2319,9 +2319,17 @@ static void drain_pages(unsigned int cpu)
 {
 	struct zone *zone;
 
+	/*
+	 * 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);
 	for_each_populated_zone(zone) {
 		drain_pages_zone(cpu, zone);
 	}
+	mutex_unlock(&pcpu_drain_mutex);
 }
 
 /*
@@ -2377,13 +2385,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 +2423,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);
 }
 

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575567

On Tue, Feb 07, 2017 at 10:42:49AM +0000, Mel Gorman wrote:
> On Tue, Feb 07, 2017 at 10:23:31AM +0100, Vlastimil Babka wrote:
> > > cpu offlining. I have to check the code but my impression was that WQ
> > > code will ignore the cpu requested by the work item when the cpu is
> > > going offline. If the offline happens while the worker function already
> > > executes then it has to wait as we run with preemption disabled so we
> > > should be safe here. Or am I missing something obvious?
> > 
> > Tejun suggested an alternative solution to avoiding get_online_cpus() in
> > this thread:
> > https://lkml.kernel.org/r/<20170123170329.GA7820@htj.duckdns.org>
> 
> But it would look like the following as it could be serialised against
> pcpu_drain_mutex as the cpu hotplug teardown callback is allowed to sleep.
> 

Bah, this is obviously unsafe. It's guaranteed to deadlock.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575518

On Tue, Feb 07, 2017 at 09:48:56AM +0100, Michal Hocko wrote:
> > +
> > +		/*
> > +		 * Only drain from contexts allocating for user allocations.
> > +		 * Kernel allocations could be holding a CPU hotplug-related
> > +		 * mutex, particularly hot-add allocating per-cpu structures
> > +		 * while hotplug-related mutex's are held which would prevent
> > +		 * get_online_cpus ever returning.
> > +		 */
> > +		if (gfp_mask & __GFP_HARDWALL)
> > +			drain_all_pages(NULL);
> > +
> 
> This wouldn't work AFAICS. If you look at the lockdep splat, the path
> which reverses the locking order (takes pcpu_alloc_mutex prior to
> cpu_hotplug.lock is bpf_array_alloc_percpu which is GFP_USER and thus
> __GFP_HARDWALL.
> 

You're right, I looked at the wrong caller.

> I believe we shouldn't pull any dependency on the hotplug locks inside
> the allocator. This is just too fragile! Can we simply drop the
> get_online_cpus()? Why do we need it, anyway?

To stop the CPU being queued going offline. Another alternative is bringing
back try_get_offline_cpus which Peter pointed out to be offline was removed
in commit 02ef3c4a2aae65a1632b27770bfea3f83ca06772 although it'd be a
shame for just this case.

> Say we are racing with the
> cpu offlining. I have to check the code but my impression was that WQ
> code will ignore the cpu requested by the work item when the cpu is
> going offline. If the offline happens while the worker function already
> executes then it has to wait as we run with preemption disabled so we
> should be safe here. Or am I missing something obvious?

It may be safe but I'd prefer to get confirmation from Tejun. If it happens
that a drain on a particular CPU gets missed in this path, it's not the
end of the world as the CPU offlining drains the pages in another path so
nothing gets lost.

It would also be acceptable if one CPU was drained twice because the
workqueue was unbound from a CPU being hot-removed and moved during a
hotplug operation.

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.

Tejun, did I miss anything? Does a workqueue item queued on a CPU being
offline get unbound and a caller can still flush it safely? In this
specific case, it's ok that the workqueue item does not run on the CPU it
was queued on.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575537

On 02/07/2017 10:43 AM, Mel Gorman wrote:
> 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.

> Tejun, did I miss anything? Does a workqueue item queued on a CPU being
> offline get unbound and a caller can still flush it safely? In this
> specific case, it's ok that the workqueue item does not run on the CPU it
> was queued on.
> 

[toc] | [prev] | [next] | [standalone]

#1575539

On Tue 07-02-17 10:49:28, Vlastimil Babka wrote:
> On 02/07/2017 10:43 AM, Mel Gorman wrote:
> > 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.

Yeah I guess you are right. This would mean that drain_local_pages_wq
should to preempt_{disable,enable} around drain_local_pages
> 
> > Tejun, did I miss anything? Does a workqueue item queued on a CPU being
> > offline get unbound and a caller can still flush it safely? In this
> > specific case, it's ok that the workqueue item does not run on the CPU it
> > was queued on.

I guess we need to do one more step and ensure that our (rebound) worker
doesn't race with the page_alloc_cpu_notify. I guess we can just cmpxchg
pcp->count in drain_pages_zone to ensure the exclusivity. Not as simple
as I originally thought but doable I guess and definitely better than
making a subtle dependency on the hotplug locks which is just a PITA to
maintain.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575547

On Tue, Feb 07, 2017 at 10:49:28AM +0100, Vlastimil Babka wrote:
> On 02/07/2017 10:43 AM, Mel Gorman wrote:
> > 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)

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575550

On Tue 07-02-17 10:28:09, Mel Gorman wrote:
> On Tue, Feb 07, 2017 at 10:49:28AM +0100, Vlastimil Babka wrote:
> > On 02/07/2017 10:43 AM, Mel Gorman wrote:
> > > 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.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575578

On Tue, Feb 07, 2017 at 11:35:52AM +0100, Michal Hocko wrote:
> On Tue 07-02-17 10:28:09, Mel Gorman wrote:
> > On Tue, Feb 07, 2017 at 10:49:28AM +0100, Vlastimil Babka wrote:
> > > On 02/07/2017 10:43 AM, Mel Gorman wrote:
> > > > 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

[toc] | [prev] | [next] | [standalone]

#1575591

On Tue 07-02-17 11:34:35, Mel Gorman wrote:
> On Tue, Feb 07, 2017 at 11:35:52AM +0100, Michal Hocko wrote:
> > On Tue 07-02-17 10:28:09, Mel Gorman wrote:
> > > On Tue, Feb 07, 2017 at 10:49:28AM +0100, Vlastimil Babka wrote:
> > > > On 02/07/2017 10:43 AM, Mel Gorman wrote:
> > > > > 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();
>  }
>  
>  /*
[...]
> @@ -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);

You cannot put sleepable lock inside the preempt disbaled section...
We can make it a spinlock right?

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575593

On 02/07/2017 12:43 PM, Michal Hocko wrote:
>> 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();
>>  }
>>
>>  /*
> [...]
>> @@ -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);
>
> You cannot put sleepable lock inside the preempt disbaled section...
> We can make it a spinlock right?

Could we do flush_work() with a spinlock? Sounds bad too.
Maybe we could just use the fact that the whole drain happens with disabled 
irq's and obtain the current cpu under that protection?

[toc] | [prev] | [next] | [standalone]

#1575601

On Tue 07-02-17 12:54:48, Vlastimil Babka wrote:
> On 02/07/2017 12:43 PM, Michal Hocko wrote:
> > > 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();
> > >  }
> > > 
> > >  /*
> > [...]
> > > @@ -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);
> > 
> > You cannot put sleepable lock inside the preempt disbaled section...
> > We can make it a spinlock right?
> 
> Could we do flush_work() with a spinlock? Sounds bad too.

We surely cannot. I thought the lock would be gone in drain_all_pages,
we would deadlock with the lock there anyway... But it is true that we
would need a way to only allow one caller to get in. This is getting
messier and messier...

> Maybe we could just use the fact that the whole drain happens with disabled
> irq's and obtain the current cpu under that protection?

preempt_disable should be enough, no? The CPU callback is not called
from an IRQ context, right?
---
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1ee49474207e..4a9a65479435 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);
 }
 
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575625

On Tue 07-02-17 12:43:27, Michal Hocko wrote:
> On Tue 07-02-17 11:34:35, Mel Gorman wrote:
> > On Tue, Feb 07, 2017 at 11:35:52AM +0100, Michal Hocko wrote:
> > > On Tue 07-02-17 10:28:09, Mel Gorman wrote:
> > > > On Tue, Feb 07, 2017 at 10:49:28AM +0100, Vlastimil Babka wrote:
> > > > > On 02/07/2017 10:43 AM, Mel Gorman wrote:
> > > > > > 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();
> >  }
> >  
> >  /*
> [...]
> > @@ -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);
> 
> 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. 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?

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1575631

On 02/07/2017 01:37 PM, Michal Hocko wrote:
>> > @@ -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);
>>
>> 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.

Good.

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

[toc] | [prev] | [next] | [standalone]

#1575637

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?

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web