linux-kernel - Re: [PATCH 2/9] async: Introduce kfence, a N:M completion mechanism

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160713093852.GZ30921@twins.programming.kicks-ass.net>
Date:	Wed, 13 Jul 2016 11:38:52 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Chris Wilson <chris@...is-wilson.co.uk>
Cc:	linux-kernel@...r.kernel.org,
	Sumit Semwal <sumit.semwal@...aro.org>,
	Shuah Khan <shuahkh@....samsung.com>,
	Tejun Heo <tj@...nel.org>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	Kees Cook <keescook@...omium.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	Davidlohr Bueso <dave@...olabs.net>,
	Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
	"David S. Miller" <davem@...emloft.net>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH 2/9] async: Introduce kfence, a N:M completion mechanism

On Fri, Jun 24, 2016 at 10:08:46AM +0100, Chris Wilson wrote:
> diff --git a/kernel/async.c b/kernel/async.c
> index d2edd6efec56..d0bcb7cc4884 100644
> --- a/kernel/async.c
> +++ b/kernel/async.c
> @@ -50,6 +50,7 @@ asynchronous and synchronous parts of the kernel.
>  
>  #include <linux/async.h>
>  #include <linux/atomic.h>
> +#include <linux/kfence.h>
>  #include <linux/ktime.h>
>  #include <linux/export.h>
>  #include <linux/wait.h>

So why does this live in async.c? It got its own header, why not also
give it its own .c file?

Also, I'm not a particular fan of the k* naming, but I see 'fence' is
already taken.

> +/**
> + * DOC: kfence overview
> + *
> + * kfences provide synchronisation barriers between multiple processes.
> + * They are very similar to completions, or a pthread_barrier. Where
> + * kfence differs from completions is their ability to track multiple
> + * event sources rather than being a singular "completion event". Similar
> + * to completions, multiple processes or other kfences can listen or wait
> + * upon a kfence to signal its completion.
> + *
> + * The kfence is a one-shot flag, signaling that work has progressed passed
> + * a certain point (as measured by completion of all events the kfence is
> + * listening for) and the waiters upon kfence may proceed.
> + *
> + * kfences provide both signaling and waiting routines:
> + *
> + *	kfence_pending()
> + *
> + * indicates that the kfence should itself wait for another signal. A
> + * kfence created by kfence_create() starts in the pending state.

I would much prefer:

 *  - kfence_pending(): indicates that the kfence should itself wait for
 *    another signal. A kfence created by kfence_create() starts in the
 *    pending state.

Which is much clearer in what text belongs where.

Also, what !? I don't get what this function does.

> + *
> + *	kfence_signal()
> + *
> + * undoes the earlier pending notification and allows the fence to complete
> + * if all pending events have then been signaled.

So I know _signal() is the posix thing, but seeing how we already
completions, how about being consistent with those and use _complete()
for this?

> + *
> + *	kfence_wait()
> + *
> + * allows the caller to sleep (uninterruptibly) until the fence is complete.

whitespace to separate the description of kfence_wait() from whatever
else follows.

> + * Meanwhile,
> + *
> + * 	kfence_complete()
> + *
> + * reports whether or not the kfence has been passed.

kfence_done(), again to match completions.

> + *
> + * This interface is very similar to completions, with the exception of
> + * allowing multiple pending / signals to be sent before the kfence is
> + * complete.
> + *
> + *	kfence_add() / kfence_add_completion()
> + *
> + * sets the kfence to wait upon another fence, or completion respectively.
> + *
> + * Unlike completions, kfences are expected to live inside more complex graphs
> + * and form the basis for parallel execution of interdependent and so are
> + * reference counted. A kfence may be created using,
> + *
> + * 	kfence_create()
> + *
> + * The fence starts off pending a single signal. Once you have finished
> + * setting up the fence, use kfence_signal() to allow it to wait upon
> + * its event sources.
> + *
> + * Use
> + *
> + *	kfence_get() / kfence_put
> + *
> + * to acquire or release a reference on kfence respectively.
> + */