Claude review: cgroup/dmem: add dmem.memcg control file for double-charging to memcg

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

Patch Review

**1. Charge/uncharge size inconsistency (potential accounting leak)**

In `dmem_cgroup_try_charge()`:
```c
unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
...
if (charge_memcg) {
    ...
    mem_cgroup_dmem_charge(pool->cs->css.cgroup, nr_pages, GFP_KERNEL);
}
```

But in `dmem_cgroup_uncharge()`:
```c
if (atomic_read(&pool->region->memcg_status) == DMEM_MEMCG_LOCKED_ON &&
    !WARN_ON_ONCE(size > (u64)UINT_MAX << PAGE_SHIFT))
    mem_cgroup_dmem_uncharge(pool->cs->css.cgroup,
                 PAGE_ALIGN(size) >> PAGE_SHIFT);
```

The charge path checks `size > (u64)UINT_MAX << PAGE_SHIFT` and bails out early with `-EINVAL` *before* charging memcg. But the uncharge path uses `WARN_ON_ONCE` on the same condition and *skips the uncharge* if the size is too large. If somehow a large allocation was charged (e.g., due to a code change or different path), the uncharge would be silently skipped, leaking the memcg charge. The WARN_ON_ONCE is also inverted — it triggers when the condition is true (size is too large) and then the `!` causes the uncharge to be skipped. The logic is correct but confusing to read.

**2. `apply_memcg_charge` is called on every charge attempt, even if it's already locked**

```c
charge_memcg = apply_memcg_charge(&region->memcg_status);
```

This function does an `atomic_cmpxchg` on every allocation to lock the state, even after the first successful lock. For `DMEM_MEMCG_LOCKED_OFF` and `DMEM_MEMCG_LOCKED_ON`, it just reads and returns, which is fine. But for the unlocked states, it'll keep attempting cmpxchg until it succeeds. This is a minor hot-path concern — once locked, the `atomic_read` in the switch is cheap. No bug, just noting the fast path is already O(1) after the first allocation.

**3. Write handler silently ignores locked state**

```c
atomic_cmpxchg(&region->memcg_status,
           flag ? DMEM_MEMCG_OFF : DMEM_MEMCG_ON,
           flag ? DMEM_MEMCG_ON : DMEM_MEMCG_OFF);
/* Continue if a region is already locked. */
```

When a write to `dmem.memcg` tries to change a region that is already locked, the `atomic_cmpxchg` silently fails (the old value doesn't match) and the write returns success (`nbytes`). The admin gets no indication that the change was not applied. It would be more user-friendly to return `-EBUSY` or `-EPERM` when the region is locked, rather than silently discarding the write. At minimum, the admin could check by reading the file, but silent success on a no-op write is a usability issue.

**4. Write handler returns `nbytes` on partial processing**

The write handler processes lines in a loop. If parsing fails mid-way (e.g., second line is invalid), it returns `-EINVAL` having already processed the first line and released its region ref. But there's no rollback of the first line's state change. While cgroup write operations are typically single-line, the parser supports multi-line input, so this is a potential issue.

**5. Missing `CFTYPE_NS_DELEGATABLE` check**

The `dmem.memcg` file is `CFTYPE_ONLY_ON_ROOT` but has no `.access` flags to restrict writes. The commit message says "root-only cgroupfs file that lets an administrator configure" — but the write permission is just standard file permissions. Any process with write access to the cgroupfs root can modify the setting, not just uid 0. For a security-sensitive setting that affects accounting, this might want explicit access control. However, this matches the pattern of other root-only cgroup files, so it may be intentional.

**6. `depends_on` declaration**

```c
#ifdef CONFIG_MEMCG
	.depends_on	= 1 << memory_cgrp_id,
#endif
```

This ensures the memory controller is enabled whenever dmem is enabled. This is correct for the feature, but it changes behavior even when `dmem.memcg` is not used — any system with both CONFIG_MEMCG and CONFIG_CGROUP_DMEM will now force the memory controller to be active whenever dmem is. This could be surprising for users who want dmem without memcg overhead. The tradeoff makes the implementation simpler (no need to handle the case where memcg is disabled at runtime), but should be documented.

**7. `atomic_read` in uncharge path has no acquire barrier**

```c
if (atomic_read(&pool->region->memcg_status) == DMEM_MEMCG_LOCKED_ON &&
```

The uncharge path uses a plain `atomic_read` to check the locked-on state. The corresponding `atomic_cmpxchg` in `apply_memcg_charge` provides full memory ordering, so the transition to LOCKED_ON is properly visible. However, there's a window between when `apply_memcg_charge` returns `true` (transitioning to LOCKED_ON) and when the charge actually completes. If the charge fails after `apply_memcg_charge` locks the state, the state is permanently locked ON but no charge was made. The error path in `dmem_cgroup_try_charge` handles this correctly (it uncharges memcg on `page_counter_try_charge` failure), but it's worth verifying that all error paths are covered.

**8. Documentation placement**

The documentation is well-written and clear. The placement after the existing dmem documentation section is correct. One minor note: the doc says "This file is only available when the kernel is built with CONFIG_MEMCG" — this should probably also mention CONFIG_CGROUP_DMEM since both are required.

---
Generated by Claude Code Patch Reviewer