LinuxLists.cc - [PATCH] lock assertion macros for 2.5.30

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Wed, Aug 07, 2002 at 06:02:19PM -0300, Rik van Riel wrote:
> On Wed, 7 Aug 2002, Jesse Barnes wrote:
>
> > +++ linux-2.5.30-lockassert/drivers/scsi/scsi.c Wed Aug 7 11:35:32 2002
> > @@ -262,7 +262,7 @@
>
> > + MUST_NOT_HOLD(q->queue_lock);
>
> ...
>
> > +#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))
>
> Please tell me the MUST_NOT_HOLD thing is a joke.
>
> What is to prevent another CPU in another code path
> from holding this spinlock when the code you've
> inserted the MUST_NOT_HOLD in is on its merry way
> not holding the lock ?

Nothing at all, but isn't that how the scsi ASSERT_LOCK(&lock, 0)
macro worked before? I could just remove all those checks in the scsi
code I guess.

After I posted the last patch, a few people asked for MUST_NOT_HOLD so
I added it back in. Do you think it's a bad idea? See the last
thread if you're curious (Joshua's comments in particular):
http://marc.theaimsgroup.com/?t=102764009400001&r=1&w=2

Thanks,
Jesse

2002-08-07 20:59:16

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Wed, 7 Aug 2002, Jesse Barnes wrote:

> +++ linux-2.5.30-lockassert/drivers/scsi/scsi.c Wed Aug 7 11:35:32 2002
> @@ -262,7 +262,7 @@

> + MUST_NOT_HOLD(q->queue_lock);

...

> +#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))

Please tell me the MUST_NOT_HOLD thing is a joke.

What is to prevent another CPU in another code path
from holding this spinlock when the code you've
inserted the MUST_NOT_HOLD in is on its merry way
not holding the lock ?

regards,

Rik
--
Bravely reimplemented by the knights who say "NIH".

http://www.surriel.com/ http://distro.conectiva.com/

2002-08-07 21:17:44

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Wed, 7 Aug 2002, Jesse Barnes wrote:

> > > +#define MUST_HOLD(lock) BUG_ON(!spin_is_locked(lock))
> > > +#define MUST_NOT_HOLD(lock) BUG_ON(spin_is_locked(lock))
> >
> > Please tell me the MUST_NOT_HOLD thing is a joke.
>
> Nothing at all, but isn't that how the scsi ASSERT_LOCK(&lock, 0)
> macro worked before? I could just remove all those checks in the scsi
> code I guess.

That would be a better option.

> --- linux-2.5.30/drivers/scsi/scsi_lib.c Thu Aug 1 14:16:26 2002
> +++ linux-2.5.30-lockassert/drivers/scsi/scsi_lib.c Wed Aug 7 11:34:39 2002
> @@ -202,7 +202,7 @@
> Scsi_Device *SDpnt;
> struct Scsi_Host *SHpnt;
>
> - ASSERT_LOCK(q->queue_lock, 0);
> + MUST_NOT_HOLD(q->queue_lock);
>
> spin_lock_irqsave(q->queue_lock, flags);
> if (SCpnt != NULL) {

> After I posted the last patch, a few people asked for MUST_NOT_HOLD so
> I added it back in. Do you think it's a bad idea?

Just look at the above code (also from your patch).

The fact that we take the spin_lock_irqsave() at that point
means we want to protect ourselves from another CPU here.

The MUST_NOT_HOLD basically means the kernel will OOPS the
moment the lock is contended.

In effect, this debugging code makes lock contention fatal!

If you want to detect lock recursion on the same CPU, I'd
suggest the following:

1) add a 'cpu' member to spinlock_t

2) whenever we take a spinlock, assign the current CPU
id to the spinlock->cpu

3) in the spinlock slow path (ie. when the spinlock is
contended and we have to spin) check if the CPU holding
the spinlock is the current CPU ... if it is, BUG()

4) on spin_unlock, check that the CPU unlocking the spinlock
is the same one that's holding the spinlock

This will have the advantages that it'll actually work and
it will also debug spinlock recursion for ANY spinlock in
the system, without the need to insert special debugging
macros into the code...

regards,

Rik
--
Bravely reimplemented by the knights who say "NIH".

http://www.surriel.com/ http://distro.conectiva.com/

2002-08-07 21:36:19

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Wed, Aug 07, 2002 at 06:21:07PM -0300, Rik van Riel wrote:
> On Wed, 7 Aug 2002, Jesse Barnes wrote:
> > macro worked before? I could just remove all those checks in the scsi
> > code I guess.
>
> That would be a better option.

Ok.

> The MUST_NOT_HOLD basically means the kernel will OOPS the
> moment the lock is contended.

I think those macros were intended to enforce lock ordering in the
scsi layer (though I'm not sure). At any rate, there are much better
ways to enforce proper lock ordering, and the scsi layer could be
updated when/if we get one.

> If you want to detect lock recursion on the same CPU, I'd
> suggest the following:
> ...

Of course, that's what the lockmetering code does, IIRC, but I think
that's a feature for a seperate patch.

Thanks,
Jesse

2002-08-07 21:34:37

by Oliver Xymoron

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Wed, 7 Aug 2002, Jesse Barnes wrote:

> On Wed, Aug 07, 2002 at 06:02:19PM -0300, Rik van Riel wrote:
> > On Wed, 7 Aug 2002, Jesse Barnes wrote:
> >
> > > +++ linux-2.5.30-lockassert/drivers/scsi/scsi.c Wed Aug 7 11:35:32 2002
> > > @@ -262,7 +262,7 @@
> >
> > > + MUST_NOT_HOLD(q->queue_lock);
> >
> > ...
> >
> > > +#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))
> >
> > Please tell me the MUST_NOT_HOLD thing is a joke.
> >
> > What is to prevent another CPU in another code path
> > from holding this spinlock when the code you've
> > inserted the MUST_NOT_HOLD in is on its merry way
> > not holding the lock ?
>
> Nothing at all, but isn't that how the scsi ASSERT_LOCK(&lock, 0)
> macro worked before? I could just remove all those checks in the scsi
> code I guess.

Who's to say that they actually worked? They look like crap to me.

> After I posted the last patch, a few people asked for MUST_NOT_HOLD so
> I added it back in. Do you think it's a bad idea? See the last
> thread if you're curious (Joshua's comments in particular):
> http://marc.theaimsgroup.com/?t=102764009400001&r=1&w=2

Interesting. I'm still going to say that MUST_NOT_HOLD is wrong, at least
in its current form/name.

What MUST_HOLD is saying is "the current thread is holding this lock, go
ahead, double check if you want". What MUST_NOT_HOLD says is "the current
thread is not holding this lock, feel free to check". Right now the kernel
doesn't record who grabbed a lock and the best it can do is check whether
_anyone_ is holding the lock. In the first case, it can prove a negative
if no one is holding the lock, in the second case it can't because it
can't distiguish between the current task holding a lock and any other
task holding a lock.

If we want a MUST_NOT_RECURSE, we can do that, but it means adding cpu or
current into the debugging version of spinlocks. I'd also add eip, so we
can see where the lock was acquired last and dump that when we hit a
conflict/deadlock.

