[v2,1/2] Fixed a thread safe bug.

Submitted by Zhigang Gong on July 15, 2015, 12:58 a.m.

Details

Message ID 1436921892-31864-1-git-send-email-zhigang.gong@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Zhigang Gong July 15, 2015, 12:58 a.m.
From: Zhigang Gong <zhigang.gong@linux.intel.com>

last_event and current_event should be thread private data.

Signed-off-by: Zhigang Gong <zhigang.gong@linux.intel.com>
---
 src/cl_api.c           |  2 +-
 src/cl_command_queue.c | 17 +++++++++++------
 src/cl_command_queue.h |  2 --
 src/cl_event.c         | 18 +++++++++---------
 src/cl_thread.c        | 30 ++++++++++++++++++++++++++++++
 src/cl_thread.h        |  5 +++++
 6 files changed, 56 insertions(+), 18 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/cl_api.c b/src/cl_api.c
index 1ba775f..3d79dcd 100644
--- a/src/cl_api.c
+++ b/src/cl_api.c
@@ -85,7 +85,7 @@  handle_events(cl_command_queue queue, cl_int num, const cl_event *wait_list,
       cl_event_new_enqueue_callback(e, data, num, wait_list);
     }
   }
-  queue->current_event = e;
+  set_current_event(queue, e);
   return status;
 }
 
diff --git a/src/cl_command_queue.c b/src/cl_command_queue.c
index da71fb7..4e4ebfb 100644
--- a/src/cl_command_queue.c
+++ b/src/cl_command_queue.c
@@ -78,8 +78,9 @@  cl_command_queue_delete(cl_command_queue queue)
 
   // If there is a valid last event, we need to give it a chance to
   // call the call-back function.
-  if (queue->last_event && queue->last_event->user_cb)
-    cl_event_update_status(queue->last_event, 1);
+  cl_event last_event = get_last_event(queue);
+  if (last_event && last_event->user_cb)
+    cl_event_update_status(last_event, 1);
   /* Remove it from the list */
   assert(queue->ctx);
   pthread_mutex_lock(&queue->ctx->queue_lock);
@@ -259,10 +260,14 @@  cl_command_queue_flush(cl_command_queue queue)
   // be released at the call back function, no other function will access
   // the event any more. If we don't do this here, we will leak that event
   // and all the corresponding buffers which is really bad.
-  if (queue->last_event && queue->last_event->user_cb)
-    cl_event_update_status(queue->last_event, 1);
-  if (queue->current_event && err == CL_SUCCESS)
-    err = cl_event_flush(queue->current_event);
+  cl_event last_event = get_last_event(queue);
+  if (last_event && last_event->user_cb)
+    cl_event_update_status(last_event, 1);
+  cl_event current_event = get_current_event(queue);
+  if (current_event && err == CL_SUCCESS) {
+    err = cl_event_flush(current_event);
+    set_current_event(queue, NULL);
+  }
   cl_invalid_thread_gpgpu(queue);
   return err;
 }
diff --git a/src/cl_command_queue.h b/src/cl_command_queue.h
index 91c941c..2cd6739 100644
--- a/src/cl_command_queue.h
+++ b/src/cl_command_queue.h
@@ -40,8 +40,6 @@  struct _cl_command_queue {
   cl_event* wait_events;               /* Point to array of non-complete user events that block this command queue */
   cl_int    wait_events_num;           /* Number of Non-complete user events */
   cl_int    wait_events_size;          /* The size of array that wait_events point to */
-  cl_event  last_event;                /* The last event in the queue, for enqueue mark used */
-  cl_event  current_event;             /* Current event. */
   cl_command_queue_properties  props;  /* Queue properties */
   cl_command_queue prev, next;         /* We chain the command queues together */
   void *thread_data;                   /* Used to store thread context data */
diff --git a/src/cl_event.c b/src/cl_event.c
index b4734b2..56778ad 100644
--- a/src/cl_event.c
+++ b/src/cl_event.c
@@ -56,7 +56,7 @@  int cl_event_flush(cl_event event)
     event->gpgpu = NULL;
   }
   cl_gpgpu_event_flush(event->gpgpu_event);
-  event->queue->last_event = event;
+  set_last_event(event->queue, event);
   return err;
 }
 
@@ -117,8 +117,8 @@  void cl_event_delete(cl_event event)
   if (atomic_dec(&event->ref_n) > 1)
     return;
 
