Doubled stack dumps during locking testsuite

Doubled stack dumps during locking testsuite

[Matthew Wilcox]

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);

[patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent

[Ingo Molnar]

* 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;						\
 	}								\

Re: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent

[Jarek Poplawski]

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.

Re: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent

[Matthew Wilcox]

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.

Re: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent

[Jarek Poplawski]

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.

Re: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent

[Ingo Molnar]

* 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;						\
 	}								\

Re: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent

[Matthew Wilcox]

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?

Re: [patch] lock debugging: fix DEBUG_LOCKS_WARN_ON() & debug_locks_silent

[Ingo Molnar]

* 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