[PATCH 0/2] introduce wait_event_common()

[PATCH 0/2] introduce wait_event_common()

[Oleg Nesterov]

Hello.

To remind, I think that 4c663cfc "wait: fix false timeouts when using
wait_event_timeout()" is not enough, wait(wq, true, 0) still returns
zero.

But to me the main problem is that wait_event* macros duplicate the
same code again and again. Imho it would be nice to create a single
helper. To simplify the review, this is the code after 1/2:

	#define __wait_no_timeout(tout)	\
		(__builtin_constant_p(tout) && (tout) == MAX_SCHEDULE_TIMEOUT)

	/* uglified signal_pending_state() optimized for constant state */
	#define __wait_signal_pending(state)					\
		((state == TASK_INTERRUPTIBLE) ? signal_pending(current) :	\
		 (state == TASK_KILLABLE) ? fatal_signal_pending(current) :	\
		  0)

	#define __wait_event_common(wq, condition, state, tout)			\
	({									\
		DEFINE_WAIT(__wait);						\
		long __ret = 0, __tout = tout;					\
										\
		for (;;) {							\
			prepare_to_wait(&wq, &__wait, state);			\
			if (condition) {					\
				__ret = __wait_no_timeout(tout) ?: __tout ?: 1;	\
				break;						\
			}							\
										\
			if (__wait_signal_pending(state)) {			\
				__ret = -ERESTARTSYS;				\
				break;						\
			}							\
										\
			if (__wait_no_timeout(tout))				\
				schedule();					\
			else if (__tout)					\
				__tout = schedule_timeout(__tout);		\
			else							\
				break;						\
		}								\
		finish_wait(&wq, &__wait);					\
		__ret;								\
	})

	#define wait_event_common(wq, condition, state, tout)			\
	({									\
		long __ret;							\
		if (condition)							\
			__ret = __wait_no_timeout(tout) ?: (tout) ?: 1;		\
		else								\
			__ret = __wait_event_common(wq, condition, state, tout);\
		__ret;								\
	})

2/2 doesn't look like a cleanup. But personally I think that it makes
sense to shrink .text,

	-	4977769 2930984 10104832        18013585        112dd91 vmlinux
	+	4976847	2930984	10104832	18012663	112d9f7	vmlinux

on my build.

Please comment.

Oleg.

[PATCH 1/2] wait: introduce wait_event_common(wq, condition, state, timeout)

[Oleg Nesterov]

1. wait_event_timeout(wq, true, 0) returns zero, I think this is
   wrong and should be fixed.

   __wait_event_timeout() was already changed by 4c663cfc but we
   need the same logic in wait_event_timeout() if the fast-path
   check succeeds.

2. wait_event_timeout/__wait_event_timeout interface do not match
   wait_event(), you can't use __wait_event_timeout() instead of
   wait_event_timeout() if you do not need the fast-path check.

3. wait_event_* macros duplicate the same code.

This patch adds a single helper wait_event_common() which hopefully
does everything right. Compiler optimizes out the "dead" code when
we do not need signal_pending/schedule_timeout.

With this patch "size vmlinux" reports that .text/data shrinks but
I think this depends on gcc/config.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Tejun Heo <tj@kernel.org>
---
 include/linux/wait.h |  181 +++++++++++++++++++------------------------------
 1 files changed, 70 insertions(+), 111 deletions(-)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 1133695..392c54d 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -173,18 +173,52 @@ wait_queue_head_t *bit_waitqueue(void *, int);
 #define wake_up_interruptible_sync_poll(x, m)				\
 	__wake_up_sync_key((x), TASK_INTERRUPTIBLE, 1, (void *) (m))
 
