Re: spinlock assertion macros

Re: spinlock assertion macros

[pmenage]

In article <0C01A29FBAE24448A792F5C68F5EA47D2B0FDD@nasdaq.ms.ensim.com>,
 you write:
>
>MUST_NOT_HOLD is already in Jesse's patch he posted earlier today,
>though I imagine it would be used rarely if at all.
>

The spin_assert_unlocked() macro in Jesse's patch doesn't cope with
the fact that someone else might quite legitimately have the spinlock
locked. You'd need debugging spinlocks that track the owner of the
spinlock, and then check in MUST_NOT_HOLD() you'd check that
lock->owner != current. You'd also have to have some special
non-checking lock/unlock macros to handle situations where locks are
taken in non-process context or released by someone other than the
original locker (does the migration code still do that?).

Paul

Re: spinlock assertion macros

[Jesse Barnes]

On Thu, Jul 11, 2002 at 06:36:26PM -0700, pmenage@ensim.com wrote:
> The spin_assert_unlocked() macro in Jesse's patch doesn't cope with
> the fact that someone else might quite legitimately have the spinlock
> locked. You'd need debugging spinlocks that track the owner of the
> spinlock, and then check in MUST_NOT_HOLD() you'd check that
> lock->owner != current. You'd also have to have some special
> non-checking lock/unlock macros to handle situations where locks are
> taken in non-process context or released by someone other than the
> original locker (does the migration code still do that?).

You're right about that, it would be much more useful to add a
spin_assert_unlocked_all() or MUST_NOT_HOLD_ANY() macro, as Arnd
suggested.  I'll take the suggestions I've received and try to put
together a more complete patch early next week.  It'll include lock
checks for rwlocks, semaphores, and rwsems as well as the global
no-locks-held macro.  And as an added bonus, I'll even try to test it
:).

Thanks,
Jesse

Re: BKL removal

[Rick Lindsley]

I wrote:
           v
    > With a narrowly defined and used lock, it is much less difficult to
           ^
    
Larry responded:
    If you were talking about replacing a big lock with one lock, you
    might have a point.  But you aren't.  You can't be, because by
    definition if you take out the big lock you have to put in lots
    of little locks.  And then you get discover all the problems that
    the BKL was hiding that you just exposed by removing it.

And this is bad ... how? A bad locking mechanism, or incorrect usage of
a good one, is exposed. Should it not be?
    
    If you think that managing those is easier than managing the BKL,
    you don't understand the first thing about threading.

So threading is difficult to manage or even impossible without a global
lock for everyone to use? True -- I suppose that would force people to
think about what they are locking and which lock is appropriate to avoid
unnecessary contention.  It would require that the new locks' scopes and
assumptions be documented instead of handed down verbally from 
teacher to student.  It would have the side effect of making it easier
for a newcomer to come up to speed on a particular section of code, thus
allowing a greater number of people to understand the code and offer fixes
or enhancements.
    
    I think the kernel crowd is starting to sense how complex things
    are getting and are pushing back a bit.  Don't fight them, this
    isn't IRIX/Dynix/PTX/AIX/Solaris.

In some ways, that's a shame.  All of those have valuable parts to them.

    It's Linux and part of the
    appeal, part of the reason you are here, is that it is (was) simple.

Feel free to return to version 7 anytime. Life was simpler with single
cpus and only physical memory.  It is true that more complex hardware
has demanded more complex software.

    All you are doing is suggesting that it should be more complex.
    I don't agree at all.

I respect your right to disagree, but I contest the assertion that
"all" we are doing is making it more complex.  In general, finer
grained locking is a good thing, as it isolates contention (both
present AND future) in areas that truly are contending for the same
resource.  As with all things, it is possible to overdo it.  While
certainly true on SMP machines, this is especially underscored with
NUMA machines.  Sometimes the cost of allowing other processors to have
access to a lock or set of data (and the attendant flushing and
reloading of caches) can actually make it more efficient to hold the
lock longer than is necessary for the simple operation you are
attempting.  This is the exception, however, not the rule, and is not a
reason to abandon finer-grained locking.

    > So can you define for me under what conditions the BKL is appropriate
    > to use?  
    
    Can you tell me for sure that there are no races introduced by your
    proposed change?

In many cases, no more than you can tell me, "for sure", what the
weather will be tomorrow.  I can tell you, after inspection, that I
*believe* there are no races and you would learn, after enough times,
that I'm right more often than not.  You'd learn that my word (and the
implied testing behind it) is sufficient. I don't expect anybody to
grant that right off. And even then, the world will find I'm still
wrong sometimes. This truly is the way Linux is supported now.

(And you can replace "I" with, I think, about any Linux luminary.)

The reason I asked the question is to point out that nobody CAN answer
it today.  That's bad.  Every subsystem should have either a maintainer
who can answer these sorts of questions, or clear documentation that
answers it for us. The BKL has (unfortunately) evolved beyond that.

This is exactly the supportability issue that reduction of the BKL will
address. Replacing an instance of it with a well-defined,
well-documented lock should make the new lock's use obvious to
newcomers, and the BKL's remaining uses marginally easier to
understand.

    Can you tell me the list of locks and what they cover in the last
    multi threaded OS you worked in?  I thought not.  Nobody could.

Off the top of my head? no. I'm not a walking encyclopedia.  From
comments and supporting documentation and supporting staff?  about
80%.  The 20% that could not be clearly identified tended to provide a
disproportionate number of bugs, and I think most people agreed they
were undesirable from a support standpoint.

Your argument seems to be, "things are very complicated right now but
they often work, so let's not change anything."  I would argue that
unless these things ARE understood, we are destined to spend a lot of
time uncovering bugs in the future -- so let's improve it now.

Rick

Re: BKL removal

[Daniel Phillips]

On Wednesday 10 July 2002 23:28, Rick Lindsley wrote:
> So threading is difficult to manage or even impossible without a global
> lock for everyone to use? True -- I suppose that would force people to
> think about what they are locking and which lock is appropriate to avoid
> unnecessary contention.  It would require that the new locks' scopes and
> assumptions be documented instead of handed down verbally from 
> teacher to student.

