Am 08.08.2017 um 09:49 schrieb Liu, Monk: >> 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 ... Yeah, that obviously works as well :) Christian. > > BR Monk > > -----Original Message----- > From: Christian König [mailto:deathsimple at vodafone.de] > 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; >> } >> > > _______________________________________________ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
