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: cgroup/dmem: introduce a peak file
Date: Sat, 16 May 2026 10:32:53 +1000
Message-ID: <review-patch1-20260514-dmem_peak-v3-1-b64ce5d3ac38@igalia.com>
In-Reply-To: <20260514-dmem_peak-v3-1-b64ce5d3ac38@igalia.com>
References: <20260514-dmem_peak-v3-1-b64ce5d3ac38@igalia.com> <20260514-dmem_peak-v3-1-b64ce5d3ac38@igalia.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

**Correctness: Good**

The core implementation is sound:

```c
static u64 get_resource_peak(struct dmem_cgroup_pool_state *pool)
{
	return pool ? READ_ONCE(pool->cnt.watermark) : 0;
}
```

This correctly handles the NULL pool case (returns 0, matching the `get_res=
ource_current` pattern) and uses `READ_ONCE` to read the racy watermark, wh=
ich is the established pattern in `mm/page_counter.c` (line 103: `if (new >=
 READ_ONCE(c->watermark))`).

The watermark is updated hierarchically in `page_counter_try_charge()` and =
`page_counter_charge()`, walking the parent chain, so the "cgroup and its d=
escendants" claim in the documentation is correct =E2=80=94 charging to a l=
eaf updates the watermark on all ancestors.

**Flag choice: Correct**

```c
{
	.name =3D "peak",
	.seq_show =3D dmem_cgroup_region_peak_show,
	.flags =3D CFTYPE_NOT_ON_ROOT,
},
```

Using `CFTYPE_NOT_ON_ROOT` is consistent with `min`, `low`, and `max` in th=
is controller. The root cgroup's peak is not meaningful (it would just trac=
k total system device memory usage, which `capacity` already covers from a =
different angle).

**Nit =E2=80=94 v3 changelog has EDITME placeholders:**

```
Changes in v3:
- EDITME: describe what is new in this series revision.
- EDITME: use bulletpoints and terse descriptions.
```

This is a b4 template artifact that wasn't filled in. It doesn't affect the=
 code, but it's sloppy and should be fixed before the patch is applied. The=
 maintainer will likely ask for a resend.

**Nit =E2=80=94 Documentation placement:**

The `dmem.peak` documentation is placed between `dmem.current` and `dmem.ca=
pacity`, which is a good logical grouping (current/peak are related usage m=
etrics). The text is clear and mirrors the `memory.peak` description style.

**Observation =E2=80=94 No selftest:**

The patch doesn't include a selftest. While there's no existing dmem selfte=
st infrastructure to extend, a simple test (create cgroup, allocate device =
memory, read peak, verify peak >=3D current) would be valuable, especially =
since peak tracking is the kind of feature that could silently regress.

**Observation =E2=80=94 Future write-to-reset:**

The commit message mentions "for now, make it read-only." When write-to-res=
et is added later, it will likely need to use `local_watermark` (like `memo=
ry.peak` does for per-FD resets via `page_counter_reset_watermark()`). The =
current implementation reading `watermark` (the global one) is correct for =
a read-only peak file, so no changes are needed now.

**Overall: The patch is a clean, minimal, and correct addition. Fix the EDI=
TME changelog and consider adding a selftest, otherwise this is ready.**

---
Generated by Claude Code Patch Reviewer