Re: spinlock assertion macros

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

[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 <><

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

[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

[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

[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

[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 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: 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

[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

[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]

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

[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

[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 <><