Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/ttm: unsplit high-order retry drains reserves in ttm_pool_backup() - [High] drm/xe: premature scan termination in xe_shrinker_walk() and xe_bo_shrink() - [High] drm/ttm: corrupted caching and DMA state in ttm_pool_backup() on page-0 failure --
commit 40d7831061e17e7b2e436a106ab6986d2645fa67 Author: Thomas Hellström <[email protected]> drm/ttm, drm/xe: Gate reserve-dipping backup retries on may_writeback This commit gates shmem backup retries that dip into kernel memory reserves on a new may_writeback flag while separating direct swap writeback into force_writeback, and threads both flags through TTM and the Xe shrinker. > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index 8f05257e4f542..87c1ac39af94e 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c [ ... ] > @@ -1256,7 +1260,9 @@ long ttm_pool_backup(struct ttm_pool *pool, struct > ttm_tt *tt, > else > gfp = GFP_HIGHUSER; > > - alloc_gfp = GFP_KERNEL | __GFP_HIGH | __GFP_NOWARN | > __GFP_RETRY_MAYFAIL; > + /* Don't dip into kernel reserves unless @flags->may_writeback is set. > */ > + alloc_gfp = GFP_KERNEL | __GFP_HIGH | __GFP_NOWARN | > + __GFP_RETRY_MAYFAIL | __GFP_NOMEMALLOC; [Severity: High] When ttm_pool_backup() is called with flags->may_writeback set to false (such as the initial pass of xe_shrinker_walk()), can an -ENOMEM failure on the very first page leave tt with corrupted caching and DMA state? Before entering the backup loop, ttm_pool_backup() converts all resident pages of a non-partial tt with tt->caching != ttm_cached to write-back via set_pages_array_wb(), and unmaps and zeroes all tt->dma_address[] entries: drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_backup() { if (!partial && tt->caching != ttm_cached) { ... if (run_count) set_pages_array_wb(&tt->pages[run_start], run_count); } ... if (tt->dma_address || flags->purge) { ... if (tt->dma_address && tt->dma_address[i]) { ttm_pool_unmap(pool, tt->dma_address[i], num_pages); memset(&tt->dma_address[i], 0, num_pages * sizeof(*tt->dma_address)); } } } Because alloc_gfp now includes __GFP_NOMEMALLOC for all page orders (including order 0) and does not retry when flags->may_writeback is false, shmem_read_folio_gfp() can fail with -ENOMEM on the first page (i == 0, nr_backed == 0, shrunken == 0), causing ttm_pool_backup() to return -ENOMEM. In ttm_tt_backup(), tt->page_flags is only updated when ret > 0: drivers/gpu/drm/ttm/ttm_tt.c:ttm_tt_backup() { ret = ttm_pool_backup(&bdev->pool, tt, &flags); if (ret > 0) { tt->page_flags &= ~TTM_TT_FLAG_PRIV_POPULATED; ... if (tt->remaining_pages) { tt->page_flags &= ~TTM_TT_FLAG_BACKED_UP; tt->page_flags |= TTM_TT_FLAG_PARTIAL; } ... } } With TTM_TT_FLAG_PRIV_POPULATED still set and TTM_TT_FLAG_PARTIAL clear, won't subsequent calls to ttm_tt_populate() see ttm_tt_is_populated(tt) as true and return 0 without restoring WC/UC caching or DMA mappings, while ttm_tt_unpopulate() -> ttm_pool_free() returns the write-back pages directly into the WC/UC page pool and calls ttm_pool_unmap() on the zeroed tt->dma_address[] entries? > > num_pages = tt->num_pages; > > @@ -1280,14 +1286,7 @@ long ttm_pool_backup(struct ttm_pool *pool, struct > ttm_tt *tt, > order = ttm_pool_page_order(pool, page); > npages = 1UL << order; > > - /* > - * We don't allow dipping kernel reserves for high order backup > - */ > - if (order) > - alloc_gfp |= __GFP_NOMEMALLOC; > - else > - alloc_gfp &= ~__GFP_NOMEMALLOC; > - > + dip_reserves = false; > /* > * Back up the compound atomically at its native order. If > * fault injection truncated num_pages mid-compound, skip > @@ -1296,10 +1295,18 @@ long ttm_pool_backup(struct ttm_pool *pool, struct > ttm_tt *tt, > if (unlikely(i + npages > num_pages)) > break; > > +retry: > handle = ttm_backup_backup_folio(backup, page_folio(page), > - order, flags->writeback, i, > - gfp, alloc_gfp, > - &nr_backed); > + order, flags->force_writeback, > i, > + gfp, dip_reserves ? > + alloc_gfp & ~__GFP_NOMEMALLOC : > + alloc_gfp, &nr_backed); > + if (unlikely(handle == -ENOMEM && !nr_backed && > + flags->may_writeback && !dip_reserves)) { > + dip_reserves = true; > + goto retry; > + } [Severity: High] If ttm_backup_backup_folio() fails with -ENOMEM on the first subpage (!nr_backed) of a high-order compound page (order > 0), doesn't jumping directly to retry re-invoke ttm_backup_backup_folio() at the full compound order with __GFP_NOMEMALLOC cleared instead of splitting the compound into order-0 pages first? When system free memory is below normal zone watermarks during reclaim, the initial __GFP_NOMEMALLOC allocation fails on subpage 0 with handle == -ENOMEM and nr_backed == 0. Because ttm_pool_split_for_swap() is only called further down when nr_backed > 0 and nr_backed < npages, retrying at the original order forces ttm_backup_backup_folio() to allocate up to 1 << order shmem pages from kernel memory reserves before returning to ttm_pool_backup() where the source compound page can finally be freed. Should this path split a high-order page via ttm_pool_split_for_swap(pool, page) and reset order = 0 and npages = 1 before retrying with reserves, as described in the struct ttm_backup_flags kerneldoc for may_writeback, so each order-0 subpage is backed up and freed before allocating the next shmem page? [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_shrinker.c > b/drivers/gpu/drm/xe/xe_shrinker.c > index c86455ea08d18..c24f35cbc2358 100644 > --- a/drivers/gpu/drm/xe/xe_shrinker.c > +++ b/drivers/gpu/drm/xe/xe_shrinker.c > @@ -171,6 +171,7 @@ static int __xe_shrinker_walk(struct xe_shrinker > *shrinker, > static int xe_shrinker_walk(struct xe_shrinker *shrinker, > struct ttm_operation_ctx *ctx, > const struct xe_bo_shrink_flags flags, > + bool allow_writeback, > unsigned long to_scan, unsigned long *scanned, > unsigned long *freed) > { > @@ -179,7 +180,8 @@ static int xe_shrinker_walk(struct xe_shrinker *shrinker, > int ret; > > swap(no_wait_gpu, ctx->no_wait_gpu); > - save_flags.writeback = false; > + save_flags.may_writeback = false; > + save_flags.force_writeback = false; > ret = __xe_shrinker_walk(shrinker, ctx, save_flags, to_scan, scanned, > freed); > swap(no_wait_gpu, ctx->no_wait_gpu); > @@ -187,15 +189,32 @@ static int xe_shrinker_walk(struct xe_shrinker > *shrinker, > return ret; > > if (!ctx->no_wait_gpu) { > + /* > + * Blocking is now tolerated: allow retrying a failed backup > + * allocation using kernel reserves, but only if writeback is > + * permitted at all in this gfp context (i.e. __GFP_IO is > + * set); see @allow_writeback in xe_shrinker_scan(). > + */ > + save_flags.may_writeback = allow_writeback; > ret = __xe_shrinker_walk(shrinker, ctx, save_flags, to_scan, > scanned, > freed); > if (ret || *scanned >= to_scan) > return ret; > } > > - if (flags.writeback) > - ret = __xe_shrinker_walk(shrinker, ctx, flags, to_scan, scanned, > + if (allow_writeback) { > + struct xe_bo_shrink_flags wb_flags = flags; > + > + /* > + * Writeback explicitly allowed: also force writeback on any > + * retry so it actually relieves memory pressure instead of > + * just growing dirty shmem. > + */ > + wb_flags.may_writeback = true; > + wb_flags.force_writeback = true; > + ret = __xe_shrinker_walk(shrinker, ctx, wb_flags, to_scan, > scanned, > freed); > + } [Severity: High] Can xe_shrinker_walk() terminate after the first pass with 0 freed pages when __GFP_NOMEMALLOC shmem allocations fail under memory pressure, without ever reaching the passes that set may_writeback or force_writeback? In xe_bo_shrink(), *scanned is incremented by the full old_remaining page count before calling ttm_bo_shrink(): drivers/gpu/drm/xe/xe_bo.c:xe_bo_shrink() { old_remaining = ttm_tt_remaining_pages(tt); *scanned += old_remaining; lret = ttm_bo_shrink(ctx, bo, (struct ttm_bo_shrink_flags) {.purge = false, .allow_move = true, .may_writeback = flags.may_writeback, .force_writeback = flags.force_writeback}); } During the first pass of xe_shrinker_walk() (where save_flags.may_writeback is false), if shmem_read_folio_gfp() with __GFP_NOMEMALLOC fails with -ENOMEM on an idle BO, ttm_pool_backup() stops without retrying and ttm_bo_shrink() returns -EBUSY (or a partial count if it stopped mid-BO). In __xe_shrinker_walk(), lret == -EBUSY continues to the next BO on the LRU while keeping old_remaining added to *scanned. Once Pass 1 returns, *scanned >= to_scan is satisfied and xe_shrinker_walk() returns immediately without running Pass 2 or Pass 3. Additionally, if Pass 1 visits fewer than to_scan pages in idle BOs and reaches Pass 2 with allow_writeback set to true, Pass 2 runs with save_flags.may_writeback = true and save_flags.force_writeback = false. Won't Pass 2 dip into kernel memory reserves to grow dirty shmem without writeback and satisfy *scanned >= to_scan before Pass 3 (where force_writeback = true) can run? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
