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

Groups > linux.kernel > #1711949 > unrolled thread

Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature

Started byIngo Molnar <mingo@kernel.org>
First post2017-08-15 10:30 +0200
Last post2017-08-17 10:40 +0200
Articles 15 — 3 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.

Contents

  Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Ingo Molnar <mingo@kernel.org> - 2017-08-15 10:30 +0200
    Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-16 02:20 +0200
      Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Boqun Feng <boqun.feng@gmail.com> - 2017-08-16 06:10 +0200
        Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-16 06:40 +0200
          Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Boqun Feng <boqun.feng@gmail.com> - 2017-08-16 07:50 +0200
            Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-16 08:40 +0200
        Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-16 07:10 +0200
          Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Boqun Feng <boqun.feng@gmail.com> - 2017-08-16 08:00 +0200
            Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-16 09:20 +0200
              Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-16 10:10 +0200
                Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-16 11:50 +0200
        Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Ingo Molnar <mingo@kernel.org> - 2017-08-17 09:50 +0200
          Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Boqun Feng <boqun.feng@gmail.com> - 2017-08-17 10:10 +0200
            Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Ingo Molnar <mingo@kernel.org> - 2017-08-17 10:20 +0200
              Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Boqun Feng <boqun.feng@gmail.com> - 2017-08-17 10:40 +0200

#1711949 — Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature

FromIngo Molnar <mingo@kernel.org>
Date2017-08-15 10:30 +0200
SubjectRe: [PATCH v8 00/14] lockdep: Implement crossrelease feature
Message-ID<ueFoB-7kE-1@gated-at.bofh.it>
So with the latest fixes there's a new lockdep warning on one of my testboxes:

[   11.322487] EXT4-fs (sda2): mounted filesystem with ordered data mode. Opts: (null)

[   11.495661] ======================================================
[   11.502093] WARNING: possible circular locking dependency detected
[   11.508507] 4.13.0-rc5-00497-g73135c58-dirty #1 Not tainted
[   11.514313] ------------------------------------------------------
[   11.520725] umount/533 is trying to acquire lock:
[   11.525657]  ((complete)&barr->done){+.+.}, at: [<ffffffff810fdbb3>] flush_work+0x213/0x2f0
[   11.534411] 
               but task is already holding lock:
