[Beignet] [PATCH] Fix: Event callback that not executed when command already marked CL_COMPLETE
Zhigang Gong
zhigang.gong at linux.intel.com
Mon Mar 23 21:14:23 PDT 2015
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:beignet-bounces at lists.freedesktop.org] On Behalf Of
> > David Couturier
> > Sent: Saturday, March 21, 2015 05:09
> > To: beignet at lists.freedesktop.org
> > 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 <david.couturier at polymtl.ca>
> > ---
> > 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:beignet-bounces at lists.freedesktop.org] On Behalf
> > Of
> > >> David Couturier
> > >> Sent: Friday, March 20, 2015 08:20
> > >> To: Zou, Nanhai
> > >> Cc: beignet at lists.freedesktop.org
> > >> 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 <david.couturier at polymtl.ca>
> > >> ---
> > >> 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
> > >> 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
> > >
> > _______________________________________________
> > 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
More information about the Beignet
mailing list