Claude review: mm/memcontrol: add dmem charge/uncharge functions

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

Patch Review

**1. Root memcg charge silently treated as failure**

```c
memcg = mem_cgroup_from_css(mem_css);
if (!memcg || mem_cgroup_is_root(memcg)) {
    css_put(mem_css);
    return false;
}
```

`mem_cgroup_dmem_charge()` returns `false` when the effective memcg is root. In patch 2's `dmem_cgroup_try_charge()`, `false` means `-ENOMEM` and the allocation fails entirely. This means that if a process in the root cgroup (or a cgroup with no memory controller enabled in its subtree) tries to allocate dmem with memcg charging enabled, the allocation will be unconditionally rejected. This seems wrong — charging to root should probably succeed (return `true`) and just skip the actual charge, since root is not subject to limits. The same issue exists in the uncharge path where it silently returns without uncharging.

**2. MEMCG_DMEM stat output unit is pages, displayed as bytes**

The new `MEMCG_DMEM` stat item stores values in pages (via `mod_memcg_state(memcg, MEMCG_DMEM, nr_pages)`). The `memcg_page_state_unit()` function defaults to `PAGE_SIZE` for unknown items, so `memcg_page_state_output()` will multiply the stored page count by `PAGE_SIZE`, producing a byte value. This is correct for display in `memory.stat`. However, the documentation in patch 2 says the stat is "reported in bytes" — it's worth confirming this is the intended behavior, since the internal accounting is in pages but the output conversion happens to work correctly by default. This is fine, just noting it's implicit rather than explicit.

**3. `try_charge_memcg` is a static function**

```c
if (try_charge_memcg(memcg, gfp_mask, nr_pages)) {
```

`try_charge_memcg()` is declared `static` in `mm/memcontrol.c`. The new `mem_cgroup_dmem_charge()` is also in `mm/memcontrol.c`, so this works. But it's calling a fairly low-level internal function directly rather than going through a higher-level API. This is acceptable for code within the same file, but worth noting that it couples dmem charging to the internal memcg charge path. If `try_charge_memcg` semantics change, this code must be updated too.

**4. Charge/uncharge asymmetry for `css_put`**

In `mem_cgroup_dmem_charge()`, a `css_get` (via `cgroup_get_e_css`) is taken and then `css_put` is called before returning. In `mem_cgroup_dmem_uncharge()`, the same pattern is used. This means the css reference is not held between charge and uncharge. This works because the dmem pool itself holds the css alive via `pool->cs->css`, but it's subtle — the correctness depends on the dmem pool keeping its cgroup alive until uncharge. The commit message mentions this ("the pool holds a reference to the dmem cgroup state that keeps the cgroup alive until it gets uncharged"), so it's intentional, but a brief code comment would help future readers.

**5. Missing `EXPORT_SYMBOL_GPL` for the new functions**

Neither `mem_cgroup_dmem_charge()` nor `mem_cgroup_dmem_uncharge()` has `EXPORT_SYMBOL_GPL`. Since dmem.c is built-in (not a module), this is fine for now. But if dmem ever becomes modular, these would need exports. Minor point.

---

---
Generated by Claude Code Patch Reviewer