BKL removal
Subsystems (5)
Quoted 3 lines · Dave Hansen
I've created a patch that helps find bad uses of the big kernel lock.
It is explained in detail here:
http://marc.theaimsgroup.com/?l=linux-kernel&m=102535814329494&w=2
aw. you implemented the less useful check of the two you mentioned! i think the release-on-sleep property of the BKL is far more harmful than the recursive-holds. release-on-sleep means that you probably have a race there you didn't know about (eg the one i found in setfl earlier this week), and means you need to do some serious work in order to redesign your code to eliminate the race.
once you've done that redesign, chances are you'll understand the locking in that area well enough to know that there are no recursive holds in your code and you'll switch to a spinlock. this will automagically reduce the number of recursive-holds simply because fewer places will take the BKL.
as an aside, replacing the BKL by a non-recursive spinlock isn't always a great idea:
/*
* Nasty deadlock avoidance.
*
* ext2_new_block->getblk->GFP->shrink_dcache_memory->prune_dcache->
* prune_one_dentry->dput->dentry_iput->iput->inode->i_sb->s_op->
* put_inode->ext2_discard_prealloc->ext2_free_blocks->lock_super->
* DEADLOCK.
*
* We should make sure we don't hold the superblock lock over
* block allocations, but for now:
*/
if (!(gfp_mask & __GFP_FS))
return 0;
so we now decline to scavenge memory from the dcache or the icache if a filesystem causes the system to decide it's low on memory. this isn't a good thing.
btw, this comment is somewhat abbreviated. the sequence is actually:
ext2_new_block->load_block_bitmap->read_block_bitmap->sb_bread->__bread-> __getblk->grow_buffers->grow_dev_page->find_or_create_page->alloc_page->
...
it doesn't look _too_ hard to persuade ext2_new_block (btw, the problem also occurs with ext2_free_blocks) to unlock_super and retry, but that's slightly more dangerous work than i want to do right now ... particularly since some of these interfaces may change dramatically as a result of the aio work.
Signature · 1 line
Revolutions do not require corporate support.
Quoted 6 lines · Matthew Wilcox
aw. you implemented the less useful check of the two you mentioned! i think the release-on-sleep property of the BKL is far more harmful than the recursive-holds. release-on-sleep means that you probably have a race there you didn't know about (eg the one i found in setfl earlier this week), and means you need to do some serious work in order to redesign your code to eliminate the race.
Be careful what you ask for. These release-on-sleep calls happen quite often, maybe as often as a few a second and I can't think of any elegant ways of limiting the number of unique printk()s in schedule().
It was easy to limit the printing for a single un/lock_kernel() call
with static variables, but the scheduling ones are a bit more complicated. I can think of some convoluted ways of doing this, but no simple ones. If you apply this patch, be ready for a load of stuff in dmesg!
Quoted 4 lines
once you've done that redesign, chances are you'll understand the locking in that area well enough to know that there are no recursive holds in your code and you'll switch to a spinlock. this will automagically reduce the number of recursive-holds simply because fewer places will take the BKL.
Some of the times that I've either broken someone else's code or simply pointed it out to them, they've fixed it.
Quoted 2 lines
as an aside, replacing the BKL by a non-recursive spinlock isn't always a great idea:
...<snip> look at the original if you want to see the whole call sequence!
Quoted 5 lines
it doesn't look _too_ hard to persuade
ext2_new_block(btw, the problem also occurs withext2_free_blocks) tounlock_superand retry, but that's slightly more dangerous work than i want to do right now ... particularly since some of these interfaces may change dramatically as a result of the aio work.
That's icky. So, was this potential problem introduced during Al's BKL replacement in ext2? The real problem here is the faulting operation while holding a spinlock which requires the spinlock to resolve, right? I think that all you can do is release the lock, try to get some memory, and start the process over. This is the kind of thing that I hope my patch will help us find so that we don't all need to have a Viroesque understanding of VFS :)
BTW, the Brits don't have any big parties today, do they?
Signature · 2 lines
Dave Hansen [email protected]
Just in case your telepathic patch reception isn't working, here's the patch with added release-on-sleep fun.
Signature · 2 lines
Dave Hansen [email protected]
Makefile +1 -1
@@ -1,7 +1,7 @@ VERSION = 2 PATCHLEVEL = 5 SUBLEVEL = 24 -EXTRAVERSION = +EXTRAVERSION =-bkldebug12 # We are using a recursive build, so we need to do a little thinking # to get the ordering right.
arch/i386/Config.help +9 -0
@@ -932,6 +932,15 @@ best used in conjunction with the NMI watchdog so that spinlock deadlocks are also debuggable. +CONFIG_DEBUG_BKL + Say Y here to get interesting information about the Big Kernel + Lock's use in dmesg. + Shows information on the following: + - nested holds of BKL + - releases in schedule (not yet implemented) + - use in interrupts (not yet implemented) + Send any interesting output to Dave Hansen <[email protected]> + CONFIG_DEBUG_BUGVERBOSE Say Y here to make BUG() panics output the file name and line number of the BUG call as well as the EIP and oops trace. This aids
arch/i386/config.in +1 -0
@@ -416,6 +416,7 @@ bool ' Memory mapped I/O debugging' CONFIG_DEBUG_IOVIRT bool ' Magic SysRq key' CONFIG_MAGIC_SYSRQ bool ' Spinlock debugging' CONFIG_DEBUG_SPINLOCK + bool ' Big kernel lock (BKL,kernel_flag) debugging' CONFIG_DEBUG_BKL if [ "$CONFIG_HIGHMEM" = "y" ]; then bool ' Highmem debugging' CONFIG_DEBUG_HIGHMEM fi
include/asm-alpha/smplock.h +2 -2
@@ -39,13 +39,13 @@* so we only need to worry about other * CPU's. */ -static __inline__void lock_kernel (void ) +static __inline__void __lock_kernel (void ) {if (!++current->lock_depth)spin_lock (&kernel_flag); } -static __inline__void unlock_kernel (void ) +static __inline__void __unlock_kernel (void ) {if (--current->lock_depth <0 )spin_unlock (&kernel_flag);
include/asm-arm/smplock.h +2 -2
@@ -41,7 +41,7 @@* so we only need to worry about other * CPU's. */ -static inline void lock_kernel (void ) +static inline void __lock_kernel (void ) {#ifdef CONFIG_PREEMPT if (current->lock_depth ==-1 )
@@ -53,7 +53,7 @@#endif } -static inline void unlock_kernel (void ) +static inline void __unlock_kernel (void ) {if (--current->lock_depth <0 )spin_unlock (&kernel_flag);
include/asm-cris/smp_lock.h +2 -2
@@ -11,7 +11,7 @@* Locking the kernel */ -extern __inlinevoid lock_kernel (void ) +extern __inlinevoid __lock_kernel (void ) {unsigned long flags;int proc =smp_processor_id ();
@@ -49,7 +49,7 @@restore_flags (flags); } -extern __inlinevoid unlock_kernel (void ) +extern __inlinevoid __unlock_kernel (void ) {unsigned long flags;save_flags (flags);
include/asm-cris/smplock.h +2 -2
@@ -11,8 +11,8 @@#ifndef CONFIG_SMP -#define lock_kernel () do { } while(0) -#define unlock_kernel () do { } while(0) +#define __lock_kernel () do { } while(0) +#define __unlock_kernel () do { } while(0)#define release_kernel_lock (task, cpu, depth) ((depth) = 1)#define reacquire_kernel_lock (task, cpu, depth) do { } while(0)
include/asm-generic/smplock.h +2 -2
@@ -38,13 +38,13 @@* so we only need to worry about other * CPU's. */ -extern __inline__void lock_kernel (void ) +extern __inline__void __lock_kernel (void ) {if (!++current->lock_depth)spin_lock (&kernel_flag); } -extern __inline__void unlock_kernel (void ) +extern __inline__void __unlock_kernel (void ) {if (--current->lock_depth <0 )spin_unlock (&kernel_flag);
include/asm-i386/smplock.h +2 -2
@@ -54,7 +54,7 @@* so we only need to worry about other * CPU's. */ -static __inline__void lock_kernel (void ) +static __inline__void __lock_kernel (void ) {#ifdef CONFIG_PREEMPT if (current->lock_depth ==-1 )
@@ -76,7 +76,7 @@#endif } -static __inline__void unlock_kernel (void ) +static __inline__void __unlock_kernel (void ) {if (current->lock_depth <0 )BUG ();
include/asm-ia64/smplock.h +2 -2
@@ -51,14 +51,14 @@* CPU's. */ static __inline__void -lock_kernel (void ) +__lock_kernel (void ) {if (!++current->lock_depth)spin_lock (&kernel_flag); }static __inline__void -unlock_kernel (void ) +__unlock_kernel (void ) {if (--current->lock_depth <0 )spin_unlock (&kernel_flag);
include/asm-m68k/smplock.h +2 -2
@@ -38,13 +38,13 @@* so we only need to worry about other * CPU's. */ -extern __inline__void lock_kernel (void ) +extern __inline__void __lock_kernel (void ) {if (!++current->lock_depth)spin_lock (&kernel_flag); } -extern __inline__void unlock_kernel (void ) +extern __inline__void __unlock_kernel (void ) {if (--current->lock_depth <0 )spin_unlock (&kernel_flag);
include/asm-mips/smplock.h +2 -2
@@ -41,13 +41,13 @@* so we only need to worry about other * CPU's. */ -extern __inline__void lock_kernel (void ) +extern __inline__void __lock_kernel (void ) {if (!++current->lock_depth)spin_lock (&kernel_flag); } -extern __inline__void unlock_kernel (void ) +extern __inline__void __unlock_kernel (void ) {if (--current->lock_depth <0 )spin_unlock (&kernel_flag);
include/asm-mips64/smplock.h +2 -2
@@ -41,13 +41,13 @@* so we only need to worry about other * CPU's. */ -static __inline__void lock_kernel (void ) +static __inline__void __lock_kernel (void ) {if (!++current->lock_depth)spin_lock (&kernel_flag); } -static __inline__void unlock_kernel (void ) +static __inline__void __unlock_kernel (void ) {if (--current->lock_depth <0 )spin_unlock (&kernel_flag);
include/asm-parisc/smplock.h +2 -2
@@ -36,13 +36,13 @@* so we only need to worry about other * CPU's. */ -extern __inline__void lock_kernel (void ) +extern __inline__void __lock_kernel (void ) {if (!++current->lock_depth)spin_lock (&kernel_flag); } -extern __inline__void unlock_kernel (void ) +extern __inline__void __unlock_kernel (void ) {if (--current->lock_depth <0 )spin_unlock (&kernel_flag);
include/asm-ppc/smplock.h +2 -2
@@ -47,7 +47,7 @@* so we only need to worry about other * CPU's. */ -static __inline__void lock_kernel (void ) +static __inline__void __lock_kernel (void ) {#ifdef CONFIG_PREEMPT if (current->lock_depth ==-1 )
@@ -59,7 +59,7 @@#endif /* CONFIG_PREEMPT */ } -static __inline__void unlock_kernel (void ) +static __inline__void __unlock_kernel (void ) {if (--current->lock_depth <0 )spin_unlock (&kernel_flag);
include/asm-ppc64/smplock.h +2 -2
@@ -43,13 +43,13 @@* so we only need to worry about other * CPU's. */ -static __inline__void lock_kernel (void ) +static __inline__void __lock_kernel (void ) {if (!++current->lock_depth)spin_lock (&kernel_flag); } -static __inline__void unlock_kernel (void ) +static __inline__void __unlock_kernel (void ) {if (current->lock_depth <0 )BUG ();
include/asm-s390/smplock.h +2 -2
@@ -48,13 +48,13 @@* so we only need to worry about other * CPU's. */ -extern __inline__void lock_kernel (void ) +extern __inline__void __lock_kernel (void ) {if (!++current->lock_depth)spin_lock (&kernel_flag); } -extern __inline__void unlock_kernel (void ) +extern __inline__void __unlock_kernel (void ) {if (--current->lock_depth <0 )spin_unlock (&kernel_flag);
include/asm-s390x/smplock.h +2 -2
@@ -48,13 +48,13 @@* so we only need to worry about other * CPU's. */ -extern __inline__void lock_kernel (void ) +extern __inline__void __lock_kernel (void ) {if (!++current->lock_depth)spin_lock (&kernel_flag); } -extern __inline__void unlock_kernel (void ) +extern __inline__void __unlock_kernel (void ) {if (--current->lock_depth <0 )spin_unlock (&kernel_flag);
include/asm-sh/smplock.h +2 -2
@@ -11,8 +11,8 @@#ifndef CONFIG_SMP -#define lock_kernel () do { } while(0) -#define unlock_kernel () do { } while(0) +#define __lock_kernel () do { } while(0) +#define __unlock_kernel () do { } while(0)#define release_kernel_lock (task, cpu, depth) ((depth) = 1)#define reacquire_kernel_lock (task, cpu, depth) do { } while(0)
include/asm-sparc/smplock.h +2 -2
@@ -42,13 +42,13 @@* so we only need to worry about other * CPU's. */ -#define lock_kernel () \ +#define __lock_kernel () \ do { \ if (!++current->lock_depth) \ spin_lock(&kernel_flag); \ } while(0) -#define unlock_kernel () \ +#define __unlock_kernel () \ do { \ if (--current->lock_depth < 0) \ spin_unlock(&kernel_flag); \
include/asm-sparc64/smplock.h +2 -2
@@ -50,13 +50,13 @@* so we only need to worry about other * CPU's. */ -#define lock_kernel () \ +#define __lock_kernel () \ do { \ if (!++current->lock_depth) \ spin_lock(&kernel_flag); \ } while(0) -#define unlock_kernel () \ +#define __unlock_kernel () \ do { \ if (--current->lock_depth < 0) \ spin_unlock(&kernel_flag); \
include/asm-x86_64/smplock.h +2 -2
@@ -54,7 +54,7 @@* so we only need to worry about other * CPU's. */ -extern __inline__void lock_kernel (void ) +extern __inline__void __lock_kernel (void ) {#ifdef CONFIG_PREEMPT if (current->lock_depth ==-1 )
@@ -76,7 +76,7 @@#endif } -extern __inline__void unlock_kernel (void ) +extern __inline__void __unlock_kernel (void ) {if (current->lock_depth <0 )BUG ();
include/linux/sched.h +3 -0
@@ -28,6 +28,7 @@#include <linux/securebits.h> #include <linux/fs_struct.h> #include <linux/compiler.h> +#include <linux/spinlock.h> struct exec_domain ;
@@ -255,6 +256,8 @@unsigned long ptrace;int lock_depth;/* Lock depth */ +struct lock_trace lt[MAX_LOCK_TRACE_DEPTH ]; +int lt_dirty;/* don't print redundant stuff */ int prio, static_prio;list_t run_list;
include/linux/smp_lock.h +2 -2
@@ -5,8 +5,8 @@#if !defined(CONFIG_SMP ) && !defined(CONFIG_PREEMPT ) -#define lock_kernel () do { } while(0) -#define unlock_kernel () do { } while(0) +#define __lock_kernel () do { } while(0) +#define __unlock_kernel () do { } while(0)#define release_kernel_lock (task, cpu) do { } while(0)#define reacquire_kernel_lock (task) do { } while(0)#define kernel_locked () 1
include/linux/spinlock.h +99 -0
105 lines not on this page · raw message
kernel/sched.c +3 -0
@@ -815,6 +815,9 @@ prev = current; rq =this_rq (); +if (kernel_locked () ) { +print_bkl_trace ("BKL held during sleep" ); + }release_kernel_lock (prev,smp_processor_id ());prepare_arch_schedule (prev); prev->sleep_timestamp = jiffies;