[1/2] Fixed a thread safe bug.

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

Details

Message ID 1436921674-31248-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:54 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           |  4 +++-
 src/cl_command_queue.c | 18 ++++++++++++------
 src/cl_command_queue.h |  2 --
 src/cl_event.c         | 18 +++++++++---------
 src/cl_thread.c        | 34 ++++++++++++++++++++++++++++++++++
 src/cl_thread.h        |  5 +++++
 6 files changed, 63 insertions(+), 18 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/cl_api.c b/src/cl_api.c
index 1ba775f..9fdf526 100644
--- a/src/cl_api.c
+++ b/src/cl_api.c
@@ -85,7 +85,9 @@  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;
+  if (e)
+    printf("new current event %p \n", e);
+  set_current_event(queue, e);
   return status;
 }
 
diff --git a/src/cl_command_queue.c b/src/cl_command_queue.c
index da71fb7..0afc459 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,15 @@  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) {
+    printf("flush current event %p\n", current_event);
+    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..bec8a30 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,38 @@  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);
+  printf("get thread %p, cur %p\n", spec, spec->current_event);
+  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);
+  printf("get thread %p, las %p\n", spec, spec->last_event);
+  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;
+  printf("set thread %p, cur %p\n", spec, spec->current_event);
+}
+
+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;
+  printf("set thread %p, las %p\n", spec, spec->last_event);
+}
+
 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

Please ignore this version as I forgot to remove the debug message.
Just sent out the version 2.

Thanks,
Zhigang Gong.

On Wed, Jul 15, 2015 at 08:54:33AM +0800, Zhigang Gong wrote:
> 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           |  4 +++-
>  src/cl_command_queue.c | 18 ++++++++++++------
>  src/cl_command_queue.h |  2 --
>  src/cl_event.c         | 18 +++++++++---------
>  src/cl_thread.c        | 34 ++++++++++++++++++++++++++++++++++
>  src/cl_thread.h        |  5 +++++
>  6 files changed, 63 insertions(+), 18 deletions(-)
> 
> diff --git a/src/cl_api.c b/src/cl_api.c
> index 1ba775f..9fdf526 100644
> --- a/src/cl_api.c
> +++ b/src/cl_api.c
> @@ -85,7 +85,9 @@ 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;
> +  if (e)
> +    printf("new current event %p \n", e);
> +  set_current_event(queue, e);
>    return status;
>  }
>  
> diff --git a/src/cl_command_queue.c b/src/cl_command_queue.c
> index da71fb7..0afc459 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,15 @@ 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) {
> +    printf("flush current event %p\n", current_event);
> +    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..bec8a30 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,38 @@ 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);
> +  printf("get thread %p, cur %p\n", spec, spec->current_event);
> +  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);
> +  printf("get thread %p, las %p\n", spec, spec->last_event);
> +  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;
> +  printf("set thread %p, cur %p\n", spec, spec->current_event);
> +}
> +
> +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;
> +  printf("set thread %p, las %p\n", spec, spec->last_event);
> +}
> +
>  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