-#define __wait_event(wq, condition) 					\
-do {									\
+#define __wait_no_timeout(tout)	\
+	(__builtin_constant_p(tout) && (tout) == MAX_SCHEDULE_TIMEOUT)
+
+/* uglified signal_pending_state() optimized for constant state */
+#define __wait_signal_pending(state)					\
+	((state == TASK_INTERRUPTIBLE) ? signal_pending(current) :	\
+	 (state == TASK_KILLABLE) ? fatal_signal_pending(current) :	\
+	  0)
+
+#define __wait_event_common(wq, condition, state, tout)			\
+({									\
 	DEFINE_WAIT(__wait);						\
+	long __ret = 0, __tout = tout;					\
 									\
 	for (;;) {							\
-		prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE);	\
-		if (condition)						\
+		prepare_to_wait(&wq, &__wait, state);			\
+		if (condition) {					\
+			__ret = __wait_no_timeout(tout) ?: __tout ?: 1;	\
+			break;						\
+		}							\
+									\
+		if (__wait_signal_pending(state)) {			\
+			__ret = -ERESTARTSYS;				\
+			break;						\
+		}							\
+									\
+		if (__wait_no_timeout(tout))				\
+			schedule();					\
+		else if (__tout)					\
+			__tout = schedule_timeout(__tout);		\
+		else							\
 			break;						\
-		schedule();						\
 	}								\
 	finish_wait(&wq, &__wait);					\
-} while (0)
+	__ret;								\
+})
+
+#define wait_event_common(wq, condition, state, tout)			\
+({									\
+	long __ret;							\
+	if (condition)							\
+		__ret = __wait_no_timeout(tout) ?: (tout) ?: 1;		\
+	else								\
+		__ret = __wait_event_common(wq, condition, state, tout);\
+	__ret;								\
+})
 
 /**
  * wait_event - sleep until a condition gets true
@@ -198,29 +232,13 @@ do {									\
  * wake_up() has to be called after changing any variable that could
  * change the result of the wait condition.
  */
-#define wait_event(wq, condition) 					\
-do {									\
-	if (condition)	 						\
-		break;							\
-	__wait_event(wq, condition);					\
-} while (0)
+#define __wait_event(wq, condition) 					\
+	__wait_event_common(wq, condition,				\
+			TASK_UNINTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT)	\
 
-#define __wait_event_timeout(wq, condition, ret)			\
-do {									\
-	DEFINE_WAIT(__wait);						\
-									\
-	for (;;) {							\
-		prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE);	\
-		if (condition)						\
-			break;						\
-		ret = schedule_timeout(ret);				\
-		if (!ret)						\
-			break;						\
-	}								\
-	if (!ret && (condition))					\
-		ret = 1;						\
-	finish_wait(&wq, &__wait);					\
-} while (0)
+#define wait_event(wq, condition) 					\
+	wait_event_common(wq, condition,				\
+			TASK_UNINTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT)	\
 
 /**
  * wait_event_timeout - sleep until a condition gets true or a timeout elapses
@@ -239,31 +257,13 @@ do {									\
  * jiffies (at least 1) if the @condition evaluated to %true before
  * the @timeout elapsed.
  */
-#define wait_event_timeout(wq, condition, timeout)			\
-({									\
-	long __ret = timeout;						\
-	if (!(condition)) 						\
-		__wait_event_timeout(wq, condition, __ret);		\
-	__ret;								\
-})
+#define __wait_event_timeout(wq, condition, timeout)			\
+	__wait_event_common(wq, condition,				\
+			TASK_UNINTERRUPTIBLE, timeout)
 
-#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)) {				\
-			schedule();					\
-			continue;					\
-		}							\
-		ret = -ERESTARTSYS;					\
-		break;							\
-	}								\
-	finish_wait(&wq, &__wait);					\
-} while (0)
+#define wait_event_timeout(wq, condition, timeout)			\
+	wait_event_common(wq, condition,				\
+			TASK_UNINTERRUPTIBLE, timeout)
 
 /**
  * wait_event_interruptible - sleep until a condition gets true
@@ -280,35 +280,13 @@ do {									\
  * The function will return -ERESTARTSYS if it was interrupted by a
  * signal and 0 if @condition evaluated to true.
  */
-#define wait_event_interruptible(wq, condition)				\
-({									\
-	int __ret = 0;							\
-	if (!(condition))						\
-		__wait_event_interruptible(wq, condition, __ret);	\
-	__ret;								\
-})
+#define __wait_event_interruptible(wq, condition)			\
+	__wait_event_common(wq, condition,				\
+			TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT)
 
-#define __wait_event_interruptible_timeout(wq, condition, ret)		\
-do {									\
-	DEFINE_WAIT(__wait);						\
-									\
-	for (;;) {							\
-		prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);	\
-		if (condition)						\
-			break;						\
-		if (!signal_pending(current)) {				\
-			ret = schedule_timeout(ret);			\
-			if (!ret)					\
-				break;					\
-			continue;					\
-		}							\
-		ret = -ERESTARTSYS;					\
-		break;							\
-	}								\
-	if (!ret && (condition))					\
-		ret = 1;						\
-	finish_wait(&wq, &__wait);					\
-} while (0)
+#define wait_event_interruptible(wq, condition)				\
+	wait_event_common(wq, condition,				\
+			TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT)
 
 /**
  * wait_event_interruptible_timeout - sleep until a condition gets true or a timeout elapses
@@ -328,13 +306,13 @@ do {									\
  * a signal, or the remaining jiffies (at least 1) if the @condition
  * evaluated to %true before the @timeout elapsed.
  */
+#define __wait_event_interruptible_timeout(wq, condition, timeout)	\
+	__wait_event_common(wq, condition,				\
+			TASK_INTERRUPTIBLE, timeout)
+
 #define wait_event_interruptible_timeout(wq, condition, timeout)	\
