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 <[email protected]> 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/[email protected]?part=1
