[v2] GetEventProfilingInfo could query all event status.

Submitted by Zhigang Gong on Aug. 4, 2015, 5:27 a.m.

Details

Message ID 20150804052756.GA15140@ivb-gt2-rev4
State New
Headers show

Not browsing as part of any series.

Commit Message

Zhigang Gong Aug. 4, 2015, 5:27 a.m.
The spec only says if event is not in CL_COMPLETE status, it will
get CL_PROFILING_INFO_NOT_AVAILABLE. It doesn't mean a clFinish()
should update all events status. According to the spec, clFinish()
only need to make sure the previously enqueued task has been processed
and completed.

Form my point of view, if the application wants to call clGetEventProfilingInfo()
to get an event's profiling information, the application should call
clWaitForEvents() firstly. But given the fact that the specification is
not very clear for whether clFinish() should update all events' status
and some applications are really believe all events has been also updated
after the clFinish() call. We may make a work around in Beignet to make those
applications happy. Your patch is one solution and another may be better
solution may be to add cl_event_update_status(event, 0) when calling to
clGetEventProfilingInfo(). The patch is as below,

From a5a1b3f372d17f26cc20fba078490b61614f07e5 Mon Sep 17 00:00:00 2001
From: Zhigang Gong <zhigang.gong@intel.com>
Date: Tue, 4 Aug 2015 13:21:27 +0800
Subject: [PATCH] runtime: always try to update event status in
 clGetEventProfilingInfo().

Some applications forgot to call clWaitForEvents() before calling to
clGetEventProfilingInfo(). Let's update the event's status here.

Signed-off-by: Zhigang Gong <zhigang.gong@intel.com>
---
 src/cl_api.c | 1 +
 1 file changed, 1 insertion(+)

--
1.9.1


This way we can avoid unecessary event updating in clFinish() call.
The overhead is only introduced when the application calling
clGetEventProfilingInfo() and we do not need busy wait for the event
updating here.

Thanks,
Zhigang Gong.

On Tue, Aug 04, 2015 at 01:11:10PM +0800, xionghu.luo@intel.com wrote:
> From: Luo Xionghu <xionghu.luo@intel.com>
> 
> this could fix the shoc project reported CL_PROFILING_INFO_NOT_AVAILABLE
> issue.
> https://github.com/vetter/shoc/issues/47
> v2: per spec, the GetEventProfilingInfo need return
> CL_PROFILING_INFO_NOT_AVAILABLE if the event is not CL_COMPLETE, so the
> event should be updated in clFinish.
> 
> Signed-off-by: Luo Xionghu <xionghu.luo@intel.com>
> ---
>  src/cl_command_queue.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/cl_command_queue.c b/src/cl_command_queue.c
> index 4e4ebfb..4b92311 100644
> --- a/src/cl_command_queue.c
> +++ b/src/cl_command_queue.c
> @@ -276,6 +276,9 @@ LOCAL cl_int
>  cl_command_queue_finish(cl_command_queue queue)
>  {
>    cl_gpgpu_sync(cl_get_thread_batch_buf(queue));
> +  cl_event last_event = get_last_event(queue);
> +  if (last_event)
> +    cl_event_update_status(last_event, 1);
>    return CL_SUCCESS;
>  }
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Beignet mailing list
> Beignet@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet

Patch hide | download patch | download mbox

