[Beignet] [PATCH 01/11] Runtime: Add CL base object for all cl objects.
Yang, Rong R
rong.r.yang at intel.com
Thu Sep 1 07:59:04 UTC 2016
> -----Original Message-----
> From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf Of
> Yang, Rong R
> Sent: Thursday, September 1, 2016 15:51
> To: junyan.he at inbox.com; beignet at 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 at lists.freedesktop.org] On Behalf
> > Of junyan.he at inbox.com
> > Sent: Tuesday, July 19, 2016 19:26
> > To: beignet at lists.freedesktop.org
> > Subject: [Beignet] [PATCH 01/11] Runtime: Add CL base object for all
> > cl objects.
> >
> > From: Junyan He <junyan.he at 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 at 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.
More information about the Beignet
mailing list