* Doubled stack dumps during locking testsuite
@ 2006-12-16 6:26 Matthew Wilcox
2006-12-16 8:04 ` [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent Ingo Molnar
0 siblings, 1 reply; 8+ messages in thread
From: Matthew Wilcox @ 2006-12-16 6:26 UTC (permalink / raw)
To: Ingo Molnar; +Cc: linux-kernel
Hi Ingo,
I built a parisc kernel with CONFIG_DEBUG_LOCKING_API_SELFTESTS enabled
recently, and got some interesting results:
double unlock: ok | ok |failed|WARNING at /home/willy/merge-2.6/kernel/mutex-debug.c:83 debug_mutex_unlock()
Backtrace:
[<0000000040144b38>] printk+0x40/0x50
[<000000004028d5f4>] init_class_Z+0xa4/0xc0
[<0000000040144b38>] printk+0x40/0x50
[<0000000040171fbc>] register_cpu_notifier+0x5c/0x78
[<0000000040340212>] lpfc_sli_issue_mbox_wait+0x138/0x1c8
[<0000000040204d01>] sg_grt_trans+0x150/0x168
[<0000000040285700>] kobject_uevent+0x8/0x20
[<0000000040304d65>] scsi_error_handler+0x9f4/0xa48
[<0000000040207450>] dump_write+0x40/0x58
[<000000004044c67c>] packet_getsockopt+0x144/0x150
[<000000004044b05c>] packet_ioctl+0x1a4/0x1b0
[<00000000404466d4>] unix_ioctl+0x154/0x168
[<000000004042d5dc>] ipv4_sysctl_forward_strategy+0x12c/0x138
[<000000004042d558>] ipv4_sysctl_forward_strategy+0xa8/0x138
[<000000004042c1d4>] ip_mc_msfget+0x16c/0x1d0
[<00000000404230a8>] ipv4_doint_and_flush_strategy+0x110/0x138
WARNING at /home/willy/merge-2.6/kernel/mutex-debug.c:86 debug_mutex_unlock()
Backtrace:
[<0000000040144b38>] printk+0x40/0x50
[<000000004028d5f4>] init_class_Z+0xa4/0xc0
[<0000000040144b38>] printk+0x40/0x50
[<0000000040171fbc>] register_cpu_notifier+0x5c/0x78
[<0000000040340212>] lpfc_sli_issue_mbox_wait+0x138/0x1c8
[<0000000040204d01>] sg_grt_trans+0x150/0x168
[<0000000040285700>] kobject_uevent+0x8/0x20
[<0000000040304d65>] scsi_error_handler+0x9f4/0xa48
[<0000000040207450>] dump_write+0x40/0x58
[<000000004044c67c>] packet_getsockopt+0x144/0x150
[<000000004044b05c>] packet_ioctl+0x1a4/0x1b0
[<00000000404466d4>] unix_ioctl+0x154/0x168
[<000000004042d5dc>] ipv4_sysctl_forward_strategy+0x12c/0x138
[<000000004042d558>] ipv4_sysctl_forward_strategy+0xa8/0x138
[<000000004042c1d4>] ip_mc_msfget+0x16c/0x1d0
[<00000000404230a8>] ipv4_doint_and_flush_strategy+0x110/0x138
failed|failed|failed|
Now, lines 83 to 86 of mutex-debug.c are:
DEBUG_LOCKS_WARN_ON(lock->owner != current_thread_info());
DEBUG_LOCKS_WARN_ON(lock->magic != lock);
DEBUG_LOCKS_WARN_ON(!lock->wait_list.prev && !lock->wait_list.next);
DEBUG_LOCKS_WARN_ON(lock->owner != current_thread_info());
Do we really need to test the same thing twice and produce the same
stack dump?
Moreover, do we want to get stack dumps while running the locking
testsuite in the first place? From various comments, it looks
like it's supposed to be turned off, but it looks like the sense of
debug_locks_silent is inverted in the definition of DEBUG_LOCKS_WARN_ON:
if (unlikely(c)) { \
if (debug_locks_silent || debug_locks_off()) \
WARN_ON(1); \
Surely that should be:
if (!debug_locks_silent && debug_locks_off())
WARN_ON(1);
^ permalink raw reply [flat|nested] 8+ messages in thread* [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent
2006-12-16 6:26 Doubled stack dumps during locking testsuite Matthew Wilcox
@ 2006-12-16 8:04 ` Ingo Molnar
2006-12-19 8:43 ` Jarek Poplawski
0 siblings, 1 reply; 8+ messages in thread
From: Ingo Molnar @ 2006-12-16 8:04 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-kernel, Andrew Morton
* Matthew Wilcox <matthew@wil.cx> wrote:
> Moreover, do we want to get stack dumps while running the locking
> testsuite in the first place? From various comments, it looks like
> it's supposed to be turned off, but it looks like the sense of
> debug_locks_silent is inverted in the definition of
> DEBUG_LOCKS_WARN_ON:
>
> if (unlikely(c)) { \
> if (debug_locks_silent || debug_locks_off()) \
> WARN_ON(1); \
>
> Surely that should be:
>
> if (!debug_locks_silent && debug_locks_off())
> WARN_ON(1);
oops, indeed! Fix below.
Ingo
------------->
Subject: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent
From: Ingo Molnar <mingo@elte.hu>
Matthew Wilcox noticed that the debug_locks_silent use should be
inverted in DEBUG_LOCKS_WARN_ON(). This bug was causing spurious
stacktraces and incorrect failures in the locking self-test on the
parisc kernel.
Bug-found-by: Matthew Wilcox <matthew@wil.cx>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
include/linux/debug_locks.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux/include/linux/debug_locks.h
===================================================================
--- linux.orig/include/linux/debug_locks.h
+++ linux/include/linux/debug_locks.h
@@ -24,7 +24,7 @@ extern int debug_locks_off(void);
int __ret = 0; \
\
if (unlikely(c)) { \
- if (debug_locks_silent || debug_locks_off()) \
+ if (!debug_locks_silent && debug_locks_off()) \
WARN_ON(1); \
__ret = 1; \
} \
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent
2006-12-16 8:04 ` [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent Ingo Molnar
@ 2006-12-19 8:43 ` Jarek Poplawski
2006-12-19 8:51 ` Matthew Wilcox
2006-12-19 9:31 ` Ingo Molnar
0 siblings, 2 replies; 8+ messages in thread
From: Jarek Poplawski @ 2006-12-19 8:43 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Matthew Wilcox
On 16-12-2006 09:04, Ingo Molnar wrote:
> * Matthew Wilcox <matthew@wil.cx> wrote:
...
> Bug-found-by: Matthew Wilcox <matthew@wil.cx>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
> include/linux/debug_locks.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: linux/include/linux/debug_locks.h
> ===================================================================
> --- linux.orig/include/linux/debug_locks.h
> +++ linux/include/linux/debug_locks.h
> @@ -24,7 +24,7 @@ extern int debug_locks_off(void);
> int __ret = 0; \
> \
> if (unlikely(c)) { \
> - if (debug_locks_silent || debug_locks_off()) \
> + if (!debug_locks_silent && debug_locks_off()) \
> WARN_ON(1); \
> __ret = 1; \
> } \
I wonder why doing debug_locks_off depends here on
debug_lock_silent state which is only "esthetical"
flag. And debug_locks_off() takes into consideration
debug_lock_silent after all. So IMHO:
if (unlikely(c)) { \
if (debug_locks_off()) \
WARN_ON(1); \
__ret = 1; \
} \
Jarek P.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent
2006-12-19 8:43 ` Jarek Poplawski
@ 2006-12-19 8:51 ` Matthew Wilcox
2006-12-19 9:05 ` Jarek Poplawski
2006-12-19 9:31 ` Ingo Molnar
1 sibling, 1 reply; 8+ messages in thread
From: Matthew Wilcox @ 2006-12-19 8:51 UTC (permalink / raw)
To: Jarek Poplawski, linux-kernel, Ingo Molnar, Andrew Morton
On Tue, Dec 19, 2006 at 09:43:59AM +0100, Jarek Poplawski wrote:
> I wonder why doing debug_locks_off depends here on
> debug_lock_silent state which is only "esthetical"
> flag. And debug_locks_off() takes into consideration
> debug_lock_silent after all. So IMHO:
It's not 'aesthetic' at all. It's used to say "We are about to cause a
locking failure deliberately as part of the test suite". It would be
wrong to disable lock debugging as a result of running the test suite.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent
2006-12-19 8:51 ` Matthew Wilcox
@ 2006-12-19 9:05 ` Jarek Poplawski
0 siblings, 0 replies; 8+ messages in thread
From: Jarek Poplawski @ 2006-12-19 9:05 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-kernel, Ingo Molnar, Andrew Morton
On Tue, Dec 19, 2006 at 01:51:03AM -0700, Matthew Wilcox wrote:
> On Tue, Dec 19, 2006 at 09:43:59AM +0100, Jarek Poplawski wrote:
> > I wonder why doing debug_locks_off depends here on
> > debug_lock_silent state which is only "esthetical"
> > flag. And debug_locks_off() takes into consideration
> > debug_lock_silent after all. So IMHO:
>
> It's not 'aesthetic' at all. It's used to say "We are about to cause a
> locking failure deliberately as part of the test suite". It would be
> wrong to disable lock debugging as a result of running the test suite.
So it's probably something with my English...
>From lib/debug_locks.c:
"/*
* The locking-testsuite uses <debug_locks_silent> to get a
* 'silent failure': nothing is printed to the console when
* a locking bug is detected.
*/
int debug_locks_silent;"
Jarek P.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent
2006-12-19 8:43 ` Jarek Poplawski
2006-12-19 8:51 ` Matthew Wilcox
@ 2006-12-19 9:31 ` Ingo Molnar
2006-12-19 9:40 ` Matthew Wilcox
1 sibling, 1 reply; 8+ messages in thread
From: Ingo Molnar @ 2006-12-19 9:31 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: linux-kernel, Andrew Morton, Matthew Wilcox
* Jarek Poplawski <jarkao2@o2.pl> wrote:
> > if (unlikely(c)) { \
> > - if (debug_locks_silent || debug_locks_off()) \
> > + if (!debug_locks_silent && debug_locks_off()) \
btw., updated patch is below - the right order is to first do
debug_locks_off(), then debug_locks_silent.
[ btw., could you fix your mailer so that a reply to your mails doesnt
put all participants into the 'To:' line? You can do it via adding
"unset followup_to" to your $HOME/.muttrc file. ]
------------->
Subject: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent
From: Ingo Molnar <mingo@elte.hu>
Matthew Wilcox noticed that the debug_locks_silent use should be
inverted in DEBUG_LOCKS_WARN_ON(). This bug was causing spurious
stacktraces and incorrect failures in the locking self-test on the
parisc kernel.
Bug-found-by: Matthew Wilcox <matthew@wil.cx>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
include/linux/debug_locks.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux/include/linux/debug_locks.h
===================================================================
--- linux.orig/include/linux/debug_locks.h
+++ linux/include/linux/debug_locks.h
@@ -24,7 +24,7 @@ extern int debug_locks_off(void);
int __ret = 0; \
\
if (unlikely(c)) { \
- if (debug_locks_silent || debug_locks_off()) \
+ if (debug_locks_off() && !debug_locks_silent) \
WARN_ON(1); \
__ret = 1; \
} \
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent
2006-12-19 9:31 ` Ingo Molnar
@ 2006-12-19 9:40 ` Matthew Wilcox
2006-12-19 9:44 ` Ingo Molnar
0 siblings, 1 reply; 8+ messages in thread
From: Matthew Wilcox @ 2006-12-19 9:40 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Jarek Poplawski, linux-kernel, Andrew Morton
On Tue, Dec 19, 2006 at 10:31:35AM +0100, Ingo Molnar wrote:
>
> * Jarek Poplawski <jarkao2@o2.pl> wrote:
>
> > > if (unlikely(c)) { \
> > > - if (debug_locks_silent || debug_locks_off()) \
> > > + if (!debug_locks_silent && debug_locks_off()) \
>
> btw., updated patch is below - the right order is to first do
> debug_locks_off(), then debug_locks_silent.
Then how does one re-enable lock debugging after running the locking
testsuite?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent
2006-12-19 9:40 ` Matthew Wilcox
@ 2006-12-19 9:44 ` Ingo Molnar
0 siblings, 0 replies; 8+ messages in thread
From: Ingo Molnar @ 2006-12-19 9:44 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Jarek Poplawski, linux-kernel, Andrew Morton
* Matthew Wilcox <matthew@wil.cx> wrote:
> On Tue, Dec 19, 2006 at 10:31:35AM +0100, Ingo Molnar wrote:
> >
> > * Jarek Poplawski <jarkao2@o2.pl> wrote:
> >
> > > > if (unlikely(c)) { \
> > > > - if (debug_locks_silent || debug_locks_off()) \
> > > > + if (!debug_locks_silent && debug_locks_off()) \
> >
> > btw., updated patch is below - the right order is to first do
> > debug_locks_off(), then debug_locks_silent.
>
> Then how does one re-enable lock debugging after running the locking
> testsuite?
see the lib/locking-selftest.c:locking_selftest() function, if all
testcases pass then it re-enables lock debugging. If a testcase turns
off lock debugging because it triggers a bug (as many of them
legitimately do), then reset_locks()->lockdep_reset() will set
debug_locks back to 1.
Ingo
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2006-12-19 9:46 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-12-16 6:26 Doubled stack dumps during locking testsuite Matthew Wilcox
2006-12-16 8:04 ` [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent Ingo Molnar
2006-12-19 8:43 ` Jarek Poplawski
2006-12-19 8:51 ` Matthew Wilcox
2006-12-19 9:05 ` Jarek Poplawski
2006-12-19 9:31 ` Ingo Molnar
2006-12-19 9:40 ` Matthew Wilcox
2006-12-19 9:44 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox