Hi Andi, thanks for reviewing!
On 2026-07-01 at 16:38:11 +0200, Andi Shyti wrote: > Hi Krzysztof, > > On Wed, Jul 01, 2026 at 10:44:35AM +0000, Krzysztof Karas wrote: > > shmem_sg_alloc_table is a complex and hard to read function. > > Split its logic into smaller pieces to improve readability and > > reduce indentation. Change the main "for" loop into "while" to > > get rid of obscure iterator "i" and be more explicit in > > traversing scatterlist. > > any chance we can split this cleanup into smaller pieces? Yeah, I'll work something out. > > > Signed-off-by: Krzysztof Karas <[email protected]> > > ... > > > +static struct folio *shmem_shrink_get_folio(struct address_space *mapping, > > + unsigned long folio_index, > > + gfp_t gfp, unsigned int pages_left, > > + struct drm_i915_private *i915) > > +{ > > +#define MAX_READS 2 > > This MAX_READS here is very ugly! Just use 2 and explain it in a > comment. In the 'if' below you can check out of "if (... || i)" > and still explain it in a comment. If we are on the topic of personal preferences, I'd prefer moving this to a variable instead of leaving a magic number buried in the code. The comment is unnecessary if you figure out what this loop does and in the end "2" is just a number somebody picked way back. > > > + struct folio *folio; > > + unsigned int i; > > + > > + for (i = 0; i < MAX_READS; i++) { > > + cond_resched(); > > + folio = shmem_read_folio_gfp(mapping, folio_index, gfp); > > + if (!IS_ERR(folio) || i == MAX_READS - 1) > > + return folio; > > + > > + i915_gem_shrink(NULL, i915, 2 * pages_left, NULL, > > /pages_left/page_count/ I mean, sure, but is there a reason for using "count" instead of "left"? > > > + I915_SHRINK_BOUND | I915_SHRINK_UNBOUND); > > + > > ... > > > + } > > + > > + /* Should never happen */ > > + WARN_ON_ONCE(1); > > no need. Okay, I'll remove it. > > Thanks, > Andi > > > + return ERR_PTR(-EINVAL); > > +} -- Best Regards, Krzysztof
