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

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 <[email protected]>

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/[email protected]?part=1