[PATCH v2 bpf-next] selftests/bpf: Retry stat generation in cgroup_iter_memcg

[PATCH v2 bpf-next] selftests/bpf: Retry stat generation in cgroup_iter_memcg

[Andrii Nakryiko]

Each cgroup_iter_memcg subtest touches 1024 pages and expects the matching
memcg counter to be non-zero. On a host with many CPUs it reads zero
instead:

  test_anon:FAIL:final anon mapped val: actual 0 <= expected 0

memcg stats are cached per-cpu and only become visible once the periodic
flusher runs (FLUSH_TIME, 2s), or once pending updates cross
MEMCG_CHARGE_BATCH * num_online_cpus(). That threshold is 512 pages at 8
CPUs but 8192 at 128, so a single pass no longer reaches it and
bpf_mem_cgroup_flush_stats() returns without flushing anything.

Retry the stat generation, sleeping in between, so that a flusher cycle is
always covered. Sleep before dropping the mapping, so that a flusher cycle
landing in the sleep observes the mapped state. nr_anon_mapped and
nr_file_mapped are rmap gauges, and unmapping first would post a matching
negative delta for the flusher to aggregate to a net zero.

test_file asserts on both nr_file_pages and nr_file_mapped, which have
different lifetimes, as page cache pages outlive the mapping. Retry
while either one is still zero.

Fixes: 6bce6ddbe634 ("bpf: selftests: selftests for memcg stat kfuncs")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 .../bpf/prog_tests/cgroup_iter_memcg.c        | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_iter_memcg.c b/tools/testing/selftests/bpf/prog_tests/cgroup_iter_memcg.c
index b7c18d590b99..5a1e08d39a06 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_iter_memcg.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_iter_memcg.c
@@ -10,6 +10,17 @@
 #include "cgroup_iter_memcg.h"
 #include "cgroup_iter_memcg.skel.h"
 
