linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Silencing false lockdep warning related to seq lock
@ 2021-05-14 14:52 Joel Fernandes
  2021-05-17  4:21 ` Boqun Feng
  0 siblings, 1 reply; 6+ messages in thread
From: Joel Fernandes @ 2021-05-14 14:52 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Joel Fernandes (Google),
	Peter Zijlstra, Paul E. McKenney, LKML, Laurent Dufour,
	Suren Baghdasaryan

Hi Boqun,
You might have worked on such issues so I thought you're a good person to ask.

After apply Laurent's SPF patchset [1] , we're facing a large number
of (seemingly false positive) lockdep reports which are related to
circular dependencies with seq locks.

 lock(A); write_seqcount(B)
  vs.
write_seqcount(B); lock(A)

This cannot deadlock obviously. My current strategy which I hate is to
make it a raw seqcount write which bypasses lockdep. That's horrible
for obvious reasons. Do you have any tricks/patches up your sleeve to
silence these?

I suppose we still want to catch lockdep issues of the form (which
peterz chatted to me about):

 lock(A); write_seqcount(B)
  vs.
read_seqcount(B); lock(A)

which seems like it can deadlock.

I would rather make lockdep useful to catch these and not miss out on
them. Let me know what you think?

Cheers,
-Joel

[1] https://lkml.org/lkml/2019/4/16/615

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Silencing false lockdep warning related to seq lock
  2021-05-14 14:52 Silencing false lockdep warning related to seq lock Joel Fernandes
@ 2021-05-17  4:21 ` Boqun Feng
  2021-05-18  1:52   ` Joel Fernandes
  0 siblings, 1 reply; 6+ messages in thread
From: Boqun Feng @ 2021-05-17  4:21 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Joel Fernandes (Google),
	Peter Zijlstra, Paul E. McKenney, LKML, Laurent Dufour,
	Suren Baghdasaryan

Hi,

On Fri, May 14, 2021 at 10:52:31AM -0400, Joel Fernandes wrote:
> Hi Boqun,
> You might have worked on such issues so I thought you're a good person to ask.
> 
> After apply Laurent's SPF patchset [1] , we're facing a large number
> of (seemingly false positive) lockdep reports which are related to
> circular dependencies with seq locks.
> 
>  lock(A); write_seqcount(B)
>   vs.
> write_seqcount(B); lock(A)
> 

Two questions here:

*	Could you provide the lockdep splats you saw? I wonder whether
	it's similar to the one mentioned in patch #9[1].

*	What keeps write_seqcount(vm_seqcount) serialized? If it's only
	one lock that serializes the writers, we probably can make it
	as the nest_lock argument for seqcount_acquire(), and that will
	help prevent the false positives.

Regards,
Boqun

[1]: https://lore.kernel.org/lkml/20190416134522.17540-10-ldufour@linux.ibm.com/

> This cannot deadlock obviously. My current strategy which I hate is to
> make it a raw seqcount write which bypasses lockdep. That's horrible
> for obvious reasons. Do you have any tricks/patches up your sleeve to
> silence these?
> 
> I suppose we still want to catch lockdep issues of the form (which
> peterz chatted to me about):
> 
>  lock(A); write_seqcount(B)
>   vs.
> read_seqcount(B); lock(A)
> 
> which seems like it can deadlock.
> 
> I would rather make lockdep useful to catch these and not miss out on
> them. Let me know what you think?
> 
> Cheers,
> -Joel
> 
> [1] https://lkml.org/lkml/2019/4/16/615

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Silencing false lockdep warning related to seq lock
  2021-05-17  4:21 ` Boqun Feng
@ 2021-05-18  1:52   ` Joel Fernandes
  2021-05-18  2:24     ` Boqun Feng
  0 siblings, 1 reply; 6+ messages in thread
From: Joel Fernandes @ 2021-05-18  1:52 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Peter Zijlstra, Paul E. McKenney, LKML, Laurent Dufour,
	Suren Baghdasaryan

Hi Boqun,
Thanks for the reply. See below:

