Re: [PATCH 4/4] drm/ttm, drm/xe: Gate reserve-dipping backup retries on may_writeback

[sashiko-bot@kernel.org]

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 <thomas.hellstrom@linux.intel.com>

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/20261104201331.2462-1-thomas.hellstrom@linux.intel.com?part=4