Re: [PATCH 03/11] drm/amdgpu: nuke amdgpu_userq_fence_free

["Khatri, Sunil" <sukhatri@amd.com>]

On 22-04-2026 02:56 pm, Christian König wrote:
> On 4/22/26 10:29, Khatri, Sunil wrote:
>> On 21-04-2026 06:25 pm, Christian König wrote:
>>> As preparation for independent fences remove the function and do all of
>>> it's cleanup directly after signaling.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       | 13 +--
>>>   .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c   | 79 +++++++------------
>>>   .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.h   |  3 -
>>>   3 files changed, 31 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> index c6546a858597..1b15b51dc3f4 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> @@ -3162,11 +3162,7 @@ static int __init amdgpu_init(void)
>>>   
>>>   	r = amdgpu_sync_init();
>>>   	if (r)
>>> -		goto error_sync;
>>> -
>>> -	r = amdgpu_userq_fence_slab_init();
>>> -	if (r)
>>> -		goto error_fence;
>>> +		return r;
>>>   
>>>   	amdgpu_register_atpx_handler();
>>>   	amdgpu_acpi_detect();
>>> @@ -3182,12 +3178,6 @@ static int __init amdgpu_init(void)
>>>   
>>>   	/* let modprobe override vga console setting */
>>>   	return pci_register_driver(&amdgpu_kms_pci_driver);
>>> -
>>> -error_fence:
>>> -	amdgpu_sync_fini();
>>> -
>>> -error_sync:
>>> -	return r;
>>>   }
>>>   
>>>   static void __exit amdgpu_exit(void)
>>> @@ -3197,7 +3187,6 @@ static void __exit amdgpu_exit(void)
>>>   	amdgpu_unregister_atpx_handler();
>>>   	amdgpu_acpi_release();
>>>   	amdgpu_sync_fini();
>>> -	amdgpu_userq_fence_slab_fini();
>>>   	mmu_notifier_synchronize();
>>>   	amdgpu_xcp_drv_release();
>>>   }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>> index a58342c2ac44..909bdccc2a92 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>> @@ -32,29 +32,9 @@
>>>   #include "amdgpu.h"
>>>   #include "amdgpu_userq_fence.h"
>>>   
>>> -static const struct dma_fence_ops amdgpu_userq_fence_ops;
>>> -static struct kmem_cache *amdgpu_userq_fence_slab;
>>> -
>>>   #define AMDGPU_USERQ_MAX_HANDLES	(1U << 16)
>>>   
>>> -int amdgpu_userq_fence_slab_init(void)
>>> -{
>>> -	amdgpu_userq_fence_slab = kmem_cache_create("amdgpu_userq_fence",
>>> -						    sizeof(struct amdgpu_userq_fence),
>>> -						    0,
>>> -						    SLAB_HWCACHE_ALIGN,
>>> -						    NULL);
>> Are we not having benefit enough to continue create a cache here ? If that is fine that LGTM,
> Using all those kmem_cache instances was a bad idea to begin with.
>
> See the idea of a kmem_cache is to reduce the number of CPU cache lines and memory you need for certain number of objects when the object size is not a power of two.
>
> So for exampe two objects with 96 bytes only take 3 cache lines and 192 bytes instead of 256 bytes and 4 cache lines.
>
> But that difference is so marginally for most use cases that you absolutely don't need it.

Thanks for the explanation. Also if i am not wrong in cases where last 
no of such objects are often created and deleted kmem_cache helps in 
reuse and helps with internal fragmentation too.

Regards

Sunil Khatri

>
> Regards,
> Christian.
>
>> Acked-by: Sunil Khatri <sunil.khatri@amd.com>
>>
>> Regards
>> Sunil
>>
>>
>>
>>> -	if (!amdgpu_userq_fence_slab)
>>> -		return -ENOMEM;
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -void amdgpu_userq_fence_slab_fini(void)
>>> -{
>>> -	rcu_barrier();
>>> -	kmem_cache_destroy(amdgpu_userq_fence_slab);
>>> -}
>>> +static const struct dma_fence_ops amdgpu_userq_fence_ops;
>>>   
>>>   static inline struct amdgpu_userq_fence *to_amdgpu_userq_fence(struct dma_fence *f)
>>>   {
>>> @@ -146,12 +126,18 @@ amdgpu_userq_fence_driver_free(struct amdgpu_usermode_queue *userq)
>>>   }
>>>   
>>>   static void
>>> -amdgpu_userq_fence_put_fence_drv_array(struct amdgpu_userq_fence *userq_fence)
>>> +amdgpu_userq_fence_put_fence_drv_refs(struct amdgpu_userq_fence *userq_fence)
>>>   {
>>>   	unsigned long i;
>>> +
>>>   	for (i = 0; i < userq_fence->fence_drv_array_count; i++)
>>>   		amdgpu_userq_fence_driver_put(userq_fence->fence_drv_array[i]);
>>>   	userq_fence->fence_drv_array_count = 0;
>>> +	kfree(userq_fence->fence_drv_array);
>>> +	userq_fence->fence_drv_array = NULL;
>>> +
>>> +	amdgpu_userq_fence_driver_put(userq_fence->fence_drv);
>>> +	userq_fence->fence_drv = NULL;
>>>   }
>>>   
>>>   void amdgpu_userq_fence_driver_process(struct amdgpu_userq_fence_driver *fence_drv)
>>> @@ -181,10 +167,11 @@ void amdgpu_userq_fence_driver_process(struct amdgpu_userq_fence_driver *fence_d
>>>   		fence = &userq_fence->base;
>>>   		list_del_init(&userq_fence->link);
>>>   		dma_fence_signal(fence);
>>> -		/* Drop fence_drv_array outside fence_list_lock
>>> +		/*
>>> +		 * Drop fence_drv_array outside fence_list_lock
>>>   		 * to avoid the recursion lock.
>>>   		 */
>>> -		amdgpu_userq_fence_put_fence_drv_array(userq_fence);
>>> +		amdgpu_userq_fence_put_fence_drv_refs(userq_fence);
>>>   		dma_fence_put(fence);
>>>   	}
>>>   
>>> @@ -231,7 +218,7 @@ void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv)
>>>   
>>>   static int amdgpu_userq_fence_alloc(struct amdgpu_userq_fence **userq_fence)
>>>   {
>>> -	*userq_fence = kmem_cache_alloc(amdgpu_userq_fence_slab, GFP_ATOMIC);
>>> +	*userq_fence = kmalloc(sizeof(**userq_fence), GFP_ATOMIC);
>>>   	return *userq_fence ? 0 : -ENOMEM;
>>>   }
>>>   
>>> @@ -299,7 +286,7 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq,
>>>   	spin_unlock_irqrestore(&fence_drv->fence_list_lock, flags);
>>>   
>>>   	if (signaled)
>>> -		amdgpu_userq_fence_put_fence_drv_array(userq_fence);
>>> +		amdgpu_userq_fence_put_fence_drv_refs(userq_fence);
>>>   
>>>   	*f = fence;
>>>   
>>> @@ -333,29 +320,10 @@ static bool amdgpu_userq_fence_signaled(struct dma_fence *f)
>>>   	return false;
>>>   }
>>>   
>>> -static void amdgpu_userq_fence_free(struct rcu_head *rcu)
>>> -{
>>> -	struct dma_fence *fence = container_of(rcu, struct dma_fence, rcu);
>>> -	struct amdgpu_userq_fence *userq_fence = to_amdgpu_userq_fence(fence);
>>> -	struct amdgpu_userq_fence_driver *fence_drv = userq_fence->fence_drv;
>>> -
>>> -	/* Release the fence driver reference */
>>> -	amdgpu_userq_fence_driver_put(fence_drv);
>>> -
>>> -	kvfree(userq_fence->fence_drv_array);
>>> -	kmem_cache_free(amdgpu_userq_fence_slab, userq_fence);
>>> -}
>>> -
>>> -static void amdgpu_userq_fence_release(struct dma_fence *f)
>>> -{
>>> -	call_rcu(&f->rcu, amdgpu_userq_fence_free);
>>> -}
>>> -
>>>   static const struct dma_fence_ops amdgpu_userq_fence_ops = {
>>>   	.get_driver_name = amdgpu_userq_fence_get_driver_name,
>>>   	.get_timeline_name = amdgpu_userq_fence_get_timeline_name,
>>>   	.signaled = amdgpu_userq_fence_signaled,
>>> -	.release = amdgpu_userq_fence_release,
>>>   };
>>>   
>>>   /**
>>> @@ -546,7 +514,7 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
>>>   	r = amdgpu_userq_fence_create(queue, userq_fence, wptr, &fence);
>>>   	if (r) {
>>>   		mutex_unlock(&userq_mgr->userq_mutex);
>>> -		kmem_cache_free(amdgpu_userq_fence_slab, userq_fence);
>>> +		kfree(userq_fence);
>>>   		goto put_gobj_write;
>>>   	}
>>>   
>>> @@ -871,6 +839,7 @@ amdgpu_userq_wait_return_fence_info(struct drm_file *filp,
>>>   	for (i = 0, cnt = 0; i < num_fences; i++) {
>>>   		struct amdgpu_userq_fence_driver *fence_drv;
>>>   		struct amdgpu_userq_fence *userq_fence;
>>> +		unsigned long flags;
>>>   		u32 index;
>>>   
>>>   		userq_fence = to_amdgpu_userq_fence(fences[i]);
>>> @@ -886,7 +855,19 @@ amdgpu_userq_wait_return_fence_info(struct drm_file *filp,
>>>   			continue;
>>>   		}
>>>   
>>> +		spin_lock_irqsave(userq_fence->base.lock, flags);
>>> +		if (dma_fence_is_signaled_locked(&userq_fence->base)) {
>>> +			/*
>>> +			 * It is possible that fence is already signaled and the
>>> +			 * fence_drv now NULL, just skip over such fences.
>>> +			*/
>>> +			spin_unlock_irqrestore(userq_fence->base.lock, flags);
>>> +			continue;
>>> +		}
>>>   		fence_drv = userq_fence->fence_drv;
>>> +		amdgpu_userq_fence_driver_get(fence_drv);
>>> +		spin_unlock_irqrestore(userq_fence->base.lock, flags);
>>> +
>>>   		/*
>>>   		 * We need to make sure the user queue release their reference
>>>   		 * to the fence drivers at some point before queue destruction.
>>> @@ -895,10 +876,10 @@ amdgpu_userq_wait_return_fence_info(struct drm_file *filp,
>>>   		 */
>>>   		r = xa_alloc(&waitq->fence_drv_xa, &index, fence_drv,
>>>   			     xa_limit_32b, GFP_KERNEL);
>>> -		if (r)
>>> +		if (r) {
>>> +			amdgpu_userq_fence_driver_put(fence_drv);
>>>   			goto put_waitq;
>>> -
>>> -		amdgpu_userq_fence_driver_get(fence_drv);
>>> +		}
>>>   
>>>   		/* Store drm syncobj's gpu va address and value */
>>>   		fence_info[cnt].va = fence_drv->va;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h
>>> index d56246ad8c26..d355a0eecc07 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h
>>> @@ -58,9 +58,6 @@ struct amdgpu_userq_fence_driver {
>>>   	char timeline_name[TASK_COMM_LEN];
>>>   };
>>>   
>>> -int amdgpu_userq_fence_slab_init(void);
>>> -void amdgpu_userq_fence_slab_fini(void);
>>> -
>>>   void amdgpu_userq_fence_driver_get(struct amdgpu_userq_fence_driver *fence_drv);
>>>   void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv);
>>>   int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev,