drm/amdgpu: add rcu_barrier after entity fini

Submitted by Deng, Emily on May 17, 2018, 3:05 a.m.

Details

Message ID 1526526326-5130-1-git-send-email-Emily.Deng@amd.com
State New
Headers show
Series "drm/amdgpu: add rcu_barrier after entity fini" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Deng, Emily May 17, 2018, 3:05 a.m.
To free the fence from the amdgpu_fence_slab, need twice call_rcu, to avoid
the amdgpu_fence_slab_fini call kmem_cache_destroy(amdgpu_fence_slab) before
kmem_cache_free(amdgpu_fence_slab, fence), add rcu_barrier after drm_sched_entity_fini.

The kmem_cache_free(amdgpu_fence_slab, fence)'s call trace as below:
1.drm_sched_entity_fini ->
drm_sched_entity_cleanup ->
dma_fence_put(entity->last_scheduled) ->
drm_sched_fence_release_finished ->
drm_sched_fence_release_scheduled ->
call_rcu(&fence->finished.rcu, drm_sched_fence_free)

2.drm_sched_fence_free ->
dma_fence_put(fence->parent) ->
amdgpu_fence_release ->
call_rcu(&f->rcu, amdgpu_fence_free) ->
kmem_cache_free(amdgpu_fence_slab, fence);

