Messages in this thread | | | Date | Wed, 31 Dec 2014 15:29:58 -0700 | From | Jonathan Corbet <> | Subject | Re: [PATCH v2 1/2] doc: brief user documentation for completion |
| |
I'm finally getting around to looking at this. Honestly, I think we could add it now and make our documentation better, but I'm going to pick nits anyway in the hopes of one more round of improvement :)
On Tue, 23 Dec 2014 20:41:39 +0100 Nicholas Mc Guire <der.herr@hofr.at> wrote:
> diff --git a/Documentation/scheduler/completion.txt b/Documentation/scheduler/completion.txt > new file mode 100644 > index 0000000..d35a948 > --- /dev/null > +++ b/Documentation/scheduler/completion.txt > @@ -0,0 +1,198 @@ > +completion - wait for completion handler > +======================================== > + > +Origin: Linus Torvalds, kernel 2.4.7, 2001 > +Location: kernel/sched/completion.c > + include/linux/completion.h > +Users: all subsystems - mostly wait_for_completion and > + wait_for_completion_timeout is in use.
I'm not sure we need this stuff; will it really help readers to use this facility?
> +This document was originally written based on 3.18.0 (linux-next)
This, instead, is something I wish we had in more of our documentation; it lets future readers get a sense for how likely it is to be current.
> +Introduction: > +============= > + > +Completion is a code synchronization mechanism that is preferable to mis-
It would read a bit better to talk about completions in the plural everywhere. "Completions are..."
> +using of locks - semantically they are somewhat like a pthread_barrier. If > +you have one or more threads of execution that must wait for some process > +to have reached a point or a specific state, completions can provide a race > +free solution to this problem. > + > +Completion is built on top of the generic event infrastructure in Linux,
Here too
> +with the event reduced to a simple flag appropriately called "done" in > +struct completion, that tells the waiting threads of execution that they > +can continue safely. > + > +For details on completion design and implementation see completion-design.txt > + > +Usage: > +====== > + > +Basically there are three parts to the API, the initialization of the
"There are three parts to the API: ..."
> +completion, the waiting part through a call to a variant of > +wait_to_completion and the signaling side through a call to complete()
Let's use the function notation consistently (and get the name right while we're at it) - wait_for_completion()
> +or complete_all(). > + > +To use completions one needs to including <linux/completion.h> and > +creating a variable of type struct completion. The structure used for
"to include" and "create"
> +handling of completion is: > + > + struct completion { > + unsigned int done; > + wait_queue_head_t wait; > + }; > + > +providing the wait queue to place tasks on for waiting and the flag for > +indicating the state of affairs. > + > +Completions should be named to convey the intent of the waiter. A good > +example is: > + > + wait_for_completion(&early_console_added); > + > + complete(&early_console_added); > + > +good naming (as always) helps code readability. > + > + > +init_completion:
init_completion(). But even better would be "Initialization" or something like that.
> +---------------- > + > +Initialization is accomplished by init_completion() for dynamic
accomplished *by calling* init_completion()...
> +initialization. It initializes the wait-queue and sets the default state
wait queue (no hyphen)
> +to "not available", that is, "done" is set to 0. > + > +The reinitialization reinit_completion(), simply resets the done element
The reinitialization *function* reinit_completion()...
> +to "not available", thus again to 0, without touching the wait-queue. > + > +declaration and initialization macros available are:
*The* declaration and ...
> + > + static DECLARE_COMPLETION(setup_done) > + > +used for static declarations in file scope - probably NOT what you want to > +use - instead use:
There are some 50 uses, so it has its value.
> + DECLARE_COMPLETION_ONSTACK(setup_done) > + > +used for automatic/local variables on the stack and will make lockdep happy.
All this is good, but the predominant use looks to be embedding a completion directly into some other structure and initializing it explicitly. It might be worth finding a way to actually say that.
> +wait_for_completion:
wait_for_completion() (or "Waiting")
> +-------------------- > + > +For a thread of execution to wait on some other thread to reach some > +preparatory action to reach completion, this is achieved by passing the > +completion event to wait_for_completion():
That sentence is a bit hard to read. How about something like:
A thread may wait for a completion to be signaled by calling one of the variants of wait_for_completion().
> + > + wait_for_completion(struct completion *done):
Here (and with all of them) it would be nice to have the return type too. "void" in this case.
> +The default behavior is to wait without a timeout and mark the task as > +uninterruptible. > + > + > +Variants available are: > + > + wait_for_completion_interruptible(struct completion *done) > + > +marking the task TASK_INTERRUPTIBLE.
Return value? It's important here. It's -ERESTARTSYS if interrupted.
> + wait_for_completion_timeout(struct completion *done, > + unsigned long timeout)
Return value here too: it's the number of jiffies until the timeout - zero if the timeout happened.
> +passing a timeout in jiffies and marking the task as TASK_UNINTERRUPTIBLE. > + > + wait_for_completion_interruptible_timeout(struct completion *done, > + unsigned long timeout)
...and here too
> +passing a timeout in jiffies and marking the task as TASK_INTERRUPTIBLE. > + > +Further variants include _killable which passes TASK_KILLABLE as the > +designated tasks state and will return a -ERESTARTSYS if interrupted or > +else 0 if completion was achieved.
It's worth noting that a fatal signal has been received in that case. Nice to see the return value documented though! :)
> + wait_for_completion_killable(struct completion *done) > + wait_for_completion_killable_timeout(struct completion *done, > + unsigned long timeout) > + > +The _io variants wait_for_completion_io behave the same as the non-_io > +variants, except for accounting its waiting time as waiting on IO. > + > + wait_for_completion_io(struct completion *done) > + wait_for_completion_io_timeout(struct completion *done > + unsigned long timeout) > + > +complete:
complete() (or "Signaling completion")
> +--------- > + > +A thread of execution that wants to signal that the conditions for > +continuation have been achieved calls complete() to signal exactly one > +of the waiters that it can continue > + > + complete(struct completion *done) > + > +or calls complete_all to signal all current and future waiters. > + > + complete_all(struct completion *done) > + > +The signaling will work as expected even if completion is signaled before > +a thread starts waiting. This is achieved by the waiter "consuming" > +(decrementing) the done element of struct completion. > + > +If complete() is called multiple times then this will allow for that number > +of waiters to continue - each call to complete() will simply increment the > +done element. Calling complete_all() multiple times is a bug though. > + > + > +try_wait_for_completion()/completion_done(): > +-------------------------------------------- > + > +The try_wait_for_completion will not put the thread on the wait-queue but > +rather returns 0 if it would need to enqueue (block) the thread, else it > +consumes any posted completion and returns.
Um, it's a bool, so we should document the return value as such. Again, being explicit about return types is useful.
> + try_wait_for_completion(struct completion *done) > + > +Finally to check state of a completion without changing it in any way is > +provided by completion_done(); > + > + completion_done(struct completion *done) > + > + > +Constraints: > +============
The information in this section is all useful, but I really think it should be placed in the text above where it's relevant. Why make readers jump back and forth?
> + * DECLARE_COMPLETION should not be used for completion objects > + declared within functions (automatic variables) use > + DECLARE_COMPLETION_ONSTACK for that case. > + * calling init_completion() on the same completion object is most > + likely a bug - use reinit_completion() in that case. > + * Waiting threads wakeup order is the same in which they were > + enqueued (FIFO order). > + * There only can be one thread calling complete() or complete_all() > + on a particular struct completion at any time - serialized > + through the wait-queue spinlock. Any concurrent calls to > + complete() or complete_all() probably are a design bug though. > + * Calling complete() multiple time is permitted, calling > + complete_all() multiple times is very likely a bug. > + * Timeouts are in jiffies - use msecs_to_jiffies/usecs_to_jiffies to > + convert arguments. > + * By default wait_for_completion is neither interruptible nor will it > + time out - appropriate _interruptible/_timeout variants must be > + used. > + * With held locks only try_wait_for_completion is safe, all other > + variants can sleep.
With held *spinlocks*. One can hold a mutex, of course.
> + * The struct completion should be given a meaningful name - e.g. > + &cmd.complete or thread->started but not &completion. so that > + it is clear what is being waited on. > + * The completion API is basically RT safe as it only is using > + boostable locks but these could never the less be held for quite
"nevertheless"
> + lengthy periods of time. > + * In PREEMPT_RT the wait-queue used in queuing tasks is changed to a > + simple wait-queue to minimize the lock contention of the queue
again, "wait queue"
> + related lock. > + * PREEMPT_RT only changes the completion usage related to stop_machine
Should this information go in the -rt tree instead, to be merged when the relevant code is? It's hard enough to keep these things current when they're in the same tree.
> +Code that thinks of using yield() or some quirky msleep(1); loop to allow > +something else to proceed probably wants to look into using > +wait_for_completion() instead.
Thanks for doing this!
jon
| |