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

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

[RFC UPDATE PATCH] add wait_event_*_lock() functions and comments

[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.
> 
> 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.

How does this patch look? I wasn't sure if macros and DocBook-style
comments played well together, and the names of the macros pretty much
explain what they do :)

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. Adds a
comment to explain the usage pattern for all of the wait_event*()
macros.

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-11 11:55:07.000000000 -0800
@@ -156,6 +156,32 @@ wait_queue_head_t *FASTCALL(bit_waitqueu
 #define	wake_up_locked(x)		__wake_up_locked((x), TASK_UNINTERRUPTIBLE | TASK_INTERRUPTIBLE)
 #define wake_up_interruptible_sync(x)   __wake_up_sync((x),TASK_INTERRUPTIBLE, 1)
 
+/*
+ * The wait_event*() macros wait atomically on @wq for a complex
+ * @condition to become true, thus avoiding the race conditions
+ * associated with the deprecated sleep_on*() family of functions.
+ *
+ * The macros indicate their usage in their name. Unless explicitly
+ * requested to be different, the following defaults are the case:
+ * 	- no lock needs to be grabbed/released;
+ * 	- a timeout is not requested, i.e. only @condition being true
+ * 		will cause the macro to return; and
+ * 	- the sleep will be in TASK_UNINTERRUPTIBLE, i.e. signals will
+ * 		be ignored.
+ * If the macro name contains:
+ * 	lock, then @lock should be held before calling wait_event*().
+ * 		It is released before sleeping and grabbed after
+ * 		waking, saving the current IRQ mask in @flags. This lock
+ * 		should also be held when changing any variables
+ * 		affecting the condition and when waking up the process.
+ * 	timeout, then even if @condition is not true, but @timeout
+ * 		jiffies have passed, the macro will return. The number
+ * 		of jiffies remaining in the delay will be returned
+ * 	interruptible, then signals will cause the macro to return
+ * 		early with a return code of -ERESTARTSYS
+ * 	exclusive, then current is an exclusive process and must be
+ *	 	selectively woken.
+ */
 #define __wait_event(wq, condition) 					\
 do {									\
 	DEFINE_WAIT(__wait);						\
@@ -176,6 +202,28 @@ do {									\
 	__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 +247,31 @@ do {									\
 	__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 +298,34 @@ do {									\
 	__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 +354,36 @@ do {									\
 	__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 UPDATE PATCH] add wait_event_*_lock() functions and comments

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 2906 bytes --]

On Freedag 11 Februar 2005 20:55, Nishanth Aravamudan wrote:

> + * If the macro name contains:
> + * 	lock, then @lock should be held before calling wait_event*().
> + * 		It is released before sleeping and grabbed after
> + * 		waking, saving the current IRQ mask in @flags. This lock
> + * 		should also be held when changing any variables
> + * 		affecting the condition and when waking up the process.

Hmm, I see two problems with that approach:

1. It might lead to people not thinking about their locking order
thoroughly if you introduce a sleeping function that is called with
a spinlock held. Anyone relying on that lock introduces races because
it actually is given up by the macro. I'd prefer it to be called 
without the lock and then have it acquire the lock only to check the
condition, e.g:

#define __wait_event_lock(wq, condition, lock, flags)                  \
do {                                                                   \
       DEFINE_WAIT(__wait);                                            \
                                                                       \
       for (;;) {                                                      \
               prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE);    \
               spin_lock_irqsave(lock, flags);                         \
               if (condition)                                          \
                       break;                                          \
               spin_unlock_irqrestore(lock, flags);                    \
               schedule();                                             \
       }                                                               \
       spin_unlock_irqrestore(lock, flags);                            \
       finish_wait(&wq, &__wait);                                      \
} while (0)

2. You define the macros only for using spin_lock_irqsave. To make the
API complete, you would also need
spin_lock()
spin_lock_irq()
spin_lock_bh()
read_lock()
read_lock_irq()
read_lock_bh()
read_lock_irqsave()
write_lock()
write_lock_irq()
write_lock_bh()
write_lock_irqsave()

Of course, that is complete overkill if you want to define all the 
wait_event variations for each of those locking variations, but sooner or
later someone will want another one.

One solution that might work could look like
#define __cond_spin_locked(cond, lock) \
	({ __typeof__(cond) c; spin_lock(lock); \
		c = (cond); spin_unlock(lock); c; })

#define wait_event_lock(wq, condition, lock) \
	wait_event(wq, __cond_spin_locked(condition, lock))

#define wait_event_timeout_lock(wq, condition, lock, flags, timeout) \
	wait_event_timeout(wq, __cond_spin_locked(condition, lock), timeout)

and so forth.

OTOH, that is easy enough that it can as well be encapsulated in the
places where it is needed.

	Arnd <><

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 2309 bytes --]

On Sat, 12 Feb 2005 12:38:26 +0100 Arnd Bergmann wrote:

> On Freedag 11 Februar 2005 20:55, Nishanth Aravamudan wrote:
> 
> > + * If the macro name contains:
> > + * 	lock, then @lock should be held before calling wait_event*().
> > + * 		It is released before sleeping and grabbed after
> > + * 		waking, saving the current IRQ mask in @flags. This lock
> > + * 		should also be held when changing any variables
> > + * 		affecting the condition and when waking up the process.
> 
> Hmm, I see two problems with that approach:
> 
> 1. It might lead to people not thinking about their locking order
> thoroughly if you introduce a sleeping function that is called with
> a spinlock held. Anyone relying on that lock introduces races because
> it actually is given up by the macro. I'd prefer it to be called 
> without the lock and then have it acquire the lock only to check the
> condition, e.g:
> 
> #define __wait_event_lock(wq, condition, lock, flags)                  \
> do {                                                                   \
>        DEFINE_WAIT(__wait);                                            \
>                                                                        \
>        for (;;) {                                                      \
>                prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE);    \
>                spin_lock_irqsave(lock, flags);                         \
>                if (condition)                                          \
>                        break;                                          \
>                spin_unlock_irqrestore(lock, flags);                    \
>                schedule();                                             \
>        }                                                               \
>        spin_unlock_irqrestore(lock, flags);                            \
>        finish_wait(&wq, &__wait);                                      \
> } while (0)

But in this case the result of testing the condition becomes useless
after spin_unlock_irqrestore - someone might grab the lock and change
things.   Therefore the calling code would need to add a loop around
wait_event_lock - and the wait_event_* macros were added precisely to
encapsulate such a loop and avoid the need to code it manually.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]

