Re: [PATCH 05/10] drm/ttm: switch to ttm_bo_lru_for_each_reserved_guarded for swapout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] An unconditional `continue;` inside the `ttm_device_swapout` loop bypasses return value checking and early return logic.
- [Medium] The modified `ttm_bo_swapout` signature and removed export breaks KUnit tests, causing a compilation failure.
--

commit d0589786d39c3dde940e7f100cb47e8cc913ab55
Author: Christian König <christian.koenig@amd.com>

drm/ttm: switch to ttm_bo_lru_for_each_reserved_guarded for swapout

This commit refactors the TTM buffer object swapout logic to use
ttm_bo_lru_for_each_reserved_guarded instead of the wrapper walk.
It simplifies ttm_bo_swapout by removing the target page count
and resource manager arguments.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 1fb8c53da0362..24c52df169ac8 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
[ ... ]
> @@ -1183,37 +1176,6 @@ ttm_bo_swapout_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *bo)
>  	return ret;
>  }
>  
> -/**
> - * ttm_bo_swapout() - Swap out buffer objects on the LRU list to shmem.
> - * @bdev: The ttm device.
> - * @ctx: The ttm_operation_ctx governing the swapout operation.
> - * @man: The resource manager whose resources / buffer objects are
> - * goint to be swapped out.
> - * @gfp_flags: The gfp flags used for shmem page allocations.
> - * @target: The desired number of pages to swap out.
> - *
> - * Return: The number of pages actually swapped out, or negative error code
> - * on error.
> - */
> -s64 ttm_bo_swapout(struct ttm_device *bdev, struct ttm_operation_ctx *ctx,
> -		   struct ttm_resource_manager *man, gfp_t gfp_flags,
> -		   s64 target)
> -{
[ ... ]
> -}
> -EXPORT_SYMBOL_FOR_TESTS_ONLY(ttm_bo_swapout);

[Severity: Medium]
Does removing this export and changing the function signature break KUnit
tests?

It looks like ttm_bo_validate_test_cases() in 
drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c still calls this function 
using the previous 5-argument signature, which could break the build when
CONFIG_DRM_TTM_KUNIT_TEST is enabled.

> -
>  void ttm_bo_tt_destroy(struct ttm_buffer_object *bo)
>  {
>  	if (bo->ttm == NULL)
> diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c
> index d3bfb9a696a74..e4188e2ee7ab1 100644
> --- a/drivers/gpu/drm/ttm/ttm_device.c
> +++ b/drivers/gpu/drm/ttm/ttm_device.c
[ ... ]
> @@ -179,10 +185,15 @@ int ttm_device_swapout(struct ttm_device *bdev, struct ttm_operation_ctx *ctx,
>  		if (!man || !man->use_tt)
>  			continue;
>  
> -		lret = ttm_bo_swapout(bdev, ctx, man, gfp_flags, 1);
> -		/* Can be both positive (num_pages) and negative (error) */
> -		if (lret)
> -			return lret;
> +		ttm_bo_lru_for_each_reserved_guarded(&cursor, man, &arg, bo) {
> +			lret = ttm_bo_swapout(bo, ctx, gfp_flags);
> +				continue;

[Severity: High]
Does this unconditional continue make the error checking and return logic
below unreachable?

Because the loop skips to the next iteration immediately, it will
indiscriminately swap out every buffer object on the LRU list regardless of
the target pages and ignore any return values. Callers like
ttm_global_swapout() would interpret a 0 return value as failure to swap,
which could cause loops in ttm_tt_populate() to terminate prematurely even
if memory limits are exceeded.

> +			/* Can be both positive (num_pages) and negative (error) */
> +			if (lret && lret != -EBUSY && lret != -EALREADY)
> +				return lret;
> +		}
> +		if (IS_ERR(bo))
> +			return PTR_ERR(bo);
>  	}
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-ttm_2_drm_exec-v1-0-43685ac1286b@gmx.de?part=5