[Beignet] [PATCH] Fix event pthread_mutex_lock dead lock.

[Yang, Rong R]

Yes, If old thread set status to CL_ COMPLETE and new thread set to -1, will fail. I will add a check.

-----Original Message-----
From: beignet-bounces+rong.r.yang=intel.com at lists.freedesktop.org [mailto:beignet-bounces+rong.r.yang=intel.com at lists.freedesktop.org] On Behalf Of Zhigang Gong
Sent: Tuesday, August 13, 2013 5:21 PM
To: Yang, Rong R
Cc: beignet at lists.freedesktop.org
Subject: Re: [Beignet] [PATCH] Fix event pthread_mutex_lock dead lock.

Rong,

I took a look at the patch, and it seems this fix is not totally correct.

Considering one scenario, the thread switch to a new thread at the
pthread_mutex_unlock(...) before the for-loop. And in the new thread, it set same event to CL_COMPLETE or -1. Then the new thread will enter this critical region and may run to the event->enqueue_cb = NULL; then when it turn back to the original thread and continue to excute the cl_event_delete(event->enqueue_cb->wait_list[i]), it triggers segfault.

Although the above scenario is not a good usage of the event object, but the spec doesn't have such a restrication which prevent the application uses it that way.

On Tue, Aug 13, 2013 at 04:28:51PM +0800, Yang Rong wrote:
> In function cl_event_set_status, between pthread_mutex_lock and 
> pthread_mutex_unlock will call cl_event_delete, which also require the same lock, cause deak lock.
> Unlock it before call cl_event_delete.
> 
> Signed-off-by: Yang Rong <rong.r.yang at intel.com>
> ---
>  src/cl_event.c |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/src/cl_event.c b/src/cl_event.c index 48f24e5..3dabdb3 
> 100644
> --- a/src/cl_event.c
> +++ b/src/cl_event.c
> @@ -305,23 +305,30 @@ void cl_event_set_status(cl_event event, cl_int 
> status)
>  
>    pthread_mutex_lock(&event->ctx->event_lock);
>    if(status >= event->status) {
> -   return;
> +    pthread_mutex_unlock(&event->ctx->event_lock);
> +    return;
>    }
>  
>    if(status <= CL_COMPLETE) {
>      if(event->enqueue_cb) {
> +      cl_enqueue_handle(&event->enqueue_cb->data);
> +      event->status = status;  //Change the event status after 
> + enqueue and befor unlock
> +
> +      pthread_mutex_unlock(&event->ctx->event_lock);
>        for(i=0; i<event->enqueue_cb->num_events; i++)
>          cl_event_delete(event->enqueue_cb->wait_list[i]);
> +      pthread_mutex_lock(&event->ctx->event_lock);
>  
> -      cl_enqueue_handle(&event->enqueue_cb->data);
>        cl_free(event->enqueue_cb);
>        event->enqueue_cb = NULL;
>      }
> -    cl_event_delete(event);
>    }
>    event->status = status;
>    pthread_mutex_unlock(&event->ctx->event_lock);
>  
> +  if(event->status <= CL_COMPLETE)
> +    cl_event_delete(event);
> +
>    /* Call user callback */
>    user_cb = event->user_cb;
>    while(user_cb) {
> --
> 1.7.10.4
> 
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
_______________________________________________
Beignet mailing list
Beignet at lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/beignet