Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/xe: xe_bo_shrink_too_young() misses age updates for bulk-moved VM BOs --
commit e6e467b2b2dcc54cbb4e9022c6f181631ac6573a Author: Matthew Brost <[email protected]> drm/xe: Add a minimum age gate to the shrinker This commit adds a minimum age gate to the xe shrinker so recently used buffer objects in TT are not backed up before a configurable delay (shrinker_min_age_ms). It timestamps BOs on move into TT and at LRU bump time, using either the VM bulk-move timestamp or the BO's own timestamp. > 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_tt_unmap_sg(xe, ttm_bo->ttm); > } > > + /* > + * ttm_resource_alloc() already placed the new resource at the tail of > + * its LRU, or of @bo's bulk move, so only record when that happened. > + */ > + if (!ret && ttm_bo->resource && ttm_bo->resource->mem_type == XE_PL_TT) > + WRITE_ONCE(bo->lru_stamp, jiffies); > + > return ret; > } [ ... ] > +bool xe_bo_shrink_too_young(struct ttm_buffer_object *bo, unsigned long > min_age) > +{ > + struct xe_bo *xe_bo; > + unsigned long stamp; > + > + if (!min_age || bo->resource->mem_type == XE_PL_SYSTEM || > + !xe_bo_shrink_backup_candidate(bo)) > + return false; > + > + xe_bo = ttm_to_xe_bo(bo); > + if (bo->bulk_move && xe_bo->vm) > + stamp = READ_ONCE(xe_bo->vm->lru_stamp); > + else > + stamp = READ_ONCE(xe_bo->lru_stamp); > + > + return stamp && time_before(jiffies, stamp + min_age); > +} [Severity: Medium] Can xe_bo_shrink_too_young() miss age updates for bulk-moved VM BOs when reading only xe_bo->vm->lru_stamp? In __xe_bo_create_locked(), every private user BO on a non-fault-mode VM is attached to vm->lru_bulk_move: drivers/gpu/drm/xe/xe_bo.c:__xe_bo_create_locked() { ... 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 bo->bulk_move && xe_bo->vm is true for these BOs, xe_bo_shrink_too_young() ignores xe_bo->lru_stamp (even though xe_bo_move() updates bo->lru_stamp on move into XE_PL_TT) and only consults xe_bo->vm->lru_stamp. However, vm->lru_stamp is only updated in xe_vm_bump_lru(), which is skipped in xe_exec_ioctl() for all LR-mode VMs: drivers/gpu/drm/xe/xe_exec.c:xe_exec_ioctl() { ... if (!err && !xe_vm_in_lr_mode(vm)) xe_vm_bump_lru(vm, exec); ... } For a preempt-fence LR VM (XE_VM_FLAG_LR_MODE without XE_VM_FLAG_FAULT_MODE), private user BOs have bo->bulk_move set to &vm->lru_bulk_move, but xe_exec_ioctl() never calls xe_vm_bump_lru(), and preempt_rebind_work_func() only calls xe_vm_bump_lru() after an eviction or invalidation triggers rebind work. As a result, vm->lru_stamp stays 0 until a rebind occurs and remains frozen at the last rebind timestamp afterward. In addition, on any non-fault-mode VM, when a private user BO is moved into XE_PL_TT in xe_bo_move() while the VM's (XE_PL_TT, bo->priority) bulk-move range is empty (such as before the first exec, or after prior TT BOs at that priority were evicted or moved to VRAM), ttm_resource_init() and ttm_lru_bulk_move_add() place the resource at the tail of man->lru[bo->priority] without updating vm->lru_stamp. When that happens, won't xe_bo_shrink_too_young() see a 0 or stale vm->lru_stamp and treat a newly moved or active TT BO at the LRU tail as old, both allowing it to be backed up immediately and failing to trigger ttm_bo_lru_cursor_skip_priority() in xe_shrinker_lru_young() and __xe_shrinker_walk()? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
