lkml.org 
[lkml]   [2016]   [Jul]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2016-07-13 13:01    [W:0.075 / U:1.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site