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: <20160713102014.GC6157@nuc-i3427.alporthouse.com>
Date:	Wed, 13 Jul 2016 11:20:14 +0100
From:	Chris Wilson <chris@...is-wilson.co.uk>
To:	Peter Zijlstra <peterz@...radead.org>
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 Wed, Jul 13, 2016 at 11:38:52AM +0200, Peter Zijlstra wrote:
> 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?

I started in kernel/async (since my first goal was fine-grained
async_work serialisation). It is still in kernel/async.c as the embedded
fence inside the async_work needs a return code to integrate. I should
have done that before posting...

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

Agreed, I really want to rename the dma-buf fence to struct dma_fence -
we would need to do that whilst it dma-buf fencing is still in its infancy.
 
> > +/**
> > + * 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.

Ok, I was just copying the style from
Documentation/scheduler/completion.txt

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

Hmm. Something more like:

"To check the state of a kfence without changing it in any way, call
kfence_pending(), which returns true if the kfence is still waiting for
its event sources to be signaled."

s/signaled/completed/ depending on kfence_signal() vs kfence_complete()

> > + *
> > + *	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?

Possibly, but we also have the dma-buf fences to try and be fairly
consistent with. struct completion is definitely a closer sibling
though. The biggest conceptual change from completions though is that a
kfence will be signaled multiple times before it is complete - I think
that is a strong argument in favour of using _signal().

> > + *
> > + *	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.

Ok, will do a spin with completion naming convention and see how that
fits in (and complete the extraction to a separate .c)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre