[RFC PATCH] add wait_event_*_lock() functions

[RFC PATCH] add wait_event_*_lock() functions

[Nishanth Aravamudan]

Hi David, LKML,

It came up on IRC that the wait_cond*() functions from
usb/serial/gadget.c could be useful in other parts of the kernel. Does
the following patch make sense towards this? I did not add corresponding
wait_event_exclusive() macros, as I don't think they would be used, but
that is a simple addition, if it would be desired for completeness.
I would greatly appreciate any input. If the patch (in this form or in a
later one) is acceptable, then we can remove the definitions from
usb/serial/gadget.c.

Description: The following patch attempts to make the wait_cond*()
functions from usb/serial/gadget.c, which are basically the same
as wait_event*() but with locks, globally available via wait.h.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>

--- 2.6.11-rc3-v/include/linux/wait.h	2004-12-24 13:34:57.000000000 -0800
+++ 2.6.11-rc3/include/linux/wait.h	2005-02-09 11:02:08.000000000 -0800
@@ -176,6 +176,28 @@
 	__wait_event(wq, condition);					\
 } while (0)
 
+#define __wait_event_lock(wq, condition, lock, flags)			\
+do {									\
+	DEFINE_WAIT(__wait);						\
+									\
+	for (;;) {							\
+		prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE);	\
+		if (condition)						\
+			break;						\
+		spin_unlock_irqrestore(lock, flags);			\
+		schedule();						\
+		spin_lock_irqsave(lock, flags);				\
+	}								\
+	finish_wait(&wq, &__wait);					\
+} while (0)
+
+#define wait_event_lock(wq, condition, lock, flags)			\
+do {									\
+	if (condition)							\
+		break;							\
+	__wait_event_lock(wq, condition, lock, flags);			\
+} while (0)
+
 #define __wait_event_timeout(wq, condition, ret)			\
 do {									\
 	DEFINE_WAIT(__wait);						\
@@ -199,6 +221,31 @@
 	__ret;								\
 })
 
+#define __wait_event_timeout_lock(wq, condition, lock, flags, ret)	\
+do {									\
+	DEFINE_WAIT(__wait);						\
+									\
+	for (;;) {							\
+		prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE);	\
+		if (condition)						\
+			break;						\
+		spin_unlock_irqrestore(lock, flags);			\
+		ret = schedule_timeout(ret);				\
+		spin_lock_irqsave(lock, flags);				\
+		if (!ret)						\
+			break;						\
+	}								\
+	finish_wait(&wq, &__wait);					\
+} while (0)
+
+#define wait_event_timeout_lock(wq, condition, lock, flags, timeout)	\
+({									\
+	long __ret = timeout;						\
+	if (!(condition)) 						\
+		__wait_event_timeout_lock(wq, condition, lock, flags, __ret); \
+	__ret;								\
+})
+
 #define __wait_event_interruptible(wq, condition, ret)			\
 do {									\
 	DEFINE_WAIT(__wait);						\
@@ -225,6 +272,34 @@
 	__ret;								\
 })
 
+#define __wait_event_interruptible_lock(wq, condition, lock, flags, ret) \
+do {									\
+	DEFINE_WAIT(__wait);						\
+									\
+	for (;;) {							\
+		prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);	\
+		if (condition)						\
+			break;						\
+		if (!signal_pending(current)) {				\
+			spin_unlock_irqrestore(lock, flags)		\
+			schedule();					\
+			spin_lock_irqsave(lock, flags)			\
+			continue;					\
+		}							\
+		ret = -ERESTARTSYS;					\
+		break;							\
+	}								\
+	finish_wait(&wq, &__wait);					\
+} while (0)
+
+#define wait_event_interruptible_lock(wq, condition, lock, flags)	\
+({									\
+	int __ret = 0;							\
+	if (!(condition))						\
+		__wait_event_interruptible_lock(wq, condition, lock, flags, __ret); \
+	__ret;								\
+})
+
 #define __wait_event_interruptible_timeout(wq, condition, ret)		\
 do {									\
 	DEFINE_WAIT(__wait);						\
@@ -253,6 +328,36 @@
 	__ret;								\
 })
 