+/*
+ * memcg stats are cached per-cpu and only become visible once the periodic
+ * flusher runs (FLUSH_TIME, 2s), or once pending updates cross
+ * MEMCG_CHARGE_BATCH * num_online_cpus(). That threshold grows with the CPU
+ * count, so on a large machine a single pass does not reach it and
+ * bpf_mem_cgroup_flush_stats() returns without flushing anything. Retry for
+ * long enough to cover a flusher cycle.
+ */
+#define MEMCG_STAT_RETRIES		16
+#define MEMCG_STAT_RETRY_DELAY_US	(250 * 1000)
+
 static int read_stats(struct bpf_link *link)
 {
 	int fd, ret = 0;
@@ -35,11 +46,13 @@ static int read_stats(struct bpf_link *link)
 
 static void test_anon(struct bpf_link *link, struct memcg_query *memcg_query)
 {
+	int retries = 0;
 	void *map;
 	size_t len;
 
 	len = sysconf(_SC_PAGESIZE) * 1024;
 
+retry:
 	/*
 	 * Increase memcg anon usage by mapping and writing
 	 * to a new anon region.
@@ -53,6 +66,12 @@ static void test_anon(struct bpf_link *link, struct memcg_query *memcg_query)
 	if (!ASSERT_OK(read_stats(link), "read stats"))
 		goto cleanup;
 
+	if (!memcg_query->nr_anon_mapped && ++retries < MEMCG_STAT_RETRIES) {
+		usleep(MEMCG_STAT_RETRY_DELAY_US);
+		munmap(map, len);
+		goto retry;
+	}
+
 	ASSERT_GT(memcg_query->nr_anon_mapped, 0, "final anon mapped val");
 
 cleanup:
@@ -61,6 +80,7 @@ static void test_anon(struct bpf_link *link, struct memcg_query *memcg_query)
 
 static void test_file(struct bpf_link *link, struct memcg_query *memcg_query)
 {
+	int retries = 0;
 	void *map;
 	size_t len;
 	char *path;
@@ -76,6 +96,7 @@ static void test_file(struct bpf_link *link, struct memcg_query *memcg_query)
 	fd = open(path, O_CREAT | O_RDWR, 0644);
 	if (!ASSERT_OK_FD(fd, "open fd"))
 		return;
+retry:
 	if (!ASSERT_OK(ftruncate(fd, len), "ftruncate"))
 		goto cleanup_fd;
 
@@ -88,6 +109,13 @@ static void test_file(struct bpf_link *link, struct memcg_query *memcg_query)
 	if (!ASSERT_OK(read_stats(link), "read stats"))
 		goto cleanup_map;
 
+	if ((!memcg_query->nr_file_pages || !memcg_query->nr_file_mapped) &&
+	    ++retries < MEMCG_STAT_RETRIES) {
+		usleep(MEMCG_STAT_RETRY_DELAY_US);
+		munmap(map, len);
+		goto retry;
+	}
+
 	ASSERT_GT(memcg_query->nr_file_pages, 0, "final file value");
 	ASSERT_GT(memcg_query->nr_file_mapped, 0, "final file mapped value");
 
@@ -100,6 +128,7 @@ static void test_file(struct bpf_link *link, struct memcg_query *memcg_query)
 
 static void test_shmem(struct bpf_link *link, struct memcg_query *memcg_query)
 {
+	int retries = 0;
 	size_t len;
 	int fd;
 
@@ -113,12 +142,18 @@ static void test_shmem(struct bpf_link *link, struct memcg_query *memcg_query)
 	if (!ASSERT_OK_FD(fd, "memfd_create"))
 		return;
 
+retry:
 	if (!ASSERT_OK(fallocate(fd, 0, 0, len), "fallocate"))
 		goto cleanup;
 
 	if (!ASSERT_OK(read_stats(link), "read stats"))
 		goto cleanup;
 
+	if (!memcg_query->nr_shmem && ++retries < MEMCG_STAT_RETRIES) {
+		usleep(MEMCG_STAT_RETRY_DELAY_US);
+		goto retry;
+	}
+
 	ASSERT_GT(memcg_query->nr_shmem, 0, "final shmem value");
 
 cleanup:
@@ -127,11 +162,13 @@ static void test_shmem(struct bpf_link *link, struct memcg_query *memcg_query)
 
 static void test_pgfault(struct bpf_link *link, struct memcg_query *memcg_query)
 {
+	int retries = 0;
 	void *map;
 	size_t len;
 
 	len = sysconf(_SC_PAGESIZE) * 1024;
 
+retry:
 	/* Create region to use for triggering a page fault. */
 	map = mmap(NULL, len, PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
 	if (!ASSERT_NEQ(map, MAP_FAILED, "mmap anon"))
@@ -143,6 +180,12 @@ static void test_pgfault(struct bpf_link *link, struct memcg_query *memcg_query)
 	if (!ASSERT_OK(read_stats(link), "read stats"))
 		goto cleanup;
 
+	if (!memcg_query->pgfault && ++retries < MEMCG_STAT_RETRIES) {
+		usleep(MEMCG_STAT_RETRY_DELAY_US);
+		munmap(map, len);
+		goto retry;
+	}
+
 	ASSERT_GT(memcg_query->pgfault, 0, "final pgfault val");
 
 cleanup:
-- 
2.53.0-Meta

Re: [PATCH v2 bpf-next] selftests/bpf: Retry stat generation in cgroup_iter_memcg

[bot+bpf-ci]

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

> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_iter_memcg.c b/tools/testing/selftests/bpf/prog_tests/cgroup_iter_memcg.c
> index b7c18d590b99..5a1e08d39a06 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_iter_memcg.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_iter_memcg.c

[ ... ]

> @@ -100,6 +128,7 @@ static void test_file(struct bpf_link *link, struct memcg_query *memcg_query)
>
>  static void test_shmem(struct bpf_link *link, struct memcg_query *memcg_query)
>  {
> +	int retries = 0;
>  	size_t len;
>  	int fd;
>
> @@ -113,12 +142,18 @@ static void test_shmem(struct bpf_link *link, struct memcg_query *memcg_query)
>  	if (!ASSERT_OK_FD(fd, "memfd_create"))
>  		return;
>
> +retry:
>  	if (!ASSERT_OK(fallocate(fd, 0, 0, len), "fallocate"))
>  		goto cleanup;
>
>  	if (!ASSERT_OK(read_stats(link), "read stats"))
>  		goto cleanup;
>
> +	if (!memcg_query->nr_shmem && ++retries < MEMCG_STAT_RETRIES) {
> +		usleep(MEMCG_STAT_RETRY_DELAY_US);
> +		goto retry;
> +	}
> +
>  	ASSERT_GT(memcg_query->nr_shmem, 0, "final shmem value");
>
>  cleanup:

Does the retry loop in test_shmem actually regenerate stat updates the way
test_anon, test_file, and test_pgfault do?

The retry: label sits above the fallocate() call, and nothing inside the
loop tears down the allocation. From the second iteration on, the range is
already populated. shmem_fallocate() in mm/shmem.c (mode 0 path) walks
each index with shmem_get_folio(..., SGP_FALLOC), every index becomes a
page-cache hit, and no new charge or NR_SHMEM/NR_FILE_PAGES delta gets
posted to vmstats->stats_updates.

So vmstats->stats_updates stays where the first pass left it (around 1-4k
units on the 128-CPU host the commit message mentions), which is well
under the 64 * 128 = 8192 threshold. That leaves
memcg_vmstats_needs_flush() returning false, and
bpf_mem_cgroup_flush_stats() exiting without flushing.

test_anon, test_file, and test_pgfault all munmap and then remap+refault
1024 pages on each iteration, posting a fresh ~2k abs() worth of deltas,
so they can reach both mechanism (a) the periodic flusher and mechanism
(b) crossing MEMCG_CHARGE_BATCH * num_online_cpus(). test_shmem appears
to depend entirely on stats_flush_dwork landing inside the 15 x 250ms =
3.75s window.

That is still better than the original single-pass behavior, but on a
large, mostly-idle host with NOHZ, deferrable timers (which
stats_flush_dwork is, per mm/memcontrol.c DECLARE_DEFERRABLE_WORK) may
not fire reliably while the machine is idle.

Would moving retry: above memfd_create(), or punching a hole with
FALLOC_FL_PUNCH_HOLE after the usleep() and before re-fallocating, give
test_shmem the same regeneration the other three subtests have? As the
commit message notes for nr_anon_mapped, the hole punch would need to come
after the sleep so the flusher observes the allocated state.


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