All of lore.kernel.org
 help / color / mirror / Atom feed
* kselftests for memory.oom.group
@ 2018-09-05  1:08 ` jgkamat
  0 siblings, 0 replies; 51+ 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] 51+ messages in thread

* kselftests for memory.oom.group
@ 2018-09-05  1:08 ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-05  1:08 UTC (permalink / raw)


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] 51+ messages in thread

* kselftests for memory.oom.group
@ 2018-09-05  1:08 ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-05  1:08 UTC (permalink / raw)


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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
  2018-09-05  1:08 ` jgkamat
  (?)
@ 2018-09-05  1:08   ` jgkamat
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-05  1:08   ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-05  1:08 UTC (permalink / raw)


From: Jay Kamat <jgkamat at 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 at 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-05  1:08   ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-05  1:08 UTC (permalink / raw)


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 at 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] 51+ messages in thread

* [PATCH 2/2] Add tests for memory.oom.group
  2018-09-05  1:08 ` jgkamat
  (?)
@ 2018-09-05  1:08   ` jgkamat
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH 2/2] Add tests for memory.oom.group
@ 2018-09-05  1:08   ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-05  1:08 UTC (permalink / raw)


From: Jay Kamat <jgkamat at 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 at 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] 51+ messages in thread

* [PATCH 2/2] Add tests for memory.oom.group
@ 2018-09-05  1:08   ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-05  1:08 UTC (permalink / raw)


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 at 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] 51+ messages in thread

* Re: [PATCH 1/2] Fix cg_read_strcmp()
  2018-09-05  1:08   ` jgkamat
  (?)
@ 2018-09-05 14:43     ` shuah
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-05 14:43     ` shuah
  0 siblings, 0 replies; 51+ messages in thread
From: shuah @ 2018-09-05 14:43 UTC (permalink / raw)


Hi Jay,

Thanks for the patch. Couple of comments below.

On 09/04/2018 07:08 PM, jgkamat at fb.com wrote:
> From: Jay Kamat <jgkamat at 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 at 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-05 14:43     ` shuah
  0 siblings, 0 replies; 51+ messages in thread
From: Shuah Khan @ 2018-09-05 14:43 UTC (permalink / raw)


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 at 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 at 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] 51+ messages in thread

* Re: [PATCH 2/2] Add tests for memory.oom.group
  2018-09-05  1:08   ` jgkamat
  (?)
@ 2018-09-05 15:21     ` shuah
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH 2/2] Add tests for memory.oom.group
@ 2018-09-05 15:21     ` shuah
  0 siblings, 0 replies; 51+ messages in thread
From: shuah @ 2018-09-05 15:21 UTC (permalink / raw)


Hi Jay,

Thanks for the patch. Comments below:

On 09/04/2018 07:08 PM, jgkamat at fb.com wrote:
> From: Jay Kamat <jgkamat at 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 at 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] 51+ messages in thread

* [PATCH 2/2] Add tests for memory.oom.group
@ 2018-09-05 15:21     ` shuah
  0 siblings, 0 replies; 51+ messages in thread
From: Shuah Khan @ 2018-09-05 15:21 UTC (permalink / raw)


Hi Jay,

Thanks for the patch. Comments below:

On 09/04/2018 07:08 PM, jgkamat@fb.com wrote:
> From: Jay Kamat <jgkamat at 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 at 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] 51+ messages in thread

* kselftests for memory.oom.group
  2018-09-05  1:08 ` jgkamat
  (?)
@ 2018-09-07 16:49   ` jgkamat
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* kselftests for memory.oom.group
@ 2018-09-07 16:49   ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 16:49 UTC (permalink / raw)


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] 51+ messages in thread

* kselftests for memory.oom.group
@ 2018-09-07 16:49   ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 16:49 UTC (permalink / raw)


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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
  2018-09-07 16:49   ` jgkamat
  (?)
@ 2018-09-07 16:49     ` jgkamat
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-07 16:49     ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 16:49 UTC (permalink / raw)


From: Jay Kamat <jgkamat at 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 at 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-07 16:49     ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 16:49 UTC (permalink / raw)


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 at 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] 51+ messages in thread

* [PATCH 2/2] Add tests for memory.oom.group
  2018-09-07 16:49   ` jgkamat
  (?)
@ 2018-09-07 16:49     ` jgkamat
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH 2/2] Add tests for memory.oom.group
@ 2018-09-07 16:49     ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 16:49 UTC (permalink / raw)


From: Jay Kamat <jgkamat at 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 at 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] 51+ messages in thread

* [PATCH 2/2] Add tests for memory.oom.group
@ 2018-09-07 16:49     ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 16:49 UTC (permalink / raw)


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 at 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] 51+ messages in thread

* Re: [PATCH 1/2] Fix cg_read_strcmp()
  2018-09-07 16:49     ` jgkamat
  (?)