+#define __wait_event_interruptible_timeout_lock(wq, condition, lock, flags, ret) \
+do {									\
+	DEFINE_WAIT(__wait);						\
+									\
+	for (;;) {							\
+		prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE);	\
+		if (condition)						\
+			break;						\
+		if (!signal_pending(current)) {				\
+			spin_unlock_irqrestore(lock, flags);		\
+			ret = schedule_timeout(ret);			\
+			spin_lock_irqsave(lock, flags);			\
+			if (!ret)					\
+				break;					\
+			continue;					\
+		}							\
+		ret = -ERESTARTSYS;					\
+		break;							\
+	}								\
+	finish_wait(&wq, &__wait);					\
+} while (0)
+
+#define wait_event_interruptible_timeout_lock(wq, condition, lock, flags, timeout) \
+({									\
+	long __ret = timeout;						\
+	if (!(condition))						\
+		__wait_event_interruptible_timeout_lock(wq, condition, lock, flags, __ret); \
+	__ret;								\
+})
+
 #define __wait_event_interruptible_exclusive(wq, condition, ret)	\
 do {									\
 	DEFINE_WAIT(__wait);						\

Re: [RFC PATCH] add wait_event_*_lock() functions

[David Brownell]

On Thursday 10 February 2005 9:39 am, Nishanth Aravamudan wrote:
> Hi David, LKML,
> 
> It came up on IRC that the wait_cond*() functions from
> usb/serial/gadget.c could be useful in other parts of the kernel. Does
> the following patch make sense towards this? 

I know that Al Borchers -- who wrote those -- did so with that
specific notion.  And it certainly makes sense to me, in
principle, that such primitives exist in the kernel ... maybe
with some tweaks first.  (And docs for all the wait_* calls?)

But nobody's pressed the issue before, to the relevant audience:
namely, LKML.  I'd be interested to hear what other folk think.
Clearly these particular primitives don't understand how to cope
with nested spinlocks, but those are worth avoiding anyway.

- Dave

Re: [RFC PATCH] add wait_event_*_lock() functions

[Nishanth Aravamudan]

On Thu, Feb 10, 2005 at 10:21:58AM -0800, David Brownell wrote:
> On Thursday 10 February 2005 9:39 am, Nishanth Aravamudan wrote:
> > Hi David, LKML,
> > 
> > It came up on IRC that the wait_cond*() functions from
> > usb/serial/gadget.c could be useful in other parts of the kernel. Does
> > the following patch make sense towards this? 
> 
> I know that Al Borchers -- who wrote those -- did so with that
> specific notion.  And it certainly makes sense to me, in
> principle, that such primitives exist in the kernel ... maybe
> with some tweaks first.  (And docs for all the wait_* calls?)

I would be happy to document all the wait_* callers, especially when
which should be used, their correspondence to the other sleep-functions,
etc.

> But nobody's pressed the issue before, to the relevant audience:
> namely, LKML.  I'd be interested to hear what other folk think.
> Clearly these particular primitives don't understand how to cope
> with nested spinlocks, but those are worth avoiding anyway.

Yes, I was considering that issue, but I figured let's go for the simple
case now and that should be good enough for *most* cases.

Thanks for the feedback!

-Nish

Re: [RFC PATCH] add wait_event_*_lock() functions

[Al Borchers]

On Thursday 10 February 2005 9:39 am, Nishanth Aravamudan wrote:
>> It came up on IRC that the wait_cond*() functions from
>> usb/serial/gadget.c could be useful in other parts of the kernel. Does
>> the following patch make sense towards this?

Sure, if people want to use these.

I did not push them because they seemed a bit "heavy weight",
but the construct is useful and general.

The docs should explain that the purpose is to wait atomically on
a complex condition, and that the usage pattern is to hold the
lock when using the wait_event_* functions or when changing any
variable that might affect the condition and waking up the waiting
processes.

-- Al

Re: [RFC PATCH] add wait_event_*_lock() functions

[Nishanth Aravamudan]

On Fri, Feb 11, 2005 at 01:07:08AM -0600, Al Borchers wrote:
> 
> 
> On Thursday 10 February 2005 9:39 am, Nishanth Aravamudan wrote:
> >> It came up on IRC that the wait_cond*() functions from
> >> usb/serial/gadget.c could be useful in other parts of the kernel. Does
> >> the following patch make sense towards this?
> 
> Sure, if people want to use these.
> 
> I did not push them because they seemed a bit "heavy weight",
> but the construct is useful and general.

I think that is very much the case. As I was setting up patches for the
Kernel-Janitors to clean up the wait-queue usage in the kernel, I found
I was unable to use wait_event*(), as locks needed to be
released/grabbed around the sleep. wait_event_*_lock() fixes this
problem, clearly :)

> The docs should explain that the purpose is to wait atomically on
> a complex condition, and that the usage pattern is to hold the
> lock when using the wait_event_* functions or when changing any
> variable that might affect the condition and waking up the waiting
> processes.

I will submit a new patch which documents the general structure of the
wait_event_*() class of functions, including what you have written.

Thanks,
Nish