Re: [PATCH v2 1/2] drm/xe: Fix shrinker accounting double-subtraction on nested external pins

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm: permanent shrinker accounting leak for backup BOs
--

commit 816b78b38575a1b5008feed73113ef8809607017
Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>

drm/xe: Fix shrinker accounting double-subtraction on nested external pins

This commit fixes a double-subtraction in the shrinker accounting when
handling nested external pins by introducing xe_bo_pin_account() and
xe_bo_unpin_account(). These wrappers ensure shrinker pages are only updated
during the initial pin and final unpin transitions.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index f2ab9bf43a864..2fbbba7cf4b00 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -465,6 +465,66 @@ static void xe_ttm_tt_account_subtract(struct xe_device *xe, struct ttm_tt *tt)
>  		xe_shrinker_mod_pages(xe->mem.shrinker, -(long)tt->num_pages, 0);
>  }
>  
> +/*
> + * Account @bo's pages as pinned for the shrinker. Removes @bo's pages
> + * from the shrinker's shrinkable / purgeable counts on the transition
> + * from unpinned to pinned. Must be called with @bo's dma-resv held,
> + * after &ttm_buffer_object.pin_count has been incremented by
> + * ttm_bo_pin(). Safe to call unconditionally regardless of which pin
> + * path (kernel, external, framebuffer, backup object, ...) is pinning
> + * @bo, since it only acts on the true 0->1 pin_count transition.
> + */
> +static void xe_bo_account_pin(struct xe_bo *bo)
> +{
> +	struct xe_device *xe = xe_bo_device(bo);
> +
> +	if (bo->ttm.pin_count == 1 && bo->ttm.ttm && ttm_tt_is_populated(bo->ttm.ttm))
> +		xe_ttm_tt_account_subtract(xe, bo->ttm.ttm);

[Severity: High]
This isn't a bug introduced by this patch, but does this logic cause a
shrinker accounting underflow when pinning imported dma-bufs?

When an imported dma-buf is pinned (for example, through framebuffer creation
or external buffer sharing), xe_bo_pin_account() is called, which evaluates
ttm_tt_is_populated(). For imported dma-bufs, the TTM core sets this flag.

However, xe_ttm_tt_populate() returns early for imported dma-bufs:

xe_ttm_tt_populate() {
    if ((tt->page_flags & TTM_TT_FLAG_EXTERNAL) &&
        !(tt->page_flags & TTM_TT_FLAG_EXTERNAL_MAPPABLE))
            return 0;
    ...
}

Because they return early, they are never added to the shrinker in the first
place. 

Could subtracting them here unconditionally when ttm_tt_is_populated() is true
subtract pages that were never added, causing an unsigned underflow of
shrinkable_pages?

> +}
> +

[ ... ]

> @@ -1396,7 +1456,7 @@ int xe_bo_notifier_prepare_pinned(struct xe_bo *bo)
>  		}
>  
>  		backup->parent_obj = xe_bo_get(bo); /* Released by bo_destroy */
> -		ttm_bo_pin(&backup->ttm);
> +		xe_bo_pin_account(backup);

[Severity: Medium]
Does this introduce a permanent shrinker accounting leak for backup BOs?

In xe_bo_notifier_prepare_pinned(), a new system memory backup BO is created
and immediately pinned here via the new xe_bo_pin_account() helper. At this
point, the BO's TT is not yet populated (as ttm_bo_move_null() defers it),
so xe_bo_account_pin() skips subtracting the pages:

xe_bo_account_pin() {
    if (bo->ttm.pin_count == 1 && bo->ttm.ttm &&
        ttm_tt_is_populated(bo->ttm.ttm))
            xe_ttm_tt_account_subtract(xe, bo->ttm.ttm);
}

Later, when the BO is populated (for example, during eviction copy), it is
added to the shrinker via xe_ttm_tt_populate().

When the BO is eventually unpinned in xe_bo_restore_pinned() or
xe_bo_notifier_unprepare_pinned(), xe_bo_account_unpin() sees that it is now
populated and adds the pages to the shrinker again. 

Can this double-addition permanently inflate the shrinkable_pages counter?

>  		bo->backup_obj = backup;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001134018.111553-1-thomas.hellstrom@linux.intel.com?part=1