Re: [PATCH v3 3/3] drm/xe: Add a minimum age gate to the shrinker

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Oct 20, 2026 at 07:36:05PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] drm/xe: xe_bo_bump_lru_locked() and xe_bo_shrink_too_young() ignore bulk_move
> - [High] drm/xe: xe_bo_move() bumps LRU and stamps VM on VRAM-to-TT evictions
> - [Medium] drm/xe: xe_bo_init_locked() bumps LRU before setting bo->vm and bulk_move
> - [Medium] drm/xe: xe_bo_shrink_backup_candidate() includes deleted and ghost BOs
> --
> 
> commit 7080502f8c1d2ffa8fd84f4e7f1ff61a34a0e8c9
> Author: Matthew Brost <matthew.brost@intel.com>
> 
> drm/xe: Add a minimum age gate to the shrinker
> 
> This commit adds a per-device shrinker_min_age_ms sysfs attribute and Kconfig
> default to prevent the shrinker from backing up buffer objects that were
> recently moved into TT or bumped on the LRU during exec. It timestamps BOs and
> VMs on LRU bumps and skips LRU priorities whose first shrinkable backup
> candidate is younger than the configured minimum age.
> 
> > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > index bc4801c48fce5..fc8fe68ca0373 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > @@ -1359,9 +1359,114 @@ static int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
> >  		xe_tt_unmap_sg(xe, ttm_bo->ttm);
> >  	}
> >  
> > +	if (!ret && ttm_bo->resource && ttm_bo->resource->mem_type == XE_PL_TT)
> > +		xe_bo_bump_lru(bo);
> 
> [Severity: High]
> Should xe_bo_move() avoid calling xe_bo_bump_lru() when evict is true or when
> ttm_bo->resource->placement has TTM_PL_FLAG_TEMPORARY set?
> 
> Under VRAM pressure on discrete GPUs, ttm_bo_evict() calls xe_evict_flags(),
> which selects tt_placement (XE_PL_TT) for BOs in VRAM or stolen memory:
> 
> drivers/gpu/drm/xe/xe_bo.c:xe_evict_flags() {
> 	...
> 	switch (tbo->resource->mem_type) {
> 	case XE_PL_VRAM0:
> 	case XE_PL_VRAM1:
> 	case XE_PL_STOLEN:
> 		*placement = tt_placement;
> 		break;
> 	...
> }
> 
> A multihop move in xe_bo_move() can also bounce a BO through a temporary
> XE_PL_TT resource with TTM_PL_FLAG_TEMPORARY set.
> 
> When a single cold VM-private BO is evicted from XE_PL_VRAM0 or XE_PL_VRAM1
> into XE_PL_TT, calling xe_bo_bump_lru() -> xe_bo_bump_lru_locked() updates
> bo->lru_stamp and vm->lru_stamp to jiffies and calls
> ttm_lru_bulk_move_tail(&vm->lru_bulk_move). Because ttm_lru_bulk_move_tail()
> moves the VM's bulk-move ranges across all TTM_NUM_MEM_TYPES and advances
> active eviction cursors via ttm_bulk_move_adjust_cursors(), evicting one BO
> from VRAM moves all remaining cold VRAM BOs of that VM to the tail of the
> VRAM LRU so hotter VMs are evicted instead. It also stamps the evicted BO and
> all XE_PL_TT BOs of that idle VM as freshly used, protecting them from the
> shrinker for min_age_ms.
> 

This is correct. Will fix.

> > +
> >  	return ret;
> >  }
> >  
> > +/**
> > + * xe_bo_bump_lru_locked() - Bump a BO on the LRU and stamp its last use
> > + * @bo: The buffer object, reserved by the caller.
> > + *
> > + * Move @bo to the tail of its LRU and record the time for the shrinker's
> > + * minimum age gate. A BO private to a VM is anchored inside that VM's
> > + * bulk-move range, so moving it alone would only reorder it within the
> > + * range, leaving it wherever the range sits on the LRU. Move the whole
> > + * range to the tail and stamp the VM as well, matching xe_vm_bump_lru().
> > + * The caller must hold the device LRU lock.
> > + */
> > +void xe_bo_bump_lru_locked(struct xe_bo *bo)
> > +{
> > +	struct xe_vm *vm = bo->vm;
> > +
> > +	if (vm) {
> > +		WRITE_ONCE(vm->lru_stamp, jiffies);
> > +		ttm_lru_bulk_move_tail(&vm->lru_bulk_move);
> > +	}
> > +
> > +	WRITE_ONCE(bo->lru_stamp, jiffies);
> > +	ttm_bo_move_to_lru_tail(&bo->ttm);
> > +}
> 
> [Severity: Medium]
> What happens when a VM-private user BO is initially created in XE_PL_TT via
> __xe_bo_create_locked() -> xe_bo_init_locked() -> ttm_bo_init_reserved() ->
> ttm_bo_validate() -> xe_bo_move()?
> 
> In xe_bo_init_locked(), ttm_bo_init_reserved() runs before
> ttm_bo_set_bulk_move():
> 
> drivers/gpu/drm/xe/xe_bo.c:xe_bo_init_locked() {
> 	...
> 	err = ttm_bo_init_reserved(&xe->ttm, &bo->ttm, type,
> 				   placement, alignment,
> 				   &ctx, NULL, resv, xe_ttm_bo_destroy);
> 	...
> 	bo->created = true;
> 	if (bulk)
> 		ttm_bo_set_bulk_move(&bo->ttm, bulk);
> 	else
> 		ttm_bo_move_to_lru_tail_unlocked(&bo->ttm);
> 
> 	return bo;
> }
> 
> And __xe_bo_create_locked() only sets bo->vm = vm after xe_bo_init_locked()
> returns.
> 
> When xe_bo_move() calls xe_bo_bump_lru() during ttm_bo_init_reserved(), both
> bo->vm and bo->ttm.bulk_move are still NULL, so xe_bo_bump_lru_locked() only
> stamps bo->lru_stamp and moves the new BO to the LRU tail without updating
> vm->lru_stamp or calling ttm_lru_bulk_move_tail(&vm->lru_bulk_move).
> 
> Immediately afterward in xe_bo_init_locked(), ttm_bo_set_bulk_move() calls
> ttm_lru_bulk_move_add() -> ttm_lru_bulk_move_pos_tail(), which executes
> list_move(&res->lru.link, &pos->last->lru.link) if the VM already has BOs in
> (XE_PL_TT, XE_BO_PRIORITY_NORMAL). That pulls the newly stamped young BO
> backward from the LRU tail to the end of the VM's existing bulk-move range.
> If that range sits near the head of the XE_PL_TT LRU ahead of older BOs from
> other VMs, the shrinker can encounter the young BO near the head of the list
> and call ttm_bo_lru_cursor_skip_priority(), skipping older cold BOs behind
> the VM's range.

