From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755560AbcJZSXi (ORCPT ); Wed, 26 Oct 2016 14:23:38 -0400 Received: from mail-pf0-f196.google.com ([209.85.192.196]:32821 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753643AbcJZSXf (ORCPT ); Wed, 26 Oct 2016 14:23:35 -0400 Date: Wed, 26 Oct 2016 11:23:32 -0700 From: Dmitry Torokhov To: Peter Zijlstra Cc: LKML , Tejun Heo , computersforpeace@gmail.com, Ingo Molnar , der.herr@hofr.at Subject: Re: complete_all and "forever" completions Message-ID: <20161026182332.GC3989@dtor-ws> References: <20161025223054.GA22917@dtor-ws> <20161026084535.GX3102@twins.programming.kicks-ass.net> <20161026121001.GA19692@dtor-ws> <20161026154213.GD3117@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161026154213.GD3117@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Oct 26, 2016 at 05:42:13PM +0200, Peter Zijlstra wrote: > On Wed, Oct 26, 2016 at 05:10:01AM -0700, Dmitry Torokhov wrote: > > On Wed, Oct 26, 2016 at 10:45:35AM +0200, Peter Zijlstra wrote: > > > On Tue, Oct 25, 2016 at 03:30:54PM -0700, Dmitry Torokhov wrote: > > > > > Or do we need something like this in > > > > do_wait_for_common(): > > > > > > > > if (x->done < UINT_MAX/2) > > > > x->done--; > > > > > > Depends a bit, do you really want this? Seems a bit daft to keep asking > > > if its done already, seems like a waste of cycles to me. > > > > > > > The use case I am after is: > > > > 1. There is a device that is extremely dumb without firmware > > 2. The driver uses request_firmware_nowait() and signals completion from > > the firmware loading callback to let the reset of the driver know that > > firmware has been done loading (successfully or otherwise) > > 3. The driver uses wait_for_completion() in both remove() and suspend() > > methods to wait for the firmware to finish loading. > > > > While remove() happens at most once per device instance, suspend() may > > happen unbound number of times (theoretically). > > > > So the question is: should complete_all have this "forever" semantic > > (IOW is documentation right about the intent) or do we need a new > > primitive for this? From the cursory glance of users of complete_all() > > all of them expect completion to stay in signalled state either forever, > > or until they call reinit_completion() explicitly. > > Nah, if we need this we should fix this one. Adding similar but slightly > different primitives is a pain. > > But I think you might need slightly more than the proposed change, the > case I worry about is doing complete_all() when done != 0 (which isn't > all that strange). > > > Does something like so work? Yes, this looks good to me. > > --- > kernel/sched/completion.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c > index 8d0f35debf35..5deab9c789df 100644 > --- a/kernel/sched/completion.c > +++ b/kernel/sched/completion.c > @@ -51,7 +51,7 @@ void complete_all(struct completion *x) > unsigned long flags; > > spin_lock_irqsave(&x->wait.lock, flags); > - x->done += UINT_MAX/2; > + x->done = UINT_MAX/2; > __wake_up_locked(&x->wait, TASK_NORMAL, 0); > spin_unlock_irqrestore(&x->wait.lock, flags); > } > @@ -79,7 +79,10 @@ do_wait_for_common(struct completion *x, > if (!x->done) > return timeout; > } > - x->done--; > + > + if (x->done != UINT_MAX/2) > + x->done--; > + > return timeout ?: 1; > } > -- Dmitry