From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: mm/memcontrol: add dmem charge/uncharge functions Date: Mon, 25 May 2026 22:42:50 +1000 Message-ID: In-Reply-To: <20260519-cgroup-dmem-memcg-double-charge-v2-1-db4d1407062b@redhat.com> References: <20260519-cgroup-dmem-memcg-double-charge-v2-0-db4d1407062b@redhat.com> <20260519-cgroup-dmem-memcg-double-charge-v2-1-db4d1407062b@redhat.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 **1. Root memcg charge silently treated as failure** ```c memcg =3D 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 all= ocate dmem with memcg charging enabled, the allocation will be unconditiona= lly rejected. This seems wrong =E2=80=94 charging to root should probably s= ucceed (return `true`) and just skip the actual charge, since root is not s= ubject to limits. The same issue exists in the uncharge path where it silen= tly 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 def= aults to `PAGE_SIZE` for unknown items, so `memcg_page_state_output()` will= multiply the stored page count by `PAGE_SIZE`, producing a byte value. Thi= s is correct for display in `memory.stat`. However, the documentation in pa= tch 2 says the stat is "reported in bytes" =E2=80=94 it's worth confirming = this is the intended behavior, since the internal accounting is in pages bu= t 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 `me= m_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 fil= e, but worth noting that it couples dmem charging to the internal memcg cha= rge 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 take= n and then `css_put` is called before returning. In `mem_cgroup_dmem_unchar= ge()`, the same pattern is used. This means the css reference is not held b= etween charge and uncharge. This works because the dmem pool itself holds t= he css alive via `pool->cs->css`, but it's subtle =E2=80=94 the correctness= depends on the dmem pool keeping its cgroup alive until uncharge. The comm= it message mentions this ("the pool holds a reference to the dmem cgroup st= ate that keeps the cgroup alive until it gets uncharged"), so it's intentio= nal, 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 `EX= PORT_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 poi= nt. --- --- Generated by Claude Code Patch Reviewer