On 02/07/2026 7:18 am, Krzysztof Karas wrote:
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.
FWIW since I'm looking at the thread - indeed if it's just a retry loop
and the actual index value isn't significant, then often a count-down
loop can be the most self-explanatory, e.g.:
int retries = 2;
while (retries--) {
...
}
Or perhaps in this case:
do {
folio = shmem_read_folio();
if (IS_ERR(folio))
i915_gem_shrink();
while (!IS_ERR(folio) && --retries);
return folio;
Cheers,
Robin.
+ 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);
+}