On Sünnavend 12 Februar 2005 14:28, Sergey Vlasov wrote:
> On Sat, 12 Feb 2005 12:38:26 +0100 Arnd Bergmann wrote:
> > #define __wait_event_lock(wq, condition, lock, flags)                  \
> > do {                                                                   \
> >        DEFINE_WAIT(__wait);                                            \
> >                                                                        \
> >        for (;;) {                                                      \
> >                prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE);    \
> >                spin_lock_irqsave(lock, flags);                         \
> >                if (condition)                                          \
> >                        break;                                          \
> >                spin_unlock_irqrestore(lock, flags);                    \
> >                schedule();                                             \
> >        }                                                               \
> >        spin_unlock_irqrestore(lock, flags);                            \
> >        finish_wait(&wq, &__wait);                                      \
> > } while (0)
> 
> But in this case the result of testing the condition becomes useless
> after spin_unlock_irqrestore - someone might grab the lock and change
> things.   Therefore the calling code would need to add a loop around
> wait_event_lock - and the wait_event_* macros were added precisely to
> encapsulate such a loop and avoid the need to code it manually.

Ok, i understand now what the patch really wants to achieve. However,
I'm not convinced it's a good idea. In the usb/gadget/serial.c driver,
this appears to work only because an unconventional locking scheme is
used, i.e. there is an extra flag (port->port_in_use) that is set to
tell other functions about the state of the lock in case the lock holder
wants to sleep.

