Re: [PATCH v5 3/3] drm/xe: Add a minimum age gate to the shrinker

[sashiko-bot@kernel.org]

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 <matthew.brost@intel.com>

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/20261027203107.1065800-1-matthew.brost@intel.com?part=3