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

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

[Lukas Czerner]

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.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Cc: Neil Brown <neilb@suse.de>
Cc: David Howells <dhowells@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 drivers/md/md.c      |    2 +-
 drivers/md/md.h      |   26 -----------------
 drivers/md/raid1.c   |   15 ++++-----
 drivers/md/raid10.c  |   15 ++++-----
 drivers/md/raid5.c   |   12 +++----
 include/linux/wait.h |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 96 insertions(+), 50 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 9ab768a..7e513a3 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -452,7 +452,7 @@ void md_flush_request(struct mddev *mddev, struct bio *bio)
 	spin_lock_irq(&mddev->write_lock);
 	wait_event_lock_irq(mddev->sb_wait,
 			    !mddev->flush_bio,
-			    mddev->write_lock, /*nothing*/);
+			    mddev->write_lock);
 	mddev->flush_bio = bio;
 	spin_unlock_irq(&mddev->write_lock);
 
diff --git a/drivers/md/md.h b/drivers/md/md.h
index af443ab..1e2fc3d 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -551,32 +551,6 @@ struct md_thread {
 
 #define THREAD_WAKEUP  0
 
-#define __wait_event_lock_irq(wq, condition, lock, cmd) 		\
-do {									\
-	wait_queue_t __wait;						\
-	init_waitqueue_entry(&__wait, current);				\
-									\
-	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)
-
-#define wait_event_lock_irq(wq, condition, lock, cmd) 			\
-do {									\
-	if (condition)	 						\
-		break;							\
-	__wait_event_lock_irq(wq, condition, lock, cmd);		\
-} while (0)
-
 static inline void safe_put_page(struct page *p)
 {
 	if (p) put_page(p);
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 636bae0..f264a62 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -822,7 +822,7 @@ static void raise_barrier(struct r1conf *conf)
 
 	/* Wait until no block IO is waiting */
 	wait_event_lock_irq(conf->wait_barrier, !conf->nr_waiting,
-			    conf->resync_lock, );
+			    conf->resync_lock);
 
 	/* block any new IO from starting */
 	conf->barrier++;
@@ -830,7 +830,7 @@ static void raise_barrier(struct r1conf *conf)
 	/* Now wait for all pending IO to complete */
 	wait_event_lock_irq(conf->wait_barrier,
 			    !conf->nr_pending && conf->barrier < RESYNC_DEPTH,
-			    conf->resync_lock, );
+			    conf->resync_lock);
 
 	spin_unlock_irq(&conf->resync_lock);
 }
@@ -864,8 +864,7 @@ static void wait_barrier(struct r1conf *conf)
 				    (conf->nr_pending &&
 				     current->bio_list &&
 				     !bio_list_empty(current->bio_list)),
-				    conf->resync_lock,
-			);
+				    conf->resync_lock);
 		conf->nr_waiting--;
 	}
 	conf->nr_pending++;
@@ -898,10 +897,10 @@ static void freeze_array(struct r1conf *conf)
 	spin_lock_irq(&conf->resync_lock);
 	conf->barrier++;
 	conf->nr_waiting++;
-	wait_event_lock_irq(conf->wait_barrier,
-			    conf->nr_pending == conf->nr_queued+1,
-			    conf->resync_lock,
-			    flush_pending_writes(conf));
+	wait_event_lock_irq_cmd(conf->wait_barrier,
+				conf->nr_pending == conf->nr_queued+1,
+				conf->resync_lock,
+				flush_pending_writes(conf));
 	spin_unlock_irq(&conf->resync_lock);
 }
 static void unfreeze_array(struct r1conf *conf)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index d1295af..2530403 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -952,7 +952,7 @@ static void raise_barrier(struct r10conf *conf, int force)
 
 	/* Wait until no block IO is waiting (unless 'force') */
 	wait_event_lock_irq(conf->wait_barrier, force || !conf->nr_waiting,
-			    conf->resync_lock, );
+			    conf->resync_lock);
 
 	/* block any new IO from starting */
 	conf->barrier++;
@@ -960,7 +960,7 @@ static void raise_barrier(struct r10conf *conf, int force)
 	/* Now wait for all pending IO to complete */
 	wait_event_lock_irq(conf->wait_barrier,
 			    !conf->nr_pending && conf->barrier < RESYNC_DEPTH,
-			    conf->resync_lock, );
+			    conf->resync_lock);
 
 	spin_unlock_irq(&conf->resync_lock);
 }
@@ -993,8 +993,7 @@ static void wait_barrier(struct r10conf *conf)
 				    (conf->nr_pending &&
 				     current->bio_list &&
 				     !bio_list_empty(current->bio_list)),
