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

[Arnd Bergmann]

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: pgp00000.pgp
Description: signature