Date: Wed, 26 Oct 2016 11:23:32 -0700
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: LKML <linux-kernel@vger.kernel.org>, Tejun Heo <tj@kernel.org>,
        computersforpeace@gmail.com, Ingo Molnar <mingo@kernel.org>,
        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
Content-Length: 2656
Lines: 78

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