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

[PATCH 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.

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..37e2cf249be8 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) {
+		munmap(map, len);
+		usleep(MEMCG_STAT_RETRY_DELAY_US);
+		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) {
+		munmap(map, len);
+		usleep(MEMCG_STAT_RETRY_DELAY_US);
+		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) {
+		munmap(map, len);
+		usleep(MEMCG_STAT_RETRY_DELAY_US);
+		goto retry;
+	}
+
 	ASSERT_GT(memcg_query->pgfault, 0, "final pgfault val");
 
 cleanup:
-- 
2.53.0-Meta

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 5459 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 b7c18d590b993..37e2cf249be8a 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) {
> +		munmap(map, len);
> +		usleep(MEMCG_STAT_RETRY_DELAY_US);
> +		goto retry;
> +	}
> +

Does unmapping before the sleep undermine the retry mechanism for gauge
counters?

Looking at NR_ANON_MAPPED behavior: page_remove_rmap() posts -1024 when
munmap() runs, cancelling the +1024 from the memset. Any flush that lands
during the 250 ms sleep sees 0 mapped pages. The region is only mapped for
the brief mmap+memset+read_stats window but unmapped for the full sleep.

The commit message says "sleeping in between, so that a flusher cycle is
always covered." But if the 2s periodic flusher fires while the region is
unmapped, won't mem_cgroup_stat_aggregate() fold the net zero into
memcg->vmstats->state and reset stats_updates to 0?

That would leave only the threshold path (stats_updates >
MEMCG_CHARGE_BATCH * num_online_cpus()), which relies on memcg_rstat_updated()
accumulating abs(val). Each pass contributes ~2048 (1024 for faults + 1024
for unmap), bounded by the 2s flush interval: ~8 passes = ~16k, versus a
threshold of 64 * num_online_cpus(). Could this still read 0 at 256+ CPUs?

Would keeping the mapping alive across the sleep and unmapping once at
cleanup ensure the periodic flusher sees +1024 instead?


>  	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) {
> +		munmap(map, len);
> +		usleep(MEMCG_STAT_RETRY_DELAY_US);
> +		goto retry;
> +	}
> +

Does this retry condition allow early exit when only one counter is flushed?

The loop retries while BOTH nr_file_pages AND nr_file_mapped are zero, but
the assertions below expect BOTH to be non-zero. The two counters have
different lifetimes:

When munmap() runs, NR_FILE_MAPPED drops back to 0 (rmap gauge) but the
pages stay in the page cache so NR_FILE_PAGES keeps its +1024.

If the 2s periodic flusher fires during the 250 ms sleep between passes:

  mem_cgroup_stat_aggregate() folds the net per-cpu deltas
  -> memcg->vmstats->state: NR_FILE_PAGES = 1024, NR_FILE_MAPPED = 0
  -> stats_updates reset to 0

Then pass 2 does ftruncate at the same length (no-op), mmap+memset of
already-cached pages -> only NR_FILE_MAPPED +1024 pending, still below the
8192 threshold at 128 CPUs.

So memcg_page_state() returns nr_file_pages = 1024 (non-zero) and
nr_file_mapped = 0. The predicate `!nr_file_pages && !nr_file_mapped` is
false, loop exits, and ASSERT_GT(memcg_query->nr_file_mapped, 0) fails.

Since the unmapped window is 250 ms while the mapped window is just
mmap+memset+read_stats, won't the flusher land in the unmapped state with
high probability? Should this use || instead of && to retry while either
counter is zero?


>  	ASSERT_GT(memcg_query->nr_file_pages, 0, "final file value");
>  	ASSERT_GT(memcg_query->nr_file_mapped, 0, "final file mapped value");
>

[ ... ]


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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The retry loops in test_anon and test_file destroy the memcg state before waiting for the periodic flusher, causing the tests to reliably fail. Additionally, test_file uses a flawed retry condition.
--

--- Patch [1]: [PATCH bpf-next] selftests/bpf: Retry stat generation in cgroup_iter_memcg ---
commit 6d80dc385d952485dfd4001f328081e6d5523309
Author: Andrii Nakryiko <andrii@kernel.org>

selftests/bpf: Retry stat generation in cgroup_iter_memcg

This commit introduces retry loops in the cgroup_iter_memcg selftest
to wait for the periodic memcg stat flusher. This addresses test failures
on large core-count systems where a single pass does not reach the flush
threshold.

> 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 b7c18d590b993..37e2cf249be8a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/cgroup_iter_memcg.c
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_iter_memcg.c
[ ... ]
> @@ -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) {
> +		munmap(map, len);
> +		usleep(MEMCG_STAT_RETRY_DELAY_US);
> +		goto retry;
> +	}

[Severity: Medium]
Does unmapping the memory before sleeping prevent the periodic flusher from
observing the mapped state?

If the mapped memory is unmapped via munmap() before the sleep, the periodic
flusher running during usleep() will observe a net-zero state for
nr_anon_mapped. The retry loop seems to repeatedly map and unmap the memory
without letting the flusher see the steady state, which might prevent the test
from ever succeeding.

[ ... ]
> @@ -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) {
> +		munmap(map, len);
> +		usleep(MEMCG_STAT_RETRY_DELAY_US);
> +		goto retry;
> +	}

[Severity: Medium]
Could this condition cause the retry loop to exit prematurely?

In this path, munmap() removes the mapping but leaves the file in the page
cache. The periodic flusher will flush a positive nr_file_pages but a zero
nr_file_mapped.

On the next iteration, memcg_query->nr_file_pages will be non-zero, causing
the && condition to evaluate to false and the loop to exit early before the
mapped stats are satisfied.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813213701.3116369-1-andrii@kernel.org?part=1