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

[Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>]

On Mon, 2026-07-13 at 09:58 +0000, Krzysztof Karas wrote:
> Continue unloading shmem_sg_alloc_table by placing reading
> folios and shrink call into a new helper.
> Make the loop a bit more reader-friendly by removing iteration
> over a structure and replacing it with a do-while loop.
> 
> Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
> ---
> v3:
>  * Split refactoring and put it after the fix in shmem folio
>   counting suggested by Andi.
>  * Use do-while loop suggested by Robin. 
> 
>  drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 102 ++++++++++++----------
>  1 file changed, 55 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 4a61b012fb6f..7c8de8fe0a22 100644
> --- 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)) {

Going again through then unused shrinking and modification of gfp doesn't 
make sense, I believe.  Could be avoided based on retries value as an 
additional condition.

Thanks,
Janusz


> +			i915_gem_shrink(NULL, i915, 2 * page_count, NULL,
> +					I915_SHRINK_BOUND | I915_SHRINK_UNBOUND);
> +
> +			/*
> +			 * We've tried hard to allocate the memory by reaping
> +			 * our own buffer, now let the real VM do its job and
> +			 * go down in flames if truly OOM.
> +			 *
> +			 * However, since graphics tend to be disposable,
> +			 * defer the oom here by reporting the ENOMEM back
> +			 * to userspace.
> +			 *
> +			 * Reclaim and warn, but no oom.
> +			 */
> +			gfp = mapping_gfp_mask(mapping);
> +
> +			/*
> +			 * Our bo are always dirty and so we require
> +			 * kswapd to reclaim our pages (direct reclaim
> +			 * does not effectively begin pageout of our
> +			 * buffers on its own). However, direct reclaim
> +			 * only waits for kswapd when under allocation
> +			 * congestion. So as a result __GFP_RECLAIM is
> +			 * unreliable and fails to actually reclaim our
> +			 * dirty pages -- unless you try over and over
> +			 * again with !__GFP_NORETRY. However, we still
> +			 * want to fail this allocation rather than
> +			 * trigger the out-of-memory killer and for
> +			 * this we want __GFP_RETRY_MAYFAIL.
> +			 */
> +			gfp |= __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> +		}
> +	} while (IS_ERR(folio) && --retries);
> +
> +	return folio;
> +}
> +
>  int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  			 size_t size, struct intel_memory_region *mr,
>  			 struct address_space *mapping,
> @@ -117,57 +166,16 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  	for (i = 0; i < page_count; i++) {
>  		unsigned long folio_page_index = 0;
>  		unsigned long nr_pages;
> -		const unsigned int shrink[] = {
> -			I915_SHRINK_BOUND | I915_SHRINK_UNBOUND,
> -			0,
> -		}, *s = shrink;
>  		gfp_t gfp = noreclaim;
>  
>  		/* Grab the next folio if we exhausted the current one. */
>  		if (!i || i > folio_end) {
> -			do {
> -				cond_resched();
> -				folio = shmem_read_folio_gfp(mapping, i, gfp);
> -				if (!IS_ERR(folio))
> -					break;
> -
> -				if (!*s) {
> -					ret = PTR_ERR(folio);
> -					goto err_sg;
> -				}
> -
> -				i915_gem_shrink(NULL, i915, 2 * page_count, NULL, *s++);
> -
> -				/*
> -				* We've tried hard to allocate the memory by reaping
> -				* our own buffer, now let the real VM do its job and
> -				* go down in flames if truly OOM.
> -				*
> -				* However, since graphics tend to be disposable,
> -				* defer the oom here by reporting the ENOMEM back
> -				* to userspace.
> -				*/
> -				if (!*s) {
> -					/* reclaim and warn, but no oom */
> -					gfp = mapping_gfp_mask(mapping);
> -
> -					/*
> -					 * Our bo are always dirty and so we require
> -					 * kswapd to reclaim our pages (direct reclaim
> -					 * does not effectively begin pageout of our
> -					 * buffers on its own). However, direct reclaim
> -					 * only waits for kswapd when under allocation
> -					 * congestion. So as a result __GFP_RECLAIM is
> -					 * unreliable and fails to actually reclaim our
> -					 * dirty pages -- unless you try over and over
> -					 * again with !__GFP_NORETRY. However, we still
> -					 * want to fail this allocation rather than
> -					 * trigger the out-of-memory killer and for
> -					 * this we want __GFP_RETRY_MAYFAIL.
> -					 */
> -					gfp |= __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> -				}
> -			} while (1);
> +			folio = shmem_shrink_get_folio(mapping, i, gfp,
> +						       page_count, i915);
> +			if (IS_ERR(folio)) {
> +				ret = PTR_ERR(folio);
> +				goto err_sg;
> +			}
>  
>  			folio_start = folio_pgoff(folio);
>  			folio_end = folio_start + folio_nr_pages(folio) - 1;