From: Lance Yang <[email protected]>
On Wed, 19 Nov 2025 18:52:16 +0800, Guopeng Zhang wrote: > test_memcg_sock() currently requires that memory.stat's "sock " counter > is exactly zero immediately after the TCP server exits. On a busy system > this assumption is too strict: > > - Socket memory may be freed with a small delay (e.g. RCU callbacks). > - memcg statistics are updated asynchronously via the rstat flushing > worker, so the "sock " value in memory.stat can stay non-zero for a > short period of time even after all socket memory has been uncharged. > > As a result, test_memcg_sock() can intermittently fail even though socket > memory accounting is working correctly. > > Make the test more robust by polling memory.stat for the "sock " counter > and allowing it some time to drop to zero instead of checking it only > once. If the counter does not become zero within the timeout, the test > still fails as before. > > On my test system, running test_memcontrol 50 times produced: > > - Before this patch: 6/50 runs passed. > - After this patch: 50/50 runs passed. Good catch! Thanks! With more CPU cores, updates may be distributed across cores, making it slower to reach the per-CPU flush threshold, IIUC :) > > Signed-off-by: Guopeng Zhang <[email protected]> > --- > .../selftests/cgroup/test_memcontrol.c | 24 ++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c > b/tools/testing/selftests/cgroup/test_memcontrol.c > index 4e1647568c5b..86d9981cddd8 100644 > --- a/tools/testing/selftests/cgroup/test_memcontrol.c > +++ b/tools/testing/selftests/cgroup/test_memcontrol.c > @@ -1384,6 +1384,8 @@ static int test_memcg_sock(const char *root) > int bind_retries = 5, ret = KSFT_FAIL, pid, err; > unsigned short port; > char *memcg; > + long sock_post = -1; > + int i, retries = 30; > > memcg = cg_name(root, "memcg_test"); > if (!memcg) > @@ -1432,7 +1434,27 @@ static int test_memcg_sock(const char *root) > if (cg_read_long(memcg, "memory.current") < 0) > goto cleanup; > > - if (cg_read_key_long(memcg, "memory.stat", "sock ")) > + /* > + * memory.stat is updated asynchronously via the memcg rstat > + * flushing worker, so the "sock " counter may stay non-zero > + * for a short period of time after the TCP connection is > + * closed and all socket memory has been uncharged. > + * > + * Poll memory.stat for up to 3 seconds and require that the > + * "sock " counter eventually drops to zero. It might be worth mentioning that the current periodic rstat flush happens every 2 seconds (#define FLUSH_TIME (2UL*HZ)). Adding this context to the comment would explain why the 3-second timeout was chosen ;) > + */ > + for (i = 0; i < retries; i++) { > + sock_post = cg_read_key_long(memcg, "memory.stat", "sock "); > + if (sock_post < 0) > + goto cleanup; > + > + if (!sock_post) > + break; > + > + usleep(100 * 1000); /* 100ms */ Nit: It would be better to define the retry count and interval as macros (e.g., MAX_RETRIES, WAIT_INTERVAL) to avoid magic numbers and make the 3s timeout calculation explicit. > + } > + > + if (sock_post) > goto cleanup; > > ret = KSFT_PASS; Thanks, Lance