-				    conf->resync_lock,
-			);
+				    conf->resync_lock);
 		conf->nr_waiting--;
 	}
 	conf->nr_pending++;
@@ -1027,10 +1026,10 @@ static void freeze_array(struct r10conf *conf)
 	spin_lock_irq(&conf->resync_lock);
 	conf->barrier++;
 	conf->nr_waiting++;
-	wait_event_lock_irq(conf->wait_barrier,
-			    conf->nr_pending == conf->nr_queued+1,
-			    conf->resync_lock,
-			    flush_pending_writes(conf));
+	wait_event_lock_irq_cmd(conf->wait_barrier,
+				conf->nr_pending == conf->nr_queued+1,
+				conf->resync_lock,
+				flush_pending_writes(conf));
 
 	spin_unlock_irq(&conf->resync_lock);
 }
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index c5439dc..2bf617d 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -466,7 +466,7 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
 	do {
 		wait_event_lock_irq(conf->wait_for_stripe,
 				    conf->quiesce == 0 || noquiesce,
-				    conf->device_lock, /* nothing */);
+				    conf->device_lock);
 		sh = __find_stripe(conf, sector, conf->generation - previous);
 		if (!sh) {
 			if (!conf->inactive_blocked)
@@ -480,8 +480,7 @@ get_active_stripe(struct r5conf *conf, sector_t sector,
 						    (atomic_read(&conf->active_stripes)
 						     < (conf->max_nr_stripes *3/4)
 						     || !conf->inactive_blocked),
-						    conf->device_lock,
-						    );
+						    conf->device_lock);
 				conf->inactive_blocked = 0;
 			} else
 				init_stripe(sh, sector, previous);
@@ -1646,8 +1645,7 @@ static int resize_stripes(struct r5conf *conf, int newsize)
 		spin_lock_irq(&conf->device_lock);
 		wait_event_lock_irq(conf->wait_for_stripe,
 				    !list_empty(&conf->inactive_list),
-				    conf->device_lock,
-				    );
+				    conf->device_lock);
 		osh = get_free_stripe(conf);
 		spin_unlock_irq(&conf->device_lock);
 		atomic_set(&nsh->count, 1);
@@ -4000,7 +3998,7 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)
 		spin_lock_irq(&conf->device_lock);
 		wait_event_lock_irq(conf->wait_for_stripe,
 				    conf->quiesce == 0,
-				    conf->device_lock, /* nothing */);
+				    conf->device_lock);
 		atomic_inc(&conf->active_aligned_reads);
 		spin_unlock_irq(&conf->device_lock);
 
@@ -6088,7 +6086,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
 		wait_event_lock_irq(conf->wait_for_stripe,
 				    atomic_read(&conf->active_stripes) == 0 &&
 				    atomic_read(&conf->active_aligned_reads) == 0,
-				    conf->device_lock, /* nothing */);
+				    conf->device_lock);
 		conf->quiesce = 1;
 		spin_unlock_irq(&conf->device_lock);
 		/* allow reshape to continue */
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 168dfe1..96ed02a 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -550,6 +550,82 @@ do {									\
 	__ret;								\
 })
 
+#define __wait_event_lock_irq(wq, condition, lock, cmd) 		\
+do {									\
+	wait_queue_t __wait;						\
+	init_waitqueue_entry(&__wait, current);				\
+									\
+	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)
+
+/**
+ * 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.
+ * @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
+ * dropped before invoking the cmd and going to sleep and 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.
-- 
1.7.7.6

[PATCH 2/2 v4] loop: Limit the number of requests in the bio list

[Lukas Czerner]

Currently there is not limitation of number of requests in the loop bio
list. This can lead into some nasty situations when the caller spawns
tons of bio requests taking huge amount of memory. This is even more
obvious with discard where blkdev_issue_discard() will submit all bios
for the range and wait for them to finish afterwards. On really big loop
devices and slow backing file system this can lead to OOM situation as
reported by Dave Chinner.

With this patch we will wait in loop_make_request() if the number of
bios in the loop bio list would exceed 'nr_congestion_on'.
We'll wake up the process as we process the bios form the list. Some
threshold hysteresis is in place to avoid high frequency oscillation.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Reported-by: Dave Chinner <dchinner@redhat.com>
---
 v2: add threshold hysteresis
 v3: Wait uninterruptible, use nr_congestion_off/on
 v4: use wait_event_lock_irq()

 drivers/block/loop.c |   10 ++++++++++
 include/linux/loop.h |    3 +++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 54046e5..ae12512 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -463,6 +463,7 @@ out:
  */
 static void loop_add_bio(struct loop_device *lo, struct bio *bio)
 {
+	lo->lo_bio_count++;
 	bio_list_add(&lo->lo_bio_list, bio);
 }
 
