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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