I think it is temporarily not in the bulk-move list, but as soon as
xe_bo_init_locked() is called, it is moved into the bulk-move list. The
BO should end up at the tail of the bulk-move list after
ttm_bo_set_bulk_move(), so this is a non-issue. 

> 
> [ ... ]
> 
> > +/**
> > + * xe_bo_shrink_backup_candidate() - Whether the shrinker may back up a BO
> > + * @bo: The TTM buffer object, reserved by the caller.
> > + *
> > + * A BO is a backup candidate if backing it up is the only way to reclaim it.
> > + * BOs advised DONTNEED, which sit at XE_BO_PRIORITY_LOW, and purgeable ones
> > + * are not: their pages are simply dropped, which is cheap, and they are
> > + * accounted to the purgeable shrinker. Neither are BOs whose tt isn't
> > + * TTM_TT_FLAG_EXTERNAL_MAPPABLE, which xe_bo_shrink() rejects outright.
> > + *
> > + * Return: true if @bo would be backed up by a non-purge shrink pass.
> > + */
> > +bool xe_bo_shrink_backup_candidate(struct ttm_buffer_object *bo)
> > +{
> > +	struct xe_ttm_tt *xe_tt;
> > +
> > +	if (!bo->ttm || !(bo->ttm->page_flags & TTM_TT_FLAG_EXTERNAL_MAPPABLE) ||
> > +	    bo->priority <= XE_BO_PRIORITY_LOW)
> > +		return false;
> > +
> > +	xe_tt = container_of(bo->ttm, struct xe_ttm_tt, ttm);
> > +
> > +	return !xe_tt->purgeable;
> > +}
> 
> [Severity: Medium]
> Could xe_bo_shrink_backup_candidate() misclassify non-xe ghost BOs or deleted
> BOs awaiting delayed_delete that xe_bo_shrink() purges rather than backs up?
> 
> In xe_bo_shrink(), both non-xe BOs and xe_bo objects whose GEM refcount has
> reached zero are purged via xe_bo_shrink_purge():
> 
> drivers/gpu/drm/xe/xe_bo.c:xe_bo_shrink() {
> 	...
> 	if (!xe_bo_is_xe_bo(bo) || !xe_bo_get_unless_zero(xe_bo))
> 		return xe_bo_shrink_purge(ctx, bo, scanned);
> 	...
> }
> 
> When a recently stamped user BO is freed while busy (or with
> want_init_on_free() enabled) via xe_gem_object_free() -> ttm_bo_fini() ->
> ttm_bo_release(), its GEM refcount is zero and bo->deleted is true while it
> remains on man->lru[XE_BO_PRIORITY_NORMAL] awaiting delayed_delete. Once its
> fences signal before the delayed_delete worker runs, ttm_bo_shrink_suitable()
> returns true. Because neither xe_bo_shrink_backup_candidate() nor
> xe_bo_shrink_too_young() checks bo->deleted or kref_read(&bo->base.refcount),
> xe_bo_shrink_too_young() treats the freed BO as a young backup candidate,
> preventing __xe_shrinker_walk() from purging it and causing both
> __xe_shrinker_walk() and xe_shrinker_lru_young() to skip the rest of the
> priority via ttm_bo_lru_cursor_skip_priority().
> 
> Additionally, because xe_bo_shrink_backup_candidate() does not check
> xe_bo_is_xe_bo(bo), if xe_shrinker_lru_young() encounters a non-xe TTM ghost
> BO at the head of a priority list, it treats the ghost BO as that priority's
> first backup candidate and immediately returns false (since
> xe_bo_shrink_too_young() returns false for !xe_bo_is_xe_bo(bo)) without
> inspecting the first real backup candidate in that priority.
> 

