Groups > linux.kernel > #1365105 > unrolled thread

Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260

Started by	Linus Torvalds <torvalds@linux-foundation.org>
First post	2016-03-27 14:10 +0200
Last post	2016-03-30 11:40 +0200
Articles	20 on this page of 27 — 8 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

  Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-27 14:10 +0200
    Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Boqun Feng <boqun.feng@gmail.com> - 2016-03-27 15:40 +0200
    Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Theodore Ts'o <tytso@mit.edu> - 2016-03-27 20:30 +0200
      Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-27 21:50 +0200
        Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-27 22:30 +0200
    Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Peter Zijlstra <peterz@infradead.org> - 2016-03-27 22:50 +0200
      Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-27 23:50 +0200
      Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Peter Zijlstra <peterz@infradead.org> - 2016-03-28 08:40 +0200
      Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Ingo Molnar <mingo@kernel.org> - 2016-03-29 10:50 +0200
        Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Peter Zijlstra <peterz@infradead.org> - 2016-03-30 11:40 +0200
          Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-30 12:00 +0200
            Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Peter Zijlstra <peterz@infradead.org> - 2016-03-30 14:50 +0200
              Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-30 14:50 +0200
                Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Peter Zijlstra <peterz@infradead.org> - 2016-03-30 15:20 +0200
          Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Peter Zijlstra <peterz@infradead.org> - 2016-03-30 12:00 +0200
          Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Boqun Feng <boqun.feng@gmail.com> - 2016-03-30 12:10 +0200
            Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Peter Zijlstra <peterz@infradead.org> - 2016-03-30 12:40 +0200
              Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-30 13:10 +0200
            Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Peter Zijlstra <peterz@infradead.org> - 2016-03-31 17:50 +0200
              Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Boqun Feng <boqun.feng@gmail.com> - 2016-03-31 18:00 +0200
              Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Sedat Dilek <sedat.dilek@gmail.com> - 2016-04-02 08:30 +0200
          Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Peter Zijlstra <peterz@infradead.org> - 2016-03-30 16:10 +0200
            Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-30 17:30 +0200
              [PATCH] lockdep: print chain_key collision information Alfredo Alvarez Fernandez <alfredoalvarezfernandez@gmail.com> - 2016-03-30 19:10 +0200
                Re: [PATCH] lockdep: print chain_key collision information Peter Zijlstra <peterz@infradead.org> - 2016-03-30 19:20 +0200
                [tip:core/urgent] locking/lockdep: Print chain_key collision  information tip-bot for Alfredo Alvarez Fernandez <tipbot@zytor.com> - 2016-04-01 08:40 +0200
        Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at  kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260 Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-30 11:40 +0200

Page 1 of 2 [1] 2 Next page →

#1365105 — Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260

From	Linus Torvalds <torvalds@linux-foundation.org>
Date	2016-03-27 14:10 +0200
Subject	Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260
Message-ID	<rhhG1-85z-1@gated-at.bofh.it>

On Sun, Mar 27, 2016 at 1:57 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> I pulled ext4.git#dev on top of Linux v4.6-rc1...
>
> ... and did not see the call-trace.

