Messages in this thread Patch in this message | | | Date | Fri, 20 Jun 2008 18:32:20 +0400 | From | Oleg Nesterov <> | Subject | Re: wait_for_completion_timeout() spurious failure under heavy load? |
| |
On 06/20, Oleg Nesterov wrote: > > On 06/20, Ingo Molnar wrote: > > > > --- a/kernel/sched.c > > +++ b/kernel/sched.c > > @@ -4405,6 +4405,16 @@ do_wait_for_common(struct completion *x, long timeout, int state) > > spin_unlock_irq(&x->wait.lock); > > timeout = schedule_timeout(timeout); > > spin_lock_irq(&x->wait.lock); > > + > > + /* > > + * If the completion has arrived meanwhile > > + * then return 1 jiffy time left: > > + */ > > + if (x->done && !timeout) { > > + timeout = 1; > > + break; > > + } > > + > > if (!timeout) { > > __remove_wait_queue(&x->wait, &wait); > > return timeout; > > This is the real nitpick, but I wonder what is the right behaviour > of wait_for_completion_timeout(x, 0) when x->done != 0. Perhaps we > can return 1 in that case too, just for the consistency? > > IOW, how about the patch below? this also makes the code a bit > simpler because we factor out __remove_wait_queue().
Even better, we can kill the first __remove_wait_queue() as well.
Oleg.
--- kernel/sched.c +++ kernel/sched.c @@ -4739,22 +4739,20 @@ do_wait_for_common(struct completion *x, signal_pending(current)) || (state == TASK_KILLABLE && fatal_signal_pending(current))) { - __remove_wait_queue(&x->wait, &wait); - return -ERESTARTSYS; + timeout = -ERESTARTSYS; + break; } __set_current_state(state); spin_unlock_irq(&x->wait.lock); timeout = schedule_timeout(timeout); spin_lock_irq(&x->wait.lock); - if (!timeout) { - __remove_wait_queue(&x->wait, &wait); - return timeout; - } - } while (!x->done); + } while (!x->done && timeout); __remove_wait_queue(&x->wait, &wait); + if (!x->done) + return timeout; } x->done--; - return timeout; + return timeout ?: 1; } static long __sched
| |