Re: [PATCH v7 1/2] gpu/buddy: replace dual-tree/force_merge with decoupled dirty tracker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Mempool deadlock in `gpu_buddy_reset_clear` and free paths during extent allocation when multiple extents are required within a single locked section.
--

--- Patch [1]: [PATCH v7 1/2] gpu/buddy: replace dual-tree/force_merge with decoupled dirty tracker ---
commit e1f55d34ee0b3bd852cd9853e793c8cbdf7fc6f6
Author: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>

gpu/buddy: replace dual-tree/force_merge with decoupled dirty tracker

This commit replaces the dual-tree buddy allocator design with a single free
tree and an out-of-band dirty tracker using an augmented interval rbtree. It
also moves extent allocations to a mempool to guarantee progress and avoid
deadlocks associated with __GFP_NOFAIL.

> diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c
> --- a/drivers/gpu/buddy.c
> +++ b/drivers/gpu/buddy.c
> @@ -34,6 +35,441 @@
>  #endif
>  
>  static struct kmem_cache *slab_blocks;
> +static struct kmem_cache *slab_extents;
> +
> +/*
> + * A single reserved extent suffices. Every allocation uses GFP_KERNEL
> + * from sleepable context, so the underlying slab alloc almost always
> + * succeeds via reclaim; the reserve only backstops the rare case where
> + * it still returns NULL (e.g. the current task is an OOM victim),
> + * guaranteeing a non-NULL extent without __GFP_NOFAIL. Because each
> + * alloc can independently wait for reclaim, the reserve need not scale
> + * with the number of extents added in one locked section (e.g. by
> + * gpu_buddy_reset_clear()).
> + */
> +#define GPU_DIRTY_EXTENT_POOL_MIN 1

[ ... ]

> +static struct gpu_dirty_extent *extent_alloc(struct gpu_dirty_tracker *dirty_tracker)
> +{
> +	/*
> +	 * The void free/reset paths must record an extent and cannot handle
> +	 * failure, so the mempool reserve guarantees a non-NULL return
> +	 * without __GFP_NOFAIL. GFP_KERNEL is safe under the buddy lock: no
> +	 * driver frees buddy blocks from a shrinker, so reclaim cannot
> +	 * recurse into the lock we hold.
> +	 */
> +	return mempool_alloc(dirty_tracker->extent_pool, GFP_KERNEL);
> +}

[ ... ]

> +static int gpu_dirty_tracker_init(struct gpu_dirty_tracker *dirty_tracker)
> +{
> +	dirty_tracker->root = RB_ROOT;
> +	dirty_tracker->total_dirty = 0;
> +
> +	dirty_tracker->extent_pool =
> +		mempool_create_slab_pool(GPU_DIRTY_EXTENT_POOL_MIN, slab_extents);
> +	if (!dirty_tracker->extent_pool)
> +		return -ENOMEM;
> +
> +	return 0;
> +}

[ ... ]

> @@ -572,42 +1055,39 @@ static int split_block(struct gpu_buddy *mm,
>   */
>  void gpu_buddy_reset_clear(struct gpu_buddy *mm, bool is_clear)
>  {
> -	enum gpu_buddy_free_tree src_tree, dst_tree;
> -	u64 root_size, size, start;
> -	unsigned int order;
> -	int i;
> +	unsigned int i;
>  
>  	gpu_buddy_driver_lock_held(mm);

[ ... ]

> +	for (i = 0; i <= mm->max_order; ++i) {
> -		struct rb_root *root = &mm->free_trees[src_tree][i];
>  		struct gpu_buddy_block *block, *tmp;
>  
> -		rbtree_postorder_for_each_entry_safe(block, tmp, root, rb) {
> -			rbtree_remove(mm, block);
> +		rbtree_postorder_for_each_entry_safe(block, tmp,
> +						     &mm->free_tree[i], rb) {
>  			if (is_clear) {
> -				mark_cleared(block);
> -				mm->clear_avail += gpu_buddy_block_size(mm, block);
> +				if (!gpu_buddy_block_is_clear(block))
> +					block->header |= GPU_BUDDY_HEADER_CLEAR;
> +				block->has_clear = true;
> +			} else if (gpu_buddy_block_is_clear(block)) {
> +				block->header &= ~GPU_BUDDY_HEADER_CLEAR;
> +				block->has_clear = false;
> +				gpu_dirty_tracker_mark_dirty(&mm->dirty,
> +							     gpu_buddy_block_offset(block),
> +							     gpu_buddy_block_size(mm, block));

[Severity: High]
Could this loop trigger a mempool deadlock during memory pressure?

In gpu_dirty_tracker_init(), the extent pool is created with a minimum
size of GPU_DIRTY_EXTENT_POOL_MIN (1). However, this loop in
gpu_buddy_reset_clear() holds the driver lock and iterates over the free
tree, potentially calling gpu_dirty_tracker_mark_dirty() multiple times.

If the underlying slab allocation fails during extent_alloc(),
mempool_alloc() will use the single reserved extent. If a second allocation
fails within the same locked section, mempool_alloc() with GFP_KERNEL will
sleep and retry indefinitely.

Since the driver lock is held, no other thread can free blocks to return
extents to the mempool, resulting in a deadlock. Does the mempool reserve
need to scale with the maximum number of extents that can be allocated
in a single locked section?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731070741.2654251-1-Arunpravin.PaneerSelvam@amd.com?part=1