Unless you're using overlayfs or per-file encryption, I'm not seeing
that any of that should make any difference (but it's entirely
possible I'm missing something).

Was it entirely repeatable before? Maybe it just happened to happen
without that update, and then happened to _not_ happen after you
rebooted with that 'dev' branch pulled in?

Anyway, I don't think that DEBUG_LOCKS_WARN_ON() in

  kernel/locking/lockdep.c:2017 __lock_acquire

would be an ext4 issue, it looks more like an internal lockdep issue.

Adding in the lockdep people, who will set me right.

                 Linus

[toc] | [next] | [standalone]

#1365113

From	Boqun Feng <boqun.feng@gmail.com>
Date	2016-03-27 15:40 +0200
Message-ID	<rhj58-wK-9@gated-at.bofh.it>
In reply to	#1365105

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

On Sun, Mar 27, 2016 at 05:03:44AM -0700, Linus Torvalds wrote:
> On Sun, Mar 27, 2016 at 1:57 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
> >
> > I pulled ext4.git#dev on top of Linux v4.6-rc1...
> >
> > ... and did not see the call-trace.
> 
> Unless you're using overlayfs or per-file encryption, I'm not seeing
> that any of that should make any difference (but it's entirely
> possible I'm missing something).
> 
> Was it entirely repeatable before? Maybe it just happened to happen
> without that update, and then happened to _not_ happen after you
> rebooted with that 'dev' branch pulled in?
> 
> Anyway, I don't think that DEBUG_LOCKS_WARN_ON() in
> 
>   kernel/locking/lockdep.c:2017 __lock_acquire
> 

The code here is in check_no_collision(), so IIUC, there was a warning
because a real chain_key collision happened.

And chain_key is a hashsum of the ->class_idx of held_lock, calculated
via iterate_chain_key(), and the ->class_idx of a held_lock may change
from run to run IIUC, depending on the time register_lock_class() is
called for the corresponding lock class.

So this might be why Sedat didn't see the call-trace again.

Of course, I may miss something subtle here, so add the author of
check_no_collision() in CCs ;-)

If I'm right, maybe we can provide more informative dmesg here rather
than calling DEBUG_LOCKS_WARN_ON() directly?

Regards,
Boqun

> would be an ext4 issue, it looks more like an internal lockdep issue.
> 
> Adding in the lockdep people, who will set me right.
> 
>                  Linus

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

#1365160

From	Theodore Ts'o <tytso@mit.edu>
Date	2016-03-27 20:30 +0200
Message-ID	<rhnBL-3I9-1@gated-at.bofh.it>
In reply to	#1365105

On Sun, Mar 27, 2016 at 05:03:44AM -0700, Linus Torvalds wrote:
> 
> Unless you're using overlayfs or per-file encryption, I'm not seeing
> that any of that should make any difference (but it's entirely
> possible I'm missing something).
> 
> Was it entirely repeatable before? Maybe it just happened to happen
> without that update, and then happened to _not_ happen after you
> rebooted with that 'dev' branch pulled in?
> 
> Anyway, I don't think that DEBUG_LOCKS_WARN_ON() in
> 
>   kernel/locking/lockdep.c:2017 __lock_acquire
> 
> would be an ext4 issue, it looks more like an internal lockdep issue.

That's my guess.  I've been doing a lot of regression testing with
lockdep enabled, and I haven't seen the problem which Sedat has
reported.

At the moment I'm testing my ext4 bug fixes on top of 243d5067858310
(Merge branch 'overlayfs-linus'....) dating from March 22nd, and the
lockdep merges came much earlier than that, on March 15th, just two
days after v4.5 was released, and I'm not noticing any lockdep issues
with ext4 while running all of my regression tests.

     	  		       	  - Ted

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

#1365173

From	Sedat Dilek <sedat.dilek@gmail.com>
Date	2016-03-27 21:50 +0200
Message-ID	<rhoRc-4ub-17@gated-at.bofh.it>
In reply to	#1365160

On Sun, Mar 27, 2016 at 8:23 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Sun, Mar 27, 2016 at 05:03:44AM -0700, Linus Torvalds wrote:
>>
>> Unless you're using overlayfs or per-file encryption, I'm not seeing
>> that any of that should make any difference (but it's entirely
>> possible I'm missing something).
>>
>> Was it entirely repeatable before? Maybe it just happened to happen
>> without that update, and then happened to _not_ happen after you
>> rebooted with that 'dev' branch pulled in?
>>
>> Anyway, I don't think that DEBUG_LOCKS_WARN_ON() in
>>
>>   kernel/locking/lockdep.c:2017 __lock_acquire
>>
>> would be an ext4 issue, it looks more like an internal lockdep issue.
>
> That's my guess.  I've been doing a lot of regression testing with
> lockdep enabled, and I haven't seen the problem which Sedat has
> reported.
>
> At the moment I'm testing my ext4 bug fixes on top of 243d5067858310
> (Merge branch 'overlayfs-linus'....) dating from March 22nd, and the
> lockdep merges came much earlier than that, on March 15th, just two
> days after v4.5 was released, and I'm not noticing any lockdep issues
> with ext4 while running all of my regression tests.
>

So far I can say, that I am *not* seeing this with ext4.git#dev on top
of v4.6-rc1.

Not sure how I can force/reproduce the lockdep call-trace.

Any idea on how to check/test lockdep issues like this?
LTP? (Latest tarball: ltp-full-20160126.tar.xz?
xfstests?
xfstests-bld?
Does the linux-sources ship some test-suite?

- Sedat -

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

#1365179

From	Sedat Dilek <sedat.dilek@gmail.com>
Date	2016-03-27 22:30 +0200
Message-ID	<rhptT-4ZT-5@gated-at.bofh.it>
In reply to	#1365173

On Sun, Mar 27, 2016 at 9:42 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mar 27, 2016 14:40, "Sedat Dilek" <sedat.dilek@gmail.com> wrote:
>>
>> So far I can say, that I am *not* seeing this with ext4.git#dev on top
>> of v4.6-rc1.
>
> Mind re-testing just plain 4.6-rc1 again? It might not happen..
>

I needed to rebuild a 3rd one as I had thrown away my 1st binaries.

Hmm, I did 3 boots/reboots and could not see it with plain v4.6-rc1,
so hard to reproduce.

- Sedat -

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

#1365182

From	Peter Zijlstra <peterz@infradead.org>
Date	2016-03-27 22:50 +0200
Message-ID	<rhpNf-56p-1@gated-at.bofh.it>
In reply to	#1365105

On Sun, Mar 27, 2016 at 05:03:44AM -0700, Linus Torvalds wrote:
> Anyway, I don't think that DEBUG_LOCKS_WARN_ON() in
> 
>   kernel/locking/lockdep.c:2017 __lock_acquire
> 
> would be an ext4 issue, it looks more like an internal lockdep issue.
> 
> Adding in the lockdep people, who will set me right.

You are right; this is lockdep running into a hash collision; which is a
new DEBUG_LOCKDEP test. See 9e4e7554e755 ("locking/lockdep: Detect
chain_key collisions").

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

#1365199

From	Sedat Dilek <sedat.dilek@gmail.com>
Date	2016-03-27 23:50 +0200
Message-ID	<rhqJj-5PQ-1@gated-at.bofh.it>
In reply to	#1365182

On Sun, Mar 27, 2016 at 10:59 PM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
> On Sun, Mar 27, 2016 at 10:48 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> On Sun, Mar 27, 2016 at 05:03:44AM -0700, Linus Torvalds wrote:
>>> Anyway, I don't think that DEBUG_LOCKS_WARN_ON() in
>>>
>>>   kernel/locking/lockdep.c:2017 __lock_acquire
>>>
>>> would be an ext4 issue, it looks more like an internal lockdep issue.
>>>
>>> Adding in the lockdep people, who will set me right.
>>
>> You are right; this is lockdep running into a hash collision; which is a
>> new DEBUG_LOCKDEP test. See 9e4e7554e755 ("locking/lockdep: Detect
>> chain_key collisions").
>
> [1] says...
>
> "Also tested with lockdep's test suite after applying the patch:
>
> [ 0.000000] Good, all 253 testcases passed! |"
>
> Where can I find this "lockdep's test suite"?
>
> When is that checking below done or what causes this?
>
> $ grep -i lock dmesg_4.6.0-rc1-1-iniza-small.txt | grep -i dep
> [    0.000000]  RCU lockdep checking is enabled.
> [    0.000000] Lock dependency validator: Copyright (c) 2006 Red Hat,
> Inc., Ingo Molnar
> [    0.000000] ... MAX_LOCKDEP_SUBCLASSES:  8
> [    0.000000] ... MAX_LOCK_DEPTH:          48
> [    0.000000] ... MAX_LOCKDEP_KEYS:        8191
> [    0.000000] ... MAX_LOCKDEP_ENTRIES:     32768
> [    0.000000] ... MAX_LOCKDEP_CHAINS:      65536
> [    0.000000]  memory used by lock dependency info: 8159 kB
> [   77.403391] WARNING: CPU: 2 PID: 2692 at
> kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260
> [   77.403394] DEBUG_LOCKS_WARN_ON(chain->depth != curr->lockdep_depth
> - (i - 1))
>
> - Sedat -
>
> [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=9e4e7554e755

Hmm. I had several problems...

[ Building liblockdep ]

$ cd $BUILD_DIR

$ LC_ALL=C make -C tools/ liblockdep
make: Entering directory `/home/wearefam/src/linux-kernel/linux/tools'
  DESCEND  lib/lockdep
make[1]: Entering directory
`/home/wearefam/src/linux-kernel/linux/tools/lib/lockdep'
  CC       common.o
  CC       lockdep.o
  CC       preload.o
  CC       rbtree.o
  LD       liblockdep-in.o
  LD       liblockdep.a
  LD       liblockdep.so.4.6.0-rc1
make[1]: Leaving directory
`/home/wearefam/src/linux-kernel/linux/tools/lib/lockdep'
make: Leaving directory `/home/wearefam/src/linux-kernel/linux/tools'

[ run_tests.sh fails due to unsupported 'basename -s' ]

$ LC_ALL=C basename --version
basename (GNU coreutils) 8.13
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by David MacKenzie.

$ cd tools/lib/lockdep/

$ LC_ALL=C ./run_tests.sh
basename: invalid option -- 's'
Try `basename --help' for more information.
... timeout: failed to run command `./tests/': Permission denied
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
... timeout: failed to run command `./tests/': Permission denied
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
... timeout: failed to run command `./tests/': Permission denied
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
... timeout: failed to run command `./tests/': Permission denied
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
... timeout: failed to run command `./tests/': Permission denied
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
... timeout: failed to run command `./tests/': Permission denied
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
... timeout: failed to run command `./tests/': Permission denied
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
... timeout: failed to run command `./tests/': Permission denied
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
... timeout: failed to run command `./tests/': Permission denied
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
... timeout: failed to run command `./tests/': Permission denied
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
... timeout: failed to run command `./tests/': Permission denied
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
(PRELOAD) ... ./lockdep: line 3: ./tests/: Is a directory
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
(PRELOAD) ... ./lockdep: line 3: ./tests/: Is a directory
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
(PRELOAD) ... ./lockdep: line 3: ./tests/: Is a directory
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
(PRELOAD) ... ./lockdep: line 3: ./tests/: Is a directory
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
(PRELOAD) ... ./lockdep: line 3: ./tests/: Is a directory
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
(PRELOAD) ... ./lockdep: line 3: ./tests/: Is a directory
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
(PRELOAD) ... ./lockdep: line 3: ./tests/: Is a directory
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
(PRELOAD) ... ./lockdep: line 3: ./tests/: Is a directory
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
(PRELOAD) ... ./lockdep: line 3: ./tests/: Is a directory
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
(PRELOAD) ... ./lockdep: line 3: ./tests/: Is a directory
FAILED!
rm: cannot remove `tests/': Is a directory
basename: invalid option -- 's'
Try `basename --help' for more information.
(PRELOAD) ... ./lockdep: line 3: ./tests/: Is a directory
FAILED!
rm: cannot remove `tests/': Is a directory

[ Patching run_tests.sh (liblockdep) ]

--- a/tools/lib/lockdep/run_tests.sh
+++ b/tools/lib/lockdep/run_tests.sh
@@ -3,7 +3,7 @@
 make &> /dev/null

 for i in `ls tests/*.c`; do
-       testname=$(basename -s .c "$i")
+       testname=$(basename "$i" .c)
        gcc -o tests/$testname -pthread -lpthread $i liblockdep.a
-Iinclude -D__USE_LIBLOCKDEP &> /dev/null
        echo -ne "$testname... "
        if [ $(timeout 1 ./tests/$testname | wc -l) -gt 0 ]; then
@@ -11,11 +11,13 @@ for i in `ls tests/*.c`; do
        else
                echo "FAILED!"
        fi
-       rm tests/$testname
+       if [ -f "tests/$testname" ]; then
+               rm -v -f tests/$testname
+       fi
 done

 for i in `ls tests/*.c`; do
-       testname=$(basename -s .c "$i")
+       testname=$(basename "$i" .c)
        gcc -o tests/$testname -pthread -lpthread -Iinclude $i &> /dev/null
        echo -ne "(PRELOAD) $testname... "
        if [ $(timeout 1 ./lockdep ./tests/$testname | wc -l) -gt 0 ]; then
@@ -23,5 +25,7 @@ for i in `ls tests/*.c`; do
        else
                echo "FAILED!"
        fi
-       rm tests/$testname
+       if [ -f "tests/$testname" ]; then
+               rm -v -f tests/$testname
+       fi
 done

...then I get...

$ LC_ALL=C ./run_tests.sh
AA... PASSED!
removed `tests/AA'
ABA... PASSED!
removed `tests/ABA'
ABBA... PASSED!
removed `tests/ABBA'
ABBA_2threads... PASSED!
removed `tests/ABBA_2threads'
ABBCCA... PASSED!
removed `tests/ABBCCA'
ABBCCDDA... PASSED!
removed `tests/ABBCCDDA'
ABCABC... PASSED!
removed `tests/ABCABC'
ABCDBCDA... PASSED!
removed `tests/ABCDBCDA'
ABCDBDDA... PASSED!
removed `tests/ABCDBDDA'
WW... PASSED!
removed `tests/WW'
unlock_balance... PASSED!
removed `tests/unlock_balance'
(PRELOAD) AA... PASSED!
removed `tests/AA'
(PRELOAD) ABA... PASSED!
removed `tests/ABA'
(PRELOAD) ABBA... PASSED!
removed `tests/ABBA'
(PRELOAD) ABBA_2threads... PASSED!
removed `tests/ABBA_2threads'
(PRELOAD) ABBCCA... PASSED!
removed `tests/ABBCCA'
(PRELOAD) ABBCCDDA... PASSED!
removed `tests/ABBCCDDA'
(PRELOAD) ABCABC... PASSED!
removed `tests/ABCABC'
(PRELOAD) ABCDBCDA... PASSED!
removed `tests/ABCDBCDA'
(PRELOAD) ABCDBDDA... PASSED!
removed `tests/ABCDBDDA'
(PRELOAD) WW... PASSED!
removed `tests/WW'
(PRELOAD) unlock_balance... PASSED!
removed `tests/unlock_balance'

BTW, how did you test to get "[ 0.000000] Good, all 253 testcases passed!" from?

In my dmesg I see...

[ 3249.552034] show_signal_msg: 189 callbacks suppressed
[ 3249.552042] liblockdep.so[15757]: segfault at 1 ip 0000000000000001
sp 00007ffe82f88078 error 14 in
liblockdep.so.4.6.0-rc1[5578fbdbd000+c000]

Hmm, Hmm, Hmm.

Empty head,
- Sedat -

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

#1365356

From	Peter Zijlstra <peterz@infradead.org>
Date	2016-03-28 08:40 +0200
Message-ID	<rhz0e-36r-15@gated-at.bofh.it>
In reply to	#1365182

On Mon, Mar 28, 2016 at 09:05:09AM +0800, Boqun Feng wrote:
> On Sun, Mar 27, 2016 at 10:59:00PM +0200, Sedat Dilek wrote:
> > [1] says...
> > 
> > "Also tested with lockdep's test suite after applying the patch:
> > 
> > [ 0.000000] Good, all 253 testcases passed! |"
> > 
> > Where can I find this "lockdep's test suite"?

lib/locking-selftest*

> I think that means the self test suite enabled by
> CONFIG_DEBUG_LOCKING_API_SELFTESTS

Correct.

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

#1366011

From	Ingo Molnar <mingo@kernel.org>
Date	2016-03-29 10:50 +0200
Message-ID	<rhXvz-3qM-1@gated-at.bofh.it>
In reply to	#1365182

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Sun, Mar 27, 2016 at 05:03:44AM -0700, Linus Torvalds wrote:
> > Anyway, I don't think that DEBUG_LOCKS_WARN_ON() in
> > 
> >   kernel/locking/lockdep.c:2017 __lock_acquire
> > 
> > would be an ext4 issue, it looks more like an internal lockdep issue.
> > 
> > Adding in the lockdep people, who will set me right.
> 
> You are right; this is lockdep running into a hash collision; which is a new 
> DEBUG_LOCKDEP test. See 9e4e7554e755 ("locking/lockdep: Detect chain_key 
> collisions").

I've Cc:-ed Alfredo Alvarez Fernandez who added that test.

Thanks,

	Ingo

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

#1367039

From	Peter Zijlstra <peterz@infradead.org>
Date	2016-03-30 11:40 +0200
Message-ID	<rikLx-3iV-9@gated-at.bofh.it>
In reply to	#1366011

On Tue, Mar 29, 2016 at 10:47:02AM +0200, Ingo Molnar wrote:

> > You are right; this is lockdep running into a hash collision; which is a new 
> > DEBUG_LOCKDEP test. See 9e4e7554e755 ("locking/lockdep: Detect chain_key 
> > collisions").
> 
> I've Cc:-ed Alfredo Alvarez Fernandez who added that test.

OK, so while the code in check_no_collision() seems sensible, it relies
on borken bits.

The whole chain_hlocks and /proc/lockdep_chains stuff appears to have
been buggered from the start.

The below patch should fix this.

Furthermore, our hash function has definite room for improvement.

---
 include/linux/lockdep.h       |  8 +++++---
 kernel/locking/lockdep.c      | 30 ++++++++++++++++++++++++------
 kernel/locking/lockdep_proc.c |  2 ++
 3 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index d026b190c530..2568c120513b 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -196,9 +196,11 @@ struct lock_list {
  * We record lock dependency chains, so that we can cache them:
  */
 struct lock_chain {
-	u8				irq_context;
-	u8				depth;
-	u16				base;
+	/* see BUILD_BUG_ON()s in lookup_chain_cache() */
+	unsigned int			irq_context :  2,
+					depth       :  6,
+					base	    : 24;
+	/* 4 byte hole */
 	struct hlist_node		entry;
 	u64				chain_key;
 };
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 53ab2f85d77e..91a4b7780afb 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -2099,15 +2099,37 @@ static inline int lookup_chain_cache(struct task_struct *curr,
 	chain->irq_context = hlock->irq_context;
 	i = get_first_held_lock(curr, hlock);
 	chain->depth = curr->lockdep_depth + 1 - i;
+
+	BUILD_BUG_ON((1UL << 24) <= ARRAY_SIZE(chain_hlocks));
+	BUILD_BUG_ON((1UL << 6)  <= ARRAY_SIZE(curr->held_locks));
+	BUILD_BUG_ON((1UL << 8*sizeof(chain_hlocks[0])) <= ARRAY_SIZE(lock_classes));
+
 	if (likely(nr_chain_hlocks + chain->depth <= MAX_LOCKDEP_CHAIN_HLOCKS)) {
 		chain->base = nr_chain_hlocks;
-		nr_chain_hlocks += chain->depth;
 		for (j = 0; j < chain->depth - 1; j++, i++) {
 			int lock_id = curr->held_locks[i].class_idx - 1;
 			chain_hlocks[chain->base + j] = lock_id;
 		}
 		chain_hlocks[chain->base + j] = class - lock_classes;
 	}
+
+	if (nr_chain_hlocks < MAX_LOCKDEP_CHAIN_HLOCKS)
+		nr_chain_hlocks += chain->depth;
+
+#ifdef CONFIG_DEBUG_LOCKDEP
+	/*
+	 * Important for check_no_collision().
+	 */
+	if (unlikely(nr_chain_hlocks > MAX_LOCKDEP_CHAIN_HLOCKS)) {
+		if (debug_locks_off_graph_unlock())
+			return 0;
+
+		print_lockdep_off("BUG: MAX_LOCKDEP_CHAIN_HLOCKS too low!");
+		dump_stack();
+		return 0;
+	}
+#endif
+
 	hlist_add_head_rcu(&chain->entry, hash_head);
 	debug_atomic_inc(chain_lookup_misses);
 	inc_chains();
@@ -2860,11 +2882,6 @@ static int separate_irq_context(struct task_struct *curr,
 {
 	unsigned int depth = curr->lockdep_depth;
 
-	/*
-	 * Keep track of points where we cross into an interrupt context:
-	 */
-	hlock->irq_context = 2*(curr->hardirq_context ? 1 : 0) +
-				curr->softirq_context;
 	if (depth) {
 		struct held_lock *prev_hlock;
 
@@ -3164,6 +3181,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 	hlock->acquire_ip = ip;
 	hlock->instance = lock;
 	hlock->nest_lock = nest_lock;
+	hlock->irq_context = 2*(!!curr->hardirq_context) + !!curr->softirq_context;
 	hlock->trylock = trylock;
 	hlock->read = read;
 	hlock->check = check;
diff --git a/kernel/locking/lockdep_proc.c b/kernel/locking/lockdep_proc.c
index dbb61a302548..a0f61effad25 100644
--- a/kernel/locking/lockdep_proc.c
+++ b/kernel/locking/lockdep_proc.c
@@ -141,6 +141,8 @@ static int lc_show(struct seq_file *m, void *v)
 	int i;
 
 	if (v == SEQ_START_TOKEN) {
+		if (nr_chain_hlocks > MAX_LOCKDEP_CHAIN_HLOCKS)
+			seq_printf(m, "(buggered) ");
 		seq_printf(m, "all lock chains:\n");
 		return 0;
 	}

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

#1367056

From	Sedat Dilek <sedat.dilek@gmail.com>
Date	2016-03-30 12:00 +0200
Message-ID	<ril4T-3q2-11@gated-at.bofh.it>
In reply to	#1367039

On Wed, Mar 30, 2016 at 11:36 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Mar 29, 2016 at 10:47:02AM +0200, Ingo Molnar wrote:
>
>> > You are right; this is lockdep running into a hash collision; which is a new
>> > DEBUG_LOCKDEP test. See 9e4e7554e755 ("locking/lockdep: Detect chain_key
>> > collisions").
>>
>> I've Cc:-ed Alfredo Alvarez Fernandez who added that test.
>
> OK, so while the code in check_no_collision() seems sensible, it relies
> on borken bits.
>
> The whole chain_hlocks and /proc/lockdep_chains stuff appears to have
> been buggered from the start.
>
> The below patch should fix this.
>

checkpatch.pl says...

WARNING: Prefer seq_puts to seq_printf
#124: FILE: kernel/locking/lockdep_proc.c:145:
+                       seq_printf(m, "(buggered) ");

Testing your patch right now.

- Sedat -


> Furthermore, our hash function has definite room for improvement.
>
> ---
>  include/linux/lockdep.h       |  8 +++++---
>  kernel/locking/lockdep.c      | 30 ++++++++++++++++++++++++------
>  kernel/locking/lockdep_proc.c |  2 ++
>  3 files changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index d026b190c530..2568c120513b 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -196,9 +196,11 @@ struct lock_list {
>   * We record lock dependency chains, so that we can cache them:
>   */
>  struct lock_chain {
> -       u8                              irq_context;
> -       u8                              depth;
> -       u16                             base;
> +       /* see BUILD_BUG_ON()s in lookup_chain_cache() */
> +       unsigned int                    irq_context :  2,
> +                                       depth       :  6,
> +                                       base        : 24;
> +       /* 4 byte hole */
>         struct hlist_node               entry;
>         u64                             chain_key;
>  };
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 53ab2f85d77e..91a4b7780afb 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -2099,15 +2099,37 @@ static inline int lookup_chain_cache(struct task_struct *curr,
>         chain->irq_context = hlock->irq_context;
>         i = get_first_held_lock(curr, hlock);
>         chain->depth = curr->lockdep_depth + 1 - i;
> +
> +       BUILD_BUG_ON((1UL << 24) <= ARRAY_SIZE(chain_hlocks));
> +       BUILD_BUG_ON((1UL << 6)  <= ARRAY_SIZE(curr->held_locks));
> +       BUILD_BUG_ON((1UL << 8*sizeof(chain_hlocks[0])) <= ARRAY_SIZE(lock_classes));
> +
>         if (likely(nr_chain_hlocks + chain->depth <= MAX_LOCKDEP_CHAIN_HLOCKS)) {
>                 chain->base = nr_chain_hlocks;
> -               nr_chain_hlocks += chain->depth;
>                 for (j = 0; j < chain->depth - 1; j++, i++) {
>                         int lock_id = curr->held_locks[i].class_idx - 1;
>                         chain_hlocks[chain->base + j] = lock_id;
>                 }
>                 chain_hlocks[chain->base + j] = class - lock_classes;
>         }
> +
> +       if (nr_chain_hlocks < MAX_LOCKDEP_CHAIN_HLOCKS)
> +               nr_chain_hlocks += chain->depth;
> +
> +#ifdef CONFIG_DEBUG_LOCKDEP
> +       /*
> +        * Important for check_no_collision().
> +        */
> +       if (unlikely(nr_chain_hlocks > MAX_LOCKDEP_CHAIN_HLOCKS)) {
> +               if (debug_locks_off_graph_unlock())
> +                       return 0;
> +
> +               print_lockdep_off("BUG: MAX_LOCKDEP_CHAIN_HLOCKS too low!");
> +               dump_stack();
> +               return 0;
> +       }
> +#endif
> +
>         hlist_add_head_rcu(&chain->entry, hash_head);
>         debug_atomic_inc(chain_lookup_misses);
>         inc_chains();
> @@ -2860,11 +2882,6 @@ static int separate_irq_context(struct task_struct *curr,
>  {
>         unsigned int depth = curr->lockdep_depth;
>
> -       /*
> -        * Keep track of points where we cross into an interrupt context:
> -        */
> -       hlock->irq_context = 2*(curr->hardirq_context ? 1 : 0) +
> -                               curr->softirq_context;
>         if (depth) {
>                 struct held_lock *prev_hlock;
>
> @@ -3164,6 +3181,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
>         hlock->acquire_ip = ip;
>         hlock->instance = lock;
>         hlock->nest_lock = nest_lock;
> +       hlock->irq_context = 2*(!!curr->hardirq_context) + !!curr->softirq_context;
>         hlock->trylock = trylock;
>         hlock->read = read;
>         hlock->check = check;
> diff --git a/kernel/locking/lockdep_proc.c b/kernel/locking/lockdep_proc.c
> index dbb61a302548..a0f61effad25 100644
> --- a/kernel/locking/lockdep_proc.c
> +++ b/kernel/locking/lockdep_proc.c
> @@ -141,6 +141,8 @@ static int lc_show(struct seq_file *m, void *v)
>         int i;
>
>         if (v == SEQ_START_TOKEN) {
> +               if (nr_chain_hlocks > MAX_LOCKDEP_CHAIN_HLOCKS)
> +                       seq_printf(m, "(buggered) ");
>                 seq_printf(m, "all lock chains:\n");
>                 return 0;
>         }

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

#1367180

From	Peter Zijlstra <peterz@infradead.org>
Date	2016-03-30 14:50 +0200
Message-ID	<rinJo-5qT-19@gated-at.bofh.it>
In reply to	#1367056

On Wed, Mar 30, 2016 at 11:49:57AM +0200, Sedat Dilek wrote:
> On Wed, Mar 30, 2016 at 11:36 AM, Peter Zijlstra <peterz@infradead.org> wrote:

> > OK, so while the code in check_no_collision() seems sensible, it relies
> > on borken bits.
> >
> > The whole chain_hlocks and /proc/lockdep_chains stuff appears to have
> > been buggered from the start.
> >
> > The below patch should fix this.
> >
> 
> checkpatch.pl says...
> 
> WARNING: Prefer seq_puts to seq_printf
> #124: FILE: kernel/locking/lockdep_proc.c:145:
> +                       seq_printf(m, "(buggered) ");

Yeah, sod checkpatch ;-)

What's in your /proc/lockdep_stats file?

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

#1367182

From	Sedat Dilek <sedat.dilek@gmail.com>
Date	2016-03-30 14:50 +0200
Message-ID	<rinJp-5qT-25@gated-at.bofh.it>
In reply to	#1367180

On Wed, Mar 30, 2016 at 2:43 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Mar 30, 2016 at 11:49:57AM +0200, Sedat Dilek wrote:
>> On Wed, Mar 30, 2016 at 11:36 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
>> > OK, so while the code in check_no_collision() seems sensible, it relies
>> > on borken bits.
>> >
>> > The whole chain_hlocks and /proc/lockdep_chains stuff appears to have
>> > been buggered from the start.
>> >
>> > The below patch should fix this.
>> >
>>
>> checkpatch.pl says...
>>
>> WARNING: Prefer seq_puts to seq_printf
>> #124: FILE: kernel/locking/lockdep_proc.c:145:
>> +                       seq_printf(m, "(buggered) ");
>
> Yeah, sod checkpatch ;-)
>
> What's in your /proc/lockdep_stats file?

Eat thiz!

$ sudo cat /proc/lockdep_stats
 lock-classes:                         2012 [max: 8191]
 direct dependencies:                  9638 [max: 32768]
 indirect dependencies:               39300
 all direct dependencies:            256286
 dependency chains:                   12869 [max: 65536]
 dependency chain hlocks:             49608 [max: 327680]
 in-hardirq chains:                     115
 in-softirq chains:                     458
 in-process chains:                   11504
 stack-trace entries:                154861 [max: 524288]
 combined max dependencies:       612572220
 hardirq-safe locks:                     61
 hardirq-unsafe locks:                 1032
 softirq-safe locks:                    169
 softirq-unsafe locks:                  949
 irq-safe locks:                        178
 irq-unsafe locks:                     1032
 hardirq-read-safe locks:                 4
 hardirq-read-unsafe locks:             226
 softirq-read-safe locks:                 8
 softirq-read-unsafe locks:             221
 irq-read-safe locks:                     9
 irq-read-unsafe locks:                 226
 uncategorized locks:                   216
 unused locks:                            0
 max locking depth:                      17
 max bfs queue depth:                   354
 chain lookup misses:                 12974
 chain lookup hits:                36326533
 cyclic checks:                       11430
 find-mask forwards checks:            3952
 find-mask backwards checks:          74700
 hardirq on events:                41715052
 hardirq off events:               41715056
 redundant hardirq ons:                 404
 redundant hardirq offs:           19500606
 softirq on events:                  220687
 softirq off events:                 220715
 redundant softirq ons:                   0
 redundant softirq offs:                  0
 debug_locks:                             1

- Sedat -

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

#1367198

From	Peter Zijlstra <peterz@infradead.org>
Date	2016-03-30 15:20 +0200
Message-ID	<riocq-5Wf-5@gated-at.bofh.it>
In reply to	#1367182

On Wed, Mar 30, 2016 at 02:46:36PM +0200, Sedat Dilek wrote:
>  dependency chain hlocks:             49608 [max: 327680]

OK, so that is still below the u16 limit, so you're seeing an actual
hash collision and my patch will not cure that.

A different hash function _might_ help, but eventually this is an
unfixable problem. Our input space is (2^13)^48 = 2^(13*48) = 2^624 =
ff'n huge, reducing that to 2^64 is bound to generate a collision at some
point.

[ technically the 48 held_lock spots are not fully independent, so
  (2^13)^48 is slightly overestimating it, but the numbers are big
  enough for this to not matter much. ]

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

#1367057

From	Peter Zijlstra <peterz@infradead.org>
Date	2016-03-30 12:00 +0200
Message-ID	<ril4T-3q2-15@gated-at.bofh.it>
In reply to	#1367039

On Wed, Mar 30, 2016 at 11:36:59AM +0200, Peter Zijlstra wrote:
> On Tue, Mar 29, 2016 at 10:47:02AM +0200, Ingo Molnar wrote:
> 
> > > You are right; this is lockdep running into a hash collision; which is a new 
> > > DEBUG_LOCKDEP test. See 9e4e7554e755 ("locking/lockdep: Detect chain_key 
> > > collisions").
> > 
> > I've Cc:-ed Alfredo Alvarez Fernandez who added that test.
> 
> OK, so while the code in check_no_collision() seems sensible, it relies
> on borken bits.
> 
> The whole chain_hlocks and /proc/lockdep_chains stuff appears to have
> been buggered from the start.
> 
> The below patch should fix this.

Note that unless we had more than 65536 chain_hlocks consumed the patch
would not make a difference.

> Furthermore, our hash function has definite room for improvement.

And no matter how good we make it, a u64 hash is bound to collide at
some point (or of any size really).

Also, we could make them non-fatal, returning true from
lookup_chain_cache() is always correct (_very_ expensive, but correct),
so in case of doubt we could just return true.

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

#1367068

From	Boqun Feng <boqun.feng@gmail.com>
Date	2016-03-30 12:10 +0200
Message-ID	<rilez-3Ki-13@gated-at.bofh.it>
In reply to	#1367039

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

Hi Peter,

On Wed, Mar 30, 2016 at 11:36:59AM +0200, Peter Zijlstra wrote:
> On Tue, Mar 29, 2016 at 10:47:02AM +0200, Ingo Molnar wrote:
> 
> > > You are right; this is lockdep running into a hash collision; which is a new 
> > > DEBUG_LOCKDEP test. See 9e4e7554e755 ("locking/lockdep: Detect chain_key 
> > > collisions").
> > 
> > I've Cc:-ed Alfredo Alvarez Fernandez who added that test.
> 
> OK, so while the code in check_no_collision() seems sensible, it relies
> on borken bits.
> 
> The whole chain_hlocks and /proc/lockdep_chains stuff appears to have
> been buggered from the start.
> 
> The below patch should fix this.
> 
> Furthermore, our hash function has definite room for improvement.
> 
> ---
>  include/linux/lockdep.h       |  8 +++++---
>  kernel/locking/lockdep.c      | 30 ++++++++++++++++++++++++------
>  kernel/locking/lockdep_proc.c |  2 ++
>  3 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index d026b190c530..2568c120513b 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -196,9 +196,11 @@ struct lock_list {
>   * We record lock dependency chains, so that we can cache them:
>   */
>  struct lock_chain {
> -	u8				irq_context;
> -	u8				depth;
> -	u16				base;
> +	/* see BUILD_BUG_ON()s in lookup_chain_cache() */
> +	unsigned int			irq_context :  2,
> +					depth       :  6,
> +					base	    : 24;
> +	/* 4 byte hole */
>  	struct hlist_node		entry;
>  	u64				chain_key;
>  };
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 53ab2f85d77e..91a4b7780afb 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c

[...]

> @@ -2860,11 +2882,6 @@ static int separate_irq_context(struct task_struct *curr,
>  {
>  	unsigned int depth = curr->lockdep_depth;
>  
> -	/*
> -	 * Keep track of points where we cross into an interrupt context:
> -	 */
> -	hlock->irq_context = 2*(curr->hardirq_context ? 1 : 0) +
> -				curr->softirq_context;
>  	if (depth) {
>  		struct held_lock *prev_hlock;
>  
> @@ -3164,6 +3181,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
>  	hlock->acquire_ip = ip;
>  	hlock->instance = lock;
>  	hlock->nest_lock = nest_lock;
> +	hlock->irq_context = 2*(!!curr->hardirq_context) + !!curr->softirq_context;
>  	hlock->trylock = trylock;
>  	hlock->read = read;
>  	hlock->check = check;

This is just for cleaning up, right? However ->hardirq_context and
->softirq_context only defined when CONFIG_TRACE_IRQFLAGS=y.

So we should use macro like current_hardirq_context() here? Or
considering the two helpers introduced in my RFC:

http://lkml.kernel.org/g/1455602265-16490-2-git-send-email-boqun.feng@gmail.com

if you don't think that overkills ;-)

Regards,
Boqun

[...]

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

#1367084

From	Peter Zijlstra <peterz@infradead.org>
Date	2016-03-30 12:40 +0200
Message-ID	<rilHA-3WZ-25@gated-at.bofh.it>
In reply to	#1367068

On Wed, Mar 30, 2016 at 05:59:54PM +0800, Boqun Feng wrote:
> > @@ -3164,6 +3181,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
> >  	hlock->acquire_ip = ip;
> >  	hlock->instance = lock;
> >  	hlock->nest_lock = nest_lock;
> > +	hlock->irq_context = 2*(!!curr->hardirq_context) + !!curr->softirq_context;
> >  	hlock->trylock = trylock;
> >  	hlock->read = read;
> >  	hlock->check = check;
> 
> This is just for cleaning up, right? However ->hardirq_context and
> ->softirq_context only defined when CONFIG_TRACE_IRQFLAGS=y.

Ah, that is the reason it was in a 'funny' place.

The other reason is that we're careful to reduce hardirq_context to 0,1
but don't do so for softirq_context.

> So we should use macro like current_hardirq_context() here? Or
> considering the two helpers introduced in my RFC:
> 
> http://lkml.kernel.org/g/1455602265-16490-2-git-send-email-boqun.feng@gmail.com
> 
> if you don't think that overkills ;-)

Yeah, that might work, although I would like to keep the !! on both,
makes me worry less.

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

#1367104

From	Sedat Dilek <sedat.dilek@gmail.com>
Date	2016-03-30 13:10 +0200
Message-ID	<rimaC-4sj-15@gated-at.bofh.it>
In reply to	#1367084

On Wed, Mar 30, 2016 at 12:36 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Mar 30, 2016 at 05:59:54PM +0800, Boqun Feng wrote:
>> > @@ -3164,6 +3181,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
>> >     hlock->acquire_ip = ip;
>> >     hlock->instance = lock;
>> >     hlock->nest_lock = nest_lock;
>> > +   hlock->irq_context = 2*(!!curr->hardirq_context) + !!curr->softirq_context;
>> >     hlock->trylock = trylock;
>> >     hlock->read = read;
>> >     hlock->check = check;
>>
>> This is just for cleaning up, right? However ->hardirq_context and
>> ->softirq_context only defined when CONFIG_TRACE_IRQFLAGS=y.
>
> Ah, that is the reason it was in a 'funny' place.
>
> The other reason is that we're careful to reduce hardirq_context to 0,1
> but don't do so for softirq_context.
>
>> So we should use macro like current_hardirq_context() here? Or
>> considering the two helpers introduced in my RFC:
>>
>> http://lkml.kernel.org/g/1455602265-16490-2-git-send-email-boqun.feng@gmail.com
>>
>> if you don't think that overkills ;-)
>
> Yeah, that might work, although I would like to keep the !! on both,
> makes me worry less.

Can you CC me on any new patches in this area?

Thanks.

- Sedat -

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

#1368409

From	Peter Zijlstra <peterz@infradead.org>
Date	2016-03-31 17:50 +0200
Message-ID	<riN18-7fd-9@gated-at.bofh.it>
In reply to	#1367068

On Wed, Mar 30, 2016 at 05:59:54PM +0800, Boqun Feng wrote:
> So we should use macro like current_hardirq_context() here? Or
> considering the two helpers introduced in my RFC:
> 
> http://lkml.kernel.org/g/1455602265-16490-2-git-send-email-boqun.feng@gmail.com
> 
> if you don't think that overkills ;-)

I changed it into the below; since I did significant edits, let me know
if you disagree and / or want your name taken off.

---
Subject: lockdep: Add task_irq_context()
From: Boqun Feng <boqun.feng@gmail.com>
Date: Tue, 16 Feb 2016 13:57:40 +0800

task_irq_context(): returns the encoded irq_context of the task, the
return value is encoded in the same as ->irq_context of held_lock.
Always return 0 if !(CONFIG_TRACE_IRQFLAGS && CONFIG_PROVE_LOCKING)

Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: sasha.levin@oracle.com
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1455602265-16490-2-git-send-email-boqun.feng@gmail.com
---
 kernel/locking/lockdep.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -2932,6 +2932,11 @@ static int mark_irqflags(struct task_str
 	return 1;
 }
 
+static inline unsigned int task_irq_context(struct task_struct *task)
+{
+	return 2 * !!task->hardirq_context + !!task->softirq_context;
+}
+
 static int separate_irq_context(struct task_struct *curr,
 		struct held_lock *hlock)
 {
@@ -2940,8 +2945,6 @@ static int separate_irq_context(struct t
 	/*
 	 * Keep track of points where we cross into an interrupt context:
 	 */
-	hlock->irq_context = 2*(curr->hardirq_context ? 1 : 0) +
-				curr->softirq_context;
 	if (depth) {
 		struct held_lock *prev_hlock;
 
@@ -2973,6 +2976,11 @@ static inline int mark_irqflags(struct t
 	return 1;
 }
 
+static inline unsigned int task_irq_context(struct task_struct *task)
+{
+	return 0;
+}
+
 static inline int separate_irq_context(struct task_struct *curr,
 		struct held_lock *hlock)
 {
@@ -3241,6 +3249,7 @@ static int __lock_acquire(struct lockdep
 	hlock->acquire_ip = ip;
 	hlock->instance = lock;
 	hlock->nest_lock = nest_lock;
+	hlock->irq_context = task_irq_context(curr);
 	hlock->trylock = trylock;
 	hlock->read = read;
 	hlock->check = check;

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

#1368414

From	Boqun Feng <boqun.feng@gmail.com>
Date	2016-03-31 18:00 +0200
Message-ID	<riNaN-7j4-7@gated-at.bofh.it>
In reply to	#1368409

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

On Thu, Mar 31, 2016 at 05:42:34PM +0200, Peter Zijlstra wrote:
> On Wed, Mar 30, 2016 at 05:59:54PM +0800, Boqun Feng wrote:
> > So we should use macro like current_hardirq_context() here? Or
> > considering the two helpers introduced in my RFC:
> > 
> > http://lkml.kernel.org/g/1455602265-16490-2-git-send-email-boqun.feng@gmail.com
> > 
> > if you don't think that overkills ;-)
> 
> I changed it into the below; since I did significant edits, let me know
> if you disagree and / or want your name taken off.
> 

Thank you, Peter! It looks good to me ;-)

Regards,
Boqun

> ---
> Subject: lockdep: Add task_irq_context()
> From: Boqun Feng <boqun.feng@gmail.com>
> Date: Tue, 16 Feb 2016 13:57:40 +0800
> 
> task_irq_context(): returns the encoded irq_context of the task, the
> return value is encoded in the same as ->irq_context of held_lock.
> Always return 0 if !(CONFIG_TRACE_IRQFLAGS && CONFIG_PROVE_LOCKING)
> 
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: sasha.levin@oracle.com
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: http://lkml.kernel.org/r/1455602265-16490-2-git-send-email-boqun.feng@gmail.com
> ---
>  kernel/locking/lockdep.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -2932,6 +2932,11 @@ static int mark_irqflags(struct task_str
>  	return 1;
>  }
>  
> +static inline unsigned int task_irq_context(struct task_struct *task)
> +{
> +	return 2 * !!task->hardirq_context + !!task->softirq_context;
> +}
> +
>  static int separate_irq_context(struct task_struct *curr,
>  		struct held_lock *hlock)
>  {
> @@ -2940,8 +2945,6 @@ static int separate_irq_context(struct t
>  	/*
>  	 * Keep track of points where we cross into an interrupt context:
>  	 */
> -	hlock->irq_context = 2*(curr->hardirq_context ? 1 : 0) +
> -				curr->softirq_context;
>  	if (depth) {
>  		struct held_lock *prev_hlock;
>  
> @@ -2973,6 +2976,11 @@ static inline int mark_irqflags(struct t
>  	return 1;
>  }
>  
> +static inline unsigned int task_irq_context(struct task_struct *task)
> +{
> +	return 0;
> +}
> +
>  static inline int separate_irq_context(struct task_struct *curr,
>  		struct held_lock *hlock)
>  {
> @@ -3241,6 +3249,7 @@ static int __lock_acquire(struct lockdep
>  	hlock->acquire_ip = ip;
>  	hlock->instance = lock;
>  	hlock->nest_lock = nest_lock;
> +	hlock->irq_context = task_irq_context(curr);
>  	hlock->trylock = trylock;
>  	hlock->read = read;
>  	hlock->check = check;

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

Page 1 of 2 [1] 2 Next page →

csiph-web

Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260

Contents

#1365105 — Re: [Linux-v4.6-rc1] ext4: WARNING: CPU: 2 PID: 2692 at kernel/locking/lockdep.c:2017 __lock_acquire+0x180e/0x2260

#1365113

#1365160

#1365173

#1365179

#1365182

#1365199

#1365356

#1366011

#1367039

#1367056

#1367180

#1367182

#1367198

#1367057

#1367068

#1367084

#1367104

#1368409

#1368414