From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/xe: Avoid shrinker reclaim from kswapd under fragmentation Date: Tue, 05 May 2026 10:13:42 +1000 Message-ID: In-Reply-To: <20260430182335.2132382-7-matthew.brost@intel.com> References: <20260430182335.2132382-1-matthew.brost@intel.com> <20260430182335.2132382-7-matthew.brost@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Overall:** The payoff patch that uses the helper from patch 4 in the Xe s= hrinker. ```c if (nr_scanned >=3D nr_to_scan || !can_backup) goto out; + if (ttm_bo_shrink_kswap_maybe_fragmented(sc->nid, sc->order)) + goto out; + /* If we didn't wake before, try to do it now if needed. */ if (!runtime_pm) ``` **Concerns:** - **Placement is significant:** The check is placed *after* purgeable objec= t scanning (lines 228-233) but *before* the non-purgeable (active BO backup= /eviction) walk (lines 243-248). This means purgeable objects will still be= reclaimed under fragmentation, but active BOs won't be evicted. This is ex= actly the right design =E2=80=94 purgeable objects are cheap to re-create, = while evicting active BOs triggers the expensive rebind loop described in t= he cover letter. - **Return value:** When this `goto out` triggers, the function returns eit= her `freed` (if something was purged) or `SHRINK_STOP` (if `nr_scanned =3D= =3D 0`). Returning `SHRINK_STOP` tells the mm core to stop invoking this sh= rinker. This seems appropriate =E2=80=94 the shrinker is saying "I can't he= lp, don't call me again for this reclaim cycle." - **No tracing/debugging:** When the shrinker bails out due to fragmentatio= n, there's no trace event or debug print. For a heuristic that skips potent= ially useful reclaim, it would be valuable to have a trace point (even if b= ehind `CONFIG_DEBUG` or xe's own debug flags) so developers can observe whe= n this kicks in. Not a blocker, but worth considering. - **Missing Reviewed-by:** Unlike patches 3 and 5, this patch has no Review= ed-by tag. Given it directly changes Xe shrinker behavior, getting Thomas H= ellstr=C3=B6m's or Matthew Auld's review would strengthen it. --- **Summary:** The series is well-structured and addresses a real problem wit= h measurable results. The main areas for scrutiny are: (1) the `~__GFP_RECL= AIM` change in patch 3 which suppresses kswapd wakeup =E2=80=94 this is a s= tronger change than the commit message suggests, (2) the mm core changes (p= atches 1-2) need mm maintainer buy-in, and (3) the fragmentation heuristic = is intentionally coarse which is fine for a first iteration but could benef= it from being compared against the beneficial_order rather than just `order= !=3D 0`. --- Generated by Claude Code Patch Reviewer