complete_all and "forever" completions

complete_all and "forever" completions

[Dmitry Torokhov]

Hi,

Reading Documentation/scheduler/completion.txt, complete_all() is
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". 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. 

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? Or do we need something like this in
do_wait_for_common():

	if (x->done < UINT_MAX/2)
		x->done--;

Thanks.

-- 
Dmitry

Re: complete_all and "forever" completions

[Peter Zijlstra]

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

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

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

Re: complete_all and "forever" completions

[Nicholas Mc Guire]

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

bad wording maybe - the intent of setting it to UINT_MAX/2
as far as I can judge is though that UINT_MAX/2 effectively would be
infinity in practice. Is it realistic to assume that there would be
a complete_all() call followed by 2147483648 calls to wait_for_completion() ?
The note on "future waiters" was to make it clear that once you called
complete_all() future wait_for_completion() have no synchronizing effect.

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

I would claim that if you have a complete_all() (done=2147483648) and you
actually did manage to decrement it to 0 over time so a call finally blocks
(presumably for ever) this would be uncovering a deisgn bug in the use of
completion as such a setup does not make any sense (Or Im just not creative
enough to think of such a situation).

thx!
hofrat

Re: complete_all and "forever" completions

[Daniel Wagner]

On Wed, Oct 26, 2016 at 09:15:19AM +0000, Nicholas Mc Guire 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.
> > 
> 
> I would claim that if you have a complete_all() (done=2147483648) and you
> actually did manage to decrement it to 0 over time so a call finally blocks
> (presumably for ever) this would be uncovering a deisgn bug in the use of
> completion as such a setup does not make any sense (Or Im just not creative
> enough to think of such a situation).

I am reviewing all the complete_all() users in order to figure out if
we could weaken the garantees which complete_all() gives you: can be
used in hard irq context and irq disabled context. But that is a
different story.

So while doing the review I found things like

drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:

vchiq_arm_init_state() {
	[...]

		init_completion(&arm_state->vc_resume_complete);
		/* Initialise to 'done' state.  We only want to block on resume
		 * completion while videocore is suspended. */
		set_resume_state(arm_state, VC_RESUME_RESUMED);

		init_completion(&arm_state->resume_blocker);
		/* Initialise to 'done' state.  We only want to block on this
		 * completion while resume is blocked */
		complete_all(&arm_state->resume_blocker);

		init_completion(&arm_state->blocked_blocker);
		/* Initialise to 'done' state.  We only want to block on this
		 * completion while things are waiting on the resume blocker */
		complete_all(&arm_state->blocked_blocker);

	[...]
}

If I read this corredtly, there are some 'interesting' uses of
completion where you might run into limits.

cheers,
daniel

Re: complete_all and "forever" completions

[Dmitry Torokhov]

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

Re: complete_all and "forever" completions

[Peter Zijlstra]

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?

---
 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;
 }
 

Re: complete_all and "forever" completions

[Peter Zijlstra]

On Wed, Oct 26, 2016 at 05:42:13PM +0200, Peter Zijlstra wrote:

> Does something like so work?

try_wait_for_completion() would need a similar change.

> ---
>  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;
>  }
>  

Re: complete_all and "forever" completions

[Nicholas Mc Guire]

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

I do not quite see how that would work out for the Use-Case noted
if completion calls complete_all() once at the first firmware load
and at that point that completion object is "completed" forever then
the suspend() would not wait for any completion in this model.

Im probably simply misunderstanding something here - but a 
complete_all() seems to have been intended for a logically single
case of concurrent initialization but not for this use-case
I do think that this is a broken design - if the suspend()/resume()
needed to reload the firware then it also would need to wait for
the same and that would not occure without reinitializing the
completion object.

what am I missing ?

thx!
hofrat

Re: complete_all and "forever" completions

[Dmitry Torokhov]

On Wed, Oct 26, 2016 at 05:12:36PM +0000, Nicholas Mc Guire wrote:
> 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).
> 
> I do not quite see how that would work out for the Use-Case noted
> if completion calls complete_all() once at the first firmware load
> and at that point that completion object is "completed" forever then
> the suspend() would not wait for any completion in this model.

That is correct. In the use case above the firmware is loaded only once
upon binding device and driver and is not reloaded unless device is
completely unbound and then re-bound again. So we do not want suspend()
to wait after the firmware is loaded.

Note that I said we are using request_firmware_nowait() so that firmware
loading is done by the thread other than probe() thread and probe()
returns before firmware is loaded. Which means that suspend() (however
unlikely it is) may start executing before firmware loading completes,
and we want suspend to pause in this case. Once firmware is loaded
suspend should no longer wait.

> 
> Im probably simply misunderstanding something here - but a 
> complete_all() seems to have been intended for a logically single
> case of concurrent initialization but not for this use-case
> I do think that this is a broken design - if the suspend()/resume()
> needed to reload the firware then it also would need to wait for
> the same and that would not occure without reinitializing the
> completion object.
> 
> what am I missing ?

The fact that we only load firmware once in the case I described.

Thanks.

-- 
Dmitry

Re: complete_all and "forever" completions

[Dmitry Torokhov]

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