Re: [PATCH bpf-next v5 4/4] selftests/bpf: Add a test for arena fault-in under memory.max

> diff --git a/tools/testing/selftests/bpf/prog_tests/arena_memcg.c b/tools/testing/selftests/bpf/prog_tests/arena_memcg.c
> new file mode 100644
> index 000000000000..c57b98494cfa
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/arena_memcg.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include <fcntl.h>
> +#include <signal.h>
> +#include <sys/mman.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +#include <sys/user.h>
> +#ifndef PAGE_SIZE /* on some archs it comes in sys/user.h */
> +#include <unistd.h>
> +#define PAGE_SIZE getpagesize()
> +#endif

This isn't a bug, but are fcntl.h, signal.h and sys/mman.h needed here?
Nothing in the file appears to use open()/O_* flags, signal APIs (SIGSEGV
and SIGKILL appear only in comments, and WIFSIGNALED comes from
sys/wait.h), or mmap()/PROT_*/MAP_* constants.

Also, since unistd.h is already included above, could the copy inside the
PAGE_SIZE block be dropped?

> +
> +#include "cgroup_helpers.h"
> +#include "arena_memcg.skel.h"

[ ... ]

> +void serial_test_arena_memcg(void)
> +{

A subsystem pattern flags this as potentially concerning: a new standalone
test file is added to prog_tests/ for user-space arena fault-in, while
prog_tests/arena_mem_usage.c in the same directory already exercises that
same interface. arena_mem_usage.c's 'User-space fault-in' block does
exactly the same thing this test's inner loop does:

  base = bpf_map__initial_value(skel->maps.arena, &sz);
  ...
  for (i = 0; i < 8; i++)
          base[i * ps] = 1;

Both files are serial_ tests, both duplicate the same PAGE_SIZE preamble,
the same page-size local, and the same bpf_map__initial_value() /
base[i * ps] = 1 fault-in idiom, and progs/arena_memcg.c's map stanza is
byte-identical to progs/arena_mem_usage.c's apart from max_entries. The
question is whether this belongs as an additional case or subtest in the
existing arena fault-in test rather than a new binary-level test with its
own setup.

On the other hand, the two tests assert different behaviour, not a narrow
variant of one behaviour: arena_mem_usage.c asserts the memlock accounting
value after fault-in, while this test asserts that hitting memory.max
during fault-in produces a memcg OOM kill rather than a SIGSEGV on a valid
address. The new test also needs a harness the existing one does not have:
setup_cgroup_environment(), a cgroup created and joined before skeleton
load so the map is charged to the right memcg, a forked child (the arena
vma is VM_DONTCOPY and the parent must stay outside the limit so the memcg
OOM cannot pick test_progs), and exit-code-based error reporting because
ASSERT_* does not cross fork(). Folding that into arena_mem_usage.c would
put every one of its existing assertions behind a fork and a memory limit.

Is the separate file the right call here, or should this be integrated
into the existing test?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/arena_memcg.c b/tools/testing/selftests/bpf/progs/arena_memcg.c
> new file mode 100644
> index 000000000000..88259cfea06b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/arena_memcg.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_arena_common.h"
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_ARENA);
> +	__uint(map_flags, BPF_F_MMAPABLE);
> +	__uint(max_entries, 50000); /* number of pages */
> +#ifdef __TARGET_ARCH_arm64
> +	__ulong(map_extra, 0x1ull << 32); /* start of mmap() region */
> +#else
> +	__ulong(map_extra, 0x1ull << 44); /* start of mmap() region */
> +#endif
> +} arena SEC(".maps");
> +
> +SEC("syscall")
> +int noop(void *ctx)
> +{
> +	return 0;
> +}

This isn't a bug, but is the noop program load-bearing here - does the
object need at least one program for the skeleton/load path - or could
progs/arena_memcg.c be just the arena map definition? The test only calls
arena_memcg__open_and_load() and then works through bpf_map__initial_value()
/ bpf_map__max_entries() on maps.arena, so cskel->progs.noop is never
referenced.


---
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/32713810092