Re: [PATCH 1/2] wait: add wait_event_lock_irq() interface

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 20 Nov 2012 10:23:04 +0100
Lukas Czerner <lczerner@redhat.com> wrote:

> New wait_event_lock_irq{,cmd} macros added. This commit moves the
> private wait_event_lock_irq() macro from MD to regular wait includes,
> introduces new macro wait_event_lock_irq_cmd() instead of using the old
> method with omitting cmd parameter which is ugly and makes a use of new
> macros in the MD.
> 
> The use of new interface is when one have a special lock to protect data
> structures used in the condition, or one also needs to invoke "cmd"
> before putting it to sleep.
> 
> Both new macros are expected to be called with the lock taken. The lock
> is released before sleep and reacquired afterwards. We will leave the
> macro with the lock held.

Moving generic code out of md is a good thing.  It never should have
been put there.  Bad md.

> ...
>
> +#define __wait_event_lock_irq(wq, condition, lock, cmd) 		\
> +do {									\
> +	wait_queue_t __wait;						\
> +	init_waitqueue_entry(&__wait, current);				\
> +									\

The above two lines should be swapped - the blank line goes between
end-of-locals and start-of-code.

> +	add_wait_queue(&wq, &__wait);					\
> +	for (;;) {							\
> +		set_current_state(TASK_UNINTERRUPTIBLE);		\
> +		if (condition)						\
> +			break;						\
> +		spin_unlock_irq(&lock);					\
> +		cmd;							\
> +		schedule();						\
> +		spin_lock_irq(&lock);					\
> +	}								\
> +	current->state = TASK_RUNNING;					\
> +	remove_wait_queue(&wq, &__wait);				\
> +} while (0)

I'm scratching my head a bit over which situations this will be used
in, particularly outside md.

Because calling schedule() immediately after calling `cmd' might be a
problem for some callers.  Or at least, suboptimal.  If that evaluation
of `cmd' results in `condition' becoming true then we don't *want* to
call schedule().  Yes, `cmd' would have put this thread into
TASK_RUNNING, but it was just a waste of cycles.

So I wonder if we should retest `condition' there.  Or, perhaps, test
the return value of `cmd'.

Also, wait_event() uses prepare_to_wait().  It's a bit neater and more
efficient because the wakeup removes the waiter from the waitqueue.  I
wonder if we can use prepare_to_wait() here.

Also, we will surely end up needing TASK_INTERRUPTIBLE versions of
these macros, so you may as well design for that (or actually implement
them) in version 1.

> +/**
> + * wait_event_lock_irq_cmd - sleep until a condition gets true. The
> + * 			     condition is checked under the lock. This
> + * 			     is expected to be called with the lock
> + * 			     taken.
> + * @wq: the waitqueue to wait on
> + * @condition: a C expression for the event to wait for
> + * @lock: a locked lock, which will be released before cmd and schedule()
> + * 	  and reacquired afterwards.

@lock isn't just any old lock.  It must have type spinlock_t.  It's
worth mentioning this here.  This is a significant restriction of this
interface!

> + * @cmd: a command which is invoked outside the critical section before
> + *       sleep
> + *
> + * The process is put to sleep (TASK_UNINTERRUPTIBLE) until the
> + * @condition evaluates to true. The @condition is checked each time
> + * the waitqueue @wq is woken up.
> + *
> + * wake_up() has to be called after changing any variable that could
> + * change the result of the wait condition.
> + *
> + * This is supposed to be called with holding the lock. The lock is

s/with/while/

> + * dropped before invoking the cmd and going to sleep and reacquired

s/reacquired/ is reacquired/

> + * afterwards.
> + */
> +#define wait_event_lock_irq_cmd(wq, condition, lock, cmd) 		\
> +do {									\
> +	if (condition)	 						\
> +		break;							\
> +	__wait_event_lock_irq(wq, condition, lock, cmd);		\
> +} while (0)
> +
> +/**
> + * wait_event_lock_irq - sleep until a condition gets true. The
> + * 			 condition is checked under the lock. This
> + * 			 is expected to be called with the lock
> + * 			 taken.
> + * @wq: the waitqueue to wait on
> + * @condition: a C expression for the event to wait for
> + * @lock: a locked lock, which will be released before schedule()
> + * 	  and reacquired afterwards.
> + *
> + * The process is put to sleep (TASK_UNINTERRUPTIBLE) until the
> + * @condition evaluates to true. The @condition is checked each time
> + * the waitqueue @wq is woken up.
> + *
> + * wake_up() has to be called after changing any variable that could
> + * change the result of the wait condition.
> + *
> + * This is supposed to be called with holding the lock. The lock is
> + * dropped before going to sleep and reacquired afterwards.
> + */
> +#define wait_event_lock_irq(wq, condition, lock) 			\
> +do {									\
> +	if (condition)	 						\
> +		break;							\
> +	__wait_event_lock_irq(wq, condition, lock, );			\
> +} while (0)
> +
>  /*
>   * These are the old interfaces to sleep waiting for an event.
>   * They are racy.  DO NOT use them, use the wait_event* interfaces above.
>
> ...
>