xe_bo_shrink_backup_candidate should likely check !xe_bo_is_xe_bo ||
!kref_read(xe_bo) and return false for both.

> > +
> > +/**
> > + * xe_bo_shrink_too_young() - Whether a BO was used too recently to shrink
> > + * @bo: The TTM buffer object, reserved by the caller.
> > + * @min_age: Minimum age in jiffies. 0 disables the check.
> > + *
> > + * The age is taken from the later of the BO's own &xe_bo.lru_stamp and, for
> > + * user BOs private to a VM, the VM's &xe_vm.lru_stamp.
> > + *
> > + * BOs that the shrink walk would not back up anyway are never considered
> > + * young: non-xe BOs and everything rejected by
> > + * xe_bo_shrink_backup_candidate(). Neither are BOs in XE_PL_SYSTEM: they are
> > + * not GPU mapped, hence not part of a working set, and must be moved back to
> > + * TT, refreshing the stamp, before use.
> > + *
> > + * Return: true if @bo was touched less than @min_age jiffies ago.
> > + */
> > +bool xe_bo_shrink_too_young(struct ttm_buffer_object *bo, unsigned long min_age)
> > +{
> > +	struct xe_bo *xe_bo;
> > +	unsigned long stamp;
> > +
> > +	if (!min_age || !xe_bo_is_xe_bo(bo) ||
> > +	    bo->resource->mem_type == XE_PL_SYSTEM ||
> > +	    !xe_bo_shrink_backup_candidate(bo))
> > +		return false;
> > +
> > +	xe_bo = ttm_to_xe_bo(bo);
> > +	stamp = READ_ONCE(xe_bo->lru_stamp);
> > +	if (xe_bo->vm && xe_bo_is_user(xe_bo)) {
> > +		unsigned long vm_stamp = READ_ONCE(xe_bo->vm->lru_stamp);
> > +
> > +		if (vm_stamp && (!stamp || time_after(vm_stamp, stamp)))
> > +			stamp = vm_stamp;
> > +	}
> > +
> > +	return stamp && time_before(jiffies, stamp + min_age);
> > +}
> 
> [Severity: High]
> Should xe_bo_bump_lru_locked() and xe_bo_shrink_too_young() check
> bo->ttm.bulk_move instead of bo->vm?
> 


The check should be...

if (xe_bo->vm && bulk..

> There are two cases where a user BO has xe_bo->vm != NULL and
> XE_BO_FLAG_USER set, but bo->ttm.bulk_move is NULL:
> 
> 1. In __xe_bo_create_locked(), private user BOs in a fault-mode VM
> (xe_vm_in_fault_mode(vm)) are created with bulk = NULL while bo->vm = vm is
> still assigned:
> 
> drivers/gpu/drm/xe/xe_bo.c:__xe_bo_create_locked() {
> 	...
> 	bo = xe_bo_init_locked(xe, bo, tile, vm ? xe_vm_resv(vm) : NULL,
> 			       vm && !xe_vm_in_fault_mode(vm) &&
> 			       flags & XE_BO_FLAG_USER ?
> 			       &vm->lru_bulk_move : NULL, size,
> 			       cpu_caching, type, flags, NULL, exec);
> 	...
> 	bo->vm = vm;
> }
> 
> 2. In xe_gem_object_close(), closing the GEM handle of a private user BO that
> remains mapped in a non-fault-mode VM clears bo->ttm.bulk_move while leaving
> bo->vm set:
> 
> drivers/gpu/drm/xe/xe_bo.c:xe_gem_object_close() {
> 	struct xe_bo *bo = gem_to_xe_bo(obj);
> 
> 	if (bo->vm && !xe_vm_in_fault_mode(bo->vm)) {
> 		xe_assert(xe_bo_device(bo), xe_bo_is_user(bo));
> 
> 		xe_bo_lock(bo, false);
> 		ttm_bo_set_bulk_move(&bo->ttm, NULL);
> 		xe_bo_unlock(bo);

This is probably not correct in current code.

BOs can outlive their handles, so I'm reading this correctly after a
handle closes for private user BO, we never bump the LRU.

Matt

> 	}
> }
> 
> Non-user VM BOs such as XE_BO_FLAG_PAGETABLE BOs also have bo->vm == vm and
> bo->ttm.bulk_move == NULL.
> 
> Whenever any BO of that VM moves into XE_PL_TT (calling xe_bo_move() ->
> xe_bo_bump_lru() -> xe_bo_bump_lru_locked()) or when xe_vm_bump_lru() runs on
> exec, vm->lru_stamp is updated to jiffies. However,
> ttm_lru_bulk_move_tail(&vm->lru_bulk_move) only moves BOs attached to
> vm->lru_bulk_move, and the loop in xe_vm_bump_lru() skips private BOs because
> drm_gpuvm_is_extobj() is false when obj->resv == drm_gpuvm_resv(&vm->gpuvm).
> 
> As a result, detached or fault-mode user BOs with bo->ttm.bulk_move == NULL
> are never moved to the LRU tail and stay at the oldest head of
> man->lru[XE_BO_PRIORITY_NORMAL], yet xe_bo_shrink_too_young() gives them the
> fresh vm->lru_stamp. When xe_shrinker_lru_young() or __xe_shrinker_walk() in
> drivers/gpu/drm/xe/xe_shrinker.c inspects the head of
> man->lru[XE_BO_PRIORITY_NORMAL], it sees the un-moved cold BO at the head as
> young and calls ttm_bo_lru_cursor_skip_priority(&curs), skipping the entire
> XE_BO_PRIORITY_NORMAL list across all VMs on the device.
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261020190840.854525-1-matthew.brost@intel.com?part=3