[   11.540661]  (lock#3){+.+.}, at: [<ffffffff8122678d>] lru_add_drain_all_cpuslocked+0x3d/0x190
[   11.549613] 
               which lock already depends on the new lock.

The full splat is below. The kernel config is nothing fancy - distro derived, 
pretty close to defconfig, with lockdep enabled.

Thanks,

	Ingo

[   11.322487] EXT4-fs (sda2): mounted filesystem with ordered data mode. Opts: (null)

[   11.495661] ======================================================
[   11.502093] WARNING: possible circular locking dependency detected
[   11.508507] 4.13.0-rc5-00497-g73135c58-dirty #1 Not tainted
[   11.514313] ------------------------------------------------------
[   11.520725] umount/533 is trying to acquire lock:
[   11.525657]  ((complete)&barr->done){+.+.}, at: [<ffffffff810fdbb3>] flush_work+0x213/0x2f0
[   11.534411] 
               but task is already holding lock:
[   11.540661]  (lock#3){+.+.}, at: [<ffffffff8122678d>] lru_add_drain_all_cpuslocked+0x3d/0x190
[   11.549613] 
               which lock already depends on the new lock.

[   11.558349] 
               the existing dependency chain (in reverse order) is:
[   11.566229] 
               -> #3 (lock#3){+.+.}:
[   11.571439]        lock_acquire+0xe7/0x1d0
[   11.575765]        __mutex_lock+0x75/0x8e0
[   11.580086]        lru_add_drain_all_cpuslocked+0x3d/0x190
[   11.585797]        lru_add_drain_all+0xf/0x20
[   11.590402]        invalidate_bdev+0x3e/0x60
[   11.594901]        ext4_put_super+0x1f9/0x3d0
[   11.599485]        generic_shutdown_super+0x64/0x110
[   11.604685]        kill_block_super+0x21/0x50
[   11.609270]        deactivate_locked_super+0x39/0x70
[   11.614462]        cleanup_mnt+0x3b/0x70
[   11.618612]        task_work_run+0x72/0x90
[   11.622955]        exit_to_usermode_loop+0x93/0xa0
[   11.627971]        do_syscall_64+0x1a2/0x1c0
[   11.632470]        return_from_SYSCALL_64+0x0/0x7a
[   11.637487] 
               -> #2 (cpu_hotplug_lock.rw_sem){++++}:
[   11.644144]        lock_acquire+0xe7/0x1d0
[   11.648487]        cpus_read_lock+0x2b/0x60
[   11.652897]        apply_workqueue_attrs+0x12/0x50
[   11.657917]        __alloc_workqueue_key+0x2f2/0x510
[   11.663110]        scsi_host_alloc+0x353/0x470
[   11.667780]        _scsih_probe+0x5bb/0x7b0
[   11.672192]        local_pci_probe+0x3f/0x90
[   11.676714]        work_for_cpu_fn+0x10/0x20
[   11.681213]        process_one_work+0x1fc/0x670
[   11.685971]        worker_thread+0x219/0x3e0
[   11.690469]        kthread+0x13a/0x170
[   11.694465]        ret_from_fork+0x27/0x40
[   11.698790] 
               -> #1 ((&wfc.work)){+.+.}:
[   11.704433]        worker_thread+0x219/0x3e0
[   11.708930]        kthread+0x13a/0x170
[   11.712908]        ret_from_fork+0x27/0x40
[   11.717234]        0xffffffffffffffff
[   11.721142] 
               -> #0 ((complete)&barr->done){+.+.}:
[   11.727633]        __lock_acquire+0x1433/0x14a0
[   11.732392]        lock_acquire+0xe7/0x1d0
[   11.736715]        wait_for_completion+0x4e/0x170
[   11.741664]        flush_work+0x213/0x2f0
[   11.745919]        lru_add_drain_all_cpuslocked+0x149/0x190
[   11.751718]        lru_add_drain_all+0xf/0x20
[   11.756303]        invalidate_bdev+0x3e/0x60
[   11.760819]        ext4_put_super+0x1f9/0x3d0
[   11.765403]        generic_shutdown_super+0x64/0x110
[   11.770596]        kill_block_super+0x21/0x50
[   11.775181]        deactivate_locked_super+0x39/0x70
[   11.780372]        cleanup_mnt+0x3b/0x70
[   11.784522]        task_work_run+0x72/0x90
[   11.788848]        exit_to_usermode_loop+0x93/0xa0
[   11.793875]        do_syscall_64+0x1a2/0x1c0
[   11.798399]        return_from_SYSCALL_64+0x0/0x7a
[   11.803416] 
               other info that might help us debug this:

[   11.811997] Chain exists of:
                 (complete)&barr->done --> cpu_hotplug_lock.rw_sem --> lock#3

[   11.823810]  Possible unsafe locking scenario:

[   11.830120]        CPU0                    CPU1
[   11.834878]        ----                    ----
[   11.839636]   lock(lock#3);
[   11.842653]                                lock(cpu_hotplug_lock.rw_sem);
[   11.849697]                                lock(lock#3);
[   11.855236]   lock((complete)&barr->done);
[   11.859560] 
                *** DEADLOCK ***

[   11.866054] 3 locks held by umount/533:
[   11.870117]  #0:  (&type->s_umount_key#24){+.+.}, at: [<ffffffff8129b7ad>] deactivate_super+0x4d/0x60
[   11.879737]  #1:  (cpu_hotplug_lock.rw_sem){++++}, at: [<ffffffff812268ea>] lru_add_drain_all+0xa/0x20
[   11.889445]  #2:  (lock#3){+.+.}, at: [<ffffffff8122678d>] lru_add_drain_all_cpuslocked+0x3d/0x190
[   11.898805] 
               stack backtrace:
[   11.903573] CPU: 12 PID: 533 Comm: umount Not tainted 4.13.0-rc5-00497-g73135c58-dirty #1
[   11.912169] Hardware name: Supermicro H8DG6/H8DGi/H8DG6/H8DGi, BIOS 2.0b       03/01/2012
[   11.920759] Call Trace:
[   11.923433]  dump_stack+0x5e/0x8e
[   11.926975]  print_circular_bug+0x204/0x310
[   11.931385]  ? add_lock_to_list.isra.29+0xb0/0xb0
[   11.936316]  check_prev_add+0x444/0x860
[   11.940382]  ? generic_shutdown_super+0x64/0x110
[   11.945237]  ? add_lock_to_list.isra.29+0xb0/0xb0
[   11.950168]  ? __lock_acquire+0x1433/0x14a0
[   11.954578]  __lock_acquire+0x1433/0x14a0
[   11.958818]  lock_acquire+0xe7/0x1d0
[   11.962621]  ? flush_work+0x213/0x2f0
[   11.966506]  wait_for_completion+0x4e/0x170
[   11.970915]  ? flush_work+0x213/0x2f0
[   11.974807]  ? flush_work+0x1e6/0x2f0
[   11.978699]  flush_work+0x213/0x2f0
[   11.982416]  ? flush_workqueue_prep_pwqs+0x1b0/0x1b0
[   11.987610]  ? mark_held_locks+0x66/0x90
[   11.991778]  ? queue_work_on+0x41/0x70
[   11.995755]  lru_add_drain_all_cpuslocked+0x149/0x190
[   12.001034]  lru_add_drain_all+0xf/0x20
[   12.005124]  invalidate_bdev+0x3e/0x60
[   12.009094]  ext4_put_super+0x1f9/0x3d0
[   12.013159]  generic_shutdown_super+0x64/0x110
[   12.017856]  kill_block_super+0x21/0x50
[   12.021922]  deactivate_locked_super+0x39/0x70
[   12.026591]  cleanup_mnt+0x3b/0x70
[   12.030242]  task_work_run+0x72/0x90
[   12.034063]  exit_to_usermode_loop+0x93/0xa0
[   12.038561]  do_syscall_64+0x1a2/0x1c0
[   12.042541]  entry_SYSCALL64_slow_path+0x25/0x25
[   12.047384] RIP: 0033:0x7fc3f2854a37
[   12.051189] RSP: 002b:00007fff660582b8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a6
[   12.059162] RAX: 0000000000000000 RBX: 00000074471c14e0 RCX: 00007fc3f2854a37
[   12.066530] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000074471c22e0
[   12.073895] RBP: 00000074471c22e0 R08: 0000000000000000 R09: 0000000000000002
[   12.081264] R10: 00007fff66058050 R11: 0000000000000246 R12: 00007fc3f35e6890
[   12.088656] R13: 0000000000000000 R14: 00000074471c1660 R15: 0000000000000000
[   12.110307] dracut: Checking ext4: /dev/sda2

[toc] | [next] | [standalone]

#1712536

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-16 02:20 +0200
Message-ID<ueUdX-8jK-9@gated-at.bofh.it>
In reply to#1711949
On Tue, Aug 15, 2017 at 10:20:20AM +0200, Ingo Molnar wrote:
> 
> So with the latest fixes there's a new lockdep warning on one of my testboxes:
> 
> [   11.322487] EXT4-fs (sda2): mounted filesystem with ordered data mode. Opts: (null)
> 
> [   11.495661] ======================================================
> [   11.502093] WARNING: possible circular locking dependency detected
> [   11.508507] 4.13.0-rc5-00497-g73135c58-dirty #1 Not tainted
> [   11.514313] ------------------------------------------------------
> [   11.520725] umount/533 is trying to acquire lock:
> [   11.525657]  ((complete)&barr->done){+.+.}, at: [<ffffffff810fdbb3>] flush_work+0x213/0x2f0
> [   11.534411] 
>                but task is already holding lock:
> [   11.540661]  (lock#3){+.+.}, at: [<ffffffff8122678d>] lru_add_drain_all_cpuslocked+0x3d/0x190
> [   11.549613] 
>                which lock already depends on the new lock.
> 
> The full splat is below. The kernel config is nothing fancy - distro derived, 
> pretty close to defconfig, with lockdep enabled.

I see...

Worker A : acquired of wfc.work -> wait for cpu_hotplug_lock to be released
Task   B : acquired of cpu_hotplug_lock -> wait for lock#3 to be released
Task   C : acquired of lock#3 -> wait for completion of barr->done
Worker D : wait for wfc.work to be released -> will complete barr->done

The report below is telling that a deadlock would happen if the four tasks
run simultaniously. Here, I wonder if wfc.work sould be acquired with a
write version. I am not familiar with workqueue. Could anyone explain it
for me?

Thank you,
Byungchul

> Thanks,
> 
> 	Ingo
> 
> [   11.322487] EXT4-fs (sda2): mounted filesystem with ordered data mode. Opts: (null)
> 
> [   11.495661] ======================================================
> [   11.502093] WARNING: possible circular locking dependency detected
> [   11.508507] 4.13.0-rc5-00497-g73135c58-dirty #1 Not tainted
> [   11.514313] ------------------------------------------------------
> [   11.520725] umount/533 is trying to acquire lock:
> [   11.525657]  ((complete)&barr->done){+.+.}, at: [<ffffffff810fdbb3>] flush_work+0x213/0x2f0
> [   11.534411] 
>                but task is already holding lock:
> [   11.540661]  (lock#3){+.+.}, at: [<ffffffff8122678d>] lru_add_drain_all_cpuslocked+0x3d/0x190
> [   11.549613] 
>                which lock already depends on the new lock.
> 
> [   11.558349] 
>                the existing dependency chain (in reverse order) is:
> [   11.566229] 
>                -> #3 (lock#3){+.+.}:
> [   11.571439]        lock_acquire+0xe7/0x1d0
> [   11.575765]        __mutex_lock+0x75/0x8e0
> [   11.580086]        lru_add_drain_all_cpuslocked+0x3d/0x190
> [   11.585797]        lru_add_drain_all+0xf/0x20
> [   11.590402]        invalidate_bdev+0x3e/0x60
> [   11.594901]        ext4_put_super+0x1f9/0x3d0
> [   11.599485]        generic_shutdown_super+0x64/0x110
> [   11.604685]        kill_block_super+0x21/0x50
> [   11.609270]        deactivate_locked_super+0x39/0x70
> [   11.614462]        cleanup_mnt+0x3b/0x70
> [   11.618612]        task_work_run+0x72/0x90
> [   11.622955]        exit_to_usermode_loop+0x93/0xa0
> [   11.627971]        do_syscall_64+0x1a2/0x1c0
> [   11.632470]        return_from_SYSCALL_64+0x0/0x7a
> [   11.637487] 
>                -> #2 (cpu_hotplug_lock.rw_sem){++++}:
> [   11.644144]        lock_acquire+0xe7/0x1d0
> [   11.648487]        cpus_read_lock+0x2b/0x60
> [   11.652897]        apply_workqueue_attrs+0x12/0x50
> [   11.657917]        __alloc_workqueue_key+0x2f2/0x510
> [   11.663110]        scsi_host_alloc+0x353/0x470
> [   11.667780]        _scsih_probe+0x5bb/0x7b0
> [   11.672192]        local_pci_probe+0x3f/0x90
> [   11.676714]        work_for_cpu_fn+0x10/0x20
> [   11.681213]        process_one_work+0x1fc/0x670
> [   11.685971]        worker_thread+0x219/0x3e0
> [   11.690469]        kthread+0x13a/0x170
> [   11.694465]        ret_from_fork+0x27/0x40
> [   11.698790] 
>                -> #1 ((&wfc.work)){+.+.}:
> [   11.704433]        worker_thread+0x219/0x3e0
> [   11.708930]        kthread+0x13a/0x170
> [   11.712908]        ret_from_fork+0x27/0x40
> [   11.717234]        0xffffffffffffffff
> [   11.721142] 
>                -> #0 ((complete)&barr->done){+.+.}:
> [   11.727633]        __lock_acquire+0x1433/0x14a0
> [   11.732392]        lock_acquire+0xe7/0x1d0
> [   11.736715]        wait_for_completion+0x4e/0x170
> [   11.741664]        flush_work+0x213/0x2f0
> [   11.745919]        lru_add_drain_all_cpuslocked+0x149/0x190
> [   11.751718]        lru_add_drain_all+0xf/0x20
> [   11.756303]        invalidate_bdev+0x3e/0x60
> [   11.760819]        ext4_put_super+0x1f9/0x3d0
> [   11.765403]        generic_shutdown_super+0x64/0x110
> [   11.770596]        kill_block_super+0x21/0x50
> [   11.775181]        deactivate_locked_super+0x39/0x70
> [   11.780372]        cleanup_mnt+0x3b/0x70
> [   11.784522]        task_work_run+0x72/0x90
> [   11.788848]        exit_to_usermode_loop+0x93/0xa0
> [   11.793875]        do_syscall_64+0x1a2/0x1c0
> [   11.798399]        return_from_SYSCALL_64+0x0/0x7a
> [   11.803416] 
>                other info that might help us debug this:
> 
> [   11.811997] Chain exists of:
>                  (complete)&barr->done --> cpu_hotplug_lock.rw_sem --> lock#3
> 
> [   11.823810]  Possible unsafe locking scenario:
> 
> [   11.830120]        CPU0                    CPU1
> [   11.834878]        ----                    ----
> [   11.839636]   lock(lock#3);
> [   11.842653]                                lock(cpu_hotplug_lock.rw_sem);
> [   11.849697]                                lock(lock#3);
> [   11.855236]   lock((complete)&barr->done);
> [   11.859560] 
>                 *** DEADLOCK ***
> 
> [   11.866054] 3 locks held by umount/533:
> [   11.870117]  #0:  (&type->s_umount_key#24){+.+.}, at: [<ffffffff8129b7ad>] deactivate_super+0x4d/0x60
> [   11.879737]  #1:  (cpu_hotplug_lock.rw_sem){++++}, at: [<ffffffff812268ea>] lru_add_drain_all+0xa/0x20
> [   11.889445]  #2:  (lock#3){+.+.}, at: [<ffffffff8122678d>] lru_add_drain_all_cpuslocked+0x3d/0x190
> [   11.898805] 
>                stack backtrace:
> [   11.903573] CPU: 12 PID: 533 Comm: umount Not tainted 4.13.0-rc5-00497-g73135c58-dirty #1
> [   11.912169] Hardware name: Supermicro H8DG6/H8DGi/H8DG6/H8DGi, BIOS 2.0b       03/01/2012
> [   11.920759] Call Trace:
> [   11.923433]  dump_stack+0x5e/0x8e
> [   11.926975]  print_circular_bug+0x204/0x310
> [   11.931385]  ? add_lock_to_list.isra.29+0xb0/0xb0
> [   11.936316]  check_prev_add+0x444/0x860
> [   11.940382]  ? generic_shutdown_super+0x64/0x110
> [   11.945237]  ? add_lock_to_list.isra.29+0xb0/0xb0
> [   11.950168]  ? __lock_acquire+0x1433/0x14a0
> [   11.954578]  __lock_acquire+0x1433/0x14a0
> [   11.958818]  lock_acquire+0xe7/0x1d0
> [   11.962621]  ? flush_work+0x213/0x2f0
> [   11.966506]  wait_for_completion+0x4e/0x170
> [   11.970915]  ? flush_work+0x213/0x2f0
> [   11.974807]  ? flush_work+0x1e6/0x2f0
> [   11.978699]  flush_work+0x213/0x2f0
> [   11.982416]  ? flush_workqueue_prep_pwqs+0x1b0/0x1b0
> [   11.987610]  ? mark_held_locks+0x66/0x90
> [   11.991778]  ? queue_work_on+0x41/0x70
> [   11.995755]  lru_add_drain_all_cpuslocked+0x149/0x190
> [   12.001034]  lru_add_drain_all+0xf/0x20
> [   12.005124]  invalidate_bdev+0x3e/0x60
> [   12.009094]  ext4_put_super+0x1f9/0x3d0
> [   12.013159]  generic_shutdown_super+0x64/0x110
> [   12.017856]  kill_block_super+0x21/0x50
> [   12.021922]  deactivate_locked_super+0x39/0x70
> [   12.026591]  cleanup_mnt+0x3b/0x70
> [   12.030242]  task_work_run+0x72/0x90
> [   12.034063]  exit_to_usermode_loop+0x93/0xa0
> [   12.038561]  do_syscall_64+0x1a2/0x1c0
> [   12.042541]  entry_SYSCALL64_slow_path+0x25/0x25
> [   12.047384] RIP: 0033:0x7fc3f2854a37
> [   12.051189] RSP: 002b:00007fff660582b8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a6
> [   12.059162] RAX: 0000000000000000 RBX: 00000074471c14e0 RCX: 00007fc3f2854a37
> [   12.066530] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000074471c22e0
> [   12.073895] RBP: 00000074471c22e0 R08: 0000000000000000 R09: 0000000000000002
> [   12.081264] R10: 00007fff66058050 R11: 0000000000000246 R12: 00007fc3f35e6890
> [   12.088656] R13: 0000000000000000 R14: 00000074471c1660 R15: 0000000000000000
> [   12.110307] dracut: Checking ext4: /dev/sda2

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

#1712650

FromBoqun Feng <boqun.feng@gmail.com>
Date2017-08-16 06:10 +0200
Message-ID<ueXOx-2cs-1@gated-at.bofh.it>
In reply to#1712536
On Wed, Aug 16, 2017 at 09:16:37AM +0900, Byungchul Park wrote:
> On Tue, Aug 15, 2017 at 10:20:20AM +0200, Ingo Molnar wrote:
> > 
> > So with the latest fixes there's a new lockdep warning on one of my testboxes:
> > 
> > [   11.322487] EXT4-fs (sda2): mounted filesystem with ordered data mode. Opts: (null)
> > 
> > [   11.495661] ======================================================
> > [   11.502093] WARNING: possible circular locking dependency detected
> > [   11.508507] 4.13.0-rc5-00497-g73135c58-dirty #1 Not tainted
> > [   11.514313] ------------------------------------------------------
> > [   11.520725] umount/533 is trying to acquire lock:
> > [   11.525657]  ((complete)&barr->done){+.+.}, at: [<ffffffff810fdbb3>] flush_work+0x213/0x2f0
> > [   11.534411] 
> >                but task is already holding lock:
> > [   11.540661]  (lock#3){+.+.}, at: [<ffffffff8122678d>] lru_add_drain_all_cpuslocked+0x3d/0x190
> > [   11.549613] 
> >                which lock already depends on the new lock.
> > 
> > The full splat is below. The kernel config is nothing fancy - distro derived, 
> > pretty close to defconfig, with lockdep enabled.
> 
> I see...
> 
> Worker A : acquired of wfc.work -> wait for cpu_hotplug_lock to be released
> Task   B : acquired of cpu_hotplug_lock -> wait for lock#3 to be released
> Task   C : acquired of lock#3 -> wait for completion of barr->done

From the stack trace below, this barr->done is for flush_work() in
lru_add_drain_all_cpuslocked(), i.e. for work "per_cpu(lru_add_drain_work)"

> Worker D : wait for wfc.work to be released -> will complete barr->done

and this barr->done is for work "wfc.work".

So those two barr->done could not be the same instance, IIUC. Therefore
the deadlock case is not possible.

The problem here is all barr->done instances are initialized at
insert_wq_barrier() and they belongs to the same lock class, to fix
this, we need to differ barr->done with different lock classes based on
the corresponding works.

How about the this(only compilation test):

----------------->8
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index e86733a8b344..d14067942088 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2431,6 +2431,27 @@ struct wq_barrier {
 	struct task_struct	*task;	/* purely informational */
 };
 
+#ifdef CONFIG_LOCKDEP_COMPLETE
+# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
+do {										\
+	INIT_WORK_ONSTACK(&(barr)->work, func);					\
+	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
+	lockdep_init_map_crosslock((struct lockdep_map *)&(barr)->done.map,	\
+				   "(complete)" #barr,				\
+				   (target)->lockdep_map.key, 1); 		\
+	__init_completion(&barr->done);						\
+	barr->task = current;							\
+} while (0)
+#else
+# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
+do {										\
+	INIT_WORK_ONSTACK(&(barr)->work, func);					\
+	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
+	init_completion(&barr->done);						\
+	barr->task = current;							\
+} while (0)
+#endif
+
 static void wq_barrier_func(struct work_struct *work)
 {
 	struct wq_barrier *barr = container_of(work, struct wq_barrier, work);
@@ -2474,10 +2495,7 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
 	 * checks and call back into the fixup functions where we
 	 * might deadlock.
 	 */
-	INIT_WORK_ONSTACK(&barr->work, wq_barrier_func);
-	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&barr->work));
-	init_completion(&barr->done);
-	barr->task = current;
+	INIT_WQ_BARRIER_ONSTACK(barr, wq_barrier_func, target);
 
 	/*
 	 * If @target is currently being executed, schedule the

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

#1712657

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-16 06:40 +0200
Message-ID<ueYhz-2p6-7@gated-at.bofh.it>
In reply to#1712650
On Wed, Aug 16, 2017 at 12:05:31PM +0800, Boqun Feng wrote:
> On Wed, Aug 16, 2017 at 09:16:37AM +0900, Byungchul Park wrote:
> > On Tue, Aug 15, 2017 at 10:20:20AM +0200, Ingo Molnar wrote:
> > > 
> > > So with the latest fixes there's a new lockdep warning on one of my testboxes:
> > > 
> > > [   11.322487] EXT4-fs (sda2): mounted filesystem with ordered data mode. Opts: (null)
> > > 
> > > [   11.495661] ======================================================
> > > [   11.502093] WARNING: possible circular locking dependency detected
> > > [   11.508507] 4.13.0-rc5-00497-g73135c58-dirty #1 Not tainted
> > > [   11.514313] ------------------------------------------------------
> > > [   11.520725] umount/533 is trying to acquire lock:
> > > [   11.525657]  ((complete)&barr->done){+.+.}, at: [<ffffffff810fdbb3>] flush_work+0x213/0x2f0
> > > [   11.534411] 
> > >                but task is already holding lock:
> > > [   11.540661]  (lock#3){+.+.}, at: [<ffffffff8122678d>] lru_add_drain_all_cpuslocked+0x3d/0x190
> > > [   11.549613] 
> > >                which lock already depends on the new lock.
> > > 
> > > The full splat is below. The kernel config is nothing fancy - distro derived, 
> > > pretty close to defconfig, with lockdep enabled.
> > 
> > I see...
> > 
> > Worker A : acquired of wfc.work -> wait for cpu_hotplug_lock to be released
> > Task   B : acquired of cpu_hotplug_lock -> wait for lock#3 to be released
> > Task   C : acquired of lock#3 -> wait for completion of barr->done
> 
> >From the stack trace below, this barr->done is for flush_work() in
> lru_add_drain_all_cpuslocked(), i.e. for work "per_cpu(lru_add_drain_work)"
> 
> > Worker D : wait for wfc.work to be released -> will complete barr->done
> 
> and this barr->done is for work "wfc.work".

I think it can be the same instance. wait_for_completion() in flush_work()
e.g. at task C in my example, waits for completion which we expect to be
done by a worker e.g. worker D in my example.

I think the problem is caused by a write-acquisition of wfc.work in
process_one_work(). The acquisition of wfc.work should be reenterable,
that is, read-acquisition, shouldn't it?

I might be wrong... Please fix me if so.

Thank you,
Byungchul

> So those two barr->done could not be the same instance, IIUC. Therefore
> the deadlock case is not possible.
> 
> The problem here is all barr->done instances are initialized at
> insert_wq_barrier() and they belongs to the same lock class, to fix
> this, we need to differ barr->done with different lock classes based on
> the corresponding works.
> 
> How about the this(only compilation test):
> 
> ----------------->8
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index e86733a8b344..d14067942088 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2431,6 +2431,27 @@ struct wq_barrier {
>  	struct task_struct	*task;	/* purely informational */
>  };
>  
> +#ifdef CONFIG_LOCKDEP_COMPLETE
> +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> +do {										\
> +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> +	lockdep_init_map_crosslock((struct lockdep_map *)&(barr)->done.map,	\
> +				   "(complete)" #barr,				\
> +				   (target)->lockdep_map.key, 1); 		\
> +	__init_completion(&barr->done);						\
> +	barr->task = current;							\
> +} while (0)
> +#else
> +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> +do {										\
> +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> +	init_completion(&barr->done);						\
> +	barr->task = current;							\
> +} while (0)
> +#endif
> +
>  static void wq_barrier_func(struct work_struct *work)
>  {
>  	struct wq_barrier *barr = container_of(work, struct wq_barrier, work);
> @@ -2474,10 +2495,7 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
>  	 * checks and call back into the fixup functions where we
>  	 * might deadlock.
>  	 */
> -	INIT_WORK_ONSTACK(&barr->work, wq_barrier_func);
> -	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&barr->work));
> -	init_completion(&barr->done);
> -	barr->task = current;
> +	INIT_WQ_BARRIER_ONSTACK(barr, wq_barrier_func, target);
>  
>  	/*
>  	 * If @target is currently being executed, schedule the

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

#1712700

FromBoqun Feng <boqun.feng@gmail.com>
Date2017-08-16 07:50 +0200
Message-ID<ueZnj-36e-5@gated-at.bofh.it>
In reply to#1712657

[Multipart message — attachments visible in raw view] — view raw

On Wed, Aug 16, 2017 at 01:37:46PM +0900, Byungchul Park wrote:
> On Wed, Aug 16, 2017 at 12:05:31PM +0800, Boqun Feng wrote:
> > On Wed, Aug 16, 2017 at 09:16:37AM +0900, Byungchul Park wrote:
> > > On Tue, Aug 15, 2017 at 10:20:20AM +0200, Ingo Molnar wrote:
> > > > 
> > > > So with the latest fixes there's a new lockdep warning on one of my testboxes:
> > > > 
> > > > [   11.322487] EXT4-fs (sda2): mounted filesystem with ordered data mode. Opts: (null)
> > > > 
> > > > [   11.495661] ======================================================
> > > > [   11.502093] WARNING: possible circular locking dependency detected
> > > > [   11.508507] 4.13.0-rc5-00497-g73135c58-dirty #1 Not tainted
> > > > [   11.514313] ------------------------------------------------------
> > > > [   11.520725] umount/533 is trying to acquire lock:
> > > > [   11.525657]  ((complete)&barr->done){+.+.}, at: [<ffffffff810fdbb3>] flush_work+0x213/0x2f0
> > > > [   11.534411] 
> > > >                but task is already holding lock:
> > > > [   11.540661]  (lock#3){+.+.}, at: [<ffffffff8122678d>] lru_add_drain_all_cpuslocked+0x3d/0x190
> > > > [   11.549613] 
> > > >                which lock already depends on the new lock.
> > > > 
> > > > The full splat is below. The kernel config is nothing fancy - distro derived, 
> > > > pretty close to defconfig, with lockdep enabled.
> > > 
> > > I see...
> > > 
> > > Worker A : acquired of wfc.work -> wait for cpu_hotplug_lock to be released
> > > Task   B : acquired of cpu_hotplug_lock -> wait for lock#3 to be released
> > > Task   C : acquired of lock#3 -> wait for completion of barr->done
> > 
> > >From the stack trace below, this barr->done is for flush_work() in
> > lru_add_drain_all_cpuslocked(), i.e. for work "per_cpu(lru_add_drain_work)"
> > 
> > > Worker D : wait for wfc.work to be released -> will complete barr->done
> > 
> > and this barr->done is for work "wfc.work".
> 
> I think it can be the same instance. wait_for_completion() in flush_work()
> e.g. at task C in my example, waits for completion which we expect to be
> done by a worker e.g. worker D in my example.
> 
> I think the problem is caused by a write-acquisition of wfc.work in
> process_one_work(). The acquisition of wfc.work should be reenterable,
> that is, read-acquisition, shouldn't it?
> 

The only thing is that wfc.work is not a real and please see code in
flush_work(). And if a task C do a flush_work() for "wfc.work" with
lock#3 held, it needs to "acquire" wfc.work before it
wait_for_completion(), which is already a deadlock case:

	lock#3 -> wfc.work -> cpu_hotplug_lock -+
          ^                                     |
	  |                                     |
	  +-------------------------------------+

, without crossrelease enabled. So the task C didn't flush work wfc.work
in the previous case, which implies barr->done in Task C and Worker D
are not the same instance.

Make sense?

Regards,
Boqun

> I might be wrong... Please fix me if so.
> 
> Thank you,
> Byungchul
> 
> > So those two barr->done could not be the same instance, IIUC. Therefore
> > the deadlock case is not possible.
> > 
> > The problem here is all barr->done instances are initialized at
> > insert_wq_barrier() and they belongs to the same lock class, to fix
> > this, we need to differ barr->done with different lock classes based on
> > the corresponding works.
> > 
> > How about the this(only compilation test):
> > 
> > ----------------->8
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index e86733a8b344..d14067942088 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -2431,6 +2431,27 @@ struct wq_barrier {
> >  	struct task_struct	*task;	/* purely informational */
> >  };
> >  
> > +#ifdef CONFIG_LOCKDEP_COMPLETE
> > +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> > +do {										\
> > +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> > +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> > +	lockdep_init_map_crosslock((struct lockdep_map *)&(barr)->done.map,	\
> > +				   "(complete)" #barr,				\
> > +				   (target)->lockdep_map.key, 1); 		\
> > +	__init_completion(&barr->done);						\
> > +	barr->task = current;							\
> > +} while (0)
> > +#else
> > +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> > +do {										\
> > +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> > +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> > +	init_completion(&barr->done);						\
> > +	barr->task = current;							\
> > +} while (0)
> > +#endif
> > +
> >  static void wq_barrier_func(struct work_struct *work)
> >  {
> >  	struct wq_barrier *barr = container_of(work, struct wq_barrier, work);
> > @@ -2474,10 +2495,7 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
> >  	 * checks and call back into the fixup functions where we
> >  	 * might deadlock.
> >  	 */
> > -	INIT_WORK_ONSTACK(&barr->work, wq_barrier_func);
> > -	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&barr->work));
> > -	init_completion(&barr->done);
> > -	barr->task = current;
> > +	INIT_WQ_BARRIER_ONSTACK(barr, wq_barrier_func, target);
> >  
> >  	/*
> >  	 * If @target is currently being executed, schedule the

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

#1712717

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-16 08:40 +0200
Message-ID<uf09I-3AX-5@gated-at.bofh.it>
In reply to#1712700
On Wed, Aug 16, 2017 at 01:40:51PM +0800, Boqun Feng wrote:
> > > > Worker A : acquired of wfc.work -> wait for cpu_hotplug_lock to be released
> > > > Task   B : acquired of cpu_hotplug_lock -> wait for lock#3 to be released
> > > > Task   C : acquired of lock#3 -> wait for completion of barr->done
> > > 
> > > >From the stack trace below, this barr->done is for flush_work() in
> > > lru_add_drain_all_cpuslocked(), i.e. for work "per_cpu(lru_add_drain_work)"
> > > 
> > > > Worker D : wait for wfc.work to be released -> will complete barr->done
> > > 
> > > and this barr->done is for work "wfc.work".
> > 
> > I think it can be the same instance. wait_for_completion() in flush_work()
> > e.g. at task C in my example, waits for completion which we expect to be
> > done by a worker e.g. worker D in my example.
> > 
> > I think the problem is caused by a write-acquisition of wfc.work in
> > process_one_work(). The acquisition of wfc.work should be reenterable,
> > that is, read-acquisition, shouldn't it?
> > 
> 
> The only thing is that wfc.work is not a real and please see code in
> flush_work(). And if a task C do a flush_work() for "wfc.work" with
> lock#3 held, it needs to "acquire" wfc.work before it
> wait_for_completion(), which is already a deadlock case:
> 
> 	lock#3 -> wfc.work -> cpu_hotplug_lock -+
>           ^                                     |
> 	  |                                     |
> 	  +-------------------------------------+
> 
> , without crossrelease enabled. So the task C didn't flush work wfc.work
> in the previous case, which implies barr->done in Task C and Worker D
> are not the same instance.
> 
> Make sense?

Thank you very much for your explanation. I misunderstood how flush_work()
works. Yes, it seems to be led by incorrect class of completion.

Thanks,
Byungchul

> 
> Regards,
> Boqun
> 
> > I might be wrong... Please fix me if so.
> > 
> > Thank you,
> > Byungchul
> > 
> > > So those two barr->done could not be the same instance, IIUC. Therefore
> > > the deadlock case is not possible.
> > > 
> > > The problem here is all barr->done instances are initialized at
> > > insert_wq_barrier() and they belongs to the same lock class, to fix
> > > this, we need to differ barr->done with different lock classes based on
> > > the corresponding works.
> > > 
> > > How about the this(only compilation test):
> > > 
> > > ----------------->8
> > > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > > index e86733a8b344..d14067942088 100644
> > > --- a/kernel/workqueue.c
> > > +++ b/kernel/workqueue.c
> > > @@ -2431,6 +2431,27 @@ struct wq_barrier {
> > >  	struct task_struct	*task;	/* purely informational */
> > >  };
> > >  
> > > +#ifdef CONFIG_LOCKDEP_COMPLETE
> > > +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> > > +do {										\
> > > +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> > > +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> > > +	lockdep_init_map_crosslock((struct lockdep_map *)&(barr)->done.map,	\
> > > +				   "(complete)" #barr,				\
> > > +				   (target)->lockdep_map.key, 1); 		\
> > > +	__init_completion(&barr->done);						\
> > > +	barr->task = current;							\
> > > +} while (0)
> > > +#else
> > > +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> > > +do {										\
> > > +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> > > +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> > > +	init_completion(&barr->done);						\
> > > +	barr->task = current;							\
> > > +} while (0)
> > > +#endif
> > > +
> > >  static void wq_barrier_func(struct work_struct *work)
> > >  {
> > >  	struct wq_barrier *barr = container_of(work, struct wq_barrier, work);
> > > @@ -2474,10 +2495,7 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
> > >  	 * checks and call back into the fixup functions where we
> > >  	 * might deadlock.
> > >  	 */
> > > -	INIT_WORK_ONSTACK(&barr->work, wq_barrier_func);
> > > -	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&barr->work));
> > > -	init_completion(&barr->done);
> > > -	barr->task = current;
> > > +	INIT_WQ_BARRIER_ONSTACK(barr, wq_barrier_func, target);
> > >  
> > >  	/*
> > >  	 * If @target is currently being executed, schedule the

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

#1712683

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-16 07:10 +0200
Message-ID<ueYKB-2PV-1@gated-at.bofh.it>
In reply to#1712650
On Wed, Aug 16, 2017 at 12:05:31PM +0800, Boqun Feng wrote:
> > I see...
> > 
> > Worker A : acquired of wfc.work -> wait for cpu_hotplug_lock to be released
> > Task   B : acquired of cpu_hotplug_lock -> wait for lock#3 to be released
> > Task   C : acquired of lock#3 -> wait for completion of barr->done
> 
> >From the stack trace below, this barr->done is for flush_work() in
> lru_add_drain_all_cpuslocked(), i.e. for work "per_cpu(lru_add_drain_work)"
> 
> > Worker D : wait for wfc.work to be released -> will complete barr->done
> 
> and this barr->done is for work "wfc.work".
> 
> So those two barr->done could not be the same instance, IIUC. Therefore
> the deadlock case is not possible.
> 
> The problem here is all barr->done instances are initialized at
> insert_wq_barrier() and they belongs to the same lock class, to fix

I'm not sure this caused the lockdep warning but, if they belongs to the
same class even though they couldn't be the same instance as you said, I
also think that is another problem and should be fixed.

> this, we need to differ barr->done with different lock classes based on
> the corresponding works.
> 
> How about the this(only compilation test):
> 
> ----------------->8
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index e86733a8b344..d14067942088 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2431,6 +2431,27 @@ struct wq_barrier {
>  	struct task_struct	*task;	/* purely informational */
>  };
>  
> +#ifdef CONFIG_LOCKDEP_COMPLETE
> +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> +do {										\
> +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> +	lockdep_init_map_crosslock((struct lockdep_map *)&(barr)->done.map,	\
> +				   "(complete)" #barr,				\
> +				   (target)->lockdep_map.key, 1); 		\
> +	__init_completion(&barr->done);						\
> +	barr->task = current;							\
> +} while (0)
> +#else
> +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> +do {										\
> +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> +	init_completion(&barr->done);						\
> +	barr->task = current;							\
> +} while (0)
> +#endif
> +
>  static void wq_barrier_func(struct work_struct *work)
>  {
>  	struct wq_barrier *barr = container_of(work, struct wq_barrier, work);
> @@ -2474,10 +2495,7 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
>  	 * checks and call back into the fixup functions where we
>  	 * might deadlock.
>  	 */
> -	INIT_WORK_ONSTACK(&barr->work, wq_barrier_func);
> -	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&barr->work));
> -	init_completion(&barr->done);
> -	barr->task = current;
> +	INIT_WQ_BARRIER_ONSTACK(barr, wq_barrier_func, target);
>  
>  	/*
>  	 * If @target is currently being executed, schedule the

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

#1712704

FromBoqun Feng <boqun.feng@gmail.com>
Date2017-08-16 08:00 +0200
Message-ID<ueZx0-39t-9@gated-at.bofh.it>
In reply to#1712683

[Multipart message — attachments visible in raw view] — view raw

On Wed, Aug 16, 2017 at 02:05:06PM +0900, Byungchul Park wrote:
> On Wed, Aug 16, 2017 at 12:05:31PM +0800, Boqun Feng wrote:
> > > I see...
> > > 
> > > Worker A : acquired of wfc.work -> wait for cpu_hotplug_lock to be released
> > > Task   B : acquired of cpu_hotplug_lock -> wait for lock#3 to be released
> > > Task   C : acquired of lock#3 -> wait for completion of barr->done
> > 
> > >From the stack trace below, this barr->done is for flush_work() in
> > lru_add_drain_all_cpuslocked(), i.e. for work "per_cpu(lru_add_drain_work)"
> > 
> > > Worker D : wait for wfc.work to be released -> will complete barr->done
> > 
> > and this barr->done is for work "wfc.work".
> > 
> > So those two barr->done could not be the same instance, IIUC. Therefore
> > the deadlock case is not possible.
> > 
> > The problem here is all barr->done instances are initialized at
> > insert_wq_barrier() and they belongs to the same lock class, to fix
> 
> I'm not sure this caused the lockdep warning but, if they belongs to the
> same class even though they couldn't be the same instance as you said, I
> also think that is another problem and should be fixed.
> 

My point was more like this is a false positive case, which we should
avoid as hard as we can, because this very case doesn't look like a
deadlock to me.

Maybe the pattern above does exist in current kernel, but we need to
guide/adjust lockdep to find the real case showing it's happening.

Regards,
Boqun

> > this, we need to differ barr->done with different lock classes based on
> > the corresponding works.
> > 
> > How about the this(only compilation test):
> > 
> > ----------------->8
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index e86733a8b344..d14067942088 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -2431,6 +2431,27 @@ struct wq_barrier {
> >  	struct task_struct	*task;	/* purely informational */
> >  };
> >  
> > +#ifdef CONFIG_LOCKDEP_COMPLETE
> > +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> > +do {										\
> > +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> > +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> > +	lockdep_init_map_crosslock((struct lockdep_map *)&(barr)->done.map,	\
> > +				   "(complete)" #barr,				\
> > +				   (target)->lockdep_map.key, 1); 		\
> > +	__init_completion(&barr->done);						\
> > +	barr->task = current;							\
> > +} while (0)
> > +#else
> > +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> > +do {										\
> > +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> > +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> > +	init_completion(&barr->done);						\
> > +	barr->task = current;							\
> > +} while (0)
> > +#endif
> > +
> >  static void wq_barrier_func(struct work_struct *work)
> >  {
> >  	struct wq_barrier *barr = container_of(work, struct wq_barrier, work);
> > @@ -2474,10 +2495,7 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
> >  	 * checks and call back into the fixup functions where we
> >  	 * might deadlock.
> >  	 */
> > -	INIT_WORK_ONSTACK(&barr->work, wq_barrier_func);
> > -	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&barr->work));
> > -	init_completion(&barr->done);
> > -	barr->task = current;
> > +	INIT_WQ_BARRIER_ONSTACK(barr, wq_barrier_func, target);
> >  
> >  	/*
> >  	 * If @target is currently being executed, schedule the

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

#1712728

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-16 09:20 +0200
Message-ID<uf0Mp-42X-3@gated-at.bofh.it>
In reply to#1712704
On Wed, Aug 16, 2017 at 01:58:08PM +0800, Boqun Feng wrote:
> > I'm not sure this caused the lockdep warning but, if they belongs to the
> > same class even though they couldn't be the same instance as you said, I
> > also think that is another problem and should be fixed.
> > 
> 
> My point was more like this is a false positive case, which we should
> avoid as hard as we can, because this very case doesn't look like a
> deadlock to me.
> 
> Maybe the pattern above does exist in current kernel, but we need to
> guide/adjust lockdep to find the real case showing it's happening.

As long as they are initialized as a same class, there's no way to
distinguish between them within lockdep.

And I also think we should avoid false positive cases. Do you think
there are many places where completions are initialized in a same place
even though they could never be the same instance?

If no, it would be better to fix it whenever we face it, as you did.

If yes, we have to change it for completion, for example:

1. Do not apply crossrelease into completions initialized on stack.

or

2. Use the full call path instead of a call site as a lockdep_map key.

or

3. So on.

Could you let me know your opinion about it?

Thanks,
Byungchul

> Regards,
> Boqun
> 
> > > this, we need to differ barr->done with different lock classes based on
> > > the corresponding works.
> > > 
> > > How about the this(only compilation test):
> > > 
> > > ----------------->8
> > > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > > index e86733a8b344..d14067942088 100644
> > > --- a/kernel/workqueue.c
> > > +++ b/kernel/workqueue.c
> > > @@ -2431,6 +2431,27 @@ struct wq_barrier {
> > >  	struct task_struct	*task;	/* purely informational */
> > >  };
> > >  
> > > +#ifdef CONFIG_LOCKDEP_COMPLETE
> > > +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> > > +do {										\
> > > +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> > > +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> > > +	lockdep_init_map_crosslock((struct lockdep_map *)&(barr)->done.map,	\
> > > +				   "(complete)" #barr,				\
> > > +				   (target)->lockdep_map.key, 1); 		\
> > > +	__init_completion(&barr->done);						\
> > > +	barr->task = current;							\
> > > +} while (0)
> > > +#else
> > > +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> > > +do {										\
> > > +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> > > +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> > > +	init_completion(&barr->done);						\
> > > +	barr->task = current;							\
> > > +} while (0)
> > > +#endif
> > > +
> > >  static void wq_barrier_func(struct work_struct *work)
> > >  {
> > >  	struct wq_barrier *barr = container_of(work, struct wq_barrier, work);
> > > @@ -2474,10 +2495,7 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
> > >  	 * checks and call back into the fixup functions where we
> > >  	 * might deadlock.
> > >  	 */
> > > -	INIT_WORK_ONSTACK(&barr->work, wq_barrier_func);
> > > -	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&barr->work));
> > > -	init_completion(&barr->done);
> > > -	barr->task = current;
> > > +	INIT_WQ_BARRIER_ONSTACK(barr, wq_barrier_func, target);
> > >  
> > >  	/*
> > >  	 * If @target is currently being executed, schedule the

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

#1712757

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-16 10:10 +0200
Message-ID<uf1yO-4Ab-17@gated-at.bofh.it>
In reply to#1712728
On Wed, Aug 16, 2017 at 04:14:21PM +0900, Byungchul Park wrote:
> On Wed, Aug 16, 2017 at 01:58:08PM +0800, Boqun Feng wrote:
> > > I'm not sure this caused the lockdep warning but, if they belongs to the
> > > same class even though they couldn't be the same instance as you said, I
> > > also think that is another problem and should be fixed.
> > > 
> > 
> > My point was more like this is a false positive case, which we should
> > avoid as hard as we can, because this very case doesn't look like a
> > deadlock to me.
> > 
> > Maybe the pattern above does exist in current kernel, but we need to
> > guide/adjust lockdep to find the real case showing it's happening.
> 
> As long as they are initialized as a same class, there's no way to
> distinguish between them within lockdep.
> 
> And I also think we should avoid false positive cases. Do you think
> there are many places where completions are initialized in a same place
> even though they could never be the same instance?
> 
> If no, it would be better to fix it whenever we face it, as you did.

BTW, of course, the same problem would have occured when applying
lockdep for the first time. How did you solve it?

I mean that lockdep basically identifies classes even for typical locks
with the call site. So two locks could be the same class even though
they should not be the same. Of course, for now, we avoid the problemaic
cases with sub-class. Anyway, the problems certainly would have arised
for the first time. I want to follow that solution you did.

Thanks,
Byungchul

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

#1712809

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-16 11:50 +0200
Message-ID<uf37A-5nW-17@gated-at.bofh.it>
In reply to#1712757
On Wed, Aug 16, 2017 at 05:06:23PM +0900, Byungchul Park wrote:
> On Wed, Aug 16, 2017 at 04:14:21PM +0900, Byungchul Park wrote:
> > On Wed, Aug 16, 2017 at 01:58:08PM +0800, Boqun Feng wrote:
> > > > I'm not sure this caused the lockdep warning but, if they belongs to the
> > > > same class even though they couldn't be the same instance as you said, I
> > > > also think that is another problem and should be fixed.
> > > > 
> > > 
> > > My point was more like this is a false positive case, which we should
> > > avoid as hard as we can, because this very case doesn't look like a
> > > deadlock to me.
> > > 
> > > Maybe the pattern above does exist in current kernel, but we need to
> > > guide/adjust lockdep to find the real case showing it's happening.
> > 
> > As long as they are initialized as a same class, there's no way to
> > distinguish between them within lockdep.
> > 
> > And I also think we should avoid false positive cases. Do you think
> > there are many places where completions are initialized in a same place
> > even though they could never be the same instance?
> > 
> > If no, it would be better to fix it whenever we face it, as you did.
> 
> BTW, of course, the same problem would have occured when applying
> lockdep for the first time. How did you solve it?
> 
> I mean that lockdep basically identifies classes even for typical locks
> with the call site. So two locks could be the same class even though
> they should not be the same. Of course, for now, we avoid the problemaic
> cases with sub-class. Anyway, the problems certainly would have arised
             ^
             or setting a class or re-design code like what Boqun
             suggested. And so on...

> for the first time. I want to follow that solution you did.
> 
> Thanks,
> Byungchul

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

#1713606

FromIngo Molnar <mingo@kernel.org>
Date2017-08-17 09:50 +0200
Message-ID<ufnJ0-1Il-15@gated-at.bofh.it>
In reply to#1712650
* Boqun Feng <boqun.feng@gmail.com> wrote:

> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2431,6 +2431,27 @@ struct wq_barrier {
>  	struct task_struct	*task;	/* purely informational */
>  };
>  
> +#ifdef CONFIG_LOCKDEP_COMPLETE
> +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> +do {										\
> +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> +	lockdep_init_map_crosslock((struct lockdep_map *)&(barr)->done.map,	\
> +				   "(complete)" #barr,				\
> +				   (target)->lockdep_map.key, 1); 		\
> +	__init_completion(&barr->done);						\
> +	barr->task = current;							\
> +} while (0)
> +#else
> +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> +do {										\
> +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> +	init_completion(&barr->done);						\
> +	barr->task = current;							\
> +} while (0)
> +#endif

