* [PATCH 1/6] sched, wait: Make the signal_pending() checks consistent
2013-09-30 15:22 [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Peter Zijlstra
@ 2013-09-30 15:22 ` Peter Zijlstra
2013-09-30 15:22 ` [PATCH 2/6] sched, wait: Change timeout logic Peter Zijlstra
` (7 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Peter Zijlstra @ 2013-09-30 15:22 UTC (permalink / raw)
To: Ingo Molnar
Cc: Oleg Nesterov, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel, Peter Zijlstra
[-- Attachment #1: peterz-wait-change-signal.patch --]
[-- Type: text/plain, Size: 2702 bytes --]
There's two patterns to check signals in the __wait_event*() macros:
if (!signal_pending(current)) {
schedule();
continue;
}
ret = -ERESTARTSYS;
break;
And the more natural:
if (signal_pending(current)) {
ret = -ERESTARTSYS;
break;
}
schedule();
Change them all into the latter form.
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
include/linux/tty.h | 13 ++++++-------
include/linux/wait.h | 35 ++++++++++++++++-------------------
2 files changed, 22 insertions(+), 26 deletions(-)
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -686,14 +686,13 @@ do { \
prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
if (condition) \
break; \
- if (!signal_pending(current)) { \
- tty_unlock(tty); \
- schedule(); \
- tty_lock(tty); \
- continue; \
+ if (signal_pending(current)) { \
+ ret = -ERESTARTSYS; \
+ break; \
} \
- ret = -ERESTARTSYS; \
- break; \
+ tty_unlock(tty); \
+ schedule(); \
+ tty_lock(tty); \
} \
finish_wait(&wq, &__wait); \
} while (0)
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -261,12 +261,11 @@ do { \
prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
if (condition) \
break; \
- if (!signal_pending(current)) { \
- schedule(); \
- continue; \
+ if (signal_pending(current)) { \
+ ret = -ERESTARTSYS; \
+ break; \
} \
- ret = -ERESTARTSYS; \
- break; \
+ schedule(); \
} \
finish_wait(&wq, &__wait); \
} while (0)
@@ -302,14 +301,13 @@ do { \
prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
if (condition) \
break; \
- if (!signal_pending(current)) { \
- ret = schedule_timeout(ret); \
- if (!ret) \
- break; \
- continue; \
+ if (signal_pending(current)) { \
+ ret = -ERESTARTSYS; \
+ break; \
} \
- ret = -ERESTARTSYS; \
- break; \
+ ret = schedule_timeout(ret); \
+ if (!ret) \
+ break; \
} \
if (!ret && (condition)) \
ret = 1; \
@@ -439,14 +437,13 @@ do { \
finish_wait(&wq, &__wait); \
break; \
} \
- if (!signal_pending(current)) { \
- schedule(); \
- continue; \
- } \
- ret = -ERESTARTSYS; \
- abort_exclusive_wait(&wq, &__wait, \
+ if (signal_pending(current)) { \
+ ret = -ERESTARTSYS; \
+ abort_exclusive_wait(&wq, &__wait, \
TASK_INTERRUPTIBLE, NULL); \
- break; \
+ break; \
+ } \
+ schedule(); \
} \
} while (0)
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 2/6] sched, wait: Change timeout logic
2013-09-30 15:22 [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Peter Zijlstra
2013-09-30 15:22 ` [PATCH 1/6] sched, wait: Make the signal_pending() checks consistent Peter Zijlstra
@ 2013-09-30 15:22 ` Peter Zijlstra
2013-09-30 15:48 ` Linus Torvalds
2013-09-30 15:22 ` [PATCH 3/6] sched, wait: Change the wait_exclusive control flow Peter Zijlstra
` (6 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Peter Zijlstra @ 2013-09-30 15:22 UTC (permalink / raw)
To: Ingo Molnar
Cc: Oleg Nesterov, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel, Peter Zijlstra
[-- Attachment #1: peterz-wait-change-sched-timo.patch --]
[-- Type: text/plain, Size: 2498 bytes --]
Commit 4c663cf ("wait: fix false timeouts when using
wait_event_timeout()") introduced an additional condition check after
a timeout but there's a few issues;
- it forgot one site
- it put the check after the main loop; not at the actual timeout
check.
Cure both; by wrapping the condition (as suggested by Oleg), this
avoids double evaluation of 'condition' which could be quite big.
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
include/linux/wait.h | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -179,6 +179,14 @@ wait_queue_head_t *bit_waitqueue(void *,
#define wake_up_interruptible_sync_poll(x, m) \
__wake_up_sync_key((x), TASK_INTERRUPTIBLE, 1, (void *) (m))
+#define ___wait_cond_timeout(condition, ret) \
+({ \
+ bool __cond = (condition); \
+ if (__cond && !ret) \
+ ret = 1; \
+ __cond || !ret; \
+})
+
#define __wait_event(wq, condition) \
do { \
DEFINE_WAIT(__wait); \
@@ -217,14 +225,10 @@ do { \
\
for (;;) { \
prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE); \
- if (condition) \
+ if (___wait_cond_timeout(condition, ret)) \
break; \
ret = schedule_timeout(ret); \
- if (!ret) \
- break; \
} \
- if (!ret && (condition)) \
- ret = 1; \
finish_wait(&wq, &__wait); \
} while (0)
@@ -299,18 +303,14 @@ do { \
\
for (;;) { \
prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
- if (condition) \
+ if (___wait_cond_timeout(condition, ret)) \
break; \
if (signal_pending(current)) { \
ret = -ERESTARTSYS; \
break; \
} \
ret = schedule_timeout(ret); \
- if (!ret) \
- break; \
} \
- if (!ret && (condition)) \
- ret = 1; \
finish_wait(&wq, &__wait); \
} while (0)
@@ -815,7 +815,7 @@ do { \
\
for (;;) { \
prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
- if (condition) \
+ if (___wait_cond_timeout(condition, ret)) \
break; \
if (signal_pending(current)) { \
ret = -ERESTARTSYS; \
@@ -824,8 +824,6 @@ do { \
spin_unlock_irq(&lock); \
ret = schedule_timeout(ret); \
spin_lock_irq(&lock); \
- if (!ret) \
- break; \
} \
finish_wait(&wq, &__wait); \
} while (0)
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 2/6] sched, wait: Change timeout logic
2013-09-30 15:22 ` [PATCH 2/6] sched, wait: Change timeout logic Peter Zijlstra
@ 2013-09-30 15:48 ` Linus Torvalds
2013-09-30 16:09 ` Peter Zijlstra
0 siblings, 1 reply; 23+ messages in thread
From: Linus Torvalds @ 2013-09-30 15:48 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Oleg Nesterov, Paul McKenney, Thomas Gleixner,
Andrew Morton, Linux Kernel Mailing List
On Mon, Sep 30, 2013 at 8:22 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> Commit 4c663cf ("wait: fix false timeouts when using
> wait_event_timeout()")
Btw, unrelated to the patch itselt: you seem to use the old broken
7-character short SHA1 format.
Yes, 7 hex characters is generally still unique. But no, it won't stay
so. The birthday paradox means that you start to get collisions more
quickly than you'd think, and the original 7-character hex string
default was done early in git development, when the BK repository was
considered "big", and that one was approaching having 16-bit commit
numbers.
Oh, how naive. In the eight+ years we've used git, we've flown past
that old 16-bit limit in commit numbers (we're now around 400k
commits), and we have over 3M objects. So we already have lots of
collisions in 7 characters. Now, git is smart enough that when it
picks a short form of the SHA1, it will never pick a collision, but
since the tree keeps growing, they can happen later.
As a result, I currently strongly suggest kernel-developers do
git config --global core.abbrev 12
to set the default abbreviated SHA1 count to 12 characters, not the
old default of 7. That pushes out the collision worry to the point
where we're talking "later generations" rather than "forseeable
future".
Linus
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 2/6] sched, wait: Change timeout logic
2013-09-30 15:48 ` Linus Torvalds
@ 2013-09-30 16:09 ` Peter Zijlstra
0 siblings, 0 replies; 23+ messages in thread
From: Peter Zijlstra @ 2013-09-30 16:09 UTC (permalink / raw)
To: Linus Torvalds
Cc: Ingo Molnar, Oleg Nesterov, Paul McKenney, Thomas Gleixner,
Andrew Morton, Linux Kernel Mailing List
On Mon, Sep 30, 2013 at 08:48:09AM -0700, Linus Torvalds wrote:
> As a result, I currently strongly suggest kernel-developers do
>
> git config --global core.abbrev 12
>
Done!
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/6] sched, wait: Change the wait_exclusive control flow
2013-09-30 15:22 [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Peter Zijlstra
2013-09-30 15:22 ` [PATCH 1/6] sched, wait: Make the signal_pending() checks consistent Peter Zijlstra
2013-09-30 15:22 ` [PATCH 2/6] sched, wait: Change timeout logic Peter Zijlstra
@ 2013-09-30 15:22 ` Peter Zijlstra
2013-09-30 15:22 ` [PATCH 4/6] sched, wait: Collapse __wait_event macros Peter Zijlstra
` (5 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Peter Zijlstra @ 2013-09-30 15:22 UTC (permalink / raw)
To: Ingo Molnar
Cc: Oleg Nesterov, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel, Peter Zijlstra
[-- Attachment #1: peterz-wait-change-exclusive.patch --]
[-- Type: text/plain, Size: 1128 bytes --]
Purely a preparatory patch; it changes the control flow to match what
will soon be generated by generic code so that that patch can be a
unity transform.
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
include/linux/wait.h | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -430,23 +430,24 @@ do { \
#define __wait_event_interruptible_exclusive(wq, condition, ret) \
do { \
+ __label__ __out; \
DEFINE_WAIT(__wait); \
\
for (;;) { \
prepare_to_wait_exclusive(&wq, &__wait, \
TASK_INTERRUPTIBLE); \
- if (condition) { \
- finish_wait(&wq, &__wait); \
+ if (condition) \
break; \
- } \
if (signal_pending(current)) { \
ret = -ERESTARTSYS; \
abort_exclusive_wait(&wq, &__wait, \
TASK_INTERRUPTIBLE, NULL); \
- break; \
+ goto __out; \
} \
schedule(); \
} \
+ finish_wait(&wq, &__wait); \
+__out: ; \
} while (0)
#define wait_event_interruptible_exclusive(wq, condition) \
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 4/6] sched, wait: Collapse __wait_event macros
2013-09-30 15:22 [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Peter Zijlstra
` (2 preceding siblings ...)
2013-09-30 15:22 ` [PATCH 3/6] sched, wait: Change the wait_exclusive control flow Peter Zijlstra
@ 2013-09-30 15:22 ` Peter Zijlstra
2013-09-30 15:22 ` [PATCH 5/6] sched, wait: Also use ___wait_event() for __wait_event_hrtimeout Peter Zijlstra
` (4 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Peter Zijlstra @ 2013-09-30 15:22 UTC (permalink / raw)
To: Ingo Molnar
Cc: Oleg Nesterov, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel, Peter Zijlstra
[-- Attachment #1: peterz-wait-deduplicate.patch --]
[-- Type: text/plain, Size: 8853 bytes --]
There's far too much duplication in the __wait_event macros; fix this.
With the previous patches changing the various __wait_event*()
implementations to be more uniform; we can now collapse the lot
without also changing generated code.
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
include/linux/tty.h | 21 +----
include/linux/wait.h | 192 +++++++++++++++------------------------------------
2 files changed, 63 insertions(+), 150 deletions(-)
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -679,23 +679,10 @@ static inline void tty_wait_until_sent_f
})
#define __wait_event_interruptible_tty(tty, wq, condition, ret) \
-do { \
- DEFINE_WAIT(__wait); \
- \
- for (;;) { \
- prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
- if (condition) \
- break; \
- if (signal_pending(current)) { \
- ret = -ERESTARTSYS; \
- break; \
- } \
- tty_unlock(tty); \
- schedule(); \
- tty_lock(tty); \
- } \
- finish_wait(&wq, &__wait); \
-} while (0)
+ ___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, ret, \
+ tty_unlock(tty); \
+ schedule(); \
+ tty_lock(tty))
#ifdef CONFIG_PROC_FS
extern void proc_tty_register_driver(struct tty_driver *);
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -187,19 +187,46 @@ wait_queue_head_t *bit_waitqueue(void *,
__cond || !ret; \
})
-#define __wait_event(wq, condition) \
+#define ___wait_signal_pending(state) \
+ ((state == TASK_INTERRUPTIBLE && signal_pending(current)) || \
+ (state == TASK_KILLABLE && fatal_signal_pending(current)))
+
+#define ___wait_nop_ret int ret __always_unused
+
+#define ___wait_event(wq, condition, state, exclusive, ret, cmd) \
do { \
+ __label__ __out; \
DEFINE_WAIT(__wait); \
\
for (;;) { \
- prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE); \
+ if (exclusive) \
+ prepare_to_wait_exclusive(&wq, &__wait, state); \
+ else \
+ prepare_to_wait(&wq, &__wait, state); \
+ \
if (condition) \
break; \
- schedule(); \
+ \
+ if (___wait_signal_pending(state)) { \
+ ret = -ERESTARTSYS; \
+ if (exclusive) { \
+ abort_exclusive_wait(&wq, &__wait, \
+ state, NULL); \
+ goto __out; \
+ } \
+ break; \
+ } \
+ \
+ cmd; \
} \
finish_wait(&wq, &__wait); \
+__out: ; \
} while (0)
+#define __wait_event(wq, condition) \
+ ___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 0, \
+ ___wait_nop_ret, schedule())
+
/**
* wait_event - sleep until a condition gets true
* @wq: the waitqueue to wait on
@@ -220,17 +247,9 @@ do { \
} while (0)
#define __wait_event_timeout(wq, condition, ret) \
-do { \
- DEFINE_WAIT(__wait); \
- \
- for (;;) { \
- prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE); \
- if (___wait_cond_timeout(condition, ret)) \
- break; \
- ret = schedule_timeout(ret); \
- } \
- finish_wait(&wq, &__wait); \
-} while (0)
+ ___wait_event(wq, ___wait_cond_timeout(condition, ret), \
+ TASK_UNINTERRUPTIBLE, 0, ret, \
+ ret = schedule_timeout(ret))
/**
* wait_event_timeout - sleep until a condition gets true or a timeout elapses
@@ -258,21 +277,8 @@ do { \
})
#define __wait_event_interruptible(wq, condition, ret) \
-do { \
- DEFINE_WAIT(__wait); \
- \
- for (;;) { \
- prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
- if (condition) \
- break; \
- if (signal_pending(current)) { \
- ret = -ERESTARTSYS; \
- break; \
- } \
- schedule(); \
- } \
- finish_wait(&wq, &__wait); \
-} while (0)
+ ___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, ret, \
+ schedule())
/**
* wait_event_interruptible - sleep until a condition gets true
@@ -298,21 +304,9 @@ do { \
})
#define __wait_event_interruptible_timeout(wq, condition, ret) \
-do { \
- DEFINE_WAIT(__wait); \
- \
- for (;;) { \
- prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
- if (___wait_cond_timeout(condition, ret)) \
- break; \
- if (signal_pending(current)) { \
- ret = -ERESTARTSYS; \
- break; \
- } \
- ret = schedule_timeout(ret); \
- } \
- finish_wait(&wq, &__wait); \
-} while (0)
+ ___wait_event(wq, ___wait_cond_timeout(condition, ret), \
+ TASK_INTERRUPTIBLE, 0, ret, \
+ ret = schedule_timeout(ret))
/**
* wait_event_interruptible_timeout - sleep until a condition gets true or a timeout elapses
@@ -427,26 +421,8 @@ do { \
})
#define __wait_event_interruptible_exclusive(wq, condition, ret) \
-do { \
- __label__ __out; \
- DEFINE_WAIT(__wait); \
- \
- for (;;) { \
- prepare_to_wait_exclusive(&wq, &__wait, \
- TASK_INTERRUPTIBLE); \
- if (condition) \
- break; \
- if (signal_pending(current)) { \
- ret = -ERESTARTSYS; \
- abort_exclusive_wait(&wq, &__wait, \
- TASK_INTERRUPTIBLE, NULL); \
- goto __out; \
- } \
- schedule(); \
- } \
- finish_wait(&wq, &__wait); \
-__out: ; \
-} while (0)
+ ___wait_event(wq, condition, TASK_INTERRUPTIBLE, 1, ret, \
+ schedule())
#define wait_event_interruptible_exclusive(wq, condition) \
({ \
@@ -606,22 +582,7 @@ __out: ; \
#define __wait_event_killable(wq, condition, ret) \
-do { \
- DEFINE_WAIT(__wait); \
- \
- for (;;) { \
- prepare_to_wait(&wq, &__wait, TASK_KILLABLE); \
- if (condition) \
- break; \
- if (!fatal_signal_pending(current)) { \
- schedule(); \
- continue; \
- } \
- ret = -ERESTARTSYS; \
- break; \
- } \
- finish_wait(&wq, &__wait); \
-} while (0)
+ ___wait_event(wq, condition, TASK_KILLABLE, 0, ret, schedule())
/**
* wait_event_killable - sleep until a condition gets true
@@ -648,20 +609,12 @@ do { \
#define __wait_event_lock_irq(wq, condition, lock, cmd) \
-do { \
- DEFINE_WAIT(__wait); \
- \
- for (;;) { \
- prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE); \
- if (condition) \
- break; \
- spin_unlock_irq(&lock); \
- cmd; \
- schedule(); \
- spin_lock_irq(&lock); \
- } \
- finish_wait(&wq, &__wait); \
-} while (0)
+ ___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 0, \
+ ___wait_nop_ret, \
+ spin_unlock_irq(&lock); \
+ cmd; \
+ schedule(); \
+ spin_lock_irq(&lock))
/**
* wait_event_lock_irq_cmd - sleep until a condition gets true. The
@@ -721,26 +674,12 @@ do { \
} while (0)
-#define __wait_event_interruptible_lock_irq(wq, condition, \
- lock, ret, cmd) \
-do { \
- DEFINE_WAIT(__wait); \
- \
- for (;;) { \
- prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
- if (condition) \
- break; \
- if (signal_pending(current)) { \
- ret = -ERESTARTSYS; \
- break; \
- } \
- spin_unlock_irq(&lock); \
- cmd; \
- schedule(); \
- spin_lock_irq(&lock); \
- } \
- finish_wait(&wq, &__wait); \
-} while (0)
+#define __wait_event_interruptible_lock_irq(wq, condition, lock, ret, cmd) \
+ ___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, ret, \
+ spin_unlock_irq(&lock); \
+ cmd; \
+ schedule(); \
+ spin_lock_irq(&lock))
/**
* wait_event_interruptible_lock_irq_cmd - sleep until a condition gets true.
@@ -809,25 +748,12 @@ do { \
__ret; \
})
-#define __wait_event_interruptible_lock_irq_timeout(wq, condition, \
- lock, ret) \
-do { \
- DEFINE_WAIT(__wait); \
- \
- for (;;) { \
- prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
- if (___wait_cond_timeout(condition, ret)) \
- break; \
- if (signal_pending(current)) { \
- ret = -ERESTARTSYS; \
- break; \
- } \
- spin_unlock_irq(&lock); \
- ret = schedule_timeout(ret); \
- spin_lock_irq(&lock); \
- } \
- finish_wait(&wq, &__wait); \
-} while (0)
+#define __wait_event_interruptible_lock_irq_timeout(wq, condition, lock, ret) \
+ ___wait_event(wq, ___wait_cond_timeout(condition, ret), \
+ TASK_INTERRUPTIBLE, 0, ret, \
+ spin_unlock_irq(&lock); \
+ ret = schedule_timeout(ret); \
+ spin_lock_irq(&lock));
/**
* wait_event_interruptible_lock_irq_timeout - sleep until a condition gets true or a timeout elapses.
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 5/6] sched, wait: Also use ___wait_event() for __wait_event_hrtimeout
2013-09-30 15:22 [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Peter Zijlstra
` (3 preceding siblings ...)
2013-09-30 15:22 ` [PATCH 4/6] sched, wait: Collapse __wait_event macros Peter Zijlstra
@ 2013-09-30 15:22 ` Peter Zijlstra
2013-09-30 15:22 ` [PATCH 6/6] sched, wait: Make the __wait_event*() interface more friendly Peter Zijlstra
` (3 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Peter Zijlstra @ 2013-09-30 15:22 UTC (permalink / raw)
To: Ingo Molnar
Cc: Oleg Nesterov, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel, Peter Zijlstra
[-- Attachment #1: peterz-wait-dedup-hrtimer.patch --]
[-- Type: text/plain, Size: 1296 bytes --]
The actual core of __wait_event_hrtimeout() is identical to what
___wait_event() generates.
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
include/linux/wait.h | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -338,7 +338,6 @@ do { \
#define __wait_event_hrtimeout(wq, condition, timeout, state) \
({ \
int __ret = 0; \
- DEFINE_WAIT(__wait); \
struct hrtimer_sleeper __t; \
\
hrtimer_init_on_stack(&__t.timer, CLOCK_MONOTONIC, \
@@ -349,25 +348,15 @@ do { \
current->timer_slack_ns, \
HRTIMER_MODE_REL); \
\
- for (;;) { \
- prepare_to_wait(&wq, &__wait, state); \
- if (condition) \
- break; \
- if (state == TASK_INTERRUPTIBLE && \
- signal_pending(current)) { \
- __ret = -ERESTARTSYS; \
- break; \
- } \
+ ___wait_event(wq, condition, state, 0, __ret, \
if (!__t.task) { \
__ret = -ETIME; \
break; \
} \
- schedule(); \
- } \
+ schedule()); \
\
hrtimer_cancel(&__t.timer); \
destroy_hrtimer_on_stack(&__t.timer); \
- finish_wait(&wq, &__wait); \
__ret; \
})
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH 6/6] sched, wait: Make the __wait_event*() interface more friendly
2013-09-30 15:22 [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Peter Zijlstra
` (4 preceding siblings ...)
2013-09-30 15:22 ` [PATCH 5/6] sched, wait: Also use ___wait_event() for __wait_event_hrtimeout Peter Zijlstra
@ 2013-09-30 15:22 ` Peter Zijlstra
2013-10-01 6:39 ` Ingo Molnar
2013-09-30 15:50 ` [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Linus Torvalds
` (2 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Peter Zijlstra @ 2013-09-30 15:22 UTC (permalink / raw)
To: Ingo Molnar
Cc: Oleg Nesterov, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel, Peter Zijlstra
[-- Attachment #1: peterz-wait-___wait_event-ret.patch --]
[-- Type: text/plain, Size: 12026 bytes --]
Change all __wait_event*() implementations to match the corresponding
wait_event*() signature for convenience.
In particular this does away with the weird 'ret' logic. Since there
are __wait_event*() users this requires we update them too.
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
arch/mips/kernel/rtlx.c | 19 +++---
include/linux/tty.h | 10 +--
include/linux/wait.h | 113 +++++++++++++++++++---------------------
net/irda/af_irda.c | 5 -
net/netfilter/ipvs/ip_vs_sync.c | 7 --
5 files changed, 73 insertions(+), 81 deletions(-)
--- a/arch/mips/kernel/rtlx.c
+++ b/arch/mips/kernel/rtlx.c
@@ -172,8 +172,9 @@ int rtlx_open(int index, int can_sleep)
if (rtlx == NULL) {
if( (p = vpe_get_shared(tclimit)) == NULL) {
if (can_sleep) {
- __wait_event_interruptible(channel_wqs[index].lx_queue,
- (p = vpe_get_shared(tclimit)), ret);
+ ret = __wait_event_interruptible(
+ channel_wqs[index].lx_queue,
+ (p = vpe_get_shared(tclimit)));
if (ret)
goto out_fail;
} else {
@@ -263,11 +264,10 @@ unsigned int rtlx_read_poll(int index, i
/* data available to read? */
if (chan->lx_read == chan->lx_write) {
if (can_sleep) {
- int ret = 0;
-
- __wait_event_interruptible(channel_wqs[index].lx_queue,
+ int ret = __wait_event_interruptible(
+ channel_wqs[index].lx_queue,
(chan->lx_read != chan->lx_write) ||
- sp_stopping, ret);
+ sp_stopping);
if (ret)
return ret;
@@ -440,14 +440,13 @@ static ssize_t file_write(struct file *f
/* any space left... */
if (!rtlx_write_poll(minor)) {
- int ret = 0;
+ int ret;
if (file->f_flags & O_NONBLOCK)
return -EAGAIN;
- __wait_event_interruptible(channel_wqs[minor].rt_queue,
- rtlx_write_poll(minor),
- ret);
+ ret = __wait_event_interruptible(channel_wqs[minor].rt_queue,
+ rtlx_write_poll(minor));
if (ret)
return ret;
}
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -672,14 +672,14 @@ static inline void tty_wait_until_sent_f
#define wait_event_interruptible_tty(tty, wq, condition) \
({ \
int __ret = 0; \
- if (!(condition)) { \
- __wait_event_interruptible_tty(tty, wq, condition, __ret); \
- } \
+ if (!(condition)) \
+ __ret = __wait_event_interruptible_tty(tty, wq, \
+ condition); \
__ret; \
})
-#define __wait_event_interruptible_tty(tty, wq, condition, ret) \
- ___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, ret, \
+#define __wait_event_interruptible_tty(tty, wq, condition) \
+ ___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, 0, \
tty_unlock(tty); \
schedule(); \
tty_lock(tty))
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -179,24 +179,23 @@ wait_queue_head_t *bit_waitqueue(void *,
#define wake_up_interruptible_sync_poll(x, m) \
__wake_up_sync_key((x), TASK_INTERRUPTIBLE, 1, (void *) (m))
-#define ___wait_cond_timeout(condition, ret) \
+#define ___wait_cond_timeout(condition) \
({ \
bool __cond = (condition); \
- if (__cond && !ret) \
- ret = 1; \
- __cond || !ret; \
+ if (__cond && !__ret) \
+ __ret = 1; \
+ __cond || !__ret; \
})
#define ___wait_signal_pending(state) \
((state == TASK_INTERRUPTIBLE && signal_pending(current)) || \
(state == TASK_KILLABLE && fatal_signal_pending(current)))
-#define ___wait_nop_ret int ret __always_unused
-
#define ___wait_event(wq, condition, state, exclusive, ret, cmd) \
-do { \
+({ \
__label__ __out; \
DEFINE_WAIT(__wait); \
+ long __ret = ret; \
\
for (;;) { \
if (exclusive) \
@@ -208,7 +207,7 @@ do { \
break; \
\
if (___wait_signal_pending(state)) { \
- ret = -ERESTARTSYS; \
+ __ret = -ERESTARTSYS; \
if (exclusive) { \
abort_exclusive_wait(&wq, &__wait, \
state, NULL); \
@@ -220,12 +219,12 @@ do { \
cmd; \
} \
finish_wait(&wq, &__wait); \
-__out: ; \
-} while (0)
+__out: __ret; \
+})
#define __wait_event(wq, condition) \
- ___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 0, \
- ___wait_nop_ret, schedule())
+ (void)___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 0, 0, \
+ schedule())
/**
* wait_event - sleep until a condition gets true
@@ -246,10 +245,10 @@ do { \
__wait_event(wq, condition); \
} while (0)
-#define __wait_event_timeout(wq, condition, ret) \
- ___wait_event(wq, ___wait_cond_timeout(condition, ret), \
- TASK_UNINTERRUPTIBLE, 0, ret, \
- ret = schedule_timeout(ret))
+#define __wait_event_timeout(wq, condition, timeout) \
+ ___wait_event(wq, ___wait_cond_timeout(condition), \
+ TASK_UNINTERRUPTIBLE, 0, timeout, \
+ __ret = schedule_timeout(__ret))
/**
* wait_event_timeout - sleep until a condition gets true or a timeout elapses
@@ -272,12 +271,12 @@ do { \
({ \
long __ret = timeout; \
if (!(condition)) \
- __wait_event_timeout(wq, condition, __ret); \
+ __ret = __wait_event_timeout(wq, condition, timeout); \
__ret; \
})
-#define __wait_event_interruptible(wq, condition, ret) \
- ___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, ret, \
+#define __wait_event_interruptible(wq, condition) \
+ ___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, 0, \
schedule())
/**
@@ -299,14 +298,14 @@ do { \
({ \
int __ret = 0; \
if (!(condition)) \
- __wait_event_interruptible(wq, condition, __ret); \
+ __ret = __wait_event_interruptible(wq, condition); \
__ret; \
})
-#define __wait_event_interruptible_timeout(wq, condition, ret) \
- ___wait_event(wq, ___wait_cond_timeout(condition, ret), \
- TASK_INTERRUPTIBLE, 0, ret, \
- ret = schedule_timeout(ret))
+#define __wait_event_interruptible_timeout(wq, condition, timeout) \
+ ___wait_event(wq, ___wait_cond_timeout(condition), \
+ TASK_INTERRUPTIBLE, 0, timeout, \
+ __ret = schedule_timeout(__ret))
/**
* wait_event_interruptible_timeout - sleep until a condition gets true or a timeout elapses
@@ -330,7 +329,8 @@ do { \
({ \
long __ret = timeout; \
if (!(condition)) \
- __wait_event_interruptible_timeout(wq, condition, __ret); \
+ __ret = __wait_event_interruptible_timeout(wq, \
+ condition, timeout); \
__ret; \
})
@@ -347,7 +347,7 @@ do { \
current->timer_slack_ns, \
HRTIMER_MODE_REL); \
\
- ___wait_event(wq, condition, state, 0, __ret, \
+ __ret = ___wait_event(wq, condition, state, 0, 0, \
if (!__t.task) { \
__ret = -ETIME; \
break; \
@@ -409,15 +409,15 @@ do { \
__ret; \
})
-#define __wait_event_interruptible_exclusive(wq, condition, ret) \
- ___wait_event(wq, condition, TASK_INTERRUPTIBLE, 1, ret, \
+#define __wait_event_interruptible_exclusive(wq, condition) \
+ ___wait_event(wq, condition, TASK_INTERRUPTIBLE, 1, 0, \
schedule())
#define wait_event_interruptible_exclusive(wq, condition) \
({ \
int __ret = 0; \
if (!(condition)) \
- __wait_event_interruptible_exclusive(wq, condition, __ret);\
+ __ret = __wait_event_interruptible_exclusive(wq, condition);\
__ret; \
})
@@ -570,8 +570,8 @@ do { \
-#define __wait_event_killable(wq, condition, ret) \
- ___wait_event(wq, condition, TASK_KILLABLE, 0, ret, schedule())
+#define __wait_event_killable(wq, condition) \
+ ___wait_event(wq, condition, TASK_KILLABLE, 0, 0, schedule())
/**
* wait_event_killable - sleep until a condition gets true
@@ -592,18 +592,17 @@ do { \
({ \
int __ret = 0; \
if (!(condition)) \
- __wait_event_killable(wq, condition, __ret); \
+ __ret = __wait_event_killable(wq, condition); \
__ret; \
})
#define __wait_event_lock_irq(wq, condition, lock, cmd) \
- ___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 0, \
- ___wait_nop_ret, \
- spin_unlock_irq(&lock); \
- cmd; \
- schedule(); \
- spin_lock_irq(&lock))
+ (void)___wait_event(wq, condition, TASK_UNINTERRUPTIBLE, 0, 0, \
+ spin_unlock_irq(&lock); \
+ cmd; \
+ schedule(); \
+ spin_lock_irq(&lock))
/**
* wait_event_lock_irq_cmd - sleep until a condition gets true. The
@@ -663,11 +662,11 @@ do { \
} while (0)
-#define __wait_event_interruptible_lock_irq(wq, condition, lock, ret, cmd) \
- ___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, ret, \
- spin_unlock_irq(&lock); \
- cmd; \
- schedule(); \
+#define __wait_event_interruptible_lock_irq(wq, condition, lock, cmd) \
+ ___wait_event(wq, condition, TASK_INTERRUPTIBLE, 0, 0, \
+ spin_unlock_irq(&lock); \
+ cmd; \
+ schedule(); \
spin_lock_irq(&lock))
/**
@@ -698,10 +697,9 @@ do { \
#define wait_event_interruptible_lock_irq_cmd(wq, condition, lock, cmd) \
({ \
int __ret = 0; \
- \
if (!(condition)) \
- __wait_event_interruptible_lock_irq(wq, condition, \
- lock, __ret, cmd); \
+ __ret = __wait_event_interruptible_lock_irq(wq, \
+ condition, lock, cmd); \
__ret; \
})
@@ -730,18 +728,18 @@ do { \
#define wait_event_interruptible_lock_irq(wq, condition, lock) \
({ \
int __ret = 0; \
- \
if (!(condition)) \
- __wait_event_interruptible_lock_irq(wq, condition, \
- lock, __ret, ); \
+ __ret = __wait_event_interruptible_lock_irq(wq, \
+ condition, lock,) \
__ret; \
})
-#define __wait_event_interruptible_lock_irq_timeout(wq, condition, lock, ret) \
- ___wait_event(wq, ___wait_cond_timeout(condition, ret), \
- TASK_INTERRUPTIBLE, 0, ret, \
- spin_unlock_irq(&lock); \
- ret = schedule_timeout(ret); \
+#define __wait_event_interruptible_lock_irq_timeout(wq, condition, \
+ lock, timeout) \
+ ___wait_event(wq, ___wait_cond_timeout(condition), \
+ TASK_INTERRUPTIBLE, 0, ret, \
+ spin_unlock_irq(&lock); \
+ __ret = schedule_timeout(__ret); \
spin_lock_irq(&lock));
/**
@@ -771,11 +769,10 @@ do { \
#define wait_event_interruptible_lock_irq_timeout(wq, condition, lock, \
timeout) \
({ \
- int __ret = timeout; \
- \
+ long __ret = timeout; \
if (!(condition)) \
- __wait_event_interruptible_lock_irq_timeout( \
- wq, condition, lock, __ret); \
+ __ret = __wait_event_interruptible_lock_irq_timeout( \
+ wq, condition, lock, timeout); \
__ret; \
})
--- a/net/irda/af_irda.c
+++ b/net/irda/af_irda.c
@@ -2563,9 +2563,8 @@ static int irda_getsockopt(struct socket
jiffies + msecs_to_jiffies(val));
/* Wait for IR-LMP to call us back */
- __wait_event_interruptible(self->query_wait,
- (self->cachedaddr != 0 || self->errno == -ETIME),
- err);
+ err = __wait_event_interruptible(self->query_wait,
+ (self->cachedaddr != 0 || self->errno == -ETIME));
/* If watchdog is still activated, kill it! */
del_timer(&(self->watchdog));
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1637,12 +1637,9 @@ static int sync_thread_master(void *data
continue;
}
while (ip_vs_send_sync_msg(tinfo->sock, sb->mesg) < 0) {
- int ret = 0;
-
- __wait_event_interruptible(*sk_sleep(sk),
+ int ret = __wait_event_interruptible(*sk_sleep(sk),
sock_writeable(sk) ||
- kthread_should_stop(),
- ret);
+ kthread_should_stop());
if (unlikely(kthread_should_stop()))
goto done;
}
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 6/6] sched, wait: Make the __wait_event*() interface more friendly
2013-09-30 15:22 ` [PATCH 6/6] sched, wait: Make the __wait_event*() interface more friendly Peter Zijlstra
@ 2013-10-01 6:39 ` Ingo Molnar
0 siblings, 0 replies; 23+ messages in thread
From: Ingo Molnar @ 2013-10-01 6:39 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Oleg Nesterov, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel
* Peter Zijlstra <peterz@infradead.org> wrote:
> Change all __wait_event*() implementations to match the corresponding
> wait_event*() signature for convenience.
>
> In particular this does away with the weird 'ret' logic. Since there
> are __wait_event*() users this requires we update them too.
>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> ---
> arch/mips/kernel/rtlx.c | 19 +++---
> include/linux/tty.h | 10 +--
> include/linux/wait.h | 113 +++++++++++++++++++---------------------
> net/irda/af_irda.c | 5 -
> net/netfilter/ipvs/ip_vs_sync.c | 7 --
> 5 files changed, 73 insertions(+), 81 deletions(-)
Since now everyone seems to agree about this series, and since this
particular patch changes generated code it would be really nice to split
it into about ten per interface patches:
- __wait_event_interruptible()
- __wait_event_interruptible_timeout()
- __wait_event_interruptible_exclusive()
- __wait_event_interruptible_lock_irq()
- __wait_event_interruptible_tty()
- __wait_event()
- __wait_event_timeout()
- __wait_event_killable()
- __wait_event_interruptible_lock_irq_timeout()
The changes seem to be mostly isolated, they are even in separate patch
hunks most of the time making a splitup relatively easy - with a handful
of semantic interdependencies.
There's very little downside to doing it the split-up way, and there's a
lot of bisection and bug review upside, should any of these changes prove
problematic...
Thanks,
Ingo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4
2013-09-30 15:22 [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Peter Zijlstra
` (5 preceding siblings ...)
2013-09-30 15:22 ` [PATCH 6/6] sched, wait: Make the __wait_event*() interface more friendly Peter Zijlstra
@ 2013-09-30 15:50 ` Linus Torvalds
2013-09-30 16:11 ` Peter Zijlstra
2013-09-30 17:40 ` Oleg Nesterov
2013-10-01 17:01 ` [RFC] introduce prepare_to_wait_event() Oleg Nesterov
8 siblings, 1 reply; 23+ messages in thread
From: Linus Torvalds @ 2013-09-30 15:50 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Oleg Nesterov, Paul McKenney, Thomas Gleixner,
Andrew Morton, Linux Kernel Mailing List
On Mon, Sep 30, 2013 at 8:22 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Compile and boot tested on x86_64.
Btw, I assume the odd binary size reduction is gone now, and code
generation is generally identical?
Linus
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4
2013-09-30 15:50 ` [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Linus Torvalds
@ 2013-09-30 16:11 ` Peter Zijlstra
0 siblings, 0 replies; 23+ messages in thread
From: Peter Zijlstra @ 2013-09-30 16:11 UTC (permalink / raw)
To: Linus Torvalds
Cc: Ingo Molnar, Oleg Nesterov, Paul McKenney, Thomas Gleixner,
Andrew Morton, Linux Kernel Mailing List
On Mon, Sep 30, 2013 at 08:50:04AM -0700, Linus Torvalds wrote:
> On Mon, Sep 30, 2013 at 8:22 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > Compile and boot tested on x86_64.
>
> Btw, I assume the odd binary size reduction is gone now, and code
> generation is generally identical?
Over patch 4; yes. Patches 1-3 generate different kernels, esp patch 2
has a large drop in size, 5 too is invariant. Patch 6 however increases
code size again, but then it actually changes the generated code so
that's somewhat expected.
But with patches 1-3 its all clear what changes and why; and having the
big scary conversion patch 4 invariant is good.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4
2013-09-30 15:22 [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Peter Zijlstra
` (6 preceding siblings ...)
2013-09-30 15:50 ` [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Linus Torvalds
@ 2013-09-30 17:40 ` Oleg Nesterov
2013-09-30 18:00 ` Oleg Nesterov
2013-09-30 18:09 ` Peter Zijlstra
2013-10-01 17:01 ` [RFC] introduce prepare_to_wait_event() Oleg Nesterov
8 siblings, 2 replies; 23+ messages in thread
From: Oleg Nesterov @ 2013-09-30 17:40 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel
On 09/30, Peter Zijlstra wrote:
>
> Compile and boot tested on x86_64.
Looks fine, but I'll try to actually read this series tomorrow.
Not that I think this needs my review ;)
But at first glance, I think that something like below makes sense
as 7/6.
Once again, of course I do not blame this series, but
wait_event_timeout(wq, true, 0) still returns 0.
Oleg.
--- x/include/linux/wait.h
+++ x/include/linux/wait.h
@@ -184,9 +184,14 @@ wait_queue_head_t *bit_waitqueue(void *,
bool __cond = (condition); \
if (__cond && !__ret) \
__ret = 1; \
- __cond || !__ret; \
+ __cond; \
})
+#define ___wait_schedule_timeout(tout) \
+ if (!tout) \
+ break; \
+ tout = schedule_timeout(tout)
+
#define ___wait_signal_pending(state) \
((state == TASK_INTERRUPTIBLE && signal_pending(current)) || \
(state == TASK_KILLABLE && fatal_signal_pending(current)))
@@ -248,7 +253,7 @@ do { \
#define __wait_event_timeout(wq, condition, timeout) \
___wait_event(wq, ___wait_cond_timeout(condition), \
TASK_UNINTERRUPTIBLE, 0, timeout, \
- __ret = schedule_timeout(__ret))
+ ___wait_schedule_timeout(__ret))
/**
* wait_event_timeout - sleep until a condition gets true or a timeout elapses
@@ -270,7 +275,7 @@ do { \
#define wait_event_timeout(wq, condition, timeout) \
({ \
long __ret = timeout; \
- if (!(condition)) \
+ if (!___wait_cond_timeout(condition)) \
__ret = __wait_event_timeout(wq, condition, timeout); \
__ret; \
})
@@ -305,7 +310,7 @@ do { \
#define __wait_event_interruptible_timeout(wq, condition, timeout) \
___wait_event(wq, ___wait_cond_timeout(condition), \
TASK_INTERRUPTIBLE, 0, timeout, \
- __ret = schedule_timeout(__ret))
+ ___wait_schedule_timeout(__ret))
/**
* wait_event_interruptible_timeout - sleep until a condition gets true or a timeout elapses
@@ -328,7 +333,7 @@ do { \
#define wait_event_interruptible_timeout(wq, condition, timeout) \
({ \
long __ret = timeout; \
- if (!(condition)) \
+ if (!___wait_cond_timeout(condition)) \
__ret = __wait_event_interruptible_timeout(wq, \
condition, timeout); \
__ret; \
@@ -739,7 +744,7 @@ do { \
___wait_event(wq, ___wait_cond_timeout(condition), \
TASK_INTERRUPTIBLE, 0, ret, \
spin_unlock_irq(&lock); \
- __ret = schedule_timeout(__ret); \
+ ___wait_schedule_timeout(__ret); \
spin_lock_irq(&lock));
/**
@@ -770,7 +775,7 @@ do { \
timeout) \
({ \
long __ret = timeout; \
- if (!(condition)) \
+ if (!___wait_cond_timeout(condition)) \
__ret = __wait_event_interruptible_lock_irq_timeout( \
wq, condition, lock, timeout); \
__ret; \
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4
2013-09-30 17:40 ` Oleg Nesterov
@ 2013-09-30 18:00 ` Oleg Nesterov
2013-09-30 18:09 ` Peter Zijlstra
1 sibling, 0 replies; 23+ messages in thread
From: Oleg Nesterov @ 2013-09-30 18:00 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel
On 09/30, Oleg Nesterov wrote:
>
> But at first glance, I think that something like below makes sense
> as 7/6.
Well, _something like_ perhaps,
> ___wait_event(wq, ___wait_cond_timeout(condition), \
> TASK_INTERRUPTIBLE, 0, ret, \
> spin_unlock_irq(&lock); \
> - __ret = schedule_timeout(__ret); \
> + ___wait_schedule_timeout(__ret); \
this is obviously wrong ;)
Oleg.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4
2013-09-30 17:40 ` Oleg Nesterov
2013-09-30 18:00 ` Oleg Nesterov
@ 2013-09-30 18:09 ` Peter Zijlstra
2013-09-30 18:13 ` Oleg Nesterov
1 sibling, 1 reply; 23+ messages in thread
From: Peter Zijlstra @ 2013-09-30 18:09 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel
On Mon, Sep 30, 2013 at 07:40:54PM +0200, Oleg Nesterov wrote:
> Once again, of course I do not blame this series, but
> wait_event_timeout(wq, true, 0) still returns 0.
So we have:
#define ___wait_cond_timeout(condition) \
({ \
bool __cond = (condition); \
if (__cond && !__ret) \
__ret = 1; \
__cond || !__ret; \
})
#define ___wait_event(wq, condition, state, exclusive, ret, cmd) \
({ \
__label__ __out; \
DEFINE_WAIT(__wait); \
long __ret = ret; \
\
for (;;) { \
if (exclusive) \
prepare_to_wait_exclusive(&wq, &__wait, state); \
else \
prepare_to_wait(&wq, &__wait, state); \
\
if (condition) \
break; \
\
if (___wait_signal_pending(state)) { \
__ret = -ERESTARTSYS; \
if (exclusive) { \
abort_exclusive_wait(&wq, &__wait, \
state, NULL); \
goto __out; \
} \
break; \
} \
\
cmd; \
} \
finish_wait(&wq, &__wait); \
__out: __ret; \
})
So wait_event_timeout(wq, true, 0) turns into:
({
DEFINE_WAIT(__wait);
long __ret = 0;
for (;;) {
prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE);
if (({
bool __cond = (true);
if (__cond && !__ret)
__ret = 1;
__cond || !__ret;
}))
break;
schedule();
}
finish_wait(&wq, &__wait);
__ret;
})
Which; afaict, returns 1, not 0.
> +#define ___wait_schedule_timeout(tout) \
> + if (!tout) \
> + break; \
> + tout = schedule_timeout(tout)
> +
> ___wait_event(wq, ___wait_cond_timeout(condition), \
> TASK_INTERRUPTIBLE, 0, ret, \
> spin_unlock_irq(&lock); \
> - __ret = schedule_timeout(__ret); \
> + ___wait_schedule_timeout(__ret); \
> spin_lock_irq(&lock));
You can't do that; you'll break/return without the lock held.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4
2013-09-30 18:09 ` Peter Zijlstra
@ 2013-09-30 18:13 ` Oleg Nesterov
2013-10-01 14:09 ` Oleg Nesterov
0 siblings, 1 reply; 23+ messages in thread
From: Oleg Nesterov @ 2013-09-30 18:13 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel
On 09/30, Peter Zijlstra wrote:
>
> On Mon, Sep 30, 2013 at 07:40:54PM +0200, Oleg Nesterov wrote:
> > Once again, of course I do not blame this series, but
> > wait_event_timeout(wq, true, 0) still returns 0.
>
> So we have:
>
> [...snip...]
> So wait_event_timeout(wq, true, 0) turns into:
Not really, because of fast-path check,
#define wait_event_timeout(wq, condition, timeout) \
({ \
long __ret = timeout; \
if (!(condition)) \
__ret = __wait_event_timeout(wq, condition, timeout); \
__ret; \
})
we do not even call __wait_event_timeout() if "condition" is already
true at the start.
> > ___wait_event(wq, ___wait_cond_timeout(condition), \
> > TASK_INTERRUPTIBLE, 0, ret, \
> > spin_unlock_irq(&lock); \
> > - __ret = schedule_timeout(__ret); \
> > + ___wait_schedule_timeout(__ret); \
> > spin_lock_irq(&lock));
>
> You can't do that; you'll break/return without the lock held.
Yes, see another email, already noticed this.
And let me repeat just in case, I think this series is fine, just
wait.h needs another minor/orthogonal fix.
Oleg.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4
2013-09-30 18:13 ` Oleg Nesterov
@ 2013-10-01 14:09 ` Oleg Nesterov
2013-10-01 14:39 ` Peter Zijlstra
0 siblings, 1 reply; 23+ messages in thread
From: Oleg Nesterov @ 2013-10-01 14:09 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel
On 09/30, Oleg Nesterov wrote:
>
> On 09/30, Peter Zijlstra wrote:
> >
> > On Mon, Sep 30, 2013 at 07:40:54PM +0200, Oleg Nesterov wrote:
> > > Once again, of course I do not blame this series, but
> > > wait_event_timeout(wq, true, 0) still returns 0.
> >
> > So we have:
> >
> > [...snip...]
>
> > So wait_event_timeout(wq, true, 0) turns into:
>
> Not really, because of fast-path check,
>
> #define wait_event_timeout(wq, condition, timeout) \
> ({ \
> long __ret = timeout; \
> if (!(condition)) \
> __ret = __wait_event_timeout(wq, condition, timeout); \
> __ret; \
> })
>
> we do not even call __wait_event_timeout() if "condition" is already
> true at the start.
But somehow I didn't realize that ___wait_cond_timeout() can be used
as is, so the simple patch below should work?
Oleg.
--- x/include/linux/wait.h
+++ x/include/linux/wait.h
@@ -270,7 +270,7 @@ do { \
#define wait_event_timeout(wq, condition, timeout) \
({ \
long __ret = timeout; \
- if (!(condition)) \
+ if (!___wait_cond_timeout(condition)) \
__ret = __wait_event_timeout(wq, condition, timeout); \
__ret; \
})
@@ -328,7 +328,7 @@ do { \
#define wait_event_interruptible_timeout(wq, condition, timeout) \
({ \
long __ret = timeout; \
- if (!(condition)) \
+ if (!___wait_cond_timeout(condition)) \
__ret = __wait_event_interruptible_timeout(wq, \
condition, timeout); \
__ret; \
@@ -770,7 +770,7 @@ do { \
timeout) \
({ \
long __ret = timeout; \
- if (!(condition)) \
+ if (!___wait_cond_timeout(condition)) \
__ret = __wait_event_interruptible_lock_irq_timeout( \
wq, condition, lock, timeout); \
__ret; \
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4
2013-10-01 14:09 ` Oleg Nesterov
@ 2013-10-01 14:39 ` Peter Zijlstra
2013-10-01 15:16 ` Oleg Nesterov
0 siblings, 1 reply; 23+ messages in thread
From: Peter Zijlstra @ 2013-10-01 14:39 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel
On Tue, Oct 01, 2013 at 04:09:40PM +0200, Oleg Nesterov wrote:
> But somehow I didn't realize that ___wait_cond_timeout() can be used
> as is, so the simple patch below should work?
Yeah, should work.. But how often do people use timeout=0? Should we
really care about that case to the effect of adding more code.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4
2013-10-01 14:39 ` Peter Zijlstra
@ 2013-10-01 15:16 ` Oleg Nesterov
0 siblings, 0 replies; 23+ messages in thread
From: Oleg Nesterov @ 2013-10-01 15:16 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, linux-kernel
On 10/01, Peter Zijlstra wrote:
>
> On Tue, Oct 01, 2013 at 04:09:40PM +0200, Oleg Nesterov wrote:
> > But somehow I didn't realize that ___wait_cond_timeout() can be used
> > as is, so the simple patch below should work?
>
> Yeah, should work.. But how often do people use timeout=0?
I do not know. Perhaps never.
> Should we
> really care about that case to the effect of adding more code.
Again, I do not really know. But imo it would be better to fix
this anyway, even if the problem is really minor. If nothing
else, wait_event_timeout() and __wait_event_timeout() should have
the same semantics.
And suppose that we ha a helper(timeout) which calls wait_event_timeout(),
and checks the non-trivial condition inside. Now suppose that someone
does
timeout = DEFAULT_TIMEOUT;
if (option_nonblock)
timeout = 0;
ok = helper(timeout);
So do you think we should ignore this or I should send 7/6 with the
changelog ?
(In fact I am going to send another patch on top of this series later.
At least, try to send for discussion because I know you dislike the
idea to move the signal-pending checks out of line).
Oleg.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC] introduce prepare_to_wait_event()
2013-09-30 15:22 [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Peter Zijlstra
` (7 preceding siblings ...)
2013-09-30 17:40 ` Oleg Nesterov
@ 2013-10-01 17:01 ` Oleg Nesterov
2013-10-01 17:25 ` Peter Zijlstra
8 siblings, 1 reply; 23+ messages in thread
From: Oleg Nesterov @ 2013-10-01 17:01 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Linus Torvalds
Cc: Paul McKenney, Thomas Gleixner, Andrew Morton, linux-kernel
I didn't test is yet, and I did write the changelog.
But let me show the patch for review right now, before we all
forget the details... On top of Peter's "wait: Collapse __wait_event
macros -v4".
This patch moves the signal-pending checks and part of DEFINE_WAIT's
code into the new helper: prepare_to_wait_event().
Yes, sure, prepare_to_wait_event() becomes a little bit slower than
prepare_to_wait/prepare_to_wait_exclusive. But this is the slow path
anyway, we are likely going to sleep. IMO, it is better to shrink
.text, and on my build the difference is
- 5124686 2955056 10117120 18196862 115a97e vmlinux
+ 5123212 2955088 10117120 18195420 115a3dc vmlinux
The code with the patch is
#define ___wait_is_interruptible(state) \
(!__builtin_constant_p(state) || \
state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE) \
#define ___wait_event(wq, condition, state, exclusive, ret, cmd) \
({ \
__label__ __out; \
wait_queue_t __wait; \
long __ret = ret; \
\
INIT_LIST_HEAD(&__wait.task_list); \
if (exclusive) \
__wait.flags = WQ_FLAG_EXCLUSIVE; \
else \
__wait.flags = 0; \
\
for (;;) { \
long intr = prepare_to_wait_event(&wq, &__wait, state); \
\
if (condition) \
break; \
\
if (___wait_is_interruptible(state) && intr) { \
__ret = intr; \
if (exclusive) { \
abort_exclusive_wait(&wq, &__wait, \
state, NULL); \
goto __out; \
} \
break; \
} \
\
cmd; \
} \
finish_wait(&wq, &__wait); \
__out: __ret; \
})
Compiler should optimize out "long intr" if !interruptible/killable.
What do you think?
Oleg.
---
diff --git a/include/linux/wait.h b/include/linux/wait.h
index bd4bd7b..aa758d3 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -187,27 +187,30 @@ wait_queue_head_t *bit_waitqueue(void *, int);
__cond || !__ret; \
})
-#define ___wait_signal_pending(state) \
- ((state == TASK_INTERRUPTIBLE && signal_pending(current)) || \
- (state == TASK_KILLABLE && fatal_signal_pending(current)))
+#define ___wait_is_interruptible(state) \
+ (!__builtin_constant_p(state) || \
+ state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE) \
#define ___wait_event(wq, condition, state, exclusive, ret, cmd) \
({ \
__label__ __out; \
- DEFINE_WAIT(__wait); \
+ wait_queue_t __wait; \
long __ret = ret; \
\
+ INIT_LIST_HEAD(&__wait.task_list); \
+ if (exclusive) \
+ __wait.flags = WQ_FLAG_EXCLUSIVE; \
+ else \
+ __wait.flags = 0; \
+ \
for (;;) { \
- if (exclusive) \
- prepare_to_wait_exclusive(&wq, &__wait, state); \
- else \
- prepare_to_wait(&wq, &__wait, state); \
+ long intr = prepare_to_wait_event(&wq, &__wait, state); \
\
if (condition) \
break; \
\
- if (___wait_signal_pending(state)) { \
- __ret = -ERESTARTSYS; \
+ if (___wait_is_interruptible(state) && intr) { \
+ __ret = intr; \
if (exclusive) { \
abort_exclusive_wait(&wq, &__wait, \
state, NULL); \
@@ -794,6 +797,7 @@ extern long interruptible_sleep_on_timeout(wait_queue_head_t *q,
*/
void prepare_to_wait(wait_queue_head_t *q, wait_queue_t *wait, int state);
void prepare_to_wait_exclusive(wait_queue_head_t *q, wait_queue_t *wait, int state);
+long prepare_to_wait_event(wait_queue_head_t *q, wait_queue_t *wait, int state);
void finish_wait(wait_queue_head_t *q, wait_queue_t *wait);
void abort_exclusive_wait(wait_queue_head_t *q, wait_queue_t *wait,
unsigned int mode, void *key);
diff --git a/kernel/wait.c b/kernel/wait.c
index d550920..de21c63 100644
--- a/kernel/wait.c
+++ b/kernel/wait.c
@@ -92,6 +92,30 @@ prepare_to_wait_exclusive(wait_queue_head_t *q, wait_queue_t *wait, int state)
}
EXPORT_SYMBOL(prepare_to_wait_exclusive);
+long prepare_to_wait_event(wait_queue_head_t *q, wait_queue_t *wait, int state)
+{
+ unsigned long flags;
+
+ if (signal_pending_state(state, current))
+ return -ERESTARTSYS;
+
+ wait->private = current;
+ wait->func = autoremove_wake_function;
+
+ spin_lock_irqsave(&q->lock, flags);
+ if (list_empty(&wait->task_list)) {
+ if (wait->flags & WQ_FLAG_EXCLUSIVE)
+ __add_wait_queue_tail(q, wait);
+ else
+ __add_wait_queue(q, wait);
+ }
+ set_current_state(state);
+ spin_unlock_irqrestore(&q->lock, flags);
+
+ return 0;
+}
+EXPORT_SYMBOL(prepare_to_wait_event);
+
/**
* finish_wait - clean up after waiting in a queue
* @q: waitqueue waited on
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [RFC] introduce prepare_to_wait_event()
2013-10-01 17:01 ` [RFC] introduce prepare_to_wait_event() Oleg Nesterov
@ 2013-10-01 17:25 ` Peter Zijlstra
2013-10-01 17:33 ` Oleg Nesterov
0 siblings, 1 reply; 23+ messages in thread
From: Peter Zijlstra @ 2013-10-01 17:25 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Linus Torvalds, Paul McKenney, Thomas Gleixner,
Andrew Morton, linux-kernel
On Tue, Oct 01, 2013 at 07:01:37PM +0200, Oleg Nesterov wrote:
> This patch moves the signal-pending checks and part of DEFINE_WAIT's
> code into the new helper: prepare_to_wait_event().
>
> Yes, sure, prepare_to_wait_event() becomes a little bit slower than
> prepare_to_wait/prepare_to_wait_exclusive. But this is the slow path
> anyway, we are likely going to sleep. IMO, it is better to shrink
> .text, and on my build the difference is
>
> - 5124686 2955056 10117120 18196862 115a97e vmlinux
> + 5123212 2955088 10117120 18195420 115a3dc vmlinux
>
> The code with the patch is
>
> #define ___wait_is_interruptible(state) \
> (!__builtin_constant_p(state) || \
> state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE) \
>
> #define ___wait_event(wq, condition, state, exclusive, ret, cmd) \
> ({ \
> __label__ __out; \
> wait_queue_t __wait; \
> long __ret = ret; \
> \
> INIT_LIST_HEAD(&__wait.task_list); \
> if (exclusive) \
> __wait.flags = WQ_FLAG_EXCLUSIVE; \
> else \
> __wait.flags = 0; \
__wait.flags = exclusive * WQ_FLAG_EXCLUSIVE;
or is that too obscure? ;-)
> \
> for (;;) { \
> long intr = prepare_to_wait_event(&wq, &__wait, state); \
int __intr = ...;
The interruptible bit doesn't actually need long; and local variables
have __ prefixes in this context.
> \
> if (condition) \
> break; \
> \
> if (___wait_is_interruptible(state) && intr) { \
> __ret = intr; \
> if (exclusive) { \
> abort_exclusive_wait(&wq, &__wait, \
> state, NULL); \
> goto __out; \
> } \
> break; \
> } \
> \
> cmd; \
> } \
> finish_wait(&wq, &__wait); \
> __out: __ret; \
> })
>
> Compiler should optimize out "long intr" if !interruptible/killable.
Yeah, and I think even the if (0 && __intr) would suffice for the unused
check; otherwise we'd have to adorn the thing with __maybe_unused.
> What do you think?
That would actually work I think.. the ___wait_is_interruptible() nicely
does away with the unused code; the only slightly more expensive thing
would be the prepare_to_wait_event() thing.
And if that really turns out to be a problem we could even re-use
___wait_is_interruptible() to call prepare_to_wait() instead.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [RFC] introduce prepare_to_wait_event()
2013-10-01 17:25 ` Peter Zijlstra
@ 2013-10-01 17:33 ` Oleg Nesterov
2013-10-01 17:44 ` Peter Zijlstra
0 siblings, 1 reply; 23+ messages in thread
From: Oleg Nesterov @ 2013-10-01 17:33 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Linus Torvalds, Paul McKenney, Thomas Gleixner,
Andrew Morton, linux-kernel
On 10/01, Peter Zijlstra wrote:
>
> On Tue, Oct 01, 2013 at 07:01:37PM +0200, Oleg Nesterov wrote:
> > if (exclusive) \
> > __wait.flags = WQ_FLAG_EXCLUSIVE; \
> > else \
> > __wait.flags = 0; \
>
> __wait.flags = exclusive * WQ_FLAG_EXCLUSIVE;
>
> or is that too obscure? ;-)
I do not mind ;) The generated code should be the same.
> > for (;;) { \
> > long intr = prepare_to_wait_event(&wq, &__wait, state); \
>
> int __intr = ...;
>
> The interruptible bit doesn't actually need long;
Yes, it can be even "bool", but see below.
> and local variables
> have __ prefixes in this context.
Yes, yes, will fix.
> > if (condition) \
> > break; \
> > \
> > if (___wait_is_interruptible(state) && intr) { \
> > __ret = intr; \
Since typeof(__ret) == typeof(intr) gcc can (likely) simply do "mov r1, r2",
so "long intr" make the code better.
I am not saying that "int intr" can make it worse, but to me "long" looks
better in this context. But I wouldn't mind to change this.
> > Compiler should optimize out "long intr" if !interruptible/killable.
>
> Yeah, and I think even the if (0 && __intr) would suffice for the unused
> check; otherwise we'd have to adorn the thing with __maybe_unused.
Hmm yes, I didn't see any warning during the compilation, but perhaps
__maybe_unused is needed, thanks.
> > What do you think?
>
> That would actually work I think.. the ___wait_is_interruptible() nicely
> does away with the unused code; the only slightly more expensive thing
> would be the prepare_to_wait_event() thing.
>
> And if that really turns out to be a problem we could even re-use
> ___wait_is_interruptible() to call prepare_to_wait() instead.
OK, thanks.
So I'll wait until your series is applied the resend it officially.
Oleg.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [RFC] introduce prepare_to_wait_event()
2013-10-01 17:33 ` Oleg Nesterov
@ 2013-10-01 17:44 ` Peter Zijlstra
0 siblings, 0 replies; 23+ messages in thread
From: Peter Zijlstra @ 2013-10-01 17:44 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Linus Torvalds, Paul McKenney, Thomas Gleixner,
Andrew Morton, linux-kernel
On Tue, Oct 01, 2013 at 07:33:37PM +0200, Oleg Nesterov wrote:
> > > if (___wait_is_interruptible(state) && intr) { \
> > > __ret = intr; \
>
> Since typeof(__ret) == typeof(intr) gcc can (likely) simply do "mov r1, r2",
> so "long intr" make the code better.
ok, fair enough; keep it long.
^ permalink raw reply [flat|nested] 23+ messages in thread