Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757461AbYFTOaR (ORCPT ); Fri, 20 Jun 2008 10:30:17 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1755910AbYFTOaF (ORCPT ); Fri, 20 Jun 2008 10:30:05 -0400 Received: from x346.tv-sign.ru ([89.108.83.215]:40255 "EHLO mail.screens.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755581AbYFTOaE (ORCPT ); Fri, 20 Jun 2008 10:30:04 -0400 Date: Fri, 20 Jun 2008 18:32:20 +0400 From: Oleg Nesterov To: Ingo Molnar Cc: Jiri Slaby , Roland Dreier , linux-kernel@vger.kernel.org, Eli Cohen , general@lists.openfabrics.org, Peter Zijlstra Subject: Re: wait_for_completion_timeout() spurious failure under heavy load? Message-ID: <20080620143220.GA441@tv-sign.ru> References: <485B50F1.2020802@gmail.com> <20080620112042.GE7439@elte.hu> <20080620141400.GA411@tv-sign.ru> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080620141400.GA411@tv-sign.ru> User-Agent: Mutt/1.5.11 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2071 Lines: 72 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/