[01/11] Runtime: Add CL base object for all cl objects.

Submitted by junyan.he@inbox.com on July 19, 2016, 11:25 a.m.

Details

Message ID 1468927557-5051-1-git-send-email-junyan.he@inbox.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Beignet

Browsing this patch as part of:
"Series without cover letter" rev 1 in Beignet
<< prev patch [1/11] next patch >>

Commit Message

junyan.he@inbox.com July 19, 2016, 11:25 a.m.
From: Junyan He <junyan.he@intel.com>

The runtime code is a little verbose in CL object handle.
Every CL objects should have a reference, a lock to protect itself
and an ICD dispatcher. We can organize them to a struct and place
it at the beginning of each CL object.
This base object is also used to protect the CL objects MT safe.
CL_OBJECT_LOCK/CL_OBJECT_UNLOCK macro will lock/unlock objects,
but we should use them within one function call, and the critical
region should be short.
We add CL_OBJECT_TAKE_OWNERSHIP/CL_OBJECT_RELEASE_OWNERSHIP macro
to own the object for a long time. CL_OBJECT_TAKE_OWNERSHIP will
not hold the lock and so will not cause deadlock problems.
For example, when we call NDRange on some memobj, we should take
the ownship of the memobj. If another thread call NDRange on the
same memobj, we should return some error like CL_OUT_OF_RESOURCE
to users and protect the memobj from accessing simultaneously.

Signed-off-by: Junyan He <junyan.he@intel.com>
---
 src/CMakeLists.txt   |    1 +
 src/cl_base_object.c |  102 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/cl_base_object.h |   77 +++++++++++++++++++++++++++++++++++++
 3 files changed, 180 insertions(+)
 create mode 100644 src/cl_base_object.c
 create mode 100644 src/cl_base_object.h

