Date: Wed, 26 Oct 2016 05:10:01 -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: <20161026121001.GA19692@dtor-ws>
References: <20161025223054.GA22917@dtor-ws>
 <20161026084535.GX3102@twins.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161026084535.GX3102@twins.programming.kicks-ass.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2436
Lines: 71

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:
> > Hi,
> > 
> > Reading Documentation/scheduler/completion.txt, complete_all() is
> 
> Oh, there is documentation? /me goes read.
> 
> > supposed to be usable with "forever" completions, i.e. when we have an
> > action that happens once and stays "done" for the rest of lifetime of an
> > object, no matter how many times we check for "doneness".
> 
> I suppose you allude to this wording:
> 
>   "calls complete_all() to signal all current and future waiters."

Yes.

> 
> > However the
> > implementation for complete_all() simply sets the counter to be greater
> > or equal UINT_MAX/2 and do_wait_for_common() happily decreases it on
> > every call.
> 
> This is indeed so.
> 
> > Is it simply an artefact of [older] implementation where we do not
> > expect to make that many calls to wait_for_completion*() so that
> > completion that is signalled with ocmplete_all() is practically stays
> > signalled forever?
> 
> The text says it was written against v3.18 or thereabout, and that
> implementation looks a lot like todays, so I doubt it ever worked like
> that.

Yes, as far as I can see if was doing x->done += UNIT_MAX/2 since dawn
of time.

> 
> > 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.

Thanks.

-- 
Dmitry