Re: [PATCH v2 1/2] doc: brief user documentation for completion

[Nicholas Mc Guire]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/