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 281f376af4d072a5a3001932f9bcc749e37e12a5 Author: Matthew Brost <[email protected]> drm/xe: Add a minimum age gate to the shrinker This patch introduces a per-device shrinker_min_age_ms sysfs attribute and Kconfig option to prevent the shrinker from backing up buffer objects that were recently moved into TT or bumped on the LRU. Buffer objects are timestamped on moves into XE_PL_TT and LRU updates, with the shrinker count and scan walks skipping LRU priorities whose oldest candidate is younger than the configured minimum age. > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index 42a6aa1a2e4a0..3163c01012f73 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c > @@ -1357,9 +1357,96 @@ 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) > + 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); > + xe_bo_assert_held(xe_bo); > + > + if (bo->bulk_move && xe_bo->vm) > + stamp = xe_bo->vm->lru_stamp; > + else > + stamp = xe_bo->lru_stamp; > + > + return stamp && time_before(jiffies, stamp + min_age); > +} [Severity: Medium] As reported by Sashiko on v6 [1], can xe_bo_shrink_too_young() miss age updates for private BOs that use a VM's bulk move? In drivers/gpu/drm/xe/xe_bo.c, __xe_bo_create_locked() attaches every private user BO created on a non-fault-mode VM 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 xe_bo_shrink_too_young() checks bo->bulk_move && xe_bo->vm and reads only xe_bo->vm->lru_stamp (ignoring xe_bo->lru_stamp), three cases appear to leave the stamp out of sync with the BO's actual activity or LRU position: 1. For preempt-fence LR VMs (XE_VM_FLAG_LR_MODE without XE_VM_FLAG_FAULT_MODE), !xe_vm_in_fault_mode(vm) is true so private user BOs have bo->bulk_move set. However, xe_exec_ioctl() in drivers/gpu/drm/xe/xe_exec.c skips xe_vm_bump_lru() for all LR 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); } And preempt_rebind_work_func() in drivers/gpu/drm/xe/xe_vm.c exits early in xe_preempt_work_begin() when !preempt_fences_waiting(vm), so vm->lru_stamp remains 0 (or stale from the last eviction/invalidation rebind) while the VM creates BOs and submits work. When kswapd runs with ctx.no_wait_gpu == false, won't xe_bo_shrink_too_young() treat these active BOs as old and allow them to be preempted and backed up regardless of shrinker_min_age_ms? 2. In xe_bo_move(), moving a BO into XE_PL_TT updates bo->lru_stamp = jiffies without updating vm->lru_stamp. When a private BO on any non-fault-mode VM is created in or moved into XE_PL_TT while its priority bucket in vm->lru_bulk_move is empty (for instance, before the VM's first exec or when no other TT BOs exist in that bucket), ttm_resource_alloc() -> ttm_lru_bulk_move_add() places the resource at the tail of man->lru. Since xe_bo_shrink_too_young() ignores bo->lru_stamp when bo->bulk_move is set, a BO newly moved into TT will still read the stale or zero vm->lru_stamp and won't be protected by shrinker_min_age_ms until an exec bumps the VM. 3. In drivers/gpu/drm/xe/xe_bo.c, xe_gem_object_close() detaches a private BO from vm->lru_bulk_move in place when its GEM handle is closed while mappings remain in the VM: drivers/gpu/drm/xe/xe_bo.c:xe_gem_object_close() { if (bo->vm && !xe_vm_in_fault_mode(bo->vm)) { xe_bo_lock(bo, false); ttm_bo_set_bulk_move(&bo->ttm, NULL); xe_bo_unlock(bo); } } Once bo->bulk_move is cleared, xe_bo_shrink_too_young() switches back to reading xe_bo->lru_stamp without updating either the stamp or the BO's LRU position. If the BO was recently bumped via xe_vm_bump_lru(), closing the handle reverts its age to the stale bo->lru_stamp; conversely, if the BO was recently moved into TT inside an older bulk-move range near the head of the LRU and then closed, its young bo->lru_stamp becomes visible near the LRU head and causes ttm_bo_lru_cursor_skip_priority() to skip older BOs behind it. Would it make sense to propagate or take max(xe_bo->vm->lru_stamp, xe_bo->lru_stamp) when appropriate, and ensure preempt-fence LR VMs and bulk-move detach paths keep the stamp consistent with the LRU position? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
