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 <[email protected]> > > > --- > > > 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
