wait: add wait_event_lock_irq() interface
Subsystems (3)
PATCH 1/2 wait: add wait_event_lock_irq() interface
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.
--- 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(-)
drivers/md/md.c +1 -1
@@ -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);
drivers/md/md.h +0 -26
@@ -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);
drivers/md/raid1.c +7 -8
@@ -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)
drivers/md/raid10.c +7 -8
@@ -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); }
drivers/md/raid5.c +5 -7
@@ -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 */
include/linux/wait.h +76 -0
82 lines not on this page · raw message
Signature · 1 line
1.7.7.6
PATCH v4 2/2 loop: Limit the number of requests in the bio list
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.
---
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(-)
drivers/block/loop.c +10 -0
@@ -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;
include/linux/loop.h +3 -0
@@ -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;
Signature · 1 line
1.7.7.6
Quoted 6 lines · On Tue, 20 Nov 2012 10:23:04 +0100 Lukas Czerner <[email protected]> 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 macrowait_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.
Quoted 7 lines
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.
include/linux/wait.h do { \ · line 557
Quoted 7 lines
...
include/linux/wait.h +5
-0+#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.
include/linux/wait.h do { \ · line 570
Quoted 13 lines
include/linux/wait.h +13
-0+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.
Quoted 9 lines
+/**
+ *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 andschedule()
+ * 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!
Quoted 11 lines
+ * @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/
Quoted 1 line
+ * dropped before invoking the cmd and going to sleep and reacquired
s/reacquired/ is reacquired/
Quoted 42 lines
+ * afterwards.
+ */include/linux/wait.h +34
-0+#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.
Quoted 31 lines · Andrew Morton
Date: Tue, 20 Nov 2012 12:14:14 -0800
From: Andrew Morton <[email protected]>
To: Lukas Czerner <[email protected]>
Cc: [email protected], [email protected], [email protected],
[email protected], Neil Brown <[email protected]>,
David Howells <[email protected]>, Ingo Molnar <[email protected]>, Peter Zijlstra <[email protected]>
Subject: Re: [PATCH 1/2] wait: addwait_event_lock_irq()interfaceQuoted 6 lines · On Tue, 20 Nov 2012 10:23:04 +0100 Lukas Czerner <[email protected]> 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 macrowait_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.Quoted 7 lines
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.
Quoted 2 lines
...
Quoted 8 lines
include/linux/wait.h do { \ · line 557
Quoted 5 lines
include/linux/wait.h +5
-0+#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.
Quoted 18 lines
Quoted 4 lines
include/linux/wait.h +3
-0+add_wait_queue (&wq, &__wait); \ +for (;;) { \ +set_current_state (TASK_UNINTERRUPTIBLE ); \include/linux/wait.h do { \ · line 570
Quoted 10 lines
include/linux/wait.h +10
-0+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.
Quoted 8 lines
Because calling
schedule()immediately after callingcmd' might be a problem for some callers. Or at least, suboptimal. If that evaluation ofcmd' results incondition' becoming true then we don't *want* to call schedule(). Yes,cmd' would have put this thread intoTASK_RUNNING, but it was just a waste of cycles.So I wonder if we should retest
condition' there. Or, perhaps, test the return value ofcmd'.
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.
Quoted 4 lines
Also,
wait_event()usesprepare_to_wait(). It's a bit neater and more efficient because the wakeup removes the waiter from the waitqueue. I wonder if we can useprepare_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().
Quoted 4 lines
Also, we will surely end up needing
TASK_INTERRUPTIBLEversions 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.
Quoted 11 lines
Quoted 10 lines
+/**
+ *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 andschedule()
+ * and reacquired afterwards.
Quoted 3 lines
@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.
Quoted 13 lines
Quoted 12 lines
+ * @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
Quoted 5 lines
s/with/while/
Quoted 1 line
+ * dropped before invoking the cmd and going to sleep and reacquired
s/reacquired/ is reacquired/
Thanks!
-Lukas
Quoted 44 lines
Quoted 43 lines
+ * afterwards.
+ */include/linux/wait.h +34
-0+#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.