Re: [PATCH v2 1/2] drm/xe: Track number of populated ttm_tts in the shrinker

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Double subtraction of shrinker accounts (including the new populated_tts) during nested external pins.
--

--- Patch [1]: [PATCH v2 1/2] drm/xe: Track number of populated ttm_tts in the shrinker ---
commit 96b66a44985d6775af6634dd1b400406d1705547
Author: Thomas Hellström <[email protected]>

drm/xe: Track number of populated ttm_tts in the shrinker

This commit introduces a populated_tts counter to the xe shrinker to track
the number of shrinkable or purgeable ttm_tt objects. It modifies
xe_ttm_tt_account_add() and xe_ttm_tt_account_subtract() to increment and
decrement this new counter alongside existing page accounting. Additionally,
it updates xe_shrinker_mod_pages() to handle the new tts delta.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index dde309821237b..0ddc1d13d6ba6 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -450,9 +450,9 @@ static void xe_ttm_tt_account_subtract(struct xe_device *xe, struct ttm_tt *tt)
>  	struct xe_ttm_tt *xe_tt = container_of(tt, struct xe_ttm_tt, ttm);
>  
>  	if (xe_tt->purgeable)
> -		xe_shrinker_mod_pages(xe->mem.shrinker, 0, -(long)tt->num_pages);
> +		xe_shrinker_mod_pages(xe->mem.shrinker, 0, -(long)tt->num_pages, -1);
>  	else
> -		xe_shrinker_mod_pages(xe->mem.shrinker, -(long)tt->num_pages, 0);
> +		xe_shrinker_mod_pages(xe->mem.shrinker, -(long)tt->num_pages, 0, -1);
>  }

[Severity: High]
This is a pre-existing issue, but won't the new populated_tts counter also
underflow here when external DMA-bufs are pinned multiple times?

In xe_bo_pin_external(), the BO is unconditionally subtracted from the
shrinker tracking even if it is already pinned:

	ttm_bo_pin(&bo->ttm);
	if (bo->ttm.ttm && ttm_tt_is_populated(bo->ttm.ttm))
		xe_ttm_tt_account_subtract(xe, bo->ttm.ttm);

Similarly, xe_bo_unpin_external() unconditionally adds it back without
checking if bo->ttm.pin_count actually reached 0:

	ttm_bo_unpin(&bo->ttm);
	if (bo->ttm.ttm && ttm_tt_is_populated(bo->ttm.ttm))
		xe_ttm_tt_account_add(xe, bo->ttm.ttm);

If a userspace process imports an external DMA-buf and maps/pins it multiple
times, xe_ttm_tt_account_subtract() is called for every pin, and
xe_ttm_tt_account_add() for every unpin. Since xe_ttm_tt_account_add() was
only called once upon initial population, the nested subtractions cause an
underflow.

Could this lead to the shrinker fields, including the new populated_tts,
becoming massive positive numbers when evaluated as unsigned long in
xe_shrinker_count(), potentially misleading kswapd to continuously scan
and consume 100% CPU?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1