Re: [PATCH v5 1/2] gpu/buddy: restore two-pass clear/dirty guard for range-biased allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpu/buddy: block_incompatible() mishandles split and mixed blocks in __alloc_range_bias()
--

commit 6dff7f0e6dce9c68ecb98f2d911199843d2b4401
Author: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>

gpu/buddy: restore two-pass clear/dirty guard for range-biased allocation

This commit introduces a two-pass search in __gpu_buddy_alloc_range_bias()
using block_incompatible() in the strict pass to prevent range-restricted
allocations from splitting opposite-class (clear vs. dirty) blocks unless
no matching blocks exist in the range.

> diff --git a/drivers/gpu/buddy.c b/drivers/gpu/buddy.c
> index 503f7d5934e4b..91935c71c5255 100644
> --- a/drivers/gpu/buddy.c
> +++ b/drivers/gpu/buddy.c
> @@ -1240,11 +1240,19 @@ static void __gpu_buddy_undo_splits(struct gpu_buddy *mm,
>  	}
>  }
>  
> +static bool block_incompatible(struct gpu_buddy_block *block, unsigned int flags)
> +{
> +	bool needs_clear = flags & GPU_BUDDY_CLEAR_ALLOCATION;
> +
> +	return needs_clear != gpu_buddy_block_is_clear(block);
> +}
> +
>  static struct gpu_buddy_block *
>  __alloc_range_bias(struct gpu_buddy *mm,
>  		   u64 start, u64 end,
>  		   unsigned int order,
> -		   unsigned long flags)
> +		   unsigned long flags,
> +		   bool fallback)
>  {

[ ... ]

> @@ -1298,6 +1304,9 @@ __alloc_range_bias(struct gpu_buddy *mm,
>  				continue;
>  		}
>  
> +		if (!fallback && block_incompatible(block, flags))
> +			continue;
> +
>  		if (contains(start, end, block_start, block_end) &&
>  		    order == gpu_buddy_block_order(block)) {
>  			/*
> @@ -1315,38 +1324,8 @@ __alloc_range_bias(struct gpu_buddy *mm,
>  				goto err_undo;
>  		}
>  
> -		/*
> -		 * Top-down is a strict address-placement policy, so when it is
> -		 * requested we ignore clear steering and simply descend the
> -		 * right (higher-address) child first. Only a non-top-down clear
> -		 * request biases the descent towards clear/has_clear subtrees.
> -		 */
> -		if ((flags & GPU_BUDDY_CLEAR_ALLOCATION) &&
> -		    !(flags & GPU_BUDDY_TOPDOWN_ALLOCATION)) {
> -			struct gpu_buddy_block *prefer;
> -
> -			if (gpu_buddy_block_is_clear(block->right))
> -				prefer = block->right;
> -			else if (gpu_buddy_block_is_clear(block->left))
> -				prefer = block->left;
> -			else if (block->right->has_clear)
> -				prefer = block->right;
> -			else if (block->left->has_clear)
> -				prefer = block->left;
> -			else
> -				prefer = block->right;
> -
> -			if (prefer == block->right) {
> -				list_add(&block->left->tmp_link, &dfs);
> -				list_add(&block->right->tmp_link, &dfs);
> -			} else {
> -				list_add(&block->right->tmp_link, &dfs);
> -				list_add(&block->left->tmp_link, &dfs);
> -			}
> -		} else {
> -			list_add(&block->left->tmp_link, &dfs);
> -			list_add(&block->right->tmp_link, &dfs);
> -		}
> +		list_add(&block->left->tmp_link, &dfs);
> +		list_add(&block->right->tmp_link, &dfs);
>  	} while (1);

[Severity: High]
Does calling block_incompatible() on every visited node in
__alloc_range_bias() before checking whether the block is split or mixed
mishandle internal GPU_BUDDY_SPLIT nodes and coalesced GPU_BLOCK_MIXED free
blocks?

First, when mark_split() transitions a block to GPU_BUDDY_SPLIT, it leaves
the previous GPU_BUDDY_HEADER_CLEAR bit intact on the internal node:

drivers/gpu/buddy.c:mark_split() {
	block->header &= ~GPU_BUDDY_HEADER_STATE;
	block->header |= GPU_BUDDY_SPLIT;
	...
}

Because gpu_buddy_init() initializes roots as dirty, any root or ancestor
split while dirty keeps gpu_buddy_block_is_clear(block) == false while any
descendant remains allocated. Conversely, a clear block that is split and
later has a descendant freed as dirty keeps gpu_buddy_block_is_clear(block)
== true on the split ancestor.

When __gpu_buddy_alloc_range_bias() runs the first pass (fallback = false),
__alloc_range_bias() evaluates block_incompatible(block, flags) on those
GPU_BUDDY_SPLIT ancestors and skips the entire subtree without descending
into its children. If no other root satisfies the request, the first pass
returns -ENOSPC and __gpu_buddy_alloc_range_bias() falls back to the second
pass (fallback = true), where block_incompatible() is disabled and the
clear/has_clear child-ordering preference has been removed, causing the
search to split the highest-address extent regardless of class.

Second, __gpu_buddy_free() coalesces adjacent clear and dirty free buddies
into a single GPU_BUDDY_FREE block with state GPU_BLOCK_MIXED, which has
gpu_buddy_block_is_clear(block) == false and block->has_clear == true:

1. For clear range allocations (flags & GPU_BUDDY_CLEAR_ALLOCATION), Pass 1
   sees gpu_buddy_block_is_clear(block) == false on a GPU_BLOCK_MIXED block
   and skips it via block_incompatible() without splitting it to reach the
   clear sub-blocks inside. Pass 2 then splits the mixed block without clear
   child-steering and unconditionally visits block->right first, returning a
   dirty block whenever the clear half is in block->left.

2. For dirty range allocations (!(flags & GPU_BUDDY_CLEAR_ALLOCATION)),
   block_incompatible() returns false on a higher-order GPU_BLOCK_MIXED
   block because gpu_buddy_block_is_clear(block) is false. Pass 1 then calls
   split_block(mm, block), which reclassifies block->left and block->right
   via mark_free(). If the child overlapping [start, end) is clear while the
   dirty child lies outside [start, end), Pass 1 rejects the clear child via
   block_incompatible() and skips the other child without calling
   __gpu_buddy_undo_splits().

Wouldn't leaving that split GPU_BLOCK_MIXED block unmerged keep both
children in GPU_BUDDY_FREE state, preventing ancestor coalescing on teardown
and triggering gpu_buddy_assert(gpu_buddy_block_is_free(mm->roots[i])) while
leaking the split child gpu_buddy_block structures in gpu_buddy_fini()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020055537.1640552-1-arunpravin.paneerselvam@amd.com?part=1