On 2/10/26 13:09, Thomas Hellström wrote: > On Tue, 2026-02-10 at 10:59 +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. If SA allocation is done by holding sub-allocator mutex, this >> design >> can lead to reclaim safety issues. >> >> By splitting the kmalloc() step outside of the critical section, we >> allow >> the memory allocation to use GFP_KERNEL (reclaim-safe) while ensuring >> that >> the initialization step that holds reclaim-tainted locks (sub- >> allocator >> mutex) operates in a reclaim-unsafe context with pre-allocated >> memory. >> >> This separation prevents potential deadlocks where memory reclaim >> could >> attempt to acquire locks that are already held during the sub- >> allocator >> operations. >> >> 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]> >> Cc: Christian König <[email protected]> >> Cc: [email protected] > > LGTM. > Reviewed-by: Thomas Hellström <[email protected]>
Reviewed-by: Christian König <[email protected]> Where are patches 2 and 3 in that series? Regards, Christian. > >> >> --- >> V2 -> V3: >> - Updated commit message (Matt, Thomas & Christian). >> - Removed timeout logic from drm_suballoc_init(). (Thomas & >> Christian). >> >> V1 -> V2: >> - Splitted drm_suballoc_new() into drm_suballoc_alloc() and >> drm_suballoc_init() (Thomas). >> --- >> drivers/gpu/drm/drm_suballoc.c | 110 ++++++++++++++++++++++++++----- >> -- >> include/drm/drm_suballoc.h | 8 +++ >> 2 files changed, 97 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_suballoc.c >> b/drivers/gpu/drm/drm_suballoc.c >> index 879ea33dbbc4..b97ffcd98d45 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,74 @@ 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. >> * >> - * 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) >> { >> 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); >> @@ -348,7 +377,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 >> */ >> @@ -385,8 +414,47 @@ drm_suballoc_new(struct drm_suballoc_manager >> *sa_manager, size_t size, >> } 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); >> + 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..b8d1d5449fd8 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); >> + >> struct drm_suballoc * >> drm_suballoc_new(struct drm_suballoc_manager *sa_manager, size_t >> size, >> gfp_t gfp, bool intr, size_t align);
