Re: [PATCH v3 4/5] drm/i915/gem: Read and shrink memory in a separate function

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unnecessary double execution of the memory shrinker on the allocation failure path.
--

--- Patch [4]: [PATCH v3 4/5] drm/i915/gem: Read and shrink memory in a separate function ---
commit 323006b149f5bd15e61d31522a7aa9cdbf860e09
Author: Krzysztof Karas <krzysztof.karas@intel.com>

drm/i915/gem: Read and shrink memory in a separate function

This commit extracts the memory reading and shrinking logic from
shmem_sg_alloc_table into a new shmem_shrink_get_folio helper. It also
replaces the previous array-based iteration with a do-while loop.

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
[ ... ]
> @@ -78,6 +78,55 @@ static int validate_size(size_t size, unsigned int page_count,
>  	return 0;
>  }
>  
> +static struct folio *shmem_shrink_get_folio(struct address_space *mapping,
> +					    unsigned long folio_index,
> +					    gfp_t gfp, unsigned int page_count,
> +					    struct drm_i915_private *i915)
> +{
> +	struct folio *folio = NULL;
> +	unsigned int retries = 2;
> +
> +	do {
> +		cond_resched();
> +		folio = shmem_read_folio_gfp(mapping, folio_index, gfp);
> +		if (IS_ERR(folio)) {
> +			i915_gem_shrink(NULL, i915, 2 * page_count, NULL,
> +					I915_SHRINK_BOUND | I915_SHRINK_UNBOUND);

[Severity: Medium]
Will this result in an unnecessary double execution of the memory shrinker
when allocations fail?

In the original code, the array-based state machine checked the shrink state
and jumped to the error path before calling i915_gem_shrink again on the
final failure. With the refactored do-while loop, if the second
shmem_read_folio_gfp call fails, the loop unconditionally executes the
shrinker a second time before decrementing the retries counter to 0 and
exiting.

Could this waste CPU cycles and increase lock contention on i915->mm.obj_lock
immediately before returning -ENOMEM to the caller, considering the memory
freed by this second pass is completely ignored?

> +
> +			/*
> +			 * We've tried hard to allocate the memory by reaping
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713095812.1014365-1-krzysztof.karas@intel.com?part=4