| Message ID | 20220422155728.3055914-5-void@manifault.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix bugs in memcontroller cgroup tests | expand |
On Fri, Apr 22, 2022 at 08:57:28AM -0700, David Vernet wrote: > test_memcg_sock() in the cgroup memcg tests, verifies expected memory > accounting for sockets. The test forks a process which functions as a TCP > server, and sends large buffers back and forth between itself (as the TCP > client) and the forked TCP server. While doing so, it verifies that > memory.current and memory.stat.sock look correct. > > There is currently a check in tcp_client() which asserts memory.current >= > memory.stat.sock. This check is racy, as between memory.current and > memory.stat.sock being queried, a packet could come in which causes > mem_cgroup_charge_skmem() to be invoked. This could cause memory.stat.sock > to exceed memory.current. Reversing the order of querying doesn't address > the problem either, as memory may be reclaimed between the two calls. But just curious, does it fix the flakiness (assuming there is no memory pressure)? > Instead, this patch just removes that assertion altogether, and instead > relies on the values_close() check that follows to validate the expected > accounting. Acked-by: Roman Gushchin <roman.gushchin@linux.dev>
On Fri, Apr 22, 2022 at 04:50:12PM -0700, Roman Gushchin wrote: > On Fri, Apr 22, 2022 at 08:57:28AM -0700, David Vernet wrote: > > test_memcg_sock() in the cgroup memcg tests, verifies expected memory > > accounting for sockets. The test forks a process which functions as a TCP > > server, and sends large buffers back and forth between itself (as the TCP > > client) and the forked TCP server. While doing so, it verifies that > > memory.current and memory.stat.sock look correct. > > > > There is currently a check in tcp_client() which asserts memory.current >= > > memory.stat.sock. This check is racy, as between memory.current and > > memory.stat.sock being queried, a packet could come in which causes > > mem_cgroup_charge_skmem() to be invoked. This could cause memory.stat.sock > > to exceed memory.current. Reversing the order of querying doesn't address > > the problem either, as memory may be reclaimed between the two calls. > > But just curious, does it fix the flakiness (assuming there is no memory > pressure)? Yes, it does fix the flakiness. I saw it fail once or twice in my runs, but to your point that was only in the presence of memory pressure, which could make many of the tests in the file fail. Let me know if you'd prefer to put the check back in, and instead reverse the order of querying memory.current and memory.stat.sock. > > > Instead, this patch just removes that assertion altogether, and instead > > relies on the values_close() check that follows to validate the expected > > accounting. > > Acked-by: Roman Gushchin <roman.gushchin@linux.dev> > Thanks!
diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c index d88e0ca3f3d1..c4735fa36a3d 100644 --- a/tools/testing/selftests/cgroup/test_memcontrol.c +++ b/tools/testing/selftests/cgroup/test_memcontrol.c @@ -993,9 +993,6 @@ static int tcp_client(const char *cgroup, unsigned short port) if (current < 0 || sock < 0) goto close_sk; - if (current < sock) - goto close_sk; - if (values_close(current, sock, 10)) { ret = KSFT_PASS; break;
test_memcg_sock() in the cgroup memcg tests, verifies expected memory accounting for sockets. The test forks a process which functions as a TCP server, and sends large buffers back and forth between itself (as the TCP client) and the forked TCP server. While doing so, it verifies that memory.current and memory.stat.sock look correct. There is currently a check in tcp_client() which asserts memory.current >= memory.stat.sock. This check is racy, as between memory.current and memory.stat.sock being queried, a packet could come in which causes mem_cgroup_charge_skmem() to be invoked. This could cause memory.stat.sock to exceed memory.current. Reversing the order of querying doesn't address the problem either, as memory may be reclaimed between the two calls. Instead, this patch just removes that assertion altogether, and instead relies on the values_close() check that follows to validate the expected accounting. Signed-off-by: David Vernet <void@manifault.com> --- tools/testing/selftests/cgroup/test_memcontrol.c | 3 --- 1 file changed, 3 deletions(-)