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

[Sergey Vlasov]

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