On Mon, May 17, 2021 at 12:21:38PM +0800, Boqun Feng wrote:
[...]
> > After apply Laurent's SPF patchset [1] , we're facing a large number
> > of (seemingly false positive) lockdep reports which are related to
> > circular dependencies with seq locks.
> > 
> >  lock(A); write_seqcount(B)
> >   vs.
> > write_seqcount(B); lock(A)
> > 
> 
> Two questions here:
> 
> *	Could you provide the lockdep splats you saw? I wonder whether
> 	it's similar to the one mentioned in patch #9[1].

Sure I have appended them to this email. Here is the tree with Laurent's
patches applied, in case you want to take a look:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-5.4

Yes, the splat is similar to the one mentioned in that patch #9, however the
first splat I appended below shows an issue with lockdep false positive -
there is clearly problem with lockdep where it thinks following sequence is
bad, when in fact it is not:

    init process (skipped some functions):
    exec->
     flush_old_exec->
      exit_mmap ->
        free_pgtables->
          vm_write_begin(vma) // Y: acquires seq lock in write mode
             unlink_anon_vmas // Z: acquires anon_vma->rwsem

    exec->
    load_elf_binary->
      -> setup_arg_pages
        -> expand_downwards (in the if (grow <=) block)
           mm->page_table_lock // X
           vm_write_begin(vma) // Y: acquires seq lock

    exec->
     do_execve_file->
       ->get_user_pages
         -> handle_pte_fault
          -> anon_vma_prepare
              -> acquire anon_vma->rwsem  // Z
              -> acquire mm->page_table_lock // X

    If vm_write_begin ever had to wait, then it could lockup like this if following happened concurrently:
    Acquire->TryToAcquire
    Y->Z
    X->Y
    Z->X

    But Y can never result in a wait since it is a sequence lock. So this is
    a lockdep false positive.

> 
> *	What keeps write_seqcount(vm_seqcount) serialized? If it's only
> 	one lock that serializes the writers, we probably can make it
> 	as the nest_lock argument for seqcount_acquire(), and that will
> 	help prevent the false positives.

I think the write seqcount is serialized by the mmap_sem in all the code
paths I audited in Laurent's patchset.

I am not sure how making it nest_lock argument will help though? The issue is
that lockdep's understanding of seqcount needs to relate seqcount readers to
seqcount writers when considering deadlocks. Unless there's a seqcount reader
involved in the mix, lockdep should just shutup, no? And we don't want it to
shutup incase there is a real locking issue.

Appending splats:
Here is the first splat (which I analuzed above as X, Y, Z):

[    1.177766] ffff984c36357070 (&anon_vma->rwsem){+.+.}, at: unlink_anon_vmas+0x79/0x1a0
[    1.179061]
[    1.179061] but task is already holding lock:
[    1.180024] ffff984c363520e0 (&vma->vm_sequence){+.+.}, at: exit_mmap+0x96/0x160
[    1.181207]
[    1.181207] which lock already depends on the new lock.
[    1.181207]
[    1.182543]
[    1.182543] the existing dependency chain (in reverse order) is:
[    1.183769]
[    1.183769] -> #2 (&vma->vm_sequence){+.+.}:
[    1.184731]        expand_downwards+0x21a/0x390
[    1.185485]        setup_arg_pages+0x193/0x220
[    1.186264]        load_elf_binary+0x3d8/0x1570
[    1.187017]        search_binary_handler.part.0+0x56/0x220
[    1.187942]        __do_execve_file+0x680/0xb60
[    1.188695]        do_execve+0x22/0x30
[    1.189318]        call_usermodehelper_exec_async+0x159/0x1b0
[    1.190259]        ret_from_fork+0x3a/0x50
[    1.190933]
[    1.190933] -> #1 (&(&mm->page_table_lock)->rlock){+.+.}:
[    1.192067]        _raw_spin_lock+0x27/0x40
[    1.192763]        __anon_vma_prepare+0x5e/0x180
[    1.193532]        handle_pte_fault+0x97c/0x9d0
[    1.194284]        __handle_mm_fault+0x172/0x1f0
[    1.195049]        __get_user_pages+0x322/0x840
[    1.195804]        get_user_pages_remote+0x9f/0x290
[    1.196609]        copy_strings.isra.0+0x1d3/0x360
[    1.197401]        copy_strings_kernel+0x2f/0x40
[    1.198160]        __do_execve_file+0x568/0xb60
[    1.198915]        do_execve+0x22/0x30
[    1.199547]        call_usermodehelper_exec_async+0x159/0x1b0
[    1.200492]        ret_from_fork+0x3a/0x50
[    1.201173]
[    1.201173] -> #0 (&anon_vma->rwsem){+.+.}:
[    1.202104]        __lock_acquire+0xe1a/0x1930
[    1.202837]        lock_acquire+0x9a/0x180
[    1.203520]        down_write+0x20/0x60
[    1.204152]        unlink_anon_vmas+0x79/0x1a0
[    1.204886]        free_pgtables+0x89/0x190
[    1.205575]        exit_mmap+0x96/0x160
[    1.206210]        mmput+0x23/0xd0
[    1.206775]        do_exit+0x33b/0xb10
[    1.207405]        do_group_exit+0x34/0xb0
[    1.208081]        __x64_sys_exit_group+0xf/0x10
[    1.208852]        do_syscall_64+0x47/0xb0
[    1.209529]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
[    1.210443]

