Claude review: cgroup/dmem: introduce a peak file

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

Patch Review

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

**Correctness: Good.** The peak tracking semantics are correct:
- Global watermark (`pool->cnt.watermark`) is reported for regions the fd has not reset.
- Per-fd peak (`max(ofp->value, pool->cnt.local_watermark)`) is reported for 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 generic cgroup infrastructure and the memory controller. Every `memory.peak` open file now carries an unused `struct dmem_cgroup_pool_state *pool`. While harmless (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` to `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_pool_state *pool)
   ```
   `get_resource_low()`, `get_resource_min()`, `get_resource_max()`, and `get_resource_current()` all gain an `sf` parameter they never use, solely to match the callback signature needed by `get_resource_peak()`. This is a code 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_pool_state *pool)
   {
       struct cgroup_of_peak *ofp = of_peak(sf->private);
       ...
       of_pool = READ_ONCE(ofp->pool);
       fd_peak = READ_ONCE(ofp->value);
       if (of_pool != pool || fd_peak == OFP_PEAK_UNSET)
           peak = pool->cnt.watermark;
       else
           peak = 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 and a concurrent read having already loaded the old value). However, since the pointer is only used for an equality comparison against the valid `pool` from the iterator — never dereferenced — this is safe. The fallback to `pool->cnt.watermark` on mismatch makes this robust. This is fine 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->value == OFP_PEAK_UNSET`:**
   ```c
   static void dmem_cgroup_region_peak_remove(struct cgroup_of_peak *ofp)
   {
       struct dmem_cgroup_pool_state *pool;
       ...
       pool = xchg(&ofp->pool, NULL);
       if (!pool)
           return;
       ...
   ```
   It relies on `ofp->pool == NULL` for the "no prior write" case, which is correct since `pool` is only set in the write path. But `dmem_cgroup_region_peak_release()` additionally checks `ofp->value == OFP_PEAK_UNSET` as a fast path before calling `remove`. This is slightly redundant but harmless — 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 well-described.

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

**One question for the author:** When multiple fds reset the same region, the `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 — until new allocations push `local_watermark` back up. This is the same behavior as `memory.peak` so it's consistent, but worth confirming that this is the intended semantic for dmem as well, since device memory allocation patterns may differ from system memory.

---
Generated by Claude Code Patch Reviewer