Is there any place in the kernel that would benefit of the 
wait_event_lock() macro family while using locks without such
special magic?

	Arnd <><

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Nish Aravamudan]

On Sun, 13 Feb 2005 03:41:01 +0100, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sünnavend 12 Februar 2005 14:28, Sergey Vlasov wrote:
> > On Sat, 12 Feb 2005 12:38:26 +0100 Arnd Bergmann wrote:
> > > #define __wait_event_lock(wq, condition, lock, flags)                  \
> > > do {                                                                   \
> > >        DEFINE_WAIT(__wait);                                            \
> > >                                                                        \
> > >        for (;;) {                                                      \
> > >                prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE);    \
> > >                spin_lock_irqsave(lock, flags);                         \
> > >                if (condition)                                          \
> > >                        break;                                          \
> > >                spin_unlock_irqrestore(lock, flags);                    \
> > >                schedule();                                             \
> > >        }                                                               \
> > >        spin_unlock_irqrestore(lock, flags);                            \
> > >        finish_wait(&wq, &__wait);                                      \
> > > } while (0)
> >
> > But in this case the result of testing the condition becomes useless
> > after spin_unlock_irqrestore - someone might grab the lock and change
> > things.   Therefore the calling code would need to add a loop around
> > wait_event_lock - and the wait_event_* macros were added precisely to
> > encapsulate such a loop and avoid the need to code it manually.
> 
> Ok, i understand now what the patch really wants to achieve. However,
> I'm not convinced it's a good idea. In the usb/gadget/serial.c driver,
> this appears to work only because an unconventional locking scheme is
> used, i.e. there is an extra flag (port->port_in_use) that is set to
> tell other functions about the state of the lock in case the lock holder
> wants to sleep.
> 
> Is there any place in the kernel that would benefit of the
> wait_event_lock() macro family while using locks without such
> special magic?

Sorry for replying from a different account, but it's the best I can
do right now. I know while I was scanning the whole kernel for other
wait_event*() replacements, I thought at least a handful of times,
"ugh, I could replace this whole block of code, except for that lock!"
I will try to get you a more concrete example on Monday. Thanks for
the feedback & patience!

-Nish

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

[Nishanth Aravamudan]

On Sat, Feb 12, 2005 at 09:00:52PM -0800, Nish Aravamudan wrote:
> On Sun, 13 Feb 2005 03:41:01 +0100, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sünnavend 12 Februar 2005 14:28, Sergey Vlasov wrote:
> > > On Sat, 12 Feb 2005 12:38:26 +0100 Arnd Bergmann wrote:
> > > > #define __wait_event_lock(wq, condition, lock, flags)                  \
> > > > do {                                                                   \
> > > >        DEFINE_WAIT(__wait);                                            \
> > > >                                                                        \
> > > >        for (;;) {                                                      \
> > > >                prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE);    \
> > > >                spin_lock_irqsave(lock, flags);                         \
> > > >                if (condition)                                          \
> > > >                        break;                                          \
> > > >                spin_unlock_irqrestore(lock, flags);                    \
> > > >                schedule();                                             \
> > > >        }                                                               \
> > > >        spin_unlock_irqrestore(lock, flags);                            \
> > > >        finish_wait(&wq, &__wait);                                      \
> > > > } while (0)
> > >
> > > But in this case the result of testing the condition becomes useless
> > > after spin_unlock_irqrestore - someone might grab the lock and change
> > > things.   Therefore the calling code would need to add a loop around
> > > wait_event_lock - and the wait_event_* macros were added precisely to
> > > encapsulate such a loop and avoid the need to code it manually.
> > 
> > Ok, i understand now what the patch really wants to achieve. However,
> > I'm not convinced it's a good idea. In the usb/gadget/serial.c driver,
> > this appears to work only because an unconventional locking scheme is
> > used, i.e. there is an extra flag (port->port_in_use) that is set to
> > tell other functions about the state of the lock in case the lock holder
> > wants to sleep.
> > 
> > Is there any place in the kernel that would benefit of the
> > wait_event_lock() macro family while using locks without such
> > special magic?
> 
> Sorry for replying from a different account, but it's the best I can
> do right now. I know while I was scanning the whole kernel for other
> wait_event*() replacements, I thought at least a handful of times,
> "ugh, I could replace this whole block of code, except for that lock!"
> I will try to get you a more concrete example on Monday. Thanks for
> the feedback & patience!

