Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/ttm: DMA mapping leak in ttm_pool_backup() on -EBUSY after restore abort - [High] drm/ttm: ttm_tt_backup() leaves tt populated if backup fails at first page - [Medium] drm/xe: stale remaining_pages and shrinker counts on failed ttm_tt_restore() --
commit fc282ba9ab604d69dff7ad175cd51a0231a6c0b3 Author: Thomas Hellström <[email protected]> drm/ttm, drm/xe: Introduce TTM_TT_FLAG_PARTIAL for partially backed-up tts This commit introduces TTM_TT_FLAG_PARTIAL and ttm_tt::remaining_pages to track partially backed-up ttm_tt objects so they stay on the normal LRU and remain eligible for shrinker reclaim. It also adds ttm_pool_restore_abort() to clean up interrupted restores before backup and updates xe shrinker page accounting to track resident pages. > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index 1bf37023fed66..8f05257e4f542 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c [ ... ] > @@ -1039,6 +1057,91 @@ void ttm_pool_drop_backed_up(struct ttm_tt *tt) > ttm_pool_free_range(NULL, tt, ttm_cached, start_page, tt->num_pages); > } > > +/** > + * ttm_pool_restore_abort() - Abort an ongoing restore of a partial tt > + * @tt: The struct ttm_tt with an active restore. [ ... ] > +static void ttm_pool_restore_abort(struct ttm_tt *tt) > +{ > + struct ttm_pool_tt_restore *restore = tt->restore; > + pgoff_t i, nr, mid; [ ... ] > + /* > + * Don't DMA-unmap committed restored pages here; their DMA > + * addresses are in tt->dma_address[] and the backup path > + * will unmap them. > + */ > + > + /* Convert non-cached committed pages to write-back. */ > + mid = restore->snapshot_alloc.caching_divide - tt->pages; > +#ifdef CONFIG_X86 > + if (mid > 0 && tt->caching != ttm_cached) > + set_pages_array_wb(tt->pages, mid); > +#endif > + > + kfree(restore); > + tt->restore = NULL; > + > + for (i = 0, nr = 0; i < tt->num_pages; i++) { > + struct page *p = tt->pages[i]; > + > + if (p && !ttm_backup_page_ptr_is_handle(p)) > + nr++; > + } > + tt->remaining_pages = nr; > +} > + > /** > * ttm_pool_backup() - Back up or purge a struct ttm_tt > * @pool: The pool used when allocating the struct ttm_tt. > @@ -1071,10 +1174,13 @@ long ttm_pool_backup(struct ttm_pool *pool, struct > ttm_tt *tt, > pgoff_t shrunken = 0; > pgoff_t i, j, num_pages, npages; > pgoff_t nr_backed; > + bool partial = ttm_tt_is_partial(tt); > > if (WARN_ON(ttm_tt_is_backed_up(tt))) > return -EINVAL; > > + ttm_pool_restore_abort(tt); > + > if ((!ttm_backup_bytes_avail() && !flags->purge) || > ttm_pool_uses_dma_alloc(pool) || ttm_tt_is_backed_up(tt)) > return -EBUSY; [Severity: High] Can this leak DMA mappings for committed restored pages when ttm_pool_backup() returns -EBUSY right after ttm_pool_restore_abort(tt)? When a restore of a backed-up or partial ttm_tt (with tt->dma_address != NULL) fails after committing one or more pages via ttm_pool_allocated_page_commit(), tt->pages[0 .. restore->alloced_pages - 1] are populated and DMA-mapped in tt->dma_address[], and tt is marked TTM_TT_FLAG_PARTIAL with tt->restore non-NULL. When ttm_pool_backup() is later called by the shrinker, it calls ttm_pool_restore_abort(tt), which intentionally leaves committed restored pages DMA-mapped in tt->dma_address[] for the backup loop to unmap, and frees tt->restore (setting tt->restore = NULL). If ttm_pool_backup() then takes the early -EBUSY return (for example, when swap space is exhausted so !ttm_backup_bytes_avail() && !flags->purge), the DMA-unmap loop below is skipped. That leaves a partial tt with tt->restore == NULL while tt->pages[0 .. alloced_pages - 1] remain DMA-mapped: - On teardown, ttm_tt_fini() -> ttm_pool_drop_backed_up() sees tt->restore == NULL and calls: ttm_pool_free_range(NULL, tt, ttm_cached, 0, tt->num_pages); Because pool is NULL, ttm_pool_unmap_and_free() skips ttm_pool_unmap() and frees the still-DMA-mapped pages back to the page allocator. - On a subsequent restore, ttm_pool_restore_and_alloc() starts a fresh restore at index 0, and ttm_pool_restore_commit() frees the retained resident pages via __free_pages_gpu_account(p, 0, false) without DMA-unmapping them and overwrites tt->dma_address[]. > diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c > index b645a18181843..2b441bf41d927 100644 > --- a/drivers/gpu/drm/ttm/ttm_tt.c > +++ b/drivers/gpu/drm/ttm/ttm_tt.c [ ... ] > @@ -289,7 +289,18 @@ long ttm_tt_backup(struct ttm_device *bdev, struct > ttm_tt *tt, > ret = ttm_pool_backup(&bdev->pool, tt, &flags); > if (ret > 0) { > tt->page_flags &= ~TTM_TT_FLAG_PRIV_POPULATED; > - tt->page_flags |= TTM_TT_FLAG_BACKED_UP; > + if (ret >= tt->remaining_pages) > + tt->remaining_pages = 0; > + else > + tt->remaining_pages -= ret; > + > + if (tt->remaining_pages) { > + tt->page_flags &= ~TTM_TT_FLAG_BACKED_UP; > + tt->page_flags |= TTM_TT_FLAG_PARTIAL; > + } else { > + tt->page_flags &= ~TTM_TT_FLAG_PARTIAL; > + tt->page_flags |= TTM_TT_FLAG_BACKED_UP; > + } > } > > return ret; [Severity: High] What happens if ttm_pool_backup() fails on the very first page after it has already converted the pages to write-back and unmapped tt->dma_address[]? In ttm_pool_backup(), for a populated tt (!partial), all pages are first converted to write-back via set_pages_array_wb() (when tt->caching != ttm_cached), and all entries in tt->dma_address[] are DMA-unmapped via ttm_pool_unmap() and zeroed via memset() before the folio backup loop starts. If ttm_backup_backup_folio() fails on the first page (i == 0) with nr_backed == 0 (or fault injection truncates num_pages mid-compound on the first compound page), ttm_pool_backup() breaks out of the loop with shrunken == 0 and returns a negative error code (or 0). Because ttm_tt_backup() only clears TTM_TT_FLAG_PRIV_POPULATED and sets TTM_TT_FLAG_PARTIAL when ret > 0, the tt remains marked TTM_TT_FLAG_PRIV_POPULATED (and not TTM_TT_FLAG_PARTIAL) even though its DMA mappings have been unmapped and zeroed and its pages have been converted to write-back: - Subsequent ttm_bo_populate() / ttm_tt_populate() calls see ttm_tt_is_populated(tt) == true and return 0 without restoring DMA mappings or caching. - A subsequent ttm_pool_backup() call sees !partial and calls set_pages_array_wb() a second time on pages that are already WB. - Subsequent ttm_tt_unpopulate() -> ttm_pool_free() calls ttm_pool_unmap() on zeroed DMA addresses and returns the WB pages into the WC/UC pool via ttm_pool_type_give(). > @@ -300,10 +311,20 @@ int ttm_tt_restore(struct ttm_device *bdev, struct > ttm_tt *tt, > { > int ret = ttm_pool_restore_and_alloc(&bdev->pool, tt, ctx); > > - if (ret) > + if (ret) { > + /* > + * Let the shrinker reclaim pages made resident by the > + * failed restore. remaining_pages is recounted on backup. > + */ > + if (ttm_tt_is_backed_up(tt) && > + ttm_pool_restore_has_resident(tt)) { > + tt->page_flags &= ~TTM_TT_FLAG_BACKED_UP; > + tt->page_flags |= TTM_TT_FLAG_PARTIAL; > + } > return ret; > + } > > - tt->page_flags &= ~TTM_TT_FLAG_BACKED_UP; > + tt->page_flags &= ~(TTM_TT_FLAG_BACKED_UP | TTM_TT_FLAG_PARTIAL); > > return 0; > } [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index 2afee1a6de668..6907b683dee07 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c [ ... ] > @@ -676,20 +696,35 @@ static int xe_ttm_tt_populate(struct ttm_device > *ttm_dev, struct ttm_tt *tt, > !(tt->page_flags & TTM_TT_FLAG_EXTERNAL_MAPPABLE)) > return 0; > > - if (ttm_tt_is_backed_up(tt) && !xe_tt->purgeable) { > + /* > + * A purgeable backed-up tt was purged and holds neither pages nor > + * handles. Partial tts are never purgeable: madvise only marks > + * populated tts purgeable, and the shrinker purges those fully. > + */ > + if ((ttm_tt_is_backed_up(tt) || ttm_tt_is_partial(tt)) && > + !xe_tt->purgeable) { > + pgoff_t remaining = ttm_tt_remaining_pages(tt); > + long backed_up_pages = tt->num_pages - remaining; > + > err = ttm_tt_restore(ttm_dev, tt, ctx); > + if (!err) { > + xe_tt->purgeable = false; > + xe_ttm_tt_account_add(ttm_to_xe_device(ttm_dev), tt, > + backed_up_pages, !remaining); > + update_global_total_pages(ttm_dev, backed_up_pages); > + } > } else { [ ... ] > @@ -1478,7 +1522,8 @@ long xe_bo_shrink(struct ttm_operation_ctx *ctx, struct > ttm_buffer_object *bo, > if (needs_rpm && !xe_pm_runtime_get_if_active(xe)) > goto out_unref; > > - *scanned += tt->num_pages; > + old_remaining = ttm_tt_remaining_pages(tt); > + *scanned += old_remaining; > lret = ttm_bo_shrink(ctx, bo, (struct ttm_bo_shrink_flags) > {.purge = false, > .writeback = flags.writeback, > @@ -1486,10 +1531,7 @@ long xe_bo_shrink(struct ttm_operation_ctx *ctx, > struct ttm_buffer_object *bo, > if (needs_rpm) > xe_pm_runtime_put(xe); > > - if (lret > 0) { > - xe_ttm_tt_account_subtract(xe, tt, tt->num_pages, 1); > - update_global_total_pages(bo->bdev, -(long)tt->num_pages); > - } > + xe_ttm_tt_account_shrunk(bo->bdev, tt, old_remaining); [Severity: Medium] When ttm_tt_restore() fails after making pages resident, doesn't leaving tt->remaining_pages and the xe shrinker counters unupdated break shrinker accounting and scanning? When a fully backed-up tt (with tt->remaining_pages == 0 and 0 pages accounted in xe->mem.shrinker) fails in ttm_tt_restore() after committing or restoring pages, ttm_tt_restore() transitions it to TTM_TT_FLAG_PARTIAL, but tt->remaining_pages stays 0 until ttm_pool_restore_abort() runs during backup, and xe_ttm_tt_populate() skips xe_ttm_tt_account_add() because err != 0: 1. Because the newly resident pages were never added via xe_ttm_tt_account_add(), xe_shrinker_count() does not see them and can return SHRINK_EMPTY if no other BOs are populated. 2. When xe_bo_shrink() (or xe_bo_shrink_purge()) runs on this BO, it reads old_remaining = ttm_tt_remaining_pages(tt) (which is still 0) and does *scanned += old_remaining before calling ttm_bo_shrink() -> ttm_tt_backup() -> ttm_pool_backup() -> ttm_pool_restore_abort(). In xe_shrinker_scan(), if nr_scanned remains 0 while freed > 0, it will return SHRINK_STOP instead of freed: drivers/gpu/drm/xe/xe_shrinker.c:xe_shrinker_scan() { ... return nr_scanned ? freed : SHRINK_STOP; } 3. If the BO is destroyed via xe_ttm_tt_destroy() after a failed restore without an intervening backup attempt, or if a backup attempt in xe_bo_shrink() aborts the restore and increases tt->remaining_pages from 0 to N (say, when only part of those N pages are backed up), xe_ttm_tt_account_shrunk() and xe_ttm_tt_destroy() will subtract pages that were never added in xe_ttm_tt_populate(), underflowing the shrinker counters. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
