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
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.28/fs/inode.c linux-2.5.28-lockassert/fs/inode.c
--- linux-2.5.28/fs/inode.c Wed Jul 24 14:03:32 2002
+++ linux-2.5.28-lockassert/fs/inode.c Thu Jul 25 16:17:41 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.28/include/asm-ia64/semaphore.h linux-2.5.28-lockassert/include/asm-ia64/semaphore.h
--- linux-2.5.28/include/asm-ia64/semaphore.h Wed Jul 24 14:03:27 2002
+++ linux-2.5.28-lockassert/include/asm-ia64/semaphore.h Thu Jul 25 16:19:37 2002
@@ -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) \
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
--- linux-2.5.28/include/asm-ia64/spinlock.h Wed Jul 24 14:03:19 2002
+++ linux-2.5.28-lockassert/include/asm-ia64/spinlock.h Thu Jul 25 16:17:41 2002
@@ -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 { \
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
--- linux-2.5.28/include/linux/rwsem-spinlock.h Wed Jul 24 14:03:29 2002
+++ linux-2.5.28-lockassert/include/linux/rwsem-spinlock.h Thu Jul 25 16:17:41 2002
@@ -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 }
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.28/include/linux/rwsem.h linux-2.5.28-lockassert/include/linux/rwsem.h
--- linux-2.5.28/include/linux/rwsem.h Wed Jul 24 14:03:24 2002
+++ linux-2.5.28-lockassert/include/linux/rwsem.h Thu Jul 25 16:17:41 2002
@@ -7,6 +7,7 @@
#ifndef _LINUX_RWSEM_H
#define _LINUX_RWSEM_H
+#include <linux/config.h>
#include <linux/linkage.h>
#define RWSEM_DEBUG 0
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.28/include/linux/spinlock.h linux-2.5.28-lockassert/include/linux/spinlock.h
--- linux-2.5.28/include/linux/spinlock.h Wed Jul 24 14:03:28 2002
+++ linux-2.5.28-lockassert/include/linux/spinlock.h Thu Jul 25 16:17:41 2002
@@ -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
Jesse Barnes wrote:
> 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.
On Thu, Jul 25, 2002 at 04:30:47PM -0700, Jesse Barnes wrote:
> 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
>
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
On Fri, 2002-07-26 at 10:42, Jesse Barnes wrote:
> 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
On Fri, Jul 26, 2002 at 10:42:58AM -0700, Jesse Barnes wrote:
> On Fri, Jul 26, 2002 at 04:09:18PM +0400, Joshua MacDonald wrote:
> > 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.
Why don't you just generalize the scsi version that already support this?
reinventing the wheel everywhere..
On Fri, Jul 26, 2002 at 06:54:16PM +0100, Christoph Hellwig wrote:
> On Fri, Jul 26, 2002 at 10:42:58AM -0700, Jesse Barnes wrote:
> > On Fri, Jul 26, 2002 at 04:09:18PM +0400, Joshua MacDonald wrote:
> > > 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.
>
> 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
On Fri, Jul 26, 2002 at 07:11:28AM +0200, Marcin Dalecki wrote:
> 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
On Fri, Jul 26, 2002 at 04:09:18PM +0400, Joshua MacDonald wrote:
> 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
On Friday 26 July 2002 19:40, Jesse Barnes wrote:
> On Fri, Jul 26, 2002 at 07:11:28AM +0200, Marcin Dalecki wrote:
> > 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.
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.
--
Daniel
On Friday 26 July 2002 19:54, Christoph Hellwig wrote:
> On Fri, Jul 26, 2002 at 10:42:58AM -0700, Jesse Barnes wrote:
> > On Fri, Jul 26, 2002 at 04:09:18PM +0400, Joshua MacDonald wrote:
> > > 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.
>
> 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.
--
Daniel
On Fri, Jul 26, 2002 at 10:42:58AM -0700, Jesse Barnes wrote:
> On Fri, Jul 26, 2002 at 04:09:18PM +0400, Joshua MacDonald wrote:
> > 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.
>
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