>Please add some spaces in "src?&src->refcount:NULL" after each operator. >With that fixed the patch is Reviewed-by: Christian König <christian.koenig at amd.com>. Actually this " src?&src->refcount:NULL" is totally removed in second cleanup patch ... BR Monk -----Original Message----- From: Christian König [mailto:deathsimple@xxxxxxxxxxx] Sent: Tuesday, August 8, 2017 3:44 PM To: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org Subject: Re: [PATCH libdrm 1/2] drm:fix race issue between two bo functions(v2) Am 08.08.2017 um 09:34 schrieb Monk Liu: > From: Monk Liu <monk.liu at amd.com> > > there is race issue between two threads on amdgpu_bo_reference and > amdgpu_bo_import, this patch tends to fix it by moving the > pthread_mutex_lock out of bo_free_internal and move to bo_reference to > cover the update_reference part. > > The mutex_unlock in bo_import should also cover bo refcount > increasement. > > Change-Id: I1f65eacf74cd28cc0d3a71ef2f7a19b890d63c29 > Signed-off-by: Monk Liu <monk.liu at amd.com> > --- > amdgpu/amdgpu_bo.c | 5 +---- > amdgpu/amdgpu_internal.h | 15 ++++++++++++--- > 2 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c index > 07eb743..82f38c0 100644 > --- a/amdgpu/amdgpu_bo.c > +++ b/amdgpu/amdgpu_bo.c > @@ -55,14 +55,12 @@ static void amdgpu_close_kms_handle(amdgpu_device_handle dev, > void amdgpu_bo_free_internal(amdgpu_bo_handle bo) > { > /* Remove the buffer from the hash tables. */ > - pthread_mutex_lock(&bo->dev->bo_table_mutex); > util_hash_table_remove(bo->dev->bo_handles, > (void*)(uintptr_t)bo->handle); > if (bo->flink_name) { > util_hash_table_remove(bo->dev->bo_flink_names, > (void*)(uintptr_t)bo->flink_name); > } > - pthread_mutex_unlock(&bo->dev->bo_table_mutex); > > /* Release CPU access. */ > if (bo->cpu_map_count > 0) { > @@ -340,10 +338,9 @@ int amdgpu_bo_import(amdgpu_device_handle dev, > } > > if (bo) { > - pthread_mutex_unlock(&dev->bo_table_mutex); > - > /* The buffer already exists, just bump the refcount. */ > atomic_inc(&bo->refcount); > + pthread_mutex_unlock(&dev->bo_table_mutex); > > output->buf_handle = bo; > output->alloc_size = bo->alloc_size; diff --git > a/amdgpu/amdgpu_internal.h b/amdgpu/amdgpu_internal.h index > 0508131..79da0e7 100644 > --- a/amdgpu/amdgpu_internal.h > +++ b/amdgpu/amdgpu_internal.h > @@ -143,7 +143,7 @@ void amdgpu_vamgr_deinit(struct amdgpu_bo_va_mgr *mgr); > uint64_t amdgpu_vamgr_find_va(struct amdgpu_bo_va_mgr *mgr, uint64_t size, > uint64_t alignment, uint64_t base_required); > > -void amdgpu_vamgr_free_va(struct amdgpu_bo_va_mgr *mgr, uint64_t va, > +void amdgpu_vamgr_free_va(struct amdgpu_bo_va_mgr *mgr, uint64_t va, > uint64_t size); > > int amdgpu_query_gpu_info_init(amdgpu_device_handle dev); @@ -193,8 > +193,17 @@ static inline bool update_references(atomic_t *dst, atomic_t *src) > static inline void amdgpu_bo_reference(struct amdgpu_bo **dst, > struct amdgpu_bo *src) > { > - if (update_references(&(*dst)->refcount, &src->refcount)) > - amdgpu_bo_free_internal(*dst); > + pthread_mutex_t *mlock; > + struct amdgpu_bo* bo = *dst; > + > + assert(bo != NULL); > + mlock = &bo->dev->bo_table_mutex; > + pthread_mutex_lock(mlock); > + > + if (update_references(&bo->refcount, src?&src->refcount:NULL)) Please add some spaces in "src?&src->refcount:NULL" after each operator. With that fixed the patch is Reviewed-by: Christian König <christian.koenig at amd.com>. Regards, Christian. > + amdgpu_bo_free_internal(bo); > + > + pthread_mutex_unlock(mlock); > *dst = src; > } >
