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/ttm: Issue direct reclaim at beneficial_order Date: Tue, 05 May 2026 10:13:41 +1000 Message-ID: In-Reply-To: <20260430182335.2132382-4-matthew.brost@intel.com> References: <20260430182335.2132382-1-matthew.brost@intel.com> <20260430182335.2132382-4-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:** This is the most impactful patch in terms of allocation behavi= or change. **Key change:** ```c - if (beneficial_order && order > beneficial_order) - gfp_flags &=3D ~__GFP_DIRECT_RECLAIM; + if (order && beneficial_order && order !=3D beneficial_order) + gfp_flags &=3D ~__GFP_RECLAIM; ``` **Concerns:** - **`~__GFP_RECLAIM` vs `~__GFP_DIRECT_RECLAIM`:** The old code only suppre= ssed direct reclaim (keeping kswapd wakeup possible). The new code clears `= __GFP_RECLAIM` entirely (`__GFP_DIRECT_RECLAIM | __GFP_KSWAPD_RECLAIM`), me= aning these allocations won't even wake kswapd. This is a substantial seman= tic change. The commit message says "issuing direct reclaim or triggering k= swap at a lower order than beneficial_order is ineffective" =E2=80=94 this = is true for the driver, but kswapd serves the entire system, not just this = driver. Suppressing kswapd wakeup from TTM allocations could delay legitima= te system-wide reclaim. This deserves more justification or at least acknow= ledgment in the commit message. - **Orders below beneficial_order:** The old condition was `order > benefic= ial_order` =E2=80=94 only orders *above* the sweet spot lost direct reclaim= . The new condition `order !=3D beneficial_order` also covers orders *below= * beneficial_order. Combined with the broader `~__GFP_RECLAIM`, this means = a TTM order-4 allocation on a system with beneficial_order=3D9 will now be = `__GFP_NORETRY | __GFP_NOWARN | __GFP_THISNODE` with NO reclaim at all. If = this allocation fails, TTM falls back to smaller orders, and the order-0 fa= llback (`if (order)` is false for order 0) will still get full reclaim. But= intermediate orders between 1 and beneficial_order-1 also lose all reclaim= . Is that intended? The commit message only discusses orders above and at b= eneficial_order, not below. - **Interaction with existing `__GFP_NORETRY`:** Note that for `order > 0`,= the function already sets `__GFP_NORETRY` at line 161. Combined with clear= ing `__GFP_RECLAIM`, these allocations become pure "try once, fail fast" = =E2=80=94 which is probably fine since TTM has its own fallback path, but i= t's worth calling out. - **Reviewed-by from Christian Koenig** is reassuring since he understands = TTM pool behavior deeply. --- Generated by Claude Code Patch Reviewer