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: Thu, 23 Apr 2026 09:01:58 +1000 Message-ID: In-Reply-To: <20260421012608.1474950-2-matthew.brost@intel.com> References: <20260421012608.1474950-1-matthew.brost@intel.com> <20260421012608.1474950-2-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 This patch changes the reclaim behavior in `ttm_pool_alloc_page()` for non-= beneficial orders. **The 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; ``` There are two distinct changes here that should be clearly called out: **1. Condition broadened from `order > beneficial_order` to `order !=3D ben= eficial_order`:** The old code only disabled reclaim for orders *above* beneficial_order. The= new code also disables reclaim for orders *below* beneficial_order (e.g., = orders 1-8 when beneficial_order is 9). The rationale in the commit message= =E2=80=94 "issuing direct reclaim or triggering kswap at a lower order tha= n beneficial_order is ineffective, since the driver does not benefit from r= eclaiming lower-order pages" =E2=80=94 is correct but understates the signi= ficance. This is a meaningful behavioral change for the entire fallback pat= h. Looking at the allocation loop in `__ttm_pool_alloc()` (ttm_pool.c:786-829)= , when order-9 allocation fails, the allocator falls back through orders 8,= 7, ... 1, 0. Under the old code, these fallback orders would still trigger= reclaim. Under the new code, only order 0 retains reclaim. This is correct= in the fragmentation scenario (plenty of free pages, just not contiguous),= but worth verifying that it doesn't hurt the genuine-memory-pressure scena= rio where even order-0 allocations are tight. In the genuine pressure case: order 9 with `__GFP_NORETRY` would attempt li= mited reclaim and fail, orders 8-1 would fail immediately (no reclaim), and= order 0 would succeed with full reclaim (no `__GFP_NORETRY`, full `__GFP_R= ECLAIM`). This seems acceptable since order-0 is the ultimate fallback and = handles true pressure. **2. Flag change from `~__GFP_DIRECT_RECLAIM` to `~__GFP_RECLAIM`:** `__GFP_RECLAIM` is `__GFP_DIRECT_RECLAIM | __GFP_KSWAPD_RECLAIM` (gfp_types= .h:259). The old code only disabled synchronous direct reclaim but still al= lowed waking kswapd. The new code disables *both*. This is more aggressive = =E2=80=94 non-beneficial-order allocations won't even wake kswapd. The comm= it message says "direct reclaim should only be issued with __GFP_NORETRY at= exactly beneficial_order" but doesn't explicitly mention that kswapd wakeu= p is also now suppressed for all non-beneficial orders. **Suggestion:** The commit message should explicitly call out both changes:= (a) reclaim is now disabled for orders *below* beneficial_order (not just = above), and (b) kswapd wakeup is also suppressed (not just direct reclaim).= These are individually defensible but together represent a substantially m= ore aggressive policy than the old code. **Minor nit:** The existing comment above this code says "device tolds us" = =E2=80=94 pre-existing typo, but if you're touching adjacent lines, conside= r fixing to "device told us". --- Generated by Claude Code Patch Reviewer