-  if(event->queue && event->queue->last_event == event)
-    event->queue->last_event = NULL;
+  if(event->queue && get_last_event(event->queue) == event)
+    set_last_event(event->queue, NULL);
 
   /* Call all user's callback if haven't execute */
   cl_event_call_callback(event, CL_COMPLETE, CL_TRUE); // CL_COMPLETE status will force all callbacks that are not executed to run
@@ -568,9 +568,9 @@  cl_int cl_event_marker_with_wait_list(cl_command_queue queue,
     return CL_SUCCESS;
   }
 
-  if(queue->last_event && queue->last_event->gpgpu_event) {
-    cl_gpgpu_event_update_status(queue->last_event->gpgpu_event, 1);
-  }
+  cl_event last_event = get_last_event(queue);
+  if(last_event && last_event->gpgpu_event)
+    cl_gpgpu_event_update_status(last_event->gpgpu_event, 1);
 
   cl_event_set_status(e, CL_COMPLETE);
   return CL_SUCCESS;
@@ -605,9 +605,9 @@  cl_int cl_event_barrier_with_wait_list(cl_command_queue queue,
     return CL_SUCCESS;
   }
 
-  if(queue->last_event && queue->last_event->gpgpu_event) {
-    cl_gpgpu_event_update_status(queue->last_event->gpgpu_event, 1);
-  }
+  cl_event last_event = get_last_event(queue);
+  if(last_event && last_event->gpgpu_event)
+    cl_gpgpu_event_update_status(last_event->gpgpu_event, 1);
 
   cl_event_set_status(e, CL_COMPLETE);
   return CL_SUCCESS;
diff --git a/src/cl_thread.c b/src/cl_thread.c
index 0d99574..5e5a351 100644
--- a/src/cl_thread.c
+++ b/src/cl_thread.c
@@ -45,6 +45,8 @@  typedef struct _thread_spec_data {
   cl_gpgpu gpgpu ;
   int valid;
   void* thread_batch_buf;
+  cl_event last_event;
+  cl_event current_event;
   int thread_magic;
 } thread_spec_data;
 
@@ -106,6 +108,34 @@  static thread_spec_data * __create_thread_spec_data(cl_command_queue queue, int
   return spec;
 }
 
+cl_event get_current_event(cl_command_queue queue)
+{
+  thread_spec_data* spec = __create_thread_spec_data(queue, 1);
+  assert(spec && spec->thread_magic == thread_magic);
+  return spec->current_event;
+}
+
+cl_event get_last_event(cl_command_queue queue)
+{
+  thread_spec_data* spec = __create_thread_spec_data(queue, 1);
+  assert(spec && spec->thread_magic == thread_magic);
+  return spec->last_event;
+}
+
+void set_current_event(cl_command_queue queue, cl_event e)
+{
+  thread_spec_data* spec = __create_thread_spec_data(queue, 1);
+  assert(spec && spec->thread_magic == thread_magic);
+  spec->current_event = e;
+}
+
+void set_last_event(cl_command_queue queue, cl_event e)
+{
+  thread_spec_data* spec = __create_thread_spec_data(queue, 1);
+  assert(spec && spec->thread_magic == thread_magic);
+  spec->last_event = e;
+}
+
 void* cl_thread_data_create(void)
 {
   queue_thread_private* thread_private = CALLOC(queue_thread_private);
diff --git a/src/cl_thread.h b/src/cl_thread.h
index 7b48a26..d77526b 100644
--- a/src/cl_thread.h
+++ b/src/cl_thread.h
@@ -44,4 +44,9 @@  void* cl_get_thread_batch_buf(cl_command_queue queue);
 /* take current gpgpu from the thread gpgpu pool. */
 cl_gpgpu cl_thread_gpgpu_take(cl_command_queue queue);
 
+cl_event get_current_event(cl_command_queue queue);
+cl_event get_last_event(cl_command_queue queue);
+void set_current_event(cl_command_queue queue, cl_event e);
+void set_last_event(cl_command_queue queue, cl_event e);
+
 #endif /* __CL_THREAD_H__ */

Comments

The patchset LGTM, pushed, thanks.

> -----Original Message-----

> From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf Of

> Zhigang Gong

> Sent: Wednesday, July 15, 2015 08:58

> To: beignet@lists.freedesktop.org

> Cc: Zhigang Gong

> Subject: [Beignet] [Patch v2 1/2] Fixed a thread safe bug.

> 

> From: Zhigang Gong <zhigang.gong@linux.intel.com>

> 

> last_event and current_event should be thread private data.

> 

> Signed-off-by: Zhigang Gong <zhigang.gong@linux.intel.com>

> ---

>  src/cl_api.c           |  2 +-

>  src/cl_command_queue.c | 17 +++++++++++------

> src/cl_command_queue.h |  2 --

>  src/cl_event.c         | 18 +++++++++---------

>  src/cl_thread.c        | 30 ++++++++++++++++++++++++++++++

>  src/cl_thread.h        |  5 +++++

>  6 files changed, 56 insertions(+), 18 deletions(-)

> 

> diff --git a/src/cl_api.c b/src/cl_api.c index 1ba775f..3d79dcd 100644

> --- a/src/cl_api.c

> +++ b/src/cl_api.c

> @@ -85,7 +85,7 @@ handle_events(cl_command_queue queue, cl_int num,

> const cl_event *wait_list,

>        cl_event_new_enqueue_callback(e, data, num, wait_list);

>      }

>    }

