* Re: [PATCH 6/6] sched, wait: Make the __wait_event*() interface more friendly
[not found] ` <20130927161852.137894317@infradead.org>
@ 2013-09-27 17:08 ` Peter Zijlstra
0 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2013-09-27 17:08 UTC (permalink / raw)
To: Ingo Molnar
Cc: Oleg Nesterov, Paul McKenney, Linus Torvalds, Thomas Gleixner,
Andrew Morton, netfilter-devel, pablo
On Fri, Sep 27, 2013 at 06:15:07PM +0200, Peter Zijlstra wrote:
> +++ 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;
> }
That site seems to be ignoring the interruptible state... seems wrong.
Pablo?
^ permalink raw reply [flat|nested] 6+ 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
@ 2013-09-30 15:22 ` Peter Zijlstra
2013-10-01 6:39 ` Ingo Molnar
0 siblings, 1 reply; 6+ 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] 6+ 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; 6+ 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] 6+ messages in thread
* Re: [PATCH 6/6] sched, wait: Make the __wait_event*() interface more friendly
@ 2013-10-01 8:00 Julian Anastasov
2013-10-01 17:19 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: Julian Anastasov @ 2013-10-01 8:00 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Oleg Nesterov, Paul McKenney, Linus Torvalds,
Thomas Gleixner, Andrew Morton, netfilter-devel, pablo
Hello,
> On Fri, Sep 27, 2013 at 06:15:07PM +0200, Peter Zijlstra wrote:
> > +++ 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;
> > }
>
> That site seems to be ignoring the interruptible state... seems wrong.
This is a kthread which ignores signals by default.
It terminates only with kthread_stop. I hope it works
correctly for such context. This is a loop that polls socket
space for writing when there is a buffer for sending.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 6/6] sched, wait: Make the __wait_event*() interface more friendly
2013-10-01 8:00 [PATCH 6/6] sched, wait: Make the __wait_event*() interface more friendly Julian Anastasov
@ 2013-10-01 17:19 ` Oleg Nesterov
2013-10-01 19:58 ` Julian Anastasov
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2013-10-01 17:19 UTC (permalink / raw)
To: Julian Anastasov
Cc: Peter Zijlstra, Ingo Molnar, Paul McKenney, Linus Torvalds,
Thomas Gleixner, Andrew Morton, netfilter-devel, pablo
On 10/01, Julian Anastasov wrote:
>
> Hello,
>
> > On Fri, Sep 27, 2013 at 06:15:07PM +0200, Peter Zijlstra wrote:
> > > +++ 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;
> > > }
> >
> > That site seems to be ignoring the interruptible state... seems wrong.
>
> This is a kthread which ignores signals by default.
> It terminates only with kthread_stop.
This only means that (with or without this change)
__wait_event_interruptible() generates the dead code but this
thread doesn't contribute to load_avg.
Anyway this patch should not makes any difference.
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 6/6] sched, wait: Make the __wait_event*() interface more friendly
2013-10-01 17:19 ` Oleg Nesterov
@ 2013-10-01 19:58 ` Julian Anastasov
0 siblings, 0 replies; 6+ messages in thread
From: Julian Anastasov @ 2013-10-01 19:58 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Peter Zijlstra, Ingo Molnar, Paul McKenney, Linus Torvalds,
Thomas Gleixner, Andrew Morton, netfilter-devel, pablo
Hello,
On Tue, 1 Oct 2013, Oleg Nesterov wrote:
> On 10/01, Julian Anastasov wrote:
> >
> > Hello,
> >
> > > On Fri, Sep 27, 2013 at 06:15:07PM +0200, Peter Zijlstra wrote:
> > > > +++ 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;
> > > > }
> > >
> > > That site seems to be ignoring the interruptible state... seems wrong.
> >
> > This is a kthread which ignores signals by default.
> > It terminates only with kthread_stop.
>
> This only means that (with or without this change)
> __wait_event_interruptible() generates the dead code but this
> thread doesn't contribute to load_avg.
>
> Anyway this patch should not makes any difference.
Yes, your patch looks ok to me. In the past
we used ssleep() but IPVS users were confused why
IPVS threads increase the load average. So, we
switched to _interruptible calls and later the socket
polling was added.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-01 19:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-01 8:00 [PATCH 6/6] sched, wait: Make the __wait_event*() interface more friendly Julian Anastasov
2013-10-01 17:19 ` Oleg Nesterov
2013-10-01 19:58 ` Julian Anastasov
-- strict thread matches above, loose matches on Subject: below --
2013-09-30 15:22 [PATCH 0/6] sched, wait: Collapse __wait_event macros -v4 Peter Zijlstra
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
[not found] <20130927161501.323694767@infradead.org>
[not found] ` <20130927161852.137894317@infradead.org>
2013-09-27 17:08 ` Peter Zijlstra
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.