Re: [PATCH v4 1/5] drm/xe: Bump the LRU of every BO an exec touches

[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() never visits private BOs in drm_exec loop
--

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

drm/xe: Bump the LRU of every BO an exec touches

This commit updates xe_vm_bump_lru() to replace the drm_gpuvm_is_extobj()
check in its drm_exec_for_each_locked_object() loop with explicit checks
skipping xe_vm_obj(vm) and BOs whose bulk_move matches vm->lru_bulk_move,
intending to bump non-bulk-moved private BOs such as those in fault-mode VMs.

> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 1ee72721b2ce3..2b84721804a77 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -426,10 +426,8 @@ int xe_vm_validate_rebind(struct xe_vm *vm, struct drm_exec *exec,
>   * @vm: The vm for which we are bumping LRUs.
>   * @exec: The struct drm_exec with the locked GEM objects.
>   *
> - * Move the VM's private BOs, and all external objects locked by @exec,
> - * to the LRU tail. Private BOs are moved in bulk using @vm's
> - * &xe_vm.lru_bulk_move. External objects, which may be shared with other
> - * VMs, are not part of that bulk move and are bumped individually.
> + * Move all BOs locked by @exec to the LRU tail. Those covered by @vm's
> + * &xe_vm.lru_bulk_move are moved in bulk, the rest individually.
>   */
>  void xe_vm_bump_lru(struct xe_vm *vm, struct drm_exec *exec)
>  {
> @@ -439,10 +437,17 @@ void xe_vm_bump_lru(struct xe_vm *vm, struct drm_exec *exec)
>  	spin_lock(lru_lock);
>  	ttm_lru_bulk_move_tail(&vm->lru_bulk_move);
>  	drm_exec_for_each_locked_object(exec, obj) {
> -		if (!drm_gpuvm_is_extobj(&vm->gpuvm, obj))
> +		struct xe_bo *bo;
> +
> +		/* The VM's dummy resv object, not a BO. */
> +		if (obj == xe_vm_obj(vm))
> +			continue;
> +
> +		bo = gem_to_xe_bo(obj);
> +		if (bo->ttm.bulk_move == &vm->lru_bulk_move)
>  			continue;
>  
> -		ttm_bo_move_to_lru_tail(&gem_to_xe_bo(obj)->ttm);
> +		ttm_bo_move_to_lru_tail(&bo->ttm);

[Severity: Medium]
Can drm_exec_for_each_locked_object() in xe_vm_bump_lru() ever visit a
private BO of the VM?

In drivers/gpu/drm/xe/xe_bo.c:__xe_bo_create_locked(), private BOs (where
vm is non-null) are initialized with xe_vm_resv(vm) as their reservation
object:

	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 private BOs share the VM's dma_resv, drm_gpuvm_is_extobj() is false
and they are never added to gpuvm->extobj.list.

When callers prepare the drm_exec context before calling xe_vm_bump_lru()
(via xe_validation_exec_lock() -> drm_gpuvm_exec_lock() in xe_exec_ioctl()
or xe_preempt_work_begin() in preempt_rebind_work_func()),
drm_gpuvm_prepare_vm() only locks xe_vm_obj(vm) and
drm_gpuvm_prepare_objects_locked() only locks external objects from
gpuvm->extobj.list:

	drm_gpuvm_resv_assert_held(gpuvm);
	list_for_each_entry(vm_bo, &gpuvm->extobj.list, list.entry.extobj) {
		if (drm_gpuvm_bo_is_zombie(vm_bo))
			continue;

		ret = exec_prepare_obj(exec, vm_bo->obj, num_fences);

In addition, even when the follow-up commit a6d00986d9f1 ("drm/xe: Bump the
LRU on a successful page fault") calls xe_vm_bump_lru() from
xe_pagefault_handle_vma(), xe_pagefault_begin() -> xe_vm_lock_vma() only
locks xe_vm_obj(vm) and external BOs:

	err = drm_exec_lock_obj(exec, xe_vm_obj(vm));
	if (!err && bo && !bo->vm)
		err = drm_exec_lock_obj(exec, &bo->ttm.base);

Since private BOs are never recorded in exec->objects, the loop in
xe_vm_bump_lru() only ever visits xe_vm_obj(vm) and external BOs (where
bo->vm and bo->ttm.bulk_move are always null). Does this mean the
bo->ttm.bulk_move == &vm->lru_bulk_move check is unreachable and private BOs
without a bulk_move (such as fault-mode private BOs or private BOs detached
from bulk_move in xe_gem_object_close()) still never get their LRU bumped?

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