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 <[email protected]>
>> Suggested-by: Matthew Brost <[email protected]>
>> Cc: Thomas Hellström <[email protected]>
>> Cc: Michal Wajdeczko <[email protected]>
>> Cc: Matthew Auld <[email protected]>
>
> 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);
