Re: [PATCH 3/4] drm/ttm, drm/xe: Introduce TTM_TT_FLAG_PARTIAL for partially backed-up tts

[sashiko-bot@kernel.org]

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

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