Is there any progress with this bug? This false positive warning regression is 
blocking the locking tree.

BTW., I don't think the #ifdef is necessary: lockdep_init_map_crosslock should map 
to nothing when lockdep is disabled, right?

Thanks,

	Ingo

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

#1713651

FromBoqun Feng <boqun.feng@gmail.com>
Date2017-08-17 10:10 +0200
Message-ID<ufo2n-25Q-27@gated-at.bofh.it>
In reply to#1713606

[Multipart message — attachments visible in raw view] — view raw

On Thu, Aug 17, 2017 at 09:48:11AM +0200, Ingo Molnar wrote:
> 
> * Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -2431,6 +2431,27 @@ struct wq_barrier {
> >  	struct task_struct	*task;	/* purely informational */
> >  };
> >  
> > +#ifdef CONFIG_LOCKDEP_COMPLETE
> > +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> > +do {										\
> > +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> > +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> > +	lockdep_init_map_crosslock((struct lockdep_map *)&(barr)->done.map,	\
> > +				   "(complete)" #barr,				\
> > +				   (target)->lockdep_map.key, 1); 		\
> > +	__init_completion(&barr->done);						\
> > +	barr->task = current;							\
> > +} while (0)
> > +#else
> > +# define INIT_WQ_BARRIER_ONSTACK(barr, func, target)				\
> > +do {										\
> > +	INIT_WORK_ONSTACK(&(barr)->work, func);					\
> > +	__set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(&(barr)->work));	\
> > +	init_completion(&barr->done);						\
> > +	barr->task = current;							\
> > +} while (0)
> > +#endif
> 
> Is there any progress with this bug? This false positive warning regression is 
> blocking the locking tree.
> 

