Re: [PATCH v4 4/4] drm/xe: Update non-purgeable shrinker batch size based on average BO size

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Fri, 2026-10-16 at 13:45 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [Medium] drm/xe: shrinker count callback serializes direct reclaim
> via write_lock
> --
> 
> commit a78bd8842e67f957a2ced6d827e1c4a6430ff29b
> Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> drm/xe: Update non-purgeable shrinker batch size based on average BO
> size
> 
> This commit updates the preferred vmscan batch size for the non-
> purgeable 
> shrinker based on the average buffer object size. It introduces an 
> exponential moving average that is updated during the shrinker count
> phase.
> 
> > diff --git a/drivers/gpu/drm/xe/xe_shrinker.c
> > b/drivers/gpu/drm/xe/xe_shrinker.c
> > index 8e3fe675e0f34..955908b3bdf3e 100644
> > --- a/drivers/gpu/drm/xe/xe_shrinker.c
> > +++ b/drivers/gpu/drm/xe/xe_shrinker.c
> [ ... ]
> > @@ -210,15 +235,26 @@ xe_shrinker_count(struct shrinker *shrink,
> > struct shrink_control *sc)
> >  	unsigned long num_pages;
> >  	bool can_backup = !!(sc->gfp_mask & __GFP_FS);
> >  
> > +	if (!can_backup)
> > +		return SHRINK_EMPTY;
> > +
> >  	num_pages = ttm_backup_bytes_avail() >> PAGE_SHIFT;
> > -	read_lock(&shrinker->lock);
> > +	/* Consistently use write_lock() for simplicity. */
> > +	write_lock(&shrinker->lock);
> 
> [Severity: Medium]
> Will this exclusive write lock create a system-wide performance
> bottleneck
> during memory reclaim?
> 
> The core mm subsystem frequently calls the shrinker's count_objects
> callback
> (xe_shrinker_count) from do_shrink_slab() during direct reclaim. This
> occurs
> concurrently across multiple allocating tasks and CPUs when under
> memory
> pressure.
> 
> By changing read_lock() to write_lock() to update the batch_avg
> heuristic,
> this forces all concurrent direct reclaimers system-wide to serialize
> on this
> single lock, which could lead to severe lock contention and
> performance
> degradation.

This is grossly overstated. False positive.

/Thomas



> 
> >  
> > -	if (can_backup)
> > -		num_pages = min_t(unsigned long, num_pages,
> > shrinker->shrinkable_pages);
> > -	else
> > -		num_pages = 0;
> > +	num_pages = min_t(unsigned long, num_pages, shrinker-
> > >shrinkable_pages);
> >  
> > -	read_unlock(&shrinker->lock);
> > +	if (shrinker->populated_tts) {
> > +		unsigned long avg = 2 * (shrinker-
> > >shrinkable_pages +
> > +					 shrinker-
> > >purgeable_pages) /
> > +				     shrinker->populated_tts;
> > +
> > +		shrinker->batch_avg = max((shrinker->batch_avg +
> > (long)avg) >> 1,
> > +					  128L /* default
> > SHRINK_BATCH */);
> > +		WRITE_ONCE(shrink->batch, shrinker->batch_avg);
> > +	}
> > +
> > +	write_unlock(&shrinker->lock);
> >  
> >  	return num_pages ? num_pages : SHRINK_EMPTY;
> >  }