Hear hear!  Well, Al at least has made a pretty good attempt to do that
for VFS locks.  The rest of the kernel is pretty much a disaster.  If
we're lucky, we find the odd comment here and there: 'must be called
holding such-and-such lock', and on a good day the comment is even
correct.  Which reminds me of a janitorial idea I discussed briefly with
Acme, which is to replace all those above-the-function lock coverage
comments with assert-like thingies:

   spin_assert(&pagemap_lru_lock);

And everbody knows what that does: when compiled with no spinlock
debugging it does nothing, but with spinlock debugging enabled, it oopses
unless pagemap_lru_lock is held at that point in the code.  The practical
effect of this is that lots of 3 line comments get replaced with a
one line assert that actually does something useful.  That is, besides
documenting the lock coverage, this thing will actually check to see if
you're telling the truth, if you ask it to.

Oh, and they will stay up to date much better than the comments do,
because nobody needs to to be an ueber-hacker to turn on the option and
post any resulting oopses to lkml.

> It would have the side effect of making it easier
> for a newcomer to come up to speed on a particular section of code, thus
> allowing a greater number of people to understand the code and offer fixes
> or enhancements.

Yup, and double-yup.

-- 
Daniel

Re: spinlock assertion macros

[Jesse Barnes]

On Thu, Jul 11, 2002 at 12:24:06AM +0200, Daniel Phillips wrote:
> Acme, which is to replace all those above-the-function lock coverage
> comments with assert-like thingies:
> 
>    spin_assert(&pagemap_lru_lock);
> 
> And everbody knows what that does: when compiled with no spinlock
> debugging it does nothing, but with spinlock debugging enabled, it oopses
> unless pagemap_lru_lock is held at that point in the code.  The practical
> effect of this is that lots of 3 line comments get replaced with a
> one line assert that actually does something useful.  That is, besides
> documenting the lock coverage, this thing will actually check to see if
> you're telling the truth, if you ask it to.
> 
> Oh, and they will stay up to date much better than the comments do,
> because nobody needs to to be an ueber-hacker to turn on the option and
> post any resulting oopses to lkml.

Sounds like a great idea to me.  Were you thinking of something along
the lines of what I have below or perhaps something more
sophisticated?  I suppose it would be helpful to have the name of the
lock in addition to the file and line number...

Jesse


diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/fs/inode.c linux-2.5.25-spinassert/fs/inode.c
--- linux-2.5.25/fs/inode.c	Fri Jul  5 16:42:38 2002
+++ linux-2.5.25-spinassert/fs/inode.c	Wed Jul 10 16:30:18 2002
@@ -183,6 +183,8 @@
  */
 void __iget(struct inode * inode)
 {
+	spin_assert_locked(&inode_lock);
+
 	if (atomic_read(&inode->i_count)) {
 		atomic_inc(&inode->i_count);
 		return;
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/include/linux/spinlock.h linux-2.5.25-spinassert/include/linux/spinlock.h
--- linux-2.5.25/include/linux/spinlock.h	Fri Jul  5 16:42:24 2002
+++ linux-2.5.25-spinassert/include/linux/spinlock.h	Wed Jul 10 16:20:06 2002
@@ -118,6 +118,18 @@
 
 #endif /* !SMP */
 
+/*
+ * Simple lock assertions for debugging and documenting where locks need
+ * to be locked/unlocked.
+ */
+#ifdef CONFIG_DEBUG_SPINLOCK
+#define spin_assert_locked(lock)		if (!spin_is_locked(lock)) { printk("lock assertion failure: lock at %s:%d should be locked!\n", __FILE__, __LINE__); }
+#define spin_assert_unlocked(lock)		if (spin_is_locked(lock)) { printk("lock assertion failure: lock at %s:%d should be unlocked!\n", __FILE__, __LINE__); }
+#else
+#define spin_assert_locked(lock)		do { } while(0)
+#define spin_assert_unlocked(lock)		do { } while(0)
+#endif /* CONFIG_DEBUG_SPINLOCK */
+
 #ifdef CONFIG_PREEMPT
 
 asmlinkage void preempt_schedule(void);

Re: spinlock assertion macros

[Andreas Dilger]

On Jul 10, 2002  16:36 -0700, Jesse Barnes wrote:
> Sounds like a great idea to me.  Were you thinking of something along
> the lines of what I have below or perhaps something more
> sophisticated?  I suppose it would be helpful to have the name of the
> lock in addition to the file and line number...
> 
> Jesse
> 
> 
> diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/fs/inode.c linux-2.5.25-spinassert/fs/inode.c
> --- linux-2.5.25/fs/inode.c	Fri Jul  5 16:42:38 2002
> +++ linux-2.5.25-spinassert/fs/inode.c	Wed Jul 10 16:30:18 2002
> @@ -183,6 +183,8 @@
>   */
>  void __iget(struct inode * inode)
>  {
> +	spin_assert_locked(&inode_lock);
> +
>  	if (atomic_read(&inode->i_count)) {
>  		atomic_inc(&inode->i_count);
>  		return;
> diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/include/linux/spinlock.h linux-2.5.25-spinassert/include/linux/spinlock.h
> --- linux-2.5.25/include/linux/spinlock.h	Fri Jul  5 16:42:24 2002
> +++ linux-2.5.25-spinassert/include/linux/spinlock.h	Wed Jul 10 16:20:06 2002
> @@ -118,6 +118,18 @@
>  
>  #endif /* !SMP */
>  
> +/*
> + * Simple lock assertions for debugging and documenting where locks need
> + * to be locked/unlocked.
> + */
> +#ifdef CONFIG_DEBUG_SPINLOCK
> +#define spin_assert_locked(lock)	if (!spin_is_locked(lock)) { printk("lock assertion failure: lock at %s:%d should be locked!\n", __FILE__, __LINE__); }

You can use CPP to add in the lock name like:

#define spin_assert_locked(lock)	if (!spin_is_locked(lock))	\
	printk("lock assertion error: %s:%d: " #lock			\
	       " should be locked!\n", __FILE__, __LINE__)

#define spin_assert_unlocked(lock)	if (!spin_is_locked(lock))	\
	printk("lock assertion error: %s:%d: " #lock			\
	       " should be unlocked!\n", __FILE__, __LINE__)

Cheers, Andreas
--
Andreas Dilger
http://www-mddsp.enel.ucalgary.ca/People/adilger/
http://sourceforge.net/projects/ext2resize/

Re: spinlock assertion macros

[Jesse Barnes]

On Wed, Jul 10, 2002 at 06:54:24PM -0600, Andreas Dilger wrote:
> You can use CPP to add in the lock name like:
> 
> #define spin_assert_locked(lock)	if (!spin_is_locked(lock))	\
> 	printk("lock assertion error: %s:%d: " #lock			\
> 	       " should be locked!\n", __FILE__, __LINE__)
> 
> #define spin_assert_unlocked(lock)	if (!spin_is_locked(lock))	\
> 	printk("lock assertion error: %s:%d: " #lock			\
> 	       " should be unlocked!\n", __FILE__, __LINE__)

Oh yeah, I should have done that the first time.  How does this look?

Thanks,
Jesse


diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/fs/inode.c linux-2.5.25-spinassert/fs/inode.c
--- linux-2.5.25/fs/inode.c	Fri Jul  5 16:42:38 2002
+++ linux-2.5.25-spinassert/fs/inode.c	Wed Jul 10 16:30:18 2002
@@ -183,6 +183,8 @@
  */
 void __iget(struct inode * inode)
 {
+	spin_assert_locked(&inode_lock);
+
 	if (atomic_read(&inode->i_count)) {
 		atomic_inc(&inode->i_count);
 		return;
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/include/linux/spinlock.h linux-2.5.25-spinassert/include/linux/spinlock.h
--- linux-2.5.25/include/linux/spinlock.h	Fri Jul  5 16:42:24 2002
+++ linux-2.5.25-spinassert/include/linux/spinlock.h	Wed Jul 10 18:05:13 2002
@@ -118,6 +118,22 @@
 
 #endif /* !SMP */
 
+/*
+ * Simple lock assertions for debugging and documenting where locks need
+ * to be locked/unlocked.
+ */
+#if defined(CONFIG_DEBUG_SPINLOCK) && defined(SMP)
+#define spin_assert_locked(lock)		if (!spin_is_locked(lock)) { \
+	printk("lock assertion failure: %s:%d lock " #lock \
+		"should be locked!\n", __FILE__, __LINE__); }
+#define spin_assert_unlocked(lock)		if (spin_is_locked(lock)) { \
+	printk("lock assertion failure: %s:%d lock " #lock \
+		"should be unlocked!\n", __FILE__, __LINE__); }
+#else
+#define spin_assert_locked(lock)		do { } while(0)
+#define spin_assert_unlocked(lock)		do { } while(0)
+#endif /* CONFIG_DEBUG_SPINLOCK && SMP */
+
 #ifdef CONFIG_PREEMPT
 
 asmlinkage void preempt_schedule(void);

Re: spinlock assertion macros

[Daniel Phillips]

On Thursday 11 July 2002 01:36, Jesse Barnes wrote:
> On Thu, Jul 11, 2002 at 12:24:06AM +0200, Daniel Phillips wrote:
> > Acme, which is to replace all those above-the-function lock coverage
> > comments with assert-like thingies:
> > 
> >    spin_assert(&pagemap_lru_lock);
> > 
> > And everbody knows what that does: when compiled with no spinlock
> > debugging it does nothing, but with spinlock debugging enabled, it oopses
> > unless pagemap_lru_lock is held at that point in the code.  The practical
> > effect of this is that lots of 3 line comments get replaced with a
> > one line assert that actually does something useful.  That is, besides
> > documenting the lock coverage, this thing will actually check to see if
> > you're telling the truth, if you ask it to.
> > 
> > Oh, and they will stay up to date much better than the comments do,
> > because nobody needs to to be an ueber-hacker to turn on the option and
> > post any resulting oopses to lkml.
> 
> Sounds like a great idea to me.  Were you thinking of something along
> the lines of what I have below or perhaps something more
> sophisticated?  I suppose it would be helpful to have the name of the
> lock in addition to the file and line number...

I was thinking of something as simple as:

   #define spin_assert_locked(LOCK) BUG_ON(!spin_is_locked(LOCK))

but in truth I'd be happy regardless of the internal implementation.  A note
on names: Linus likes to shout the names of his BUG macros.  I've never been
one for shouting, but it's not my kernel, and anyway, I'm happy he now likes
asserts.  I bet he'd like it more spelled like this though:

   MUST_HOLD(&lock);

And, dare I say it, what I'd *really* like to happen when the thing triggers
is to get dropped into kdb.  Ah well, perhaps in a parallel universe...

When one of these things triggers I do think you want everything to come to
a screeching halt, since, to misquote Matrix, "you're already dead", and you
don't want any one-per-year warnings to slip off into the gloomy depths of
some forgotten log file.

-- 
Daniel

Re: spinlock assertion macros

[george anzinger]

Daniel Phillips wrote:
> 
> On Thursday 11 July 2002 01:36, Jesse Barnes wrote:
> > On Thu, Jul 11, 2002 at 12:24:06AM +0200, Daniel Phillips wrote:
> > > Acme, which is to replace all those above-the-function lock coverage
> > > comments with assert-like thingies:
> > >
> > >    spin_assert(&pagemap_lru_lock);
> > >
> > > And everbody knows what that does: when compiled with no spinlock
> > > debugging it does nothing, but with spinlock debugging enabled, it oopses
> > > unless pagemap_lru_lock is held at that point in the code.  The practical
> > > effect of this is that lots of 3 line comments get replaced with a
> > > one line assert that actually does something useful.  That is, besides
> > > documenting the lock coverage, this thing will actually check to see if
> > > you're telling the truth, if you ask it to.
> > >
> > > Oh, and they will stay up to date much better than the comments do,
> > > because nobody needs to to be an ueber-hacker to turn on the option and
> > > post any resulting oopses to lkml.
> >
> > Sounds like a great idea to me.  Were you thinking of something along
> > the lines of what I have below or perhaps something more
> > sophisticated?  I suppose it would be helpful to have the name of the
> > lock in addition to the file and line number...
> 
> I was thinking of something as simple as:
> 
>    #define spin_assert_locked(LOCK) BUG_ON(!spin_is_locked(LOCK))
> 
> but in truth I'd be happy regardless of the internal implementation.  A note
> on names: Linus likes to shout the names of his BUG macros.  I've never been
> one for shouting, but it's not my kernel, and anyway, I'm happy he now likes
> asserts.  I bet he'd like it more spelled like this though:
> 
>    MUST_HOLD(&lock);
> 
> And, dare I say it, what I'd *really* like to happen when the thing triggers
> is to get dropped into kdb.  Ah well, perhaps in a parallel universe...

I should hope that, when BUG executes the unimplemented
instruction, it does go directly to kdb.  It certainly does
with my kgdb, as do all Oops, NULL dereferences, etc., etc.
> 
> When one of these things triggers I do think you want everything to come to
> a screeching halt, since, to misquote Matrix, "you're already dead", and you
> don't want any one-per-year warnings to slip off into the gloomy depths of
> some forgotten log file.
> 
> --
> Daniel
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

Re: spinlock assertion macros

[Oliver Xymoron]

On Thu, 11 Jul 2002, Daniel Phillips wrote:

> I was thinking of something as simple as:
>
>    #define spin_assert_locked(LOCK) BUG_ON(!spin_is_locked(LOCK))
>
> but in truth I'd be happy regardless of the internal implementation.  A note
> on names: Linus likes to shout the names of his BUG macros.  I've never been
> one for shouting, but it's not my kernel, and anyway, I'm happy he now likes
> asserts.  I bet he'd like it more spelled like this though:
>
>    MUST_HOLD(&lock);

I prefer that form too.

> And, dare I say it, what I'd *really* like to happen when the thing triggers
> is to get dropped into kdb.  Ah well, perhaps in a parallel universe...

It ought to.

As long as we're talking about spinlock debugging, I've found it extremely
useful to add an entry to the spinlock to record where the spinlock was
taken.

-- 
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.."

Re: spinlock assertion macros

[Sandy Harris]

Oliver Xymoron wrote:
> 
> On Thu, 11 Jul 2002, Daniel Phillips wrote:
> 
> > I was thinking of something as simple as:
> >
> >    #define spin_assert_locked(LOCK) BUG_ON(!spin_is_locked(LOCK))
> >
> > but in truth I'd be happy regardless of the internal implementation.  A note
> > on names: Linus likes to shout the names of his BUG macros.  I've never been
> > one for shouting, but it's not my kernel, and anyway, I'm happy he now likes
> > asserts.  I bet he'd like it more spelled like this though:
> >
> >    MUST_HOLD(&lock);
> 
> I prefer that form too.

Is it worth adding MUST_NOT_HOLD(&lock) in an attempt to catch potential
deadlocks?

Say that if two or more of locks A, B and C are to be taken, then
they must be taken in that order. You might then have code like:

	MUST_NOT_HOLD(&lock_B) ;
	MUST_NOT_HOLD(&lock_C) ;
	spinlock(&lock_A) ;

I think you need a separate asertion for this !MUST_NOT_HOLD(&lock)
has different semantics.

Re: spinlock assertion macros

[Daniel Phillips]

On Friday 12 July 2002 01:52, Sandy Harris wrote:
> Oliver Xymoron wrote:
> > 
> > On Thu, 11 Jul 2002, Daniel Phillips wrote:
> > 
> > > I was thinking of something as simple as:
> > >
> > >    #define spin_assert_locked(LOCK) BUG_ON(!spin_is_locked(LOCK))
> > >
> > > but in truth I'd be happy regardless of the internal implementation.  A note
> > > on names: Linus likes to shout the names of his BUG macros.  I've never been
> > > one for shouting, but it's not my kernel, and anyway, I'm happy he now likes
> > > asserts.  I bet he'd like it more spelled like this though:
> > >
> > >    MUST_HOLD(&lock);
> > 
> > I prefer that form too.
> 
> Is it worth adding MUST_NOT_HOLD(&lock) in an attempt to catch potential
> deadlocks?
> 
> Say that if two or more of locks A, B and C are to be taken, then
> they must be taken in that order. You might then have code like:
> 
> 	MUST_NOT_HOLD(&lock_B) ;
> 	MUST_NOT_HOLD(&lock_C) ;
> 	spinlock(&lock_A) ;
> 
> I think you need a separate asertion for this !MUST_NOT_HOLD(&lock)
> has different semantics.

MUST_NOT_HOLD is already in Jesse's patch he posted earlier today,
though I imagine it would be used rarely if at all.

!MUST_NOT_HOLD(&lock) is an error, MUST_NOT_HOLD is a statement not a
function.

-- 
Daniel

Re: spinlock assertion macros

[Oliver Xymoron]

On Thu, 11 Jul 2002, Sandy Harris wrote:

> Oliver Xymoron wrote:
> >
> > On Thu, 11 Jul 2002, Daniel Phillips wrote:
> >
> > > I was thinking of something as simple as:
> > >
> > >    #define spin_assert_locked(LOCK) BUG_ON(!spin_is_locked(LOCK))
> > >
> > > but in truth I'd be happy regardless of the internal implementation.  A note
> > > on names: Linus likes to shout the names of his BUG macros.  I've never been
> > > one for shouting, but it's not my kernel, and anyway, I'm happy he now likes
> > > asserts.  I bet he'd like it more spelled like this though:
> > >
> > >    MUST_HOLD(&lock);
> >
> > I prefer that form too.
>
> Is it worth adding MUST_NOT_HOLD(&lock) in an attempt to catch potential
> deadlocks?

No, you'd rather do that inside the debugging version of spinlock itself.
I see MUST_HOLD happening inside helper functions that presume a lock is
in effect but don't do any on their own to ensure integrity, while
MUST_NOT_HOLD is a matter of forcing ordering and avoiding deadlocks.
Locking order is larger than functions and should be documented at the
point of declaration of the locks.

You can do lock order checking in the spinlock debugging code something
like this:

struct spinlock
{
	atomic_t val;
#ifdef SPINLOCK_DEBUG
	void *eip;
	struct spinlock *previous;
	struct spinlock *ordering;
};

DECLARE_SPINLOCK(a, 0); /* a is outermost for this group */
DECLARE_SPINLOCK(b, &a); /* b must nest inside a */
DECLARE_SPINLOCK(c, &b);

Each time you take a spinlock, record the eip, stick the address of the
current lock in the task struct, and stick the outer lock in previous.
Then see if the current lock appears anywhere in the previous lock's
ordering chain (it shouldn't). This may be a bit overkill though.

It might make sense to have a MAY_SLEEP assert in a few key places (high
up in the chains of things that rarely sleep or for which the call path to
sleeping isn't obvious).

-- 
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.."

Re: spinlock assertion macros

[Jesse Barnes]

On Thu, Jul 11, 2002 at 07:31:09AM +0200, Daniel Phillips wrote:
> I was thinking of something as simple as:
> 
>    #define spin_assert_locked(LOCK) BUG_ON(!spin_is_locked(LOCK))
> 
> but in truth I'd be happy regardless of the internal implementation.  A note
> on names: Linus likes to shout the names of his BUG macros.  I've never been
> one for shouting, but it's not my kernel, and anyway, I'm happy he now likes
> asserts.  I bet he'd like it more spelled like this though:
> 
>    MUST_HOLD(&lock);

I like lowercase better too, but you're right that Linus likes to
shout...

> And, dare I say it, what I'd *really* like to happen when the thing triggers
> is to get dropped into kdb.  Ah well, perhaps in a parallel universe...

As long as you've got kdb patched in, this _should_ happen on BUG().

How about this?  Are there simple *_is_locked() calls for the other
mutex mechanisms?  If so, I could add those too...

Thanks,
Jesse


diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/fs/inode.c linux-2.5.25-spinassert/fs/inode.c
--- linux-2.5.25/fs/inode.c	Fri Jul  5 16:42:38 2002
+++ linux-2.5.25-spinassert/fs/inode.c	Thu Jul 11 10:59:23 2002
@@ -183,6 +183,8 @@
  */
 void __iget(struct inode * inode)
 {
+	MUST_HOLD(&inode_lock);
+
 	if (atomic_read(&inode->i_count)) {
 		atomic_inc(&inode->i_count);
 		return;
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/include/linux/spinlock.h linux-2.5.25-spinassert/include/linux/spinlock.h
--- linux-2.5.25/include/linux/spinlock.h	Fri Jul  5 16:42:24 2002
+++ linux-2.5.25-spinassert/include/linux/spinlock.h	Thu Jul 11 11:02:17 2002
@@ -116,7 +116,19 @@
 #define _raw_write_lock(lock)	(void)(lock) /* Not "unused variable". */
 #define _raw_write_unlock(lock)	do { } while(0)
 
-#endif /* !SMP */
+#endif /* !CONFIG_SMP */
+
+/*
+ * Simple lock assertions for debugging and documenting where locks need
+ * to be locked/unlocked.
+ */
+#if defined(CONFIG_DEBUG_SPINLOCK) && defined(CONFIG_SMP)
+#define MUST_HOLD(lock)		BUG_ON(!spin_is_locked(lock))
+#define MUST_NOT_HOLD(lock)	BUG_ON(spin_is_locked(lock))
+#else
+#define MUST_HOLD(lock)		do { } while(0)
+#define MUST_NOT_HOLD(lock)	do { } while(0)
+#endif /* CONFIG_DEBUG_SPINLOCK && CONFIG_SMP */
 
 #ifdef CONFIG_PREEMPT
 

Re: spinlock assertion macros

[Daniel Phillips]

On Thursday 11 July 2002 20:03, Jesse Barnes wrote:
> How about this?

It looks good, the obvious thing we don't get is what the actual lock
count is, and actually, we don't care because we know what it is in
this case.

> Are there simple *_is_locked() calls for the other
> mutex mechanisms?

I didn't look, but my guess is no and you're in for some reverse
engineering, or you could just ask Ben.

> If so, I could add those too...

Definitely needed.  Note there are both counting and mutex forms of
semaphore and they are actually the same primitive - this doesn't mean
the asserts should treat them the same.  That is, it doesn't make
sense to think of the counting form of semaphore in terms of lock
coverage, so actually, we're never interested in semaphore count
values other than 0 and 1,  the simple BUG_ON is all we need.  This
is nice, not only because it reads well, but because it doesn't
generate a lot of code.  This whole feature will be a lot more useful
if code size isn't a problem, in which case I'd tend to run with the
option enabled in the normal course of development.

> Thanks,
> Jesse
> 
> 
> diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/fs/inode.c linux-2.5.25-spinassert/fs/inode.c
> --- linux-2.5.25/fs/inode.c	Fri Jul  5 16:42:38 2002
> +++ linux-2.5.25-spinassert/fs/inode.c	Thu Jul 11 10:59:23 2002
> @@ -183,6 +183,8 @@
>   */
>  void __iget(struct inode * inode)
>  {
> +	MUST_HOLD(&inode_lock);
> +
>  	if (atomic_read(&inode->i_count)) {
>  		atomic_inc(&inode->i_count);
>  		return;
> diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/include/linux/spinlock.h linux-2.5.25-spinassert/include/linux/spinlock.h
> --- linux-2.5.25/include/linux/spinlock.h	Fri Jul  5 16:42:24 2002
> +++ linux-2.5.25-spinassert/include/linux/spinlock.h	Thu Jul 11 11:02:17 2002
> @@ -116,7 +116,19 @@
>  #define _raw_write_lock(lock)	(void)(lock) /* Not "unused variable". */
>  #define _raw_write_unlock(lock)	do { } while(0)
>  
> -#endif /* !SMP */
> +#endif /* !CONFIG_SMP */
> +
> +/*
> + * Simple lock assertions for debugging and documenting where locks need
> + * to be locked/unlocked.
> + */
> +#if defined(CONFIG_DEBUG_SPINLOCK) && defined(CONFIG_SMP)
> +#define MUST_HOLD(lock)		BUG_ON(!spin_is_locked(lock))
> +#define MUST_NOT_HOLD(lock)	BUG_ON(spin_is_locked(lock))
> +#else
> +#define MUST_HOLD(lock)		do { } while(0)
> +#define MUST_NOT_HOLD(lock)	do { } while(0)
> +#endif /* CONFIG_DEBUG_SPINLOCK && CONFIG_SMP */
>  
>  #ifdef CONFIG_PREEMPT
>  
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

-- 
Daniel

Re: spinlock assertion macros

[Dave Jones]

On Thu, Jul 11, 2002 at 09:17:44PM +0200, Daniel Phillips wrote:
 > On Thursday 11 July 2002 20:03, Jesse Barnes wrote:
 > > How about this?
 > 
 > It looks good, the obvious thing we don't get is what the actual lock
 > count is, and actually, we don't care because we know what it is in
 > this case.

Something I've been meaning to hack up for a while is some spinlock
debugging code that adds a FUNCTION_SLEEPS() to (ta-da) functions that
may sleep. This macro then checks whether we're currently holding any
locks, and if so printk's the names of locks held, and where they were taken.

When I came up with the idea[1] I envisioned some linked-lists frobbing,
but in more recent times, we can now check the preempt_count for a
quick-n-dirty implementation (without the additional info of which locks
we hold, lock-taker, etc).

        Dave

[1] Not an original idea, in fact I think Ingo came up with an
    implementation back in `98 or so.

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: spinlock assertion macros

[Daniel Phillips]

On Friday 12 July 2002 14:07, Dave Jones wrote:
> On Thu, Jul 11, 2002 at 09:17:44PM +0200, Daniel Phillips wrote:
>  > On Thursday 11 July 2002 20:03, Jesse Barnes wrote:
>  > > How about this?
>  > 
>  > It looks good, the obvious thing we don't get is what the actual lock
>  > count is, and actually, we don't care because we know what it is in
>  > this case.
> 
> Something I've been meaning to hack up for a while is some spinlock
> debugging code that adds a FUNCTION_SLEEPS() to (ta-da) functions that
> may sleep.

Yesss.  May I suggest simply SLEEPS()?  (Chances are, we know it's a
function.)

> This macro then checks whether we're currently holding any
> locks, and if so printk's the names of locks held, and where they were taken.

And then oopes?

> When I came up with the idea[1] I envisioned some linked-lists frobbing,
> but in more recent times, we can now check the preempt_count for a
> quick-n-dirty implementation (without the additional info of which locks
> we hold, lock-taker, etc).

Spin_lock just has to store the address/location of the lock in a
per-cpu vector, and the assert prints that out when it oopses.  Such
bugs won't live too long under those conditions.

Any idea how one might implement NEVER_SLEEPS()?  Maybe as:

   NEVER_ [code goes here] _SLEEPS

which inc/dec the preeempt count, triggering a BUG in schedule().

-- 
Daniel

Re: spinlock assertion macros

[Arnd Bergmann]

Daniel Phillips wrote:

> Any idea how one might implement NEVER_SLEEPS()?  Maybe as:

Why would you want that? AFAICS there are two kinds of "never
sleeping" functions: 1. those that don't sleep but don't care
about it and 2. those that must not sleep because a lock is
held.

For 1. no point marking it because it might change without
being a bug. You also don't want to mark every function
in the kernel SLEEPS() or NEVER_SLEEPS().

For 2. we already have MUST_HOLD(foo) or similar, which implicitly 
means it can never sleep. The same is true for functions
with spinlocks or preempt_disable around their body.

        Arnd <><

Re: spinlock assertion macros

[Daniel Phillips]

On Friday 12 July 2002 21:24, Arnd Bergmann wrote:
> Daniel Phillips wrote:
> 
> > Any idea how one might implement NEVER_SLEEPS()?  Maybe as:
> 
> Why would you want that? AFAICS there are two kinds of "never
> sleeping" functions: 1. those that don't sleep but don't care
> about it and 2. those that must not sleep because a lock is
> held.
> 
> For 1. no point marking it because it might change without
> being a bug. You also don't want to mark every function
> in the kernel SLEEPS() or NEVER_SLEEPS().
> 
> For 2. we already have MUST_HOLD(foo) or similar, which implicitly 
> means it can never sleep. The same is true for functions
> with spinlocks or preempt_disable around their body.

Thanks for that.

So far, only the MUST_HOLD style of executable locking documentation has 
really survived scrutiny.  It needs some variants: MUST_HOLD_READ, 
MUST_HOLD_WRITE, MUST_HOLD_SEM, MUST_HOLD_READ_SEM and MUST_HOLD_WRITE_SEM, 
or names to that effect.

-- 
Daniel

Re: spinlock assertion macros

[Jesse Barnes]

On Fri, Jul 12, 2002 at 07:42:09PM +0200, Daniel Phillips wrote:
> So far, only the MUST_HOLD style of executable locking documentation has 
> really survived scrutiny.  It needs some variants: MUST_HOLD_READ, 
> MUST_HOLD_WRITE, MUST_HOLD_SEM, MUST_HOLD_READ_SEM and MUST_HOLD_WRITE_SEM, 
> or names to that effect.

I'm not quite sure where to put the semaphore versions of the MUST_*
macros, I guess they'd have to go in each of the arch-specific header
files?  Anyway, I've got spinlock and rwlock versions of them below,
maybe they're useful enough to go in as a start?  I only coded the
ia64 version of rwlock_is_*_locked since it was easy--the i386
versions were a little intimidating...

I thought Oliver's suggestion for tracking the order of spinlock
acquisition was good, hopefully someone will take a stab at it along
with Dave's FUNCTION_SLEEPS() implementation.

Jesse


diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/fs/inode.c linux-2.5.25-spinassert/fs/inode.c
--- linux-2.5.25/fs/inode.c	Fri Jul  5 16:42:38 2002
+++ linux-2.5.25-spinassert/fs/inode.c	Tue Jul 16 19:04:01 2002
@@ -183,6 +183,8 @@
  */
 void __iget(struct inode * inode)
 {
+	MUST_HOLD_SPIN(&inode_lock);
+
 	if (atomic_read(&inode->i_count)) {
 		atomic_inc(&inode->i_count);
 		return;
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/include/asm-ia64/spinlock.h linux-2.5.25-spinassert/include/asm-ia64/spinlock.h
--- linux-2.5.25/include/asm-ia64/spinlock.h	Fri Jul  5 16:42:03 2002
+++ linux-2.5.25-spinassert/include/asm-ia64/spinlock.h	Tue Jul 16 18:31:53 2002
@@ -109,6 +109,8 @@
 #define RW_LOCK_UNLOCKED (rwlock_t) { 0, 0 }
 
 #define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+#define rwlock_is_read_locked(x) ((x)->read_counter != 0 || (x)->write_lock != 0)
+#define rwlock_is_write_locked(x) ((x)->write_lock != 0)
 
 #define _raw_read_lock(rw)							\
 do {										\
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/include/linux/spinlock.h linux-2.5.25-spinassert/include/linux/spinlock.h
--- linux-2.5.25/include/linux/spinlock.h	Fri Jul  5 16:42:24 2002
+++ linux-2.5.25-spinassert/include/linux/spinlock.h	Tue Jul 16 18:58:40 2002
@@ -116,7 +116,21 @@
 #define _raw_write_lock(lock)	(void)(lock) /* Not "unused variable". */
 #define _raw_write_unlock(lock)	do { } while(0)
 
-#endif /* !SMP */
+#endif /* !CONFIG_SMP */
+
+/*
+ * Simple lock assertions for debugging and documenting where locks need
+ * to be locked/unlocked.
+ */
+#if defined(CONFIG_DEBUG_SPINLOCK) && defined(CONFIG_SMP)
+#define MUST_HOLD_SPIN(lock)		BUG_ON(!spin_is_locked(lock))
+#define MUST_HOLD_READ(lock)		BUG_ON(!rwlock_is_read_locked(lock))
+#define MUST_HOLD_WRITE(lock)		BUG_ON(!rwlock_is_write_locked(lock))
+#else
+#define MUST_HOLD_SPIN(lock)		do { } while(0)
+#define MUST_HOLD_READ(lock)		do { } while(0)
+#define MUST_HOLD_WRITE(lock)		do { } while(0)
+#endif /* CONFIG_DEBUG_SPINLOCK && CONFIG_SMP */
 
 #ifdef CONFIG_PREEMPT
 

Re: spinlock assertion macros

[Daniel Phillips]

On Wednesday 17 July 2002 04:22, Jesse Barnes wrote:
> On Fri, Jul 12, 2002 at 07:42:09PM +0200, Daniel Phillips wrote:
> > So far, only the MUST_HOLD style of executable locking documentation has 
> > really survived scrutiny.  It needs some variants: MUST_HOLD_READ, 
> > MUST_HOLD_WRITE, MUST_HOLD_SEM, MUST_HOLD_READ_SEM and MUST_HOLD_WRITE_SEM, 
> > or names to that effect.
> 
> I'm not quite sure where to put the semaphore versions of the MUST_*
> macros, I guess they'd have to go in each of the arch-specific header
> files?

You could create linux/semaphore.h which includes asm/semaphore.h, making
the whole arrangement more similar to spinlocks.  That would be the manly
thing to do, however, manliness not necessarily being the fashion at the
moment, putting them in the arch-specific headers seems like the route of
least resistance.  One day, a prince on a white horse will come along and
clean up all the header files...

> Anyway, I've got spinlock and rwlock versions of them below,
> maybe they're useful enough to go in as a start?  I only coded the
> ia64 version of rwlock_is_*_locked since it was easy--the i386
> versions were a little intimidating...
> 
> I thought Oliver's suggestion for tracking the order of spinlock
> acquisition was good, hopefully someone will take a stab at it along
> with Dave's FUNCTION_SLEEPS() implementation.

Indeed, it's a nice realization of Dave's idea, very clever.

On the minor niggle side, I think "MUST_HOLD" scans better than
"MUST_HOLD_SPIN", since the former is closer to the way we say it when
we're talking amongst ourselves.

-- 
Daniel

Re: spinlock assertion macros

[Arnd Bergmann]

On Wednesday 17 July 2002 04:22, Jesse Barnes wrote:
> files?  Anyway, I've got spinlock and rwlock versions of them below,
> maybe they're useful enough to go in as a start?  I only coded the
> ia64 version of rwlock_is_*_locked since it was easy--the i386
> versions were a little intimidating...
>
> I thought Oliver's suggestion for tracking the order of spinlock
> acquisition was good, hopefully someone will take a stab at it along
> with Dave's FUNCTION_SLEEPS() implementation.

I suppose you can simplify your interface when the code tracking the lock 
holder (i.e. the address of the lock call) is there: 

#define MUST_HOLD(lock)		BUG_ON(!(lock)->holder)

is independent of whether lock is a spinlock or an rw_lock, so you
don't need MUST_HOLD_READ anymore. I'd even go as far as dropping 
MUST_HOLD_WRITE as well, since it helps only in the corner case
where the lock is held but only for reading.

	Arnd <><

Re: spinlock assertion macros

[Oliver Xymoron]

On Fri, 12 Jul 2002, Daniel Phillips wrote:

> On Friday 12 July 2002 14:07, Dave Jones wrote:
> > On Thu, Jul 11, 2002 at 09:17:44PM +0200, Daniel Phillips wrote:
> >  > On Thursday 11 July 2002 20:03, Jesse Barnes wrote:
> >  > > How about this?
> >  >
> >  > It looks good, the obvious thing we don't get is what the actual lock
> >  > count is, and actually, we don't care because we know what it is in
> >  > this case.
> >
> > Something I've been meaning to hack up for a while is some spinlock
> > debugging code that adds a FUNCTION_SLEEPS() to (ta-da) functions that
> > may sleep.
>
> Yesss.  May I suggest simply SLEEPS()?  (Chances are, we know it's a
> function.)
>
> > This macro then checks whether we're currently holding any
> > locks, and if so printk's the names of locks held, and where they were taken.
>
> And then oopes?
>
> > When I came up with the idea[1] I envisioned some linked-lists frobbing,
> > but in more recent times, we can now check the preempt_count for a
> > quick-n-dirty implementation (without the additional info of which locks
> > we hold, lock-taker, etc).
>
> Spin_lock just has to store the address/location of the lock in a
> per-cpu vector, and the assert prints that out when it oopses.  Such
> bugs won't live too long under those conditions.

Store it in the task struct, and store a pointer to the previous (outer
lock) in that lock, then you've got a linked list of locks per task - very
useful. You'll need a helper function - current() is hard to get at from
spinlock.h due to tangled dependencies. As I mentioned before, it can also
be very handy to stash the address of the code that took the lock in the
lock itself.

> Any idea how one might implement NEVER_SLEEPS()?  Maybe as:
>
>    NEVER_ [code goes here] _SLEEPS
>
> which inc/dec the preeempt count, triggering a BUG in schedule().

NEVER_SLEEPS will only trigger on the rare events that blow up anyway,
while the MAY_SLEEP version catches _potential_ problems even when the
fatal sleep doesn't happen.

-- 
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.."

Re: spinlock assertion macros

[Daniel Phillips]

On Friday 12 July 2002 22:41, Oliver Xymoron wrote:
> On Fri, 12 Jul 2002, Daniel Phillips wrote:
> > On Friday 12 July 2002 14:07, Dave Jones wrote:
> > > Something I've been meaning to hack up for a while is some spinlock
> > > debugging code that adds a FUNCTION_SLEEPS() to (ta-da) functions that
> > > may sleep.
> >
> > Yesss.  May I suggest simply SLEEPS()?  (Chances are, we know it's a
> > function.)
> >
> > > This macro then checks whether we're currently holding any
> > > locks, and if so printk's the names of locks held, and where they were taken.
> >
> > And then oopes?
> >
> > > When I came up with the idea[1] I envisioned some linked-lists frobbing,
> > > but in more recent times, we can now check the preempt_count for a
> > > quick-n-dirty implementation (without the additional info of which locks
> > > we hold, lock-taker, etc).
> >
> > Spin_lock just has to store the address/location of the lock in a
> > per-cpu vector, and the assert prints that out when it oopses.  Such
> > bugs won't live too long under those conditions.
> 
> Store it in the task struct, and store a pointer to the previous (outer
> lock) in that lock, then you've got a linked list of locks per task - very
> useful.

Yes, thanks, I was in fact feeling a little guilty about proposing that
non-nested solution, even if it would in practice point you at the
correct culprit most of the time.

So in schedule() we check the task struct field and oops if it's non-null.
In other words, one instance of SLEEPS goes in shedule() and others are
sprinkled around the kernel as executable documentation.

> You'll need a helper function - current() is hard to get at from
> spinlock.h due to tangled dependencies.

What is the problem?  It looks like (struct thread_info *) can be forward
declared, and hence current_thread_info and get_current can be defined
early.  Maybe I didn't sniff at enough architectures.

Anyway, if there are nasty dependencies then they are probably similar
to the ones I unravelled with my early-page patch, which allows struct
page to be declared right at the start of mm.h instead of way later,
below a lot of things that want to reference it.

> As I mentioned before, it can also
> be very handy to stash the address of the code that took the lock in the
> lock itself.

And since the stack is chained through the locks, that just works.
 
> > Any idea how one might implement NEVER_SLEEPS()?  Maybe as:
> >
> >    NEVER_ [code goes here] _SLEEPS
> >
> > which inc/dec the preeempt count, triggering a BUG in schedule().
> 
> NEVER_SLEEPS will only trigger on the rare events that blow up anyway,
> while the MAY_SLEEP version catches _potential_ problems even when the
> fatal sleep doesn't happen.

Yes, that one's already been rejected as largely pointless.

-- 
Daniel

Re: spinlock assertion macros

[Robert Love]

On Fri, 2002-07-12 at 05:07, Dave Jones wrote:

> When I came up with the idea[1] I envisioned some linked-lists frobbing,
> but in more recent times, we can now check the preempt_count for a
> quick-n-dirty implementation (without the additional info of which locks
> we hold, lock-taker, etc).

Neat idea.  I have seen some other good similar ideas: check
preempt_count on schedule(), check preempt_count in usleep/msleep
(Arjan's idea), and check preempt_count in wakeup/context switch/etc.
code...

Note some of these need one or both of: subtracting out
current->lock_depth+1 since we can sleep with the BKL and NAND'ing out
PREEMPT_ACTIVE as that is set before entering schedule off a preemption.

As we move preempt_count to more of a generic "are we atomic" count in
2.5, these become easier and more useful...

	Robert Love

Re: spinlock assertion macros

[Dave Jones]

On Fri, Jul 12, 2002 at 10:49:42AM -0700, Robert Love wrote:

 > > When I came up with the idea[1] I envisioned some linked-lists frobbing,
 > > but in more recent times, we can now check the preempt_count for a
 > > quick-n-dirty implementation (without the additional info of which locks
 > > we hold, lock-taker, etc).
 > 
 > Neat idea.  I have seen some other good similar ideas: check
 > preempt_count on schedule(), check preempt_count in usleep/msleep
 > (Arjan's idea), and check preempt_count in wakeup/context switch/etc.
 > code...

Sounds sensible. I'd like to see more self-checking bits added for
preemption. It may be the only way we ever pin down some of the
outstanding "don't make any sense" bugs.

        Dave

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: spinlock assertion macros

[Arnd Bergmann]

On Thursday 11 July 2002 01:36, Jesse Barnes wrote:

+#define spin_assert_unlocked(lock) if (spin_is_locked(lock)) { printk("lock assertion failure: lock at %s:%d should be unlocked!\n", __FILE__, __LINE__); }

I suppose what would at least be as helpful is to check if _any_ 
lock is held, e.g. when calling a potentially sleeping function.

Something along these lines:

#ifdef CONFIG_DEBUG_SPINLOCK
extern char *volatile last_spinlock[NR_CPUS];

#define spin_assert_unlocked_all() ({ \
	char *lock = last_spinlock[smp_processor_id()]; \
	if (lock) { \
		printk (KERN_CRIT "%s:%d: lock %s is held\n", \
			__func__, __LINE__, lock); \
		BUG(); \
	} \
})

#define spin_lock(lock) ({ \
	last_spinlock[smp_processor_id()] = __stringify(lock) "@" \
		__FILE__ ":" __stringify(__LINE__); \
	__really_spin_lock(lock); \
})
#endif

probably, a per-cpu lock depth should be used to also catch
spin_lock(foo_lock); 
spin_lock(bar_lock); 
spin_unlock(bar_lock);
spin_assert_unlock_all();

	Arnd <><