Change-Id: I8dcadd3372f97e72461bf46b41cc26d90f09b8df
Signed-off-by: Emily Deng <Emily.Deng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
 1 file changed, 1 insertion(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index cc3b067..07b2e10 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -134,6 +134,7 @@  static void amdgpu_ttm_global_fini(struct amdgpu_device *adev)
 	if (adev->mman.mem_global_referenced) {
 		drm_sched_entity_fini(adev->mman.entity.sched,
 				      &adev->mman.entity);
+		rcu_barrier();
 		mutex_destroy(&adev->mman.gtt_window_lock);
 		drm_global_item_unref(&adev->mman.bo_global_ref.ref);
 		drm_global_item_unref(&adev->mman.mem_global_ref);

Comments

On 2018-05-17 05:05 AM, Emily Deng wrote:
> To free the fence from the amdgpu_fence_slab, need twice call_rcu, to avoid
> the amdgpu_fence_slab_fini call kmem_cache_destroy(amdgpu_fence_slab) before
> kmem_cache_free(amdgpu_fence_slab, fence), add rcu_barrier after drm_sched_entity_fini.
> 
> The kmem_cache_free(amdgpu_fence_slab, fence)'s call trace as below:
> 1.drm_sched_entity_fini ->
> drm_sched_entity_cleanup ->
> dma_fence_put(entity->last_scheduled) ->
> drm_sched_fence_release_finished ->
> drm_sched_fence_release_scheduled ->
> call_rcu(&fence->finished.rcu, drm_sched_fence_free)
> 
> 2.drm_sched_fence_free ->
> dma_fence_put(fence->parent) ->
> amdgpu_fence_release ->
> call_rcu(&f->rcu, amdgpu_fence_free) ->
> kmem_cache_free(amdgpu_fence_slab, fence);
> 
> Change-Id: I8dcadd3372f97e72461bf46b41cc26d90f09b8df
> Signed-off-by: Emily Deng <Emily.Deng@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index cc3b067..07b2e10 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -134,6 +134,7 @@ static void amdgpu_ttm_global_fini(struct amdgpu_device *adev)
>  	if (adev->mman.mem_global_referenced) {
>  		drm_sched_entity_fini(adev->mman.entity.sched,
>  				      &adev->mman.entity);
> +		rcu_barrier();
>  		mutex_destroy(&adev->mman.gtt_window_lock);
>  		drm_global_item_unref(&adev->mman.bo_global_ref.ref);
>  		drm_global_item_unref(&adev->mman.mem_global_ref);
> 

Hmm, this makes me wonder if
https://bugs.freedesktop.org/show_bug.cgi?id=106225 could be related to
something like this as well.
Am 17.05.2018 um 05:05 schrieb Emily Deng:
> To free the fence from the amdgpu_fence_slab, need twice call_rcu, to avoid
> the amdgpu_fence_slab_fini call kmem_cache_destroy(amdgpu_fence_slab) before
> kmem_cache_free(amdgpu_fence_slab, fence), add rcu_barrier after drm_sched_entity_fini.
>
> The kmem_cache_free(amdgpu_fence_slab, fence)'s call trace as below:
> 1.drm_sched_entity_fini ->
> drm_sched_entity_cleanup ->
> dma_fence_put(entity->last_scheduled) ->
> drm_sched_fence_release_finished ->
> drm_sched_fence_release_scheduled ->
> call_rcu(&fence->finished.rcu, drm_sched_fence_free)
>
> 2.drm_sched_fence_free ->
> dma_fence_put(fence->parent) ->
> amdgpu_fence_release ->
> call_rcu(&f->rcu, amdgpu_fence_free) ->
> kmem_cache_free(amdgpu_fence_slab, fence);
>
> Change-Id: I8dcadd3372f97e72461bf46b41cc26d90f09b8df
> Signed-off-by: Emily Deng <Emily.Deng@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index cc3b067..07b2e10 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -134,6 +134,7 @@ static void amdgpu_ttm_global_fini(struct amdgpu_device *adev)
>   	if (adev->mman.mem_global_referenced) {
>   		drm_sched_entity_fini(adev->mman.entity.sched,
>   				      &adev->mman.entity);
> +		rcu_barrier();

Good catch, but why don't you put the barrier() before the 
kmem_cache_destroy()?

That looks like it would make more sense.

Christian.

>   		mutex_destroy(&adev->mman.gtt_window_lock);
>   		drm_global_item_unref(&adev->mman.bo_global_ref.ref);
>   		drm_global_item_unref(&adev->mman.mem_global_ref);
Hi Christian,
     Good suggestion. Put the barrier() before the kmem_cache_destroy() will be better. I will send a v2 patch later. Thanks

Best Wishes,
Emily Deng

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

> From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com]

> Sent: Thursday, May 17, 2018 4:39 PM

> To: Deng, Emily <Emily.Deng@amd.com>; amd-gfx@lists.freedesktop.org

> Subject: Re: [PATCH] drm/amdgpu: add rcu_barrier after entity fini

> 

> Am 17.05.2018 um 05:05 schrieb Emily Deng:

> > To free the fence from the amdgpu_fence_slab, need twice call_rcu, to

> > avoid the amdgpu_fence_slab_fini call

> > kmem_cache_destroy(amdgpu_fence_slab) before

> kmem_cache_free(amdgpu_fence_slab, fence), add rcu_barrier after

> drm_sched_entity_fini.

> >

> > The kmem_cache_free(amdgpu_fence_slab, fence)'s call trace as below:

> > 1.drm_sched_entity_fini ->

> > drm_sched_entity_cleanup ->

> > dma_fence_put(entity->last_scheduled) ->

> > drm_sched_fence_release_finished ->

> drm_sched_fence_release_scheduled

> > -> call_rcu(&fence->finished.rcu, drm_sched_fence_free)

> >

> > 2.drm_sched_fence_free ->

> > dma_fence_put(fence->parent) ->

> > amdgpu_fence_release ->

> > call_rcu(&f->rcu, amdgpu_fence_free) ->

> > kmem_cache_free(amdgpu_fence_slab, fence);

> >

> > Change-Id: I8dcadd3372f97e72461bf46b41cc26d90f09b8df

> > Signed-off-by: Emily Deng <Emily.Deng@amd.com>

> > ---

> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 1 +

> >   1 file changed, 1 insertion(+)

> >

> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c

> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c

> > index cc3b067..07b2e10 100644

> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c

> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c

> > @@ -134,6 +134,7 @@ static void amdgpu_ttm_global_fini(struct

> amdgpu_device *adev)

> >   	if (adev->mman.mem_global_referenced) {

> >   		drm_sched_entity_fini(adev->mman.entity.sched,

> >   				      &adev->mman.entity);

> > +		rcu_barrier();

> 

> Good catch, but why don't you put the barrier() before the

> kmem_cache_destroy()?

> 

> That looks like it would make more sense.

> 

> Christian.

> 

> >   		mutex_destroy(&adev->mman.gtt_window_lock);

> >   		drm_global_item_unref(&adev->mman.bo_global_ref.ref);

> >   		drm_global_item_unref(&adev->mman.mem_global_ref);