Re: [PATCH v3 8/8] selftests/dmabuf-heaps: Add dma-buf memcg accounting tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] selftests/dmabuf-heaps: check_deps() runs before handle_build() in vmtest.sh
- [Medium] selftests/dmabuf-heaps: fail instead of skip on EOPNOTSUPP in XFER_CHARGE test
- [Medium] selftests/dmabuf-heaps: pidfd_alloc_supported() treats EOPNOTSUPP as supported
- [Medium] selftests: dmabuf-heaps: no memory controller check in open_self_cgroupfd()
- [Medium] selftests: cgroup: MEMCG_DMABUF_WAIT_RETRIES is shorter than 2s rstat FLUSH_TIME
--

commit 764a620a8574ac6b7e6c57e6f71e4c743c373656
Author: Albert Esteve <aesteve@redhat.com>

selftests/dmabuf-heaps: Add dma-buf memcg accounting tests

This commit adds selftests for dma-buf memcg accounting via charge_fd in
DMA_HEAP_IOCTL_ALLOC and DMA_BUF_IOCTL_XFER_CHARGE across the dmabuf-heaps
and cgroup test suites, along with a vmtest.sh runner script.

> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index 0ed82347044ed..0b41c3c6f62c1 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> @@ -19,10 +19,20 @@
>  #include <errno.h>
>  #include <sys/mman.h>
>  
> +#include <linux/dma-buf.h>
> +#include <linux/dma-heap.h>
> +#include <signal.h>
> +#include <sys/ioctl.h>
> +
> +#include "../pidfd/pidfd.h"
>  #include "kselftest.h"
>  #include "cgroup_util.h"
>  
> +#define DMA_HEAP_SYSTEM		"/dev/dma_heap/system"
> +#define ONE_MEG			(1024 * 1024)
> +
>  #define MEMCG_SOCKSTAT_WAIT_RETRIES        30
> +#define MEMCG_DMABUF_WAIT_RETRIES          15

[Severity: Medium]
Is MEMCG_DMABUF_WAIT_RETRIES (15 retries at 100ms intervals, or 1.4 seconds
before the final read) long enough to avoid races with the periodic rstat
flush in test_memcg_dmabuf() and test_memcg_dmabuf_xfer_at_limit()?

In mm/memcontrol.c, reading memory.stat calls mem_cgroup_flush_stats(), which
only flushes rstat synchronously when stats_updates exceeds
MEMCG_CHARGE_BATCH * num_online_cpus() (64 * num_online_cpus()):

mm/memcontrol.c:mem_cgroup_flush_stats()
    if (atomic_long_read(&vmstats->stats_updates) >
        MEMCG_CHARGE_BATCH * num_online_cpus())
        ...

Charging or uncharging a ONE_MEG (1 MB) dma-buf in a fresh child cgroup
updates MEMCG_DMABUF by 256 pages on 4KB-page kernels (or 16 pages on
64KB-page kernels). When num_online_cpus() >= 4 (where 256 > 64 * 4 is false)
or on >4KB-page kernels, reading memory.stat skips synchronous flushing and
relies on the periodic deferred work stats_flush_dwork, which runs every
FLUSH_TIME (2UL * HZ, or 2.0 seconds).

In test_memcg_dmabuf(), even if the first poll for ONE_MEG catches a periodic
flush, that flush resets stats_updates to 0 and schedules the next
stats_flush_dwork 2.0 seconds later. Immediately closing dmabuf_fd adds only
256 to stats_updates, so the subsequent 1.4-second poll for dmabuf == 0 can
time out before the next 2.0-second periodic flush runs. Should
MEMCG_DMABUF_WAIT_RETRIES be at least 30, matching
MEMCG_SOCKSTAT_WAIT_RETRIES?

> diff --git a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> index 1d49df6719198..90ef14d2ecba0 100644
> --- a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> +++ b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c

[ ... ]

> @@ -496,6 +505,218 @@ static void test_alloc_no_fd_leak_on_efault(char *heap_name)

[ ... ]

> +/*
> + * Probe whether the kernel honours charge_fd in DMA_HEAP_IOCTL_ALLOC.
> + */
> +static bool pidfd_alloc_supported(int heap_fd)
> +{
> +	int devnull_fd, dmabuf_fd = -1, ret;
> +
> +	devnull_fd = open("/dev/null", O_RDONLY);
> +	if (devnull_fd < 0)
> +		return false;
> +
> +	ret = dmabuf_heap_alloc_pidfd(heap_fd, ONE_MEG, 0, devnull_fd, &dmabuf_fd);
> +	if (dmabuf_fd >= 0) {
> +		close(dmabuf_fd);
> +		dmabuf_fd = -1;
> +	}
> +	close(devnull_fd);
> +	return ret < 0;
> +}

