Right, the patch embedded in previous email has format problem. Could you use "git send-email" to send the patch again?
Thanks for your contribution. On Tue, Mar 24, 2015 at 04:58:10AM +0000, Yang, Rong R wrote: > 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 _______________________________________________ Beignet mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/beignet
