might_sleep() improvements
Subsystems (4)
PATCH 1/1 might_sleep() improvements
Andrew - I thought this might be appropriate for -mm kernels.
This patch makes the following improvements to might_sleep():
o Add a "might_sleep_if()" macro for when we might sleep only if some condition is met. I think this is a bit better than the currently used "if (cond) might_sleep();" since it's clearer that the test won't be compiled in if spinlock sleep debugging is turned off. (Obviously gcc is smart enough to omit simple conditions in that case) It also looks cleaner, IMO. Think of it as analogous to BUG()/BUG_ON().
o Add might_sleep checks to skb_share_check() and skb_unshare() which sometimes need to allocate memory.
o Make all architectures call might_sleep() in both down() and down_interruptible(). Before only ppc, ppc64, and i386 did this check. (sh did the check on down() but not down_interruptible())
Patch is versus 2.6.0-test4, hopefully it still applies cleanly.
-Mitch
arch/sparc64/kernel/semaphore.c +2 -0
@@ -110,6 +110,7 @@void down (struct semaphore *sem) { +might_sleep ();/* This atomically does: * old_val = sem->count; * new_val = sem->count - 1;
@@ -219,6 +220,7 @@ {int ret =0 ; +might_sleep ();/* This atomically does: * old_val = sem->count; * new_val = sem->count - 1;
include/asm-alpha/semaphore.h +6 -2
@@ -88,14 +88,18 @@static inline void __down (struct semaphore *sem) { -long count =atomic_dec_return (&sem->count); +long count; +might_sleep (); + count =atomic_dec_return (&sem->count);if (unlikely (count <0 ))__down_failed (sem); }static inline int __down_interruptible (struct semaphore *sem) { -long count =atomic_dec_return (&sem->count); +long count; +might_sleep (); + count =atomic_dec_return (&sem->count);if (unlikely (count <0 ))return __down_failed_interruptible (sem);return 0 ;
include/asm-arm/semaphore.h +2 -2
@@ -88,7 +88,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif - +might_sleep ();__down_op (sem, __down_failed); }
@@ -101,7 +101,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif - +might_sleep ();return __down_op_ret (sem, __down_interruptible_failed); }
include/asm-arm26/semaphore.h +2 -2
@@ -84,7 +84,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif - +might_sleep ();__down_op (sem, __down_failed); }
@@ -97,7 +97,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif - +might_sleep ();return __down_op_ret (sem, __down_interruptible_failed); }
include/asm-cris/semaphore.h +2 -0
@@ -79,6 +79,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif +might_sleep ();/* atomically decrement the semaphores count, and if its negative, we wait */ local_save_flags (flags);
@@ -104,6 +105,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif +might_sleep ();/* atomically decrement the semaphores count, and if its negative, we wait */ local_save_flags (flags);
include/asm-h8300/semaphore.h +2 -0
@@ -90,6 +90,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif +might_sleep (); count = &(sem->count); __asm__ __volatile__(
@@ -117,6 +118,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif +might_sleep (); count = &(sem->count); __asm__ __volatile__(
include/asm-ia64/semaphore.h +2 -0
@@ -73,6 +73,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif +might_sleep ();if (atomic_dec_return (&sem->count) <0 )__down (sem); }
@@ -89,6 +90,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif +might_sleep ();if (atomic_dec_return (&sem->count) <0 ) ret =__down_interruptible (sem);return ret;
include/asm-m68k/semaphore.h +2 -2
@@ -89,7 +89,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif - +might_sleep (); __asm__ __volatile__("| atomic down operation\n\t" "subql #1,%0@\n\t"
@@ -112,7 +112,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif - +might_sleep (); __asm__ __volatile__("| atomic interruptible down operation\n\t" "subql #1,%1@\n\t"
include/asm-m68knommu/semaphore.h +2 -2
@@ -88,7 +88,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif - +might_sleep (); __asm__ __volatile__("| atomic down operation\n\t" "movel %0, %%a1\n\t"
@@ -108,7 +108,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif - +might_sleep (); __asm__ __volatile__("| atomic down operation\n\t" "movel %1, %%a1\n\t"
include/asm-mips/semaphore.h +2 -0
@@ -88,6 +88,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif +might_sleep ();if (atomic_dec_return (&sem->count) <0 )__down (sem); }
@@ -103,6 +104,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif +might_sleep ();if (atomic_dec_return (&sem->count) <0 ) ret =__down_interruptible (sem);return ret;
include/asm-parisc/semaphore.h +2 -2
@@ -84,7 +84,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif - +might_sleep ();spin_lock_irq (&sem->sentry);if (sem->count >0 ) { sem->count--;
@@ -100,7 +100,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif - +might_sleep ();spin_lock_irq (&sem->sentry);if (sem->count >0 ) { sem->count--;
include/asm-s390/semaphore.h +2 -0
@@ -60,6 +60,7 @@static inline void down (struct semaphore * sem) { +might_sleep ();if (atomic_dec_return (&sem->count) <0 )__down (sem); }
@@ -68,6 +69,7 @@ {int ret =0 ; +might_sleep ();if (atomic_dec_return (&sem->count) <0 ) ret =__down_interruptible (sem);return ret;
include/asm-sh/semaphore.h +1 -0
@@ -107,6 +107,7 @@CHECK_MAGIC (sem->__magic);#endif +might_sleep ();if (atomic_dec_return (&sem->count) <0 ) ret =__down_interruptible (sem);return ret;
include/asm-sparc/semaphore.h +2 -0
@@ -71,6 +71,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif +might_sleep (); ptr = &(sem->count.counter); increment =1 ;
@@ -107,6 +108,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif +might_sleep (); ptr = &(sem->count.counter); increment =1 ;
include/asm-v850/semaphore.h +2 -0
@@ -57,6 +57,7 @@extern inline void down (struct semaphore * sem) { +might_sleep ();if (atomic_dec_return (&sem->count) <0 )__down (sem); }
@@ -64,6 +65,7 @@extern inline int down_interruptible (struct semaphore * sem) {int ret =0 ; +might_sleep ();if (atomic_dec_return (&sem->count) <0 ) ret =__down_interruptible (sem);return ret;
include/asm-x86_64/semaphore.h +2 -0
@@ -118,6 +118,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif +might_sleep (); __asm__ __volatile__( "# atomic down operation\n\t"
@@ -144,6 +145,7 @@#if WAITQUEUE_DEBUG CHECK_MAGIC (sem->__magic);#endif +might_sleep (); __asm__ __volatile__( "# atomic interruptible down operation\n\t"
include/linux/kernel.h +2 -0
@@ -52,8 +52,10 @@#ifdef CONFIG_DEBUG_SPINLOCK_SLEEP void __might_sleep (char *file,int line);#define might_sleep () __might_sleep(__FILE__, __LINE__) +#define might_sleep_if (cond) do { if (cond) might_sleep(); } while (0)#else #define might_sleep () do {} while(0) +#define might_sleep_if (cond) do {} while (0)#endif extern struct notifier_block *panic_notifier_list;
include/linux/skbuff.h +2 -0
@@ -389,6 +389,7 @@*/ static inline struct sk_buff *skb_share_check (struct sk_buff *skb,int pri) { +might_sleep_if (pri !=GFP_ATOMIC );if (skb_shared (skb)) {struct sk_buff *nskb =skb_clone (skb, pri);kfree_skb (skb);
@@ -419,6 +420,7 @@*/ static inline struct sk_buff *skb_unshare (struct sk_buff *skb,int pri) { +might_sleep_if (pri !=GFP_ATOMIC );if (skb_cloned (skb)) {struct sk_buff *nskb =skb_copy (skb, pri);kfree_skb (skb);/* Free our shared copy */
mm/page_alloc.c +1 -2
@@ -542,8 +542,7 @@int do_retry;struct reclaim_state reclaim_state; -if (wait) -might_sleep (); +might_sleep_if (wait); cold =0 ;if (gfp_mask & __GFP_COLD)
mm/rmap.c +1 -2
@@ -503,8 +503,7 @@struct pte_chain *ret;struct pte_chain **pte_chainp; -if (gfp_flags & __GFP_WAIT) -might_sleep (); +might_sleep_if (gfp_flags & __GFP_WAIT); pte_chainp = &get_cpu_var (local_pte_chain);if (*pte_chainp) {
mm/slab.c +1 -2
@@ -1813,8 +1813,7 @@static inline void cache_alloc_debugcheck_before (kmem_cache_t *cachep,int flags) { -if (flags & __GFP_WAIT) -might_sleep (); +might_sleep_if (flags & __GFP_WAIT);#if DEBUG kmem_flagcheck (cachep, flags);#endif
Quoted 11 lines · Mitchell Blank Jr
Andrew - I thought this might be appropriate for -mm kernels.
This patch makes the following improvements to
might_sleep():o Add a "
might_sleep_if()" macro for when we might sleep only if some condition is met. I think this is a bit better than the currently used "if (cond)might_sleep();" since it's clearer that the test won't be compiled in if spinlock sleep debugging is turned off. (Obviously gcc is smart enough to omit simple conditions in that case) It also looks cleaner, IMO. Think of it as analogous to BUG()/BUG_ON().
I think these should be pushed down to where the sleeping actually happens if possible.
Quoted 12 lines · Nick Piggin
Quoted 11 lines
Andrew - I thought this might be appropriate for -mm kernels.
This patch makes the following improvements to
might_sleep():o Add a "
might_sleep_if()" macro for when we might sleep only if some condition is met. I think this is a bit better than the currently used "if (cond)might_sleep();" since it's clearer that the test won't be compiled in if spinlock sleep debugging is turned off. (Obviously gcc is smart enough to omit simple conditions in that case) It also looks cleaner, IMO. Think of it as analogous to BUG()/BUG_ON().
Quoted 2 lines
I think these should be pushed down to where the sleeping actually happens if possible.
No, you want to generate the warning as early as possible in case the sleeping case happens very infrequently. For instance:
newskb = skb_unshare(skb, GFP_KERNEL);
might not even need to do any allocation (much less a sleep) in 99.9% of cases, but it's still a bug if it's called in atomic context and we want spinlock sleep debugging to catch that for us.
-Mitch
Quoted 18 lines · Mitchell Blank Jr
Quoted 12 lines · Nick Piggin
Quoted 12 lines
Andrew - I thought this might be appropriate for -mm kernels.
This patch makes the following improvements to
might_sleep():o Add a "
might_sleep_if()" macro for when we might sleep only if some condition is met. I think this is a bit better than the currently used "if (cond)might_sleep();" since it's clearer that the test won't be compiled in if spinlock sleep debugging is turned off. (Obviously gcc is smart enough to omit simple conditions in that case) It also looks cleaner, IMO. Think of it as analogous to BUG()/BUG_ON().Quoted 3 lines
I think these should be pushed down to where the sleeping actually happens if possible.
Quoted 10 lines
No, you want to generate the warning as early as possible in case the sleeping case happens very infrequently. For instance:
newskb = skb_unshare(skb, GFP_KERNEL);might not even need to do any allocation (much less a sleep) in 99.9% of cases, but it's still a bug if it's called in atomic context and we want spinlock sleep debugging to catch that for us.
Yeah well in this case I guess skb_unshare is as low as might_sleep can be pushed. I guess I don't have a problem with might_sleep_if as long as its used nicely.
Quoted 3 lines · Nick Piggin
I think these should be pushed down to where the sleeping actually happens if possible.
No, that ends up doing the wrong thing for most of the really common cases.
In particular, most of the memory allocation functions very seldom actually sleep. After all, they'll find plenty of free memory (or easily freeable memory) without having to wait for any pageouts or anything like that.
Yet the bug is there - the call _could_ have slept.
So "might_sleep()" really does what the name suggests: it is used to say that a particular case _may_ sleep, even if it ends up being unlikely.
Because what we're after is not a bug actually happening, but a latent bug that has been hidden by the fact that it happens so rarely in practice.
This is why "might_sleep()" should happen as early as possible, and not get pushed down.
Linus
Quoted 14 lines · Linus Torvalds
Quoted 3 lines · Nick Piggin
I think these should be pushed down to where the sleeping actually happens if possible.
No, that ends up doing the wrong thing for most of the really common cases.
In particular, most of the memory allocation functions very seldom actually sleep. After all, they'll find plenty of free memory (or easily freeable memory) without having to wait for any pageouts or anything like that.
Yet the bug is there - the call _could_ have slept.
Quoted 10 lines
So "
might_sleep()" really does what the name suggests: it is used to say that a particular case _may_ sleep, even if it ends up being unlikely.Because what we're after is not a bug actually happening, but a latent bug that has been hidden by the fact that it happens so rarely in practice.
This is why "
might_sleep()" should happen as early as possible, and not get pushed down.
Yes I see. I agree. I thought some could be pushed down further without losing info. I was mainly worried about adding the might_sleep_if function.
Quoted 3 lines · Mitchell Blank Jr
Andrew - I thought this might be appropriate for -mm kernels.
This patch makes the following improvements to
might_sleep():
Good. These checks are very useful.
Quoted 2 lines
o Add a "
might_sleep_if()" macro for when we might sleep only if some condition is met.
But I am neutral about this. One thing that BUG_ON() gives is that the if has an unlikely() in it, so it at least guarantees some improved semantics.
Another bit is that we have historically avoided conditional code (i.e., cruft like smp_if() etc. that you see elsewhere) like this. Maybe renaming this "might_sleep_on()" at least brings it more in line with BUG_ON(), and avoids looking like the gross constructs I fear.
Quoted 2 lines
o Add
might_sleepchecks toskb_share_check()andskb_unshare()which sometimes need to allocate memory.
Great.
Quoted 3 lines
o Make all architectures call
might_sleep()in bothdown()anddown_interruptible(). Before only ppc, ppc64, and i386 did this check. (sh did the check ondown()but notdown_interruptible())
Even better.
Robert Love
-
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 3 lines · Mitchell Blank Jr
Andrew - I thought this might be appropriate for -mm kernels.
This patch makes the following improvements to
might_sleep():
Good. These checks are very useful.
Quoted 2 lines
o Add a "
might_sleep_if()" macro for when we might sleep only if some condition is met.
But I am neutral about this. One thing that BUG_ON() gives is that the if has an unlikely() in it, so it at least guarantees some improved semantics.
Another bit is that we have historically avoided conditional code (i.e., cruft like smp_if() etc. that you see elsewhere) like this. Maybe renaming this "might_sleep_on()" at least brings it more in line with BUG_ON(), and avoids looking like the gross constructs I fear.
Quoted 2 lines
o Add
might_sleepchecks toskb_share_check()andskb_unshare()which sometimes need to allocate memory.
Great.
Quoted 3 lines
o Make all architectures call
might_sleep()in bothdown()anddown_interruptible(). Before only ppc, ppc64, and i386 did this check. (sh did the check ondown()but notdown_interruptible())
Even better.
Robert Love
Quoted 4 lines · Robert Love
Quoted 2 lines
o Add a "
might_sleep_if()" macro for when we might sleep only if some condition is met.But I am neutral about this.
That's understandable - I have some of the same reservations myself. In the end I think it's a slight win though.
Quoted 3 lines
Maybe
renaming this "might_sleep_on()" at least brings it more in line withBUG_ON(), and avoids looking like the gross constructs I fear.
I named it that at first but I was afraid that someone might get confused and thing the semantics were "might_sleep_on(&waitqueue)" since 'sleeping on' means something already. To me might_sleep_if(cond) looked a lot clearer at first glance.
-Mitch
-
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 4 lines · Robert Love
Quoted 2 lines
o Add a "
might_sleep_if()" macro for when we might sleep only if some condition is met.But I am neutral about this.
That's understandable - I have some of the same reservations myself. In the end I think it's a slight win though.
Quoted 3 lines
Maybe
renaming this "might_sleep_on()" at least brings it more in line withBUG_ON(), and avoids looking like the gross constructs I fear.
I named it that at first but I was afraid that someone might get confused and thing the semantics were "might_sleep_on(&waitqueue)" since 'sleeping on' means something already. To me might_sleep_if(cond) looked a lot clearer at first glance.
-Mitch
Quoted 9 lines · Mitchell Blank Jr
Quoted 4 lines · Robert Love
Quoted 2 lines
o Add a "
might_sleep_if()" macro for when we might sleep only if some condition is met.But I am neutral about this.
That's understandable - I have some of the same reservations myself. In the end I think it's a slight win though.
Quoted 8 lines
Quoted 3 lines
Maybe
renaming this "might_sleep_on()" at least brings it more in line withBUG_ON(), and avoids looking like the gross constructs I fear.I named it that at first but I was afraid that someone might get confused and thing the semantics were "
might_sleep_on(&waitqueue)" since 'sleeping on' means something already. To memight_sleep_if(cond)looked a lot clearer at first glance.
might_sleep_when?
Rob