Subject: Re: [PATCH 1/2] drm/xe: count UMA shrinkable BOs as reclaimable without swap I'm withdrawing this patch , approach is wrong Signed-off-by: aquiles <[email protected]>
On Fri, Jun 26, 2026 at 11:40 AM <[email protected]> wrote: > Thank you for your contribution! Sashiko AI review found 2 potential > issue(s) to consider: > - [High] xe_shrinker_count() reports shrinkable UMA BOs regardless of > __GFP_FS, but xe_shrinker_scan() bails out without scanning them when > __GFP_FS is missing. > - [High] Bypassing ttm_backup_bytes_avail() for UMA shrinkable BOs falsely > assumes they can be freed without swap space, causing useless CPU overhead > yielding zero net memory reclamation. > -- > > commit eb93ae097c20ae5f357e41557ba0c3d785cb142c > Author: aquiles <[email protected]> > > drm/xe: count UMA shrinkable BOs as reclaimable without swap > > This commit modifies the xe shrinker count logic to unconditionally report > UMA shrinkable buffer objects as reclaimable, even without swap space. It > subtracts UMA shrinkable pages from the ttm backup limit check and adds > them back unconditionally. > > > diff --git a/drivers/gpu/drm/xe/xe_shrinker.c > b/drivers/gpu/drm/xe/xe_shrinker.c > > index 83374cd576608..3ca2de342dcd6 100644 > > --- a/drivers/gpu/drm/xe/xe_shrinker.c > > +++ b/drivers/gpu/drm/xe/xe_shrinker.c > [ ... ] > > @@ -140,16 +140,22 @@ xe_shrinker_count(struct shrinker *shrink, struct > shrink_control *sc) > > { > > struct xe_shrinker *shrinker = to_xe_shrinker(shrink); > > unsigned long num_pages; > > + long uma_reclaimable_pages; > > bool can_backup = !!(sc->gfp_mask & __GFP_FS); > > + bool is_uma = !IS_DGFX(shrinker->xe); > > > > num_pages = ttm_backup_bytes_avail() >> PAGE_SHIFT; > > read_lock(&shrinker->lock); > > > > + uma_reclaimable_pages = is_uma ? shrinker->shrinkable_pages : 0; > > + > > if (can_backup) > > - num_pages = min_t(unsigned long, num_pages, > shrinker->shrinkable_pages); > > + num_pages = min_t(unsigned long, num_pages, > > + shrinker->shrinkable_pages - > uma_reclaimable_pages); > > [Severity: High] > Does bypassing the ttm_backup_bytes_avail() limit for UMA shrinkable BOs > result in zero net memory reclamation without swap? > > On UMA systems without swap, the shrinker uses ttm_bo_shrink() which > backs up the BO by copying its pages to a shmem file. Because no swap space > is available, the newly allocated shmem backup pages cannot be swapped to > disk and remain permanently pinned in physical memory. > > This deceives the core MM into trying to reclaim these pages, burning CPU > cycles on copying memory that yields zero net free physical memory. > > > else > > num_pages = 0; > > > > + num_pages += uma_reclaimable_pages; > > [Severity: High] > Can unconditionally returning uma_reclaimable_pages cause an infinite > reclaim retry loop when __GFP_FS is missing? > > When the shrinker is invoked with __GFP_FS missing (e.g., direct reclaim in > the filesystem or block layer), can_backup is false, but this code adds > uma_reclaimable_pages to the returned count anyway. > > Looking at xe_shrinker_scan() in the same file, the scan loop explicitly > bails out if !can_backup before it ever attempts to scan non-purgeable > (shrinkable) pages: > > if (nr_scanned >= nr_to_scan || !can_backup) > goto out; > > /* ... */ > shrink_flags.purge = false; > > The shrinker API strictly requires the count to accurately reflect only the > objects that the scan function will attempt to free under the given > gfp_mask. > Returning a falsely high count for objects that the scan function > explicitly > skips breaks the VM reclaim feedback loop, causing infinite reclaim loops. > > > num_pages += shrinker->purgeable_pages; > > read_unlock(&shrinker->lock); > > > > -- > Sashiko AI review · > https://sashiko.dev/#/patchset/[email protected]?part=1 >
