Re: [PATCH v2 2/2] drm/xe: Update shrinker batch size based on average BO size

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

On Thu, 2026-09-10 at 12:54 +0200, Thomas Hellström wrote:
> On Tue, 2026-08-18 at 14:32 -0700, Matthew Brost wrote:
> > On Tue, Aug 18, 2026 at 02:10:35PM +0200, Thomas Hellström wrote:
> > > On Fri, 2026-08-14 at 16:24 -0700, Matthew Brost wrote:
> > > > On Fri, Aug 14, 2026 at 04:37:37PM +0200, Thomas Hellström
> > > > wrote:
> > > > > Update our preferred vmscan batch size on each count pass to
> > > > > avoid
> > > > > invoking scan_objects for requests too small to free even a
> > > > > single
> > > > > average-sized GEM object. Our rough estimate for an effective
> > > > > batch
> > > > > is twice the average number of pages per populated ttm_tt
> > > > > across
> > > > > all
> > > > > shrinkable and purgeable objects. The factor of two provides
> > > > > headroom
> > > > > so that most scan invocations can free at least one GEM
> > > > > object
> > > > > despite
> > > > > variability in object sizes.
> > > > > 
> > > > > The batch value is updated as an exponential moving average,
> > > > > (old_batch + avg) / 2, to smooth out sudden changes in the
> > > > > object population. It is floored at 128 pages, the kernel
> > > > > default
> > > > > SHRINK_BATCH, to ensure the shrinker remains responsive when
> > > > > there
> > > > > are very few objects.
> > > > > 
> > > > > The populated_tts counter introduced in the previous commit
> > > > > provides
> > > > > the object count needed for the average. We inherit the same
> > > > > justification as the analogous mechanism in i915: shrinking a
> > > > > GEM
> > > > > object has non-trivial locking overhead, so firing the
> > > > > shrinker
> > > > > for
> > > > > requests smaller than a single object is wasteful.
> > > > > 
> > > > > v2:
> > > > > - Fix the average object size estimate to account for the
> > > > > full
> > > > >   shrinkable and purgeable population.
> > > > > 
> > > > > Assisted-by: GitHub_Copilot:claude-sonnet-4.6
> > > > > Assisted-by: GitHub_Copilot:claude-sonnet-5
> > > > 
> > > > This is probably the right direction given what we currently
> > > > have
> > > > in
> > > > terms of shrinker control, but the core heuristic is still a
> > > > pretty
> > > > poor
> > > > one. My understanding is that it combines batch and seek values
> > > > using
> > > > some odd math to determine whether a scan is worthwhile at a
> > > > given
> > > > priority level. We probably want to avoid shrinking at the
> > > > initial
> > > > scan
> > > > priorities, and I believe this change accomplishes that.
> > > 
> > > Yes, but I think that's a side-effect not to be fully relied
> > > upon.
> > > 
> > > The meaning of this value IMO is to tell the core how many
> > > objects
> > > to
> > > expect for a scan request, so that the core can hold off
> > > shrinking
> > > until that many objects is actually this shrinker's fair share of
> > > its
> > > available objects. So the side effect would be that this
> > > shrinker's
> > > fair share of shrinking may not trigger a scan request if
> > > shrinking
> > > is
> > > triggered by compacting?
> > > 
> > 
> > I think you mean higher order allocations, not compaction. Reclaim
> > is
> > the input to compaction - see compaction_ready, compact_gap usage
> > in
> > vmscan.c
> 
> Yes, I meant shrinking triggered by higher order allocations, but
> used
> compaction as a term for shrinking any order page in order to be able
> to coalesce memory into higher order. That might not be the correct
> terminology, though.
> 
> > 
> > So I think a side affect could be higher order allocation never
> > enter
> > our shrinker if compaction_ready flips to true before our batch
> > size
> > /
> > seek values are asked for (total_scan math in do_shrink_slab).
> 
> Yes, that's a possible side-effect.
> 
> > 
> > > > 
> > > > That said, I think we really want two shrinkers instead: one
> > > > with
> > > > the
> > > > default settings (or perhaps even a reduced seek value) for
> > > > purgeable
> > > > BOs, and another for BOs that we legitimately need to back up.
> > > > The
> > > > purgeable one should be favored to run eariler, likewise the
> > > > TTM
> > > > pool
> > > > shrinker should be favored run before our shrinker too.
> > > 
> > > I don't think we can or should use the batch size to decide which
> > > shrinker should be prioritized. IIRC one of the comments to
> > > previous
> > 
> > It probably isn't the right approach, but my concern is that our
> > shrinker
> > won't run at higher orders when there are cheap reclaimable pages
> > (i.e.,
> > we have purged BOs that can immediately make higher-order pages
> > available
> > or allow compaction to do its job of forming higher-order pages). I
> > have
> > already seen shrinker backoff being too aggressive when
> > compaction_ready()
> > returns true, resulting in virtually zero THP availability because
> > shrinkers hold onto enough non-movable pages scattered throughout
> > memory
> > to prevent successful compaction (I have a local core MM patch that
> > fixes
> > this issue).
> > 
> > Purgable and non-purgable pages have fundamentally different
> > shrinking
> > costs, and that distinction needs to be expressed somehow. The
> > opportunistic compaction (wrongly named) shrinker series attempts
> > to
> > capture this.
> 
> But since the core attempts to be fair poking shrinkers, and that's
> not
> really what we want (we want it to shrink purgeable stuff first, and
> avoid shrinking non-purgeable stuff). 

Actually with separate shrinkers we can modify the count to handle
this. I'll take a look at that.

/Thomas