On Wed, 31 Dec 2014, Jonathan Corbet wrote:
> 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 :)
>
thanks for the detailed feedback - obviously this needs at least
one more round of cleanup and refinement - will give it a further
run and repost it
thx!
hofrat
> On Tue, 23 Dec 2014 20:41:39 +0100
> Nicholas Mc Guire <der.herr@xxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