> -  queue->current_event = e;

> +  set_current_event(queue, e);

>    return status;

>  }

> 

> diff --git a/src/cl_command_queue.c b/src/cl_command_queue.c index

> da71fb7..4e4ebfb 100644

> --- a/src/cl_command_queue.c

> +++ b/src/cl_command_queue.c

> @@ -78,8 +78,9 @@ cl_command_queue_delete(cl_command_queue

> queue)

> 

>    // If there is a valid last event, we need to give it a chance to

>    // call the call-back function.

> -  if (queue->last_event && queue->last_event->user_cb)

> -    cl_event_update_status(queue->last_event, 1);

> +  cl_event last_event = get_last_event(queue);  if (last_event &&

> + last_event->user_cb)

> +    cl_event_update_status(last_event, 1);

>    /* Remove it from the list */

>    assert(queue->ctx);

>    pthread_mutex_lock(&queue->ctx->queue_lock);

> @@ -259,10 +260,14 @@ cl_command_queue_flush(cl_command_queue

> queue)

>    // be released at the call back function, no other function will access

>    // the event any more. If we don't do this here, we will leak that event

>    // and all the corresponding buffers which is really bad.

> -  if (queue->last_event && queue->last_event->user_cb)

> -    cl_event_update_status(queue->last_event, 1);

> -  if (queue->current_event && err == CL_SUCCESS)

> -    err = cl_event_flush(queue->current_event);

> +  cl_event last_event = get_last_event(queue);  if (last_event &&

> + last_event->user_cb)

> +    cl_event_update_status(last_event, 1);  cl_event current_event =

> + get_current_event(queue);  if (current_event && err == CL_SUCCESS) {

> +    err = cl_event_flush(current_event);

> +    set_current_event(queue, NULL);

> +  }

>    cl_invalid_thread_gpgpu(queue);

>    return err;

>  }

> diff --git a/src/cl_command_queue.h b/src/cl_command_queue.h index

> 91c941c..2cd6739 100644

> --- a/src/cl_command_queue.h

> +++ b/src/cl_command_queue.h

> @@ -40,8 +40,6 @@ struct _cl_command_queue {

>    cl_event* wait_events;               /* Point to array of non-complete user

> events that block this command queue */

>    cl_int    wait_events_num;           /* Number of Non-complete user events

> */

>    cl_int    wait_events_size;          /* The size of array that wait_events point to

> */

> -  cl_event  last_event;                /* The last event in the queue, for enqueue

> mark used */

> -  cl_event  current_event;             /* Current event. */

>    cl_command_queue_properties  props;  /* Queue properties */

>    cl_command_queue prev, next;         /* We chain the command queues

> together */

>    void *thread_data;                   /* Used to store thread context data */

> diff --git a/src/cl_event.c b/src/cl_event.c index b4734b2..56778ad 100644

> --- a/src/cl_event.c

> +++ b/src/cl_event.c

> @@ -56,7 +56,7 @@ int cl_event_flush(cl_event event)

>      event->gpgpu = NULL;

>    }

>    cl_gpgpu_event_flush(event->gpgpu_event);

> -  event->queue->last_event = event;

> +  set_last_event(event->queue, event);

>    return err;

>  }

> 

> @@ -117,8 +117,8 @@ void cl_event_delete(cl_event event)

>    if (atomic_dec(&event->ref_n) > 1)

>      return;