[Severity: Medium]
What happens in pidfd_alloc_supported() when the kernel is built with
CONFIG_MEMCG=n?

In drivers/dma-buf/dma-heap.c, dma_heap_ioctl_allocate() passes a non-zero
charge_fd to mem_cgroup_get_from_fd():

drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate()
    if (heap_allocation->charge_fd) {
        memcg = mem_cgroup_get_from_fd(heap_allocation->charge_fd);
        if (IS_ERR(memcg))
            return ERR_CAST(memcg);
    }

When CONFIG_MEMCG is disabled, the stub in include/linux/memcontrol.h returns
ERR_PTR(-EOPNOTSUPP):

include/linux/memcontrol.h:mem_cgroup_get_from_fd()
    static inline struct mem_cgroup *mem_cgroup_get_from_fd(int fd)
    {
        return ERR_PTR(-EOPNOTSUPP);
    }

Because dmabuf_heap_alloc_pidfd() returns -1 with errno == EOPNOTSUPP,
returning ret < 0 causes pidfd_alloc_supported() to return true when
CONFIG_MEMCG=n, and test_alloc_pidfd_self(), test_alloc_pidfd_child(),
test_alloc_cgroupfd_self(), and test_xfer_charge_cgroupfd_self() then proceed
and report test failures instead of skipping. Should pidfd_alloc_supported()
also verify that errno != EOPNOTSUPP (or errno == EINVAL)?

[ ... ]

> +static int open_self_cgroupfd(void)
> +{
> +	char cgroup_path[PATH_MAX];
> +	char buf[PATH_MAX];
> +	FILE *f;
> +
> +	f = fopen("/proc/self/cgroup", "r");
> +	if (!f)
> +		return -1;
> +
> +	cgroup_path[0] = '\0';
> +	while (fgets(buf, sizeof(buf), f)) {
> +		char *p = buf;
> +
> +		if (strncmp(p, "0::", 3) == 0) {
> +			p += 3;
> +			p[strcspn(p, "\n")] = '\0';
> +			snprintf(cgroup_path, sizeof(cgroup_path),
> +				 "/sys/fs/cgroup%s", p);
> +			break;
> +		}
> +	}
> +	fclose(f);
> +
> +	if (!cgroup_path[0])
> +		return -1;
> +
> +	return open(cgroup_path, O_RDONLY | O_DIRECTORY);
> +}
> +
> +/*
> + * Test: allocate charging the calling process's own cgroup via a cgroupfd.
> + */
> +static void test_alloc_cgroupfd_self(char *heap_name)
> +{
> +	int heap_fd = -1, cgroup_fd = -1, dmabuf_fd = -1, ret;
> +
> +	heap_fd = dmabuf_heap_open(heap_name);
> +
> +	if (!pidfd_alloc_supported(heap_fd)) {
> +		ksft_test_result_skip("charge_fd not supported by this kernel\n");
> +		goto out;
> +	}
> +
> +	cgroup_fd = open_self_cgroupfd();
> +	if (cgroup_fd < 0) {
> +		ksft_test_result_skip("cannot open self cgroupfd\n");
> +		goto out;
> +	}
> +
> +	ret = dmabuf_heap_alloc_pidfd(heap_fd, ONE_MEG, 0, cgroup_fd, &dmabuf_fd);
> +	ksft_test_result(!ret, "Allocation with cgroupfd %d\n", ret);
> +	if (dmabuf_fd >= 0)
> +		close(dmabuf_fd);
> +	close(cgroup_fd);
> +out:
> +	close(heap_fd);
> +}

[Severity: Medium]
Can test_alloc_cgroupfd_self() and test_xfer_charge_cgroupfd_self() fail with
ENOENT instead of skipping when the test binary is run outside vmtest.sh (for
example via make run_tests or vmtest.sh -s) in a cgroup where the memory
controller is not enabled?

Unlike the pidfd path in mem_cgroup_get_from_fd(), which uses
get_mem_cgroup_from_mm() and walks up to an online ancestor, the cgroupfd
path requires the memory controller CSS to be directly enabled on the target
cgroup itself:

mm/memcontrol.c:mem_cgroup_get_from_fd()
    cgrp = cgroup_get_from_fd(fd);
    if (!IS_ERR(cgrp)) {
        rcu_read_lock();
        css = cgroup_css(cgrp, &memory_cgrp_subsys);
        if (!css || !css_tryget_online(css)) {
            rcu_read_unlock();
            cgroup_put(cgrp);
            return ERR_PTR(-ENOENT);
        }

When dmabuf-heap runs in a leaf cgroup (such as a standard systemd user or
service scope) whose parent has not enabled +memory in
cgroup.subtree_control, or when booted with cgroup_disable=memory,
open_self_cgroupfd() still succeeds in opening the directory, and the ioctl
fails with ENOENT. Should open_self_cgroupfd() check that memory is active in
that cgroup (or should these two tests skip when the ioctl fails with
errno == ENOENT)?

> +/*
> + * Test: transfer a dma-buf's charge to self cgroup via DMA_BUF_IOCTL_XFER_CHARGE.
> + */
> +static void test_xfer_charge_cgroupfd_self(char *heap_name)
> +{
> +	int heap_fd = -1, cgroup_fd = -1, dmabuf_fd = -1, ret;
> +	struct dma_buf_transfer_charge xfer = { 0 };
> +
> +	heap_fd = dmabuf_heap_open(heap_name);
> +
> +	if (!pidfd_alloc_supported(heap_fd)) {
> +		ksft_test_result_skip("charge_fd not supported by this kernel\n");
> +		goto out;
> +	}
> +
> +	ret = dmabuf_heap_alloc(heap_fd, ONE_MEG, 0, &dmabuf_fd);
> +	if (ret) {
> +		ksft_test_result_fail("Allocation failed %d\n", ret);
> +		goto out;
> +	}
> +
> +	cgroup_fd = open_self_cgroupfd();
> +	if (cgroup_fd < 0) {
> +		ksft_test_result_skip("cannot open self cgroupfd\n");
> +		close(dmabuf_fd);
> +		goto out;
> +	}
> +
> +	xfer.charge_fd = cgroup_fd;
> +	ret = ioctl(dmabuf_fd, DMA_BUF_IOCTL_XFER_CHARGE, &xfer);
> +	ksft_test_result(!ret, "XFER_CHARGE to self cgroupfd %d\n", ret);

[Severity: Medium]
Does test_xfer_charge_cgroupfd_self() report a failure instead of skipping
when main() iterates over non-system heaps in /dev/dma_heap (such as CMA
heaps when CONFIG_DMABUF_HEAPS_CMA=y)?

In drivers/dma-buf/dma-heap.c, dma_heap_ioctl_allocate() validates charge_fd
before calling heap->ops->allocate(), so pidfd_alloc_supported() returns true
for all heaps. However, in drivers/dma-buf/dma-buf.c, dma_buf_xfer_charge()
checks whether the exporter enabled memcg accounting on the buffer:

drivers/dma-buf/dma-buf.c:dma_buf_xfer_charge()
    if (!dmabuf->memcg_account)
        return -EOPNOTSUPP;

Because only system_heap sets exp_info.memcg_account = true (while exporters
such as cma_heap leave it false), ioctl(dmabuf_fd, DMA_BUF_IOCTL_XFER_CHARGE,
&xfer) returns -1 with errno == EOPNOTSUPP on those heaps. Should
test_xfer_charge_cgroupfd_self() call ksft_test_result_skip() when ret < 0
and errno == EOPNOTSUPP?

> diff --git a/tools/testing/selftests/dmabuf-heaps/vmtest.sh b/tools/testing/selftests/dmabuf-heaps/vmtest.sh
> new file mode 100755
> index 0000000000000..3574b71ca83e8
> --- /dev/null
> +++ b/tools/testing/selftests/dmabuf-heaps/vmtest.sh

[ ... ]

> +check_deps() {
> +	for dep in vng make; do
> +		if [[ ! -x $(command -v "${dep}") ]]; then
> +			echo -e "skip:    dependency ${dep} not found!\n"
> +			exit "${KSFT_SKIP}"
> +		fi
> +	done
> +
> +	if ${SHELL_MODE}; then
> +		return
> +	fi
> +
> +	if [[ ! -x "${DMABUF_HEAP_TEST}" ]]; then
> +		printf "skip:    %s not found!" "${DMABUF_HEAP_TEST}"
> +		printf " Please build the kselftest dmabuf-heaps target (or use -b).\n"
> +		exit "${KSFT_SKIP}"
> +	fi
> +
> +	if [[ ! -x "${MEMCONTROL_TEST}" ]]; then
> +		printf "skip:    %s not found!" "${MEMCONTROL_TEST}"
> +		printf " Please build the kselftest cgroup target (or use -b).\n"
> +		exit "${KSFT_SKIP}"
> +	fi
> +}

[ ... ]

> +trap cleanup EXIT
> +
> +check_deps
> +check_vng
> +handle_build
> +make_runner

[Severity: Medium]
If vmtest.sh is invoked with -b on a clean tree where the selftest binaries
have not been built yet, won't check_deps() exit with KSFT_SKIP before
handle_build() has a chance to build them?

In the main flow, check_deps() runs before handle_build(), and inside
check_deps() only SHELL_MODE returns early before checking for
DMABUF_HEAP_TEST and MEMCONTROL_TEST. Should check_deps() also skip the
binary existence checks when BUILD is true (or run after handle_build())?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-v2_20230123_tjmercier_google_com-v3-0-78d2c4cb8647@redhat.com?part=8