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]> > > --- > 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);