@ 2018-09-07 16:56       ` guro
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-07 16:56       ` guro
  0 siblings, 0 replies; 51+ messages in thread
From: guro @ 2018-09-07 16:56 UTC (permalink / raw)


On Fri, Sep 07, 2018 at 09:49:23AM -0700, jgkamat at fb.com wrote:
> From: Jay Kamat <jgkamat at 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 at fb.com>

^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-07 16:56       ` guro
  0 siblings, 0 replies; 51+ messages in thread
From: Roman Gushchin @ 2018-09-07 16:56 UTC (permalink / raw)


On Fri, Sep 07, 2018@09:49:23AM -0700, jgkamat@fb.com wrote:
> From: Jay Kamat <jgkamat at 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 at fb.com>

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 2/2] Add tests for memory.oom.group
  2018-09-07 16:49     ` jgkamat
  (?)
@ 2018-09-07 16:57       ` guro
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH 2/2] Add tests for memory.oom.group
@ 2018-09-07 16:57       ` guro
  0 siblings, 0 replies; 51+ messages in thread
From: guro @ 2018-09-07 16:57 UTC (permalink / raw)


On Fri, Sep 07, 2018 at 09:49:24AM -0700, jgkamat at fb.com wrote:
> From: Jay Kamat <jgkamat at 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 at fb.com>

Acked-by: Roman Gushchin <guro at fb.com>

^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 2/2] Add tests for memory.oom.group
@ 2018-09-07 16:57       ` guro
  0 siblings, 0 replies; 51+ messages in thread
From: Roman Gushchin @ 2018-09-07 16:57 UTC (permalink / raw)


On Fri, Sep 07, 2018@09:49:24AM -0700, jgkamat@fb.com wrote:
> From: Jay Kamat <jgkamat at 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 at fb.com>

Acked-by: Roman Gushchin <guro at fb.com>

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/2] Fix cg_read_strcmp()
  2018-09-07 16:49     ` jgkamat
  (?)
@ 2018-09-07 17:06       ` shuah
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-07 17:06       ` shuah
  0 siblings, 0 replies; 51+ messages in thread
From: shuah @ 2018-09-07 17:06 UTC (permalink / raw)


On 09/07/2018 10:49 AM, jgkamat at fb.com wrote:
> From: Jay Kamat <jgkamat at 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 at 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-07 17:06       ` shuah
  0 siblings, 0 replies; 51+ messages in thread
From: Shuah Khan @ 2018-09-07 17:06 UTC (permalink / raw)


On 09/07/2018 10:49 AM, jgkamat@fb.com wrote:
> From: Jay Kamat <jgkamat at 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 at 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] 51+ messages in thread

* Re: [PATCH 1/2] Fix cg_read_strcmp()
  2018-09-07 17:06       ` shuah
  (?)
@ 2018-09-07 18:28         ` jgkamat
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-07 18:28         ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 18:28 UTC (permalink / raw)



Shuah Khan writes:

> On 09/07/2018 10:49 AM, jgkamat at fb.com wrote:
>> From: Jay Kamat <jgkamat at 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 at 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-07 18:28         ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: Jay Kamat @ 2018-09-07 18:28 UTC (permalink / raw)



Shuah Khan writes:

> On 09/07/2018 10:49 AM, jgkamat@fb.com wrote:
>> From: Jay Kamat <jgkamat at 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 at 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] 51+ messages in thread

* Re: [PATCH 1/2] Fix cg_read_strcmp()
  2018-09-07 18:28         ` jgkamat
  (?)
@ 2018-09-07 18:53           ` shuah
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-07 18:53           ` shuah
  0 siblings, 0 replies; 51+ messages in thread
From: shuah @ 2018-09-07 18:53 UTC (permalink / raw)


On 09/07/2018 12:28 PM, Jay Kamat wrote:
> 
> Shuah Khan writes:
> 
>> On 09/07/2018 10:49 AM, jgkamat at fb.com wrote:
>>> From: Jay Kamat <jgkamat at 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 at 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] 51+ messages in thread

* [PATCH 1/2] Fix cg_read_strcmp()
@ 2018-09-07 18:53           ` shuah
  0 siblings, 0 replies; 51+ messages in thread
From: Shuah Khan @ 2018-09-07 18:53 UTC (permalink / raw)


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 at 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 at 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] 51+ messages in thread

* kselftests for memory.oom.group
  2018-09-07 16:49   ` jgkamat
  (?)
@ 2018-09-07 21:34     ` jgkamat
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* kselftests for memory.oom.group
@ 2018-09-07 21:34     ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 21:34 UTC (permalink / raw)


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] 51+ messages in thread

* kselftests for memory.oom.group
@ 2018-09-07 21:34     ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 21:34 UTC (permalink / raw)


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] 51+ messages in thread

* [PATCH v3 1/2] Fix cg_read_strcmp()
  2018-09-07 21:34     ` jgkamat
  (?)