Patch hide | download patch | download mbox

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index a002865..cec7cfc 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -65,6 +65,7 @@  MakeKernelBinStr ("${CMAKE_CURRENT_SOURCE_DIR}/kernels/" "${BUILT_IN_NAME}")
 
 set(OPENCL_SRC
     ${KERNEL_STR_FILES}
+    cl_base_object.c
     cl_api.c
     cl_alloc.c
     cl_kernel.c
diff --git a/src/cl_base_object.c b/src/cl_base_object.c
new file mode 100644
index 0000000..4661977
--- /dev/null
+++ b/src/cl_base_object.c
@@ -0,0 +1,102 @@ 
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+#include <stdio.h>
+#include "cl_base_object.h"
+
+static pthread_t invalid_thread_id = -1;
+
+LOCAL void
+cl_object_init_base(cl_base_object obj, cl_ulong magic)
+{
+  obj->magic = magic;
+  obj->ref = 1;
+  SET_ICD(obj->dispatch);
+  pthread_mutex_init(&obj->mutex, NULL);
+  pthread_cond_init(&obj->cond, NULL);
+  obj->owner = invalid_thread_id;
+}
+
+LOCAL void
+cl_object_destroy_base(cl_base_object obj)
+{
+  int ref = CL_OBJECT_GET_REF(obj);
+  if (ref != 0) {
+    DEBUGP(DL_ERROR, "CL object %p, call destroy with a reference %d", obj,
+           ref);
+    assert(0);
+  }
+
+  if (!CL_OBJECT_IS_VALID(obj)) {
+    DEBUGP(DL_ERROR,
+           "CL object %p, call destroy while it is already a dead object", obj);
+    assert(0);
+  }
+
+  if (obj->owner != invalid_thread_id) {
+    DEBUGP(DL_ERROR, "CL object %p, call destroy while still has a owener %d",
+           obj, (int)obj->owner);
+    assert(0);
+  }
+
+  obj->magic = CL_OBJECT_INVALID_MAGIC;
+  pthread_mutex_destroy(&obj->mutex);
+  pthread_cond_destroy(&obj->cond);
+}
+
+LOCAL cl_int
+cl_object_take_ownership(cl_base_object obj, cl_int wait)
+{
+  pthread_t self;
+
+  assert(CL_OBJECT_IS_VALID(obj));
+
+  self = pthread_self();
+
+  pthread_mutex_lock(&obj->mutex);
+  if (pthread_equal(obj->owner, invalid_thread_id)) {
+    obj->owner = self;
+    pthread_mutex_unlock(&obj->mutex);
+    return 1;
+  }
+
+  if (wait == 0) {
+    pthread_mutex_unlock(&obj->mutex);
+    return 0;
+  }
+
+  while (!pthread_equal(obj->owner, invalid_thread_id)) {
+    pthread_cond_wait(&obj->cond, &obj->mutex);
+  }
+
+  obj->owner = self;
+  pthread_mutex_unlock(&obj->mutex);
+  return 1;
+}
+
+LOCAL void
+cl_object_release_ownership(cl_base_object obj)
+{
+  assert(CL_OBJECT_IS_VALID(obj));
+
+  pthread_mutex_lock(&obj->mutex);
+  assert(pthread_equal(pthread_self(), obj->owner));
+  obj->owner = invalid_thread_id;
+  pthread_cond_broadcast(&obj->cond);
+
+  pthread_mutex_unlock(&obj->mutex);
+}
diff --git a/src/cl_base_object.h b/src/cl_base_object.h
new file mode 100644
index 0000000..cba4330
--- /dev/null
+++ b/src/cl_base_object.h
@@ -0,0 +1,77 @@ 
+/*
+ * Copyright © 2012 Intel Corporation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef __CL_BASE_OBJECT_H__
+#define __CL_BASE_OBJECT_H__
+
+#include "cl_utils.h"
+#include "cl_khr_icd.h"
+#include "CL/cl.h"
+#include <pthread.h>
+#include <assert.h>
+
+/************************************************************************
+  Every CL objects should have:
+    ICD dispatcher: Hold the ICD function table pointer.
+
+    Reference: To maintain its' life time. CL retain/release API will
+    change its value. We will destroy the object when the count reach 0.
+
+    Magic: Just a number to represent each CL object. We will use it
+    to check whether it is the object we want.
+
+    Mutex & Cond: Used to protect the CL objects MT safe. lock/unlock
+    critical region should be short enough and should not have any block
+    function call. take_ownership/release_ownership  can own the object
+    for a long time. take_ownership will not hold the lock and so will
+    not cause deadlock problems. we can wait on the cond to get the
+    ownership.
+*************************************************************************/
+
+typedef struct _cl_base_object {
+  DEFINE_ICD(dispatch);         /* Dispatch function table for icd */
+  cl_ulong magic;               /* Magic number for each CL object */
+  atomic_t ref;                 /* Reference for each CL object */
+  pthread_mutex_t mutex;        /* THe mutex to protect this object MT safe */
+  pthread_cond_t cond;          /* Condition to wait for getting the object */
+  pthread_t owner;              /* The thread which own this object */
+} _cl_base_object;
+
+typedef struct _cl_base_object *cl_base_object;
+
+#define CL_OBJECT_INVALID_MAGIC 0xFEFEFEFEFEFEFEFELL
+#define CL_OBJECT_IS_VALID(obj) (((cl_base_object)obj)->magic != CL_OBJECT_INVALID_MAGIC)
+
+#define CL_OBJECT_INC_REF(obj) (atomic_inc(&((cl_base_object)obj)->ref))
+#define CL_OBJECT_DEC_REF(obj) (atomic_dec(&((cl_base_object)obj)->ref))
+#define CL_OBJECT_GET_REF(obj) (atomic_add(&((cl_base_object)obj)->ref, 0))
+
+#define CL_OBJECT_LOCK(obj) (pthread_mutex_lock(&((cl_base_object)obj)->mutex))
+#define CL_OBJECT_UNLOCK(obj) (pthread_mutex_unlock(&((cl_base_object)obj)->mutex))
+
+extern void cl_object_init_base(cl_base_object obj, cl_ulong magic);
+extern void cl_object_destroy_base(cl_base_object obj);
+extern cl_int cl_object_take_ownership(cl_base_object obj, cl_int wait);
+extern void cl_object_release_ownership(cl_base_object obj);
+
+#define CL_OBJECT_INIT_BASE(obj, magic) (cl_object_init_base((cl_base_object)obj, magic))
+#define CL_OBJECT_DESTROY_BASE(obj) (cl_object_destroy_base((cl_base_object)obj))
+#define CL_OBJECT_TAKE_OWNERSHIP(obj, wait) (cl_object_take_ownership((cl_base_object)obj, wait))
+#define CL_OBJECT_RELEASE_OWNERSHIP(obj) (cl_object_release_ownership((cl_base_object)obj))
+
+#endif /* __CL_BASE_OBJECT_H__ */

Comments

The whole patch set should be push to dev/runtime first.
When it is stable, we can merge them back.

On Tue, Jul 19, 2016 at 07:25:47PM +0800, junyan.he@inbox.com wrote:
> Date: Tue, 19 Jul 2016 19:25:47 +0800
> From: junyan.he@inbox.com
> To: beignet@lists.freedesktop.org
> Subject: [Beignet] [PATCH 01/11] Runtime: Add CL base object for all cl
>  objects.
> X-Mailer: git-send-email 1.7.9.5
> 
> From: Junyan He <junyan.he@intel.com>
> 
> The runtime code is a little verbose in CL object handle.
> Every CL objects should have a reference, a lock to protect itself
> and an ICD dispatcher. We can organize them to a struct and place
> it at the beginning of each CL object.
> This base object is also used to protect the CL objects MT safe.
> CL_OBJECT_LOCK/CL_OBJECT_UNLOCK macro will lock/unlock objects,
> but we should use them within one function call, and the critical
> region should be short.
> We add CL_OBJECT_TAKE_OWNERSHIP/CL_OBJECT_RELEASE_OWNERSHIP macro
> to own the object for a long time. CL_OBJECT_TAKE_OWNERSHIP will
> not hold the lock and so will not cause deadlock problems.
> For example, when we call NDRange on some memobj, we should take
> the ownship of the memobj. If another thread call NDRange on the
> same memobj, we should return some error like CL_OUT_OF_RESOURCE
> to users and protect the memobj from accessing simultaneously.
> 
> Signed-off-by: Junyan He <junyan.he@intel.com>
> ---
>  src/CMakeLists.txt   |    1 +
>  src/cl_base_object.c |  102 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/cl_base_object.h |   77 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 180 insertions(+)
>  create mode 100644 src/cl_base_object.c
>  create mode 100644 src/cl_base_object.h
> 
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index a002865..cec7cfc 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -65,6 +65,7 @@ MakeKernelBinStr ("${CMAKE_CURRENT_SOURCE_DIR}/kernels/" "${BUILT_IN_NAME}")
>  
>  set(OPENCL_SRC
>      ${KERNEL_STR_FILES}
> +    cl_base_object.c
>      cl_api.c
>      cl_alloc.c
>      cl_kernel.c
> diff --git a/src/cl_base_object.c b/src/cl_base_object.c
> new file mode 100644
> index 0000000..4661977
> --- /dev/null
> +++ b/src/cl_base_object.c
> @@ -0,0 +1,102 @@
> +/*
> + * Copyright © 2012 Intel Corporation
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +#include <stdio.h>
> +#include "cl_base_object.h"
> +
> +static pthread_t invalid_thread_id = -1;
> +
> +LOCAL void
> +cl_object_init_base(cl_base_object obj, cl_ulong magic)
> +{
> +  obj->magic = magic;
> +  obj->ref = 1;
> +  SET_ICD(obj->dispatch);
> +  pthread_mutex_init(&obj->mutex, NULL);
> +  pthread_cond_init(&obj->cond, NULL);
> +  obj->owner = invalid_thread_id;
> +}
> +
> +LOCAL void
> +cl_object_destroy_base(cl_base_object obj)
> +{
> +  int ref = CL_OBJECT_GET_REF(obj);
> +  if (ref != 0) {
> +    DEBUGP(DL_ERROR, "CL object %p, call destroy with a reference %d", obj,
> +           ref);
> +    assert(0);
> +  }
> +
> +  if (!CL_OBJECT_IS_VALID(obj)) {
> +    DEBUGP(DL_ERROR,
> +           "CL object %p, call destroy while it is already a dead object", obj);
> +    assert(0);
> +  }
> +
> +  if (obj->owner != invalid_thread_id) {
> +    DEBUGP(DL_ERROR, "CL object %p, call destroy while still has a owener %d",
> +           obj, (int)obj->owner);
> +    assert(0);
> +  }
> +
> +  obj->magic = CL_OBJECT_INVALID_MAGIC;
> +  pthread_mutex_destroy(&obj->mutex);
> +  pthread_cond_destroy(&obj->cond);
> +}
> +
> +LOCAL cl_int
> +cl_object_take_ownership(cl_base_object obj, cl_int wait)
> +{
> +  pthread_t self;
> +
> +  assert(CL_OBJECT_IS_VALID(obj));
> +
> +  self = pthread_self();
> +
> +  pthread_mutex_lock(&obj->mutex);
> +  if (pthread_equal(obj->owner, invalid_thread_id)) {
> +    obj->owner = self;
> +    pthread_mutex_unlock(&obj->mutex);
> +    return 1;
> +  }
> +
> +  if (wait == 0) {
> +    pthread_mutex_unlock(&obj->mutex);
> +    return 0;
> +  }
> +
> +  while (!pthread_equal(obj->owner, invalid_thread_id)) {
> +    pthread_cond_wait(&obj->cond, &obj->mutex);
> +  }
> +
> +  obj->owner = self;
> +  pthread_mutex_unlock(&obj->mutex);
> +  return 1;
> +}
> +
> +LOCAL void
> +cl_object_release_ownership(cl_base_object obj)
> +{
> +  assert(CL_OBJECT_IS_VALID(obj));
> +
> +  pthread_mutex_lock(&obj->mutex);
> +  assert(pthread_equal(pthread_self(), obj->owner));
> +  obj->owner = invalid_thread_id;
> +  pthread_cond_broadcast(&obj->cond);
> +
> +  pthread_mutex_unlock(&obj->mutex);
> +}
> diff --git a/src/cl_base_object.h b/src/cl_base_object.h
> new file mode 100644
> index 0000000..cba4330
> --- /dev/null
> +++ b/src/cl_base_object.h
> @@ -0,0 +1,77 @@
> +/*
> + * Copyright © 2012 Intel Corporation
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef __CL_BASE_OBJECT_H__
> +#define __CL_BASE_OBJECT_H__
> +
> +#include "cl_utils.h"
> +#include "cl_khr_icd.h"
> +#include "CL/cl.h"
> +#include <pthread.h>
> +#include <assert.h>
> +
> +/************************************************************************
> +  Every CL objects should have:
> +    ICD dispatcher: Hold the ICD function table pointer.
> +
> +    Reference: To maintain its' life time. CL retain/release API will
> +    change its value. We will destroy the object when the count reach 0.
> +
> +    Magic: Just a number to represent each CL object. We will use it
> +    to check whether it is the object we want.
> +
> +    Mutex & Cond: Used to protect the CL objects MT safe. lock/unlock
> +    critical region should be short enough and should not have any block
> +    function call. take_ownership/release_ownership  can own the object
> +    for a long time. take_ownership will not hold the lock and so will
> +    not cause deadlock problems. we can wait on the cond to get the
> +    ownership.
> +*************************************************************************/
> +
> +typedef struct _cl_base_object {
> +  DEFINE_ICD(dispatch);         /* Dispatch function table for icd */
> +  cl_ulong magic;               /* Magic number for each CL object */
> +  atomic_t ref;                 /* Reference for each CL object */
> +  pthread_mutex_t mutex;        /* THe mutex to protect this object MT safe */
> +  pthread_cond_t cond;          /* Condition to wait for getting the object */
> +  pthread_t owner;              /* The thread which own this object */
> +} _cl_base_object;
> +
> +typedef struct _cl_base_object *cl_base_object;
> +
> +#define CL_OBJECT_INVALID_MAGIC 0xFEFEFEFEFEFEFEFELL
> +#define CL_OBJECT_IS_VALID(obj) (((cl_base_object)obj)->magic != CL_OBJECT_INVALID_MAGIC)
> +
> +#define CL_OBJECT_INC_REF(obj) (atomic_inc(&((cl_base_object)obj)->ref))
> +#define CL_OBJECT_DEC_REF(obj) (atomic_dec(&((cl_base_object)obj)->ref))
> +#define CL_OBJECT_GET_REF(obj) (atomic_add(&((cl_base_object)obj)->ref, 0))
> +
> +#define CL_OBJECT_LOCK(obj) (pthread_mutex_lock(&((cl_base_object)obj)->mutex))
> +#define CL_OBJECT_UNLOCK(obj) (pthread_mutex_unlock(&((cl_base_object)obj)->mutex))
> +
> +extern void cl_object_init_base(cl_base_object obj, cl_ulong magic);
> +extern void cl_object_destroy_base(cl_base_object obj);
> +extern cl_int cl_object_take_ownership(cl_base_object obj, cl_int wait);
> +extern void cl_object_release_ownership(cl_base_object obj);
> +
> +#define CL_OBJECT_INIT_BASE(obj, magic) (cl_object_init_base((cl_base_object)obj, magic))
> +#define CL_OBJECT_DESTROY_BASE(obj) (cl_object_destroy_base((cl_base_object)obj))
> +#define CL_OBJECT_TAKE_OWNERSHIP(obj, wait) (cl_object_take_ownership((cl_base_object)obj, wait))
> +#define CL_OBJECT_RELEASE_OWNERSHIP(obj) (cl_object_release_ownership((cl_base_object)obj))
> +
> +#endif /* __CL_BASE_OBJECT_H__ */
> -- 
> 1.7.9.5
> 
> ____________________________________________________________
> FREE 3D EARTH SCREENSAVER - Watch the Earth right on your desktop!
> Check it out at http://www.inbox.com/earth
> 
> 
> _______________________________________________
> Beignet mailing list
> Beignet@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/beignet
One comment.

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

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

> junyan.he@inbox.com

> Sent: Tuesday, July 19, 2016 19:26

> To: beignet@lists.freedesktop.org

> Subject: [Beignet] [PATCH 01/11] Runtime: Add CL base object for all cl

> objects.

> 

> From: Junyan He <junyan.he@intel.com>

> 

> The runtime code is a little verbose in CL object handle.

> Every CL objects should have a reference, a lock to protect itself and an ICD

> dispatcher. We can organize them to a struct and place it at the beginning of

> each CL object.

> This base object is also used to protect the CL objects MT safe.

> CL_OBJECT_LOCK/CL_OBJECT_UNLOCK macro will lock/unlock objects, but

> we should use them within one function call, and the critical region should be

> short.

> We add CL_OBJECT_TAKE_OWNERSHIP/CL_OBJECT_RELEASE_OWNERSHIP

> macro to own the object for a long time. CL_OBJECT_TAKE_OWNERSHIP will

> not hold the lock and so will not cause deadlock problems.

> For example, when we call NDRange on some memobj, we should take the

> ownship of the memobj. If another thread call NDRange on the same

> memobj, we should return some error like CL_OUT_OF_RESOURCE to users

> and protect the memobj from accessing simultaneously.

> 

> Signed-off-by: Junyan He <junyan.he@intel.com>

> ---

>  src/CMakeLists.txt   |    1 +

>  src/cl_base_object.c |  102

> ++++++++++++++++++++++++++++++++++++++++++++++++++

>  src/cl_base_object.h |   77 +++++++++++++++++++++++++++++++++++++

>  3 files changed, 180 insertions(+)

>  create mode 100644 src/cl_base_object.c  create mode 100644

> src/cl_base_object.h

> 

> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index a002865..cec7cfc

> 100644

> --- a/src/CMakeLists.txt

> +++ b/src/CMakeLists.txt

> @@ -65,6 +65,7 @@ MakeKernelBinStr

> ("${CMAKE_CURRENT_SOURCE_DIR}/kernels/" "${BUILT_IN_NAME}")

> 

>  set(OPENCL_SRC

>      ${KERNEL_STR_FILES}

> +    cl_base_object.c

>      cl_api.c

>      cl_alloc.c

>      cl_kernel.c

> diff --git a/src/cl_base_object.c b/src/cl_base_object.c new file mode 100644

> index 0000000..4661977

> --- /dev/null

> +++ b/src/cl_base_object.c

> @@ -0,0 +1,102 @@

> +/*

> + * Copyright © 2012 Intel Corporation

> + *

> + * This library is free software; you can redistribute it and/or

> + * modify it under the terms of the GNU Lesser General Public

> + * License as published by the Free Software Foundation; either

> + * version 2.1 of the License, or (at your option) any later version.

> + *

> + * This library is distributed in the hope that it will be useful,

> + * but WITHOUT ANY WARRANTY; without even the implied warranty of

> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

> GNU

> + * Lesser General Public License for more details.

> + *

> + * You should have received a copy of the GNU Lesser General Public

> + * License along with this library. If not, see

> <http://www.gnu.org/licenses/>.

> + *

> + */

> +#include <stdio.h>

> +#include "cl_base_object.h"

> +

> +static pthread_t invalid_thread_id = -1;

> +

> +LOCAL void

> +cl_object_init_base(cl_base_object obj, cl_ulong magic) {

> +  obj->magic = magic;

> +  obj->ref = 1;

> +  SET_ICD(obj->dispatch);

> +  pthread_mutex_init(&obj->mutex, NULL);

> +  pthread_cond_init(&obj->cond, NULL);

> +  obj->owner = invalid_thread_id;

> +}

> +

> +LOCAL void

> +cl_object_destroy_base(cl_base_object obj) {

> +  int ref = CL_OBJECT_GET_REF(obj);

> +  if (ref != 0) {

> +    DEBUGP(DL_ERROR, "CL object %p, call destroy with a reference %d", obj,

> +           ref);

> +    assert(0);

> +  }

> +

> +  if (!CL_OBJECT_IS_VALID(obj)) {

> +    DEBUGP(DL_ERROR,

> +           "CL object %p, call destroy while it is already a dead object", obj);

> +    assert(0);

> +  }

> +

> +  if (obj->owner != invalid_thread_id) {

> +    DEBUGP(DL_ERROR, "CL object %p, call destroy while still has a

> owener %d",

> +           obj, (int)obj->owner);

> +    assert(0);

> +  }

> +

> +  obj->magic = CL_OBJECT_INVALID_MAGIC;

> +  pthread_mutex_destroy(&obj->mutex);

> +  pthread_cond_destroy(&obj->cond);

> +}

> +

> +LOCAL cl_int

> +cl_object_take_ownership(cl_base_object obj, cl_int wait) {

> +  pthread_t self;

> +

> +  assert(CL_OBJECT_IS_VALID(obj));

> +

> +  self = pthread_self();

> +

> +  pthread_mutex_lock(&obj->mutex);

> +  if (pthread_equal(obj->owner, invalid_thread_id)) {

> +    obj->owner = self;

> +    pthread_mutex_unlock(&obj->mutex);

> +    return 1;

> +  }

> +

> +  if (wait == 0) {

> +    pthread_mutex_unlock(&obj->mutex);

> +    return 0;

> +  }

> +

> +  while (!pthread_equal(obj->owner, invalid_thread_id)) {

> +    pthread_cond_wait(&obj->cond, &obj->mutex);  }

> +

> +  obj->owner = self;

> +  pthread_mutex_unlock(&obj->mutex);

> +  return 1;

> +}

> +

> +LOCAL void

> +cl_object_release_ownership(cl_base_object obj) {

> +  assert(CL_OBJECT_IS_VALID(obj));

> +

> +  pthread_mutex_lock(&obj->mutex);

> +  assert(pthread_equal(pthread_self(), obj->owner));  obj->owner =

> + invalid_thread_id;  pthread_cond_broadcast(&obj->cond);

> +

> +  pthread_mutex_unlock(&obj->mutex);

> +}

> diff --git a/src/cl_base_object.h b/src/cl_base_object.h new file mode

> 100644 index 0000000..cba4330

> --- /dev/null

> +++ b/src/cl_base_object.h

> @@ -0,0 +1,77 @@

> +/*

> + * Copyright © 2012 Intel Corporation

> + *

> + * This library is free software; you can redistribute it and/or

> + * modify it under the terms of the GNU Lesser General Public

> + * License as published by the Free Software Foundation; either

> + * version 2.1 of the License, or (at your option) any later version.

> + *

> + * This library is distributed in the hope that it will be useful,

> + * but WITHOUT ANY WARRANTY; without even the implied warranty of

> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

> GNU

> + * Lesser General Public License for more details.

> + *

> + * You should have received a copy of the GNU Lesser General Public

> + * License along with this library. If not, see

> <http://www.gnu.org/licenses/>.

> + *

> + */

> +

> +#ifndef __CL_BASE_OBJECT_H__

> +#define __CL_BASE_OBJECT_H__

> +

> +#include "cl_utils.h"

> +#include "cl_khr_icd.h"

> +#include "CL/cl.h"

> +#include <pthread.h>

> +#include <assert.h>

> +

> +/*********************************************************

> *************

> +**

> +  Every CL objects should have:

> +    ICD dispatcher: Hold the ICD function table pointer.

> +

> +    Reference: To maintain its' life time. CL retain/release API will

> +    change its value. We will destroy the object when the count reach 0.

> +

> +    Magic: Just a number to represent each CL object. We will use it

> +    to check whether it is the object we want.

> +

> +    Mutex & Cond: Used to protect the CL objects MT safe. lock/unlock

> +    critical region should be short enough and should not have any block

> +    function call. take_ownership/release_ownership  can own the object

> +    for a long time. take_ownership will not hold the lock and so will

> +    not cause deadlock problems. we can wait on the cond to get the

> +    ownership.

> +*********************************************************

> **************

> +**/

> +

> +typedef struct _cl_base_object {

> +  DEFINE_ICD(dispatch);         /* Dispatch function table for icd */

> +  cl_ulong magic;               /* Magic number for each CL object */

> +  atomic_t ref;                 /* Reference for each CL object */

> +  pthread_mutex_t mutex;        /* THe mutex to protect this object MT safe

> */

> +  pthread_cond_t cond;          /* Condition to wait for getting the object */

> +  pthread_t owner;              /* The thread which own this object */

> +} _cl_base_object;

> +

> +typedef struct _cl_base_object *cl_base_object;

> +

> +#define CL_OBJECT_INVALID_MAGIC 0xFEFEFEFEFEFEFEFELL #define

> +CL_OBJECT_IS_VALID(obj) (((cl_base_object)obj)->magic !=

> +CL_OBJECT_INVALID_MAGIC)

> +

> +#define CL_OBJECT_INC_REF(obj)

> +(atomic_inc(&((cl_base_object)obj)->ref))

> +#define CL_OBJECT_DEC_REF(obj)

> +(atomic_dec(&((cl_base_object)obj)->ref))

> +#define CL_OBJECT_GET_REF(obj) (atomic_add(&((cl_base_object)obj)-

> >ref,

> +0))


Is it need using atomic_add to implement CL_OBJECT_GET_REF? I think return ref directly is safe.
> -----Original Message-----

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

> Yang, Rong R

> Sent: Thursday, September 1, 2016 15:51

> To: junyan.he@inbox.com; beignet@lists.freedesktop.org

> Subject: Re: [Beignet] [PATCH 01/11] Runtime: Add CL base object for all cl

> objects.

> 

> One comment.

> 

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

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

> > Of junyan.he@inbox.com

> > Sent: Tuesday, July 19, 2016 19:26

> > To: beignet@lists.freedesktop.org

> > Subject: [Beignet] [PATCH 01/11] Runtime: Add CL base object for all

> > cl objects.

> >

> > From: Junyan He <junyan.he@intel.com>

> >

> > The runtime code is a little verbose in CL object handle.

> > Every CL objects should have a reference, a lock to protect itself and

> > an ICD dispatcher. We can organize them to a struct and place it at

> > the beginning of each CL object.

> > This base object is also used to protect the CL objects MT safe.

> > CL_OBJECT_LOCK/CL_OBJECT_UNLOCK macro will lock/unlock objects, but

> we

> > should use them within one function call, and the critical region

> > should be short.

> > We add CL_OBJECT_TAKE_OWNERSHIP/CL_OBJECT_RELEASE_OWNERSHIP

> > macro to own the object for a long time. CL_OBJECT_TAKE_OWNERSHIP

> will

> > not hold the lock and so will not cause deadlock problems.

> > For example, when we call NDRange on some memobj, we should take the

> > ownship of the memobj. If another thread call NDRange on the same

> > memobj, we should return some error like CL_OUT_OF_RESOURCE to

> users

> > and protect the memobj from accessing simultaneously.

> >

> > Signed-off-by: Junyan He <junyan.he@intel.com>

> > ---

> >  src/CMakeLists.txt   |    1 +

> >  src/cl_base_object.c |  102

> > ++++++++++++++++++++++++++++++++++++++++++++++++++

> >  src/cl_base_object.h |   77

> +++++++++++++++++++++++++++++++++++++

> >  3 files changed, 180 insertions(+)

> >  create mode 100644 src/cl_base_object.c  create mode 100644

> > src/cl_base_object.h

> >

> > diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index

> > a002865..cec7cfc

> > 100644

> > --- a/src/CMakeLists.txt

> > +++ b/src/CMakeLists.txt

> > @@ -65,6 +65,7 @@ MakeKernelBinStr

> > ("${CMAKE_CURRENT_SOURCE_DIR}/kernels/" "${BUILT_IN_NAME}")

> >

> >  set(OPENCL_SRC

> >      ${KERNEL_STR_FILES}

> > +    cl_base_object.c

> >      cl_api.c

> >      cl_alloc.c

> >      cl_kernel.c

> > diff --git a/src/cl_base_object.c b/src/cl_base_object.c new file mode

> > 100644 index 0000000..4661977

> > --- /dev/null

> > +++ b/src/cl_base_object.c

> > @@ -0,0 +1,102 @@

> > +/*

> > + * Copyright © 2012 Intel Corporation

> > + *

> > + * This library is free software; you can redistribute it and/or

> > + * modify it under the terms of the GNU Lesser General Public

> > + * License as published by the Free Software Foundation; either

> > + * version 2.1 of the License, or (at your option) any later version.

> > + *

> > + * This library is distributed in the hope that it will be useful,

> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of

> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

> > GNU

> > + * Lesser General Public License for more details.

> > + *

> > + * You should have received a copy of the GNU Lesser General Public

> > + * License along with this library. If not, see

> > <http://www.gnu.org/licenses/>.

> > + *

> > + */

> > +#include <stdio.h>

> > +#include "cl_base_object.h"

> > +

> > +static pthread_t invalid_thread_id = -1;

> > +

> > +LOCAL void

> > +cl_object_init_base(cl_base_object obj, cl_ulong magic) {

> > +  obj->magic = magic;

> > +  obj->ref = 1;

> > +  SET_ICD(obj->dispatch);

> > +  pthread_mutex_init(&obj->mutex, NULL);

> > +  pthread_cond_init(&obj->cond, NULL);

> > +  obj->owner = invalid_thread_id;

> > +}

> > +

> > +LOCAL void

> > +cl_object_destroy_base(cl_base_object obj) {

> > +  int ref = CL_OBJECT_GET_REF(obj);

> > +  if (ref != 0) {

> > +    DEBUGP(DL_ERROR, "CL object %p, call destroy with a reference %d",

> obj,

> > +           ref);

> > +    assert(0);

> > +  }

> > +

> > +  if (!CL_OBJECT_IS_VALID(obj)) {

> > +    DEBUGP(DL_ERROR,

> > +           "CL object %p, call destroy while it is already a dead object", obj);

> > +    assert(0);

> > +  }

> > +

> > +  if (obj->owner != invalid_thread_id) {

> > +    DEBUGP(DL_ERROR, "CL object %p, call destroy while still has a

> > owener %d",

> > +           obj, (int)obj->owner);

> > +    assert(0);

> > +  }

> > +

> > +  obj->magic = CL_OBJECT_INVALID_MAGIC;

> > +  pthread_mutex_destroy(&obj->mutex);

> > +  pthread_cond_destroy(&obj->cond);

> > +}

> > +

> > +LOCAL cl_int

> > +cl_object_take_ownership(cl_base_object obj, cl_int wait) {

> > +  pthread_t self;

> > +

> > +  assert(CL_OBJECT_IS_VALID(obj));

> > +

> > +  self = pthread_self();

> > +

> > +  pthread_mutex_lock(&obj->mutex);

> > +  if (pthread_equal(obj->owner, invalid_thread_id)) {

> > +    obj->owner = self;

> > +    pthread_mutex_unlock(&obj->mutex);

> > +    return 1;

> > +  }

> > +

> > +  if (wait == 0) {

> > +    pthread_mutex_unlock(&obj->mutex);

> > +    return 0;

> > +  }

> > +

> > +  while (!pthread_equal(obj->owner, invalid_thread_id)) {

> > +    pthread_cond_wait(&obj->cond, &obj->mutex);  }

> > +

> > +  obj->owner = self;

> > +  pthread_mutex_unlock(&obj->mutex);

> > +  return 1;

> > +}

> > +

> > +LOCAL void

> > +cl_object_release_ownership(cl_base_object obj) {

> > +  assert(CL_OBJECT_IS_VALID(obj));

> > +

> > +  pthread_mutex_lock(&obj->mutex);

> > +  assert(pthread_equal(pthread_self(), obj->owner));  obj->owner =

> > + invalid_thread_id;  pthread_cond_broadcast(&obj->cond);

> > +

> > +  pthread_mutex_unlock(&obj->mutex);

> > +}

> > diff --git a/src/cl_base_object.h b/src/cl_base_object.h new file mode

> > 100644 index 0000000..cba4330

> > --- /dev/null

> > +++ b/src/cl_base_object.h

> > @@ -0,0 +1,77 @@

> > +/*

> > + * Copyright © 2012 Intel Corporation

> > + *

> > + * This library is free software; you can redistribute it and/or

> > + * modify it under the terms of the GNU Lesser General Public

> > + * License as published by the Free Software Foundation; either

> > + * version 2.1 of the License, or (at your option) any later version.

> > + *

> > + * This library is distributed in the hope that it will be useful,

> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of

> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

> > GNU

> > + * Lesser General Public License for more details.

> > + *

> > + * You should have received a copy of the GNU Lesser General Public

> > + * License along with this library. If not, see

> > <http://www.gnu.org/licenses/>.

> > + *

> > + */

> > +

> > +#ifndef __CL_BASE_OBJECT_H__

> > +#define __CL_BASE_OBJECT_H__

> > +

> > +#include "cl_utils.h"

> > +#include "cl_khr_icd.h"

> > +#include "CL/cl.h"

> > +#include <pthread.h>

> > +#include <assert.h>

> > +

> >

> +/*********************************************************

> > *************

> > +**

> > +  Every CL objects should have:

> > +    ICD dispatcher: Hold the ICD function table pointer.

> > +

> > +    Reference: To maintain its' life time. CL retain/release API will

> > +    change its value. We will destroy the object when the count reach 0.

> > +

> > +    Magic: Just a number to represent each CL object. We will use it

> > +    to check whether it is the object we want.

> > +

> > +    Mutex & Cond: Used to protect the CL objects MT safe. lock/unlock

> > +    critical region should be short enough and should not have any block

> > +    function call. take_ownership/release_ownership  can own the object

> > +    for a long time. take_ownership will not hold the lock and so will

> > +    not cause deadlock problems. we can wait on the cond to get the

> > +    ownership.

> >

> +*********************************************************

> > **************

> > +**/

> > +

> > +typedef struct _cl_base_object {

> > +  DEFINE_ICD(dispatch);         /* Dispatch function table for icd */

> > +  cl_ulong magic;               /* Magic number for each CL object */

> > +  atomic_t ref;                 /* Reference for each CL object */

> > +  pthread_mutex_t mutex;        /* THe mutex to protect this object MT

> safe

> > */

> > +  pthread_cond_t cond;          /* Condition to wait for getting the object */

> > +  pthread_t owner;              /* The thread which own this object */

> > +} _cl_base_object;

> > +

> > +typedef struct _cl_base_object *cl_base_object;

> > +

> > +#define CL_OBJECT_INVALID_MAGIC 0xFEFEFEFEFEFEFEFELL #define

> > +CL_OBJECT_IS_VALID(obj) (((cl_base_object)obj)->magic !=

> > +CL_OBJECT_INVALID_MAGIC)

> > +

> > +#define CL_OBJECT_INC_REF(obj)

> > +(atomic_inc(&((cl_base_object)obj)->ref))

> > +#define CL_OBJECT_DEC_REF(obj)

> > +(atomic_dec(&((cl_base_object)obj)->ref))

> > +#define CL_OBJECT_GET_REF(obj) (atomic_add(&((cl_base_object)obj)-

> > >ref,

> > +0))

> 

> Is it need using atomic_add to implement CL_OBJECT_GET_REF? I think

> return ref directly is safe.

ref is atomic_t now, atomic_read() is ok.