Re: [PATCH v4 2/5] drm/xe: Bump the LRU on a successful page fault

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: xe_vm_bump_lru() in xe_pagefault_handle_vma() misses private BOs
--

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

drm/xe: Bump the LRU on a successful page fault

This commit calls xe_vm_bump_lru() in xe_pagefault_handle_vma() after a
successful VMA rebind so that faulted BOs are moved to the tail of the TTM
LRU.

> diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> index 5247321c805ec..3719258aa6f09 100644
> --- a/drivers/gpu/drm/xe/xe_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_pagefault.c
> @@ -221,6 +221,8 @@ static int xe_pagefault_handle_vma(struct xe_gt *gt, struct xe_vma *vma,
>  				xe_pagefault_set_error(pf, XE_PAGEFAULT_ERROR_VMA_REBIND);
>  				break;
>  			}
> +
> +			xe_vm_bump_lru(vm, &exec);

[Severity: Medium]
Does xe_vm_bump_lru() actually move VM-private BOs to the LRU tail when
handling a page fault?

When a VM-private BO is created for a fault-mode VM in
__xe_bo_create_locked(), it is not added 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 page faults only occur on fault-mode VMs, vm->lru_bulk_move is empty
and ttm_lru_bulk_move_tail(&vm->lru_bulk_move) in xe_vm_bump_lru() is a
no-op.

Furthermore, during xe_pagefault_begin() -> xe_vm_lock_vma(), only external
BOs (!bo->vm) are locked individually in exec, whereas VM-private BOs share
the VM's reservation object and only lock xe_vm_obj(vm):

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_vm_bump_lru() then iterates over the locked objects in exec, it skips
xe_vm_obj(vm):

drivers/gpu/drm/xe/xe_vm.c:xe_vm_bump_lru() {
	...
	drm_exec_for_each_locked_object(exec, obj) {
		struct xe_bo *bo;

		/* The VM's dummy resv object, not a BO. */
		if (obj == xe_vm_obj(vm))
			continue;
	...
}

If a faulted VM-private BO's backing store is already in a compatible
placement so that ttm_bo_validate() returns early without moving the resource,
will the BO's TTM LRU position remain completely unchanged here?

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