Skip to contentSkip to conversation

lock assertion macros for 2.5.28

PATCH In review Jesse Barnes 6 participants 11 messages Jul 25, 2002 – Aug 2, 2002

Subsystems (2)
VFS Locking primitives
lore
Share & tools
lore

Jesse Barnes

author
#0 lore raw

PATCH 1/1 lock assertion macros for 2.5.28

Here's the lastest version of the lockassert patch. It includes:
o MUST_HOLD for all architectures
o MUST_HOLD_RW for architectures implementing rwlock_is_locked (only ia64 at the moment, as part of this patch)
o MUST_HOLD_RWSEM for arcitectures that use rwsem-spinlock.h
o MUST_HOLD_SEM for ia64
o a call to MUST_HOLD(&inode_lock) in inode.c:__iget().

I'd be happy to take patches that implement the above routines for other architectures and/or patches that sprinkle the macros where they're needed.

Thanks,
Jesse

fs/inode.c +2 -0
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.28/fs/inode.c linux-2.5.28-lockassert/fs/inode.c
@@ -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;
include/asm-ia64/semaphore.h +7 -0
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.28/include/asm-ia64/semaphore.h linux-2.5.28-lockassert/include/asm-ia64/semaphore.h
@@ -6,6 +6,7 @@
  * Copyright (C) 1998-2000 David Mosberger-Tang <[email protected]>
  */
 
+#include <linux/config.h>
 #include <linux/wait.h>
 #include <linux/rwsem.h>
 

@@ -24,6 +25,12 @@
 # define __SEM_DEBUG_INIT(name)		, (long) &(name).__magic
 #else
 # define __SEM_DEBUG_INIT(name)
+#endif
+
+#ifdef CONFIG_DEBUG_SPINLOCK
+# define MUST_HOLD_SEM(sem)		do { BUG_ON(atomic_read(sem->count)); } while(0);
+#else
+# define MUST_HOLD_SEM(sem)		do { } while(0)
 #endif
 
 #define __SEMAPHORE_INITIALIZER(name,count)					\
