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: Add reclaim callback for lowering max below current usage
Date: Sat, 16 May 2026 14:48:42 +1000
Message-ID: <review-patch2-20260511173008.36526-3-thomas.hellstrom@linux.intel.com>
In-Reply-To: <20260511173008.36526-3-thomas.hellstrom@linux.intel.com>
References: <20260511173008.36526-1-thomas.hellstrom@linux.intel.com> <20260511173008.36526-3-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

**Verdict: Good with minor comments.**

The core reclaim infrastructure is well designed. The key change in `set_re=
source_max()`:

```c
xchg(&pool->cnt.max, (unsigned long)val);
```

replaces:

```c
page_counter_set_max(&pool->cnt, val);
```

This is a significant semantic change. `page_counter_set_max()` returns `-E=
BUSY` when usage exceeds the new limit (and never actually sets the limit i=
n that case). The raw `xchg()` unconditionally sets the limit. The original=
 code was silently ignoring the `-EBUSY` return, meaning `dmem.max` writes =
below current usage were no-ops. The new code intentionally sets the limit =
regardless, then attempts reclaim. This is correct behavior, but the commit=
 message could mention this semantic difference more explicitly.

The reclaim loop is bounded (5 retries) and interruptible:

```c
for (int retries =3D 5; retries > 0; retries--) {
    ...
    if (signal_pending(current))
        break;
    if (down_read_interruptible(&region->unregister_sem))
        break;
    ...
}
```

The use of `down_read_interruptible()` is good =E2=80=94 it prevents an adm=
in `kill` from being stuck waiting for a long eviction.

The `nonblock` parameter is threaded through `set_resource_min` and `set_re=
source_low` even though only `set_resource_max` uses it. This is for functi=
on pointer signature consistency since they're passed as callbacks via the =
`apply` parameter in `dmemcg_limit_write()`. That's fine.

Minor: The series doesn't return an error to userspace if reclaim fails to =
bring usage below the limit (the loop just exits). The commit message says =
this "matches the behavior of memory.max in the memory cgroup controller." =
This is acceptable but worth noting that if an admin sets `dmem.max =3D 0` =
and reclaim can't evict everything (pinned BOs), the write succeeds silentl=
y while the limit is technically set but violated.

The `unregister_sem` synchronization:
- `dmem_cgroup_region_set_reclaim()`: write-locks, sets `reclaim_priv` then=
 `reclaim` (WRITE_ONCE), unlocks.
- `set_resource_max()`: READ_ONCE on `reclaim`, then read-locks, re-checks =
reclaim, calls it, unlocks.
- `dmem_cgroup_unregister_region()`: write-locks, WRITE_ONCE nulls `reclaim=
`, unlocks.

This correctly ensures: (1) `reclaim_priv` is visible when `reclaim` is non=
-NULL, (2) in-progress reclaim completes before the region is unregistered.=
 Solid.

---
Generated by Claude Code Patch Reviewer