Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] BOs moved to SYSTEM memory during a failed shrinker purge are incorrectly left on the defrag list. - [Medium] Framebuffer pinning bypasses defrag list tracking updates, violating the invariant that pinned BOs are removed from the list. --
--- Patch [15]: [PATCH v2 15/33] drm/xe: Track BOs backed at a sub-optimal page order --- commit d8c62c56c566775d903f43756778e071af0acb80 Author: Matthew Brost <[email protected]> drm/xe: Track BOs backed at a sub-optimal page order Add per-device tracking of BOs whose backing TT pages were allocated below the pool's beneficial order, as reported by the TTM_TT_FLAG_BENEFICIAL_ORDER_FAILED tt page flag. These BOs are future candidates for a defragmentation pass that reallocates their pages at the beneficial order. > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index cca617cf34d13..d7b0536564ab2 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c [ ... ] > @@ -1261,6 +1358,15 @@ static long xe_bo_shrink_purge(struct > ttm_operation_ctx *ctx, > if (lret > 0) { > xe_ttm_tt_account_subtract(xe, bo->ttm); > update_global_total_pages(bo->bdev, -(long)tt->num_pages); > + > + /* > + * The pages are gone, so a tracked BO no longer needs defrag. > + * This shrinker purge path frees pages without going through > + * xe_bo_move(), so drop any stale defrag list entry here. Ghost > + * bos are never tracked, so guard for real xe bos. > + */ > + if (xe_bo_is_xe_bo(bo)) > + xe_bo_defrag_remove(ttm_to_xe_bo(bo)); > } > > return lret; [Severity: Medium] When xe_bo_shrink_purge() attempts to purge a BO, it uses ttm_bo_move_null() to move it to XE_PL_SYSTEM, which bypasses xe_bo_defrag_update(). If the subsequent ttm_bo_shrink() call fails (lret <= 0), this explicit xe_bo_defrag_remove() is skipped. Could this leave a tracked BO in SYSTEM memory while incorrectly remaining on the defrag list, violating the tracking invariant and wasting worker cycles? [ ... ] > @@ -3072,6 +3183,8 @@ int xe_bo_pin(struct xe_bo *bo, struct drm_exec *exec) > if (bo->ttm.ttm && ttm_tt_is_populated(bo->ttm.ttm)) > xe_ttm_tt_account_subtract(xe, bo->ttm.ttm); > > + xe_bo_defrag_remove(bo); > + > /* > * FIXME: If we always use the reserve / unreserve functions for locking > * we do not need this. [Severity: Medium] Will this miss defrag list tracking updates for framebuffer BOs? xe_fb_pin() and __xe_unpin_fb_vma() in drivers/gpu/drm/xe/display/xe_fb_pin.c call ttm_bo_pin() and ttm_bo_unpin() directly rather than using this xe_bo_pin() wrapper. If a framebuffer BO resides in XE_PL_TT with sub-optimal page allocations, could it remain on the defrag list even when pinned, causing the defrag worker to incorrectly process pinned BOs? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