I have been trying to reproduce the false positive on my machine, but
haven't succeeded. ;-( Have you tried this?

But I have been using this patch for a day and haven't shoot my foot
yet.

> BTW., I don't think the #ifdef is necessary: lockdep_init_map_crosslock should map 
> to nothing when lockdep is disabled, right?

IIUC, lockdep_init_map_crosslock is only defined when
CONFIG_LOCKDEP_CROSSRELEASE=y, moreover, completion::map, which used as
the parameter of lockdep_init_map_crosslock(), is only defined when
CONFIG_LOCKDEP_COMPLETE=y. So the #ifdef is necessary, but maybe we can
clean this thing up in the future.

I will send a proper patch, so the thing could move forwards. Just a
minute ;-)

Regards,
Boqun

> 
> Thanks,
> 
> 	Ingo

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

#1713662

FromIngo Molnar <mingo@kernel.org>
Date2017-08-17 10:20 +0200
Message-ID<ufoc1-29K-21@gated-at.bofh.it>
In reply to#1713651
* Boqun Feng <boqun.feng@gmail.com> wrote:

> > BTW., I don't think the #ifdef is necessary: lockdep_init_map_crosslock should map 
> > to nothing when lockdep is disabled, right?
> 
> IIUC, lockdep_init_map_crosslock is only defined when
> CONFIG_LOCKDEP_CROSSRELEASE=y,

