* kselftests for memory.oom.group
@ 2018-09-05 1:08 jgkamat
2018-09-05 1:08 ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: jgkamat @ 2018-09-05 1:08 UTC (permalink / raw)
To: shuah
Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team,
linux-kernel, jaygkamat, jgkamat
Hi Shuah,
I wrote some tests for the new memory.oom.group feature in cgroups 2.
In the process, I discovered a few small bugs in the cgroups tests, which
I have fixed as well in a separate commit.
This is my first ever patch to Linux, so let me know if you see any issues or
improvements that can be made.
Thanks for all your amazing work on Linux!
-Jay
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 1/2] Fix cg_read_strcmp() 2018-09-05 1:08 kselftests for memory.oom.group jgkamat @ 2018-09-05 1:08 ` jgkamat 2018-09-05 14:43 ` Shuah Khan 2018-09-05 1:08 ` [PATCH 2/2] Add tests for memory.oom.group jgkamat 2018-09-07 16:49 ` kselftests " jgkamat 2 siblings, 1 reply; 17+ messages in thread From: jgkamat @ 2018-09-05 1:08 UTC (permalink / raw) To: shuah Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, jgkamat From: Jay Kamat <jgkamat@fb.com> Fix a couple issues with cg_read_strcmp(), to improve correctness of cgroup tests - Fix cg_read_strcmp() always returning 0 for empty "needle" strings - Fix a memory leak in cg_read_strcmp() Fixes: 84092dbcf901 ("selftests: cgroup: add memory controller self-tests") Signed-off-by: Jay Kamat <jgkamat@fb.com> --- tools/testing/selftests/cgroup/cgroup_util.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c index 1e9e3c470561..4aadf38bcd5d 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.c +++ b/tools/testing/selftests/cgroup/cgroup_util.c @@ -91,15 +91,24 @@ int cg_read_strcmp(const char *cgroup, const char *control, { size_t size = strlen(expected) + 1; char *buf; + int ret; + + /* Handle the case of comparing against empty string */ + if (size == 1) + size = 32; buf = malloc(size); if (!buf) return -1; - if (cg_read(cgroup, control, buf, size)) + if (cg_read(cgroup, control, buf, size)) { + free(buf); return -1; + } - return strcmp(expected, buf); + ret = strcmp(expected, buf); + free(buf); + return ret; } int cg_read_strstr(const char *cgroup, const char *control, const char *needle) -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] Fix cg_read_strcmp() 2018-09-05 1:08 ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat @ 2018-09-05 14:43 ` Shuah Khan 0 siblings, 0 replies; 17+ messages in thread From: Shuah Khan @ 2018-09-05 14:43 UTC (permalink / raw) To: jgkamat Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, Shuah Khan Hi Jay, Thanks for the patch. Couple of comments below. On 09/04/2018 07:08 PM, jgkamat@fb.com wrote: > From: Jay Kamat <jgkamat@fb.com> > > Fix a couple issues with cg_read_strcmp(), to improve correctness of > cgroup tests > - Fix cg_read_strcmp() always returning 0 for empty "needle" strings > - Fix a memory leak in cg_read_strcmp() > > Fixes: 84092dbcf901 ("selftests: cgroup: add memory controller self-tests") > > Signed-off-by: Jay Kamat <jgkamat@fb.com> > --- > tools/testing/selftests/cgroup/cgroup_util.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c > index 1e9e3c470561..4aadf38bcd5d 100644 > --- a/tools/testing/selftests/cgroup/cgroup_util.c > +++ b/tools/testing/selftests/cgroup/cgroup_util.c > @@ -91,15 +91,24 @@ int cg_read_strcmp(const char *cgroup, const char *control, > { > size_t size = strlen(expected) + 1; > char *buf; > + int ret; > + > + /* Handle the case of comparing against empty string */ > + if (size == 1) > + size = 32; Why not test for !expected and avoid strlen(expected) all together? > > buf = malloc(size); > if (!buf) > return -1; > > - if (cg_read(cgroup, control, buf, size)) > + if (cg_read(cgroup, control, buf, size)) { > + free(buf); > return -1; > + } > > - return strcmp(expected, buf); > + ret = strcmp(expected, buf); > + free(buf); > + return ret; > } > > int cg_read_strstr(const char *cgroup, const char *control, const char *needle) > thanks, -- Shuah ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] Add tests for memory.oom.group 2018-09-05 1:08 kselftests for memory.oom.group jgkamat 2018-09-05 1:08 ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat @ 2018-09-05 1:08 ` jgkamat 2018-09-05 15:21 ` Shuah Khan 2018-09-07 16:49 ` kselftests " jgkamat 2 siblings, 1 reply; 17+ messages in thread From: jgkamat @ 2018-09-05 1:08 UTC (permalink / raw) To: shuah Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, jgkamat From: Jay Kamat <jgkamat@fb.com> Add tests for memory.oom.group for the following cases: - Killing all processes in a leaf cgroup, but leaving the parent untouched - Killing all processes in a parent and leaf cgroup - Keeping processes marked by OOM_SCORE_ADJ_MIN alive when considered for being killed by the group oom killer. Signed-off-by: Jay Kamat <jgkamat@fb.com> --- tools/testing/selftests/cgroup/cgroup_util.c | 21 ++ tools/testing/selftests/cgroup/cgroup_util.h | 1 + .../selftests/cgroup/test_memcontrol.c | 205 ++++++++++++++++++ 3 files changed, 227 insertions(+) diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c index 4aadf38bcd5d..6799c69d7f03 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.c +++ b/tools/testing/selftests/cgroup/cgroup_util.c @@ -338,3 +338,24 @@ int is_swap_enabled(void) return cnt > 1; } + +int set_oom_adj_score(int pid, int score) +{ + char path[PATH_MAX]; + int fd, len; + + sprintf(path, "/proc/%d/oom_score_adj", pid); + + fd = open(path, O_WRONLY | O_APPEND); + if (fd < 0) + return fd; + + len = dprintf(fd, "%d", score); + if (len < 0) { + close(fd); + return len; + } + + close(fd); + return 0; +} diff --git a/tools/testing/selftests/cgroup/cgroup_util.h b/tools/testing/selftests/cgroup/cgroup_util.h index fe82a297d4e0..cabd43fd137a 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.h +++ b/tools/testing/selftests/cgroup/cgroup_util.h @@ -39,3 +39,4 @@ extern int get_temp_fd(void); extern int alloc_pagecache(int fd, size_t size); extern int alloc_anon(const char *cgroup, void *arg); extern int is_swap_enabled(void); +extern int set_oom_adj_score(int pid, int score); diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c index cf0bddc9d271..017c15a7a935 100644 --- a/tools/testing/selftests/cgroup/test_memcontrol.c +++ b/tools/testing/selftests/cgroup/test_memcontrol.c @@ -2,6 +2,7 @@ #define _GNU_SOURCE #include <linux/limits.h> +#include <linux/oom.h> #include <fcntl.h> #include <stdio.h> #include <stdlib.h> @@ -202,6 +203,36 @@ static int alloc_pagecache_50M_noexit(const char *cgroup, void *arg) return 0; } +static int alloc_anon_noexit(const char *cgroup, void *arg) +{ + int ppid = getppid(); + + if (alloc_anon(cgroup, arg)) + return -1; + + while (getppid() == ppid) + sleep(1); + + return 0; +} + +/* + * Wait until processes are killed asynchronously by the OOM killer + * If we exceed a timeout, fail. + */ +static int cg_test_proc_killed(const char *cgroup) +{ + int limit; + + for (limit = 10; limit > 0; limit--) { + if (cg_read_strcmp(cgroup, "cgroup.procs", "") == 0) + return 0; + + usleep(100000); + } + return -1; +} + /* * First, this test creates the following hierarchy: * A memory.min = 50M, memory.max = 200M @@ -964,6 +995,177 @@ static int test_memcg_sock(const char *root) return ret; } +/* + * This test disables swapping and tries to allocate anonymous memory + * up to OOM with memory.group.oom set. Then it checks that all + * processes in the leaf (but not the parent) were killed. + */ +static int test_memcg_oom_group_leaf_events(const char *root) +{ + int ret = KSFT_FAIL; + char *parent, *child; + + parent = cg_name(root, "memcg_test_0"); + child = cg_name(root, "memcg_test_0/memcg_test_1"); + + if (!parent || !child) + goto cleanup; + + if (cg_create(parent)) + goto cleanup; + + if (cg_create(child)) + goto cleanup; + + if (cg_write(parent, "cgroup.subtree_control", "+memory")) + goto cleanup; + + if (cg_write(child, "memory.max", "50M")) + goto cleanup; + + if (cg_write(child, "memory.swap.max", "0")) + goto cleanup; + + if (cg_write(child, "memory.oom.group", "1")) + goto cleanup; + + cg_run_nowait(parent, alloc_anon_noexit, (void *) MB(60)); + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); + if (!cg_run(child, alloc_anon, (void *)MB(100))) + goto cleanup; + + if (cg_test_proc_killed(child)) + goto cleanup; + + if (cg_read_key_long(child, "memory.events", "oom_kill ") <= 0) + goto cleanup; + + if (cg_read_key_long(parent, "memory.events", "oom_kill ") != 0) + goto cleanup; + + ret = KSFT_PASS; + +cleanup: + if (child) + cg_destroy(child); + if (parent) + cg_destroy(parent); + free(child); + free(parent); + + return ret; +} + +/* + * This test disables swapping and tries to allocate anonymous memory + * up to OOM with memory.group.oom set. Then it checks that all + * processes in the parent and leaf were killed. + */ +static int test_memcg_oom_group_parent_events(const char *root) +{ + int ret = KSFT_FAIL; + char *parent, *child; + + parent = cg_name(root, "memcg_test_0"); + child = cg_name(root, "memcg_test_0/memcg_test_1"); + + if (!parent || !child) + goto cleanup; + + if (cg_create(parent)) + goto cleanup; + + if (cg_create(child)) + goto cleanup; + + if (cg_write(parent, "memory.max", "80M")) + goto cleanup; + + if (cg_write(parent, "memory.swap.max", "0")) + goto cleanup; + + if (cg_write(parent, "memory.oom.group", "1")) + goto cleanup; + + cg_run_nowait(parent, alloc_anon_noexit, (void *) MB(60)); + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); + + if (!cg_run(child, alloc_anon, (void *)MB(100))) + goto cleanup; + + if (cg_test_proc_killed(child)) + goto cleanup; + if (cg_test_proc_killed(parent)) + goto cleanup; + + ret = KSFT_PASS; + +cleanup: + if (child) + cg_destroy(child); + if (parent) + cg_destroy(parent); + free(child); + free(parent); + + return ret; +} + +/* + * This test disables swapping and tries to allocate anonymous memory + * up to OOM with memory.group.oom set. Then it checks that all + * processes were killed except those set with OOM_SCORE_ADJ_MIN + */ +static int test_memcg_oom_group_score_events(const char *root) +{ + int ret = KSFT_FAIL; + char *memcg; + int safe_pid; + + memcg = cg_name(root, "memcg_test_0"); + + if (!memcg) + goto cleanup; + + if (cg_create(memcg)) + goto cleanup; + + if (cg_write(memcg, "memory.max", "50M")) + goto cleanup; + + if (cg_write(memcg, "memory.swap.max", "0")) + goto cleanup; + + if (cg_write(memcg, "memory.oom.group", "1")) + goto cleanup; + + safe_pid = cg_run_nowait(memcg, alloc_anon_noexit, (void *) MB(1)); + if (set_oom_adj_score(safe_pid, OOM_SCORE_ADJ_MIN)) + goto cleanup; + + cg_run_nowait(memcg, alloc_anon_noexit, (void *) MB(1)); + if (!cg_run(memcg, alloc_anon, (void *)MB(100))) + goto cleanup; + + if (cg_read_key_long(memcg, "memory.events", "oom_kill ") != 3) + goto cleanup; + + if (kill(safe_pid, SIGKILL)) + return -1; + + ret = KSFT_PASS; + +cleanup: + if (memcg) + cg_destroy(memcg); + free(memcg); + + return ret; +} + + #define T(x) { x, #x } struct memcg_test { int (*fn)(const char *root); @@ -978,6 +1180,9 @@ struct memcg_test { T(test_memcg_oom_events), T(test_memcg_swap_max), T(test_memcg_sock), + T(test_memcg_oom_group_leaf_events), + T(test_memcg_oom_group_parent_events), + T(test_memcg_oom_group_score_events), }; #undef T -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] Add tests for memory.oom.group 2018-09-05 1:08 ` [PATCH 2/2] Add tests for memory.oom.group jgkamat @ 2018-09-05 15:21 ` Shuah Khan 0 siblings, 0 replies; 17+ messages in thread From: Shuah Khan @ 2018-09-05 15:21 UTC (permalink / raw) To: jgkamat Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, Shuah Khan Hi Jay, Thanks for the patch. Comments below: On 09/04/2018 07:08 PM, jgkamat@fb.com wrote: > From: Jay Kamat <jgkamat@fb.com> > > Add tests for memory.oom.group for the following cases: > - Killing all processes in a leaf cgroup, but leaving the > parent untouched > - Killing all processes in a parent and leaf cgroup > - Keeping processes marked by OOM_SCORE_ADJ_MIN alive when considered > for being killed by the group oom killer. > > Signed-off-by: Jay Kamat <jgkamat@fb.com> > --- > tools/testing/selftests/cgroup/cgroup_util.c | 21 ++ > tools/testing/selftests/cgroup/cgroup_util.h | 1 + > .../selftests/cgroup/test_memcontrol.c | 205 ++++++++++++++++++ > 3 files changed, 227 insertions(+) > > diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c > index 4aadf38bcd5d..6799c69d7f03 100644 > --- a/tools/testing/selftests/cgroup/cgroup_util.c > +++ b/tools/testing/selftests/cgroup/cgroup_util.c > @@ -338,3 +338,24 @@ int is_swap_enabled(void) > > return cnt > 1; > } > + > +int set_oom_adj_score(int pid, int score) > +{ > + char path[PATH_MAX]; > + int fd, len; > + > + sprintf(path, "/proc/%d/oom_score_adj", pid); > + > + fd = open(path, O_WRONLY | O_APPEND); > + if (fd < 0) > + return fd; > + > + len = dprintf(fd, "%d", score); > + if (len < 0) { > + close(fd); > + return len; > + } > + > + close(fd); > + return 0; > +} > diff --git a/tools/testing/selftests/cgroup/cgroup_util.h b/tools/testing/selftests/cgroup/cgroup_util.h > index fe82a297d4e0..cabd43fd137a 100644 > --- a/tools/testing/selftests/cgroup/cgroup_util.h > +++ b/tools/testing/selftests/cgroup/cgroup_util.h > @@ -39,3 +39,4 @@ extern int get_temp_fd(void); > extern int alloc_pagecache(int fd, size_t size); > extern int alloc_anon(const char *cgroup, void *arg); > extern int is_swap_enabled(void); > +extern int set_oom_adj_score(int pid, int score); > diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c > index cf0bddc9d271..017c15a7a935 100644 > --- a/tools/testing/selftests/cgroup/test_memcontrol.c > +++ b/tools/testing/selftests/cgroup/test_memcontrol.c > @@ -2,6 +2,7 @@ > #define _GNU_SOURCE > > #include <linux/limits.h> > +#include <linux/oom.h> > #include <fcntl.h> > #include <stdio.h> > #include <stdlib.h> > @@ -202,6 +203,36 @@ static int alloc_pagecache_50M_noexit(const char *cgroup, void *arg) > return 0; > } > > +static int alloc_anon_noexit(const char *cgroup, void *arg) > +{ > + int ppid = getppid(); > + > + if (alloc_anon(cgroup, arg)) > + return -1; > + > + while (getppid() == ppid) > + sleep(1); > + > + return 0; > +} > + > +/* > + * Wait until processes are killed asynchronously by the OOM killer > + * If we exceed a timeout, fail. > + */ > +static int cg_test_proc_killed(const char *cgroup) > +{ > + int limit; > + > + for (limit = 10; limit > 0; limit--) { > + if (cg_read_strcmp(cgroup, "cgroup.procs", "") == 0) > + return 0; > + > + usleep(100000); > + } > + return -1; > +} > + > /* > * First, this test creates the following hierarchy: > * A memory.min = 50M, memory.max = 200M > @@ -964,6 +995,177 @@ static int test_memcg_sock(const char *root) > return ret; > } > > +/* > + * This test disables swapping and tries to allocate anonymous memory > + * up to OOM with memory.group.oom set. Then it checks that all > + * processes in the leaf (but not the parent) were killed. > + */ > +static int test_memcg_oom_group_leaf_events(const char *root) > +{ > + int ret = KSFT_FAIL; > + char *parent, *child; > + > + parent = cg_name(root, "memcg_test_0"); > + child = cg_name(root, "memcg_test_0/memcg_test_1"); > + > + if (!parent || !child) > + goto cleanup; > + > + if (cg_create(parent)) > + goto cleanup; > + > + if (cg_create(child)) > + goto cleanup; > + > + if (cg_write(parent, "cgroup.subtree_control", "+memory")) > + goto cleanup; > + > + if (cg_write(child, "memory.max", "50M")) > + goto cleanup; > + > + if (cg_write(child, "memory.swap.max", "0")) > + goto cleanup; > + > + if (cg_write(child, "memory.oom.group", "1")) > + goto cleanup; > + > + cg_run_nowait(parent, alloc_anon_noexit, (void *) MB(60)); > + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); > + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); > + if (!cg_run(child, alloc_anon, (void *)MB(100))) > + goto cleanup; > + > + if (cg_test_proc_killed(child)) > + goto cleanup; > + > + if (cg_read_key_long(child, "memory.events", "oom_kill ") <= 0) > + goto cleanup; > + > + if (cg_read_key_long(parent, "memory.events", "oom_kill ") != 0) > + goto cleanup; > + > + ret = KSFT_PASS; > + > +cleanup: > + if (child) > + cg_destroy(child); > + if (parent) > + cg_destroy(parent); > + free(child); > + free(parent); > + > + return ret; > +} > + > +/* > + * This test disables swapping and tries to allocate anonymous memory > + * up to OOM with memory.group.oom set. Then it checks that all > + * processes in the parent and leaf were killed. > + */ > +static int test_memcg_oom_group_parent_events(const char *root) > +{ > + int ret = KSFT_FAIL; > + char *parent, *child; > + > + parent = cg_name(root, "memcg_test_0"); > + child = cg_name(root, "memcg_test_0/memcg_test_1"); > + > + if (!parent || !child) > + goto cleanup; > + > + if (cg_create(parent)) > + goto cleanup; > + > + if (cg_create(child)) > + goto cleanup; > + > + if (cg_write(parent, "memory.max", "80M")) > + goto cleanup; > + > + if (cg_write(parent, "memory.swap.max", "0")) > + goto cleanup; > + > + if (cg_write(parent, "memory.oom.group", "1")) > + goto cleanup; > + > + cg_run_nowait(parent, alloc_anon_noexit, (void *) MB(60)); > + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); > + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); > + > + if (!cg_run(child, alloc_anon, (void *)MB(100))) > + goto cleanup; > + > + if (cg_test_proc_killed(child)) > + goto cleanup; > + if (cg_test_proc_killed(parent)) > + goto cleanup; > + > + ret = KSFT_PASS; > + > +cleanup: > + if (child) > + cg_destroy(child); > + if (parent) > + cg_destroy(parent); > + free(child); > + free(parent); > + > + return ret; > +} > + > +/* > + * This test disables swapping and tries to allocate anonymous memory > + * up to OOM with memory.group.oom set. Then it checks that all > + * processes were killed except those set with OOM_SCORE_ADJ_MIN > + */ > +static int test_memcg_oom_group_score_events(const char *root) > +{ > + int ret = KSFT_FAIL; > + char *memcg; > + int safe_pid; > + > + memcg = cg_name(root, "memcg_test_0"); > + > + if (!memcg) > + goto cleanup; > + > + if (cg_create(memcg)) > + goto cleanup; > + > + if (cg_write(memcg, "memory.max", "50M")) > + goto cleanup; > + > + if (cg_write(memcg, "memory.swap.max", "0")) > + goto cleanup; > + > + if (cg_write(memcg, "memory.oom.group", "1")) > + goto cleanup; > + > + safe_pid = cg_run_nowait(memcg, alloc_anon_noexit, (void *) MB(1)); > + if (set_oom_adj_score(safe_pid, OOM_SCORE_ADJ_MIN)) > + goto cleanup; > + > + cg_run_nowait(memcg, alloc_anon_noexit, (void *) MB(1)); > + if (!cg_run(memcg, alloc_anon, (void *)MB(100))) > + goto cleanup; > + > + if (cg_read_key_long(memcg, "memory.events", "oom_kill ") != 3) > + goto cleanup; > + > + if (kill(safe_pid, SIGKILL)) > + return -1; Missing cleanup? Should the return be just ret which is KSFT_FAIL > + > + ret = KSFT_PASS; > + > +cleanup: > + if (memcg) > + cg_destroy(memcg); > + free(memcg); > + > + return ret; > +} > + > + > #define T(x) { x, #x } > struct memcg_test { > int (*fn)(const char *root); > @@ -978,6 +1180,9 @@ struct memcg_test { > T(test_memcg_oom_events), > T(test_memcg_swap_max), > T(test_memcg_sock), > + T(test_memcg_oom_group_leaf_events), > + T(test_memcg_oom_group_parent_events), > + T(test_memcg_oom_group_score_events), > }; > #undef T > > thanks, -- Shuah ^ permalink raw reply [flat|nested] 17+ messages in thread
* kselftests for memory.oom.group 2018-09-05 1:08 kselftests for memory.oom.group jgkamat 2018-09-05 1:08 ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat 2018-09-05 1:08 ` [PATCH 2/2] Add tests for memory.oom.group jgkamat @ 2018-09-07 16:49 ` jgkamat 2018-09-07 16:49 ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat ` (2 more replies) 2 siblings, 3 replies; 17+ messages in thread From: jgkamat @ 2018-09-07 16:49 UTC (permalink / raw) To: shuah Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, jgkamat Here is an amended patch which fixes those issues. Please let me know if you see anything else that could be improved. Changes since the last patchset: - Use if statement to check for empty string in cgroup_util.c - Cleanup and return properly when failing to kill process in test_memcg_oom_group_score_events Thanks, -Jay ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] Fix cg_read_strcmp() 2018-09-07 16:49 ` kselftests " jgkamat @ 2018-09-07 16:49 ` jgkamat 2018-09-07 16:56 ` Roman Gushchin 2018-09-07 17:06 ` Shuah Khan 2018-09-07 16:49 ` [PATCH 2/2] Add tests for memory.oom.group jgkamat 2018-09-07 21:34 ` kselftests " jgkamat 2 siblings, 2 replies; 17+ messages in thread From: jgkamat @ 2018-09-07 16:49 UTC (permalink / raw) To: shuah Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, jgkamat From: Jay Kamat <jgkamat@fb.com> Fix a couple issues with cg_read_strcmp(), to improve correctness of cgroup tests - Fix cg_read_strcmp() always returning 0 for empty "needle" strings - Fix a memory leak in cg_read_strcmp() Fixes: 84092dbcf901 ("selftests: cgroup: add memory controller self-tests") Signed-off-by: Jay Kamat <jgkamat@fb.com> --- tools/testing/selftests/cgroup/cgroup_util.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c index 1e9e3c470561..8b644ea39725 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.c +++ b/tools/testing/selftests/cgroup/cgroup_util.c @@ -89,17 +89,28 @@ int cg_read(const char *cgroup, const char *control, char *buf, size_t len) int cg_read_strcmp(const char *cgroup, const char *control, const char *expected) { - size_t size = strlen(expected) + 1; + size_t size; char *buf; + int ret; + + /* Handle the case of comparing against empty string */ + if (!expected) + size = 32; + else + size = strlen(expected) + 1; buf = malloc(size); if (!buf) return -1; - if (cg_read(cgroup, control, buf, size)) + if (cg_read(cgroup, control, buf, size)) { + free(buf); return -1; + } - return strcmp(expected, buf); + ret = strcmp(expected, buf); + free(buf); + return ret; } int cg_read_strstr(const char *cgroup, const char *control, const char *needle) -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] Fix cg_read_strcmp() 2018-09-07 16:49 ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat @ 2018-09-07 16:56 ` Roman Gushchin 2018-09-07 17:06 ` Shuah Khan 1 sibling, 0 replies; 17+ messages in thread From: Roman Gushchin @ 2018-09-07 16:56 UTC (permalink / raw) To: jgkamat Cc: shuah, linux-kselftest, Tejun Heo, kernel-team, linux-kernel, jaygkamat On Fri, Sep 07, 2018 at 09:49:23AM -0700, jgkamat@fb.com wrote: > From: Jay Kamat <jgkamat@fb.com> > > Fix a couple issues with cg_read_strcmp(), to improve correctness of > cgroup tests > - Fix cg_read_strcmp() always returning 0 for empty "needle" strings > - Fix a memory leak in cg_read_strcmp() > > Fixes: 84092dbcf901 ("selftests: cgroup: add memory controller self-tests") Hi Jay! Thank you for working on this! Acked-by: Roman Gushchin <guro@fb.com> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] Fix cg_read_strcmp() 2018-09-07 16:49 ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat 2018-09-07 16:56 ` Roman Gushchin @ 2018-09-07 17:06 ` Shuah Khan 2018-09-07 18:28 ` Jay Kamat 1 sibling, 1 reply; 17+ messages in thread From: Shuah Khan @ 2018-09-07 17:06 UTC (permalink / raw) To: jgkamat Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, Shuah Khan On 09/07/2018 10:49 AM, jgkamat@fb.com wrote: > From: Jay Kamat <jgkamat@fb.com> > > Fix a couple issues with cg_read_strcmp(), to improve correctness of > cgroup tests > - Fix cg_read_strcmp() always returning 0 for empty "needle" strings > - Fix a memory leak in cg_read_strcmp() > > Fixes: 84092dbcf901 ("selftests: cgroup: add memory controller self-tests") > > Signed-off-by: Jay Kamat <jgkamat@fb.com> > --- > tools/testing/selftests/cgroup/cgroup_util.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c > index 1e9e3c470561..8b644ea39725 100644 > --- a/tools/testing/selftests/cgroup/cgroup_util.c > +++ b/tools/testing/selftests/cgroup/cgroup_util.c > @@ -89,17 +89,28 @@ int cg_read(const char *cgroup, const char *control, char *buf, size_t len) > int cg_read_strcmp(const char *cgroup, const char *control, > const char *expected) > { > - size_t size = strlen(expected) + 1; > + size_t size; > char *buf; > + int ret; > + > + /* Handle the case of comparing against empty string */ > + if (!expected) > + size = 32; This doesn't look right. I would think expected shouldn't be null? It gets used below. > + else > + size = strlen(expected) + 1; > > buf = malloc(size); > if (!buf) > return -1; > > - if (cg_read(cgroup, control, buf, size)) > + if (cg_read(cgroup, control, buf, size)) { > + free(buf); > return -1; > + } > > - return strcmp(expected, buf); > + ret = strcmp(expected, buf); If expected is null, what's the point in running the test? Is empty "needle" string a valid test scenario? > + free(buf); > + return ret; > } > > int cg_read_strstr(const char *cgroup, const char *control, const char *needle) > thanks, -- Shuah ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] Fix cg_read_strcmp() 2018-09-07 17:06 ` Shuah Khan @ 2018-09-07 18:28 ` Jay Kamat 2018-09-07 18:53 ` Shuah Khan 0 siblings, 1 reply; 17+ messages in thread From: Jay Kamat @ 2018-09-07 18:28 UTC (permalink / raw) To: Shuah Khan Cc: jgkamat, linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat Shuah Khan writes: > On 09/07/2018 10:49 AM, jgkamat@fb.com wrote: >> From: Jay Kamat <jgkamat@fb.com> >> >> Fix a couple issues with cg_read_strcmp(), to improve correctness of >> cgroup tests >> - Fix cg_read_strcmp() always returning 0 for empty "needle" strings >> - Fix a memory leak in cg_read_strcmp() >> >> Fixes: 84092dbcf901 ("selftests: cgroup: add memory controller self-tests") >> >> Signed-off-by: Jay Kamat <jgkamat@fb.com> >> --- >> tools/testing/selftests/cgroup/cgroup_util.c | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c >> index 1e9e3c470561..8b644ea39725 100644 >> --- a/tools/testing/selftests/cgroup/cgroup_util.c >> +++ b/tools/testing/selftests/cgroup/cgroup_util.c >> @@ -89,17 +89,28 @@ int cg_read(const char *cgroup, const char *control, char *buf, size_t len) >> int cg_read_strcmp(const char *cgroup, const char *control, >> const char *expected) >> { >> - size_t size = strlen(expected) + 1; >> + size_t size; >> char *buf; >> + int ret; >> + >> + /* Handle the case of comparing against empty string */ >> + if (!expected) >> + size = 32; > > This doesn't look right. I would think expected shouldn't be null? > It gets used below. > >> + else >> + size = strlen(expected) + 1; >> >> buf = malloc(size); >> if (!buf) >> return -1; >> >> - if (cg_read(cgroup, control, buf, size)) >> + if (cg_read(cgroup, control, buf, size)) { >> + free(buf); >> return -1; >> + } >> >> - return strcmp(expected, buf); >> + ret = strcmp(expected, buf); > > If expected is null, what's the point in running the test? > Is empty "needle" string a valid test scenario? There are a couple places where an empty "needle" string is used currently: - cg_test_proc_killed (newly added in the next patch): Verify cgroup.procs is empty (there are no processes running) - test_memcg_oom_events: Verify cgroup.procs is empty Previously, when passing in an empty needle string, this function would always return 0, as the size allocated (1) would not be enough to read any data in 'cg_read', and strcmp would compare two null strings. > >> + free(buf); >> + return ret; >> } >> >> int cg_read_strstr(const char *cgroup, const char *control, const char *needle) >> > > thanks, > -- Shuah I could definitely remove the unneeded strcmp in the null 'expected' case, but I am worried it would feel a bit too hacky or add too much duplication. Would something like this be the best solution? If you had something else in mind (or if I'm misunderstanding something), please let me know, and I'll update the patchset! size_t size; char *buf; int ret; /* Handle the case of comparing against empty string */ if (!expected) size = 32; else size = strlen(expected) + 1; buf = malloc(size); if (!buf) return -1; if (cg_read(cgroup, control, buf, size)) { free(buf); return -1; } if (!expected) ret = !buf; else ret = strcmp(expected, buf); free(buf); return ret; Thanks, -Jay ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] Fix cg_read_strcmp() 2018-09-07 18:28 ` Jay Kamat @ 2018-09-07 18:53 ` Shuah Khan 0 siblings, 0 replies; 17+ messages in thread From: Shuah Khan @ 2018-09-07 18:53 UTC (permalink / raw) To: Jay Kamat Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, Shuah Khan On 09/07/2018 12:28 PM, Jay Kamat wrote: > > Shuah Khan writes: > >> On 09/07/2018 10:49 AM, jgkamat@fb.com wrote: >>> From: Jay Kamat <jgkamat@fb.com> >>> >>> Fix a couple issues with cg_read_strcmp(), to improve correctness of >>> cgroup tests >>> - Fix cg_read_strcmp() always returning 0 for empty "needle" strings >>> - Fix a memory leak in cg_read_strcmp() >>> >>> Fixes: 84092dbcf901 ("selftests: cgroup: add memory controller self-tests") >>> >>> Signed-off-by: Jay Kamat <jgkamat@fb.com> >>> --- >>> tools/testing/selftests/cgroup/cgroup_util.c | 17 ++++++++++++++--- >>> 1 file changed, 14 insertions(+), 3 deletions(-) >>> >>> diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c >>> index 1e9e3c470561..8b644ea39725 100644 >>> --- a/tools/testing/selftests/cgroup/cgroup_util.c >>> +++ b/tools/testing/selftests/cgroup/cgroup_util.c >>> @@ -89,17 +89,28 @@ int cg_read(const char *cgroup, const char *control, char *buf, size_t len) >>> int cg_read_strcmp(const char *cgroup, const char *control, >>> const char *expected) >>> { >>> - size_t size = strlen(expected) + 1; >>> + size_t size; >>> char *buf; >>> + int ret; >>> + >>> + /* Handle the case of comparing against empty string */ >>> + if (!expected) >>> + size = 32; >> >> This doesn't look right. I would think expected shouldn't be null? >> It gets used below. >> >>> + else >>> + size = strlen(expected) + 1; >>> >>> buf = malloc(size); >>> if (!buf) >>> return -1; >>> >>> - if (cg_read(cgroup, control, buf, size)) >>> + if (cg_read(cgroup, control, buf, size)) { >>> + free(buf); >>> return -1; >>> + } >>> >>> - return strcmp(expected, buf); >>> + ret = strcmp(expected, buf); >> >> If expected is null, what's the point in running the test? >> Is empty "needle" string a valid test scenario? > > There are a couple places where an empty "needle" string is used currently: > > - cg_test_proc_killed (newly added in the next patch): Verify cgroup.procs is > empty (there are no processes running) > - test_memcg_oom_events: Verify cgroup.procs is empty Yes I see the empty neede string usage now. > > Previously, when passing in an empty needle string, this function would always > return 0, as the size allocated (1) would not be enough to read any data in > 'cg_read', and strcmp would compare two null strings. Thanks for explaining this. Yes this fix is good. This would be good information to add to the change log. Could you please add this to the change log and send v2. I will pull these in for the next rc. thanks, -- Shuah ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] Add tests for memory.oom.group 2018-09-07 16:49 ` kselftests " jgkamat 2018-09-07 16:49 ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat @ 2018-09-07 16:49 ` jgkamat 2018-09-07 16:57 ` Roman Gushchin 2018-09-07 21:34 ` kselftests " jgkamat 2 siblings, 1 reply; 17+ messages in thread From: jgkamat @ 2018-09-07 16:49 UTC (permalink / raw) To: shuah Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, jgkamat From: Jay Kamat <jgkamat@fb.com> Add tests for memory.oom.group for the following cases: - Killing all processes in a leaf cgroup, but leaving the parent untouched - Killing all processes in a parent and leaf cgroup - Keeping processes marked by OOM_SCORE_ADJ_MIN alive when considered for being killed by the group oom killer. Signed-off-by: Jay Kamat <jgkamat@fb.com> --- tools/testing/selftests/cgroup/cgroup_util.c | 21 ++ tools/testing/selftests/cgroup/cgroup_util.h | 1 + .../selftests/cgroup/test_memcontrol.c | 205 ++++++++++++++++++ 3 files changed, 227 insertions(+) diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c index 8b644ea39725..e0db048331cb 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.c +++ b/tools/testing/selftests/cgroup/cgroup_util.c @@ -340,3 +340,24 @@ int is_swap_enabled(void) return cnt > 1; } + +int set_oom_adj_score(int pid, int score) +{ + char path[PATH_MAX]; + int fd, len; + + sprintf(path, "/proc/%d/oom_score_adj", pid); + + fd = open(path, O_WRONLY | O_APPEND); + if (fd < 0) + return fd; + + len = dprintf(fd, "%d", score); + if (len < 0) { + close(fd); + return len; + } + + close(fd); + return 0; +} diff --git a/tools/testing/selftests/cgroup/cgroup_util.h b/tools/testing/selftests/cgroup/cgroup_util.h index fe82a297d4e0..cabd43fd137a 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.h +++ b/tools/testing/selftests/cgroup/cgroup_util.h @@ -39,3 +39,4 @@ extern int get_temp_fd(void); extern int alloc_pagecache(int fd, size_t size); extern int alloc_anon(const char *cgroup, void *arg); extern int is_swap_enabled(void); +extern int set_oom_adj_score(int pid, int score); diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c index cf0bddc9d271..28d321ba311b 100644 --- a/tools/testing/selftests/cgroup/test_memcontrol.c +++ b/tools/testing/selftests/cgroup/test_memcontrol.c @@ -2,6 +2,7 @@ #define _GNU_SOURCE #include <linux/limits.h> +#include <linux/oom.h> #include <fcntl.h> #include <stdio.h> #include <stdlib.h> @@ -202,6 +203,36 @@ static int alloc_pagecache_50M_noexit(const char *cgroup, void *arg) return 0; } +static int alloc_anon_noexit(const char *cgroup, void *arg) +{ + int ppid = getppid(); + + if (alloc_anon(cgroup, arg)) + return -1; + + while (getppid() == ppid) + sleep(1); + + return 0; +} + +/* + * Wait until processes are killed asynchronously by the OOM killer + * If we exceed a timeout, fail. + */ +static int cg_test_proc_killed(const char *cgroup) +{ + int limit; + + for (limit = 10; limit > 0; limit--) { + if (cg_read_strcmp(cgroup, "cgroup.procs", "") == 0) + return 0; + + usleep(100000); + } + return -1; +} + /* * First, this test creates the following hierarchy: * A memory.min = 50M, memory.max = 200M @@ -964,6 +995,177 @@ static int test_memcg_sock(const char *root) return ret; } +/* + * This test disables swapping and tries to allocate anonymous memory + * up to OOM with memory.group.oom set. Then it checks that all + * processes in the leaf (but not the parent) were killed. + */ +static int test_memcg_oom_group_leaf_events(const char *root) +{ + int ret = KSFT_FAIL; + char *parent, *child; + + parent = cg_name(root, "memcg_test_0"); + child = cg_name(root, "memcg_test_0/memcg_test_1"); + + if (!parent || !child) + goto cleanup; + + if (cg_create(parent)) + goto cleanup; + + if (cg_create(child)) + goto cleanup; + + if (cg_write(parent, "cgroup.subtree_control", "+memory")) + goto cleanup; + + if (cg_write(child, "memory.max", "50M")) + goto cleanup; + + if (cg_write(child, "memory.swap.max", "0")) + goto cleanup; + + if (cg_write(child, "memory.oom.group", "1")) + goto cleanup; + + cg_run_nowait(parent, alloc_anon_noexit, (void *) MB(60)); + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); + if (!cg_run(child, alloc_anon, (void *)MB(100))) + goto cleanup; + + if (cg_test_proc_killed(child)) + goto cleanup; + + if (cg_read_key_long(child, "memory.events", "oom_kill ") <= 0) + goto cleanup; + + if (cg_read_key_long(parent, "memory.events", "oom_kill ") != 0) + goto cleanup; + + ret = KSFT_PASS; + +cleanup: + if (child) + cg_destroy(child); + if (parent) + cg_destroy(parent); + free(child); + free(parent); + + return ret; +} + +/* + * This test disables swapping and tries to allocate anonymous memory + * up to OOM with memory.group.oom set. Then it checks that all + * processes in the parent and leaf were killed. + */ +static int test_memcg_oom_group_parent_events(const char *root) +{ + int ret = KSFT_FAIL; + char *parent, *child; + + parent = cg_name(root, "memcg_test_0"); + child = cg_name(root, "memcg_test_0/memcg_test_1"); + + if (!parent || !child) + goto cleanup; + + if (cg_create(parent)) + goto cleanup; + + if (cg_create(child)) + goto cleanup; + + if (cg_write(parent, "memory.max", "80M")) + goto cleanup; + + if (cg_write(parent, "memory.swap.max", "0")) + goto cleanup; + + if (cg_write(parent, "memory.oom.group", "1")) + goto cleanup; + + cg_run_nowait(parent, alloc_anon_noexit, (void *) MB(60)); + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); + + if (!cg_run(child, alloc_anon, (void *)MB(100))) + goto cleanup; + + if (cg_test_proc_killed(child)) + goto cleanup; + if (cg_test_proc_killed(parent)) + goto cleanup; + + ret = KSFT_PASS; + +cleanup: + if (child) + cg_destroy(child); + if (parent) + cg_destroy(parent); + free(child); + free(parent); + + return ret; +} + +/* + * This test disables swapping and tries to allocate anonymous memory + * up to OOM with memory.group.oom set. Then it checks that all + * processes were killed except those set with OOM_SCORE_ADJ_MIN + */ +static int test_memcg_oom_group_score_events(const char *root) +{ + int ret = KSFT_FAIL; + char *memcg; + int safe_pid; + + memcg = cg_name(root, "memcg_test_0"); + + if (!memcg) + goto cleanup; + + if (cg_create(memcg)) + goto cleanup; + + if (cg_write(memcg, "memory.max", "50M")) + goto cleanup; + + if (cg_write(memcg, "memory.swap.max", "0")) + goto cleanup; + + if (cg_write(memcg, "memory.oom.group", "1")) + goto cleanup; + + safe_pid = cg_run_nowait(memcg, alloc_anon_noexit, (void *) MB(1)); + if (set_oom_adj_score(safe_pid, OOM_SCORE_ADJ_MIN)) + goto cleanup; + + cg_run_nowait(memcg, alloc_anon_noexit, (void *) MB(1)); + if (!cg_run(memcg, alloc_anon, (void *)MB(100))) + goto cleanup; + + if (cg_read_key_long(memcg, "memory.events", "oom_kill ") != 3) + goto cleanup; + + if (kill(safe_pid, SIGKILL)) + goto cleanup; + + ret = KSFT_PASS; + +cleanup: + if (memcg) + cg_destroy(memcg); + free(memcg); + + return ret; +} + + #define T(x) { x, #x } struct memcg_test { int (*fn)(const char *root); @@ -978,6 +1180,9 @@ struct memcg_test { T(test_memcg_oom_events), T(test_memcg_swap_max), T(test_memcg_sock), + T(test_memcg_oom_group_leaf_events), + T(test_memcg_oom_group_parent_events), + T(test_memcg_oom_group_score_events), }; #undef T -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] Add tests for memory.oom.group 2018-09-07 16:49 ` [PATCH 2/2] Add tests for memory.oom.group jgkamat @ 2018-09-07 16:57 ` Roman Gushchin 0 siblings, 0 replies; 17+ messages in thread From: Roman Gushchin @ 2018-09-07 16:57 UTC (permalink / raw) To: jgkamat Cc: shuah, linux-kselftest, Tejun Heo, kernel-team, linux-kernel, jaygkamat On Fri, Sep 07, 2018 at 09:49:24AM -0700, jgkamat@fb.com wrote: > From: Jay Kamat <jgkamat@fb.com> > > Add tests for memory.oom.group for the following cases: > - Killing all processes in a leaf cgroup, but leaving the > parent untouched > - Killing all processes in a parent and leaf cgroup > - Keeping processes marked by OOM_SCORE_ADJ_MIN alive when considered > for being killed by the group oom killer. > > Signed-off-by: Jay Kamat <jgkamat@fb.com> Acked-by: Roman Gushchin <guro@fb.com> ^ permalink raw reply [flat|nested] 17+ messages in thread
* kselftests for memory.oom.group 2018-09-07 16:49 ` kselftests " jgkamat 2018-09-07 16:49 ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat 2018-09-07 16:49 ` [PATCH 2/2] Add tests for memory.oom.group jgkamat @ 2018-09-07 21:34 ` jgkamat 2018-09-07 21:34 ` [PATCH v3 1/2] Fix cg_read_strcmp() jgkamat ` (2 more replies) 2 siblings, 3 replies; 17+ messages in thread From: jgkamat @ 2018-09-07 21:34 UTC (permalink / raw) To: shuah Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, jgkamat Here is the third version of the patchset. Changes since the last patchset: - Updated commit message of first patch to clarify fixes - Add ack from Roman There should be no code changes since the last patchset. Let me know if any improvements can be made, and thanks for your time! -Jay ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/2] Fix cg_read_strcmp() 2018-09-07 21:34 ` kselftests " jgkamat @ 2018-09-07 21:34 ` jgkamat 2018-09-07 21:34 ` [PATCH v3 2/2] Add tests for memory.oom.group jgkamat 2018-09-07 22:53 ` kselftests " Shuah Khan 2 siblings, 0 replies; 17+ messages in thread From: jgkamat @ 2018-09-07 21:34 UTC (permalink / raw) To: shuah Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, jgkamat From: Jay Kamat <jgkamat@fb.com> Fix a couple issues with cg_read_strcmp(), to improve correctness of cgroup tests - Fix cg_read_strcmp() always returning 0 for empty "needle" strings. Previously, this function read to a size = 1 buffer when comparing against empty strings, which would lead to cg_read_strcmp() comparing two empty strings. - Fix a memory leak in cg_read_strcmp() Fixes: 84092dbcf901 ("selftests: cgroup: add memory controller self-tests") Signed-off-by: Jay Kamat <jgkamat@fb.com> Acked-by: Roman Gushchin <guro@fb.com> --- tools/testing/selftests/cgroup/cgroup_util.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c index 1e9e3c470561..8b644ea39725 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.c +++ b/tools/testing/selftests/cgroup/cgroup_util.c @@ -89,17 +89,28 @@ int cg_read(const char *cgroup, const char *control, char *buf, size_t len) int cg_read_strcmp(const char *cgroup, const char *control, const char *expected) { - size_t size = strlen(expected) + 1; + size_t size; char *buf; + int ret; + + /* Handle the case of comparing against empty string */ + if (!expected) + size = 32; + else + size = strlen(expected) + 1; buf = malloc(size); if (!buf) return -1; - if (cg_read(cgroup, control, buf, size)) + if (cg_read(cgroup, control, buf, size)) { + free(buf); return -1; + } - return strcmp(expected, buf); + ret = strcmp(expected, buf); + free(buf); + return ret; } int cg_read_strstr(const char *cgroup, const char *control, const char *needle) -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 2/2] Add tests for memory.oom.group 2018-09-07 21:34 ` kselftests " jgkamat 2018-09-07 21:34 ` [PATCH v3 1/2] Fix cg_read_strcmp() jgkamat @ 2018-09-07 21:34 ` jgkamat 2018-09-07 22:53 ` kselftests " Shuah Khan 2 siblings, 0 replies; 17+ messages in thread From: jgkamat @ 2018-09-07 21:34 UTC (permalink / raw) To: shuah Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, jgkamat From: Jay Kamat <jgkamat@fb.com> Add tests for memory.oom.group for the following cases: - Killing all processes in a leaf cgroup, but leaving the parent untouched - Killing all processes in a parent and leaf cgroup - Keeping processes marked by OOM_SCORE_ADJ_MIN alive when considered for being killed by the group oom killer. Signed-off-by: Jay Kamat <jgkamat@fb.com> Acked-by: Roman Gushchin <guro@fb.com> --- tools/testing/selftests/cgroup/cgroup_util.c | 21 ++ tools/testing/selftests/cgroup/cgroup_util.h | 1 + .../selftests/cgroup/test_memcontrol.c | 205 ++++++++++++++++++ 3 files changed, 227 insertions(+) diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c index 8b644ea39725..e0db048331cb 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.c +++ b/tools/testing/selftests/cgroup/cgroup_util.c @@ -340,3 +340,24 @@ int is_swap_enabled(void) return cnt > 1; } + +int set_oom_adj_score(int pid, int score) +{ + char path[PATH_MAX]; + int fd, len; + + sprintf(path, "/proc/%d/oom_score_adj", pid); + + fd = open(path, O_WRONLY | O_APPEND); + if (fd < 0) + return fd; + + len = dprintf(fd, "%d", score); + if (len < 0) { + close(fd); + return len; + } + + close(fd); + return 0; +} diff --git a/tools/testing/selftests/cgroup/cgroup_util.h b/tools/testing/selftests/cgroup/cgroup_util.h index fe82a297d4e0..cabd43fd137a 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.h +++ b/tools/testing/selftests/cgroup/cgroup_util.h @@ -39,3 +39,4 @@ extern int get_temp_fd(void); extern int alloc_pagecache(int fd, size_t size); extern int alloc_anon(const char *cgroup, void *arg); extern int is_swap_enabled(void); +extern int set_oom_adj_score(int pid, int score); diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c index cf0bddc9d271..28d321ba311b 100644 --- a/tools/testing/selftests/cgroup/test_memcontrol.c +++ b/tools/testing/selftests/cgroup/test_memcontrol.c @@ -2,6 +2,7 @@ #define _GNU_SOURCE #include <linux/limits.h> +#include <linux/oom.h> #include <fcntl.h> #include <stdio.h> #include <stdlib.h> @@ -202,6 +203,36 @@ static int alloc_pagecache_50M_noexit(const char *cgroup, void *arg) return 0; } +static int alloc_anon_noexit(const char *cgroup, void *arg) +{ + int ppid = getppid(); + + if (alloc_anon(cgroup, arg)) + return -1; + + while (getppid() == ppid) + sleep(1); + + return 0; +} + +/* + * Wait until processes are killed asynchronously by the OOM killer + * If we exceed a timeout, fail. + */ +static int cg_test_proc_killed(const char *cgroup) +{ + int limit; + + for (limit = 10; limit > 0; limit--) { + if (cg_read_strcmp(cgroup, "cgroup.procs", "") == 0) + return 0; + + usleep(100000); + } + return -1; +} + /* * First, this test creates the following hierarchy: * A memory.min = 50M, memory.max = 200M @@ -964,6 +995,177 @@ static int test_memcg_sock(const char *root) return ret; } +/* + * This test disables swapping and tries to allocate anonymous memory + * up to OOM with memory.group.oom set. Then it checks that all + * processes in the leaf (but not the parent) were killed. + */ +static int test_memcg_oom_group_leaf_events(const char *root) +{ + int ret = KSFT_FAIL; + char *parent, *child; + + parent = cg_name(root, "memcg_test_0"); + child = cg_name(root, "memcg_test_0/memcg_test_1"); + + if (!parent || !child) + goto cleanup; + + if (cg_create(parent)) + goto cleanup; + + if (cg_create(child)) + goto cleanup; + + if (cg_write(parent, "cgroup.subtree_control", "+memory")) + goto cleanup; + + if (cg_write(child, "memory.max", "50M")) + goto cleanup; + + if (cg_write(child, "memory.swap.max", "0")) + goto cleanup; + + if (cg_write(child, "memory.oom.group", "1")) + goto cleanup; + + cg_run_nowait(parent, alloc_anon_noexit, (void *) MB(60)); + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); + if (!cg_run(child, alloc_anon, (void *)MB(100))) + goto cleanup; + + if (cg_test_proc_killed(child)) + goto cleanup; + + if (cg_read_key_long(child, "memory.events", "oom_kill ") <= 0) + goto cleanup; + + if (cg_read_key_long(parent, "memory.events", "oom_kill ") != 0) + goto cleanup; + + ret = KSFT_PASS; + +cleanup: + if (child) + cg_destroy(child); + if (parent) + cg_destroy(parent); + free(child); + free(parent); + + return ret; +} + +/* + * This test disables swapping and tries to allocate anonymous memory + * up to OOM with memory.group.oom set. Then it checks that all + * processes in the parent and leaf were killed. + */ +static int test_memcg_oom_group_parent_events(const char *root) +{ + int ret = KSFT_FAIL; + char *parent, *child; + + parent = cg_name(root, "memcg_test_0"); + child = cg_name(root, "memcg_test_0/memcg_test_1"); + + if (!parent || !child) + goto cleanup; + + if (cg_create(parent)) + goto cleanup; + + if (cg_create(child)) + goto cleanup; + + if (cg_write(parent, "memory.max", "80M")) + goto cleanup; + + if (cg_write(parent, "memory.swap.max", "0")) + goto cleanup; + + if (cg_write(parent, "memory.oom.group", "1")) + goto cleanup; + + cg_run_nowait(parent, alloc_anon_noexit, (void *) MB(60)); + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); + cg_run_nowait(child, alloc_anon_noexit, (void *) MB(1)); + + if (!cg_run(child, alloc_anon, (void *)MB(100))) + goto cleanup; + + if (cg_test_proc_killed(child)) + goto cleanup; + if (cg_test_proc_killed(parent)) + goto cleanup; + + ret = KSFT_PASS; + +cleanup: + if (child) + cg_destroy(child); + if (parent) + cg_destroy(parent); + free(child); + free(parent); + + return ret; +} + +/* + * This test disables swapping and tries to allocate anonymous memory + * up to OOM with memory.group.oom set. Then it checks that all + * processes were killed except those set with OOM_SCORE_ADJ_MIN + */ +static int test_memcg_oom_group_score_events(const char *root) +{ + int ret = KSFT_FAIL; + char *memcg; + int safe_pid; + + memcg = cg_name(root, "memcg_test_0"); + + if (!memcg) + goto cleanup; + + if (cg_create(memcg)) + goto cleanup; + + if (cg_write(memcg, "memory.max", "50M")) + goto cleanup; + + if (cg_write(memcg, "memory.swap.max", "0")) + goto cleanup; + + if (cg_write(memcg, "memory.oom.group", "1")) + goto cleanup; + + safe_pid = cg_run_nowait(memcg, alloc_anon_noexit, (void *) MB(1)); + if (set_oom_adj_score(safe_pid, OOM_SCORE_ADJ_MIN)) + goto cleanup; + + cg_run_nowait(memcg, alloc_anon_noexit, (void *) MB(1)); + if (!cg_run(memcg, alloc_anon, (void *)MB(100))) + goto cleanup; + + if (cg_read_key_long(memcg, "memory.events", "oom_kill ") != 3) + goto cleanup; + + if (kill(safe_pid, SIGKILL)) + goto cleanup; + + ret = KSFT_PASS; + +cleanup: + if (memcg) + cg_destroy(memcg); + free(memcg); + + return ret; +} + + #define T(x) { x, #x } struct memcg_test { int (*fn)(const char *root); @@ -978,6 +1180,9 @@ struct memcg_test { T(test_memcg_oom_events), T(test_memcg_swap_max), T(test_memcg_sock), + T(test_memcg_oom_group_leaf_events), + T(test_memcg_oom_group_parent_events), + T(test_memcg_oom_group_score_events), }; #undef T -- 2.17.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: kselftests for memory.oom.group 2018-09-07 21:34 ` kselftests " jgkamat 2018-09-07 21:34 ` [PATCH v3 1/2] Fix cg_read_strcmp() jgkamat 2018-09-07 21:34 ` [PATCH v3 2/2] Add tests for memory.oom.group jgkamat @ 2018-09-07 22:53 ` Shuah Khan 2 siblings, 0 replies; 17+ messages in thread From: Shuah Khan @ 2018-09-07 22:53 UTC (permalink / raw) To: jgkamat Cc: linux-kselftest, Roman Gushchin, Tejun Heo, kernel-team, linux-kernel, jaygkamat, Shuah Khan On 09/07/2018 03:34 PM, jgkamat@fb.com wrote: > Here is the third version of the patchset. > > Changes since the last patchset: > - Updated commit message of first patch to clarify fixes > - Add ack from Roman > > There should be no code changes since the last patchset. > > Let me know if any improvements can be made, and thanks for your time! > -Jay > > Hi Jay, Thanks for these patches. Applied to linux-kselftest next -- Shuah ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-09-07 22:53 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-05 1:08 kselftests for memory.oom.group jgkamat 2018-09-05 1:08 ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat 2018-09-05 14:43 ` Shuah Khan 2018-09-05 1:08 ` [PATCH 2/2] Add tests for memory.oom.group jgkamat 2018-09-05 15:21 ` Shuah Khan 2018-09-07 16:49 ` kselftests " jgkamat 2018-09-07 16:49 ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat 2018-09-07 16:56 ` Roman Gushchin 2018-09-07 17:06 ` Shuah Khan 2018-09-07 18:28 ` Jay Kamat 2018-09-07 18:53 ` Shuah Khan 2018-09-07 16:49 ` [PATCH 2/2] Add tests for memory.oom.group jgkamat 2018-09-07 16:57 ` Roman Gushchin 2018-09-07 21:34 ` kselftests " jgkamat 2018-09-07 21:34 ` [PATCH v3 1/2] Fix cg_read_strcmp() jgkamat 2018-09-07 21:34 ` [PATCH v3 2/2] Add tests for memory.oom.group jgkamat 2018-09-07 22:53 ` kselftests " Shuah Khan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).