From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: memcg: Track exported dma-buffers Date: Sat, 16 May 2026 13:56:21 +1000 Message-ID: In-Reply-To: <20260512-v2_20230123_tjmercier_google_com-v1-1-6326701c3691@redhat.com> References: <20260512-v2_20230123_tjmercier_google_com-v1-0-6326701c3691@redhat.com> <20260512-v2_20230123_tjmercier_google_com-v1-1-6326701c3691@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 This patch (from T.J. Mercier's earlier series) adds `MEMCG_DMABUF` as a me= mcg stat counter, charges at `dma_buf_export()` time, and uncharges at `dma= _buf_release()` time. **Issue 1 - Bisectability:** This patch charges in `dma_buf_export()` for A= LL dma-buf exports, but patch 2 immediately removes that code and moves cha= rging to `dma_heap_buffer_alloc()` only. Between patches 1 and 2, every DRM= driver, V4L2 exporter, etc. that calls `dma_buf_export()` incurs a memcg c= harge. This creates a window where behavior differs from both the pre-serie= s state and the final intended state. The infrastructure (stat counter, cha= rge/uncharge functions) should be added in patch 1 without hooking into `dm= a_buf_export()`, and patch 2 should add the charging at the heap level. ```c + dmabuf->memcg =3D get_mem_cgroup_from_mm(current->mm); + if (!mem_cgroup_charge_dmabuf(dmabuf->memcg, PAGE_ALIGN(dmabuf->size) / P= AGE_SIZE, + GFP_KERNEL)) { ``` This charges unconditionally in `dma_buf_export()`, then patch 2 removes it= entirely. **Issue 2 - Uncharge before ops->release():** The uncharge is placed before= `dmabuf->ops->release()`: ```c + mem_cgroup_uncharge_dmabuf(dmabuf->memcg, PAGE_ALIGN(dmabuf->size) / PAGE= _SIZE); + mem_cgroup_put(dmabuf->memcg); + dmabuf->ops->release(dmabuf); ``` This means the memcg charge is removed before the backing pages are actuall= y freed. If `ops->release()` takes significant time or blocks, there's a wi= ndow where the pages are still allocated but not charged. This is a minor a= ccuracy concern but worth noting. **Issue 3 - doc tag:** The documentation adds `dmabuf (npn)` =E2=80=94 it w= ould be helpful to confirm that `(npn)` is the correct format tag. Looking = at the existing doc entries, most stats don't use this tag pattern, so this= might be incorrect formatting or an undefined abbreviation. --- Generated by Claude Code Patch Reviewer