Then lockdep_init_map_crosslock() should be defined in the !LOCKDEP case as well.

> [...] moreover, completion::map, which used as
> the parameter of lockdep_init_map_crosslock(), is only defined when
> CONFIG_LOCKDEP_COMPLETE=y.

If the !LOCKDEP wrapper is a CPP macro then it can ignore that parameter just 
fine, and it won't be built.

Thanks,

	Ingo

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

#1713676

FromBoqun Feng <boqun.feng@gmail.com>
Date2017-08-17 10:40 +0200
Message-ID<ufovo-2hc-29@gated-at.bofh.it>
In reply to#1713662

[Multipart message — attachments visible in raw view] — view raw

On Thu, Aug 17, 2017 at 10:12:24AM +0200, Ingo Molnar wrote:
> 
> * Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > > BTW., I don't think the #ifdef is necessary: lockdep_init_map_crosslock should map 
> > > to nothing when lockdep is disabled, right?
> > 
> > IIUC, lockdep_init_map_crosslock is only defined when
> > CONFIG_LOCKDEP_CROSSRELEASE=y,
> 
> Then lockdep_init_map_crosslock() should be defined in the !LOCKDEP case as well.
> 
> > [...] moreover, completion::map, which used as
> > the parameter of lockdep_init_map_crosslock(), is only defined when
> > CONFIG_LOCKDEP_COMPLETE=y.
> 
> If the !LOCKDEP wrapper is a CPP macro then it can ignore that parameter just 
> fine, and it won't be built.
> 

Oops, I miss this part.. so I will cook a patch define
lockdep_init_map_crosslock() when !LOCKDEP and I think based on that,
there is no need to introducde INIT_WQ_BARRIER_ONSTACK(), we can simply:

	lockdep_init_map_crosslock(...);
	__init_completion();

in insert_wq_barrier(). Simpler.

Thanks for your suggestion.

Regards,
Boqun

> Thanks,
> 
> 	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web