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/pool: back up at native page order
Date: Tue, 05 May 2026 08:39:00 +1000
Message-ID: <review-patch1-20260504042619.2896273-1-matthew.brost@intel.com>
In-Reply-To: <20260504042619.2896273-1-matthew.brost@intel.com>
References: <20260504042619.2896273-1-matthew.brost@intel.com> <20260504042619.2896273-1-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

**Build dependency =E2=80=94 `__free_pages_gpu_account` does not exist**

The patch replaces the existing `put_page(page)` with:

```c
page->private =3D 0;
__free_pages_gpu_account(page, order, false);
```

This function does not exist anywhere in the drm-next tree. Grep across the=
 entire kernel tree returns no matches. This is presumably introduced by th=
e referenced series [1] (patchwork.freedesktop.org/series/165330/). The dep=
endency should be stated explicitly in the commit message, or the patch sho=
uld be rebased to use the existing free path. Note that the existing code's=
 `put_page()` also lacks GPU page accounting (`mod_lruvec_page_state`), so =
if `__free_pages_gpu_account` wraps both the accounting and the free, this =
is actually an improvement over the original =E2=80=94 but the dependency m=
ust be declared.

For a standalone fix targeting stable, you'd likely want:

```c
page->private =3D 0;
mod_lruvec_page_state(page, NR_GPU_ACTIVE, -(1 << order));
__free_pages(page, order);
```

to match the pattern in `ttm_pool_free_page()` at line 225-228 of the curre=
nt tree.

**Fault injection error value**

```c
if (IS_ENABLED(CONFIG_FAULT_INJECTION) &&
    should_fail(&backup_fault_inject, 1))
	shandle =3D -1;
```

Using bare `-1` (which is `-EPERM`) is unconventional. The outer fault inje=
ction at the top of the function truncates `num_pages`, which is a cleaner =
injection mechanism. Here, `-ENOMEM` would be more self-documenting since a=
 shmem backup failure is most likely an allocation failure. Minor nit, not =
a correctness issue since the caller only checks sign.

**Rollback correctness =E2=80=94 looks correct**

The rollback path on per-subpage failure:

```c
for (k =3D 0; k < j; ++k) {
	handle =3D ttm_backup_page_ptr_to_handle(tt->pages[i + k]);
	ttm_backup_drop(backup, handle);
	tt->pages[i + k] =3D page + k;
}
goto out;
```

This correctly drops already-written handles and restores the original page=
 pointers, preserving compound-level atomicity. On a retry, the compound wi=
ll be attempted again from scratch. The `out:` label placement before the r=
eturn is correct.

**Handle-slot iteration for previously backed-up compounds**

When re-entering after a partial backup, already-handled slots are detected:

```c
if (unlikely(ttm_backup_page_ptr_is_handle(page)))
	continue;
```

With `npages =3D 1` still set from the default, this iterates one slot at a=
 time through backed-up compounds. This is correct but slightly less effici=
ent than stepping by the original compound order. Acceptable since the retr=
y case is uncommon.

**Skipping truncated compounds is safe**

```c
if (unlikely(i + npages > num_pages))
	break;
```

When fault injection truncates `num_pages` mid-compound, the entire compoun=
d is skipped rather than split. This is the right conservative choice and a=
voids introducing the fragmentation the patch is designed to prevent.

**`page->private =3D 0` only on head page =E2=80=94 correct**

The split path clears `private` on all sub-pages, but when freeing at nativ=
e order, only the head page's `private` needs to be cleared since the buddy=
 allocator operates on the head page. This matches the purge path pattern a=
t the current tree's line 1070.

**`set_pages_array_wb` interaction =E2=80=94 no issue**

The existing `set_pages_array_wb(tt->pages, tt->num_pages)` call before the=
 backup loop passes all sub-page pointers. Since `tt->pages[]` is populated=
 with individual sub-page pointers (head, head+1, head+2, ...) during alloc=
ation via `ttm_pool_allocated_page_commit()`, the caching change applies co=
rrectly to each physical page regardless of whether they're later split or =
freed as a group.

**`shrunken` accounting change**

Old code: `shrunken++` per order-0 page. New code: `shrunken +=3D npages` p=
er compound. The return type is `long` and represents page count, so this i=
s correct and consistent with the purge path.

**Summary**: The approach is sound and the rollback logic is well-construct=
ed. The primary blocker is the unstated dependency on `__free_pages_gpu_acc=
ount()`. If this is intended to be applied atop the referenced series, that=
 should be explicit. For stable backport, the free call needs adaptation to=
 use existing kernel APIs. The `Assisted-by: Claude` tag is noted.

---
Generated by Claude Code Patch Reviewer