Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/xe: missing lru_lock around xe_bo_bump_lru_locked() in pagefault - [Medium] drm/xe: __xe_shrinker_walk() fails to skip priority on GPU-busy young BOs --
commit b66983cf80f56fab6416b52fa5320ec4dd988b90 Author: Matthew Brost <[email protected]> drm/xe: Add a minimum age gate to the shrinker This commit adds a minimum age gate to the non-purgeable BO shrinker, configured via Kconfig and a per-device shrinker_min_age_ms sysfs attribute. BOs and VM bulk moves are timestamped when their LRU positions are updated so the shrinker can skip backing up recently used BOs. > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index 42a6aa1a2e4a0..a4c0c6284a24d 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, [ ... ] > +/** > + * xe_bo_bump_lru_locked() - Bump a BO on the LRU and stamp its last use > + * @bo: The buffer object, reserved by the caller. > + * > + * Move @bo to the tail of its LRU and record the time for the shrinker's > + * minimum age gate. Only meaningful for BOs outside a bulk move, since a > + * BO in one is anchored inside its range and is bumped, and aged, with it. > + * The caller must hold the device LRU lock. > + */ > +void xe_bo_bump_lru_locked(struct xe_bo *bo) > +{ > + WRITE_ONCE(bo->lru_stamp, jiffies); > + ttm_bo_move_to_lru_tail(&bo->ttm); > +} [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_pagefault.c > b/drivers/gpu/drm/xe/xe_pagefault.c > index 7d363160103ab..8e25bc67772e4 100644 > --- a/drivers/gpu/drm/xe/xe_pagefault.c > +++ b/drivers/gpu/drm/xe/xe_pagefault.c > @@ -225,7 +225,7 @@ static int xe_pagefault_handle_vma(struct xe_gt *gt, > struct xe_vma *vma, > } > > if (bo) > - ttm_bo_move_to_lru_tail(&bo->ttm); > + xe_bo_bump_lru_locked(bo); [Severity: High] Does xe_pagefault_handle_vma() need to hold vm->xe->ttm.lru_lock around xe_bo_bump_lru_locked(bo)? While the pre-existing ttm_bo_move_to_lru_tail() call here also lacked lru_lock, xe_bo_bump_lru_locked() requires the caller to hold the device LRU lock because the call chain: xe_pagefault_handle_vma() xe_bo_bump_lru_locked() ttm_bo_move_to_lru_tail() ttm_resource_move_to_lru_tail() asserts lockdep_assert_held(&bo->bdev->lru_lock) and mutates the shared LRU lists via list_move_tail(). Without acquiring vm->xe->ttm.lru_lock, servicing a GPU page fault on a BO-backed VMA will trigger a lockdep warning and race with concurrent TTM LRU operations. > diff --git a/drivers/gpu/drm/xe/xe_shrinker.c > b/drivers/gpu/drm/xe/xe_shrinker.c > index c86455ea08d18..c650726ff6ded 100644 > --- a/drivers/gpu/drm/xe/xe_shrinker.c > +++ b/drivers/gpu/drm/xe/xe_shrinker.c [ ... ] > @@ -139,6 +143,16 @@ static int __xe_shrinker_walk(struct xe_shrinker > *shrinker, > if (!ttm_bo_shrink_suitable(ttm_bo, ctx)) > continue; > > + /* > + * Each LRU priority is roughly age ordered, so the > + * rest of this priority is most likely young too. > + * Move on to the next priority. > + */ > + if (xe_bo_shrink_too_young(ttm_bo, min_age)) { > + ttm_bo_lru_cursor_skip_priority(&curs); > + continue; > + } [Severity: Medium] Should the xe_bo_shrink_too_young() check in __xe_shrinker_walk() happen before ttm_bo_shrink_suitable(), or otherwise not depend on ctx->no_wait_gpu? In xe_shrinker_walk(), the first pass (and the only pass during direct reclaim when ttm_bo_shrink_avoid_wait() is true) forces ctx->no_wait_gpu to true: swap(no_wait_gpu, ctx->no_wait_gpu); save_flags.writeback = false; ret = __xe_shrinker_walk(shrinker, ctx, save_flags, min_age, to_scan, scanned, freed); swap(no_wait_gpu, ctx->no_wait_gpu); When ctx->no_wait_gpu is true, ttm_bo_shrink_suitable() returns false for any BO with unsignaled GPU fences: bool ttm_bo_shrink_suitable(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx) { return bo->ttm && ttm_tt_is_populated(bo->ttm) && !bo->pin_count && (!ctx->no_wait_gpu || dma_resv_test_signaled(bo->base.resv, DMA_RESV_USAGE_BOOKKEEP)); } Because BOs that were recently moved into TT or bumped by an exec are likely to still have active GPU fences, they will fail ttm_bo_shrink_suitable() and hit continue before reaching xe_bo_shrink_too_young(). Won't that prevent ttm_bo_lru_cursor_skip_priority(&curs) from triggering and cause __xe_shrinker_walk() to walk every busy young BO in the priority list without advancing *scanned? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
