[Beignet] [PATCH 1/2] Fixed a thread safe bug.
Zhigang Gong
zhigang.gong at linux.intel.com
Tue Jul 14 17:58:50 PDT 2015
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 at linux.intel.com>
>
> last_event and current_event should be thread private data.
>
> Signed-off-by: Zhigang Gong <zhigang.gong at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
More information about the Beignet
mailing list