include/asm-ia64/spinlock.h +1 -0
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.28/include/asm-ia64/spinlock.h linux-2.5.28-lockassert/include/asm-ia64/spinlock.h
@@ -109,6 +109,7 @@
 #define RW_LOCK_UNLOCKED (rwlock_t) { 0, 0 }
 
 #define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+#define rwlock_is_locked(x) ((x)->read_counter != 0 || (x)->write_lock != 0)
 
 #define _raw_read_lock(rw)							\
 do {										\
include/linux/rwsem-spinlock.h +6 -0
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.28/include/linux/rwsem-spinlock.h linux-2.5.28-lockassert/include/linux/rwsem-spinlock.h
@@ -46,6 +46,12 @@
 #define __RWSEM_DEBUG_INIT	/* */
 #endif
 
+#ifdef CONFIG_DEBUG_SPINLOCK
+#define MUST_HOLD_RWSEM(sem)		BUG_ON(!sem->activity))
+#else
+#define MUST_HOLD_RWSEM(sem)		do { } while (0)
+#endif
+
 #define __RWSEM_INITIALIZER(name) \
 { 0, SPIN_LOCK_UNLOCKED, LIST_HEAD_INIT((name).wait_list) __RWSEM_DEBUG_INIT }
include/linux/rwsem.h +1 -0
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.28/include/linux/rwsem.h linux-2.5.28-lockassert/include/linux/rwsem.h
@@ -7,6 +7,7 @@
 #ifndef _LINUX_RWSEM_H
 #define _LINUX_RWSEM_H
 
+#include <linux/config.h>
 #include <linux/linkage.h>
 
 #define RWSEM_DEBUG 0
include/linux/spinlock.h +13 -1
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.28/include/linux/spinlock.h linux-2.5.28-lockassert/include/linux/spinlock.h
@@ -117,7 +117,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_HOLD_RW(lock)		BUG_ON(!rwlock_is_locked(lock))
+#else
+#define MUST_HOLD(lock)			do { } while(0)
+#define MUST_HOLD_RW(lock)		do { } while(0)
+#endif /* CONFIG_DEBUG_SPINLOCK && CONFIG_SMP */
 
 #ifdef CONFIG_PREEMPT
#1 lore raw
Quoted 11 lines · Jesse Barnes

Here's the lastest version of the lockassert patch. It includes:
o MUST_HOLD for all architectures
o MUST_HOLD_RW for architectures implementing rwlock_is_locked (only ia64 at the moment, as part of this patch)
o MUST_HOLD_RWSEM for arcitectures that use rwsem-spinlock.h
o MUST_HOLD_SEM for ia64
o a call to MUST_HOLD(&inode_lock) in inode.c:__iget().

I'd be happy to take patches that implement the above routines for other architectures and/or patches that sprinkle the macros where they're needed.

Well one one place? Every single implementation of the request_fn method from the request_queue_t needs to hold some
lock associated with the queue in question.

In fact you will find ASSERT_LOCK macros sparnkled through the scsi code already right now. BTW> ASSERT_HOLDS would sound a bit more familiar to some of us.

This minor issue asside I think that your idea is a good thing.

author
#2 lore raw
Quoted 9 lines · Marcin Dalecki

Well one one place? Every single implementation of the request_fn method from the request_queue_t needs to hold some
lock associated with the queue in question.

In fact you will find ASSERT_LOCK macros sparnkled through the scsi code already right now. BTW> ASSERT_HOLDS would sound a bit more familiar to some of us.

This minor issue asside I think that your idea is a good thing.

Thanks for the pointer. I'll change those assertions over in the next revision.

Jesse

#3 lore raw
Quoted 5 lines · Jesse Barnes
Quoted 4 lines · Marcin Dalecki

Well one one place? Every single implementation of the request_fn method from the request_queue_t needs to hold some
lock associated with the queue in question.

Quoted 8 lines
Quoted 5 lines

In fact you will find ASSERT_LOCK macros sparnkled through the scsi code already right now. BTW> ASSERT_HOLDS would sound a bit more familiar to some of us.

This minor issue asside I think that your idea is a good thing.

Thanks for the pointer. I'll change those assertions over in the next revision.

The scsi version is not the same, it's going to need to be changed to this sensible version.

The original name is better and shorter. I doubt there is anybody who will not know immediately what MUST_HOLD(&lock) means.

Signature · 1 line
Daniel
#4 lore raw
In reply to Jesse Barnes
Quoted 8 lines · Jesse Barnes

Here's the lastest version of the lockassert patch. It includes:
o MUST_HOLD for all architectures
o MUST_HOLD_RW for architectures implementing rwlock_is_locked (only ia64 at the moment, as part of this patch)
o MUST_HOLD_RWSEM for arcitectures that use rwsem-spinlock.h
o MUST_HOLD_SEM for ia64
o a call to MUST_HOLD(&inode_lock) in inode.c:__iget().

Quoted 7 lines

I'd be happy to take patches that implement the above routines for other architectures and/or patches that sprinkle the macros where they're needed.

Thanks,
Jesse

Jesse,

In reiser4 we are looking forward to having a MUST_NOT_HOLD (i.e., spin_is_not_locked) assertion for kernel spinlocks. Do you know if any progress has been made in that direction?

We have implemented a user-level testing framework for our file system and we are already using a spin_is_not_locked() method, but these assertions are disabled when compiled into the kernel.

-josh

author
#5 lore raw
Quoted 3 lines · Joshua MacDonald

In reiser4 we are looking forward to having a MUST_NOT_HOLD (i.e., spin_is_not_locked) assertion for kernel spinlocks. Do you know if any progress has been made in that direction?

Well, I had that in one version of the patch, but people didn't think it would be useful. Maybe you'd like to check out Oliver's comments at http://marc.theaimsgroup.com/?l=linux-kernel&m=102644431806734&w=2
and respond? If there's demand for MUST_NOT_HOLD, I'd be happy to add it since it should be easy. But if you're using it to enforce lock ordering as Oliver suggests, then there are probably more robust solutions.

Thanks,
Jesse

#6 lore raw
Quoted 5 lines · Jesse Barnes
Quoted 3 lines · Joshua MacDonald

In reiser4 we are looking forward to having a MUST_NOT_HOLD (i.e., spin_is_not_locked) assertion for kernel spinlocks. Do you know if any progress has been made in that direction?

Quoted 7 lines

Well, I had that in one version of the patch, but people didn't think it would be useful. Maybe you'd like to check out Oliver's comments at http://marc.theaimsgroup.com/?l=linux-kernel&m=102644431806734&w=2
and respond? If there's demand for MUST_NOT_HOLD, I'd be happy to add it since it should be easy. But if you're using it to enforce lock ordering as Oliver suggests, then there are probably more robust solutions.

Why don't you just generalize the scsi version that already support this?

reinventing the wheel everywhere..

author
#7 lore raw
Quoted 14 lines · Christoph Hellwig
Quoted 5 lines · Jesse Barnes
Quoted 3 lines · Joshua MacDonald

In reiser4 we are looking forward to having a MUST_NOT_HOLD (i.e., spin_is_not_locked) assertion for kernel spinlocks. Do you know if any progress has been made in that direction?

Quoted 7 lines

Well, I had that in one version of the patch, but people didn't think it would be useful. Maybe you'd like to check out Oliver's comments at http://marc.theaimsgroup.com/?l=linux-kernel&m=102644431806734&w=2
and respond? If there's demand for MUST_NOT_HOLD, I'd be happy to add it since it should be easy. But if you're using it to enforce lock ordering as Oliver suggests, then there are probably more robust solutions.

Quoted 3 lines

Why don't you just generalize the scsi version that already support this?

reinventing the wheel everywhere..

Well, I wouldn't go that far. The macros are really simple and implementing a MUST_NOT_HOLD should be easy too. It could also be done in a much more useful way than how ASSERT_LOCK works, by tracking where the locks where acquired for example.

Did you check out the thread above? Having ASSERT_LOCK(&lock, 0) doesn't seem that useful by itself. A lot of the scsi code does things like:

  ASSERT_LOCK(&lock, 0);
  ...
  spin_lock(&lock);

What does that buy you? The suggestions for tracking where the lock was acquired (in the thread above) seem much more useful.

Thanks,
Jesse

#8 lore raw
In reply to Christoph Hellwig
Quoted 14 lines · Christoph Hellwig
Quoted 5 lines · Jesse Barnes
Quoted 3 lines · Joshua MacDonald

In reiser4 we are looking forward to having a MUST_NOT_HOLD (i.e., spin_is_not_locked) assertion for kernel spinlocks. Do you know if any progress has been made in that direction?

Quoted 7 lines

Well, I had that in one version of the patch, but people didn't think it would be useful. Maybe you'd like to check out Oliver's comments at http://marc.theaimsgroup.com/?l=linux-kernel&m=102644431806734&w=2
and respond? If there's demand for MUST_NOT_HOLD, I'd be happy to add it since it should be easy. But if you're using it to enforce lock ordering as Oliver suggests, then there are probably more robust solutions.

Quoted 3 lines

Why don't you just generalize the scsi version that already support this?

reinventing the wheel everywhere..

The scsi version is stupid. It panics instead of oopses and it takes two parameters.

More like improving a wheel, having observed that it works better if its round.

Signature · 1 line
Daniel
#9 lore raw
In reply to Jesse Barnes
Quoted 7 lines · Jesse Barnes

Well, I had that in one version of the patch, but people didn't think it would be useful. Maybe you'd like to check out Oliver's comments at http://marc.theaimsgroup.com/?l=linux-kernel&m=102644431806734&w=2
and respond? If there's demand for MUST_NOT_HOLD, I'd be happy to add it since it should be easy. But if you're using it to enforce lock ordering as Oliver suggests, then there are probably more robust solutions.

Two other suggestions you could implement are CAN_SLEEP and CANNOT_SLEEP. You can implement them via the preempt_count.

Even if CONFIG_PREEMPT is not set, you will get preempt_count values representing whether or not you are in an interrupt or softirq (and thus atomic and cannot sleep). If CONFIG_PREEMPT is set, you get a counter that represents exactly the atomicity of the code including locks held.

E.g.,

	#define CAN_SLEEP	do { \
		assert(unlikely(!preempt_count())); \
	} while (0)

	#define CANNOT_SLEEP	do { \
		assert(unlikely(preempt_count())); \
	} while (0)

