Re: [PATCH v4 5/5] 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: fault-mode VM-private BOs are not stamped or aged in xe_vm_bump_lru()
--

commit 169ae3636dd7a7f18b87dc4d22d9738909855961
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
timestamps buffer objects on LRU updates so the shrinker avoids backing up
recently used objects. Buffer objects in a VM's bulk move use the VM-wide
timestamp updated in xe_vm_bump_lru(), while other buffer objects use their
own timestamp updated in xe_bo_move() and xe_bo_bump_lru_locked().

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index ba16b260dbcb7..59874f9a1ed7a 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,

[ ... ]

> +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]
Does this leave VM-private buffer objects in fault-mode VMs with a stale
lru_stamp when they are faulted and rebound through xe_vm_bump_lru()?

In drivers/gpu/drm/xe/xe_bo.c, __xe_bo_create_locked() initializes
VM-private user buffer objects in fault-mode VMs with bo->vm set, resv set
to xe_vm_resv(vm), and bulk_move set to NULL:

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

When a GPU page fault is handled via
xe_pagefault_handle_vma()->xe_pagefault_begin()->xe_vm_lock_vma(),
drivers/gpu/drm/xe/xe_vm.c only adds the buffer object to drm_exec when
bo->vm is NULL, since VM-private objects share xe_vm_obj(vm)'s reservation:

drivers/gpu/drm/xe/xe_vm.c:xe_vm_lock_vma() {
	...
	err = drm_exec_lock_obj(exec, xe_vm_obj(vm));
	if (!err && bo && !bo->vm)
		err = drm_exec_lock_obj(exec, &bo->ttm.base);
	...
}

When xe_pagefault_handle_vma() then calls xe_vm_bump_lru(vm, &exec),
vm->lru_stamp is updated, but drm_exec_for_each_locked_object(exec, obj)
only sees xe_vm_obj(vm) (which is skipped) and any external objects, so
xe_bo_bump_lru_locked(bo) is never called for VM-private objects.

When the shrinker later calls xe_bo_shrink_too_young(), bo->bulk_move is
NULL for fault-mode VM-private objects, so it reads xe_bo->lru_stamp
rather than xe_bo->vm->lru_stamp. If the object was already resident in
XE_PL_TT when faulted (for example, on another tile or after VMA
invalidation), won't its age remain stuck at the initial xe_bo_move()
timestamp rather than reflecting the fault activity?

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