kselftests for memory.oom.group

kselftests for memory.oom.group

[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

kselftests for memory.oom.group

[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

kselftests for memory.oom.group

[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

[PATCH 1/2] Fix cg_read_strcmp()

[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

[PATCH 1/2] Fix cg_read_strcmp()

[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 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

[PATCH 1/2] Fix cg_read_strcmp()

[jgkamat]

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

Re: [PATCH 1/2] Fix cg_read_strcmp()

[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

[PATCH 1/2] Fix cg_read_strcmp()

[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 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

[PATCH 1/2] Fix cg_read_strcmp()

[shuah]

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

[PATCH 2/2] Add tests for memory.oom.group

[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

[PATCH 2/2] Add tests for memory.oom.group

[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 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

[PATCH 2/2] Add tests for memory.oom.group

[jgkamat]

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

Re: [PATCH 2/2] Add tests for memory.oom.group

[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

[PATCH 2/2] Add tests for memory.oom.group

[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 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

[PATCH 2/2] Add tests for memory.oom.group

[shuah]

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

kselftests for memory.oom.group

[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

kselftests for memory.oom.group

[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

kselftests for memory.oom.group

[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

[PATCH 1/2] Fix cg_read_strcmp()

[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

[PATCH 1/2] Fix cg_read_strcmp()

[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 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

[PATCH 1/2] Fix cg_read_strcmp()

[jgkamat]

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

Re: [PATCH 1/2] Fix cg_read_strcmp()

[Roman Gushchin]

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>

[PATCH 1/2] Fix cg_read_strcmp()

[Roman Gushchin]

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>

[PATCH 1/2] Fix cg_read_strcmp()

[guro]

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>

Re: [PATCH 1/2] Fix cg_read_strcmp()

[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

[PATCH 1/2] Fix cg_read_strcmp()

[Shuah Khan]

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

[PATCH 1/2] Fix cg_read_strcmp()

[shuah]

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

Re: [PATCH 1/2] Fix cg_read_strcmp()

[Jay Kamat]

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

[PATCH 1/2] Fix cg_read_strcmp()

[Jay Kamat]

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

[PATCH 1/2] Fix cg_read_strcmp()

[jgkamat]

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

Re: [PATCH 1/2] Fix cg_read_strcmp()

[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

[PATCH 1/2] Fix cg_read_strcmp()

[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 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

[PATCH 1/2] Fix cg_read_strcmp()

[shuah]

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

[PATCH 2/2] Add tests for memory.oom.group

[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

[PATCH 2/2] Add tests for memory.oom.group

[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 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

[PATCH 2/2] Add tests for memory.oom.group

[jgkamat]

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

Re: [PATCH 2/2] Add tests for memory.oom.group

[Roman Gushchin]

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>

[PATCH 2/2] Add tests for memory.oom.group

[Roman Gushchin]

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>

[PATCH 2/2] Add tests for memory.oom.group

[guro]

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>

kselftests for memory.oom.group

[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

kselftests for memory.oom.group

[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

kselftests for memory.oom.group

[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

[PATCH v3 1/2] Fix cg_read_strcmp()

[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

[PATCH v3 1/2] Fix cg_read_strcmp()

[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 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

[PATCH v3 1/2] Fix cg_read_strcmp()

[jgkamat]

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

[PATCH v3 2/2] Add tests for memory.oom.group

[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

[PATCH v3 2/2] Add tests for memory.oom.group

[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 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

[PATCH v3 2/2] Add tests for memory.oom.group

[jgkamat]

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

Re: kselftests for memory.oom.group

[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

kselftests for memory.oom.group

[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

kselftests for memory.oom.group

[shuah]

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