Re: [PATCH v3] doc: brief user documentation for completion

[Valdis.Kletnieks@vt.edu]

[-- Attachment #1: Type: text/plain, Size: 3607 bytes --]

On Thu, 29 Jan 2015 16:55:45 +0100, Nicholas Mc Guire said:
> Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> ---
>
> v3: cleanups and merged review notes from Jonathan Corbet <corbet@lwn.net>
>
> patch is against 3.19.0-rc5 -next-20150119
>
>  Documentation/scheduler/completion.txt |  243 ++++++++++++++++++++++++++++++++
>  1 file changed, 243 insertions(+)
>  create mode 100644 Documentation/scheduler/completion.txt
>
> diff --git a/Documentation/scheduler/completion.txt b/Documentation/scheduler/completion.txt
> new file mode 100644
> index 0000000..5396656
> --- /dev/null
> +++ b/Documentation/scheduler/completion.txt
> @@ -0,0 +1,243 @@

Cleaning up the English a bit... I think I found most of the issues...


> +misuse of locks. Any time you think of using yield() or some quirky
> +msleep(1); loop to allow something else to proceed, you probably wants to

s/wants to/want to/

> +To use completions one needs to include <linux/completions.h> and

To use completions, you need to....



> +good naming (as always) helps code readability.

s/good/Good/

> +The re-initialization function reinit_completions(), simply resets the

This needs either an additional comma after function  or lose the one.

> +For static declaration and initialization macros are available, these are:

initialization, macros are available.  These are:


> +used for static declarations in file scope. Within functions the static
> +initialization should always use:
> +
> +	DECLARE_COMPLETION_ONSTACK(setup_done)
> +
> +suitable for automatic/local variables on the stack and will make lockdep
> +happy.

This probably needs a big fat warning about making *sure* the completion
remains in-scope, and no references remain to on-stack data when the function
returns.


> +For a thread of execution to wait for some concurrent work to finish, it
> +will call wait_for_completion() on the initialized completion structure.

s/it will call/it calls/


> +Note that wait_for_completion() is calling spin_lock_irq/spin_unlock_irq
> +so it can only be called safely when you know that interrupts are enabled
> +calling it from hard-irq context will result in hard to detect spurious
> +enabling of interrupts.

interrupts are enabled.  Calling it from ...

> +As all variants of wait_for_completion() can (obviously) block for a long
> +time, one probably does not want to call this with held locks - see also

time, you probably don't want to....

> +The below variants all return status and this status should be checked in
> +most(/all) cases - in cases where the status is deliberately not checked you
> +probably want to make a note explaining this (e.g. see
> +arch/arm/kernel/smp.c:__cpu_up()).

Not a doc question - but should these be tagged __must_check to enforce it?

> +A common problem that occurred is to have unclean assignment of return types,

occurs

> +marking the task TASK_INTERRUPTIBLE. If a signal was received while waiting

while waiting,

> +it will return -ERESTARTSYS and 0 otherwise.



> +The _io variants wait_for_completions_io behave the same as the non-_io
> +variants, except for accounting waiting time as waiting on IO, which has
> +an impact on how scheduling is calculated only.

s/only//


> +The signaling will work as expected even if completions is signaled before

if completions are signaled

> +There only can be one thread calling complete() or complete_all() on a
> +particular struct completions at any time - serialized through the wait
> +queue spinlock. Any such concurrent calls to complete() or complete_all()
> +probably are a design bug though.

s/though//



[-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --]