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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: xe_bo_shrink_too_young() misses age updates for bulk-moved VM BOs
--

commit e6e467b2b2dcc54cbb4e9022c6f181631ac6573a
Author: Matthew Brost <matthew.brost@intel.com>

drm/xe: Add a minimum age gate to the shrinker

This commit adds a minimum age gate to the xe shrinker so recently used
buffer objects in TT are not backed up before a configurable delay
(shrinker_min_age_ms). It timestamps BOs on move into TT and at LRU bump
time, using either the VM bulk-move timestamp or the BO's own timestamp.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 42a6aa1a2e4a0..a4c0c6284a24d 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -1357,9 +1357,92 @@ static int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
>  		xe_tt_unmap_sg(xe, ttm_bo->ttm);
>  	}
>  
> +	/*
> +	 * ttm_resource_alloc() already placed the new resource at the tail of
> +	 * its LRU, or of @bo's bulk move, so only record when that happened.
> +	 */
> +	if (!ret && ttm_bo->resource && ttm_bo->resource->mem_type == XE_PL_TT)
> +		WRITE_ONCE(bo->lru_stamp, jiffies);
> +
>  	return ret;
>  }
[ ... ]
> +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 || bo->resource->mem_type == XE_PL_SYSTEM ||
> +	    !xe_bo_shrink_backup_candidate(bo))
> +		return false;
> +
> +	xe_bo = ttm_to_xe_bo(bo);
> +	if (bo->bulk_move && xe_bo->vm)
> +		stamp = READ_ONCE(xe_bo->vm->lru_stamp);
> +	else
> +		stamp = READ_ONCE(xe_bo->lru_stamp);
> +
> +	return stamp && time_before(jiffies, stamp + min_age);
> +}

[Severity: Medium]
Can xe_bo_shrink_too_young() miss age updates for bulk-moved VM BOs when
reading only xe_bo->vm->lru_stamp?

In __xe_bo_create_locked(), every private user BO on a non-fault-mode VM is
attached to vm->lru_bulk_move:

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);
	...
}

Because bo->bulk_move && xe_bo->vm is true for these BOs,
xe_bo_shrink_too_young() ignores xe_bo->lru_stamp (even though xe_bo_move()
updates bo->lru_stamp on move into XE_PL_TT) and only consults
xe_bo->vm->lru_stamp.

However, vm->lru_stamp is only updated in xe_vm_bump_lru(), which is
skipped in xe_exec_ioctl() for all LR-mode VMs:

drivers/gpu/drm/xe/xe_exec.c:xe_exec_ioctl() {
	...
	if (!err && !xe_vm_in_lr_mode(vm))
		xe_vm_bump_lru(vm, exec);
	...
}

For a preempt-fence LR VM (XE_VM_FLAG_LR_MODE without
XE_VM_FLAG_FAULT_MODE), private user BOs have bo->bulk_move set to
&vm->lru_bulk_move, but xe_exec_ioctl() never calls xe_vm_bump_lru(), and
preempt_rebind_work_func() only calls xe_vm_bump_lru() after an eviction or
invalidation triggers rebind work. As a result, vm->lru_stamp stays 0 until
a rebind occurs and remains frozen at the last rebind timestamp afterward.

In addition, on any non-fault-mode VM, when a private user BO is moved into
XE_PL_TT in xe_bo_move() while the VM's (XE_PL_TT, bo->priority) bulk-move
range is empty (such as before the first exec, or after prior TT BOs at
that priority were evicted or moved to VRAM), ttm_resource_init() and
ttm_lru_bulk_move_add() place the resource at the tail of
man->lru[bo->priority] without updating vm->lru_stamp.

When that happens, won't xe_bo_shrink_too_young() see a 0 or stale
vm->lru_stamp and treat a newly moved or active TT BO at the LRU tail as
old, both allowing it to be backed up immediately and failing to trigger
ttm_bo_lru_cursor_skip_priority() in xe_shrinker_lru_young() and
__xe_shrinker_walk()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027224902.1085762-1-matthew.brost@intel.com?part=3