Re: [PATCH v6 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 3 potential issue(s) to consider:

New issues:
- [Critical] Mempool exhaustion and direct reclaim deadlock in dirty extent allocation (`extent_alloc`).
- [High] `__gpu_buddy_free` misclassifies MIXED blocks as purely DIRTY, permanently losing track of clear pages.

Pre-existing issues:
- [High] Reclaim deadlock during block splitting due to `GFP_KERNEL` in `gpu_block_alloc`.
--

--- Patch [1]: [PATCH v6 1/2] gpu/buddy: replace dual-tree/force_merge with decoupled dirty tracker ---
commit 8d70f22110bb225d8340a7718c3a49a77a89a42c
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 with a single free tree
and a decoupled out-of-band dirty tracker. Fully cleared free blocks are
tracked outside using an augmented interval rbtree to enable O(log E)
lookup of the largest cleared extents.

> diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c
> index dc81fe0301ce8..755a0b9e47458 100644
> --- a/drivers/gpu/buddy.c
> +++ b/drivers/gpu/buddy.c

[ ... ]

> @@ -34,6 +35,418 @@
> +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);

[Severity: Critical]
Can this mempool size of 1 lead to a starvation deadlock?

Many operations (such as gpu_buddy_reset_clear() or gpu_buddy_free_list())
might require multiple extent allocations in a single locked transaction.

If the slab allocator fails, mempool_alloc() could exhaust the single
reserve element and then loop indefinitely waiting for an element to be
freed.

Since the thread holding the lock is blocked, no elements can be freed,
which could cause a permanent deadlock.

> +	if (!dirty_tracker->extent_pool)
> +		return -ENOMEM;
> +
> +	return 0;
> +}

[ ... ]

> +static struct gpu_dirty_extent *extent_alloc(struct gpu_dirty_tracker *dirty_tracker)
> +{
> +	struct gpu_dirty_extent *dirty_extent;
> +
> +	dirty_extent = mempool_alloc(dirty_tracker->extent_pool, GFP_KERNEL);

[Severity: Critical]
Is it safe to use GFP_KERNEL here while holding a driver lock?

This allocation is performed while holding a driver lock. GFP_KERNEL sets
__GFP_DIRECT_RECLAIM, which allows direct memory reclaim.

If the shrinker attempts to acquire the same driver lock to evict memory,
it could result in an AB-BA deadlock.

> +
> +	return dirty_extent;
> +}

[ ... ]

> @@ -253,37 +770,37 @@ __get_buddy(struct gpu_buddy_block *block)
>  }
>  
>  static unsigned int __gpu_buddy_free(struct gpu_buddy *mm,
> -				     struct gpu_buddy_block *block,
> -				     bool force_merge)
> +				     struct gpu_buddy_block *block)
>  {
> +	enum gpu_block_state block_state;
>  	struct gpu_buddy_block *parent;
>  	unsigned int order;
>  
> -	while ((parent = block->parent)) {
> -		struct gpu_buddy_block *buddy;
> +	block_state = gpu_buddy_block_is_clear(block) ? GPU_BLOCK_CLEAR :
> +							GPU_BLOCK_DIRTY;

[Severity: High]
Does this misclassify mixed blocks as purely dirty?

If a block is in the GPU_BLOCK_MIXED state (where has_clear is true but
is_clear is false), this initialization sets it to GPU_BLOCK_DIRTY because
it doesn't check the has_clear flag.

If the block's buddy is also dirty, the parent block state evaluates to
dirty and clears its has_clear flag.

This could cause the clear memory tracked by the dirty tracker to become
permanently inaccessible to GPU_BUDDY_CLEAR_ALLOCATION requests, as the
free tree's augment flags will cause the allocator to skip the subtree,
resulting in premature -ENOSPC allocation failures.

>  
> -		buddy = __get_buddy(block);
> +		if (!gpu_buddy_block_is_free(buddy))

[ ... ]

> @@ -696,7 +1139,7 @@ static void __gpu_buddy_undo_splits(struct gpu_buddy *mm,
>  	     gpu_buddy_block_is_free(buddy))) {
>  		rbtree_remove(mm, block);
>  		mm->free_scoreboard[gpu_buddy_block_order(block)]--;
> -		__gpu_buddy_free(mm, block, false);
> +		__gpu_buddy_free(mm, block);

[Severity: High]
Does feeding free mixed blocks into __gpu_buddy_free() here lose track
of clear pages?

When split_block() succeeds but the subsequent allocation fails, this path
merges the free child blocks. If the block is mixed, passing it to
__gpu_buddy_free() without marking it fully dirty in the tracker will cause
the misclassification issue mentioned above.

>  	}
>  }

[ ... ]

[Severity: High]
This is a pre-existing issue, but is it safe to use GFP_KERNEL when
allocating buddy blocks in gpu_block_alloc()?

Looking at gpu_block_alloc() in drivers/gpu/buddy.c:

	block = kmem_cache_zalloc(slab_blocks, GFP_KERNEL);
	if (!block)
		return NULL;

This allocation can occur during block splitting in gpu_buddy_alloc_blocks()
and gpu_buddy_block_trim(), which are executed while holding the driver's
memory manager lock.

Since GFP_KERNEL allows direct reclaim, the shrinker could attempt to evict
VRAM blocks and acquire the same lock, causing an AB-BA deadlock.

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