This works great because after the IRQ changes in 2.5.28, preempt_count is a universal "are we atomic" count.

Robert Love

#10 lore raw
In reply to Jesse Barnes
Quoted 5 lines · Jesse Barnes
Quoted 3 lines · Joshua MacDonald

In reiser4 we are looking forward to having a MUST_NOT_HOLD (i.e., spin_is_not_locked) assertion for kernel spinlocks. Do you know if any progress has been made in that direction?

Quoted 8 lines

Well, I had that in one version of the patch, but people didn't think it would be useful. Maybe you'd like to check out Oliver's comments at http://marc.theaimsgroup.com/?l=linux-kernel&m=102644431806734&w=2
and respond? If there's demand for MUST_NOT_HOLD, I'd be happy to add it since it should be easy. But if you're using it to enforce lock ordering as Oliver suggests, then there are probably more robust solutions.

I read Oliver's comments and I do not fully agree. It is true that often the MUST_NOT_HOLD macro is used to assert that you are not about to attempt a recursive lock, which a debugging spinlock implementation would catch as soon as the recursive attempt is made. However, it is difficult to make a case against adding support for this kind of assertion since it has many possible uses.

It may be useful to catch a recursive spinlock attempt several stack frames before it actually happens, or to assert that an unusual calling convention such as "This function is called with the spinlock held and if it returns 0 the spinlock remains held, but if the function returns non-zero the spinlock is released". We have such a function in reiser4.

As for preventing deadlock, it is true that (as Oliver says) "Locking order is larger than functions and should be documented at the point of declaration of the locks." We have a mechanism in reiser4 which is not quite the same as Oliver outlined for making assertions about lock ordering. We maintain per-thread counts of each spinlock class and use those counts in a locking predicate that is applied before a lock of each class is taken.

So I agree that recursive locking should be checked as part of the debugging spin_lock() routine and that deadlock detection requires more general work, but the MUST_NOT_HOLD assertion is still useful in some contexts.

-josh