Here are some more splats that could be related:

======================================================
WARNING: possible circular locking dependency detected
5.4.109-lockdep-13764-g4f6fee83851c #1 Tainted: G     U  W
------------------------------------------------------
chrome/1627 is trying to acquire lock:
ffff8881468c76d8 (&vma->vm_sequence#3){+.+.}, at: do_user_addr_fault+0x9c4/0xba1

ffff8881468e2bd0 (&(&mm->page_table_lock)->rlock){+.+.}, at: expand_downwards+0x50f/0xaaa



       _raw_spin_lock+0x3c/0x70
       sync_global_pgds_l4+0x127/0x2c3
       register_die_notifier+0x17/0x2b
       int3_selftest+0x4f/0xca
       alternative_instructions+0xf/0x14f
       check_bugs+0x155/0x1e5
       start_kernel+0x648/0x706
       secondary_startup_64+0xa5/0xb0

       _raw_spin_lock+0x3c/0x70
       pgd_free+0x71/0x1be
       __mmdrop+0xbd/0x29d
       finish_task_switch+0x527/0x6ad
       __schedule+0x117e/0x397d
       preempt_schedule_irq+0x94/0xf8
       retint_kernel+0x1b/0x2b
       __sanitizer_cov_trace_pc+0x0/0x4e
       unmap_page_range+0x1427/0x1888
       unmap_vmas+0x15f/0x2a0
       exit_mmap+0x21b/0x395
       __mmput+0xaf/0x2e6
       flush_old_exec+0x463/0x640
       load_elf_binary+0x5f7/0x1ef1
       search_binary_handler+0x17d/0x4fb
       load_script+0x759/0x840
       search_binary_handler+0x17d/0x4fb
       exec_binprm+0x15a/0x549
       __do_execve_file+0x8da/0xeef
       __x64_sys_execve+0x99/0xa8
       do_syscall_64+0x111/0x260
       entry_SYSCALL_64_after_hwframe+0x49/0xbe

       __lock_acquire+0x25a3/0x5cc0
       lock_acquire+0x28a/0x34c
       expand_downwards+0x6b0/0xaaa
       do_user_addr_fault+0x9c4/0xba1
       page_fault+0x34/0x40

Chain exists of:
 Possible unsafe locking scenario:
       CPU0                    CPU1
       ----                    ----
  lock(&(&mm->page_table_lock)->rlock);
                               lock(pgd_lock);
                               lock(&(&mm->page_table_lock)->rlock);
  lock(&vma->vm_sequence#3);

3 locks held by chrome/1627:
 #0: ffff8881468e2c78 (&mm->mmap_sem#2){++++}, at: do_user_addr_fault+0x318/0xba1
 #1: ffff888123d64ba0 (&anon_vma->rwsem){++++}, at: expand_downwards+0x1c1/0xaaa
 #2: ffff8881468e2bd0 (&(&mm->page_table_lock)->rlock){+.+.}, at: expand_downwards+0x50f/0xaaa

CPU: 1 PID: 1627 Comm: chrome Tainted: G     U  W         5.4.109-lockdep-13764-g4f6fee83851c #1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Silencing false lockdep warning related to seq lock
  2021-05-18  1:52   ` Joel Fernandes
@ 2021-05-18  2:24     ` Boqun Feng
  2021-05-18 15:53       ` Joel Fernandes
  0 siblings, 1 reply; 6+ messages in thread
From: Boqun Feng @ 2021-05-18  2:24 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Peter Zijlstra, Paul E. McKenney, LKML, Laurent Dufour,
	Suren Baghdasaryan

On Mon, May 17, 2021 at 09:52:55PM -0400, Joel Fernandes wrote:
> Hi Boqun,
> Thanks for the reply. See below:
> 
> On Mon, May 17, 2021 at 12:21:38PM +0800, Boqun Feng wrote:
> [...]
> > > After apply Laurent's SPF patchset [1] , we're facing a large number
> > > of (seemingly false positive) lockdep reports which are related to
> > > circular dependencies with seq locks.
> > > 
> > >  lock(A); write_seqcount(B)
> > >   vs.
> > > write_seqcount(B); lock(A)
> > > 
> > 
> > Two questions here:
> > 
> > *	Could you provide the lockdep splats you saw? I wonder whether
> > 	it's similar to the one mentioned in patch #9[1].
> 
> Sure I have appended them to this email. Here is the tree with Laurent's
> patches applied, in case you want to take a look:
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-5.4
> 
> Yes, the splat is similar to the one mentioned in that patch #9, however the
> first splat I appended below shows an issue with lockdep false positive -
> there is clearly problem with lockdep where it thinks following sequence is
> bad, when in fact it is not:
> 
>     init process (skipped some functions):
>     exec->
>      flush_old_exec->
>       exit_mmap ->
>         free_pgtables->
>           vm_write_begin(vma) // Y: acquires seq lock in write mode
>              unlink_anon_vmas // Z: acquires anon_vma->rwsem
> 
>     exec->
>     load_elf_binary->
>       -> setup_arg_pages
>         -> expand_downwards (in the if (grow <=) block)
>            mm->page_table_lock // X
>            vm_write_begin(vma) // Y: acquires seq lock
> 
>     exec->
>      do_execve_file->
>        ->get_user_pages
>          -> handle_pte_fault
>           -> anon_vma_prepare
>               -> acquire anon_vma->rwsem  // Z
>               -> acquire mm->page_table_lock // X
> 
>     If vm_write_begin ever had to wait, then it could lockup like this if following happened concurrently:
>     Acquire->TryToAcquire
>     Y->Z
>     X->Y
>     Z->X
> 
>     But Y can never result in a wait since it is a sequence lock. So this is
>     a lockdep false positive.
> 
> > 
> > *	What keeps write_seqcount(vm_seqcount) serialized? If it's only
> > 	one lock that serializes the writers, we probably can make it
> > 	as the nest_lock argument for seqcount_acquire(), and that will
> > 	help prevent the false positives.
> 
> I think the write seqcount is serialized by the mmap_sem in all the code
> paths I audited in Laurent's patchset.
> 
> I am not sure how making it nest_lock argument will help though? The issue is
> that lockdep's understanding of seqcount needs to relate seqcount readers to

The thing lockdep will not report deadlock for the following sequences:

	T1:
	lock(A);
	lock_nest(B, A);
	lock(C);

	T2:
	lock(A);
	lock(C);
	lock_nest(B, A);

because with the nest_lock, lockdep would know A serializes B, so the
following case cannot happen:

	CPU 0			CPU 1
	============		============
	lock_nest(B,A);
				lock(C);
	lock(C);
				lock_nest(B, A);

becaue either CPU 0 or CPU 1 will already hold A, so they are
serialized. So nest_lock can solve part of the problem if all the
writers of vm_seqcount are serialized somehow.

Yes, seqcount writers cannot block each other, however, they are
supposed to be seralized with each other, right? So if we have the
reason to believe the above two CPUs case can happen, though it's not
a deadlock, but it's a data race, right?

I think the proper fix here is to annotate vm_seqcount with the correct
locks serializing the writers.

Regards,
Boqun

> seqcount writers when considering deadlocks. Unless there's a seqcount reader
> involved in the mix, lockdep should just shutup, no? And we don't want it to
> shutup incase there is a real locking issue.
> 
> Appending splats:
> Here is the first splat (which I analuzed above as X, Y, Z):
> 
> [    1.177766] ffff984c36357070 (&anon_vma->rwsem){+.+.}, at: unlink_anon_vmas+0x79/0x1a0
> [    1.179061]
> [    1.179061] but task is already holding lock:
> [    1.180024] ffff984c363520e0 (&vma->vm_sequence){+.+.}, at: exit_mmap+0x96/0x160
> [    1.181207]
> [    1.181207] which lock already depends on the new lock.
> [    1.181207]
> [    1.182543]
> [    1.182543] the existing dependency chain (in reverse order) is:
> [    1.183769]
> [    1.183769] -> #2 (&vma->vm_sequence){+.+.}:
> [    1.184731]        expand_downwards+0x21a/0x390
> [    1.185485]        setup_arg_pages+0x193/0x220
> [    1.186264]        load_elf_binary+0x3d8/0x1570
> [    1.187017]        search_binary_handler.part.0+0x56/0x220
> [    1.187942]        __do_execve_file+0x680/0xb60
> [    1.188695]        do_execve+0x22/0x30
> [    1.189318]        call_usermodehelper_exec_async+0x159/0x1b0
> [    1.190259]        ret_from_fork+0x3a/0x50
> [    1.190933]
> [    1.190933] -> #1 (&(&mm->page_table_lock)->rlock){+.+.}:
> [    1.192067]        _raw_spin_lock+0x27/0x40
> [    1.192763]        __anon_vma_prepare+0x5e/0x180
> [    1.193532]        handle_pte_fault+0x97c/0x9d0
> [    1.194284]        __handle_mm_fault+0x172/0x1f0
> [    1.195049]        __get_user_pages+0x322/0x840
> [    1.195804]        get_user_pages_remote+0x9f/0x290
> [    1.196609]        copy_strings.isra.0+0x1d3/0x360
> [    1.197401]        copy_strings_kernel+0x2f/0x40
> [    1.198160]        __do_execve_file+0x568/0xb60
> [    1.198915]        do_execve+0x22/0x30
> [    1.199547]        call_usermodehelper_exec_async+0x159/0x1b0
> [    1.200492]        ret_from_fork+0x3a/0x50
> [    1.201173]
> [    1.201173] -> #0 (&anon_vma->rwsem){+.+.}:
> [    1.202104]        __lock_acquire+0xe1a/0x1930
> [    1.202837]        lock_acquire+0x9a/0x180
> [    1.203520]        down_write+0x20/0x60
> [    1.204152]        unlink_anon_vmas+0x79/0x1a0
> [    1.204886]        free_pgtables+0x89/0x190
> [    1.205575]        exit_mmap+0x96/0x160
> [    1.206210]        mmput+0x23/0xd0
> [    1.206775]        do_exit+0x33b/0xb10
> [    1.207405]        do_group_exit+0x34/0xb0
> [    1.208081]        __x64_sys_exit_group+0xf/0x10
> [    1.208852]        do_syscall_64+0x47/0xb0
> [    1.209529]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [    1.210443]
> 
> Here are some more splats that could be related:
> 
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.4.109-lockdep-13764-g4f6fee83851c #1 Tainted: G     U  W
> ------------------------------------------------------
> chrome/1627 is trying to acquire lock:
> ffff8881468c76d8 (&vma->vm_sequence#3){+.+.}, at: do_user_addr_fault+0x9c4/0xba1
> 
> ffff8881468e2bd0 (&(&mm->page_table_lock)->rlock){+.+.}, at: expand_downwards+0x50f/0xaaa
> 
> 
> 
>        _raw_spin_lock+0x3c/0x70
>        sync_global_pgds_l4+0x127/0x2c3
>        register_die_notifier+0x17/0x2b
>        int3_selftest+0x4f/0xca
>        alternative_instructions+0xf/0x14f
>        check_bugs+0x155/0x1e5
>        start_kernel+0x648/0x706
>        secondary_startup_64+0xa5/0xb0
> 
>        _raw_spin_lock+0x3c/0x70
>        pgd_free+0x71/0x1be
>        __mmdrop+0xbd/0x29d
>        finish_task_switch+0x527/0x6ad
>        __schedule+0x117e/0x397d
>        preempt_schedule_irq+0x94/0xf8
>        retint_kernel+0x1b/0x2b
>        __sanitizer_cov_trace_pc+0x0/0x4e
>        unmap_page_range+0x1427/0x1888
>        unmap_vmas+0x15f/0x2a0
>        exit_mmap+0x21b/0x395
>        __mmput+0xaf/0x2e6
>        flush_old_exec+0x463/0x640
>        load_elf_binary+0x5f7/0x1ef1
>        search_binary_handler+0x17d/0x4fb
>        load_script+0x759/0x840
>        search_binary_handler+0x17d/0x4fb
>        exec_binprm+0x15a/0x549
>        __do_execve_file+0x8da/0xeef
>        __x64_sys_execve+0x99/0xa8
>        do_syscall_64+0x111/0x260
>        entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
>        __lock_acquire+0x25a3/0x5cc0
>        lock_acquire+0x28a/0x34c
>        expand_downwards+0x6b0/0xaaa
>        do_user_addr_fault+0x9c4/0xba1
>        page_fault+0x34/0x40
> 
> Chain exists of:
>  Possible unsafe locking scenario:
>        CPU0                    CPU1
>        ----                    ----
>   lock(&(&mm->page_table_lock)->rlock);
>                                lock(pgd_lock);
>                                lock(&(&mm->page_table_lock)->rlock);
>   lock(&vma->vm_sequence#3);
> 
> 3 locks held by chrome/1627:
>  #0: ffff8881468e2c78 (&mm->mmap_sem#2){++++}, at: do_user_addr_fault+0x318/0xba1
>  #1: ffff888123d64ba0 (&anon_vma->rwsem){++++}, at: expand_downwards+0x1c1/0xaaa
>  #2: ffff8881468e2bd0 (&(&mm->page_table_lock)->rlock){+.+.}, at: expand_downwards+0x50f/0xaaa
> 
> CPU: 1 PID: 1627 Comm: chrome Tainted: G     U  W         5.4.109-lockdep-13764-g4f6fee83851c #1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Silencing false lockdep warning related to seq lock
  2021-05-18  2:24     ` Boqun Feng
@ 2021-05-18 15:53       ` Joel Fernandes
  2021-05-19  4:50         ` Boqun Feng
  0 siblings, 1 reply; 6+ messages in thread
From: Joel Fernandes @ 2021-05-18 15:53 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Peter Zijlstra, Paul E. McKenney, LKML, Laurent Dufour,
	Suren Baghdasaryan

On Mon, May 17, 2021 at 10:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > [...]
> > > > After apply Laurent's SPF patchset [1] , we're facing a large number
> > > > of (seemingly false positive) lockdep reports which are related to
> > > > circular dependencies with seq locks.
> > > >
> > > >  lock(A); write_seqcount(B)
> > > >   vs.
> > > > write_seqcount(B); lock(A)
> > > >
> > >
> > > Two questions here:
> > >
> > > *   Could you provide the lockdep splats you saw? I wonder whether
> > >     it's similar to the one mentioned in patch #9[1].
> >
> > Sure I have appended them to this email. Here is the tree with Laurent's
> > patches applied, in case you want to take a look:
> > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-5.4
> >
> > Yes, the splat is similar to the one mentioned in that patch #9, however the
> > first splat I appended below shows an issue with lockdep false positive -
> > there is clearly problem with lockdep where it thinks following sequence is
> > bad, when in fact it is not:
> >
> >     init process (skipped some functions):
> >     exec->
> >      flush_old_exec->
> >       exit_mmap ->
> >         free_pgtables->
> >           vm_write_begin(vma) // Y: acquires seq lock in write mode
> >              unlink_anon_vmas // Z: acquires anon_vma->rwsem
> >
> >     exec->
> >     load_elf_binary->
> >       -> setup_arg_pages
> >         -> expand_downwards (in the if (grow <=) block)
> >            mm->page_table_lock // X
> >            vm_write_begin(vma) // Y: acquires seq lock
> >
> >     exec->
> >      do_execve_file->
> >        ->get_user_pages
> >          -> handle_pte_fault
> >           -> anon_vma_prepare
> >               -> acquire anon_vma->rwsem  // Z
> >               -> acquire mm->page_table_lock // X
> >
> >     If vm_write_begin ever had to wait, then it could lockup like this if following happened concurrently:
> >     Acquire->TryToAcquire
> >     Y->Z
> >     X->Y
> >     Z->X
> >
> >     But Y can never result in a wait since it is a sequence lock. So this is
> >     a lockdep false positive.
> >
> > >
> > > *   What keeps write_seqcount(vm_seqcount) serialized? If it's only
> > >     one lock that serializes the writers, we probably can make it
> > >     as the nest_lock argument for seqcount_acquire(), and that will
> > >     help prevent the false positives.
> >
> > I think the write seqcount is serialized by the mmap_sem in all the code
> > paths I audited in Laurent's patchset.
> >
> > I am not sure how making it nest_lock argument will help though? The issue is
> > that lockdep's understanding of seqcount needs to relate seqcount readers to
>
> The thing lockdep will not report deadlock for the following sequences:
>
>         T1:
>         lock(A);
>         lock_nest(B, A);
>         lock(C);
>
>         T2:
>         lock(A);
>         lock(C);
>         lock_nest(B, A);
>
> because with the nest_lock, lockdep would know A serializes B, so the
> following case cannot happen:
>
>         CPU 0                   CPU 1
>         ============            ============
>         lock_nest(B,A);
>                                 lock(C);
>         lock(C);
>                                 lock_nest(B, A);
>
> becaue either CPU 0 or CPU 1 will already hold A, so they are
> serialized. So nest_lock can solve part of the problem if all the
> writers of vm_seqcount are serialized somehow.
>
> Yes, seqcount writers cannot block each other, however, they are
> supposed to be seralized with each other, right? So if we have the
> reason to believe the above two CPUs case can happen, though it's not
> a deadlock, but it's a data race, right?
>
> I think the proper fix here is to annotate vm_seqcount with the correct
> locks serializing the writers.
>

I agree with you now and that's the best way forward. I will work on
something like this (unless you already did), thanks Boqun!

-Joel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Silencing false lockdep warning related to seq lock
  2021-05-18 15:53       ` Joel Fernandes
@ 2021-05-19  4:50         ` Boqun Feng
  0 siblings, 0 replies; 6+ messages in thread
From: Boqun Feng @ 2021-05-19  4:50 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Peter Zijlstra, Paul E. McKenney, LKML, Laurent Dufour,
	Suren Baghdasaryan

On Tue, May 18, 2021 at 11:53:57AM -0400, Joel Fernandes wrote:
> On Mon, May 17, 2021 at 10:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > [...]
> > > > > After apply Laurent's SPF patchset [1] , we're facing a large number
> > > > > of (seemingly false positive) lockdep reports which are related to
> > > > > circular dependencies with seq locks.
> > > > >
> > > > >  lock(A); write_seqcount(B)
> > > > >   vs.
> > > > > write_seqcount(B); lock(A)
> > > > >
> > > >
> > > > Two questions here:
> > > >
> > > > *   Could you provide the lockdep splats you saw? I wonder whether
> > > >     it's similar to the one mentioned in patch #9[1].
> > >
> > > Sure I have appended them to this email. Here is the tree with Laurent's
> > > patches applied, in case you want to take a look:
> > > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-5.4
> > >
> > > Yes, the splat is similar to the one mentioned in that patch #9, however the
> > > first splat I appended below shows an issue with lockdep false positive -
> > > there is clearly problem with lockdep where it thinks following sequence is
> > > bad, when in fact it is not:
> > >
> > >     init process (skipped some functions):
> > >     exec->
> > >      flush_old_exec->
> > >       exit_mmap ->
> > >         free_pgtables->
> > >           vm_write_begin(vma) // Y: acquires seq lock in write mode
> > >              unlink_anon_vmas // Z: acquires anon_vma->rwsem
> > >
> > >     exec->
> > >     load_elf_binary->
> > >       -> setup_arg_pages
> > >         -> expand_downwards (in the if (grow <=) block)
> > >            mm->page_table_lock // X
> > >            vm_write_begin(vma) // Y: acquires seq lock
> > >
> > >     exec->
> > >      do_execve_file->
> > >        ->get_user_pages
> > >          -> handle_pte_fault
> > >           -> anon_vma_prepare
> > >               -> acquire anon_vma->rwsem  // Z
> > >               -> acquire mm->page_table_lock // X
> > >
> > >     If vm_write_begin ever had to wait, then it could lockup like this if following happened concurrently:
> > >     Acquire->TryToAcquire
> > >     Y->Z
> > >     X->Y
> > >     Z->X
> > >
> > >     But Y can never result in a wait since it is a sequence lock. So this is
> > >     a lockdep false positive.
> > >
> > > >
> > > > *   What keeps write_seqcount(vm_seqcount) serialized? If it's only
> > > >     one lock that serializes the writers, we probably can make it
> > > >     as the nest_lock argument for seqcount_acquire(), and that will
> > > >     help prevent the false positives.
> > >
> > > I think the write seqcount is serialized by the mmap_sem in all the code
> > > paths I audited in Laurent's patchset.
> > >
> > > I am not sure how making it nest_lock argument will help though? The issue is
> > > that lockdep's understanding of seqcount needs to relate seqcount readers to
> >
> > The thing lockdep will not report deadlock for the following sequences:
> >
> >         T1:
> >         lock(A);
> >         lock_nest(B, A);
> >         lock(C);
> >
> >         T2:
> >         lock(A);
> >         lock(C);
> >         lock_nest(B, A);
> >
> > because with the nest_lock, lockdep would know A serializes B, so the
> > following case cannot happen:
> >
> >         CPU 0                   CPU 1
> >         ============            ============
> >         lock_nest(B,A);
> >                                 lock(C);
> >         lock(C);
> >                                 lock_nest(B, A);
> >
> > becaue either CPU 0 or CPU 1 will already hold A, so they are
> > serialized. So nest_lock can solve part of the problem if all the
> > writers of vm_seqcount are serialized somehow.
> >
> > Yes, seqcount writers cannot block each other, however, they are
> > supposed to be seralized with each other, right? So if we have the
> > reason to believe the above two CPUs case can happen, though it's not
> > a deadlock, but it's a data race, right?
> >
> > I think the proper fix here is to annotate vm_seqcount with the correct
> > locks serializing the writers.
> >
> 
> I agree with you now and that's the best way forward. I will work on
> something like this (unless you already did), thanks Boqun!
> 

Go ahead! I haven't started anything yet, although I did read a little
to understand what is needed to change.

Regards,
Boqun

> -Joel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-05-19  4:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-14 14:52 Silencing false lockdep warning related to seq lock Joel Fernandes
2021-05-17  4:21 ` Boqun Feng
2021-05-18  1:52   ` Joel Fernandes
2021-05-18  2:24     ` Boqun Feng
2021-05-18 15:53       ` Joel Fernandes
2021-05-19  4:50         ` Boqun Feng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).