From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot 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:04:00 +1000 Message-ID: In-Reply-To: <20260512082406.44470-3-thomas.hellstrom@linux.intel.com> References: <20260512082406.44470-1-thomas.hellstrom@linux.intel.com> <20260512082406.44470-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 observations.** **Core design change =E2=80=94 `xchg` replacing `page_counter_set_max`:** ```c + xchg(&pool->cnt.max, (unsigned long)val); ``` The original `page_counter_set_max()` refuses to set max below current usag= e (returns `-EBUSY`). The new code unconditionally sets max via `xchg`, whi= ch preserves the full memory barrier semantics while allowing max < usage. = This is the right approach: it throttles concurrent allocations immediately= while reclaim runs. Well motivated. **Retry loop termination on partial progress:** ```c + for (int retries =3D 5; retries > 0; retries--) { + ... + ret =3D region->reclaim(pool, usage - val, region->reclaim_priv); + ... + if (ret) + break; ``` The reclaim callback (in patch 3) returns `-ENOSPC` when partial progress i= s made. This causes the retry loop to break even if some memory was freed. = A more aggressive approach would be to retry on partial progress (similar t= o how the memory cgroup controller distinguishes `-EAGAIN` for "retry" from= hard errors). However, since `page_counter_read()` reflects the actual usa= ge and the max is already set to throttle new allocations, this best-effort= approach is defensible and matches the stated design intent. **rwsem protection looks correct:** - `down_write` in `dmem_cgroup_unregister_region()` ensures all in-flight r= eclaim callbacks complete before clearing the callback pointer. - `down_read_interruptible` in `set_resource_max()` allows concurrent recla= im and handles signals gracefully. - `WRITE_ONCE`/`READ_ONCE` pairs on `region->reclaim` are properly placed. **Unused `nonblock` parameter in `set_resource_min`/`set_resource_low`:** ```c static void -set_resource_min(struct dmem_cgroup_pool_state *pool, u64 val) +set_resource_min(struct dmem_cgroup_pool_state *pool, u64 val, bool nonblo= ck) ``` These functions accept `nonblock` but ignore it, keeping the function point= er signature uniform for `dmemcg_limit_write()`. This is clean. --- --- Generated by Claude Code Patch Reviewer