> On Fri, 9 Apr 2010, Michal Nazarewicz wrote:
>> New wait_event_interruptible{,_exclusive}_locked{,_irq,_irqsave}
>> macros added. They work just like versions without _locked* suffix
Thomas Gleixner <tglx@xxxxxxxxxxxxx> writes:
> The _irqsave variant is really not necessary. It's actively wrong.
>
> If you go to wait then the state _before_ acquiring the waitqueue head
> lock must be irqs enabled. Otherwise you would schedule with
> interrupts disabled after the unlock_irqrestore which is a BUG.
>
> So if there is code which uses spin_lock_irqsave() in the wait path
> then this code is wrong and needs to be fixed to spin_(un)lock_irq()
> first instead of adding a bogus interface.
I haven't seen the big picture here. Thanks for pointing that out.
>> +
>> +#define __wait_event_interruptible_locked(wq, condition, ret, exclusive, lock, unlock, lock_args) \
>
> That will also simplify this to (wq, condition, exclusive, lockmode)
>
>> +do { \
>> + DEFINE_WAIT(__wait); \
>> + \
>> + if (exclusive) \
>> + __wait.flags |= WQ_FLAG_EXCLUSIVE; \
>> + else \
>> + __wait.flags &= ~WQ_FLAG_EXCLUSIVE; \
>> + __add_wait_queue_tail(&(wq), &__wait); \
>> + \
>> + do { \
>> + set_current_state(TASK_INTERRUPTIBLE); \
>> + if (signal_pending(current)) { \
>> + ret = -ERESTARTSYS; \
>> + break; \
>> + } \
>> + spin_unlock ##unlock lock_args; \
>> + schedule(); \
>> + spin_lock ##lock lock_args; \
>> + } while (!(condition)); \
>> + __remove_wait_queue(&(wq), &__wait); \
>> + __set_current_state(TASK_RUNNING); \
>> +} while (0)
>> +
>> +
>> +/**
>> + * wait_event_interruptible_locked - sleep until a condition gets true
>> + * @wq: the waitqueue to wait on
>> + * @condition: a C expression for the event to wait for
>> + *
>> + * The process is put to sleep (TASK_INTERRUPTIBLE) until the
>> + * @condition evaluates to true or a signal is received.
>> + * The @condition is checked each time the waitqueue @wq is woken up.
>> + *
>> + * It must be called with wq.lock being held. This spinlock is
>> + * unlocked while sleeping but @condition testing is done while lock
>> + * is held and when this macro exits the lock is held.
>> + *
>> + * The lock is locked/unlocked using spin_lock()/spin_unlock()
>> + * functions which must match the way they are locked/unlocked outside
>> + * of this macro.
>> + *
>> + * wake_up_locked() has to be called after changing any variable that could
>> + * change the result of the wait condition.
>> + *
>> + * The function will return -ERESTARTSYS if it was interrupted by a
>> + * signal and 0 if @condition evaluated to true.
>> + */
>> +#define wait_event_interruptible_locked(wq, condition) \
>> +({ \
>> + int __ret = 0; \
>> + if (!(condition)) \
>> + __wait_event_interruptible_locked(wq, condition, __ret, 0, , , (&(wq).lock)); \
>
> I had to look more than once to figure out how that code might
> return anything else than 0. Can we please change that to
>
> if (!(condition))
> __ret = __wait_.....();
>
> to make that less confusing ?
That's really how the rest of the wait_event*() macros are done so I'd
prefer to stack with the rest of the code.
>> + __ret; \
>> +})
Again, I'll resend the patches by the end of the week.
--
Best regards, _ _
.o. | Liege of Serenly Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michal "mina86" Nazarewicz (o o)
ooo +--<mina86-tlen.pl>--<jid:mina86-jabber.org>--ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
