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: Thu, 07 May 2026 13:37:56 +1000
Message-ID: <review-patch2-20260506-dmem_peak-v1-2-8d803eb3449c@igalia.com>
In-Reply-To: <20260506-dmem_peak-v1-2-8d803eb3449c@igalia.com>
References: <20260506-dmem_peak-v1-0-8d803eb3449c@igalia.com> <20260506-dmem_peak-v1-2-8d803eb3449c@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

This is the main patch, adding `dmem.peak` file support with per-region, pe=
r-fd peak tracking.

**Correctness: Good.** The peak tracking semantics are correct:
- Global watermark (`pool->cnt.watermark`) is reported for regions the fd h=
as not reset.
- Per-fd peak (`max(ofp->value, pool->cnt.local_watermark)`) is reported fo=
r the region the fd has reset.
- Writing a different region name correctly "detaches" from the old region =
(reverting to global watermark) and "attaches" to the new one.

**Reference counting is correct:** `get_cg_pool_unlocked()` acquires a pool=
 ref that is transferred to `ofp->pool`. `dmem_cgroup_region_peak_remove()`=
 drops it. The region ref from `dmemcg_get_region_by_name()` is dropped at =
`out_put`. The release path handles both the fast path (no writes) and the =
watcher-cleanup path.

**Design concerns:**

1. **`pool` pointer in generic `cgroup_of_peak` struct:**
   ```c
   struct cgroup_of_peak {
       unsigned long       value;
       struct list_head    list;
   +   struct dmem_cgroup_pool_state *pool;
   };
   ```
   This adds a DRM-subsystem-specific pointer to a struct used by the gener=
ic cgroup infrastructure and the memory controller. Every `memory.peak` ope=
n file now carries an unused `struct dmem_cgroup_pool_state *pool`. While h=
armless (it's zero-initialized via `kzalloc` of the containing `cgroup_file=
_ctx`), this is a layering violation that cgroup/mm maintainers may object =
to. Consider using a wrapper struct in dmem.c or adding a `void *private` t=
o `cgroup_of_peak` instead.

2. **Unused `struct seq_file *sf` parameter added to four functions:**
   ```c
   -static u64 get_resource_low(struct dmem_cgroup_pool_state *pool)
   +static u64 get_resource_low(struct seq_file *sf, struct dmem_cgroup_poo=
l_state *pool)
   ```
   `get_resource_low()`, `get_resource_min()`, `get_resource_max()`, and `g=
et_resource_current()` all gain an `sf` parameter they never use, solely to=
 match the callback signature needed by `get_resource_peak()`. This is a co=
de smell. An alternative would be to have the show function pass a context =
struct or to handle the peak case with a separate callback type. At minimum=
, marking the parameter `__always_unused` would signal the intent.

3. **`get_resource_peak()` reads `ofp` without any lock:**
   ```c
   static u64 get_resource_peak(struct seq_file *sf, struct dmem_cgroup_poo=
l_state *pool)
   {
       struct cgroup_of_peak *ofp =3D of_peak(sf->private);
       ...
       of_pool =3D READ_ONCE(ofp->pool);
       fd_peak =3D READ_ONCE(ofp->value);
       if (of_pool !=3D pool || fd_peak =3D=3D OFP_PEAK_UNSET)
           peak =3D pool->cnt.watermark;
       else
           peak =3D max(fd_peak, READ_ONCE(pool->cnt.local_watermark));
   ```
   This is intentionally lockless for the read path, relying on `READ_ONCE`=
 for individual fields. There is a window where `of_pool` could point to a =
freed pool (between `dmem_cgroup_region_peak_remove()` setting it to NULL a=
nd a concurrent read having already loaded the old value). However, since t=
he pointer is only used for an equality comparison against the valid `pool`=
 from the iterator =E2=80=94 never dereferenced =E2=80=94 this is safe. The=
 fallback to `pool->cnt.watermark` on mismatch makes this robust. This is f=
ine but worth a brief comment in the code explaining why the lockless read =
is safe.

4. **The `dmem_cgroup_region_peak_remove()` function does not check `ofp->v=
alue =3D=3D OFP_PEAK_UNSET`:**
   ```c
   static void dmem_cgroup_region_peak_remove(struct cgroup_of_peak *ofp)
   {
       struct dmem_cgroup_pool_state *pool;
       ...
       pool =3D xchg(&ofp->pool, NULL);
       if (!pool)
           return;
       ...
   ```
   It relies on `ofp->pool =3D=3D NULL` for the "no prior write" case, whic=
h is correct since `pool` is only set in the write path. But `dmem_cgroup_r=
egion_peak_release()` additionally checks `ofp->value =3D=3D OFP_PEAK_UNSET=
` as a fast path before calling `remove`. This is slightly redundant but ha=
rmless =E2=80=94 the two checks are equivalent for the no-write case.

5. **Documentation is good:**
   ```
   +  dmem.peak
   +    A readwrite nested-keyed file that exists on non-root cgroups.
   +    ...
   ```
   Clear and follows the existing documentation style. The semantics are we=
ll-described.

6. **Initialization in `alloc_pool_single()`:**
   ```c
   +   INIT_LIST_HEAD(&pool->peaks);
   ```
   Correctly initializes the peaks watcher list for new pools. The `spin_lo=
ck_init(&dmemcs->peaks_lock)` in `dmemcs_alloc()` is also correct.

**One question for the author:** When multiple fds reset the same region, t=
he `local_watermark` is lowered to current usage on each reset. This means =
fd1's peak could temporarily appear lower than its actual peak-since-reset =
if fd2 resets afterward at a lower usage point =E2=80=94 until new allocati=
ons push `local_watermark` back up. This is the same behavior as `memory.pe=
ak` so it's consistent, but worth confirming that this is the intended sema=
ntic for dmem as well, since device memory allocation patterns may differ f=
rom system memory.

---
Generated by Claude Code Patch Reviewer