From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: selftests/dmabuf-heaps: Add dma-buf memcg accounting tests Date: Sat, 16 May 2026 13:56:22 +1000 Message-ID: In-Reply-To: <20260512-v2_20230123_tjmercier_google_com-v1-5-6326701c3691@redhat.com> References: <20260512-v2_20230123_tjmercier_google_com-v1-0-6326701c3691@redhat.com> <20260512-v2_20230123_tjmercier_google_com-v1-5-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 Adds tests for the charge_pid_fd feature in both the dmabuf-heaps and cgrou= p test suites, plus a vmtest.sh script. **Issue 1 - Feature detection:** ```c +static bool pidfd_alloc_supported(int heap_fd) +{ + int devnull_fd, dmabuf_fd =3D -1, ret; + devnull_fd =3D open("/dev/null", O_RDONLY); + ... + ret =3D dmabuf_heap_alloc_pidfd(heap_fd, ONE_MEG, 0, devnull_fd, &dmabuf_= fd); ``` This probes by passing `/dev/null` as a pidfd and checking for failure. The= logic `return ret < 0` assumes that if the ioctl fails with a non-pidfd, t= he kernel supports the field. But on an old kernel that ignores the new fie= lds (because of the zero-fill ioctl handling), the ioctl would *succeed* (a= llocating without any pidfd charging), and `ret` would be 0. Then `return r= et < 0` returns `false`, meaning "not supported" =E2=80=94 which is wrong. = The detection logic should check whether the failure mode indicates the fie= ld was recognized, e.g., by checking for `-EBADF` (bad fd) specifically, ra= ther than any failure. On an old kernel the ioctl would succeed (ignoring t= he extra bytes), so the probe returns "not supported," which happens to be = correct since the old kernel wouldn't act on the field. Actually on reflect= ion this works by accident =E2=80=94 the old kernel zero-fills the extra fi= elds and ignores them, the ioctl succeeds, and `ret < 0` is false, correctl= y indicating the feature isn't supported. But the function also allocates a= 1MB dma-buf on old kernels as a side effect, which leaks until `dmabuf_fd`= is closed. Actually, looking again: `if (dmabuf_fd >=3D 0) close(dmabuf_fd= )` handles that case. OK, the detection logic is correct but fragile =E2=80= =94 it depends on the specific failure mode. A comment explaining the detec= tion strategy would help. **Issue 2 - test_memcontrol filtering:** The patch adds command-line filter= ing to test_memcontrol's main(): ```c + if (argc > 1) + filter =3D argv[1]; ``` This is a useful utility change but is unrelated to dma-buf accounting. It = should be a separate preparatory patch. **Issue 3 - Missing error path tests:** The selftests cover the happy path = (self pidfd, child pidfd, cross-cgroup accounting) but don't test: - Invalid pidfd (non-pidfd fd) - Pidfd pointing to an exited process - Allocation exceeding the target cgroup's memory limit - Simultaneous charging from multiple allocators **Issue 4 - `cg_read_key_long_poll` usage:** The test polls up to 15 times = with 200ms delays: ```c + if (dmabuf_stat !=3D ONE_MEG) + dmabuf_stat =3D cg_read_key_long_poll(child_cg, "memory.stat", + "dmabuf ", ONE_MEG, + 15, 200000); ``` The stat update should be synchronous (it happens in the charge path), so t= he need for polling suggests either the stat is lazy-flushed or there's a t= iming issue. If the charge is synchronous, a single read with a brief delay= should suffice. --- Generated by Claude Code Patch Reviewer