diff --git a/src/cl_api.c b/src/cl_api.c
index 69eb0bc..5c9b250 100644
--- a/src/cl_api.c
+++ b/src/cl_api.c
@@ -1468,6 +1468,7 @@  clGetEventProfilingInfo(cl_event             event,
   cl_ulong ret_val;

   CHECK_EVENT(event);
+  cl_event_update_status(event, 0);

   if (event->type == CL_COMMAND_USER ||
       !(event->queue->props & CL_QUEUE_PROFILING_ENABLE) ||

Comments

I think these two patches both need when application GetEventProfilingInfo.

From application view, it is make sense that all event that don't depend on other events should have been flushed and finished when clFinish return.
The problem is the event have finished, but beignet don't update the event's status, so I think it is better to update the event status before exit clFinish.

And GetEventProfilingInfo is another point need to update the event' status. Suppose in the application, don't call clFinish, but maybe event has completed, should not return CL_PROFILING_INFO_NOT_AVAILABLE.

> -----Original Message-----

> From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf Of

> Zhigang Gong

> Sent: Tuesday, August 4, 2015 13:28

> To: Luo, Xionghu

> Cc: beignet@lists.freedesktop.org

> Subject: Re: [Beignet] [PATCH v2] GetEventProfilingInfo could query all event

> status.

> 

> 

> The spec only says if event is not in CL_COMPLETE status, it will get

> CL_PROFILING_INFO_NOT_AVAILABLE. It doesn't mean a clFinish() should

> update all events status. According to the spec, clFinish() only need to make

> sure the previously enqueued task has been processed and completed.

> 

> Form my point of view, if the application wants to call

> clGetEventProfilingInfo() to get an event's profiling information, the

> application should call

> clWaitForEvents() firstly. But given the fact that the specification is not very

> clear for whether clFinish() should update all events' status and some

> applications are really believe all events has been also updated after the

> clFinish() call. We may make a work around in Beignet to make those

> applications happy. Your patch is one solution and another may be better

> solution may be to add cl_event_update_status(event, 0) when calling to

> clGetEventProfilingInfo(). The patch is as below,

> 

> From a5a1b3f372d17f26cc20fba078490b61614f07e5 Mon Sep 17 00:00:00 2001

> From: Zhigang Gong <zhigang.gong@intel.com>

> Date: Tue, 4 Aug 2015 13:21:27 +0800

> Subject: [PATCH] runtime: always try to update event status in

> clGetEventProfilingInfo().

> 

> Some applications forgot to call clWaitForEvents() before calling to

> clGetEventProfilingInfo(). Let's update the event's status here.

> 

> Signed-off-by: Zhigang Gong <zhigang.gong@intel.com>

> ---

>  src/cl_api.c | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/src/cl_api.c b/src/cl_api.c index 69eb0bc..5c9b250 100644

> --- a/src/cl_api.c

> +++ b/src/cl_api.c

> @@ -1468,6 +1468,7 @@ clGetEventProfilingInfo(cl_event             event,

>    cl_ulong ret_val;

> 

>    CHECK_EVENT(event);

> +  cl_event_update_status(event, 0);

> 

>    if (event->type == CL_COMMAND_USER ||

>        !(event->queue->props & CL_QUEUE_PROFILING_ENABLE) ||

> --

> 1.9.1

> 

> 

> This way we can avoid unecessary event updating in clFinish() call.

> The overhead is only introduced when the application calling

> clGetEventProfilingInfo() and we do not need busy wait for the event

> updating here.

> 

> Thanks,

> Zhigang Gong.

> 

> On Tue, Aug 04, 2015 at 01:11:10PM +0800, xionghu.luo@intel.com wrote:

> > From: Luo Xionghu <xionghu.luo@intel.com>

> >

> > this could fix the shoc project reported

> > CL_PROFILING_INFO_NOT_AVAILABLE issue.

> > https://github.com/vetter/shoc/issues/47

> > v2: per spec, the GetEventProfilingInfo need return

> > CL_PROFILING_INFO_NOT_AVAILABLE if the event is not CL_COMPLETE,

> so

> > the event should be updated in clFinish.

> >

> > Signed-off-by: Luo Xionghu <xionghu.luo@intel.com>

> > ---

> >  src/cl_command_queue.c | 3 +++

> >  1 file changed, 3 insertions(+)

> >

> > diff --git a/src/cl_command_queue.c b/src/cl_command_queue.c index

> > 4e4ebfb..4b92311 100644

> > --- a/src/cl_command_queue.c

> > +++ b/src/cl_command_queue.c

> > @@ -276,6 +276,9 @@ LOCAL cl_int

> >  cl_command_queue_finish(cl_command_queue queue)  {

> >    cl_gpgpu_sync(cl_get_thread_batch_buf(queue));

> > +  cl_event last_event = get_last_event(queue);  if (last_event)

> > +    cl_event_update_status(last_event, 1);

> >    return CL_SUCCESS;

> >  }

> >

> > --

> > 1.9.1

> >

> > _______________________________________________

> > Beignet mailing list

> > Beignet@lists.freedesktop.org

> > http://lists.freedesktop.org/mailman/listinfo/beignet

> _______________________________________________

> Beignet mailing list

> Beignet@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/beignet
Beignet current uses an on-demand manner to maintain the events' status.
For those APIs which need to get current events' status, we insert a
cl_gpgpu_event_update_status(). It seems there is no need for clFinish().

If you could find any case that if we don't insert it in clFinish() then it will cause
functional error, I will agree that we need both patches. Even though xionghu's
patch need to refine the commit log message, as it is not to fix GetEventProfilingInfo()
issue.

Thanks,
Zhigang Gong.

> -----Original Message-----
> From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf Of
> Yang, Rong R
> Sent: Tuesday, August 4, 2015 3:31 PM
> To: Zhigang Gong; Luo, Xionghu
> Cc: beignet@lists.freedesktop.org
> Subject: Re: [Beignet] [PATCH v2] GetEventProfilingInfo could query all event
> status.
> 
> I think these two patches both need when application GetEventProfilingInfo.
> 
> From application view, it is make sense that all event that don't depend on
> other events should have been flushed and finished when clFinish return.
> The problem is the event have finished, but beignet don't update the event's
> status, so I think it is better to update the event status before exit clFinish.
> 
> And GetEventProfilingInfo is another point need to update the event' status.
> Suppose in the application, don't call clFinish, but maybe event has completed,
> should not return CL_PROFILING_INFO_NOT_AVAILABLE.
> 
> > -----Original Message-----
> > From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf
> > Of Zhigang Gong
> > Sent: Tuesday, August 4, 2015 13:28
> > To: Luo, Xionghu
> > Cc: beignet@lists.freedesktop.org
> > Subject: Re: [Beignet] [PATCH v2] GetEventProfilingInfo could query
> > all event status.
> >
> >
> > The spec only says if event is not in CL_COMPLETE status, it will get
> > CL_PROFILING_INFO_NOT_AVAILABLE. It doesn't mean a clFinish() should
> > update all events status. According to the spec, clFinish() only need
> > to make sure the previously enqueued task has been processed and
> completed.
> >
> > Form my point of view, if the application wants to call
> > clGetEventProfilingInfo() to get an event's profiling information, the
> > application should call
> > clWaitForEvents() firstly. But given the fact that the specification
> > is not very clear for whether clFinish() should update all events'
> > status and some applications are really believe all events has been
> > also updated after the
> > clFinish() call. We may make a work around in Beignet to make those
> > applications happy. Your patch is one solution and another may be
> > better solution may be to add cl_event_update_status(event, 0) when
> > calling to clGetEventProfilingInfo(). The patch is as below,
> >
> > From a5a1b3f372d17f26cc20fba078490b61614f07e5 Mon Sep 17 00:00:00
> 2001
> > From: Zhigang Gong <zhigang.gong@intel.com>
> > Date: Tue, 4 Aug 2015 13:21:27 +0800
> > Subject: [PATCH] runtime: always try to update event status in
> > clGetEventProfilingInfo().
> >
> > Some applications forgot to call clWaitForEvents() before calling to
> > clGetEventProfilingInfo(). Let's update the event's status here.
> >
> > Signed-off-by: Zhigang Gong <zhigang.gong@intel.com>
> > ---
> >  src/cl_api.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/cl_api.c b/src/cl_api.c index 69eb0bc..5c9b250 100644
> > --- a/src/cl_api.c
> > +++ b/src/cl_api.c
> > @@ -1468,6 +1468,7 @@ clGetEventProfilingInfo(cl_event
> event,
> >    cl_ulong ret_val;
> >
> >    CHECK_EVENT(event);
> > +  cl_event_update_status(event, 0);
> >
> >    if (event->type == CL_COMMAND_USER ||
> >        !(event->queue->props & CL_QUEUE_PROFILING_ENABLE) ||
> > --
> > 1.9.1
> >
> >
> > This way we can avoid unecessary event updating in clFinish() call.
> > The overhead is only introduced when the application calling
> > clGetEventProfilingInfo() and we do not need busy wait for the event
> > updating here.
> >
> > Thanks,
> > Zhigang Gong.
> >
> > On Tue, Aug 04, 2015 at 01:11:10PM +0800, xionghu.luo@intel.com wrote:
> > > From: Luo Xionghu <xionghu.luo@intel.com>
> > >
> > > this could fix the shoc project reported
> > > CL_PROFILING_INFO_NOT_AVAILABLE issue.
> > > https://github.com/vetter/shoc/issues/47
> > > v2: per spec, the GetEventProfilingInfo need return
> > > CL_PROFILING_INFO_NOT_AVAILABLE if the event is not CL_COMPLETE,
> > so
> > > the event should be updated in clFinish.
> > >
> > > Signed-off-by: Luo Xionghu <xionghu.luo@intel.com>
> > > ---
> > >  src/cl_command_queue.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/src/cl_command_queue.c b/src/cl_command_queue.c index
> > > 4e4ebfb..4b92311 100644
> > > --- a/src/cl_command_queue.c
> > > +++ b/src/cl_command_queue.c
> > > @@ -276,6 +276,9 @@ LOCAL cl_int
> > >  cl_command_queue_finish(cl_command_queue queue)  {
> > >    cl_gpgpu_sync(cl_get_thread_batch_buf(queue));
> > > +  cl_event last_event = get_last_event(queue);  if (last_event)
> > > +    cl_event_update_status(last_event, 1);
> > >    return CL_SUCCESS;
> > >  }
> > >
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Beignet mailing list
> > > Beignet@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/beignet
> > _______________________________________________
> > Beignet mailing list
> > Beignet@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/beignet
> _______________________________________________
> Beignet mailing list
> Beignet@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
The root cause of beignet using on-demand manner is no point to update event's status. If there have this point, I think there is no side effect to update the status.

I just find an case:

clEnqueueNDRange(....., event);
clSetEventCallback(event, ....);
clFinish(...);
return;

In event callback, release all cl resource, include event and command queue. Then beignet will never call the callback.
Of cause, this case is the current beignet event bug, if there is not clFinish, the callback should also be called in future, but beignet will not. 
Add the update status in clFinish just a workaround, do you the method to fix it?

> -----Original Message-----

> From: Zhigang Gong [mailto:zhigang.gong@linux.intel.com]

> Sent: Tuesday, August 4, 2015 16:48

> To: Yang, Rong R; Luo, Xionghu

> Cc: beignet@lists.freedesktop.org

> Subject: RE: [Beignet] [PATCH v2] GetEventProfilingInfo could query all event

> status.

> 

> Beignet current uses an on-demand manner to maintain the events' status.

> For those APIs which need to get current events' status, we insert a

> cl_gpgpu_event_update_status(). It seems there is no need for clFinish().

> 

> If you could find any case that if we don't insert it in clFinish() then it will

> cause functional error, I will agree that we need both patches. Even though

> xionghu's patch need to refine the commit log message, as it is not to fix

> GetEventProfilingInfo() issue.

> 

> Thanks,

> Zhigang Gong.

> 

> > -----Original Message-----

> > From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf

> > Of Yang, Rong R

> > Sent: Tuesday, August 4, 2015 3:31 PM

> > To: Zhigang Gong; Luo, Xionghu

> > Cc: beignet@lists.freedesktop.org

> > Subject: Re: [Beignet] [PATCH v2] GetEventProfilingInfo could query

> > all event status.

> >

> > I think these two patches both need when application

> GetEventProfilingInfo.

> >

> > From application view, it is make sense that all event that don't

> > depend on other events should have been flushed and finished when

> clFinish return.

> > The problem is the event have finished, but beignet don't update the

> > event's status, so I think it is better to update the event status before exit

> clFinish.

> >

> > And GetEventProfilingInfo is another point need to update the event'

> status.

> > Suppose in the application, don't call clFinish, but maybe event has

> > completed, should not return CL_PROFILING_INFO_NOT_AVAILABLE.

> >

> > > -----Original Message-----

> > > From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On

> > > Behalf Of Zhigang Gong

> > > Sent: Tuesday, August 4, 2015 13:28

> > > To: Luo, Xionghu

> > > Cc: beignet@lists.freedesktop.org

> > > Subject: Re: [Beignet] [PATCH v2] GetEventProfilingInfo could query

> > > all event status.

> > >

> > >

> > > The spec only says if event is not in CL_COMPLETE status, it will

> > > get CL_PROFILING_INFO_NOT_AVAILABLE. It doesn't mean a clFinish()

> > > should update all events status. According to the spec, clFinish()

> > > only need to make sure the previously enqueued task has been

> > > processed and

> > completed.

> > >

> > > Form my point of view, if the application wants to call

> > > clGetEventProfilingInfo() to get an event's profiling information,

> > > the application should call

> > > clWaitForEvents() firstly. But given the fact that the specification

> > > is not very clear for whether clFinish() should update all events'

> > > status and some applications are really believe all events has been

> > > also updated after the

> > > clFinish() call. We may make a work around in Beignet to make those

> > > applications happy. Your patch is one solution and another may be

> > > better solution may be to add cl_event_update_status(event, 0) when

> > > calling to clGetEventProfilingInfo(). The patch is as below,

> > >

> > > From a5a1b3f372d17f26cc20fba078490b61614f07e5 Mon Sep 17 00:00:00

> > 2001

> > > From: Zhigang Gong <zhigang.gong@intel.com>

> > > Date: Tue, 4 Aug 2015 13:21:27 +0800

> > > Subject: [PATCH] runtime: always try to update event status in

> > > clGetEventProfilingInfo().

> > >

> > > Some applications forgot to call clWaitForEvents() before calling to

> > > clGetEventProfilingInfo(). Let's update the event's status here.

> > >

> > > Signed-off-by: Zhigang Gong <zhigang.gong@intel.com>

> > > ---

> > >  src/cl_api.c | 1 +

> > >  1 file changed, 1 insertion(+)

> > >

> > > diff --git a/src/cl_api.c b/src/cl_api.c index 69eb0bc..5c9b250

> > > 100644

> > > --- a/src/cl_api.c

> > > +++ b/src/cl_api.c

> > > @@ -1468,6 +1468,7 @@ clGetEventProfilingInfo(cl_event

> > event,

> > >    cl_ulong ret_val;

> > >

> > >    CHECK_EVENT(event);

> > > +  cl_event_update_status(event, 0);

> > >

> > >    if (event->type == CL_COMMAND_USER ||

> > >        !(event->queue->props & CL_QUEUE_PROFILING_ENABLE) ||

> > > --

> > > 1.9.1

> > >

> > >

> > > This way we can avoid unecessary event updating in clFinish() call.

> > > The overhead is only introduced when the application calling

> > > clGetEventProfilingInfo() and we do not need busy wait for the event

> > > updating here.

> > >

> > > Thanks,

> > > Zhigang Gong.

> > >

> > > On Tue, Aug 04, 2015 at 01:11:10PM +0800, xionghu.luo@intel.com wrote:

> > > > From: Luo Xionghu <xionghu.luo@intel.com>

> > > >

> > > > this could fix the shoc project reported

> > > > CL_PROFILING_INFO_NOT_AVAILABLE issue.

> > > > https://github.com/vetter/shoc/issues/47

> > > > v2: per spec, the GetEventProfilingInfo need return

> > > > CL_PROFILING_INFO_NOT_AVAILABLE if the event is not

> CL_COMPLETE,

> > > so

> > > > the event should be updated in clFinish.

> > > >

> > > > Signed-off-by: Luo Xionghu <xionghu.luo@intel.com>

> > > > ---

> > > >  src/cl_command_queue.c | 3 +++

> > > >  1 file changed, 3 insertions(+)

> > > >

> > > > diff --git a/src/cl_command_queue.c b/src/cl_command_queue.c index

> > > > 4e4ebfb..4b92311 100644

> > > > --- a/src/cl_command_queue.c

> > > > +++ b/src/cl_command_queue.c

> > > > @@ -276,6 +276,9 @@ LOCAL cl_int

> > > >  cl_command_queue_finish(cl_command_queue queue)  {

> > > >    cl_gpgpu_sync(cl_get_thread_batch_buf(queue));

> > > > +  cl_event last_event = get_last_event(queue);  if (last_event)

> > > > +    cl_event_update_status(last_event, 1);

> > > >    return CL_SUCCESS;

> > > >  }

> > > >

> > > > --

> > > > 1.9.1

> > > >

> > > > _______________________________________________

> > > > Beignet mailing list

> > > > Beignet@lists.freedesktop.org

> > > > http://lists.freedesktop.org/mailman/listinfo/beignet

> > > _______________________________________________

> > > Beignet mailing list

> > > Beignet@lists.freedesktop.org

> > > http://lists.freedesktop.org/mailman/listinfo/beignet

> > _______________________________________________

> > Beignet mailing list

> > Beignet@lists.freedesktop.org

> > http://lists.freedesktop.org/mailman/listinfo/beignet
For this special case, the user application has an serious bug which is forgetting to call clReleaseEvent()
and will leak one event.

For beignet, the real problem is if an application rely on event driven mechanism, then it may never
get a chance to move forward, here is a simple example:

main()
{
  ...
  clEnqueueNDRange(queue,...,event);
  clSetEventCallback(event, .., callbackFn);
  ...
  pthread_create(pFn);//creat a child thread.
  pthread_wait() // wait for the last thread to terminate.
  clReleaseEvent(event);
  clFinish(queue);
}

callbackFn() {
  pthread_cond_signal(); // notify the child thread to do some thing.
  ...
}

pFn() {
  pthread_cond_wait()
  ...//do something on CPU;
  clEnqueueNDRange(queue, ..., event2);
  ...
}

The callbackFn will never be called and the child thread will always wait for the condition variable
even the actual event has been finished soon after the first enqueuer().

So add event status update in clFinish() will not really fix this type of problem. The real fix should be to
rework the whole event handling mechanism and add one daemon thread to maintain event status.
I really can't think of one method without an additional thread to solve this problem.


Thanks,
Zhigang Gong.

> -----Original Message-----
> From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf Of
> Yang, Rong R
> Sent: Wednesday, August 5, 2015 11:38 AM
> To: Zhigang Gong; Luo, Xionghu
> Cc: beignet@lists.freedesktop.org
> Subject: Re: [Beignet] [PATCH v2] GetEventProfilingInfo could query all event
> status.
> 
> The root cause of beignet using on-demand manner is no point to update
> event's status. If there have this point, I think there is no side effect to update
> the status.
> 
> I just find an case:
> 
> clEnqueueNDRange(....., event);
> clSetEventCallback(event, ....);
> clFinish(...);
> return;
> 
> In event callback, release all cl resource, include event and command queue.
> Then beignet will never call the callback.
> Of cause, this case is the current beignet event bug, if there is not clFinish, the
> callback should also be called in future, but beignet will not.
> Add the update status in clFinish just a workaround, do you the method to fix
> it?
> 
> > -----Original Message-----
> > From: Zhigang Gong [mailto:zhigang.gong@linux.intel.com]
> > Sent: Tuesday, August 4, 2015 16:48
> > To: Yang, Rong R; Luo, Xionghu
> > Cc: beignet@lists.freedesktop.org
> > Subject: RE: [Beignet] [PATCH v2] GetEventProfilingInfo could query
> > all event status.
> >
> > Beignet current uses an on-demand manner to maintain the events' status.
> > For those APIs which need to get current events' status, we insert a
> > cl_gpgpu_event_update_status(). It seems there is no need for clFinish().
> >
> > If you could find any case that if we don't insert it in clFinish()
> > then it will cause functional error, I will agree that we need both
> > patches. Even though xionghu's patch need to refine the commit log
> > message, as it is not to fix
> > GetEventProfilingInfo() issue.
> >
> > Thanks,
> > Zhigang Gong.
> >
> > > -----Original Message-----
> > > From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On
> > > Behalf Of Yang, Rong R
> > > Sent: Tuesday, August 4, 2015 3:31 PM
> > > To: Zhigang Gong; Luo, Xionghu
> > > Cc: beignet@lists.freedesktop.org
> > > Subject: Re: [Beignet] [PATCH v2] GetEventProfilingInfo could query
> > > all event status.
> > >
> > > I think these two patches both need when application
> > GetEventProfilingInfo.
> > >
> > > From application view, it is make sense that all event that don't
> > > depend on other events should have been flushed and finished when
> > clFinish return.
> > > The problem is the event have finished, but beignet don't update the
> > > event's status, so I think it is better to update the event status
> > > before exit
> > clFinish.
> > >
> > > And GetEventProfilingInfo is another point need to update the event'
> > status.
> > > Suppose in the application, don't call clFinish, but maybe event has
> > > completed, should not return CL_PROFILING_INFO_NOT_AVAILABLE.
> > >
> > > > -----Original Message-----
> > > > From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On
> > > > Behalf Of Zhigang Gong
> > > > Sent: Tuesday, August 4, 2015 13:28
> > > > To: Luo, Xionghu
> > > > Cc: beignet@lists.freedesktop.org
> > > > Subject: Re: [Beignet] [PATCH v2] GetEventProfilingInfo could
> > > > query all event status.
> > > >
> > > >
> > > > The spec only says if event is not in CL_COMPLETE status, it will
> > > > get CL_PROFILING_INFO_NOT_AVAILABLE. It doesn't mean a clFinish()
> > > > should update all events status. According to the spec, clFinish()
> > > > only need to make sure the previously enqueued task has been
> > > > processed and
> > > completed.
> > > >
> > > > Form my point of view, if the application wants to call
> > > > clGetEventProfilingInfo() to get an event's profiling information,
> > > > the application should call
> > > > clWaitForEvents() firstly. But given the fact that the
> > > > specification is not very clear for whether clFinish() should update all
> events'
> > > > status and some applications are really believe all events has
> > > > been also updated after the
> > > > clFinish() call. We may make a work around in Beignet to make
> > > > those applications happy. Your patch is one solution and another
> > > > may be better solution may be to add cl_event_update_status(event,
> > > > 0) when calling to clGetEventProfilingInfo(). The patch is as
> > > > below,
> > > >
> > > > From a5a1b3f372d17f26cc20fba078490b61614f07e5 Mon Sep 17
> 00:00:00
> > > 2001
> > > > From: Zhigang Gong <zhigang.gong@intel.com>
> > > > Date: Tue, 4 Aug 2015 13:21:27 +0800
> > > > Subject: [PATCH] runtime: always try to update event status in
> > > > clGetEventProfilingInfo().
> > > >
> > > > Some applications forgot to call clWaitForEvents() before calling
> > > > to clGetEventProfilingInfo(). Let's update the event's status here.
> > > >
> > > > Signed-off-by: Zhigang Gong <zhigang.gong@intel.com>
> > > > ---
> > > >  src/cl_api.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/src/cl_api.c b/src/cl_api.c index 69eb0bc..5c9b250
> > > > 100644
> > > > --- a/src/cl_api.c
> > > > +++ b/src/cl_api.c
> > > > @@ -1468,6 +1468,7 @@ clGetEventProfilingInfo(cl_event
> > > event,
> > > >    cl_ulong ret_val;
> > > >
> > > >    CHECK_EVENT(event);
> > > > +  cl_event_update_status(event, 0);
> > > >
> > > >    if (event->type == CL_COMMAND_USER ||
> > > >        !(event->queue->props & CL_QUEUE_PROFILING_ENABLE) ||
> > > > --
> > > > 1.9.1
> > > >
> > > >
> > > > This way we can avoid unecessary event updating in clFinish() call.
> > > > The overhead is only introduced when the application calling
> > > > clGetEventProfilingInfo() and we do not need busy wait for the
> > > > event updating here.
> > > >
> > > > Thanks,
> > > > Zhigang Gong.
> > > >
> > > > On Tue, Aug 04, 2015 at 01:11:10PM +0800, xionghu.luo@intel.com
> wrote:
> > > > > From: Luo Xionghu <xionghu.luo@intel.com>
> > > > >
> > > > > this could fix the shoc project reported
> > > > > CL_PROFILING_INFO_NOT_AVAILABLE issue.
> > > > > https://github.com/vetter/shoc/issues/47
> > > > > v2: per spec, the GetEventProfilingInfo need return
> > > > > CL_PROFILING_INFO_NOT_AVAILABLE if the event is not
> > CL_COMPLETE,
> > > > so
> > > > > the event should be updated in clFinish.
> > > > >
> > > > > Signed-off-by: Luo Xionghu <xionghu.luo@intel.com>
> > > > > ---
> > > > >  src/cl_command_queue.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/src/cl_command_queue.c b/src/cl_command_queue.c
> > > > > index
> > > > > 4e4ebfb..4b92311 100644
> > > > > --- a/src/cl_command_queue.c
> > > > > +++ b/src/cl_command_queue.c
> > > > > @@ -276,6 +276,9 @@ LOCAL cl_int
> > > > > cl_command_queue_finish(cl_command_queue queue)  {
> > > > >    cl_gpgpu_sync(cl_get_thread_batch_buf(queue));
> > > > > +  cl_event last_event = get_last_event(queue);  if (last_event)
> > > > > +    cl_event_update_status(last_event, 1);
> > > > >    return CL_SUCCESS;
> > > > >  }
> > > > >
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > _______________________________________________
> > > > > Beignet mailing list
> > > > > Beignet@lists.freedesktop.org
> > > > > http://lists.freedesktop.org/mailman/listinfo/beignet
> > > > _______________________________________________
> > > > Beignet mailing list
> > > > Beignet@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/beignet
> > > _______________________________________________
> > > Beignet mailing list
> > > Beignet@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/beignet
> 
> _______________________________________________
> Beignet mailing list
> Beignet@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
> -----Original Message-----

> From: Zhigang Gong [mailto:zhigang.gong@linux.intel.com]

> Sent: Wednesday, August 5, 2015 12:49

> To: Yang, Rong R; Luo, Xionghu

> Cc: beignet@lists.freedesktop.org

> Subject: RE: [Beignet] [PATCH v2] GetEventProfilingInfo could query all event

> status.

> 

> For this special case, the user application has an serious bug which is

> forgetting to call clReleaseEvent() and will leak one event.

 
Suppose the application call clReleaseEvent and clReleaseCommandQueue in event's callback, and it is the last enqueue event, so beignet has no chance to call the callback.

> 

> For beignet, the real problem is if an application rely on event driven

> mechanism, then it may never get a chance to move forward, here is a

> simple example:

> 

> main()

> {

>   ...

>   clEnqueueNDRange(queue,...,event);

>   clSetEventCallback(event, .., callbackFn);

>   ...

>   pthread_create(pFn);//creat a child thread.

>   pthread_wait() // wait for the last thread to terminate.

>   clReleaseEvent(event);

>   clFinish(queue);

> }

> 

> callbackFn() {

>   pthread_cond_signal(); // notify the child thread to do some thing.

>   ...

> }

> 

> pFn() {

>   pthread_cond_wait()

>   ...//do something on CPU;

>   clEnqueueNDRange(queue, ..., event2);

>   ...

> }

> 

> The callbackFn will never be called and the child thread will always wait for

> the condition variable even the actual event has been finished soon after the

> first enqueuer().

> 

> So add event status update in clFinish() will not really fix this type of problem.

> The real fix should be to rework the whole event handling mechanism and

> add one daemon thread to maintain event status.

> I really can't think of one method without an additional thread to solve this

> problem.

[Yang, Rong R] Argee with it.
> 

> 

> Thanks,

> Zhigang Gong.

>
> -----Original Message-----
> From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf Of
> Yang, Rong R
> Sent: Wednesday, August 5, 2015 1:05 PM
> To: Zhigang Gong; Luo, Xionghu
> Cc: beignet@lists.freedesktop.org
> Subject: Re: [Beignet] [PATCH v2] GetEventProfilingInfo could query all event
> status.
> 
> 
> 
> > -----Original Message-----
> > From: Zhigang Gong [mailto:zhigang.gong@linux.intel.com]
> > Sent: Wednesday, August 5, 2015 12:49
> > To: Yang, Rong R; Luo, Xionghu
> > Cc: beignet@lists.freedesktop.org
> > Subject: RE: [Beignet] [PATCH v2] GetEventProfilingInfo could query
> > all event status.
> >
> > For this special case, the user application has an serious bug which
> > is forgetting to call clReleaseEvent() and will leak one event.
> 
> Suppose the application call clReleaseEvent and clReleaseCommandQueue in
> event's callback, and it is the last enqueue event, so beignet has no chance to
> call the callback.

Right. Put the clReleaseEvent() in a callback function is a normal case.
I'm ok you push both of the patches. But please be aware, these fixes
are only work around for some cases. If the user doesn't call clFinish() or
will call clFinish after the callback function be called, application will
be stalled.

Thanks,
Zhigang Gong.

> 
> >
> > For beignet, the real problem is if an application rely on event
> > driven mechanism, then it may never get a chance to move forward, here
> > is a simple example:
> >
> > main()
> > {
> >   ...
> >   clEnqueueNDRange(queue,...,event);
> >   clSetEventCallback(event, .., callbackFn);
> >   ...
> >   pthread_create(pFn);//creat a child thread.
> >   pthread_wait() // wait for the last thread to terminate.
> >   clReleaseEvent(event);
> >   clFinish(queue);
> > }
> >
> > callbackFn() {
> >   pthread_cond_signal(); // notify the child thread to do some thing.
> >   ...
> > }
> >
> > pFn() {
> >   pthread_cond_wait()
> >   ...//do something on CPU;
> >   clEnqueueNDRange(queue, ..., event2);
> >   ...
> > }
> >
> > The callbackFn will never be called and the child thread will always
> > wait for the condition variable even the actual event has been
> > finished soon after the first enqueuer().
> >
> > So add event status update in clFinish() will not really fix this type of problem.
> > The real fix should be to rework the whole event handling mechanism
> > and add one daemon thread to maintain event status.
> > I really can't think of one method without an additional thread to
> > solve this problem.
> [Yang, Rong R] Argee with it.
> >
> >
> > Thanks,
> > Zhigang Gong.
> >
> _______________________________________________
> Beignet mailing list
> Beignet@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
Both patches has been pushed, and correct xionghu's commit log.

> -----Original Message-----

> From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf Of

> Zhigang Gong

> Sent: Tuesday, August 4, 2015 13:28

> To: Luo, Xionghu

> Cc: beignet@lists.freedesktop.org

> Subject: Re: [Beignet] [PATCH v2] GetEventProfilingInfo could query all event

> status.

> 

> 

> The spec only says if event is not in CL_COMPLETE status, it will get

> CL_PROFILING_INFO_NOT_AVAILABLE. It doesn't mean a clFinish() should

> update all events status. According to the spec, clFinish() only need to make

> sure the previously enqueued task has been processed and completed.

> 

> Form my point of view, if the application wants to call

> clGetEventProfilingInfo() to get an event's profiling information, the

> application should call

> clWaitForEvents() firstly. But given the fact that the specification is not very

> clear for whether clFinish() should update all events' status and some

> applications are really believe all events has been also updated after the

> clFinish() call. We may make a work around in Beignet to make those

> applications happy. Your patch is one solution and another may be better

> solution may be to add cl_event_update_status(event, 0) when calling to

> clGetEventProfilingInfo(). The patch is as below,

> 

> From a5a1b3f372d17f26cc20fba078490b61614f07e5 Mon Sep 17 00:00:00 2001

> From: Zhigang Gong <zhigang.gong@intel.com>

> Date: Tue, 4 Aug 2015 13:21:27 +0800

> Subject: [PATCH] runtime: always try to update event status in

> clGetEventProfilingInfo().

> 

> Some applications forgot to call clWaitForEvents() before calling to

> clGetEventProfilingInfo(). Let's update the event's status here.

> 

> Signed-off-by: Zhigang Gong <zhigang.gong@intel.com>

> ---

>  src/cl_api.c | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/src/cl_api.c b/src/cl_api.c index 69eb0bc..5c9b250 100644

> --- a/src/cl_api.c

> +++ b/src/cl_api.c

> @@ -1468,6 +1468,7 @@ clGetEventProfilingInfo(cl_event             event,

>    cl_ulong ret_val;

> 

>    CHECK_EVENT(event);

> +  cl_event_update_status(event, 0);

> 

>    if (event->type == CL_COMMAND_USER ||

>        !(event->queue->props & CL_QUEUE_PROFILING_ENABLE) ||

> --

> 1.9.1

> 

> 

> This way we can avoid unecessary event updating in clFinish() call.

> The overhead is only introduced when the application calling

> clGetEventProfilingInfo() and we do not need busy wait for the event

> updating here.

> 

> Thanks,

> Zhigang Gong.

> 

> On Tue, Aug 04, 2015 at 01:11:10PM +0800, xionghu.luo@intel.com wrote:

> > From: Luo Xionghu <xionghu.luo@intel.com>

> >

> > this could fix the shoc project reported

> > CL_PROFILING_INFO_NOT_AVAILABLE issue.

> > https://github.com/vetter/shoc/issues/47

> > v2: per spec, the GetEventProfilingInfo need return

> > CL_PROFILING_INFO_NOT_AVAILABLE if the event is not CL_COMPLETE,

> so

> > the event should be updated in clFinish.

> >

> > Signed-off-by: Luo Xionghu <xionghu.luo@intel.com>

> > ---

> >  src/cl_command_queue.c | 3 +++

> >  1 file changed, 3 insertions(+)

> >

> > diff --git a/src/cl_command_queue.c b/src/cl_command_queue.c index

> > 4e4ebfb..4b92311 100644

> > --- a/src/cl_command_queue.c

> > +++ b/src/cl_command_queue.c

> > @@ -276,6 +276,9 @@ LOCAL cl_int

> >  cl_command_queue_finish(cl_command_queue queue)  {

> >    cl_gpgpu_sync(cl_get_thread_batch_buf(queue));

> > +  cl_event last_event = get_last_event(queue);  if (last_event)

> > +    cl_event_update_status(last_event, 1);

> >    return CL_SUCCESS;

> >  }

> >

> > --

> > 1.9.1

> >

> > _______________________________________________

> > Beignet mailing list

> > Beignet@lists.freedesktop.org

> > http://lists.freedesktop.org/mailman/listinfo/beignet

> _______________________________________________

> Beignet mailing list

> Beignet@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/beignet