complete_all and "forever" completions

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

Quoted 3 lines · Dmitry Torokhov

Hi,

Reading Documentation/scheduler/completion.txt, complete_all() is

Oh, there is documentation? /me goes read.

Quoted 3 lines

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

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 10 lines · Peter Zijlstra

Quoted 5 lines · Dmitry Torokhov

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

Quoted 3 lines · Dmitry Torokhov

Hi,

Reading Documentation/scheduler/completion.txt, complete_all() is

Oh, there is documentation? /me goes read.

Quoted 3 lines

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

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Dmitry Torokhov

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.

Does something like so work?

---
 kernel/sched/completion.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

kernel/sched/completion.c +5 -2

index 8d0f35debf35..5deab9c789df 100644
@@ -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;
 }
 

Quoted 22 lines · Dmitry Torokhov

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Dmitry Torokhov

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.

Quoted 8 lines

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

Quoted 35 lines · Peter Zijlstra

Quoted 22 lines · Dmitry Torokhov

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Dmitry Torokhov

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.

Quoted 8 lines

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.

Quoted 3 lines

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 ?

Quoted 22 lines · Dmitry Torokhov

Quoted 11 lines · Peter Zijlstra

Quoted 5 lines · Dmitry Torokhov

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.

Quoted 8 lines

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

kernel/sched/completion.c +5 -2

index 8d0f35debf35..5deab9c789df 100644
@@ -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;
 }