Re: [PATCH v5 1/2] drm/xe/shrinker: Do not discard freed pages on error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Returning `freed ? freed : lret` on error masks the error code if pages were freed, causing the shrinker to inconsistently escalate to heavier, blocking passes based on LRU object ordering.

Pre-existing issues:
- [High] The shrinker LRU walk aborts entirely upon encountering a single busy or unshrinkable buffer object, rendering the reclaim process highly ineffective and prone to starvation.
--

commit 79c697e63c32fe35a147fb885eb857cd09a270e8
Author: Shuicheng Lin <shuicheng.lin@intel.com>
drm/xe/shrinker: Do not discard freed pages on error

The patch updates the drm/xe shrinker to report the number of pages freed 
when an error is encountered during the walk, instead of immediately 
returning the error code. This aims to prevent the shrinker from incorrectly 
telling mm that it scanned without freeing any pages.

> diff --git a/drivers/gpu/drm/xe/xe_shrinker.c b/drivers/gpu/drm/xe/xe_shrinker.c
> --- a/drivers/gpu/drm/xe/xe_shrinker.c
> +++ b/drivers/gpu/drm/xe/xe_shrinker.c
> @@ -80,7 +80,7 @@ static s64 __xe_shrinker_walk(struct xe_device *xe,
>  
>  			lret = xe_bo_shrink(ctx, ttm_bo, flags, scanned);
>  			if (lret < 0)
> -				return lret;
> +				return freed ? freed : lret;

[Severity: High]
This isn't a bug introduced by this patch, but doesn't aborting the LRU 
iteration entirely upon encountering an error cause the shrinker to starve?

If xe_bo_shrink() returns a benign error like -EBUSY on an unshrinkable 
buffer object, the loop aborts and the unshrinkable buffer object remains 
at the front of the LRU. Subsequent shrinker scans will repeatedly hit this 
same buffer object and abort again under memory pressure, effectively 
preventing the shrinker from reaching other eligible objects further down 
the list.

Could __xe_shrinker_walk() handle benign errors like -EBUSY by using 
continue instead of returning?

[ ... ]

> @@ -119,7 +119,7 @@ static s64 xe_shrinker_walk(struct xe_device *xe,
>  	if (!ctx->no_wait_gpu) {
>  		lret = __xe_shrinker_walk(xe, ctx, save_flags, to_scan, scanned);
>  		if (lret < 0)
> -			return lret;
> +			return freed ? freed : lret;

[Severity: Medium]
Does masking the inner walk error disrupt the shrinker's escalation logic?

By returning a positive value when freed > 0 despite hitting an internal 
error, xe_shrinker_walk() interprets the result as a pure success. Since 
*scanned < to_scan in this scenario, this causes the shrinker to 
unpredictably escalate to heavier, blocking passes (like waiting for the 
GPU or writebacks). 

Conversely, if the exact same error is encountered before any pages are 
freed (freed == 0), the function returns a negative error and correctly 
skips the heavier phases. 

Does this create inconsistent reclaim behavior based entirely on whether 
the error happened before or after freeing at least one page?

>  		freed += lret;
>  		if (*scanned >= to_scan)
>  			return freed;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901173059.455469-1-shuicheng.lin@intel.com?part=1