lock assertion macros for 2.5.30
Subsystems (4)
PATCH 1/1 lock assertion macros for 2.5.30
Here's the latest revision of the lock assertion macros patch. I've converted over the ASSERT_LOCK stuff in the scsi layer and added a few other MUST_HOLD_* assertions elsewhere.
Joshua, do you think this will work for the reiserfs stuff you were talking about? If not, let me know what else I should add.
Please check it out and let me know if you think it's ready for 2.5 inclusion, or maybe it should be part of your spinlock.h cleanup, Robert?
Thanks,
Jesse
drivers/scsi/aha152x.c +1 -1
@@ -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 ;
drivers/scsi/scsi.c +6 -6
@@ -262,7 +262,7 @@struct request_queue *q = &SCpnt->device->request_queue;unsigned long flags; -ASSERT_LOCK (q->queue_lock,0 ); +MUST_NOT_HOLD (q->queue_lock); req->rq_status =RQ_SCSI_DONE ;/* Busy, but indicate request done */ spin_lock_irqsave (q->queue_lock, flags);
@@ -669,7 +669,7 @@ host = SCpnt->host; -ASSERT_LOCK (host->host_lock,0 ); +MUST_NOT_HOLD (host->host_lock);/* Assign a unique nonzero serial_number. */ if (++serial_number ==0 )
@@ -827,7 +827,7 @@Scsi_Device * SDpnt = SRpnt->sr_device;struct Scsi_Host *host = SDpnt->host; -ASSERT_LOCK (host->host_lock,0 ); +MUST_NOT_HOLD (host->host_lock); SCSI_LOG_MLQUEUE(4, {
@@ -924,7 +924,7 @@ {struct Scsi_Host *host = SCpnt->host; -ASSERT_LOCK (host->host_lock,0 ); +MUST_NOT_HOLD (host->host_lock); SCpnt->owner =SCSI_OWNER_MIDLEVEL ; SRpnt->sr_command = SCpnt;
@@ -1013,7 +1013,7 @@ {struct Scsi_Host *host = SCpnt->host; -ASSERT_LOCK (host->host_lock,0 ); +MUST_NOT_HOLD (host->host_lock); SCpnt->pid = scsi_pid++; SCpnt->owner =SCSI_OWNER_MIDLEVEL ;
@@ -1339,7 +1339,7 @@ host = SCpnt->host; device = SCpnt->device; -ASSERT_LOCK (host->host_lock,0 ); +MUST_NOT_HOLD (host->host_lock);/* * We need to protect the decrement, as otherwise a race condition
drivers/scsi/scsi.h +0 -15
@@ -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:
drivers/scsi/scsi_error.c +3 -3
@@ -581,7 +581,7 @@unsigned long flags;struct Scsi_Host *host = SCpnt->host; -ASSERT_LOCK (host->host_lock,0 ); +MUST_NOT_HOLD (host->host_lock); retry:/*
@@ -1251,7 +1251,7 @@Scsi_Device *SDpnt;unsigned long flags; -ASSERT_LOCK (host->host_lock,0 ); +MUST_NOT_HOLD (host->host_lock);/* * Next free up anything directly waiting upon the host. This will be
@@ -1328,7 +1328,7 @@Scsi_Cmnd *SCdone;int timed_out; -ASSERT_LOCK (host->host_lock,0 ); +MUST_NOT_HOLD (host->host_lock); SCdone =NULL ;
drivers/scsi/scsi_lib.c +5 -5
@@ -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 ) {
@@ -315,7 +315,7 @@struct request *req = SCpnt->request;unsigned long flags; -ASSERT_LOCK (q->queue_lock,0 ); +MUST_NOT_HOLD (q->queue_lock);spin_lock_irqsave (q->queue_lock, flags);/*
@@ -402,7 +402,7 @@ {struct request *req = SCpnt->request; -ASSERT_LOCK (SCpnt->host->host_lock,0 ); +MUST_NOT_HOLD (SCpnt->host->host_lock);/* * Free up any indirection buffers we allocated for DMA purposes.
@@ -465,7 +465,7 @@* would be used if we just wanted to retry, for example. * */ -ASSERT_LOCK (q->queue_lock,0 ); +MUST_NOT_HOLD (q->queue_lock);/* * Free up any indirection buffers we allocated for DMA purposes.
@@ -733,7 +733,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) {
fs/inode.c +2 -0
@@ -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 ;
include/asm-i386/semaphore.h +7 -0
@@ -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) \
include/asm-i386/spinlock.h +1 -0
@@ -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
include/asm-ia64/semaphore.h +7 -0
@@ -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
@@ -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
@@ -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
@@ -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 +15 -1
@@ -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
kernel/acct.c +2 -0
@@ -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);
kernel/printk.c +2 -0
@@ -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 ();
Quoted 4 lines · Jesse Barnes
+++
linux-2.5.30-lockassert/drivers/scsi/scsi.cWed Aug 7 11:35:32 2002(quoted diff) +1
-0@@ -262,7 +262,7 @@ + MUST_NOT_HOLD(q->queue_lock);
...
include/linux/spinlock.h line 128
Quoted 3 lines
include/linux/spinlock.h +3
-0+#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
Signature · 3 lines
Bravely reimplemented by the knights who say "NIH". http://www.surriel.com/ http://distro.conectiva.com/
Quoted 15 lines · Rik van Riel
Quoted 4 lines · Jesse Barnes
+++
linux-2.5.30-lockassert/drivers/scsi/scsi.cWed Aug 7 11:35:32 2002(quoted diff) +1
-0@@ -262,7 +262,7 @@ + MUST_NOT_HOLD(q->queue_lock);...
include/linux/spinlock.h line 128
Quoted 3 lines
include/linux/spinlock.h +3
-0+#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_HOLDthing is a joke.
Quoted 4 lines
What is to prevent another CPU in another code path from holding this spinlock when the code you've inserted the
MUST_NOT_HOLDin 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
Quoted 8 lines · Jesse Barnes
Quoted 4 lines
include/linux/spinlock.h line 128
Quoted 2 lines
include/linux/spinlock.h +2
-0+#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_HOLDthing 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.
Quoted 12 lines
drivers/scsi/scsi_lib.c Wed Aug 7 11:34:39 2002 renamed from drivers/scsi/scsi_lib.c Thu Aug 1 14:16:26 2002 +1
-1@@ -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) {
Quoted 2 lines
After I posted the last patch, a few people asked for
MUST_NOT_HOLDso 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
Signature · 3 lines
Bravely reimplemented by the knights who say "NIH". http://www.surriel.com/ http://distro.conectiva.com/
Quoted 5 lines · Rik van Riel
Quoted 2 lines · Jesse Barnes
macro worked before? I could just remove all those checks in the scsi code I guess.
That would be a better option.
Ok.
Quoted 2 lines
The
MUST_NOT_HOLDbasically 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.
Quoted 3 lines
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
Quoted 5 lines · Jesse Barnes
Quoted 2 lines
The
MUST_NOT_HOLDbasically 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).
Quoted 6 lines
Quoted 3 lines
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
Signature · 3 lines
Bravely reimplemented by the knights who say "NIH". http://www.surriel.com/ http://distro.conectiva.com/
Uz.ytkownik Jesse Barnes napisa?:
Quoted 15 lines · Rik van Riel
Quoted 7 lines · Jesse Barnes
Quoted 3 lines
macro worked before? I could just remove all those checks in the scsi code I guess.
That would be a better option.
Ok.
Quoted 2 lines
The
MUST_NOT_HOLDbasically 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.
Quoted 4 lines · Marcin Dalecki
And for Gods sake ;-) - please call it
ASSERT_and notMUST_THISMUST_THATMUST_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?
Signature · 1 line
Daniel
Quoted 5 lines · Rik van Riel
Quoted 2 lines · Jesse Barnes
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
drivers/scsi/aha152x.c +1 -1
@@ -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 ;
drivers/scsi/scsi.c +0 -11
@@ -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
drivers/scsi/scsi.h +0 -15
@@ -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:
drivers/scsi/scsi_error.c +0 -6
@@ -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 ;
drivers/scsi/scsi_lib.c +1 -8
@@ -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) {
fs/inode.c +2 -0
@@ -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 ;
include/asm-i386/semaphore.h +7 -0
@@ -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) \
include/asm-i386/spinlock.h +1 -0
@@ -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
include/asm-ia64/semaphore.h +7 -0
@@ -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
@@ -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
@@ -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
@@ -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 +15 -1
@@ -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
kernel/acct.c +2 -0
@@ -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);
kernel/printk.c +2 -0
@@ -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 ();
Quoted 8 lines · Jesse Barnes
Quoted 5 lines · Rik van Riel
Quoted 2 lines · Jesse Barnes
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
Signature · 3 lines
Bravely reimplemented by the knights who say "NIH". http://www.surriel.com/ http://distro.conectiva.com/
Quoted 1 line · Rik van Riel
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
fs/inode.c +2 -0
@@ -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 ;
include/asm-i386/semaphore.h +7 -0
@@ -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) \
include/asm-i386/spinlock.h +1 -0
@@ -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
include/asm-ia64/semaphore.h +7 -0
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
kernel/acct.c +2 -0
@@ -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);
kernel/printk.c +2 -0
@@ -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 ();
Quoted 4 lines · Jesse Barnes
Quoted 1 line · Rik van Riel
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
Signature · 5 lines
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/
Quoted 2 lines · Rik van Riel
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?).
Quoted 2 lines
The other issues you raised are probably best done in separate patches.
Ok, thanks a lot for the feedback.
Jesse
Quoted 7 lines · Jesse Barnes
Quoted 2 lines · Rik van Riel
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
_frommypatch_. 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.
Signature · 1 line
Daniel
PATCH 1/1 lock assertion macros for 2.5.31
Quoted 1 line · Daniel Phillips
Or Andrew Morton, or both.
Ok, here it is against 2.5.31. Please apply.
Thanks,
Jesse
fs/inode.c +2 -0
@@ -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 ;
include/asm-i386/semaphore.h +7 -0
@@ -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) \
include/asm-i386/spinlock.h +1 -0
@@ -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
include/asm-ia64/semaphore.h +7 -0
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
kernel/acct.c +2 -0
@@ -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);
kernel/printk.c +2 -0
@@ -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 ();
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH] lock assertion macros for 2.5.31
Quoted 9 lines · Andrew Morton
...
#definemight_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
fs/inode.c +2 -0
@@ -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 ;
include/asm-i386/semaphore.h +7 -0
@@ -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) \
include/asm-i386/spinlock.h +1 -0
@@ -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
include/asm-ia64/semaphore.h +7 -0
@@ -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) \
include/asm-ia64/spinlock.h +1 -0
@@ -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 +8 -0
@@ -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 }
include/linux/rwsem.h +1 -0
@@ -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
@@ -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
kernel/acct.c +2 -0
@@ -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);
kernel/printk.c +2 -0
@@ -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 ();
Quoted 11 lines · Jesse Barnes
Quoted 1 line · Rik van Riel
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_LOCKmacros as well as the silly version ofMUST_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
Quoted 1 line
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.
Quoted 3 lines
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.
Signature · 1 line
Daniel
Quoted 4 lines · 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
Re: [PATCH] lock assertion macros for 2.5.31
In reply to Jesse BarnesQuoted 5 lines · Jesse Barnes
Quoted 3 lines · Andrew Morton
...
#definemight_sleep()BUG_ON(preempt_count())
Quoted 8 lines
Quoted 6 lines
_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)Quoted 3 lines
Maybe it should go into
sched.hsometime
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.
Quoted 1 line
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.
Quoted 3 lines
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.
Quoted 3 lines
Should I add a small doc to
Documentation/(maybe themight_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.
Re: [PATCH] lock assertion macros for 2.5.31
In reply to Jesse BarnesQuoted 3 lines · Andrew Morton
Well I added checks just to kmalloc,
kmem_cache_alloc,__alloc_pagesand saw a shower of bloopers during bootup. Such as drivers/ide/probe.c:init_irq() callingrequest_irq()insideide_lock.
Wow. Sounds like some good code to have around.
Quoted 4 lines
Quoted 1 line
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.
Great!
Quoted 3 lines
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.
Excellent. Thanks a lot for your feedback.
Jesse
A couple of whitespace glitches:
include/linux/spinlock.h line 134
Quoted 9 lines
include/linux/spinlock.h +9
-0+#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.
Signature · 1 line
Daniel
Hi,
Quoted 5 lines · Daniel Phillips
Quoted 1 line
+#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
Hi,
Quoted 5 lines · Daniel Phillips
Random gripe: don't all those do { } whiles look silly? We need
#define NADA do { } while (0)or similar.
Call it nop.
Thunder
Signature · 1 line
.-../../-./..-/-..- .-./..-/.-.././.../.-.-.-
Quoted 16 lines · Jesse Barnes
Quoted 15 lines · Rik van Riel
Quoted 4 lines · Jesse Barnes
+++
linux-2.5.30-lockassert/drivers/scsi/scsi.cWed Aug 7 11:35:32 2002(quoted diff) +1
-0@@ -262,7 +262,7 @@ + MUST_NOT_HOLD(q->queue_lock);...
include/linux/spinlock.h line 128
Quoted 3 lines
include/linux/spinlock.h +3
-0+#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_HOLDthing is a joke.
Quoted 8 lines
Quoted 4 lines
What is to prevent another CPU in another code path from holding this spinlock when the code you've inserted the
MUST_NOT_HOLDin 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.
Quoted 4 lines
After I posted the last patch, a few people asked for
MUST_NOT_HOLDso 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?
Signature · 1 line
"Love the dolphins," she advised him. "Write by W.A.S.T.E.."
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
Quoted 67 lines · Oliver Xymoron
Quoted 16 lines · Jesse Barnes
Quoted 15 lines · Rik van Riel
Quoted 4 lines · Jesse Barnes
+++
linux-2.5.30-lockassert/drivers/scsi/scsi.cWed Aug 7 11:35:32 2002(quoted diff) +1
-0@@ -262,7 +262,7 @@ + MUST_NOT_HOLD(q->queue_lock);...
include/linux/spinlock.h line 128
Quoted 3 lines
include/linux/spinlock.h +3
-0+#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_HOLDthing is a joke.Quoted 8 lines
Quoted 4 lines
What is to prevent another CPU in another code path from holding this spinlock when the code you've inserted the
MUST_NOT_HOLDin 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.
Quoted 4 lines
After I posted the last patch, a few people asked for
MUST_NOT_HOLDso 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=2Interesting. I'm still going to say that
MUST_NOT_HOLDis wrong, at least in its current form/name.What
MUST_HOLDis saying is "the current thread is holding this lock, go ahead, double check if you want". WhatMUST_NOT_HOLDsays 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_LOCKto 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?Signature · 8 lines
"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/
Quoted 2 lines · Joshua MacDonald
In order to implement
MOST_NOT_HOLD_LOCKthe spinlock must contain some record of who holds the lock, and since the SCSI-layer apparently
Correct
Quoted 2 lines
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!
Signature · 1 line
Jens Axboe
Quoted 16 lines · Jesse Barnes
Quoted 15 lines · Rik van Riel
Quoted 4 lines · Jesse Barnes
+++
linux-2.5.30-lockassert/drivers/scsi/scsi.cWed Aug 7 11:35:32 2002(quoted diff) +1
-0@@ -262,7 +262,7 @@ + MUST_NOT_HOLD(q->queue_lock);...
include/linux/spinlock.h line 128
Quoted 3 lines
include/linux/spinlock.h +3
-0+#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_HOLDthing is a joke.
Quoted 8 lines
Quoted 4 lines
What is to prevent another CPU in another code path from holding this spinlock when the code you've inserted the
MUST_NOT_HOLDin 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.
Quoted 2 lines
After I posted the last patch, a few people asked for
MUST_NOT_HOLDso I added it back in. Do you think it's a bad idea? See the last
Your current version is surely worthless.
Signature · 1 line
Jens Axboe
Quoted 6 lines · Jens Axboe
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_HOLDto work, you need to take into account which processor took the lock etc.
That's the only way it seems to be useful...
Quoted 4 lines
Quoted 2 lines
After I posted the last patch, a few people asked for
MUST_NOT_HOLDso I added it back in. Do you think it's a bad idea? See the lastYour 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
Quoted 4 lines · Jesse Barnes
Quoted 2 lines · Jens Axboe
For
MUST_NOT_HOLDto work, you need to take into account which processor took the lock etc.
[snip]
Quoted 3 lines
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
Signature · 5 lines
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/
Quoted 7 lines · Rik van Riel
Quoted 3 lines
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
Quoted 8 lines · Rik van Riel
Quoted 4 lines · Jesse Barnes
Quoted 2 lines · Jens Axboe
For
MUST_NOT_HOLDto work, you need to take into account which processor took the lock etc.[snip]
Quoted 8 lines
Quoted 3 lines
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
Quoted 7 lines · Joshua MacDonald
Quoted 6 lines · Rik van Riel
Quoted 3 lines · Jesse Barnes
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.
Quoted 7 lines
Quoted 3 lines
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
Signature · 5 lines
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/