@@ -471,6 +472,7 @@ static void loop_add_bio(struct loop_device *lo, struct bio *bio)
  */
 static struct bio *loop_get_bio(struct loop_device *lo)
 {
+	lo->lo_bio_count--;
 	return bio_list_pop(&lo->lo_bio_list);
 }
 
@@ -489,6 +491,10 @@ static void loop_make_request(struct request_queue *q, struct bio *old_bio)
 		goto out;
 	if (unlikely(rw == WRITE && (lo->lo_flags & LO_FLAGS_READ_ONLY)))
 		goto out;
+	if (lo->lo_bio_count >= q->nr_congestion_on)
+		wait_event_lock_irq(lo->lo_req_wait,
+				    lo->lo_bio_count < q->nr_congestion_off,
+				    lo->lo_lock);
 	loop_add_bio(lo, old_bio);
 	wake_up(&lo->lo_event);
 	spin_unlock_irq(&lo->lo_lock);
@@ -546,6 +552,8 @@ static int loop_thread(void *data)
 			continue;
 		spin_lock_irq(&lo->lo_lock);
 		bio = loop_get_bio(lo);
+		if (lo->lo_bio_count < lo->lo_queue->nr_congestion_off)
+			wake_up(&lo->lo_req_wait);
 		spin_unlock_irq(&lo->lo_lock);
 
 		BUG_ON(!bio);
@@ -873,6 +881,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	lo->transfer = transfer_none;
 	lo->ioctl = NULL;
 	lo->lo_sizelimit = 0;
+	lo->lo_bio_count = 0;
 	lo->old_gfp_mask = mapping_gfp_mask(mapping);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
 
@@ -1673,6 +1682,7 @@ static int loop_add(struct loop_device **l, int i)
 	lo->lo_number		= i;
 	lo->lo_thread		= NULL;
 	init_waitqueue_head(&lo->lo_event);
+	init_waitqueue_head(&lo->lo_req_wait);
 	spin_lock_init(&lo->lo_lock);
 	disk->major		= LOOP_MAJOR;
 	disk->first_minor	= i << part_shift;
diff --git a/include/linux/loop.h b/include/linux/loop.h
index 6492181..460b60f 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -53,10 +53,13 @@ struct loop_device {
 
 	spinlock_t		lo_lock;
 	struct bio_list		lo_bio_list;
+	unsigned int		lo_bio_count;
 	int			lo_state;
 	struct mutex		lo_ctl_mutex;
 	struct task_struct	*lo_thread;
 	wait_queue_head_t	lo_event;
+	/* wait queue for incoming requests */
+	wait_queue_head_t	lo_req_wait;
 
 	struct request_queue	*lo_queue;
 	struct gendisk		*lo_disk;
-- 
1.7.7.6

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

[Andrew Morton]

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

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

[Lukáš Czerner]

On Tue, 20 Nov 2012, Andrew Morton wrote:

> Date: Tue, 20 Nov 2012 12:14:14 -0800
> From: Andrew Morton <akpm@linux-foundation.org>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org, axboe@kernel.dk,
>     jmoyer@redhat.com, Neil Brown <neilb@suse.de>,
>     David Howells <dhowells@redhat.com>, Ingo Molnar <mingo@elte.hu>,
>     Peter Zijlstra <a.p.zijlstra@chello.nl>
> Subject: Re: [PATCH 1/2] wait: add wait_event_lock_irq() interface
> 
> 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.

Good point.

> 
> > +	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'.

Right, it might makes sense to swap the lines and let cmd to be run
after the schedule(). That way, if 'cmd' would result in 'condition'
becoming true, then we would catch it before going to schedule()
again.

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

I suspect that it was a tradeoff between using the neat
prepare_to_wait() with waitqueue removal, but having to lock the
wait_queue_head_t->lock or using simply set_current_state() since
we already hold external 'lock'.

Whoever wrote the code originally thought that it was better this
way, however since the waitqueue lock should not actually be
contended at all here, I think it is ok to use prepare_to_wait().

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

Sure, I can add the TASK_INTERRUPTIBLE version as well. The reason I
did not included that initially was that I did not want to introduce
unused code. But if you're ok with it, I'll add it.

> 
> > +/**
> > + * 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!

Right, it's better to be explicit.

> 
> > + * @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/

Thanks!
-Lukas

> 
> > + * 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.
> >
> > ...
> >
>