Re: [PATCH v3 2/3] drm/xe: Split the bo shrinker into purgeable and non-purgeable shrinkers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe: split shrinkers fail to isolate purgeable objects
- [High] drm/xe: purgeable shrinker permanently stalls on non-purgeable LRU objects
--

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

    drm/xe: Split the bo shrinker into purgeable and non-purgeable shrinkers
    
    Purgeable objects are reclaimed by simply discarding their pages, with
    no writeback required. They are therefore much cheaper to shrink than
    regular shrinkable (non-purgeable) objects, whose reclaim may need to
    wait for the GPU or write dirty compressed contents back to a backup
    store, and whose contents need to be read back on next use.
    
    Split the single xe_shrinker into two independent struct shrinker
    instances registered against the shrinker core, sharing the same
    underlying accounting (shrinkable_pages, purgeable_pages,
    populated_tts) and locking:

[ ... ]

> --- a/drivers/gpu/drm/xe/xe_shrinker.c
> +++ b/drivers/gpu/drm/xe/xe_shrinker.c
> @@ -213,63 +235,52 @@ static bool xe_shrinker_runtime_pm_get(struct xe_shrinker *shrinker, bool force,

[ ... ]

> -static unsigned long xe_shrinker_scan(struct shrinker *shrink, struct shrink_control *sc)
> +static unsigned long xe_shrinker_scan_purgeable(struct shrinker *shrink,
> +						struct shrink_control *sc)
>  {
>  	struct xe_shrinker *shrinker = to_xe_shrinker(shrink);
>  	struct ttm_operation_ctx ctx = {
>  		.interruptible = false,
>  		.no_wait_gpu = ttm_bo_shrink_avoid_wait(),
>  	};
> -	unsigned long nr_to_scan, nr_scanned = 0, freed = 0;
> +	unsigned long nr_to_scan = sc->nr_to_scan, nr_scanned = 0, freed = 0;
>  	struct xe_bo_shrink_flags shrink_flags = {
>  		.purge = true,
> -		/* Don't request writeback without __GFP_IO. */
> -		.writeback = !ctx.no_wait_gpu && (sc->gfp_mask & __GFP_IO),
>  	};
> -	bool runtime_pm;
> -	bool purgeable;
> -	bool can_backup = !!(sc->gfp_mask & __GFP_FS);
> -
> -	nr_to_scan = sc->nr_to_scan;
> -
> -	read_lock(&shrinker->lock);
> -	purgeable = !!shrinker->purgeable_pages;
> -	read_unlock(&shrinker->lock);
>  
> -	/* Might need runtime PM. Try to wake early if it looks like it. */
> -	runtime_pm = xe_shrinker_runtime_pm_get(shrinker, false, nr_to_scan, can_backup);
> -
> -	if (purgeable && nr_scanned < nr_to_scan)
> -		xe_shrinker_walk(shrinker, &ctx, shrink_flags,
> -				 nr_to_scan, &nr_scanned, &freed);
> +	xe_shrinker_walk(shrinker, &ctx, shrink_flags, nr_to_scan, &nr_scanned, &freed);

[Severity: High]
Does this standalone purgeable shrinker permanently stall when it encounters
non-purgeable LRU objects?

When the core mm invokes scan_objects for the purgeable shrinker, it
traverses the TTM LRU list which mixes both purgeable and non-purgeable
objects. If a non-purgeable object is encountered, xe_bo_shrink returns
-EBUSY:

drivers/gpu/drm/xe/xe_bo.c:xe_bo_shrink() {
    ...
	if (!(tt->page_flags & TTM_TT_FLAG_EXTERNAL_MAPPABLE) ||
	    (flags.purge && !xe_tt->purgeable))
		return -EBUSY;
    ...
}

The LRU walk in __xe_shrinker_walk treats this negative return as a fatal
error and aborts the entire scan:

drivers/gpu/drm/xe/xe_shrinker.c:__xe_shrinker_walk() {
    ...
			lret = xe_bo_shrink(ctx, ttm_bo, flags, scanned);
			if (lret < 0) {
				ret = lret;
				goto out;
			}
    ...
}

Because the shrinkers are now decoupled, this shrinker can no longer rely
on the non-purgeable shrinker running immediately afterwards in the same
context to clear the obstruction. Any memory pressure triggering the shrinker
when a non-purgeable object happens to be at the LRU head might cause this
to abort without freeing any pages.

>  
>  	sc->nr_scanned = nr_scanned;
> -	if (nr_scanned >= nr_to_scan || !can_backup)
> -		goto out;
>  
> -	/* If we didn't wake before, try to do it now if needed. */
> -	if (!runtime_pm)
> -		runtime_pm = xe_shrinker_runtime_pm_get(shrinker, true, 0, can_backup);
> +	return nr_scanned ? freed : SHRINK_STOP;
> +}
>  
> -	shrink_flags.purge = false;
> +static unsigned long xe_shrinker_scan(struct shrinker *shrink, struct shrink_control *sc)
> +{
> +	struct xe_shrinker *shrinker = to_xe_shrinker(shrink);
> +	struct ttm_operation_ctx ctx = {
> +		.interruptible = false,
> +		.no_wait_gpu = ttm_bo_shrink_avoid_wait(),
> +	};
> +	unsigned long nr_to_scan = sc->nr_to_scan, nr_scanned = 0, freed = 0;
> +	struct xe_bo_shrink_flags shrink_flags = {
> +		.purge = false,
> +		/* Don't request writeback without __GFP_IO. */
> +		.writeback = !ctx.no_wait_gpu && (sc->gfp_mask & __GFP_IO),
> +	};

[Severity: High]
Does the non-purgeable shrinker fail to isolate purgeable objects?

Since this shrinker scans with flags.purge = false, xe_bo_shrink does not
skip purgeable objects:

drivers/gpu/drm/xe/xe_bo.c:xe_bo_shrink() {
    ...
	if (xe_tt->purgeable) {
		if (bo->resource->mem_type != XE_PL_SYSTEM)
			lret = xe_bo_move_notify(xe_bo, ctx);
		if (!lret)
			lret = xe_bo_shrink_purge(ctx, bo, scanned);
    ...
}

Will this cause the non-purgeable shrinker, which has a higher scan target
due to higher seeks, to fulfill its scan quota by stealing cheap purgeable
objects instead of forcing writeback on non-purgeable objects? If so, the
proportional reclaim mechanism might be defeated.

> +	bool runtime_pm;
> +
> +	/* Might need runtime PM to access backed-up or CCS-compressed data. */
> +	runtime_pm = xe_shrinker_runtime_pm_get(shrinker);
>  
> -	xe_shrinker_walk(shrinker, &ctx, shrink_flags,
> -			 nr_to_scan, &nr_scanned, &freed);
> +	xe_shrinker_walk(shrinker, &ctx, shrink_flags, nr_to_scan, &nr_scanned, &freed);
>  
>  	sc->nr_scanned = nr_scanned;
> -out:
>  	xe_shrinker_runtime_pm_put(shrinker, runtime_pm);
> +
>  	return nr_scanned ? freed : SHRINK_STOP;
>  }

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