Here's at least one example:

drivers/ieee1394/video1394.c:__video1394_ioctl()

I'm having trouble finding more (maybe I already fixed some of them via
the existing macros in different ways -- or maybe my memory is just
acting up...).

I think this patch/macro can be useful for wait-queues where the same
lock is used to protect the sleeper and the sleeper's data?

Any further feedback would be appreciated, or any recommendations for
better ways of doing things. I really would just like to have one
consistent interface for all wait-queue usage :) The fact that was is
nearly (but not quite) done by wait_event*() has to be defined somewhere
else just to get that functionality, when it costs little to add it to a
common header, makes this a pretty small change to me.

But, Arnd, I understand your concern. It would not be good if we had a
bunch of lock-holding sleepers pop up now! I will try to think of a
better solution.

Thanks,
Nish

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

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 1525 bytes --]

On Dinsdag 15 Februar 2005 02:04, Nishanth Aravamudan wrote:
> Here's at least one example:
> 
> drivers/ieee1394/video1394.c:__video1394_ioctl()
> 
AFAICS, that one should work just fine using after converting

          while (d->buffer_status[v.buffer]!= VIDEO1394_BUFFER_READY) {
                  spin_unlock_irqrestore(&d->lock, flags);
                  interruptible_sleep_on(&d->waitq);
                  spin_lock_irqsave(&d->lock, flags);
                  if (signal_pending(current)) {
                           spin_unlock_irqrestore(&d->lock,flags);
                           return -EINTR;
                  }
          }

to

static inline unsigned video1394_buffer_state(struct dma_iso_ctx *d,
						unsigned int buffer)
{
	unsigned long flags;
	unsigned int ret;
	spin_lock_irqsave(&d->lock, flags);
	ret = d->buffer_status[buffer];
	spin_unlock_irqrestore(&d->lock, flags);
	return ret;
}
...

	spin_unlock_irqrestore(&d->lock, flags);
	if (wait_event_interruptible(d->waitq, 
	    video1394_buffer_state(d, v.buffer) == VIDEO1394_BUFFER_READY))
		return -EINTR;
	spin_lock_irqsave(&d->lock, flags);

The trick here is that it is known in advance that the state does not actually
have to be protected by the lock after reading it, because the state can not 
change from READY to FREE in any other place in the code.
One exception might be two processes calling the ioctl at the same time, but
I think that is racy will any of these variations.

	Arnd <><



[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Nish Aravamudan]

On Tue, 15 Feb 2005 18:50:45 +0100, Arnd Bergmann <arnd@arndb.de> wrote:
> On Dinsdag 15 Februar 2005 02:04, Nishanth Aravamudan wrote:
> > Here's at least one example:
> >
> > drivers/ieee1394/video1394.c:__video1394_ioctl()
> >
> AFAICS, that one should work just fine using after converting

<snip>

> The trick here is that it is known in advance that the state does not actually
> have to be protected by the lock after reading it, because the state can not
> change from READY to FREE in any other place in the code.
> One exception might be two processes calling the ioctl at the same time, but
> I think that is racy will any of these variations.

Hmm, I think you might be right, actually. So, for now, I guess it is
ok to not have these macros globally available (I may end up changing
my mind as I go through trying to replace other
interruptible_sleep_on() callers, but I'll cross that bridge when I
get to it :)

Thanks,
Nish