Hello Guopeng. +Cc Leon Huang Fu <[email protected]>
On Thu, Nov 20, 2025 at 02:04:06PM +0800, Guopeng Zhang <[email protected]> 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). (FTR, I remember there is `echo 1 > /sys/module/rcutree/parameters/do_rcu_barrier`, however, I'm not sure it works always as expected (a reader may actually wait for multi-stage RCU pipeline), so plain timeout is more reliable.) > - 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. I like the approach of adaptive waiting to settle in such tests. > The timeout is set to 3 seconds to cover the periodic rstat flush > interval (FLUSH_TIME = 2*HZ by default) plus some scheduling slack. 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. BTW Have you looked into the number of retries until success? Was it in accordance with the flushing interval? > > Suggested-by: Lance Yang <[email protected]> > Reviewed-by: Lance Yang <[email protected]> > Signed-off-by: Guopeng Zhang <[email protected]> > --- > v3: > - Move MEMCG_SOCKSTAT_WAIT_* defines after the #include block as > suggested. > v2: > - Mention the periodic rstat flush interval (FLUSH_TIME = 2*HZ) in > the comment and clarify the rationale for the 3s timeout. > - Replace the hard-coded retry count and wait interval with macros > to avoid magic numbers and make the 3s timeout calculation explicit. > --- > .../selftests/cgroup/test_memcontrol.c | 30 ++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c > b/tools/testing/selftests/cgroup/test_memcontrol.c > index 4e1647568c5b..8ff7286fc80b 100644 > --- a/tools/testing/selftests/cgroup/test_memcontrol.c > +++ b/tools/testing/selftests/cgroup/test_memcontrol.c > @@ -21,6 +21,9 @@ > #include "kselftest.h" > #include "cgroup_util.h" > > +#define MEMCG_SOCKSTAT_WAIT_RETRIES 30 /* 3s total */ > +#define MEMCG_SOCKSTAT_WAIT_INTERVAL_US (100 * 1000) /* 100 ms */ > + > static bool has_localevents; > static bool has_recursiveprot; > > @@ -1384,6 +1387,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; > > memcg = cg_name(root, "memcg_test"); > if (!memcg) > @@ -1432,7 +1437,30 @@ 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, which runs periodically (every 2 seconds, > + * see FLUSH_TIME). On a busy system, 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 (~FLUSH_TIME plus some > + * scheduling slack) and require that the "sock " counter > + * eventually drops to zero. > + */ > + for (i = 0; i < MEMCG_SOCKSTAT_WAIT_RETRIES; i++) { > + sock_post = cg_read_key_long(memcg, "memory.stat", "sock "); > + if (sock_post < 0) > + goto cleanup; > + > + if (!sock_post) > + break; > + > + usleep(MEMCG_SOCKSTAT_WAIT_INTERVAL_US); > + } I think this may be useful also for othe tests (at least other memory.stat checks), so some encapsulated implementation like a macro with parameters cg_read_assert_gt_with_retries(cg, file, field, exp, timeout, retries) WDYT? Michal
signature.asc
Description: PGP signature