@ 2018-09-07 21:34       ` jgkamat
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH v3 1/2] Fix cg_read_strcmp()
@ 2018-09-07 21:34       ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 21:34 UTC (permalink / raw)


From: Jay Kamat <jgkamat at 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 at fb.com>
Acked-by: Roman Gushchin <guro at 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] 51+ messages in thread

* [PATCH v3 1/2] Fix cg_read_strcmp()
@ 2018-09-07 21:34       ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 21:34 UTC (permalink / raw)


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 at fb.com>
Acked-by: Roman Gushchin <guro at 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] 51+ messages in thread

* [PATCH v3 2/2] Add tests for memory.oom.group
  2018-09-07 21:34     ` jgkamat
  (?)
@ 2018-09-07 21:34       ` jgkamat
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* [PATCH v3 2/2] Add tests for memory.oom.group
@ 2018-09-07 21:34       ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 21:34 UTC (permalink / raw)


From: Jay Kamat <jgkamat at 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 at fb.com>
Acked-by: Roman Gushchin <guro at 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] 51+ messages in thread

* [PATCH v3 2/2] Add tests for memory.oom.group
@ 2018-09-07 21:34       ` jgkamat
  0 siblings, 0 replies; 51+ messages in thread
From: jgkamat @ 2018-09-07 21:34 UTC (permalink / raw)


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 at fb.com>
Acked-by: Roman Gushchin <guro at 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] 51+ messages in thread

* Re: kselftests for memory.oom.group
  2018-09-07 21:34     ` jgkamat
  (?)
@ 2018-09-07 22:53       ` shuah
  -1 siblings, 0 replies; 51+ 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] 51+ messages in thread

* kselftests for memory.oom.group
@ 2018-09-07 22:53       ` shuah
  0 siblings, 0 replies; 51+ messages in thread
From: shuah @ 2018-09-07 22:53 UTC (permalink / raw)


On 09/07/2018 03:34 PM, jgkamat at 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] 51+ messages in thread

* kselftests for memory.oom.group
@ 2018-09-07 22:53       ` shuah
  0 siblings, 0 replies; 51+ messages in thread
From: Shuah Khan @ 2018-09-07 22:53 UTC (permalink / raw)


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] 51+ messages in thread

end of thread, other threads:[~2018-09-07 22:53 UTC | newest]

Thread overview: 51+ 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 ` jgkamat
2018-09-05  1:08 ` jgkamat
2018-09-05  1:08 ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat
2018-09-05  1:08   ` jgkamat
2018-09-05  1:08   ` jgkamat
2018-09-05 14:43   ` Shuah Khan
2018-09-05 14:43     ` Shuah Khan
2018-09-05 14:43     ` shuah
2018-09-05  1:08 ` [PATCH 2/2] Add tests for memory.oom.group jgkamat
2018-09-05  1:08   ` jgkamat
2018-09-05  1:08   ` jgkamat
2018-09-05 15:21   ` Shuah Khan
2018-09-05 15:21     ` Shuah Khan
2018-09-05 15:21     ` shuah
2018-09-07 16:49 ` kselftests " jgkamat
2018-09-07 16:49   ` jgkamat
2018-09-07 16:49   ` jgkamat
2018-09-07 16:49   ` [PATCH 1/2] Fix cg_read_strcmp() jgkamat
2018-09-07 16:49     ` jgkamat
2018-09-07 16:49     ` jgkamat
2018-09-07 16:56     ` Roman Gushchin
2018-09-07 16:56       ` Roman Gushchin
2018-09-07 16:56       ` guro
2018-09-07 17:06     ` Shuah Khan
2018-09-07 17:06       ` Shuah Khan
2018-09-07 17:06       ` shuah
2018-09-07 18:28       ` Jay Kamat
2018-09-07 18:28         ` Jay Kamat
2018-09-07 18:28         ` jgkamat
2018-09-07 18:53         ` Shuah Khan
2018-09-07 18:53           ` Shuah Khan
2018-09-07 18:53           ` shuah
2018-09-07 16:49   ` [PATCH 2/2] Add tests for memory.oom.group jgkamat
2018-09-07 16:49     ` jgkamat
2018-09-07 16:49     ` jgkamat
2018-09-07 16:57     ` Roman Gushchin
2018-09-07 16:57       ` Roman Gushchin
2018-09-07 16:57       ` guro
2018-09-07 21:34   ` kselftests " jgkamat
2018-09-07 21:34     ` jgkamat
2018-09-07 21:34     ` jgkamat
2018-09-07 21:34     ` [PATCH v3 1/2] Fix cg_read_strcmp() jgkamat
2018-09-07 21:34       ` 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 21:34       ` jgkamat
2018-09-07 21:34       ` jgkamat
2018-09-07 22:53     ` kselftests " Shuah Khan
2018-09-07 22:53       ` Shuah Khan
2018-09-07 22:53       ` shuah

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.