[Mesa-dev] [PATCH 3/5] clover: Fix a bug with multi-threaded events

[Francisco Jerez]

Tom Stellard <thomas.stellard at amd.com> writes:

> It was possible for some events never to get triggered if one thread
> was creating events and another threads was waiting for them.
>
> This patch consolidates soft_event::wait() and hard_event::wait()
> into event::wait() so that hard_event objects will now wait for
> all their dependencies to be submitted before flushing the command
> queue.
> ---
>  src/gallium/state_trackers/clover/core/event.cpp | 19 +++++++++++++++----
>  src/gallium/state_trackers/clover/core/event.hpp |  9 ++++++---
>  2 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/core/event.cpp b/src/gallium/state_trackers/clover/core/event.cpp
> index 3c9336e..da227bb 100644
> --- a/src/gallium/state_trackers/clover/core/event.cpp
> +++ b/src/gallium/state_trackers/clover/core/event.cpp
> @@ -39,6 +39,7 @@ event::~event() {
>  void
>  event::trigger() {
>     if (!--wait_count) {
> +      signalled_cv.notify_all();
>        action_ok(*this);
>  
>        while (!_chain.empty()) {
> @@ -73,6 +74,15 @@ event::chain(event &ev) {
>     ev.deps.push_back(*this);
>  }
>  
> +void
> +event::wait() {
> +   for (event &ev : deps)
> +      ev.wait();
> +
> +   std::unique_lock<std::mutex> lock(signalled_mutex);
> +   signalled_cv.wait(lock, [=]{ return signalled(); });

When we implement proper locking in signalled() this is going to
deadlock, let's open-code it like "return !wait_count;".

> +}
> +
>  hard_event::hard_event(command_queue &q, cl_command_type command,
>                         const ref_vector<event> &deps, action action) :
>     event(q.context(), deps, profile(q, action), [](event &ev){}),
> @@ -117,9 +127,11 @@ hard_event::command() const {
>  }
>  
>  void
> -hard_event::wait() const {
> +hard_event::wait() {
>     pipe_screen *screen = queue()->device().pipe;
>  
> +   event::wait();
> +
>     if (status() == CL_QUEUED)
>        queue()->flush();
>  
> @@ -206,9 +218,8 @@ soft_event::command() const {
>  }
>  
>  void
> -soft_event::wait() const {
> -   for (event &ev : deps)
> -      ev.wait();
> +soft_event::wait() {
> +   event::wait();
>  
>     if (status() != CL_COMPLETE)
>        throw error(CL_EXEC_STATUS_ERROR_FOR_EVENTS_IN_WAIT_LIST);
> diff --git a/src/gallium/state_trackers/clover/core/event.hpp b/src/gallium/state_trackers/clover/core/event.hpp
> index d407c80..dffafb9 100644
> --- a/src/gallium/state_trackers/clover/core/event.hpp
> +++ b/src/gallium/state_trackers/clover/core/event.hpp
> @@ -23,6 +23,7 @@
>  #ifndef CLOVER_CORE_EVENT_HPP
>  #define CLOVER_CORE_EVENT_HPP
>  
> +#include <condition_variable>
>  #include <functional>
>  
>  #include "core/object.hpp"
> @@ -68,7 +69,7 @@ namespace clover {
>        virtual cl_int status() const = 0;
>        virtual command_queue *queue() const = 0;
>        virtual cl_command_type command() const = 0;
> -      virtual void wait() const = 0;
> +      virtual void wait();

I think I would like to keep wait() const, because it doesn't change the
state of the object in any appreciable way for an outside observer --
The fact that you may have to lock a mutex is just an implementation
detail.

>  
>        virtual struct pipe_fence_handle *fence() const {
>           return NULL;
> @@ -87,6 +88,8 @@ namespace clover {
>        action action_ok;
>        action action_fail;
>        std::vector<intrusive_ref<event>> _chain;
> +      std::condition_variable signalled_cv;
> +      std::mutex signalled_mutex;

We should mark these two as mutable so they can be modified by read-only
methods like signalled() or wait().  I suggest you simply call them "cv"
and "mutex" so people don't have the temptation of adding another mutex
or cv to this object in the future, one per event should be enough. ;)

>     };
>  
>     ///
> @@ -111,7 +114,7 @@ namespace clover {
>        virtual cl_int status() const;
>        virtual command_queue *queue() const;
>        virtual cl_command_type command() const;
> -      virtual void wait() const;
> +      virtual void wait();

Same here,

>  
>        const lazy<cl_ulong> &time_queued() const;
>        const lazy<cl_ulong> &time_submit() const;
> @@ -149,7 +152,7 @@ namespace clover {
>        virtual cl_int status() const;
>        virtual command_queue *queue() const;
>        virtual cl_command_type command() const;
> -      virtual void wait() const;
> +      virtual void wait();

and here.

With these fixed:
Reviewed-by: Francisco Jerez <currojerez at riseup.net>

>     };
>  }
>  
> -- 
> 2.0.4
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150509/b96b9b60/attachment.sig>