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: Fix ttm_bo_shrink() infinite LRU walk on backup failure
Date: Sat, 16 May 2026 15:07:24 +1000
Message-ID: <review-patch1-20260511162443.24352-1-thomas.hellstrom@linux.intel.com>
In-Reply-To: <20260511162443.24352-1-thomas.hellstrom@linux.intel.com>
References: <20260511162443.24352-1-thomas.hellstrom@linux.intel.com> <20260511162443.24352-1-thomas.hellstrom@linux.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

**The bug (before the fix):**

The old code in `ttm_bo_util.c:1115-1129` did:
```c
if (bo->bulk_move) {
    spin_lock(&bdev->lru_lock);
    ttm_resource_del_bulk_move(bo->resource, bo);
    spin_unlock(&bdev->lru_lock);
}

lret =3D ttm_tt_backup(...);

if (lret <=3D 0 && bo->bulk_move) {
    spin_lock(&bdev->lru_lock);
    ttm_resource_add_bulk_move(bo->resource, bo);
    spin_unlock(&bdev->lru_lock);
}
```

The `del_bulk_move` before backup removes the resource from the bulk_move l=
ist. If backup fails, `add_bulk_move` re-adds it, but this can reinsert the=
 resource at a position the LRU cursor hasn't passed yet, causing the curso=
r to re-encounter the same BO, leading to an infinite walk. Critically, no =
`ttm_resource_move_to_lru_tail()` was called on failure, so the resource ne=
ver moved away from the cursor.

**The fix:**

```c
lret =3D ttm_tt_backup(...);

if (lret > 0) {
    spin_lock(&bdev->lru_lock);
    ttm_resource_del_bulk_move_unevictable(bo->resource, bo);
    ttm_resource_move_to_lru_tail(bo->resource);
    spin_unlock(&bdev->lru_lock);
}
```

- **Failure path**: No bulk_move manipulation at all. The resource stays in=
 place, the cursor naturally advances past it. This is correct.
- **Success path**: Uses `del_bulk_move_unevictable` then `move_to_lru_tail=
`. This is necessary because after successful backup, `TTM_TT_FLAG_BACKED_U=
P` is set (`ttm_tt.c:292`), making `ttm_resource_unevictable()` return true=
. The regular `ttm_resource_del_bulk_move()` would be a **no-op** here sinc=
e it checks `!ttm_resource_unevictable(res, bo)` and skips unevictable reso=
urces (`ttm_resource.c:291`). The `_unevictable` variant is essential to co=
rrectly remove the resource from bulk_move tracking during this evictable-t=
o-unevictable transition.

**Correctness: Looks correct.** The approach is sound and matches the swapo=
ut fix pattern.

**Minor observations:**

1. **Dependency not stated in the commit message.** The patch references co=
mmit b2ed01e7ad as the analogous fix but doesn't explicitly note that `ttm_=
resource_del_bulk_move_unevictable()` is introduced by that commit. Adding =
a note (e.g., a "Depends-on:" tag or a mention in the commit body) would he=
lp patch management, especially given the `Cc: stable` tag =E2=80=94 both p=
atches need to land together in stable.

2. **`Assisted-by: GitHub_Copilot:claude-opus-4.6`** =E2=80=94 This tag for=
mat is non-standard. The kernel community hasn't converged on a standardize=
d AI-assistance tag; existing conventions lean toward something with the pr=
ovider name not GitHub Copilot when the model is Anthropic's Claude. This i=
s minor and a process/policy matter, not a code issue.

3. **No `move_to_lru_tail` on failure, unlike the swapout path.** The swapo=
ut path (`ttm_bo.c:1186-1189`) calls `move_to_lru_tail` even on failure. Th=
e shrink path omits this, which is fine: in the shrink caller (`xe_shrinker=
.c:82-83`), any negative return aborts the walk immediately (`if (lret < 0)=
 return lret`), and a zero return simply means no pages freed so the cursor=
 advances normally. The LRU tail movement on failure is unnecessary here. H=
owever, if you wanted maximum consistency with the swapout path, adding `mo=
ve_to_lru_tail` on failure would be a defensive improvement to ensure the r=
esource moves to the tail even if callers change in the future.

**Verdict:** The fix is correct and necessary. The main thing to ensure is =
that the dependency on `ttm_resource_del_bulk_move_unevictable()` is proper=
ly tracked for stable backports.

---
Generated by Claude Code Patch Reviewer