-({									\
-	long __ret = timeout;						\
-	if (!(condition))						\
-		__wait_event_interruptible_timeout(wq, condition, __ret); \
-	__ret;								\
-})
+	wait_event_common(wq, condition,				\
+			TASK_INTERRUPTIBLE, timeout)
 
 #define __wait_event_hrtimeout(wq, condition, timeout, state)		\
 ({									\
@@ -601,24 +579,6 @@ do {									\
 
 
 
-#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_killable - sleep until a condition gets true
  * @wq: the waitqueue to wait on
@@ -634,14 +594,13 @@ do {									\
  * The function will return -ERESTARTSYS if it was interrupted by a
  * signal and 0 if @condition evaluated to true.
  */
-#define wait_event_killable(wq, condition)				\
-({									\
-	int __ret = 0;							\
-	if (!(condition))						\
-		__wait_event_killable(wq, condition, __ret);		\
-	__ret;								\
-})
+#define __wait_event_killable(wq, condition)				\
+	__wait_event_common(wq, condition,				\
+			TASK_KILLABLE, MAX_SCHEDULE_TIMEOUT)
 
+#define wait_event_killable(wq, condition)				\
+	wait_event_common(wq, condition,				\
+			TASK_KILLABLE, MAX_SCHEDULE_TIMEOUT)
 
 #define __wait_event_lock_irq(wq, condition, lock, cmd)			\
 do {									\
-- 
1.5.5.1

[PATCH 2/2] wait: introduce prepare_to_wait_event()

[Oleg Nesterov]

Add the new helper, prepare_to_wait_event() which should only be used
by wait_event_common/etc.

prepare_to_wait_event() returns -ERESTARTSYS if signal_pending_state()
is true, otherwise it calls prepare_to_wait(). This allows to uninline
the signal-pending checks in wait_event_*.

Also, it can initialize wait->private/func. We do not care they were
already initialized, the values are the same. This also shaves a couple
of insns from the inlined code.

Unlike the previous change, this patch "reliably" shrinks the size of
generated code for every wait_event*() call.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/wait.h |   22 +++++++++++-----------
 kernel/wait.c        |   14 ++++++++++++++
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 392c54d..a2314dd 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -176,28 +176,27 @@ wait_queue_head_t *bit_waitqueue(void *, int);
 #define __wait_no_timeout(tout)	\
 	(__builtin_constant_p(tout) && (tout) == MAX_SCHEDULE_TIMEOUT)
 
-/* uglified signal_pending_state() optimized for constant state */
-#define __wait_signal_pending(state)					\
-	((state == TASK_INTERRUPTIBLE) ? signal_pending(current) :	\
-	 (state == TASK_KILLABLE) ? fatal_signal_pending(current) :	\
-	  0)
+#define __wait_interruptible(state)					\
+	(!__builtin_constant_p(state) ||				\
+	 	state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE)
 
 #define __wait_event_common(wq, condition, state, tout)			\
 ({									\
-	DEFINE_WAIT(__wait);						\
-	long __ret = 0, __tout = tout;					\
+	long __ret, __tout = tout;					\
+	wait_queue_t __wait;						\
+									\
+	INIT_LIST_HEAD(&__wait.task_list);				\
+	__wait.flags = 0;						\
 									\
 	for (;;) {							\
-		prepare_to_wait(&wq, &__wait, state);			\
+		__ret = prepare_to_wait_event(&wq, &__wait, state);	\
 		if (condition) {					\
 			__ret = __wait_no_timeout(tout) ?: __tout ?: 1;	\
 			break;						\
 		}							\
 									\
-		if (__wait_signal_pending(state)) {			\
-			__ret = -ERESTARTSYS;				\
+		if (__wait_interruptible(state) && __ret)		\
 			break;						\
-		}							\
 									\
 		if (__wait_no_timeout(tout))				\
 			schedule();					\
@@ -781,6 +780,7 @@ extern long interruptible_sleep_on_timeout(wait_queue_head_t *q,
  * Waitqueues which are removed from the waitqueue_head at wakeup time
  */
 void prepare_to_wait(wait_queue_head_t *q, wait_queue_t *wait, int state);
+int prepare_to_wait_event(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);
 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,
diff --git a/kernel/wait.c b/kernel/wait.c
index 6698e0c..57cc6e7 100644
--- a/kernel/wait.c
+++ b/kernel/wait.c
@@ -78,6 +78,20 @@ prepare_to_wait(wait_queue_head_t *q, wait_queue_t *wait, int state)
 }
 EXPORT_SYMBOL(prepare_to_wait);
 
+int
+prepare_to_wait_event(wait_queue_head_t *q, wait_queue_t *wait, int state)
+{
+	if (signal_pending_state(state, current))
+		return -ERESTARTSYS;
+
+	wait->private = current;
+	wait->func = autoremove_wake_function;
+	prepare_to_wait(q, wait, state);
+
+	return 0;
+}
+EXPORT_SYMBOL(prepare_to_wait_event);
+
 void
 prepare_to_wait_exclusive(wait_queue_head_t *q, wait_queue_t *wait, int state)
 {
-- 
1.5.5.1

Re: [PATCH 2/2] wait: introduce prepare_to_wait_event()

[Tejun Heo]

Hello, Oleg.

On Thu, Jun 06, 2013 at 10:03:34PM +0200, Oleg Nesterov wrote:
> Add the new helper, prepare_to_wait_event() which should only be used
> by wait_event_common/etc.
> 
> prepare_to_wait_event() returns -ERESTARTSYS if signal_pending_state()
> is true, otherwise it calls prepare_to_wait(). This allows to uninline
> the signal-pending checks in wait_event_*.
> 
> Also, it can initialize wait->private/func. We do not care they were
> already initialized, the values are the same. This also shaves a couple
> of insns from the inlined code.
> 
> Unlike the previous change, this patch "reliably" shrinks the size of
> generated code for every wait_event*() call.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Which tree are the patches based on?  I'm getting conflicts on
linus#master, mmotd and next.

Reviewed-by: Tejun Heo <tj@kernel.org>

But a couple nits.

> +int

I don't think we need to keep the unnecessary line break here.  All
other functions in the file don't do it except for the two
prepare_to_wait functions.  No need to give the weirdos more power. :)

> +prepare_to_wait_event(wait_queue_head_t *q, wait_queue_t *wait, int state)
> +{
> +	if (signal_pending_state(state, current))
> +		return -ERESTARTSYS;
> +
> +	wait->private = current;
> +	wait->func = autoremove_wake_function;
> +	prepare_to_wait(q, wait, state);

I wonder whether it'd be worthwhile to make prepare_to_wait() inline
so that it can be inlined into the above.  I think gcc is smart enough
to emit inline for in-file stuff and then build a proper function for
external references.  No biggie.  Just wondering.

Thanks.

-- 
tejun

Re: [PATCH 2/2] wait: introduce prepare_to_wait_event()

[Oleg Nesterov]

Hi Tejun,

On 06/06, Tejun Heo wrote:
>
> Which tree are the patches based on?  I'm getting conflicts on
> linus#master, mmotd and next.

Hmm. linus#master. I just verified that the patches apply cleanly
after git-pull.

> Reviewed-by: Tejun Heo <tj@kernel.org>

Thanks,

> But a couple nits.
>
> > +int
>
> I don't think we need to keep the unnecessary line break here.  All
> other functions in the file don't do it except for the two
> prepare_to_wait functions.  No need to give the weirdos more power. :)

Yes, I tried to follow the style around the new helper, but the extra
line is not needed.

Please see v2 below, no other changes.

> > +prepare_to_wait_event(wait_queue_head_t *q, wait_queue_t *wait, int state)
> > +{
> > +	if (signal_pending_state(state, current))
> > +		return -ERESTARTSYS;
> > +
> > +	wait->private = current;
> > +	wait->func = autoremove_wake_function;
> > +	prepare_to_wait(q, wait, state);
>
> I wonder whether it'd be worthwhile to make prepare_to_wait() inline
> so that it can be inlined into the above.  I think gcc is smart enough
> to emit inline for in-file stuff and then build a proper function for
> external references.  No biggie.  Just wondering.

Not sure... As you can see from this patch, I like -Os more than -O2 ;)


------------------------------------------------------------------------------
Subject: [PATCH v2 2/2] wait: introduce prepare_to_wait_event()

Add the new helper, prepare_to_wait_event() which should only be used
by wait_event_common/etc.

prepare_to_wait_event() returns -ERESTARTSYS if signal_pending_state()
is true, otherwise it calls prepare_to_wait(). This allows to uninline
the signal-pending checks in wait_event_*.

Also, it can initialize wait->private/func. We do not care they were
already initialized, the values are the same. This also shaves a couple
of insns from the inlined code.

Unlike the previous change, this patch "reliably" shrinks the size of
generated code for every wait_event*() call.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Tejun Heo <tj@kernel.org>
---
 include/linux/wait.h |   22 +++++++++++-----------
 kernel/wait.c        |   13 +++++++++++++
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 392c54d..a2314dd 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -176,28 +176,27 @@ wait_queue_head_t *bit_waitqueue(void *, int);
 #define __wait_no_timeout(tout)	\
 	(__builtin_constant_p(tout) && (tout) == MAX_SCHEDULE_TIMEOUT)
 
-/* uglified signal_pending_state() optimized for constant state */
-#define __wait_signal_pending(state)					\
-	((state == TASK_INTERRUPTIBLE) ? signal_pending(current) :	\
-	 (state == TASK_KILLABLE) ? fatal_signal_pending(current) :	\
-	  0)
+#define __wait_interruptible(state)					\
+	(!__builtin_constant_p(state) ||				\
+	 	state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE)
 
 #define __wait_event_common(wq, condition, state, tout)			\
 ({									\
-	DEFINE_WAIT(__wait);						\
-	long __ret = 0, __tout = tout;					\
+	long __ret, __tout = tout;					\
+	wait_queue_t __wait;						\
+									\
+	INIT_LIST_HEAD(&__wait.task_list);				\
+	__wait.flags = 0;						\
 									\
 	for (;;) {							\
-		prepare_to_wait(&wq, &__wait, state);			\
+		__ret = prepare_to_wait_event(&wq, &__wait, state);	\
 		if (condition) {					\
 			__ret = __wait_no_timeout(tout) ?: __tout ?: 1;	\
 			break;						\
 		}							\
 									\
-		if (__wait_signal_pending(state)) {			\
-			__ret = -ERESTARTSYS;				\
+		if (__wait_interruptible(state) && __ret)		\
 			break;						\
-		}							\
 									\
 		if (__wait_no_timeout(tout))				\
 			schedule();					\
@@ -781,6 +780,7 @@ extern long interruptible_sleep_on_timeout(wait_queue_head_t *q,
  * Waitqueues which are removed from the waitqueue_head at wakeup time
  */
 void prepare_to_wait(wait_queue_head_t *q, wait_queue_t *wait, int state);
+int prepare_to_wait_event(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);
 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,
diff --git a/kernel/wait.c b/kernel/wait.c
index 6698e0c..3b8619a 100644
--- a/kernel/wait.c
+++ b/kernel/wait.c
@@ -78,6 +78,19 @@ prepare_to_wait(wait_queue_head_t *q, wait_queue_t *wait, int state)
 }
 EXPORT_SYMBOL(prepare_to_wait);
 
+int prepare_to_wait_event(wait_queue_head_t *q, wait_queue_t *wait, int state)
+{
+	if (signal_pending_state(state, current))
+		return -ERESTARTSYS;
+
+	wait->private = current;
+	wait->func = autoremove_wake_function;
+	prepare_to_wait(q, wait, state);
+
+	return 0;
+}
+EXPORT_SYMBOL(prepare_to_wait_event);
+
 void
 prepare_to_wait_exclusive(wait_queue_head_t *q, wait_queue_t *wait, int state)
 {
-- 
1.5.5.1

Re: [PATCH 1/2] wait: introduce wait_event_common(wq, condition, state, timeout)

[Andrew Morton]

On Thu, 6 Jun 2013 22:03:16 +0200 Oleg Nesterov <oleg@redhat.com> wrote:

> 1. wait_event_timeout(wq, true, 0) returns zero, I think this is
>    wrong and should be fixed.
> 
>    __wait_event_timeout() was already changed by 4c663cfc but we
>    need the same logic in wait_event_timeout() if the fast-path
>    check succeeds.
> 
> 2. wait_event_timeout/__wait_event_timeout interface do not match
>    wait_event(), you can't use __wait_event_timeout() instead of
>    wait_event_timeout() if you do not need the fast-path check.
> 
> 3. wait_event_* macros duplicate the same code.
> 
> This patch adds a single helper wait_event_common() which hopefully
> does everything right. Compiler optimizes out the "dead" code when
> we do not need signal_pending/schedule_timeout.
> 
> With this patch "size vmlinux" reports that .text/data shrinks but
> I think this depends on gcc/config.

hm,

> -#define __wait_event_interruptible(wq, condition, ret)
> +#define __wait_event_interruptible(wq, condition)

net/irda/af_irda.c:2568:13: error: macro "__wait_event_interruptible" passed 3 arguments, but takes just 2

waddup with that?

__wait_event_interruptible() has several callsites.  I think I'll go
zap and await v2 ;)

Re: [PATCH 1/2] wait: introduce wait_event_common(wq, condition, state, timeout)

[Oleg Nesterov]

On 06/18, Andrew Morton wrote:
>
> On Thu, 6 Jun 2013 22:03:16 +0200 Oleg Nesterov <oleg@redhat.com> wrote:
> > -#define __wait_event_interruptible(wq, condition, ret)
> > +#define __wait_event_interruptible(wq, condition)
>
> net/irda/af_irda.c:2568:13: error: macro "__wait_event_interruptible" passed 3 arguments, but takes just 2

Aaah. Thanks.

> waddup with that?

Will read "man grep" again.

> __wait_event_interruptible() has several callsites.  I think I'll go
> zap and await v2 ;)

Yes, will do, thanks.

And note that one of the reasons for this patch was to make
wait_event_interruptible() and __wait_event_interruptible()
interchangeable, like wait_event() and __wait_event().
irda_getsockopt() uses __wait* exactly because it has already
checked the condition.



Samuel, will you agree with the patch below?

Yes, this adds the unnecessary check. We will remove it later,
after we change wait_event* macros, just

	- err = wait_event_interruptible(...);
	+ err = __wait_event_interruptible(...);

Otherwise, this patch should also change af_irda.c (and probably
other callers), not good.

Oleg.

--- x/net/irda/af_irda.c
+++ x/net/irda/af_irda.c
@@ -2563,9 +2563,8 @@ bed:
 				  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));