> 

> -  if(event->queue && event->queue->last_event == event)

> -    event->queue->last_event = NULL;

> +  if(event->queue && get_last_event(event->queue) == event)

> +    set_last_event(event->queue, NULL);

> 

>    /* Call all user's callback if haven't execute */

>    cl_event_call_callback(event, CL_COMPLETE, CL_TRUE); // CL_COMPLETE

> status will force all callbacks that are not executed to run @@ -568,9 +568,9

> @@ cl_int cl_event_marker_with_wait_list(cl_command_queue queue,

>      return CL_SUCCESS;

>    }

> 

> -  if(queue->last_event && queue->last_event->gpgpu_event) {

> -    cl_gpgpu_event_update_status(queue->last_event->gpgpu_event, 1);

> -  }

> +  cl_event last_event = get_last_event(queue);  if(last_event &&

> + last_event->gpgpu_event)

> +    cl_gpgpu_event_update_status(last_event->gpgpu_event, 1);

> 

>    cl_event_set_status(e, CL_COMPLETE);

>    return CL_SUCCESS;

> @@ -605,9 +605,9 @@ cl_int

> cl_event_barrier_with_wait_list(cl_command_queue queue,

>      return CL_SUCCESS;

>    }

> 

> -  if(queue->last_event && queue->last_event->gpgpu_event) {

> -    cl_gpgpu_event_update_status(queue->last_event->gpgpu_event, 1);

> -  }

> +  cl_event last_event = get_last_event(queue);  if(last_event &&

> + last_event->gpgpu_event)

> +    cl_gpgpu_event_update_status(last_event->gpgpu_event, 1);

> 

>    cl_event_set_status(e, CL_COMPLETE);

>    return CL_SUCCESS;

> diff --git a/src/cl_thread.c b/src/cl_thread.c index 0d99574..5e5a351 100644

> --- a/src/cl_thread.c

> +++ b/src/cl_thread.c

> @@ -45,6 +45,8 @@ typedef struct _thread_spec_data {

>    cl_gpgpu gpgpu ;

>    int valid;

>    void* thread_batch_buf;

> +  cl_event last_event;

> +  cl_event current_event;

>    int thread_magic;

>  } thread_spec_data;

> 

> @@ -106,6 +108,34 @@ static thread_spec_data *

> __create_thread_spec_data(cl_command_queue queue, int

>    return spec;

>  }

> 

> +cl_event get_current_event(cl_command_queue queue) {

> +  thread_spec_data* spec = __create_thread_spec_data(queue, 1);

> +  assert(spec && spec->thread_magic == thread_magic);

> +  return spec->current_event;

> +}

> +

> +cl_event get_last_event(cl_command_queue queue) {

> +  thread_spec_data* spec = __create_thread_spec_data(queue, 1);

> +  assert(spec && spec->thread_magic == thread_magic);

> +  return spec->last_event;

> +}

> +

> +void set_current_event(cl_command_queue queue, cl_event e) {

> +  thread_spec_data* spec = __create_thread_spec_data(queue, 1);

> +  assert(spec && spec->thread_magic == thread_magic);

> +  spec->current_event = e;

> +}

> +

> +void set_last_event(cl_command_queue queue, cl_event e) {

> +  thread_spec_data* spec = __create_thread_spec_data(queue, 1);

> +  assert(spec && spec->thread_magic == thread_magic);

> +  spec->last_event = e;

> +}

> +

>  void* cl_thread_data_create(void)

>  {

>    queue_thread_private* thread_private = CALLOC(queue_thread_private);

> diff --git a/src/cl_thread.h b/src/cl_thread.h index 7b48a26..d77526b 100644

> --- a/src/cl_thread.h

> +++ b/src/cl_thread.h

> @@ -44,4 +44,9 @@ void* cl_get_thread_batch_buf(cl_command_queue

> queue);

>  /* take current gpgpu from the thread gpgpu pool. */  cl_gpgpu

> cl_thread_gpgpu_take(cl_command_queue queue);

> 

> +cl_event get_current_event(cl_command_queue queue); cl_event

> +get_last_event(cl_command_queue queue); void

> +set_current_event(cl_command_queue queue, cl_event e); void

> +set_last_event(cl_command_queue queue, cl_event e);

> +

>  #endif /* __CL_THREAD_H__ */

> --

> 1.9.1

> 

> _______________________________________________

> Beignet mailing list

> Beignet@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/beignet