Re: [PATCH bpf-next v3] selftests/bpf: Add tests for memcg kfuncs

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2465 bytes --]

> selftests/bpf: Add tests for memcg kfuncs

TL;DR: Test-only nit: alloc_pagecache() in memcg_kfuncs.c duplicates the copy
in arena_memcg.c, so fixes must be made twice; consider moving it to a shared
helper such as cgroup_helpers.c.

> diff --git a/tools/testing/selftests/bpf/prog_tests/memcg_kfuncs.c b/tools/testing/selftests/bpf/prog_tests/memcg_kfuncs.c
> new file mode 100644
> index 000000000000..4527000ae408
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_kfuncs.c
> @@ -0,0 +1,306 @@

[ ... ]

> +/*
> + * Fill the page cache with @size bytes of clean, reclaimable pages by
> + * reading a sparse temp file. Returns the fd, which must stay open, or
> + * -EOPNOTSUPP if the working directory cannot back such a file, -1 on error.
> + */
> +static int alloc_pagecache(size_t size)
> +{
> +	struct statfs stfs;
> +	char buf[4096];
> +	size_t off;
> +	int fd;
> +
> +	fd = open(".", O_TMPFILE | O_RDWR | O_EXCL, 0600);
> +	if (fd < 0)
> +		return errno == EOPNOTSUPP ? -EOPNOTSUPP : -1;
> +	/* tmpfs hands out shmem pages, which are not reclaimable without swap */
> +	if (fstatfs(fd, &stfs) || stfs.f_type == TMPFS_MAGIC) {
> +		close(fd);
> +		return -EOPNOTSUPP;
> +	}
> +	if (ftruncate(fd, size))
> +		goto err;
> +	for (off = 0; off < size; off += sizeof(buf))
> +		if (read(fd, buf, sizeof(buf)) < 0)
> +			goto err;
> +	return fd;
> +err:
> +	close(fd);
> +	return -1;
> +}

this isn't a bug, but alloc_pagecache() here looks like a line-for-line
copy of the static alloc_pagecache() in prog_tests/arena_memcg.c.

Both versions have the same open(".", O_TMPFILE | O_RDWR | O_EXCL, 0600),
the same fstatfs()/TMPFS_MAGIC check, the same ftruncate() plus 4096-byte
read() loop, and the same -EOPNOTSUPP / -1 return contract.  The skip
message that goes with it, "no disk-backed O_TMPFILE in cwd", is also the
same string used in serial_test_arena_memcg().

If the reclaimability detection ever needs fixing, the fix has to be made
in both copies.  For example, ramfs also supports O_TMPFILE and its pages
are unevictable, but only TMPFS_MAGIC is checked.

Could alloc_pagecache() move to cgroup_helpers.c (or testing_helpers.c),
which test_progs already links, so both tests share one copy?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/38438128349