| Message ID | 20220422155728.3055914-3-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:26AM -0700, David Vernet wrote: > The test_memcg_low() testcase in test_memcontrol.c verifies the expected > behavior of groups using the memory.low knob. Part of the testcase verifies > that a group with memory.low that experiences reclaim due to memory > pressure elsewhere in the system, observes memory.events.low events as a > result of that reclaim. > > In commit 8a931f801340 ("mm: memcontrol: recursive memory.low protection"), > the memory controller was updated to propagate memory.low and memory.min > protection from a parent group to its children via a configurable > memory_recursiveprot mount option. This unfortunately broke the memcg > tests, which asserts that a sibling that experienced reclaim but had a > memory.low value of 0, would not observe any memory.low events. This patch > updates test_memcg_low() to account for the new behavior introduced by > memory_recursiveprot. > > So as to make the test resilient to multiple configurations, the patch also > adds a new proc_mount_contains() helper that checks for a string in > /proc/mounts, and is used to toggle behavior based on whether the default > memory_recursiveprot was present. > > Signed-off-by: David Vernet <void@manifault.com> > --- > tools/testing/selftests/cgroup/cgroup_util.c | 12 ++++++++++++ > tools/testing/selftests/cgroup/cgroup_util.h | 1 + > tools/testing/selftests/cgroup/test_memcontrol.c | 16 +++++++++++++--- > 3 files changed, 26 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c > index dbaa7aabbb4a..e5d8d727bdcf 100644 > --- a/tools/testing/selftests/cgroup/cgroup_util.c > +++ b/tools/testing/selftests/cgroup/cgroup_util.c > @@ -535,6 +535,18 @@ int set_oom_adj_score(int pid, int score) > return 0; > } > > +int proc_mount_contains(const char *option) > +{ > + char buf[4 * PAGE_SIZE]; > + ssize_t read; > + > + read = read_text("/proc/mounts", buf, sizeof(buf)); > + if (read < 0) > + return read; > + > + return strstr(buf, option) != NULL; > +} > + > ssize_t proc_read_text(int pid, bool thread, const char *item, char *buf, size_t size) > { > char path[PATH_MAX]; > diff --git a/tools/testing/selftests/cgroup/cgroup_util.h b/tools/testing/selftests/cgroup/cgroup_util.h > index 628738532ac9..756f76052b44 100644 > --- a/tools/testing/selftests/cgroup/cgroup_util.h > +++ b/tools/testing/selftests/cgroup/cgroup_util.h > @@ -48,6 +48,7 @@ extern int is_swap_enabled(void); > extern int set_oom_adj_score(int pid, int score); > extern int cg_wait_for_proc_count(const char *cgroup, int count); > extern int cg_killall(const char *cgroup); > +int proc_mount_contains(const char *option); > extern ssize_t proc_read_text(int pid, bool thread, const char *item, char *buf, size_t size); > extern int proc_read_strstr(int pid, bool thread, const char *item, const char *needle); > extern pid_t clone_into_cgroup(int cgroup_fd); > diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c > index aa50eaa8b157..ea2fd27e52df 100644 > --- a/tools/testing/selftests/cgroup/test_memcontrol.c > +++ b/tools/testing/selftests/cgroup/test_memcontrol.c > @@ -21,6 +21,8 @@ > #include "../kselftest.h" > #include "cgroup_util.h" > > +static bool has_recursiveprot; > + > /* > * This test creates two nested cgroups with and without enabling > * the memory controller. > @@ -521,15 +523,18 @@ static int test_memcg_low(const char *root) > } > > for (i = 0; i < ARRAY_SIZE(children); i++) { > + int no_low_events_index = has_recursiveprot ? 2 : 1; > + > oom = cg_read_key_long(children[i], "memory.events", "oom "); > low = cg_read_key_long(children[i], "memory.events", "low "); > > if (oom) > goto cleanup; > - if (i < 2 && low <= 0) > + if (i <= no_low_events_index && low <= 0) > goto cleanup; > - if (i >= 2 && low) > + if (i > no_low_events_index && low) > goto cleanup; > + > } > > ret = KSFT_PASS; > @@ -1272,7 +1277,7 @@ struct memcg_test { > int main(int argc, char **argv) > { > char root[PATH_MAX]; > - int i, ret = EXIT_SUCCESS; > + int i, proc_status, ret = EXIT_SUCCESS; > > if (cg_find_unified_root(root, sizeof(root))) > ksft_exit_skip("cgroup v2 isn't mounted\n"); > @@ -1288,6 +1293,11 @@ int main(int argc, char **argv) > if (cg_write(root, "cgroup.subtree_control", "+memory")) > ksft_exit_skip("Failed to set memory controller\n"); > > + proc_status = proc_mount_contains("memory_recursiveprot"); > + if (proc_status < 0) > + ksft_exit_skip("Failed to query cgroup mount option\n"); Hopefully no one has a mountpoint with the memory_recursiveprot name :) Acked-by: Roman Gushchin <roman.gushchin@linux.dev> Thanks!
On Fri, Apr 22, 2022 at 04:06:35PM -0700, Roman Gushchin wrote:
> Hopefully no one has a mountpoint with the memory_recursiveprot name :)
Heh, good point. I considered adding another root-level cgroup.features
file to match the features specified in /sys/kernel/cgroup/features, but
ultimately decided it wasn't warranted given that it just duplicates the
information in /proc/mounts.
diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c index dbaa7aabbb4a..e5d8d727bdcf 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.c +++ b/tools/testing/selftests/cgroup/cgroup_util.c @@ -535,6 +535,18 @@ int set_oom_adj_score(int pid, int score) return 0; } +int proc_mount_contains(const char *option) +{ + char buf[4 * PAGE_SIZE]; + ssize_t read; + + read = read_text("/proc/mounts", buf, sizeof(buf)); + if (read < 0) + return read; + + return strstr(buf, option) != NULL; +} + ssize_t proc_read_text(int pid, bool thread, const char *item, char *buf, size_t size) { char path[PATH_MAX]; diff --git a/tools/testing/selftests/cgroup/cgroup_util.h b/tools/testing/selftests/cgroup/cgroup_util.h index 628738532ac9..756f76052b44 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.h +++ b/tools/testing/selftests/cgroup/cgroup_util.h @@ -48,6 +48,7 @@ extern int is_swap_enabled(void); extern int set_oom_adj_score(int pid, int score); extern int cg_wait_for_proc_count(const char *cgroup, int count); extern int cg_killall(const char *cgroup); +int proc_mount_contains(const char *option); extern ssize_t proc_read_text(int pid, bool thread, const char *item, char *buf, size_t size); extern int proc_read_strstr(int pid, bool thread, const char *item, const char *needle); extern pid_t clone_into_cgroup(int cgroup_fd); diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c index aa50eaa8b157..ea2fd27e52df 100644 --- a/tools/testing/selftests/cgroup/test_memcontrol.c +++ b/tools/testing/selftests/cgroup/test_memcontrol.c @@ -21,6 +21,8 @@ #include "../kselftest.h" #include "cgroup_util.h" +static bool has_recursiveprot; + /* * This test creates two nested cgroups with and without enabling * the memory controller. @@ -521,15 +523,18 @@ static int test_memcg_low(const char *root) } for (i = 0; i < ARRAY_SIZE(children); i++) { + int no_low_events_index = has_recursiveprot ? 2 : 1; + oom = cg_read_key_long(children[i], "memory.events", "oom "); low = cg_read_key_long(children[i], "memory.events", "low "); if (oom) goto cleanup; - if (i < 2 && low <= 0) + if (i <= no_low_events_index && low <= 0) goto cleanup; - if (i >= 2 && low) + if (i > no_low_events_index && low) goto cleanup; + } ret = KSFT_PASS; @@ -1272,7 +1277,7 @@ struct memcg_test { int main(int argc, char **argv) { char root[PATH_MAX]; - int i, ret = EXIT_SUCCESS; + int i, proc_status, ret = EXIT_SUCCESS; if (cg_find_unified_root(root, sizeof(root))) ksft_exit_skip("cgroup v2 isn't mounted\n"); @@ -1288,6 +1293,11 @@ int main(int argc, char **argv) if (cg_write(root, "cgroup.subtree_control", "+memory")) ksft_exit_skip("Failed to set memory controller\n"); + proc_status = proc_mount_contains("memory_recursiveprot"); + if (proc_status < 0) + ksft_exit_skip("Failed to query cgroup mount option\n"); + has_recursiveprot = proc_status; + for (i = 0; i < ARRAY_SIZE(tests); i++) { switch (tests[i].fn(root)) { case KSFT_PASS:
The test_memcg_low() testcase in test_memcontrol.c verifies the expected behavior of groups using the memory.low knob. Part of the testcase verifies that a group with memory.low that experiences reclaim due to memory pressure elsewhere in the system, observes memory.events.low events as a result of that reclaim. In commit 8a931f801340 ("mm: memcontrol: recursive memory.low protection"), the memory controller was updated to propagate memory.low and memory.min protection from a parent group to its children via a configurable memory_recursiveprot mount option. This unfortunately broke the memcg tests, which asserts that a sibling that experienced reclaim but had a memory.low value of 0, would not observe any memory.low events. This patch updates test_memcg_low() to account for the new behavior introduced by memory_recursiveprot. So as to make the test resilient to multiple configurations, the patch also adds a new proc_mount_contains() helper that checks for a string in /proc/mounts, and is used to toggle behavior based on whether the default memory_recursiveprot was present. Signed-off-by: David Vernet <void@manifault.com> --- tools/testing/selftests/cgroup/cgroup_util.c | 12 ++++++++++++ tools/testing/selftests/cgroup/cgroup_util.h | 1 + tools/testing/selftests/cgroup/test_memcontrol.c | 16 +++++++++++++--- 3 files changed, 26 insertions(+), 3 deletions(-)