Re: [PATCH v5 1/4] drm/xe: Don't abort the shrinker walk on -EBUSY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: inflated scanned count and unbounded walk on -EBUSY in xe_bo_shrink()
--

commit 34f5fe95935fcbebfbfa95c987f4c6fb159c03dd
Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>

drm/xe: Don't abort the shrinker walk on -EBUSY

This commit updates __xe_shrinker_walk() to continue the LRU walk when
xe_bo_shrink() returns -EBUSY so that a single unshrinkable buffer object
does not abort reclaim for objects behind it on the LRU.

> diff --git a/drivers/gpu/drm/xe/xe_shrinker.c b/drivers/gpu/drm/xe/xe_shrinker.c
> index deb4378c1ec15..4e4da76936f49 100644
> --- a/drivers/gpu/drm/xe/xe_shrinker.c
> +++ b/drivers/gpu/drm/xe/xe_shrinker.c
> @@ -113,6 +113,9 @@ static int __xe_shrinker_walk(struct xe_shrinker *shrinker,
>  				continue;
>  
>  			lret = xe_bo_shrink(ctx, ttm_bo, flags, scanned);
> +			/* Like the TTM LRU walk, skip bos we can't shrink now. */
> +			if (lret == -EBUSY)
> +				continue;
>  			if (lret < 0) {
>  				ret = lret;
>  				goto out;

[Severity: Medium]
What happens to *scanned and the walk termination when xe_bo_shrink() (or
xe_bo_shrink_purge()) fails inside ttm_bo_shrink() with -EBUSY?

In both xe_bo_shrink() and xe_bo_shrink_purge() in
drivers/gpu/drm/xe/xe_bo.c, *scanned is incremented by the BO's page count
before calling ttm_bo_shrink():

drivers/gpu/drm/xe/xe_bo.c:xe_bo_shrink() {
	...
	*scanned += tt->num_pages;
	lret = ttm_bo_shrink(ctx, bo, (struct ttm_bo_shrink_flags)
			     {.purge = false,
			      .writeback = flags.writeback,
			      .allow_move = true});
}

drivers/gpu/drm/xe/xe_bo.c:xe_bo_shrink_purge() {
	...
	*scanned += bo->ttm->num_pages;
	lret = ttm_bo_shrink(ctx, bo, (struct ttm_bo_shrink_flags)
			     {.purge = true,
			      .writeback = false,
			      .allow_move = false});
}

If ttm_bo_shrink() fails in ttm_bo_validate(), times out (or hits
no_wait_gpu) in ttm_bo_wait_ctx(), or fails in ttm_tt_backup() (for
example, -ENOMEM when writeback is false or -EBUSY when swap is full), it
returns -EBUSY after *scanned has already been incremented.

Could this cause two issues?

First, in __xe_shrinker_walk(), continuing on -EBUSY skips the
*scanned >= to_scan check:

drivers/gpu/drm/xe/xe_shrinker.c:__xe_shrinker_walk() {
	...
			lret = xe_bo_shrink(ctx, ttm_bo, flags, scanned);
			/* Like the TTM LRU walk, skip bos we can't shrink now. */
			if (lret == -EBUSY)
				continue;
			if (lret < 0) {
				ret = lret;
				goto out;
			}

			*freed += lret;
			if (*scanned >= to_scan)
				goto out;
}

When multiple BOs fail ttm_bo_shrink() with -EBUSY (such as during the
first two passes where writeback is false, or when swap space is full),
__xe_shrinker_walk() will walk the entire XE_PL_SYSTEM and XE_PL_TT LRUs
without bound, moving XE_PL_TT BOs to XE_PL_SYSTEM and unmapping them
along the way while inflating sc->nr_scanned and draining do_shrink_slab()
scan budgets.

Second, if a single BO of size >= to_scan fails ttm_bo_shrink() with
-EBUSY, *scanned already reaches to_scan with 0 pages freed. As soon as a
subsequent BO returns >= 0 (or after the walk returns), the
*scanned >= to_scan checks in __xe_shrinker_walk(), xe_shrinker_walk(),
and xe_shrinker_scan() will still trigger and skip the remaining shrinker
passes (the GPU-wait pass, the writeback pass, and the non-purgeable
pass):

drivers/gpu/drm/xe/xe_shrinker.c:xe_shrinker_walk() {
	...
	ret = __xe_shrinker_walk(shrinker, ctx, save_flags, to_scan, scanned,
				 freed);
	swap(no_wait_gpu, ctx->no_wait_gpu);
	if (ret || *scanned >= to_scan)
		return ret;

	if (!ctx->no_wait_gpu) {
		ret = __xe_shrinker_walk(shrinker, ctx, save_flags, to_scan, scanned,
					 freed);
		if (ret || *scanned >= to_scan)
			return ret;
	}
}

Should *scanned only be incremented when ttm_bo_shrink() succeeds, or how
should *scanned be accounted when ttm_bo_shrink() returns -EBUSY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019095655.3501-1-thomas.hellstrom@linux.intel.com?part=1