History

sched: fix wait_for_completion_timeout() spurious failure under heavy load

It seems that the current implementaton of wait_for_completion_timeout() has a small problem under very high load for the common pattern: if (!wait_for_completion_timeout(&done, timeout)) /* handle failure */ because the implementation very roughly does (lots of code deleted to show the basic flow): static inline long __sched do_wait_for_common(struct completion *x, long timeout, int state) { if (x->done) return timeout; do { timeout = schedule_timeout(timeout); if (!timeout) return timeout; } while (!x->done); return timeout; } so if the system is very busy and x->done is not set when do_wait_for_common() is entered, it is possible that the first call to schedule_timeout() returns 0 because the task doing wait_for_completion doesn't get rescheduled for a long time, even if it is woken up early enough. In this case, wait_for_completion_timeout() returns 0 without even checking x->done again, and the code above falls into its failure case purely for scheduler reasons, even if the hardware event or whatever was being waited for happened early enough. It would make sense to add an extra test to do_wait_for() in the timeout case and return 1 if x->done is actually set. A quick audit (not exhaustive) of wait_for_completion_timeout() callers seems to indicate that no one actually cares about the return value in the success case -- they just test for 0 (timed out) versus non-zero (wait succeeded).

Roland Dreier committed 18 years ago

Commit bb10ed01 parent 8a8cde11 file · +10 −0Browse files

Showing the changes to kernel (1 of 1 file). Show all

Unified · Split

kernel/sched.c+10−0
@@ -4405,6 +4405,16 @@ do_wait_for_common(struct completion *x, long timeout, int state) · 4404 unmodified lines
4405 spin_unlock_irq(&x->wait.lock);4405 spin_unlock_irq(&x->wait.lock);
4406 timeout = schedule_timeout(timeout);4406 timeout = schedule_timeout(timeout);
4407 spin_lock_irq(&x->wait.lock);4407 spin_lock_irq(&x->wait.lock);
4408
4409 /*
4410 * If the completion has arrived meanwhile
4411 * then return 1 jiffy time left:
4412 */
4413 if (x->done && !timeout) {
4414 timeout = 1;
4415 break;
4416 }
4417
4408 if (!timeout) {4418 if (!timeout) {
4409 __remove_wait_queue(&x->wait, &wait);4419 __remove_wait_queue(&x->wait, &wait);
4410 return timeout;4420 return timeout;