Hi, the general idea of this patch looks sensible. A few naming/style nitpicks below:
On Wednesday, 4 November 2026 21:13 Thomas Hellström wrote: > Backup of a page allocates the shmem destination folio with > __GFP_NOMEMALLOC for high-order pages, to avoid dipping into kernel > memory reserves, but order-0 pages were always backed up without it. > When a high-order allocation fails mid-compound, the compound is split > and the remaining subpages are individually retried at order 0 without > __GFP_NOMEMALLOC. > > That retry happened unconditionally, regardless of whether the caller > wanted writeback or could tolerate blocking. Retrying without > writeback only grows dirty shmem rather than relieving the memory > pressure that caused the original failure, and the retry itself may > need to reclaim or block. > > Back up all pages with __GFP_NOMEMALLOC first, and add two flags, > may_writeback and force_writeback, to struct ttm_backup_flags, > replacing the single writeback flag: > - may_writeback governs whether a failed backup allocation is retried > without __GFP_NOMEMALLOC, splitting a high-order page if needed. > When clear, the backup stops at the failure, keeping whatever > progress was already made and leaving the remainder resident for a > later retry. > - force_writeback replaces the old writeback flag and is honoured both > for the regular backup and for any retry, so a caller that allows the > retry can also choose to force it to actually complete and relieve > memory pressure instead of just relocating it. Can you explain why you'd call it "may_writeback"? The name implies that no writeback is ever performed when unset, which isn't really accurate. AFAICT, all this flag really does is govern how hard we try to allocate backup shmem, and force_writeback still is the only thing that determines whether to immediately do a writeback. Or am I misunderstanding? Maybe replace may_writeback with something like use_kernel_reserves. Renaming writeback to force_writeback doesn't seem necessary as the semantics are still the exact same, so that one can just stay. > > Thread the two new flags all the way from the xe shrinker down through > xe_bo_shrink_flags and ttm_bo_shrink_flags to ttm_backup_flags. > xe_shrinker_walk() now takes an explicit allow_writeback > parameter (computed once from __GFP_IO and no_wait_gpu in > xe_shrinker_scan()) rather than carrying that gfp-context permission > inside the per-call flags, and uses it to decide, pass by pass, whether > the retry is allowed (may_writeback) and whether it should be > forced to completion (force_writeback). > > Signed-off-by: Thomas Hellström <[email protected]> > --- > drivers/gpu/drm/ttm/ttm_bo_util.c | 3 +- > drivers/gpu/drm/ttm/ttm_pool.c | 47 +++++++++++++++++++++---------- > drivers/gpu/drm/xe/xe_bo.c | 6 ++-- > drivers/gpu/drm/xe/xe_bo.h | 9 ++++-- > drivers/gpu/drm/xe/xe_shrinker.c | 39 ++++++++++++++++++++----- > include/drm/ttm/ttm_bo.h | 9 ++++-- > include/drm/ttm/ttm_tt.h | 13 +++++++-- > 7 files changed, 93 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c > b/drivers/gpu/drm/ttm/ttm_bo_util.c > index 975dea33075f..21317c212c27 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c > @@ -1122,7 +1122,8 @@ long ttm_bo_shrink(struct ttm_operation_ctx *ctx, > struct ttm_buffer_object *bo, > > lret = ttm_tt_backup(bdev, bo->ttm, (struct ttm_backup_flags) > {.purge = flags.purge, > - .writeback = flags.writeback}); > + .may_writeback = flags.may_writeback, > + .force_writeback = flags.force_writeback}); > > if (lret > 0 && !ttm_tt_is_partial(bo->ttm)) { > spin_lock(&bdev->lru_lock); > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index 8f05257e4f54..87c1ac39af94 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c > @@ -1158,7 +1158,10 @@ static void ttm_pool_restore_abort(struct ttm_tt *tt) > * If backup of a page fails for whatever reason, @ttm will still be > * partially backed up, retaining those pages for which backup fails. > * In that case, this function can be retried, possibly after freeing up > - * memory resources. > + * memory resources. In particular, if allocating backup memory fails > + * without dipping into kernel reserves, and @flags->may_writeback is not > + * set, the backup stops at that point, also leaving @ttm partially backed > + * up. > * > * Return: Number of pages actually backed up or freed, or negative > * error code on error. > @@ -1174,6 +1177,7 @@ 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 dip_reserves; It's a bit nitpicky, but I'm not sure I like the boolean here (in particular, the ternary at high levels of indentation is uncomfortable to read). How about removing the boolean and doing: > bool partial = ttm_tt_is_partial(tt); > > if (WARN_ON(ttm_tt_is_backed_up(tt))) > @@ -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; alloc_gfp = GFP_KERNEL | __GFP_HIGH | __GFP_NOWARN | __GFP_RETRY_MAYFAIL; > > 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; alloc_gfp |= __GFP_NOMEMALLOC; > /* > * 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); order, flags->force_writeback, i, gfp, alloc_gfp, &nr_backed); > + if (unlikely(handle == -ENOMEM && !nr_backed && > + flags->may_writeback && !dip_reserves)) { > + dip_reserves = true; > + goto retry; > + } if (unlikely(handle == -ENOMEM && !nr_backed && flags->may_writeback && (alloc_gfp & __GFP_NOMEMALLOC))) { alloc_gfp &= ~__GFP_NOMEMALLOC; goto retry; } This is basically what the original logic already did, too. I think I liked that better. Natalie
