Re: [PATCH v5 1/3] 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:
- [Critical] drm/xe: missing lru_lock in xe_pagefault_handle_vma() LRU bump
--

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

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

This commit moves a VMA's backing BO to the tail of the TTM LRU after
successfully servicing a page fault so active fault-mode BOs are not
prematurely evicted or shrunk.

> diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> index 5247321c805ec..7d363160103ab 100644
> --- a/drivers/gpu/drm/xe/xe_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_pagefault.c
> @@ -201,6 +201,8 @@ static int xe_pagefault_handle_vma(struct xe_gt *gt, struct xe_vma *vma,
>  		xe_validation_ctx_init(&ctx, &vm->xe->val, &exec,
>  				       (struct xe_val_flags) {});
>  		drm_exec_until_all_locked(&exec) {
> +			struct xe_bo *bo = xe_vma_bo(vma);
> +
>  			err = xe_pagefault_begin(&exec, vma, tile->mem.vram,
>  						 needs_vram == 1);
>  			drm_exec_retry_on_contention(&exec);
> @@ -221,6 +223,9 @@ 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;
>  			}
> +
> +			if (bo)
> +				ttm_bo_move_to_lru_tail(&bo->ttm);

[Severity: Critical]
Should xe_pagefault_handle_vma() call ttm_bo_move_to_lru_tail_unlocked()
here (or acquire bo->ttm.bdev->lru_lock around the call) instead of calling
ttm_bo_move_to_lru_tail() directly?

At this point in xe_pagefault_handle_vma(), only the VM and BO dma_resv
locks (via drm_exec), vma->fault_lock, and vm->lock are held, not
bo->ttm.bdev->lru_lock.

When a page fault occurs on a BO-backed VMA with a valid bo->ttm.resource,
the call chain xe_pagefault_handle_vma() -> ttm_bo_move_to_lru_tail() ->
ttm_resource_move_to_lru_tail() expects bo->bdev->lru_lock to be held:

drivers/gpu/drm/ttm/ttm_bo.c:ttm_bo_move_to_lru_tail() {
	dma_resv_assert_held(bo->base.resv);

	if (bo->resource)
		ttm_resource_move_to_lru_tail(bo->resource);
}

drivers/gpu/drm/ttm/ttm_resource.c:ttm_resource_move_to_lru_tail() {
	struct ttm_buffer_object *bo = res->bo;
	struct ttm_device *bdev = bo->bdev;

	lockdep_assert_held(&bo->bdev->lru_lock);

	if (ttm_resource_unevictable(res, bo)) {
		list_move_tail(&res->lru.link, &bdev->unevictable);

	} else if (bo->bulk_move) {
		struct ttm_lru_bulk_move_pos *pos =
			ttm_lru_bulk_move_pos(bo->bulk_move, res);

		ttm_lru_bulk_move_pos_tail(pos, res);
	} else {
		struct ttm_resource_manager *man;

		man = ttm_manager_type(bdev, res->mem_type);
		list_move_tail(&res->lru.link, &man->lru[bo->priority]);
	}
}

Without holding bo->bdev->lru_lock, won't this trigger a lockdep assertion
and race with concurrent LRU list users (such as other pagefault workers,
xe_vm_bump_lru(), resource init/fini, eviction, and the shrinker),
corrupting the shared LRU lists?

Also, a later commit in this series (b66983cf80f5) replaces this call with
xe_bo_bump_lru_locked(bo), which still calls ttm_bo_move_to_lru_tail() and
requires the caller to hold the device LRU lock without acquiring it in
xe_pagefault_handle_vma().

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