Claude review: drm/ttm: Issue direct reclaim at beneficial_order

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

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

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

```c
-	if (beneficial_order && order > beneficial_order)
-		gfp_flags &= ~__GFP_DIRECT_RECLAIM;
+	if (order && beneficial_order && order != beneficial_order)
+		gfp_flags &= ~__GFP_RECLAIM;
```

**This patch has significant issues:**

1. **The change from `__GFP_DIRECT_RECLAIM` to `__GFP_RECLAIM` is aggressive.** `__GFP_RECLAIM` is `(__GFP_DIRECT_RECLAIM | __GFP_KSWAPD_RECLAIM)`. The old code only removed direct reclaim for orders above beneficial_order, allowing kswapd to still be woken. The new code removes *all* reclaim (including kswapd wakeup) for any order that isn't exactly `beneficial_order` (and 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 != beneficial_order` condition is too broad.** The old code only penalized orders *above* beneficial_order. The new code penalizes all non-zero orders that aren't exactly equal to beneficial_order. For orders 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" — but that's the driver's preferred large page size. The allocator will still try smaller orders as fallbacks (that's how the pool works — it iterates downward), and those 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 only be issued with `__GFP_NORETRY` at exactly beneficial_order" but the code doesn't set `__GFP_NORETRY` — 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 |= __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 handled by the `if (order && ...)` guard, which is correct.

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

---

---
Generated by Claude Code Patch Reviewer