Re: [PATCH v2 1/3] drm/sa: Split drm_suballoc_new() into SA alloc and init helpers

["Christian König" <christian.koenig@amd.com>]

On 2/6/26 13:34, Thomas Hellström wrote:
> + dri-devel, Christian
> 
> On Wed, 2026-02-04 at 16:46 +0000, Satyanarayana K V P wrote:
>> drm_suballoc_new() currently both allocates the SA object using
>> kmalloc()
>> and searches for a suitable hole in the sub-allocator for the
>> requested
>> size. Since kmalloc() may take internal locks, performing memory
>> allocation while holding the sub-allocator mutex can trigger lockdep
>> deadlock warnings.
> 
> ... As Matt mentioned, This is to be able to suballocate under a
> reclaim-tainted lock: Allocation can be done outside the lock, and init
> inside the lock.
> 
>>
>> Fix this by splitting SA allocation from drm_suballoc_new(),
>> separating
>> object allocation from sub-allocator initialization and hole
>> selection.
>>
>> Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
>> Suggested-by: Matthew Brost <matthew.brost@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Matthew Auld <matthew.auld@intel.com>
> 
> This must be CC'd dri-devel as well and AMD maintainers.
> 
> Below is in addition to MBrost's comments.
> 
>>
>> ---
>> V1 -> V2:
>> - Splitted drm_suballoc_new() into drm_suballoc_alloc() and
>> drm_suballoc_init() (Thomas).
>> ---
>>  drivers/gpu/drm/drm_suballoc.c | 135 +++++++++++++++++++++++++------
>> --
>>  include/drm/drm_suballoc.h     |   8 ++
>>  2 files changed, 112 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_suballoc.c
>> b/drivers/gpu/drm/drm_suballoc.c
>> index 879ea33dbbc4..6f21f9e048d6 100644
>> --- a/drivers/gpu/drm/drm_suballoc.c
>> +++ b/drivers/gpu/drm/drm_suballoc.c
>> @@ -123,7 +123,7 @@ static void drm_suballoc_remove_locked(struct
>> drm_suballoc *sa)
>>  	list_del_init(&sa->olist);
>>  	list_del_init(&sa->flist);
>>  	dma_fence_put(sa->fence);
>> -	kfree(sa);
>> +	drm_suballoc_release(sa);
>>  }
>>  
>>  static void drm_suballoc_try_free(struct drm_suballoc_manager
>> *sa_manager)
>> @@ -293,45 +293,75 @@ static bool drm_suballoc_next_hole(struct
>> drm_suballoc_manager *sa_manager,
>>  }
>>  
>>  /**
>> - * drm_suballoc_new() - Make a suballocation.
>> + * drm_suballoc_alloc() - Allocate uninitialized suballoc object.
>> + * @gfp: gfp flags used for memory allocation.
>> + *
>> + * Allocate memory for an uninitialized suballoc object. Intended
>> usage is
>> + * allocate memory for suballoc object outside of a reclaim tainted
>> context
>> + * and then be initialized at a later time in a reclaim tainted
>> context.
>> + *
>> + * @drm_suballoc_release should be used to release the memory if
>> returned
>> + * suballoc object is in uninitialized state.
>> + *
>> + * Return: a new uninitialized suballoc object, or an ERR_PTR(-
>> ENOMEM).
>> + */
>> +struct drm_suballoc *drm_suballoc_alloc(gfp_t gfp)
>> +{
>> +	struct drm_suballoc *sa;
>> +
>> +	sa = kmalloc(sizeof(*sa), gfp);
>> +	if (!sa)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	return sa;
>> +}
>> +EXPORT_SYMBOL(drm_suballoc_alloc);
>> +
>> +/**
>> + * drm_suballoc_release() - Release memory for suballocation.
>> + * @sa: The struct drm_suballoc.
>> + */
>> +void drm_suballoc_release(struct drm_suballoc *sa)
>> +{
>> +	kfree(sa);
>> +}
>> +EXPORT_SYMBOL(drm_suballoc_release);
>> +
>> +/**
>> + * drm_suballoc_init() - Initialize a suballocation.
>>   * @sa_manager: pointer to the sa_manager
>> + * @sa: The struct drm_suballoc.
>>   * @size: number of bytes we want to suballocate.
>> - * @gfp: gfp flags used for memory allocation. Typically GFP_KERNEL
>> but
>> - *       the argument is provided for suballocations from reclaim
>> context or
>> - *       where the caller wants to avoid pipelining rather than wait
>> for
>> - *       reclaim.
>>   * @intr: Whether to perform waits interruptible. This should
>> typically
>>   *        always be true, unless the caller needs to propagate a
>>   *        non-interruptible context from above layers.
>>   * @align: Alignment. Must not exceed the default manager alignment.
>>   *         If @align is zero, then the manager alignment is used.
>> + * @timeout: Time to a wait suballocation to become available.
> 
> I think we should split out the timeout to a separate patch, since that
> is not really related to the problem of the lockdep splat, and it would
> require a separate motivation, since typically the CTRL-C in
> combination with fence timeouts are sufficient. We need to explain why
> that's not the case for this particuar use-case.

Yeah, agree completely.

Timeouts need to have a justification, either because userspace said it wants to wait only that long for something to happen or because some HW specification says an operation can only take a specific amount of time.

Otherwise timeouts have the strong potential of being abused to solve deadlocks which is clearly not allowed upstream.

> 
> Also that would make the -fixes patches for the reclaim lockdep splat
> smaller.

Agree as well, skimmed only over it but it looks pretty good of hand.

Regards,
Christian.

> 
> 
> Otherwise LGTM.
> Thomas
> 
> 
>>   *
>> - * Try to make a suballocation of size @size, which will be rounded
>> - * up to the alignment specified in specified in
>> drm_suballoc_manager_init().
>> + * Try to make a suballocation on a pre-allocated suballoc object of
>> size @size,
>> + * which will be rounded up to the alignment specified in specified
>> in
>> + * drm_suballoc_manager_init().
>>   *
>> - * Return: a new suballocated bo, or an ERR_PTR.
>> + * Return: zero on success, errno on failure.
>>   */
>> -struct drm_suballoc *
>> -drm_suballoc_new(struct drm_suballoc_manager *sa_manager, size_t
>> size,
>> -		 gfp_t gfp, bool intr, size_t align)
>> +int drm_suballoc_init(struct drm_suballoc_manager *sa_manager,
>> +		      struct drm_suballoc *sa, size_t size,
>> +		      bool intr, size_t align, signed long timeout)
>>  {
>>  	struct dma_fence *fences[DRM_SUBALLOC_MAX_QUEUES];
>>  	unsigned int tries[DRM_SUBALLOC_MAX_QUEUES];
>>  	unsigned int count;
>>  	int i, r;
>> -	struct drm_suballoc *sa;
>>  
>>  	if (WARN_ON_ONCE(align > sa_manager->align))
>> -		return ERR_PTR(-EINVAL);
>> +		return -EINVAL;
>>  	if (WARN_ON_ONCE(size > sa_manager->size || !size))
>> -		return ERR_PTR(-EINVAL);
>> +		return -EINVAL;
>>  
>>  	if (!align)
>>  		align = sa_manager->align;
>>  
>> -	sa = kmalloc(sizeof(*sa), gfp);
>> -	if (!sa)
>> -		return ERR_PTR(-ENOMEM);
>>  	sa->manager = sa_manager;
>>  	sa->fence = NULL;
>>  	INIT_LIST_HEAD(&sa->olist);
>> @@ -339,6 +369,8 @@ drm_suballoc_new(struct drm_suballoc_manager
>> *sa_manager, size_t size,
>>  
>>  	spin_lock(&sa_manager->wq.lock);
>>  	do {
>> +		long t;
>> +
>>  		for (i = 0; i < DRM_SUBALLOC_MAX_QUEUES; ++i)
>>  			tries[i] = 0;
>>  
>> @@ -348,7 +380,7 @@ drm_suballoc_new(struct drm_suballoc_manager
>> *sa_manager, size_t size,
>>  			if (drm_suballoc_try_alloc(sa_manager, sa,
>>  						   size, align)) {
>>  				spin_unlock(&sa_manager->wq.lock);
>> -				return sa;
>> +				return 0;
>>  			}
>>  
>>  			/* see if we can skip over some allocations
>> */
>> @@ -359,34 +391,75 @@ drm_suballoc_new(struct drm_suballoc_manager
>> *sa_manager, size_t size,
>>  				fences[count++] =
>> dma_fence_get(fences[i]);
>>  
>>  		if (count) {
>> -			long t;
>> -
>>  			spin_unlock(&sa_manager->wq.lock);
>>  			t = dma_fence_wait_any_timeout(fences,
>> count, intr,
>> -						      
>> MAX_SCHEDULE_TIMEOUT,
>> -						       NULL);
>> +						       timeout,
>> NULL);
>>  			for (i = 0; i < count; ++i)
>>  				dma_fence_put(fences[i]);
>>  
>> -			r = (t > 0) ? 0 : t;
>>  			spin_lock(&sa_manager->wq.lock);
>>  		} else if (intr) {
>> +			spin_unlock(&sa_manager->wq.lock);
>>  			/* if we have nothing to wait for block */
>> -			r = wait_event_interruptible_locked
>> +			r = wait_event_interruptible_timeout
>>  				(sa_manager->wq,
>> -				 __drm_suballoc_event(sa_manager,
>> size, align));
>> +				 __drm_suballoc_event(sa_manager,
>> size, align),
>> +				 timeout);
>> +			spin_lock(&sa_manager->wq.lock);
>>  		} else {
>>  			spin_unlock(&sa_manager->wq.lock);
>> -			wait_event(sa_manager->wq,
>> -				   drm_suballoc_event(sa_manager,
>> size, align));
>> -			r = 0;
>> +			t = wait_event_timeout
>> +				(sa_manager->wq,
>> +				 drm_suballoc_event(sa_manager,
>> size, align),
>> +				 timeout);
>>  			spin_lock(&sa_manager->wq.lock);
>>  		}
>> +		r = (t > 0) ? 0 : !r ? -ETIME : t;
>>  	} while (!r);
>>  
>>  	spin_unlock(&sa_manager->wq.lock);
>> -	kfree(sa);
>> -	return ERR_PTR(r);
>> +	return r;
>> +}
>> +EXPORT_SYMBOL(drm_suballoc_init);
>> +
>> +/**
>> + * drm_suballoc_new() - Make a suballocation.
>> + * @sa_manager: pointer to the sa_manager
>> + * @size: number of bytes we want to suballocate.
>> + * @gfp: gfp flags used for memory allocation. Typically GFP_KERNEL
>> but
>> + *       the argument is provided for suballocations from reclaim
>> context or
>> + *       where the caller wants to avoid pipelining rather than wait
>> for
>> + *       reclaim.
>> + * @intr: Whether to perform waits interruptible. This should
>> typically
>> + *        always be true, unless the caller needs to propagate a
>> + *        non-interruptible context from above layers.
>> + * @align: Alignment. Must not exceed the default manager alignment.
>> + *         If @align is zero, then the manager alignment is used.
>> + *
>> + * Try to make a suballocation of size @size, which will be rounded
>> + * up to the alignment specified in specified in
>> drm_suballoc_manager_init().
>> + *
>> + * Return: a new suballocated bo, or an ERR_PTR.
>> + */
>> +struct drm_suballoc *
>> +drm_suballoc_new(struct drm_suballoc_manager *sa_manager, size_t
>> size,
>> +		 gfp_t gfp, bool intr, size_t align)
>> +{
>> +	struct drm_suballoc *sa;
>> +	int err;
>> +
>> +	sa = drm_suballoc_alloc(gfp);
>> +	if (IS_ERR(sa))
>> +		return sa;
>> +
>> +	err = drm_suballoc_init(sa_manager, sa, size, intr, align,
>> +				MAX_SCHEDULE_TIMEOUT);
>> +	if (err) {
>> +		drm_suballoc_release(sa);
>> +		return ERR_PTR(err);
>> +	}
>> +
>> +	return sa;
>>  }
>>  EXPORT_SYMBOL(drm_suballoc_new);
>>  
>> diff --git a/include/drm/drm_suballoc.h b/include/drm/drm_suballoc.h
>> index 7ba72a81a808..cff0e14556d1 100644
>> --- a/include/drm/drm_suballoc.h
>> +++ b/include/drm/drm_suballoc.h
>> @@ -53,6 +53,14 @@ void drm_suballoc_manager_init(struct
>> drm_suballoc_manager *sa_manager,
>>  
>>  void drm_suballoc_manager_fini(struct drm_suballoc_manager
>> *sa_manager);
>>  
>> +struct drm_suballoc *drm_suballoc_alloc(gfp_t gfp);
>> +
>> +void drm_suballoc_release(struct drm_suballoc *sa);
>> +
>> +int drm_suballoc_init(struct drm_suballoc_manager *sa_manager,
>> +		      struct drm_suballoc *sa, size_t size, bool
>> intr,
>> +		      size_t align, signed long timeout);
>> +
>>  struct drm_suballoc *
>>  drm_suballoc_new(struct drm_suballoc_manager *sa_manager, size_t
>> size,
>>  		 gfp_t gfp, bool intr, size_t align);