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-20 at 08:18 +0000, Krzysztof Karas wrote:
> Hi Janusz,
> 
> On 2026-07-15 at 17:31:56 +0200, Janusz Krzysztofik wrote:
> > 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.
> If calling i915_gem_shrink again doesn't give us anything, then
> looping doesn not really benefit us here. We could do something
> like this instead:
> 
> folio = shmem_read_folio_gfp(...);
> if (IS_ERR(folio)) {
> 	i915_gem_shrink(...);
> 	gfp = mapping_gfp_mask(mapping);
> 	gfp |= __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> 	/* again */
> 	folio = shmem_read_folio_gfp(...);
> }
> 
> return folio;
> 
> That way we'd be explicit about shrinking once and retrying
> folio reading only once. Reduced indentation would be added
> bonus.
> 
> What do you think?

Yes, that would be much more clear.  I would only keep the cond_resched(), 
at least before retrying, unless you can justify its removal.

Thanks,
Janusz