The patch looks good to me. Can you send this patch individually? Thanks.
> -----Original Message----- > From: Beignet [mailto:[email protected]] On Behalf Of > David Couturier > Sent: Saturday, March 21, 2015 05:09 > To: [email protected] > Subject: Re: [Beignet] [PATCH] Fix: Event callback that not executed when > command already marked CL_COMPLETE > > I modified the commit as suggested. Also, I noticed that the callback handling > was not thread safe. I modified the general process to be thread safe. > > # PATCH BEGINS HERE: > > When trying to register a callback on the clEnqueueReadBuffer command, > since it is processed synchroniously all the time, the command was marked > CL_COMPLETE every time. If the event returned by clEnqueueReadBuffer > was then used to register a callback function, the callback function did no > check to execute it if nessary. > > Modified the handling of the callback registration in cl_set_event_callback to > only call the callback being created if it's status is already reached. > > Added thread safety measures for pfn_notify calls since the status value can > be changed while executing the callback. > > Grouped the pfn_notify calls to a unified function cl_event_call_callback that > handles thread safety: it queues callbacks in a node list while under the > protection of pthread_mutex and then calls the callbacks outside of the > pthread_mutex (this is required because the callback can deadlock if it calls > a > cl_api function that uses the mutex) > > Signed-off-by: David Couturier <[email protected]> > --- > src/cl_event.c | 77 > ++++++++++++++++++++++++++++++++++++++++++---------------- > src/cl_event.h | 4 ++- > 2 files changed, 59 insertions(+), 22 deletions(-) > > diff --git a/src/cl_event.c b/src/cl_event.c index f70e531..eb5d54b 100644 > --- a/src/cl_event.c > +++ b/src/cl_event.c > @@ -119,16 +119,7 @@ void cl_event_delete(cl_event event) > event->queue->last_event = NULL; > > /* Call all user's callback if haven't execute */ > - user_callback *cb = event->user_cb; > - while(event->user_cb) { > - cb = event->user_cb; > - if(cb->executed == CL_FALSE) { > - cb->executed = CL_TRUE; > - cb->pfn_notify(event, event->status, cb->user_data); > - } > - event->user_cb = cb->next; > - cl_free(cb); > - } > + cl_event_call_callback(event, CL_COMPLETE, CL_TRUE); // CL_COMPLETE > status will force all callbacks that are not executed to run > > /* delete gpgpu event object */ > if(event->gpgpu_event) > @@ -180,8 +171,22 @@ cl_int cl_event_set_callback(cl_event event , > cb->status = command_exec_callback_type; > cb->executed = CL_FALSE; > > - cb->next = event->user_cb; > - event->user_cb = cb; > + > + // It is possible that the event enqueued is already completed. > + // clEnqueueReadBuffer can be synchronous and when the callback > + // is registered after, it still needs to get executed. > + pthread_mutex_lock(&event->ctx->event_lock); // Thread safety > required: operations on the event->status can be made from many > different threads > + if(event->status <= command_exec_callback_type) { > + /* Call user callback */ > + pthread_mutex_unlock(&event->ctx->event_lock); // pfn_notify > can > call clFunctions that use the event_lock and from here it's not required > + cb->pfn_notify(event, event->status, cb->user_data); > + cl_free(cb); > + } else { > + // Enqueue to callback list > + cb->next = event->user_cb; > + event->user_cb = cb; > + pthread_mutex_unlock(&event->ctx->event_lock); > + } > > exit: > return err; > @@ -388,9 +393,46 @@ error: > goto exit; > } > > +void cl_event_call_callback(cl_event event, cl_int status, cl_bool > free_cb) { > + user_callback *user_cb = NULL; > + user_callback *queue_cb = NULL; // For thread safety, we create a > queue that holds user_callback's pfn_notify contents > + user_callback *temp_cb = NULL; > + user_cb = event->user_cb; > + pthread_mutex_lock(&event->ctx->event_lock); > + while(user_cb) { > + if(user_cb->status >= status > + && user_cb->executed == CL_FALSE) { // > Added check to not execute a > callback when it was already handled > + user_cb->executed = CL_TRUE; > + temp_cb = cl_malloc(sizeof(user_callback)); > + if(!temp_cb) { > + break; // Out of memory > + } > + temp_cb->pfn_notify = user_cb->pfn_notify; // > Minor struct copy to > call ppfn_notify out of the pthread_mutex > + temp_cb->user_data = user_cb->user_data; > + if(free_cb) { > + cl_free(user_cb); > + } > + if(!queue_cb) { > + queue_cb = temp_cb; > + queue_cb->next = NULL; > + } else { // Enqueue > + temp_cb->next = queue_cb; > + queue_cb->next = temp_cb; > + } > + } > + user_cb = user_cb->next; > + } > + pthread_mutex_unlock(&event->ctx->event_lock); > + // Calling the callbacks outside of the event_lock is required because > the callback can call cl_api functions and get deadlocked > + while(queue_cb) { // For each callback queued, actually execute the > callback > + queue_cb->pfn_notify(event, event->status, queue_cb- > >user_data); > + temp_cb = queue_cb; > + queue_cb = queue_cb->next; > + cl_free(temp_cb); > + } > +} > void cl_event_set_status(cl_event event, cl_int status) > { > - user_callback *user_cb; > cl_int ret, i; > cl_event evt; > > @@ -437,14 +479,7 @@ void cl_event_set_status(cl_event event, cl_int > status) > pthread_mutex_unlock(&event->ctx->event_lock); > > /* Call user callback */ > - user_cb = event->user_cb; > - while(user_cb) { > - if(user_cb->status >= status) { > - user_cb->executed = CL_TRUE; > - user_cb->pfn_notify(event, event->status, user_cb->user_data); > - } > - user_cb = user_cb->next; > - } > + cl_event_call_callback(event, status, CL_FALSE); > > if(event->type == CL_COMMAND_USER) { > /* Check all defer enqueue */ > diff --git a/src/cl_event.h b/src/cl_event.h > index 0730530..9bb2ac8 100644 > --- a/src/cl_event.h > +++ b/src/cl_event.h > @@ -78,8 +78,10 @@ cl_event cl_event_new(cl_context, > cl_command_queue, > cl_command_type, cl_bool); > void cl_event_delete(cl_event); > /* Add one more reference to this object */ > void cl_event_add_ref(cl_event); > -/* Rigister a user callback function for specific commond execution > status */ > +/* Register a user callback function for specific commond execution > status */ > cl_int cl_event_set_callback(cl_event, cl_int, EVENT_NOTIFY, void *); > +/* Execute the event's callback if the event's status supersedes the > callback's status. Free the callback if specified */ > +void cl_event_call_callback(cl_event event, cl_int status, cl_bool > free_cb); > /* Check events wait list for enqueue commonds */ > cl_int cl_event_check_waitlist(cl_uint, const cl_event *, cl_event *, > cl_context); > /* Wait the all events in wait list complete */ > -- > 1.9.1 > > > One comment. Thanks find and fix it. > > > >> -----Original Message----- > >> From: Beignet [mailto:[email protected]] On Behalf > Of > >> David Couturier > >> Sent: Friday, March 20, 2015 08:20 > >> To: Zou, Nanhai > >> Cc: [email protected] > >> Subject: [Beignet] [PATCH] Fix: Event callback that not executed when > >> command already marked CL_COMPLETE > >> > >> When trying to register a callback on the clEnqueueReadBuffer command, > >> since it is processed synchroniously all the time, the command was > marked > >> CL_COMPLETE every time. If the event returned by clEnqueueReadBuffer > >> was then used to register a callback function, the callback function did no > >> check to execute it if nessary. > >> > >> Fixed by adding a check at the end of the cl_event_set_callback function. > >> > >> All tests passed. > >> > >> Signed-off-by: David Couturier <[email protected]> > >> --- > >> src/cl_event.c | 15 +++++++++++++++ > >> 1 file changed, 15 insertions(+) > >> > >> diff --git a/src/cl_event.c b/src/cl_event.c index f70e531..df4a5a5 100644 > >> --- a/src/cl_event.c > >> +++ b/src/cl_event.c > >> @@ -183,6 +183,21 @@ cl_int cl_event_set_callback(cl_event event , > >> cb->next = event->user_cb; > >> event->user_cb = cb; > >> > >> + // It is possible that the event enqueued is already completed. > >> + // clEnqueueReadBuffer can be synchronious and when the callback // > >> + is registered after, it still needs to get executed. > >> + if(event->status == CL_COMPLETE) { > >> + /* Call user callback */ > >> + user_callback *user_cb = event->user_cb; > >> + while(user_cb) { > >> + if(user_cb->status >= CL_COMPLETE) { > >> + user_cb->executed = CL_TRUE; > >> + user_cb->pfn_notify(event, event->status, > >> user_cb->user_data); > >> + } > >> + user_cb = user_cb->next; > >> + } > > > > I think only the current callback should be called. Assume the scenario: > > clEnqueueReadBuffer(......,ev); > > clSetEventCallback(ev, CL_SUBMITTED, ...); > > clSetEventCallback(ev, CL_COMPLETE, ....); > > In the second clSetEventCallback, the first callback have been executed, > only need execute the second callback. > > So need execute current callback when the event's status <= > command_exec_callback_type. > > > >> + } > >> + > >> exit: > >> return err; > >> error: > >> -- > >> 1.9.1 > >> _______________________________________________ > >> Beignet mailing list > >> [email protected] > >> http://lists.freedesktop.org/mailman/listinfo/beignet > > _______________________________________________ > > Beignet mailing list > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/beignet > > > _______________________________________________ > Beignet mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/beignet _______________________________________________ Beignet mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/beignet