And if you interpret MUST_NOT_HOLD_LOCK to mean "no one is holding this
lock" then you run into Rik's problem. Anyone who actually means this
ought to be simply taking the lock, otherwise why would they care?

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

2002-08-07 21:41:00

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Wed, 7 Aug 2002, Jesse Barnes wrote:

> > The MUST_NOT_HOLD basically means the kernel will OOPS the
> > moment the lock is contended.
>
> I think those macros were intended to enforce lock ordering in the
> scsi layer (though I'm not sure).

If you can prove that a MUST_NOT_HOLD(foolock) will never
trigger because it is already protected by other locks,
then what's the point of having that foolock in the first
place ? (since the region is already protected...)

If the foolock is actually protecting something, then by
definition lock contention is possible and the kernel will
Oops in MUST_NOT_HOLD(foolock).

> > If you want to detect lock recursion on the same CPU, I'd
> > suggest the following:
> > ...
>
> Of course, that's what the lockmetering code does, IIRC, but I think
> that's a feature for a seperate patch.

Agreed.

Btw, the MUST_HOLD macro _is_ straightforward and extremely
useful. IMHO it'd be a shame to have only the SCSI code use it ;)

regards,

Rik
--
Bravely reimplemented by the knights who say "NIH".

http://www.surriel.com/ http://distro.conectiva.com/

2002-08-07 22:12:05

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Wed, Aug 07, 2002 at 06:21:07PM -0300, Rik van Riel wrote:
> On Wed, 7 Aug 2002, Jesse Barnes wrote:
> > macro worked before? I could just remove all those checks in the scsi
> > code I guess.
>
> That would be a better option.

Does this look a little better? Thanks for checking it out.

Jesse

diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/drivers/scsi/aha152x.c linux-2.5.30-lockassert/drivers/scsi/aha152x.c
--- linux-2.5.30/drivers/scsi/aha152x.c Thu Aug 1 14:16:04 2002
+++ linux-2.5.30-lockassert/drivers/scsi/aha152x.c Wed Aug 7 11:33:38 2002
@@ -1435,7 +1435,7 @@
*/
static int setup_expected_interrupts(struct Scsi_Host *shpnt)
{
- ASSERT_LOCK(&QLOCK,1);
+ MUST_HOLD(&QLOCK);

if(CURRENT_SC) {
CURRENT_SC->SCp.phase |= 1 << 16;
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/drivers/scsi/scsi.c linux-2.5.30-lockassert/drivers/scsi/scsi.c
--- linux-2.5.30/drivers/scsi/scsi.c Thu Aug 1 14:16:26 2002
+++ linux-2.5.30-lockassert/drivers/scsi/scsi.c Wed Aug 7 15:12:13 2002
@@ -262,7 +262,6 @@
struct request_queue *q = &SCpnt->device->request_queue;
unsigned long flags;

- ASSERT_LOCK(q->queue_lock, 0);
req->rq_status = RQ_SCSI_DONE; /* Busy, but indicate request done */

spin_lock_irqsave(q->queue_lock, flags);
@@ -669,8 +668,6 @@

host = SCpnt->host;

- ASSERT_LOCK(host->host_lock, 0);
-
/* Assign a unique nonzero serial_number. */
if (++serial_number == 0)
serial_number = 1;
@@ -827,8 +824,6 @@
Scsi_Device * SDpnt = SRpnt->sr_device;
struct Scsi_Host *host = SDpnt->host;

- ASSERT_LOCK(host->host_lock, 0);
-
SCSI_LOG_MLQUEUE(4,
{
int i;
@@ -924,8 +919,6 @@
{
struct Scsi_Host *host = SCpnt->host;

- ASSERT_LOCK(host->host_lock, 0);
-
SCpnt->owner = SCSI_OWNER_MIDLEVEL;
SRpnt->sr_command = SCpnt;

@@ -1013,8 +1006,6 @@
{
struct Scsi_Host *host = SCpnt->host;

- ASSERT_LOCK(host->host_lock, 0);
-
SCpnt->pid = scsi_pid++;
SCpnt->owner = SCSI_OWNER_MIDLEVEL;

@@ -1338,8 +1329,6 @@

host = SCpnt->host;
device = SCpnt->device;
-
- ASSERT_LOCK(host->host_lock, 0);

/*
* We need to protect the decrement, as otherwise a race condition
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/drivers/scsi/scsi.h linux-2.5.30-lockassert/drivers/scsi/scsi.h
--- linux-2.5.30/drivers/scsi/scsi.h Thu Aug 1 14:16:15 2002
+++ linux-2.5.30-lockassert/drivers/scsi/scsi.h Wed Aug 7 11:32:25 2002
@@ -99,21 +99,6 @@
#endif

/*
- * Used for debugging the new queueing code. We want to make sure
- * that the lock state is consistent with design. Only do this in
- * the user space simulator.
- */
-#define ASSERT_LOCK(_LOCK, _COUNT)
-
-#if defined(CONFIG_SMP) && defined(CONFIG_USER_DEBUG)
-#undef ASSERT_LOCK
-#define ASSERT_LOCK(_LOCK,_COUNT) \
- { if( (_LOCK)->lock != _COUNT ) \
- panic("Lock count inconsistent %s %d\n", __FILE__, __LINE__); \
- }
-#endif
-
-/*
* Use these to separate status msg and our bytes
*
* These are set by:
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/drivers/scsi/scsi_error.c linux-2.5.30-lockassert/drivers/scsi/scsi_error.c
--- linux-2.5.30/drivers/scsi/scsi_error.c Thu Aug 1 14:16:02 2002
+++ linux-2.5.30-lockassert/drivers/scsi/scsi_error.c Wed Aug 7 15:12:25 2002
@@ -581,8 +581,6 @@
unsigned long flags;
struct Scsi_Host *host = SCpnt->host;

- ASSERT_LOCK(host->host_lock, 0);
-
retry:
/*
* We will use a queued command if possible, otherwise we will
@@ -1251,8 +1249,6 @@
Scsi_Device *SDpnt;
unsigned long flags;

- ASSERT_LOCK(host->host_lock, 0);
-
/*
* Next free up anything directly waiting upon the host. This will be
* requests for character device operations, and also for ioctls to queued
@@ -1327,8 +1323,6 @@
Scsi_Device *SDloop;
Scsi_Cmnd *SCdone;
int timed_out;
-
- ASSERT_LOCK(host->host_lock, 0);

SCdone = NULL;

diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/drivers/scsi/scsi_lib.c linux-2.5.30-lockassert/drivers/scsi/scsi_lib.c
--- linux-2.5.30/drivers/scsi/scsi_lib.c Thu Aug 1 14:16:26 2002
+++ linux-2.5.30-lockassert/drivers/scsi/scsi_lib.c Wed Aug 7 15:12:44 2002
@@ -202,8 +202,6 @@
Scsi_Device *SDpnt;
struct Scsi_Host *SHpnt;

- ASSERT_LOCK(q->queue_lock, 0);
-
spin_lock_irqsave(q->queue_lock, flags);
if (SCpnt != NULL) {

@@ -315,8 +313,6 @@
struct request *req = SCpnt->request;
unsigned long flags;

- ASSERT_LOCK(q->queue_lock, 0);
-
spin_lock_irqsave(q->queue_lock, flags);
/*
* If there are blocks left over at the end, set up the command
@@ -402,8 +398,6 @@
{
struct request *req = SCpnt->request;

- ASSERT_LOCK(SCpnt->host->host_lock, 0);
-
/*
* Free up any indirection buffers we allocated for DMA purposes.
*/
@@ -465,7 +459,6 @@
* would be used if we just wanted to retry, for example.
*
*/
- ASSERT_LOCK(q->queue_lock, 0);

/*
* Free up any indirection buffers we allocated for DMA purposes.
@@ -733,7 +726,7 @@
struct Scsi_Host *SHpnt;
struct Scsi_Device_Template *STpnt;

- ASSERT_LOCK(q->queue_lock, 1);
+ MUST_HOLD(q->queue_lock);

SDpnt = (Scsi_Device *) q->queuedata;
if (!SDpnt) {
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/fs/inode.c linux-2.5.30-lockassert/fs/inode.c
--- linux-2.5.30/fs/inode.c Thu Aug 1 14:16:45 2002
+++ linux-2.5.30-lockassert/fs/inode.c Wed Aug 7 11:10:48 2002
@@ -193,6 +193,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.30/include/asm-i386/semaphore.h linux-2.5.30-lockassert/include/asm-i386/semaphore.h
--- linux-2.5.30/include/asm-i386/semaphore.h Thu Aug 1 14:16:16 2002
+++ linux-2.5.30-lockassert/include/asm-i386/semaphore.h Wed Aug 7 13:39:00 2002
@@ -40,6 +40,7 @@
#include <asm/atomic.h>
#include <linux/wait.h>
#include <linux/rwsem.h>
+#include <linux/config.h>

struct semaphore {
atomic_t count;
@@ -55,6 +56,12 @@
, (int)&(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.30/include/asm-i386/spinlock.h linux-2.5.30-lockassert/include/asm-i386/spinlock.h
--- linux-2.5.30/include/asm-i386/spinlock.h Thu Aug 1 14:16:14 2002
+++ linux-2.5.30-lockassert/include/asm-i386/spinlock.h Wed Aug 7 11:14:49 2002
@@ -157,6 +157,7 @@
#define RW_LOCK_UNLOCKED (rwlock_t) { RW_LOCK_BIAS RWLOCK_MAGIC_INIT }

#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+#define rwlock_is_locked(x) ((x)->lock != RW_LOCK_BIAS)

/*
* On x86, we implement read-write locks as a 32-bit counter
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/include/asm-ia64/semaphore.h linux-2.5.30-lockassert/include/asm-ia64/semaphore.h
--- linux-2.5.30/include/asm-ia64/semaphore.h Thu Aug 1 14:16:24 2002
+++ linux-2.5.30-lockassert/include/asm-ia64/semaphore.h Wed Aug 7 13:41:14 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.30/include/asm-ia64/spinlock.h linux-2.5.30-lockassert/include/asm-ia64/spinlock.h
--- linux-2.5.30/include/asm-ia64/spinlock.h Thu Aug 1 14:16:06 2002
+++ linux-2.5.30-lockassert/include/asm-ia64/spinlock.h Wed Aug 7 11:10:48 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.30/include/linux/rwsem-spinlock.h linux-2.5.30-lockassert/include/linux/rwsem-spinlock.h
--- linux-2.5.30/include/linux/rwsem-spinlock.h Thu Aug 1 14:16:28 2002
+++ linux-2.5.30-lockassert/include/linux/rwsem-spinlock.h Wed Aug 7 13:42:03 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.30/include/linux/rwsem.h linux-2.5.30-lockassert/include/linux/rwsem.h
--- linux-2.5.30/include/linux/rwsem.h Thu Aug 1 14:16:18 2002
+++ linux-2.5.30-lockassert/include/linux/rwsem.h Wed Aug 7 11:10:48 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.30/include/linux/spinlock.h linux-2.5.30-lockassert/include/linux/spinlock.h
--- linux-2.5.30/include/linux/spinlock.h Thu Aug 1 14:16:25 2002
+++ linux-2.5.30-lockassert/include/linux/spinlock.h Wed Aug 7 11:31:45 2002
@@ -117,7 +117,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(lock) BUG_ON(!spin_is_locked(lock))
+#define MUST_NOT_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_NOT_HOLD(lock) do { } while(0)
+#define MUST_HOLD_RW(lock) do { } while(0)
+#endif /* CONFIG_DEBUG_SPINLOCK && CONFIG_SMP */

#ifdef CONFIG_PREEMPT

diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/kernel/acct.c linux-2.5.30-lockassert/kernel/acct.c
--- linux-2.5.30/kernel/acct.c Thu Aug 1 14:16:27 2002
+++ linux-2.5.30-lockassert/kernel/acct.c Wed Aug 7 11:47:14 2002
@@ -160,6 +160,8 @@
{
struct file *old_acct = NULL;

+ MUST_HOLD(&acct_globals.lock);
+
if (acct_globals.file) {
old_acct = acct_globals.file;
del_timer(&acct_globals.timer);
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/kernel/printk.c linux-2.5.30-lockassert/kernel/printk.c
--- linux-2.5.30/kernel/printk.c Thu Aug 1 14:16:25 2002
+++ linux-2.5.30-lockassert/kernel/printk.c Wed Aug 7 11:46:22 2002
@@ -337,6 +337,8 @@
unsigned long cur_index, start_print;
static int msg_level = -1;

+ MUST_HOLD_SEM(&console_sem);
+
if (((long)(start - end)) > 0)
BUG();

2002-08-07 22:16:10

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Wed, 7 Aug 2002, Jesse Barnes wrote:
> On Wed, Aug 07, 2002 at 06:21:07PM -0300, Rik van Riel wrote:
> > On Wed, 7 Aug 2002, Jesse Barnes wrote:
> > > macro worked before? I could just remove all those checks in the scsi
> > > code I guess.
> >
> > That would be a better option.
>
> Does this look a little better? Thanks for checking it out.

Looks good to me. Would be even better if you removed MUST_NOT_HOLD ;)

regards,

Rik
--
Bravely reimplemented by the knights who say "NIH".

http://www.surriel.com/ http://distro.conectiva.com/

2002-08-07 22:24:46

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

A couple of whitespace glitches:

> +#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))
> +#define MUST_HOLD_RW(lock)?????????????BUG_ON(!rwlock_is_locked(lock))
> +#else
> +#define MUST_HOLD(lock)????????????????????????do { } while(0)
> +#define MUST_NOT_HOLD(lock)????????????do { } while(0)
> +#define MUST_HOLD_RW(lock)?????????????do { } while(0)
> +#endif /* CONFIG_DEBUG_SPINLOCK && CONFIG_SMP */

Random gripe: don't all those do { } whiles look silly? We need

#define NADA do { } while (0)

or similar.

--
Daniel

2002-08-07 22:38:18

by Roman Zippel

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

Hi,

On Thu, 8 Aug 2002, Daniel Phillips wrote:

> > +#define MUST_HOLD_RW(lock)?????????????do { } while(0)
>
> Random gripe: don't all those do { } whiles look silly? We need
>
> #define NADA do { } while (0)

IMO "((void)0)" looks nice too.

bye, Roman

2002-08-08 00:04:37

by Thunder from the hill

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

Hi,

On Thu, 8 Aug 2002, Daniel Phillips wrote:
> Random gripe: don't all those do { } whiles look silly? We need
>
> #define NADA do { } while (0)
>
> or similar.

Call it nop.

Thunder
--
.-../../-./..-/-..- .-./..-/.-.././.../.-.-.-

2002-08-08 05:57:47

by Jens Axboe

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Wed, Aug 07 2002, Jesse Barnes wrote:
> On Wed, Aug 07, 2002 at 06:02:19PM -0300, Rik van Riel wrote:
> > On Wed, 7 Aug 2002, Jesse Barnes wrote:
> >
> > > +++ linux-2.5.30-lockassert/drivers/scsi/scsi.c Wed Aug 7 11:35:32 2002
> > > @@ -262,7 +262,7 @@
> >
> > > + MUST_NOT_HOLD(q->queue_lock);
> >
> > ...
> >
> > > +#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))
> >
> > Please tell me the MUST_NOT_HOLD thing is a joke.
> >
> > What is to prevent another CPU in another code path
> > from holding this spinlock when the code you've
> > inserted the MUST_NOT_HOLD in is on its merry way
> > not holding the lock ?
>
> Nothing at all, but isn't that how the scsi ASSERT_LOCK(&lock, 0)
> macro worked before? I could just remove all those checks in the scsi
> code I guess.

The SCSI ASSERT_LOCK() were never used from kernel space, they are for
the user space similator. So it was always single threaded from there
and has no bearing on what actual kernel code does.

For MUST_NOT_HOLD to work, you need to take into account which processor
took the lock etc.

> After I posted the last patch, a few people asked for MUST_NOT_HOLD so
> I added it back in. Do you think it's a bad idea? See the last

Your current version is surely worthless.

--
Jens Axboe

2002-08-08 08:00:43

by Marcin Dalecki

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

Uz.ytkownik Jesse Barnes napisa?:
> On Wed, Aug 07, 2002 at 06:21:07PM -0300, Rik van Riel wrote:
>
>>On Wed, 7 Aug 2002, Jesse Barnes wrote:
>>
>>>macro worked before? I could just remove all those checks in the scsi
>>>code I guess.
>>
>>That would be a better option.
>
>
> Ok.
>
>
>>The MUST_NOT_HOLD basically means the kernel will OOPS the
>>moment the lock is contended.

And for Gods sake ;-) - please call it ASSERT_ and not MUST_THIS
MUST_THAT MUST_GO_TO_TOILET. For the nonanglosaxons among us the
personifications already frequently enough present in english
nomenclature sound already awfoul enough... Additionally gerp -i assert
will work as expected, since MUST is a far more frequent term.

2002-08-08 11:04:00

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Thursday 08 August 2002 09:58, Marcin Dalecki wrote:
> And for Gods sake ;-) - please call it ASSERT_ and not MUST_THIS
> MUST_THAT MUST_GO_TO_TOILET. For the nonanglosaxons among us the
> personifications already frequently enough present in english
> nomenclature sound already awfoul enough...

As I understand it, the kernel nomenclature is supposed to be English,
what is your point?

--
Daniel

2002-08-08 12:51:28

by Joshua MacDonald

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

MUST_NOT_HOLD_LOCK means exactly I_AM_NOT_HOLDING_LOCK, otherwise the
assertion is obviously meaningless because another processor could be holding
the lock. But since there is no reason to assert NO_ONE_IS_HOLDING_LOCK
(since it means the lock is unnecessary), the obvious meaning of
MUST_NOT_HOLD_LOCK is the correct one, that the current thread/CPU does not
hold the lock.

In order to implement MOST_NOT_HOLD_LOCK the spinlock must contain some record
of who holds the lock, and since the SCSI-layer apparently does not have such
a mechanism, it appears that something is broken in there.

-josh

On Wed, Aug 07, 2002 at 04:37:40PM -0500, Oliver Xymoron wrote:
> On Wed, 7 Aug 2002, Jesse Barnes wrote:
>
> > On Wed, Aug 07, 2002 at 06:02:19PM -0300, Rik van Riel wrote:
> > > On Wed, 7 Aug 2002, Jesse Barnes wrote:
> > >
> > > > +++ linux-2.5.30-lockassert/drivers/scsi/scsi.c Wed Aug 7 11:35:32 2002
> > > > @@ -262,7 +262,7 @@
> > >
> > > > + MUST_NOT_HOLD(q->queue_lock);
> > >
> > > ...
> > >
> > > > +#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))
> > >
> > > Please tell me the MUST_NOT_HOLD thing is a joke.
> > >
> > > What is to prevent another CPU in another code path
> > > from holding this spinlock when the code you've
> > > inserted the MUST_NOT_HOLD in is on its merry way
> > > not holding the lock ?
> >
> > Nothing at all, but isn't that how the scsi ASSERT_LOCK(&lock, 0)
> > macro worked before? I could just remove all those checks in the scsi
> > code I guess.
>
> Who's to say that they actually worked? They look like crap to me.
>
> > After I posted the last patch, a few people asked for MUST_NOT_HOLD so
> > I added it back in. Do you think it's a bad idea? See the last
> > thread if you're curious (Joshua's comments in particular):
> > http://marc.theaimsgroup.com/?t=102764009400001&r=1&w=2
>
> Interesting. I'm still going to say that MUST_NOT_HOLD is wrong, at least
> in its current form/name.
>
> What MUST_HOLD is saying is "the current thread is holding this lock, go
> ahead, double check if you want". What MUST_NOT_HOLD says is "the current
> thread is not holding this lock, feel free to check". Right now the kernel
> doesn't record who grabbed a lock and the best it can do is check whether
> _anyone_ is holding the lock. In the first case, it can prove a negative
> if no one is holding the lock, in the second case it can't because it
> can't distiguish between the current task holding a lock and any other
> task holding a lock.
>
> If we want a MUST_NOT_RECURSE, we can do that, but it means adding cpu or
> current into the debugging version of spinlocks. I'd also add eip, so we
> can see where the lock was acquired last and dump that when we hit a
> conflict/deadlock.
>
> And if you interpret MUST_NOT_HOLD_LOCK to mean "no one is holding this
> lock" then you run into Rik's problem. Anyone who actually means this
> ought to be simply taking the lock, otherwise why would they care?
>
> --
> "Love the dolphins," she advised him. "Write by W.A.S.T.E.."
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
>

2002-08-08 13:20:26

by Jens Axboe

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Thu, Aug 08 2002, Joshua MacDonald wrote:
> In order to implement MOST_NOT_HOLD_LOCK the spinlock must contain
> some record of who holds the lock, and since the SCSI-layer apparently

Correct

> does not have such a mechanism, it appears that something is broken in
> there.

I'll restate, the SCSI layer does _not_ use ASSERT_LOCK in the kernel!
If just one of the people that keep raising this point would actually
see what it does rather than assume, we would not keep seeing this
mentioned in this discussion!

--
Jens Axboe

2002-08-08 17:04:56

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Thu, Aug 08, 2002 at 08:00:45AM +0200, Jens Axboe wrote:
> The SCSI ASSERT_LOCK() were never used from kernel space, they are for
> the user space similator. So it was always single threaded from there
> and has no bearing on what actual kernel code does.
>
> For MUST_NOT_HOLD to work, you need to take into account which processor
> took the lock etc.

That's the only way it seems to be useful...

> > After I posted the last patch, a few people asked for MUST_NOT_HOLD so
> > I added it back in. Do you think it's a bad idea? See the last
>
> Your current version is surely worthless.

Agreed. I'll post another patch that doesn't mess with the scsi
stuff. Maybe later I can put together a useful
'lock-not-held-on-this-cpu' macro.

Thanks,
Jesse

2002-08-08 17:20:04

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Wed, Aug 07, 2002 at 07:19:21PM -0300, Rik van Riel wrote:
> Looks good to me. Would be even better if you removed MUST_NOT_HOLD ;)

Ok, here's yet another version. I've removed the conversion of the
scsi layer's ASSERT_LOCK macros as well as the silly version of
MUST_NOT_HOLD. Other things people seem interested in:
o sleeping function assertions
o lock ordering enforcement
o lock recursion detection
o more assertion checks in other parts of the kernel

Should any of the above be included in this patch? If so, I can try
to hack one or more of them together, otherwise maybe this is ok to go
in?

Thanks,
Jesse

diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/fs/inode.c linux-2.5.30-lockassert/fs/inode.c
--- linux-2.5.30/fs/inode.c Thu Aug 1 14:16:45 2002
+++ linux-2.5.30-lockassert/fs/inode.c Thu Aug 8 10:03:13 2002
@@ -193,6 +193,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.30/include/asm-i386/semaphore.h linux-2.5.30-lockassert/include/asm-i386/semaphore.h
--- linux-2.5.30/include/asm-i386/semaphore.h Thu Aug 1 14:16:16 2002
+++ linux-2.5.30-lockassert/include/asm-i386/semaphore.h Thu Aug 8 10:03:13 2002
@@ -40,6 +40,7 @@
#include <asm/atomic.h>
#include <linux/wait.h>
#include <linux/rwsem.h>
+#include <linux/config.h>

struct semaphore {
atomic_t count;
@@ -55,6 +56,12 @@
, (int)&(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.30/include/asm-i386/spinlock.h linux-2.5.30-lockassert/include/asm-i386/spinlock.h
--- linux-2.5.30/include/asm-i386/spinlock.h Thu Aug 1 14:16:14 2002
+++ linux-2.5.30-lockassert/include/asm-i386/spinlock.h Thu Aug 8 10:03:13 2002
@@ -157,6 +157,7 @@
#define RW_LOCK_UNLOCKED (rwlock_t) { RW_LOCK_BIAS RWLOCK_MAGIC_INIT }

#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+#define rwlock_is_locked(x) ((x)->lock != RW_LOCK_BIAS)

/*
* On x86, we implement read-write locks as a 32-bit counter
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/include/asm-ia64/semaphore.h linux-2.5.30-lockassert/include/asm-ia64/semaphore.h
--- linux-2.5.30/include/asm-ia64/semaphore.h Thu Aug 1 14:16:24 2002
+++ linux-2.5.30-lockassert/include/asm-ia64/semaphore.h Thu Aug 8 10:03:13 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.30/include/asm-ia64/spinlock.h linux-2.5.30-lockassert/include/asm-ia64/spinlock.h
--- linux-2.5.30/include/asm-ia64/spinlock.h Thu Aug 1 14:16:06 2002
+++ linux-2.5.30-lockassert/include/asm-ia64/spinlock.h Thu Aug 8 10:03:13 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.30/include/linux/rwsem-spinlock.h linux-2.5.30-lockassert/include/linux/rwsem-spinlock.h
--- linux-2.5.30/include/linux/rwsem-spinlock.h Thu Aug 1 14:16:28 2002
+++ linux-2.5.30-lockassert/include/linux/rwsem-spinlock.h Thu Aug 8 10:03:13 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.30/include/linux/rwsem.h linux-2.5.30-lockassert/include/linux/rwsem.h
--- linux-2.5.30/include/linux/rwsem.h Thu Aug 1 14:16:18 2002
+++ linux-2.5.30-lockassert/include/linux/rwsem.h Thu Aug 8 10:03:13 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.30/include/linux/spinlock.h linux-2.5.30-lockassert/include/linux/spinlock.h
--- linux-2.5.30/include/linux/spinlock.h Thu Aug 1 14:16:25 2002
+++ linux-2.5.30-lockassert/include/linux/spinlock.h Thu Aug 8 10:04:23 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 held.
+ */
+#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

diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/kernel/acct.c linux-2.5.30-lockassert/kernel/acct.c
--- linux-2.5.30/kernel/acct.c Thu Aug 1 14:16:27 2002
+++ linux-2.5.30-lockassert/kernel/acct.c Thu Aug 8 10:03:13 2002
@@ -160,6 +160,8 @@
{
struct file *old_acct = NULL;

+ MUST_HOLD(&acct_globals.lock);
+
if (acct_globals.file) {
old_acct = acct_globals.file;
del_timer(&acct_globals.timer);
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.30/kernel/printk.c linux-2.5.30-lockassert/kernel/printk.c
--- linux-2.5.30/kernel/printk.c Thu Aug 1 14:16:25 2002
+++ linux-2.5.30-lockassert/kernel/printk.c Thu Aug 8 10:03:13 2002
@@ -337,6 +337,8 @@
unsigned long cur_index, start_print;
static int msg_level = -1;

+ MUST_HOLD_SEM(&console_sem);
+
if (((long)(start - end)) > 0)
BUG();

2002-08-08 17:28:13

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Thu, 8 Aug 2002, Jesse Barnes wrote:
> On Thu, Aug 08, 2002 at 08:00:45AM +0200, Jens Axboe wrote:

> > For MUST_NOT_HOLD to work, you need to take into account which processor
> > took the lock etc.

[snip]

> Agreed. I'll post another patch that doesn't mess with the scsi
> stuff. Maybe later I can put together a useful
> 'lock-not-held-on-this-cpu' macro.

You don't need to put this in a macro. This test is valid
for ALL spinlocks in the kernel and can be done from inside
the spin_lock() macro itself, when spinlock debugging is on.

regards,

Rik
--
http://www.linuxsymposium.org/2002/
"You're one of those condescending OLS attendants"
"Here's a nickle kid. Go buy yourself a real t-shirt"

http://www.surriel.com/ http://distro.conectiva.com/

2002-08-08 17:32:21

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Thu, Aug 08, 2002 at 02:31:40PM -0300, Rik van Riel wrote:
> > Agreed. I'll post another patch that doesn't mess with the scsi
> > stuff. Maybe later I can put together a useful
> > 'lock-not-held-on-this-cpu' macro.
>
> You don't need to put this in a macro. This test is valid
> for ALL spinlocks in the kernel and can be done from inside
> the spin_lock() macro itself, when spinlock debugging is on.

Right, right after I posted I regretted using that term. Of course it
would have to include changes to spinlock_t and spin_lock/unlock.
Seems like it could be integrated somewhat easily with the lock
ordering stuff that was talked about earlier too.

Thanks,
Jesse

2002-08-08 17:34:47

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Thu, 8 Aug 2002, Jesse Barnes wrote:
> On Wed, Aug 07, 2002 at 07:19:21PM -0300, Rik van Riel wrote:
> > Looks good to me. Would be even better if you removed MUST_NOT_HOLD ;)
>
> Ok, here's yet another version.

Looks fine to me, but I'm not a SCSI guy so you'll have to
ask them about integrating the patch ;)

The other issues you raised are probably best done in
separate patches.

regards,

Rik
--
http://www.linuxsymposium.org/2002/
"You're one of those condescending OLS attendants"
"Here's a nickle kid. Go buy yourself a real t-shirt"

http://www.surriel.com/ http://distro.conectiva.com/

2002-08-08 17:37:17

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Thu, Aug 08, 2002 at 02:36:46PM -0300, Rik van Riel wrote:
> Looks fine to me, but I'm not a SCSI guy so you'll have to
> ask them about integrating the patch ;)

Oh, I meant that I removed those changes _from my patch_. Now they're
confined to x86 and ia64 header files and some generic files. I'm not
sure who to send it to now (Linus?).

> The other issues you raised are probably best done in
> separate patches.

Ok, thanks a lot for the feedback.

Jesse

2002-08-08 17:44:33

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Thu, 8 Aug 2002, Joshua MacDonald wrote:
> On Thu, Aug 08, 2002 at 02:31:40PM -0300, Rik van Riel wrote:
> > On Thu, 8 Aug 2002, Jesse Barnes wrote:

> > > Agreed. I'll post another patch that doesn't mess with the scsi
> > > stuff. Maybe later I can put together a useful
> > > 'lock-not-held-on-this-cpu' macro.
> >
> > You don't need to put this in a macro. This test is valid
> > for ALL spinlocks in the kernel and can be done from inside
> > the spin_lock() macro itself, when spinlock debugging is on.
>
> This is just not true. When you make this assertion, it doesn't mean
> you intend to take the lock. It could have to do with lock ordering, or
> it could be testing that some lock is properly released.

Hmm, I guess you might be right. This could indeed be useful
for indirectly called functions like ->open() functions in
drivers, etc...

kind regards,

Rik
--
http://www.linuxsymposium.org/2002/
"You're one of those condescending OLS attendants"
"Here's a nickle kid. Go buy yourself a real t-shirt"

http://www.surriel.com/ http://distro.conectiva.com/

2002-08-08 17:39:53

by Joshua MacDonald

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Thu, Aug 08, 2002 at 02:31:40PM -0300, Rik van Riel wrote:
> On Thu, 8 Aug 2002, Jesse Barnes wrote:
> > On Thu, Aug 08, 2002 at 08:00:45AM +0200, Jens Axboe wrote:
>
> > > For MUST_NOT_HOLD to work, you need to take into account which processor
> > > took the lock etc.
>
> [snip]
>
> > Agreed. I'll post another patch that doesn't mess with the scsi
> > stuff. Maybe later I can put together a useful
> > 'lock-not-held-on-this-cpu' macro.
>
> You don't need to put this in a macro. This test is valid
> for ALL spinlocks in the kernel and can be done from inside
> the spin_lock() macro itself, when spinlock debugging is on.
>

This is just not true. When you make this assertion, it doesn't mean you
intend to take the lock. It could have to do with lock ordering, or it could
be testing that some lock is properly released. Why do you seem to be against
more assertions? They won't get in your way.

-josh

2002-08-08 18:48:57

by Paul Menage

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

In article <[email protected]>,
you write:
>
>> Agreed. I'll post another patch that doesn't mess with the scsi
>> stuff. Maybe later I can put together a useful
>> 'lock-not-held-on-this-cpu' macro.
>
>You don't need to put this in a macro. This test is valid
>for ALL spinlocks in the kernel and can be done from inside
>the spin_lock() macro itself, when spinlock debugging is on.
>

There are some cases where this might not be true - e.g. in the
migration code in at least one version of the O(1) scheduler (included
in RedHat's 2.4.18-4) the migration_lock is taken on one CPU and
released on another (following an IPI being sent from the CPU that took
the lock).

So at the very least you'd need a separate set of spinlock primitives
that don't perform this test, which would have to be used by anyone
taking/releasing such a lock.

Paul

2002-08-08 18:57:08

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Thu, 8 Aug 2002, Paul Menage wrote:

> There are some cases where this might not be true - e.g. in the
> migration code in at least one version of the O(1) scheduler (included
> in RedHat's 2.4.18-4) the migration_lock is taken on one CPU and
> released on another (following an IPI being sent from the CPU that took
> the lock).

Colour me impressed ...

Ingo never ceases to amaze ;)

Rik
--
http://www.linuxsymposium.org/2002/
"You're one of those condescending OLS attendants"
"Here's a nickle kid. Go buy yourself a real t-shirt"

http://www.surriel.com/ http://distro.conectiva.com/

2002-08-09 02:51:09

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Thursday 08 August 2002 19:39, Jesse Barnes wrote:
> On Thu, Aug 08, 2002 at 02:36:46PM -0300, Rik van Riel wrote:
> > Looks fine to me, but I'm not a SCSI guy so you'll have to
> > ask them about integrating the patch ;)
>
> Oh, I meant that I removed those changes _from my patch_. Now they're
> confined to x86 and ia64 header files and some generic files. I'm not
> sure who to send it to now (Linus?).

Or Andrew Morton, or both.

--
Daniel

2002-08-09 02:58:45

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

On Thursday 08 August 2002 19:23, Jesse Barnes wrote:
> On Wed, Aug 07, 2002 at 07:19:21PM -0300, Rik van Riel wrote:
> > Looks good to me. Would be even better if you removed MUST_NOT_HOLD ;)
>
> Ok, here's yet another version. I've removed the conversion of the
> scsi layer's ASSERT_LOCK macros as well as the silly version of
> MUST_NOT_HOLD. Other things people seem interested in:
> o sleeping function assertions
> o lock ordering enforcement
> o lock recursion detection
> o more assertion checks in other parts of the kernel
>
> Should any of the above be included in this patch?

You would just have to break the patch up again when you submit it. You
might want create a patch that demonstrates its usage, by adding some
asserts to core code and removing comments where the assert makes them
redundant.

> If so, I can try
> to hack one or more of them together, otherwise maybe this is ok to go
> in?

It's looking good.

--
Daniel

2002-08-09 04:08:36

by Bernd Eckenfels

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.30

In article <E17d04X-0000eD-00@starship> you wrote:
> You would just have to break the patch up again when you submit it. You
> might want create a patch that demonstrates its usage, by adding some
> asserts to core code and removing comments where the assert makes them
> redundant.

Yes, I defintely thing that those asserts are a good way of documenting
contracts. They can be used to document when a function expects a lock to be
held, and they will also be able to empirical test, if it is true.

It may even help static code analysers to find places where the assertion
macros are missing.

Greetings
Bernd

2002-08-12 20:59:52

[permalink] [raw]

Subject: [PATCH] lock assertion macros for 2.5.31

On Fri, Aug 09, 2002 at 04:56:46AM +0200, Daniel Phillips wrote:
> Or Andrew Morton, or both.

Ok, here it is against 2.5.31. Please apply.

Thanks,
Jesse

diff -Naur -X /home/jbarnes/dontdiff linux-2.5.31/fs/inode.c linux-2.5.31-lockassert/fs/inode.c
--- linux-2.5.31/fs/inode.c Sat Aug 10 18:42:05 2002
+++ linux-2.5.31-lockassert/fs/inode.c Mon Aug 12 13:59:36 2002
@@ -193,6 +193,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.31/include/asm-i386/semaphore.h linux-2.5.31-lockassert/include/asm-i386/semaphore.h
--- linux-2.5.31/include/asm-i386/semaphore.h Sat Aug 10 18:41:24 2002
+++ linux-2.5.31-lockassert/include/asm-i386/semaphore.h Mon Aug 12 13:59:36 2002
@@ -40,6 +40,7 @@
#include <asm/atomic.h>
#include <linux/wait.h>
#include <linux/rwsem.h>
+#include <linux/config.h>

struct semaphore {
atomic_t count;
@@ -55,6 +56,12 @@
, (int)&(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.31/include/asm-i386/spinlock.h linux-2.5.31-lockassert/include/asm-i386/spinlock.h
--- linux-2.5.31/include/asm-i386/spinlock.h Sat Aug 10 18:41:23 2002
+++ linux-2.5.31-lockassert/include/asm-i386/spinlock.h Mon Aug 12 13:59:36 2002
@@ -157,6 +157,7 @@
#define RW_LOCK_UNLOCKED (rwlock_t) { RW_LOCK_BIAS RWLOCK_MAGIC_INIT }

#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+#define rwlock_is_locked(x) ((x)->lock != RW_LOCK_BIAS)

/*
* On x86, we implement read-write locks as a 32-bit counter
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.31/include/asm-ia64/semaphore.h linux-2.5.31-lockassert/include/asm-ia64/semaphore.h
--- linux-2.5.31/include/asm-ia64/semaphore.h Sat Aug 10 18:41:28 2002
+++ linux-2.5.31-lockassert/include/asm-ia64/semaphore.h Mon Aug 12 13:59:36 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.31/include/asm-ia64/spinlock.h linux-2.5.31-lockassert/include/asm-ia64/spinlock.h
--- linux-2.5.31/include/asm-ia64/spinlock.h Sat Aug 10 18:41:19 2002
+++ linux-2.5.31-lockassert/include/asm-ia64/spinlock.h Mon Aug 12 13:59:36 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.31/include/linux/rwsem-spinlock.h linux-2.5.31-lockassert/include/linux/rwsem-spinlock.h
--- linux-2.5.31/include/linux/rwsem-spinlock.h Sat Aug 10 18:41:38 2002
+++ linux-2.5.31-lockassert/include/linux/rwsem-spinlock.h Mon Aug 12 13:59:36 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.31/include/linux/rwsem.h linux-2.5.31-lockassert/include/linux/rwsem.h
--- linux-2.5.31/include/linux/rwsem.h Sat Aug 10 18:41:25 2002
+++ linux-2.5.31-lockassert/include/linux/rwsem.h Mon Aug 12 13:59:38 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.31/include/linux/spinlock.h linux-2.5.31-lockassert/include/linux/spinlock.h
--- linux-2.5.31/include/linux/spinlock.h Sat Aug 10 18:41:29 2002
+++ linux-2.5.31-lockassert/include/linux/spinlock.h Mon Aug 12 13:59:38 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 held.
+ */
+#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

diff -Naur -X /home/jbarnes/dontdiff linux-2.5.31/kernel/acct.c linux-2.5.31-lockassert/kernel/acct.c
--- linux-2.5.31/kernel/acct.c Sat Aug 10 18:41:36 2002
+++ linux-2.5.31-lockassert/kernel/acct.c Mon Aug 12 13:59:38 2002
@@ -160,6 +160,8 @@
{
struct file *old_acct = NULL;

+ MUST_HOLD(&acct_globals.lock);
+
if (acct_globals.file) {
old_acct = acct_globals.file;
del_timer(&acct_globals.timer);
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.31/kernel/printk.c linux-2.5.31-lockassert/kernel/printk.c
--- linux-2.5.31/kernel/printk.c Sat Aug 10 18:41:29 2002
+++ linux-2.5.31-lockassert/kernel/printk.c Mon Aug 12 13:59:38 2002
@@ -337,6 +337,8 @@
unsigned long cur_index, start_print;
static int msg_level = -1;

+ MUST_HOLD_SEM(&console_sem);
+
if (((long)(start - end)) > 0)
BUG();

2002-08-21 18:22:27

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.31

On Mon, Aug 12, 2002 at 04:12:41PM -0700, Andrew Morton wrote:
> ...
> #define might_sleep() BUG_ON(preempt_count())
>
> _this_ would catch numerous bugs, including code which is not buggy
> in 2.4, but became buggy when wild-eyed loonies changed core kernel
> rules without even looking at what drivers were doing (rant).
>
> I expect something like this will fall out of the wash soon, at
> least for preemptible kernels.

Is it really that simple? Maybe it should go into sched.h sometime
soon? I guess the real work is sprinkling it in all the places where
it needs to go.

Anyway, here's an updated version of the lock assertion patch. Should
it be split into two patches, one that implements the macros and
another that puts checks everywhere? Should I add a small doc to
Documentation/ (maybe the might_sleep() could be documented there
too)?

Thanks,
Jesse

diff -Naur -X /home/jbarnes/dontdiff linux-2.5.31/fs/inode.c linux-2.5.31-lockassert/fs/inode.c
--- linux-2.5.31/fs/inode.c Sat Aug 10 18:42:05 2002
+++ linux-2.5.31-lockassert/fs/inode.c Wed Aug 21 11:14:49 2002
@@ -193,6 +193,8 @@
*/
void __iget(struct inode * inode)
{
+ 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.31/include/asm-i386/semaphore.h linux-2.5.31-lockassert/include/asm-i386/semaphore.h
--- linux-2.5.31/include/asm-i386/semaphore.h Sat Aug 10 18:41:24 2002
+++ linux-2.5.31-lockassert/include/asm-i386/semaphore.h Wed Aug 21 11:13:11 2002
@@ -40,6 +40,7 @@
#include <asm/atomic.h>
#include <linux/wait.h>
#include <linux/rwsem.h>
+#include <linux/config.h>

struct semaphore {
atomic_t count;
@@ -55,6 +56,12 @@
, (int)&(name).__magic
#else
# define __SEM_DEBUG_INIT(name)
+#endif
+
+#ifdef CONFIG_DEBUG_SPINLOCK
+# define assert_sem_held(sem) BUG_ON(!down_trylock(sem))
+#else
+# define assert_sem_held(sem) do { } while(0)
#endif

#define __SEMAPHORE_INITIALIZER(name,count) \
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.31/include/asm-i386/spinlock.h linux-2.5.31-lockassert/include/asm-i386/spinlock.h
--- linux-2.5.31/include/asm-i386/spinlock.h Sat Aug 10 18:41:23 2002
+++ linux-2.5.31-lockassert/include/asm-i386/spinlock.h Wed Aug 21 11:05:41 2002
@@ -157,6 +157,7 @@
#define RW_LOCK_UNLOCKED (rwlock_t) { RW_LOCK_BIAS RWLOCK_MAGIC_INIT }

#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+#define rwlock_is_locked(x) ((x)->lock != RW_LOCK_BIAS)

/*
* On x86, we implement read-write locks as a 32-bit counter
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.31/include/asm-ia64/semaphore.h linux-2.5.31-lockassert/include/asm-ia64/semaphore.h
--- linux-2.5.31/include/asm-ia64/semaphore.h Sat Aug 10 18:41:28 2002
+++ linux-2.5.31-lockassert/include/asm-ia64/semaphore.h Wed Aug 21 11:13:28 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 assert_sem_held(sem) BUG_ON(!down_trylock(sem))
+#else
+# define assert_sem_held(sem) do { } while(0)
#endif

#define __SEMAPHORE_INITIALIZER(name,count) \
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.31/include/asm-ia64/spinlock.h linux-2.5.31-lockassert/include/asm-ia64/spinlock.h
--- linux-2.5.31/include/asm-ia64/spinlock.h Sat Aug 10 18:41:19 2002
+++ linux-2.5.31-lockassert/include/asm-ia64/spinlock.h Wed Aug 21 11:05: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.31/include/linux/rwsem-spinlock.h linux-2.5.31-lockassert/include/linux/rwsem-spinlock.h
--- linux-2.5.31/include/linux/rwsem-spinlock.h Sat Aug 10 18:41:38 2002
+++ linux-2.5.31-lockassert/include/linux/rwsem-spinlock.h Wed Aug 21 11:12:43 2002
@@ -46,6 +46,14 @@
#define __RWSEM_DEBUG_INIT /* */
#endif

+#ifdef CONFIG_DEBUG_SPINLOCK
+#define assert_rwsem_held_for_write(rwsem) BUG_ON(__down_read_trylock(sem))
+#define assert_rwsem_held_for_read(rwsem) BUG_ON(__down_write_trylock(rwsem))
+#else
+#define assert_rwsem_held_for_write(rwsem) do { } while(0)
+#define assert_rwsem_held_for_read(rwsem) 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.31/include/linux/rwsem.h linux-2.5.31-lockassert/include/linux/rwsem.h
--- linux-2.5.31/include/linux/rwsem.h Sat Aug 10 18:41:25 2002
+++ linux-2.5.31-lockassert/include/linux/rwsem.h Wed Aug 21 11:05: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.31/include/linux/spinlock.h linux-2.5.31-lockassert/include/linux/spinlock.h
--- linux-2.5.31/include/linux/spinlock.h Sat Aug 10 18:41:29 2002
+++ linux-2.5.31-lockassert/include/linux/spinlock.h Wed Aug 21 11:19:35 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 held.
+ */
+#if defined(CONFIG_DEBUG_SPINLOCK) && defined(CONFIG_SMP)
+#define assert_locked(lock) BUG_ON(!spin_is_locked(lock))
+#define assert_rw_locked(lock) BUG_ON(!rwlock_is_locked(lock))
+#else
+#define assert_locked(lock) do { } while(0)
+#define assert_rw_locked(lock) do { } while(0)
+#endif /* CONFIG_DEBUG_SPINLOCK && CONFIG_SMP */

#ifdef CONFIG_PREEMPT

diff -Naur -X /home/jbarnes/dontdiff linux-2.5.31/kernel/acct.c linux-2.5.31-lockassert/kernel/acct.c
--- linux-2.5.31/kernel/acct.c Sat Aug 10 18:41:36 2002
+++ linux-2.5.31-lockassert/kernel/acct.c Wed Aug 21 11:15:03 2002
@@ -160,6 +160,8 @@
{
struct file *old_acct = NULL;

+ assert_locked(&acct_globals.lock);
+
if (acct_globals.file) {
old_acct = acct_globals.file;
del_timer(&acct_globals.timer);
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.31/kernel/printk.c linux-2.5.31-lockassert/kernel/printk.c
--- linux-2.5.31/kernel/printk.c Sat Aug 10 18:41:29 2002
+++ linux-2.5.31-lockassert/kernel/printk.c Wed Aug 21 11:15:18 2002
@@ -337,6 +337,8 @@
unsigned long cur_index, start_print;
static int msg_level = -1;

+ assert_sem_held(&console_sem);
+
if (((long)(start - end)) > 0)
BUG();

2002-08-21 18:38:09

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCH] lock assertion macros for 2.5.31

Jesse Barnes wrote:
>
> On Mon, Aug 12, 2002 at 04:12:41PM -0700, Andrew Morton wrote:
> > ...
> > #define might_sleep() BUG_ON(preempt_count())
> >
> > _this_ would catch numerous bugs, including code which is not buggy
> > in 2.4, but became buggy when wild-eyed loonies changed core kernel
> > rules without even looking at what drivers were doing (rant).
> >
> > I expect something like this will fall out of the wash soon, at
> > least for preemptible kernels.
>
> Is it really that simple?

It sure is:

/**
* in_atomic_region() - determine whether it is legal to perform a context
* switch
*
* The in_atomic_region() predicate returns true if the current task is
* executing atomically, and may not perform a context switch.
*
* If preemption is enabled, in_atomic_region() is most accurate, because it
* returns true if this task has taken any spinlocks.
*
* If preemption is disabled then there is no spinlocking record available, and
* we can only look at the interrupt state.
*
* If the task has taken a lock_kernel() then it is still legal to perform a
* context switch.
*/
#ifdef CONFIG_PREEMPT
#define in_atomic_region() (preempt_count() - !!(current->lock_depth + 1))
#else
#define in_atomic_region() in_interrupt()
#endif

/**
* may_sleep() - debugging check for possible illegal scheduling.
*
* may_sleep() is to be used in code paths which _may_ perform a context switch.
* It will force a BUG if the caller is executing in an atomic region.
*/
extern void __in_atomic_region(char *file, int line);
#define may_sleep() \
do { \
if (in_atomic_region()) \
__in_atomic_region(__FILE__, __LINE__); \
} while (0)

> Maybe it should go into sched.h sometime
> soon? I guess the real work is sprinkling it in all the places where
> it needs to go.

Well I added checks just to kmalloc, kmem_cache_alloc, __alloc_pages
and saw a shower of bloopers during bootup. Such as drivers/ide/probe.c:init_irq()
calling request_irq() inside ide_lock.

> Anyway, here's an updated version of the lock assertion patch.

Well I like it. It's unintrusive, imparts useful info to the reader
and checks stuff at runtime.

> Should
> it be split into two patches, one that implements the macros and
> another that puts checks everywhere?

I don't think it needs splitting. You have the core infrastructure plus
a couple of example applications.

> Should I add a small doc to
> Documentation/ (maybe the might_sleep() could be documented there
> too)?

These things are self-evident and even self-checking. They don't need
supporting documentation. I'll put out a test tree RSN, include this
in it.

2002-08-21 18:42:32