* [PATCH] might_sleep() improvements
@ 2003-09-02 7:51 Mitchell Blank Jr
2003-09-02 8:14 ` Nick Piggin
2003-09-02 13:55 ` Robert Love
0 siblings, 2 replies; 9+ messages in thread
From: Mitchell Blank Jr @ 2003-09-02 7:51 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel
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
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/arch/sparc64/kernel/semaphore.c linux-2.6.0-test4mightsleep/arch/sparc64/kernel/semaphore.c
--- linux-2.6.0-test4-VIRGIN/arch/sparc64/kernel/semaphore.c 2003-07-13 20:37:13.000000000 -0700
+++ linux-2.6.0-test4mightsleep/arch/sparc64/kernel/semaphore.c 2003-09-01 13:38:18.000000000 -0700
@@ -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;
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-alpha/semaphore.h linux-2.6.0-test4mightsleep/include/asm-alpha/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-alpha/semaphore.h 2003-07-13 20:37:17.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-alpha/semaphore.h 2003-09-01 13:28:36.000000000 -0700
@@ -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;
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-arm/semaphore.h linux-2.6.0-test4mightsleep/include/asm-arm/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-arm/semaphore.h 2003-07-13 20:34:31.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-arm/semaphore.h 2003-09-01 13:29:33.000000000 -0700
@@ -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);
}
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-arm26/semaphore.h linux-2.6.0-test4mightsleep/include/asm-arm26/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-arm26/semaphore.h 2003-07-13 20:38:51.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-arm26/semaphore.h 2003-09-01 13:29:04.000000000 -0700
@@ -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);
}
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-cris/semaphore.h linux-2.6.0-test4mightsleep/include/asm-cris/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-cris/semaphore.h 2003-07-13 20:32:42.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-cris/semaphore.h 2003-09-01 13:30:37.000000000 -0700
@@ -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);
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-h8300/semaphore.h linux-2.6.0-test4mightsleep/include/asm-h8300/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-h8300/semaphore.h 2003-08-22 13:47:25.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-h8300/semaphore.h 2003-09-01 13:30:59.000000000 -0700
@@ -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__(
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-ia64/semaphore.h linux-2.6.0-test4mightsleep/include/asm-ia64/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-ia64/semaphore.h 2003-07-13 20:33:51.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-ia64/semaphore.h 2003-09-01 13:31:25.000000000 -0700
@@ -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;
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-m68k/semaphore.h linux-2.6.0-test4mightsleep/include/asm-m68k/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-m68k/semaphore.h 2003-07-13 20:39:22.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-m68k/semaphore.h 2003-09-01 13:32:01.000000000 -0700
@@ -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"
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-m68knommu/semaphore.h linux-2.6.0-test4mightsleep/include/asm-m68knommu/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-m68knommu/semaphore.h 2003-07-13 20:35:14.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-m68knommu/semaphore.h 2003-09-01 13:31:46.000000000 -0700
@@ -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"
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-mips/semaphore.h linux-2.6.0-test4mightsleep/include/asm-mips/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-mips/semaphore.h 2003-07-13 20:33:10.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-mips/semaphore.h 2003-09-01 13:32:26.000000000 -0700
@@ -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;
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-parisc/semaphore.h linux-2.6.0-test4mightsleep/include/asm-parisc/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-parisc/semaphore.h 2003-07-13 20:28:54.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-parisc/semaphore.h 2003-09-01 13:32:42.000000000 -0700
@@ -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--;
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-s390/semaphore.h linux-2.6.0-test4mightsleep/include/asm-s390/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-s390/semaphore.h 2003-07-13 20:28:53.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-s390/semaphore.h 2003-09-01 13:33:06.000000000 -0700
@@ -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;
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-sh/semaphore.h linux-2.6.0-test4mightsleep/include/asm-sh/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-sh/semaphore.h 2003-07-13 20:38:51.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-sh/semaphore.h 2003-09-01 13:15:52.000000000 -0700
@@ -107,6 +107,7 @@
CHECK_MAGIC(sem->__magic);
#endif
+ might_sleep();
if (atomic_dec_return(&sem->count) < 0)
ret = __down_interruptible(sem);
return ret;
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-sparc/semaphore.h linux-2.6.0-test4mightsleep/include/asm-sparc/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-sparc/semaphore.h 2003-07-13 20:39:21.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-sparc/semaphore.h 2003-09-01 13:34:21.000000000 -0700
@@ -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;
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-v850/semaphore.h linux-2.6.0-test4mightsleep/include/asm-v850/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-v850/semaphore.h 2003-07-13 20:33:12.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-v850/semaphore.h 2003-09-01 13:34:52.000000000 -0700
@@ -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;
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/asm-x86_64/semaphore.h linux-2.6.0-test4mightsleep/include/asm-x86_64/semaphore.h
--- linux-2.6.0-test4-VIRGIN/include/asm-x86_64/semaphore.h 2003-07-13 20:28:54.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/asm-x86_64/semaphore.h 2003-09-01 13:35:12.000000000 -0700
@@ -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"
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/linux/kernel.h linux-2.6.0-test4mightsleep/include/linux/kernel.h
--- linux-2.6.0-test4-VIRGIN/include/linux/kernel.h 2003-07-13 20:28:53.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/linux/kernel.h 2003-09-01 13:07:59.000000000 -0700
@@ -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;
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/include/linux/skbuff.h linux-2.6.0-test4mightsleep/include/linux/skbuff.h
--- linux-2.6.0-test4-VIRGIN/include/linux/skbuff.h 2003-08-10 05:28:41.000000000 -0700
+++ linux-2.6.0-test4mightsleep/include/linux/skbuff.h 2003-09-01 13:26:55.000000000 -0700
@@ -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 */
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/mm/page_alloc.c linux-2.6.0-test4mightsleep/mm/page_alloc.c
--- linux-2.6.0-test4-VIRGIN/mm/page_alloc.c 2003-08-22 13:47:28.000000000 -0700
+++ linux-2.6.0-test4mightsleep/mm/page_alloc.c 2003-09-01 13:16:42.000000000 -0700
@@ -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)
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/mm/rmap.c linux-2.6.0-test4mightsleep/mm/rmap.c
--- linux-2.6.0-test4-VIRGIN/mm/rmap.c 2003-07-13 20:38:02.000000000 -0700
+++ linux-2.6.0-test4mightsleep/mm/rmap.c 2003-09-01 13:17:19.000000000 -0700
@@ -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) {
diff -urN -X dontdiff linux-2.6.0-test4-VIRGIN/mm/slab.c linux-2.6.0-test4mightsleep/mm/slab.c
--- linux-2.6.0-test4-VIRGIN/mm/slab.c 2003-08-22 13:47:28.000000000 -0700
+++ linux-2.6.0-test4mightsleep/mm/slab.c 2003-09-01 13:17:01.000000000 -0700
@@ -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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] might_sleep() improvements
2003-09-02 7:51 [PATCH] might_sleep() improvements Mitchell Blank Jr
@ 2003-09-02 8:14 ` Nick Piggin
2003-09-02 8:32 ` Mitchell Blank Jr
2003-09-02 8:36 ` Linus Torvalds
2003-09-02 13:55 ` Robert Love
1 sibling, 2 replies; 9+ messages in thread
From: Nick Piggin @ 2003-09-02 8:14 UTC (permalink / raw)
To: Mitchell Blank Jr; +Cc: Andrew Morton, linux-kernel
Mitchell Blank Jr wrote:
>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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] might_sleep() improvements
2003-09-02 8:14 ` Nick Piggin
@ 2003-09-02 8:32 ` Mitchell Blank Jr
2003-09-02 8:39 ` Nick Piggin
2003-09-02 8:36 ` Linus Torvalds
1 sibling, 1 reply; 9+ messages in thread
From: Mitchell Blank Jr @ 2003-09-02 8:32 UTC (permalink / raw)
To: Nick Piggin; +Cc: linux-kernel
Nick Piggin wrote:
> >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.
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] might_sleep() improvements
2003-09-02 8:32 ` Mitchell Blank Jr
@ 2003-09-02 8:39 ` Nick Piggin
0 siblings, 0 replies; 9+ messages in thread
From: Nick Piggin @ 2003-09-02 8:39 UTC (permalink / raw)
To: Mitchell Blank Jr; +Cc: linux-kernel
Mitchell Blank Jr wrote:
>Nick Piggin wrote:
>
>>>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.
>>
>
>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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] might_sleep() improvements
2003-09-02 8:14 ` Nick Piggin
2003-09-02 8:32 ` Mitchell Blank Jr
@ 2003-09-02 8:36 ` Linus Torvalds
2003-09-02 8:44 ` Nick Piggin
1 sibling, 1 reply; 9+ messages in thread
From: Linus Torvalds @ 2003-09-02 8:36 UTC (permalink / raw)
To: linux-kernel
Nick Piggin wrote:
>
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] might_sleep() improvements
2003-09-02 8:36 ` Linus Torvalds
@ 2003-09-02 8:44 ` Nick Piggin
0 siblings, 0 replies; 9+ messages in thread
From: Nick Piggin @ 2003-09-02 8:44 UTC (permalink / raw)
To: torvalds; +Cc: linux-kernel
Linus Torvalds wrote:
>Nick Piggin wrote:
>
>>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.
>
>
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] might_sleep() improvements
2003-09-02 7:51 [PATCH] might_sleep() improvements Mitchell Blank Jr
2003-09-02 8:14 ` Nick Piggin
@ 2003-09-02 13:55 ` Robert Love
2003-09-02 18:39 ` Mitchell Blank Jr
1 sibling, 1 reply; 9+ messages in thread
From: Robert Love @ 2003-09-02 13:55 UTC (permalink / raw)
To: Mitchell Blank Jr; +Cc: Andrew Morton, linux-kernel
On Tue, 2003-09-02 at 03:51, Mitchell Blank Jr wrote:
> 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.
> 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.
> o Add might_sleep checks to skb_share_check() and skb_unshare() which
> sometimes need to allocate memory.
Great.
> 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())
Even better.
Robert Love
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
On Tue, 2003-09-02 at 03:51, Mitchell Blank Jr wrote:
> 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.
> 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.
> o Add might_sleep checks to skb_share_check() and skb_unshare() which
> sometimes need to allocate memory.
Great.
> 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())
Even better.
Robert Love
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] might_sleep() improvements
2003-09-02 13:55 ` Robert Love
@ 2003-09-02 18:39 ` Mitchell Blank Jr
2003-09-09 3:10 ` Rob Landley
0 siblings, 1 reply; 9+ messages in thread
From: Mitchell Blank Jr @ 2003-09-02 18:39 UTC (permalink / raw)
To: Robert Love; +Cc: linux-kernel
Robert Love wrote:
> > 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.
> 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.
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Robert Love wrote:
> > 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.
> 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.
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] might_sleep() improvements
2003-09-02 18:39 ` Mitchell Blank Jr
@ 2003-09-09 3:10 ` Rob Landley
0 siblings, 0 replies; 9+ messages in thread
From: Rob Landley @ 2003-09-09 3:10 UTC (permalink / raw)
To: Mitchell Blank Jr, Robert Love; +Cc: linux-kernel
On Tuesday 02 September 2003 14:39, Mitchell Blank Jr wrote:
> Robert Love wrote:
> > > 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.
>
> > 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.
>
> 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.
might_sleep_when?
Rob
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2003-09-09 14:48 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-09-02 7:51 [PATCH] might_sleep() improvements Mitchell Blank Jr
2003-09-02 8:14 ` Nick Piggin
2003-09-02 8:32 ` Mitchell Blank Jr
2003-09-02 8:39 ` Nick Piggin
2003-09-02 8:36 ` Linus Torvalds
2003-09-02 8:44 ` Nick Piggin
2003-09-02 13:55 ` Robert Love
2003-09-02 18:39 ` Mitchell Blank Jr
2003-09-09 3:10 ` Rob Landley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).