From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/ttm: Issue direct reclaim at beneficial_order
Date: Tue, 05 May 2026 10:00:00 +1000
Message-ID: <review-patch3-20260430191809.2142544-4-matthew.brost@intel.com>
In-Reply-To: <20260430191809.2142544-4-matthew.brost@intel.com>
References: <20260430191809.2142544-1-matthew.brost@intel.com> <20260430191809.2142544-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

**Subject:** `[PATCH v4 3/6] drm/ttm: Issue direct reclaim at beneficial_or=
der`

This patch changes how GFP flags are modified in `ttm_pool_alloc_page()`:

```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;
```

**This patch has significant issues:**

1. **The change from `__GFP_DIRECT_RECLAIM` to `__GFP_RECLAIM` is aggressiv=
e.** `__GFP_RECLAIM` is `(__GFP_DIRECT_RECLAIM | __GFP_KSWAPD_RECLAIM)`. Th=
e old code only removed direct reclaim for orders above beneficial_order, a=
llowing kswapd to still be woken. The new code removes *all* reclaim (inclu=
ding kswapd wakeup) for any order that isn't exactly `beneficial_order` (an=
d isn't 0). This means:
   - Orders *below* `beneficial_order` (e.g., order 1-8 when beneficial is =
9) now lose **all** reclaim, not just direct reclaim. Before, these weren't=
 affected at all.
   - Orders *above* `beneficial_order` also lose kswapd wakeup, which they =
didn't before.

2. **The `order !=3D beneficial_order` condition is too broad.** The old co=
de only penalized orders *above* beneficial_order. The new code penalizes a=
ll non-zero orders that aren't exactly equal to beneficial_order. For order=
s between 1 and `beneficial_order - 1`, the old code allowed normal reclaim=
 behavior. The commit message says "issuing direct reclaim at a lower order=
 than beneficial_order is ineffective" =E2=80=94 but that's the driver's pr=
eferred large page size. The allocator will still try smaller orders as fal=
lbacks (that's how the pool works =E2=80=94 it iterates downward), and thos=
e smaller allocations should still be allowed to trigger reclaim since they=
 may succeed where the large order failed.

3. **The commit message is misleading.** It says "direct reclaim should onl=
y be issued with `__GFP_NORETRY` at exactly beneficial_order" but the code =
doesn't set `__GFP_NORETRY` =E2=80=94 it strips `__GFP_RECLAIM`. These are =
very different semantics. Also, `__GFP_NORETRY` is already set at line 161 =
for all non-zero orders:
   ```c
   if (order)
       gfp_flags |=3D __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_NOWARN |
           __GFP_THISNODE;
   ```
   So the description of intended behavior doesn't match the implementation.

4. **The fallback to order 0 claim in the commit message** ("direct reclaim=
 without `__GFP_NORETRY` at order 0 when failure is not an option") is hand=
led by the `if (order && ...)` guard, which is correct.

**Recommendation: This patch needs rework.** The semantics change is too ag=
gressive. Consider keeping `__GFP_DIRECT_RECLAIM` removal (not `__GFP_RECLA=
IM`) and being more precise about the order range.

---

---
Generated by Claude Code Patch Reviewer