[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Li Wang]

Many of our LTP tests need Control Group in the configuration,
this patch makes cgroup unified mounting at setup phase to be
possible. The method?is extracted from mem.h with the purpose
of?extendible for further developing, and trying?to compatible
the current two versions of cgroup,

It's hard to make all APIs be strictly consistent because there
are many differences between v1 and v2. But it?capsulate the detail
of cgroup mounting in high-level functions, which will be easier
to use cgroup without considering too much technical thing.? ?

Btw, test get passed on RHEL7(x86_64), RHEL8(ppc64le), Fedora32(x86_64).

Signed-off-by: Li Wang <liwang@redhat.com>
---

Notes:
    v1 --> v2
    	* export tst_cgroup*_path, tst_cgroup_ctl_knob string to user
    	* add a shared list to store the cgroup mount path
    	* add parameter for multiple cgroups mounting support
    	* add tst_cgroup_set_knob() function
    	* add tst_cgroup_mem_swapacct_enabled() function
    	* remove the tst_cgroup_move_current() from lib invoke

 doc/test-writing-guidelines.txt |  44 +++++
 include/tst_cgroup.h            |  38 ++++
 include/tst_test.h              |   1 +
 lib/newlib_tests/test21.c       |  97 +++++++++
 lib/tst_cgroup.c                | 338 ++++++++++++++++++++++++++++++++
 5 files changed, 518 insertions(+)
 create mode 100644 include/tst_cgroup.h
 create mode 100644 lib/newlib_tests/test21.c
 create mode 100644 lib/tst_cgroup.c

diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 93ca506d9..bc450f11a 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -2053,6 +2053,50 @@ the prefix field of file pointed by path equals to the value passed to this func
 Also having a similar api pair TST_ASSERT_FILE_INT/STR(path, prefix, val) to assert
 the field value of file.
 
+2.2.36 Using Control Group
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+Some of LTP tests need Control Group in their configuration, tst_cgroup.h provides
+APIs to make cgroup unified mounting at setup phase to be possible. The method?is
+extracted from mem.h with the purpose of?extendible for further developing, and
+trying to compatible the current two versions of cgroup.
+
+Considering there are many differences between cgroup v1 and v2, here we capsulate
+the detail of cgroup mounting in high-level functions, which will be easier to use
+cgroup without caring about too much technical thing.? ?
+
+Also, we do cgroup mount/umount work for the different hierarchy automatically.
+
+[source,c]
+-------------------------------------------------------------------------------
+#include "tst_test.h"
+
+static void run(void)
+{
+	...
+
+	tst_cgroup_move_current(PATH_TMP_CG1_MEM);
+	tst_cgroup_mem_set_maxbytes(PATH_TMP_CG1_MEM, MEMSIZE);
+
+	// do test under cgroup
+	...
+}
+
+static void setup(void)
+{
+	tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_TMP_CG1_MEM);
+}
+
+static void cleanup(void)
+{
+	tst_cgroup_umount(PATH_TMP_CG1_MEM);
+}
+
+struct tst_test test = {
+	.test_all = run,
+	...
+};
+
+-------------------------------------------------------------------------------
 2.3 Writing a testcase in shell
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
new file mode 100644
index 000000000..bd813b7ec
--- /dev/null
+++ b/include/tst_cgroup.h
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Red Hat, Inc.
+ */
+
+#ifndef TST_CGROUP_H
+#define TST_CGROUP_H
+
+#define PATH_SYS_CGROUP		"/sys/fs/cgroup"
+#define PATH_TMP_CG1_MEM	"/tmp/cg1_mem"
+#define PATH_TMP_CG1_CST	"/tmp/cg1_cst"
+#define PATH_TMP_CGROUP2	"/tmp/cgroup2" /* cgroup v2 has only single hierarchy */
+
+enum tst_cgroup_ver {
+	TST_CGROUP_V1 = 1,
+	TST_CGROUP_V2 = 2,
+};
+
+enum tst_cgroup_ctrl {
+	TST_CGROUP_MEMCG = 1,
+	TST_CGROUP_CPUSET = 2,
+	/* add cgroup controller */
+};
+
+extern char tst_cgroup_mnt_path[PATH_MAX];
+extern char tst_cgroup_new_path[PATH_MAX];
+extern char tst_cgroup_ctl_knob[PATH_MAX];
+
+enum tst_cgroup_ver tst_cgroup_version(void);
+void tst_cgroup_mount(enum tst_cgroup_ctrl ctrl, const char *cgroup_dir);
+void tst_cgroup_umount(const char *cgroup_dir);
+void tst_cgroup_set_knob(const char *cgroup_dir, const char *knob, long value);
+void tst_cgroup_move_current(const char *cgroup_dir);
+void tst_cgroup_mem_set_maxbytes(const char *cgroup_dir, long memsz);
+int tst_cgroup_mem_swapacct_enabled(const char *cgroup_dir);
+void tst_cgroup_mem_set_maxswap(const char *cgroup_dir, long memsz);
+
+#endif /* TST_CGROUP_H */
diff --git a/include/tst_test.h b/include/tst_test.h
index 5bedb4f6f..b84f7b9dd 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -39,6 +39,7 @@
 #include "tst_capability.h"
 #include "tst_hugepage.h"
 #include "tst_assert.h"
+#include "tst_cgroup.h"
 
 /*
  * Reports testcase result.
diff --git a/lib/newlib_tests/test21.c b/lib/newlib_tests/test21.c
new file mode 100644
index 000000000..d8dc0f114
--- /dev/null
+++ b/lib/newlib_tests/test21.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2019 Li Wang <liwang@redhat.com>
+ */
+
+/*
+ * Tests tst_cgroup.h APIs
+ */
+
+#include "tst_test.h"
+#include "tst_cgroup.h"
+
+#define PATH_CGROUP1 "/mnt/liwang1"
+#define PATH_CGROUP2 "/mnt/liwang2"
+#define PATH_CGROUP3 "/mnt/liwang3"
+#define PATH_CGROUP4 "/mnt/liwang4"
+#define PATH_CGROUP5 "/mnt/liwang5"
+#define PATH_CGROUP6 "/mnt/liwang6"
+#define MEMSIZE 1024 * 1024
+
+static void do_test(void)
+{
+	/* we should assign one or more memory nodes to cpuset.mems
+	 * and cpuset.cpus, otherwise, echo $$ > tasks gives ?no space
+	 * left on device? when trying to use cpuset.
+	 *
+	 * Or, setting cgroup.clone_children to 1 can help in automatically
+	 * inheriting memory and node setting from parent cgroup when
+	 * a child cgroup is created.
+	 */
+	tst_cgroup_set_knob(PATH_TMP_CG1_CST, "../cgroup.clone_children", 1);
+
+	pid_t pid = SAFE_FORK();
+
+	switch (pid) {
+	case 0:
+		tst_cgroup_move_current(PATH_CGROUP1);
+		tst_cgroup_mem_set_maxbytes(PATH_CGROUP1, MEMSIZE);
+		tst_cgroup_mem_set_maxswap(PATH_CGROUP1, MEMSIZE);
+
+		tst_cgroup_move_current(PATH_CGROUP2);
+		tst_cgroup_mem_set_maxbytes(PATH_CGROUP2, MEMSIZE);
+		tst_cgroup_mem_set_maxswap(PATH_CGROUP2, MEMSIZE);
+
+		tst_cgroup_move_current(PATH_CGROUP3);
+		tst_cgroup_mem_set_maxbytes(PATH_CGROUP3, MEMSIZE);
+		tst_cgroup_mem_set_maxswap(PATH_CGROUP3, MEMSIZE);
+
+		tst_cgroup_move_current(PATH_CGROUP4);
+		tst_cgroup_move_current(PATH_CGROUP5);
+		tst_cgroup_move_current(PATH_CGROUP6);
+
+	break;
+	default:
+		tst_cgroup_move_current(PATH_TMP_CG1_CST);
+		tst_cgroup_move_current(PATH_TMP_CG1_MEM);
+
+		tst_cgroup_mem_set_maxbytes(PATH_TMP_CG1_MEM, MEMSIZE);
+		tst_cgroup_mem_set_maxswap(PATH_TMP_CG1_MEM, MEMSIZE);
+	break;
+	}
+
+	tst_res(TPASS, "Cgroup mount test");
+}
+
+static void setup(void)
+{
+	tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_TMP_CG1_MEM);
+	tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_CGROUP1);
+	tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_CGROUP2);
+	tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_CGROUP3);
+
+	tst_cgroup_mount(TST_CGROUP_CPUSET, PATH_TMP_CG1_CST);
+	tst_cgroup_mount(TST_CGROUP_CPUSET, PATH_CGROUP4);
+	tst_cgroup_mount(TST_CGROUP_CPUSET, PATH_CGROUP5);
+	tst_cgroup_mount(TST_CGROUP_CPUSET, PATH_CGROUP6);
+}
+
+static void cleanup(void)
+{
+	tst_cgroup_umount(PATH_TMP_CG1_MEM);
+	tst_cgroup_umount(PATH_CGROUP1);
+	tst_cgroup_umount(PATH_CGROUP2);
+	tst_cgroup_umount(PATH_CGROUP3);
+
+	tst_cgroup_umount(PATH_TMP_CG1_CST);
+	tst_cgroup_umount(PATH_CGROUP4);
+	tst_cgroup_umount(PATH_CGROUP5);
+	tst_cgroup_umount(PATH_CGROUP6);
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.setup = setup,
+	.cleanup = cleanup,
+	.forks_child = 1,
+};
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
new file mode 100644
index 000000000..26af77e44
--- /dev/null
+++ b/lib/tst_cgroup.c
@@ -0,0 +1,338 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Red Hat, Inc.
+ */
+
+#define TST_NO_DEFAULT_MAIN
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <time.h>
+#include <sys/mount.h>
+
+#include "tst_test.h"
+#include "tst_safe_macros.h"
+#include "tst_safe_stdio.h"
+#include "tst_cgroup.h"
+#include "tst_device.h"
+
+char tst_cgroup_mnt_path[PATH_MAX];
+char tst_cgroup_new_path[PATH_MAX];
+char tst_cgroup_ctl_knob[PATH_MAX];
+
+static enum tst_cgroup_ver tst_cg_ver;
+
+struct tst_cgroup_path {
+	char *mnt_path;
+	char *new_path;
+	struct tst_cgroup_path *next;
+};
+
+static struct tst_cgroup_path *tst_cgroup_pathes;
+
+static int tst_cgroup_check(const char *cgroup)
+{
+	char line[PATH_MAX];
+	FILE *file;
+	int cg_check = 0;
+
+	file = SAFE_FOPEN("/proc/filesystems", "r");
+	while (fgets(line, sizeof(line), file)) {
+		if (strstr(line, cgroup) != NULL) {
+			cg_check = 1;
+			break;
+		}
+	}
+	SAFE_FCLOSE(file);
+
+	return cg_check;
+}
+
+enum tst_cgroup_ver tst_cgroup_version(void)
+{
+	if (tst_cgroup_check("cgroup2")) {
+		if (!tst_is_mounted("cgroup2") && tst_is_mounted("cgroup"))
+			return TST_CGROUP_V1;
+		else
+			return TST_CGROUP_V2;
+	}
+
+	if (tst_cgroup_check("cgroup"))
+		return TST_CGROUP_V1;
+
+	tst_brk(TCONF, "Cgroup is not configured");
+}
+
+static void tst_cgroup1_mount(const char *name, const char *option,
+			const char *mnt_path, const char *new_path)
+{
+	if (tst_is_mounted(mnt_path))
+		goto out;
+
+	SAFE_MKDIR(mnt_path, 0777);
+	if (mount(name, mnt_path, "cgroup", 0, option) == -1) {
+		if (errno == ENODEV) {
+			if (rmdir(mnt_path) == -1)
+				tst_res(TWARN | TERRNO, "rmdir %s failed", mnt_path);
+			tst_brk(TCONF,
+				 "Cgroup v1 is not configured in kernel");
+		}
+		tst_brk(TBROK | TERRNO, "mount %s", mnt_path);
+	}
+
+out:
+	SAFE_MKDIR(new_path, 0777);
+
+	tst_res(TINFO, "Cgroup(%s) v1 mount at %s success", option, mnt_path);
+}
+
+static void tst_cgroup2_mount(const char *mnt_path, const char *new_path)
+{
+	if (tst_is_mounted(mnt_path))
+		goto out;
+
+	SAFE_MKDIR(mnt_path, 0777);
+	if (mount("cgroup2", mnt_path, "cgroup2", 0, NULL) == -1) {
+		if (errno == ENODEV) {
+			if (rmdir(mnt_path) == -1)
+				tst_res(TWARN | TERRNO, "rmdir %s failed", mnt_path);
+			tst_brk(TCONF,
+				 "Cgroup v2 is not configured in kernel");
+		}
+		tst_brk(TBROK | TERRNO, "mount %s", mnt_path);
+	}
+
+out:
+	SAFE_MKDIR(new_path, 0777);
+
+	tst_res(TINFO, "Cgroup v2 mount at %s success", mnt_path);
+}
+
+static void tst_cgroupN_umount(const char *mnt_path, const char *new_path)
+{
+	FILE *fp;
+	int fd;
+	char s_new[BUFSIZ], s[BUFSIZ], value[BUFSIZ];
+
+	if (!tst_is_mounted(mnt_path))
+		return;
+
+	/* Move all processes in task(v2: cgroup.procs) to its parent node. */
+	if (tst_cg_ver & TST_CGROUP_V1)
+		sprintf(s, "%s/tasks", mnt_path);
+	if (tst_cg_ver & TST_CGROUP_V2)
+		sprintf(s, "%s/cgroup.procs", mnt_path);
+
+	fd = open(s, O_WRONLY);
+	if (fd == -1)
+		tst_res(TWARN | TERRNO, "open %s", s);
+
+	if (tst_cg_ver & TST_CGROUP_V1)
+		snprintf(s_new, BUFSIZ, "%s/tasks", new_path);
+	if (tst_cg_ver & TST_CGROUP_V2)
+		snprintf(s_new, BUFSIZ, "%s/cgroup.procs", new_path);
+
+	fp = fopen(s_new, "r");
+	if (fp == NULL)
+		tst_res(TWARN | TERRNO, "fopen %s", s_new);
+	if ((fd != -1) && (fp != NULL)) {
+		while (fgets(value, BUFSIZ, fp) != NULL)
+			if (write(fd, value, strlen(value) - 1)
+			    != (ssize_t)strlen(value) - 1)
+				tst_res(TWARN | TERRNO, "write %s", s);
+	}
+	if (fd != -1)
+		close(fd);
+	if (fp != NULL)
+		fclose(fp);
+	if (rmdir(new_path) == -1)
+		tst_res(TWARN | TERRNO, "rmdir %s", new_path);
+	if (umount(mnt_path) == -1)
+		tst_res(TWARN | TERRNO, "umount %s", mnt_path);
+	if (rmdir(mnt_path) == -1)
+		tst_res(TWARN | TERRNO, "rmdir %s", mnt_path);
+
+	if (tst_cg_ver & TST_CGROUP_V1)
+		tst_res(TINFO, "Cgroup v1 unmount success");
+	if (tst_cg_ver & TST_CGROUP_V2)
+		tst_res(TINFO, "Cgroup v2 unmount success");
+}
+
+static void tst_cgroup_set_path(const char *cgroup_dir)
+{
+	struct tst_cgroup_path *tst_cgroup_path, *a;
+
+	if (!cgroup_dir)
+		tst_brk(TBROK, "Invalid cgroup dir, plese check cgroup_dir");
+
+	sprintf(tst_cgroup_mnt_path, "%s", cgroup_dir);
+	sprintf(tst_cgroup_new_path, "%s/ltp_%d", cgroup_dir, rand());
+
+	/* To store cgroup path in the shared 'path' list */
+	tst_cgroup_path = SAFE_MMAP(NULL, (sizeof(struct tst_cgroup_path)),
+			PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED, -1, 0);
+	tst_cgroup_path->mnt_path = SAFE_MALLOC(strlen(tst_cgroup_mnt_path));
+	tst_cgroup_path->new_path = SAFE_MALLOC(strlen(tst_cgroup_new_path));
+
+
+	if (!tst_cgroup_pathes) {
+		tst_cgroup_pathes = tst_cgroup_path;
+	} else {
+		a = tst_cgroup_pathes;
+		do {
+			if (!a->next) {
+				a->next = tst_cgroup_path;
+				break;
+			}
+			a = a->next;
+		} while (a);
+	}
+
+	sprintf(tst_cgroup_path->mnt_path, "%s", tst_cgroup_mnt_path);
+	sprintf(tst_cgroup_path->new_path, "%s", tst_cgroup_new_path);
+}
+
+static void tst_cgroup_get_path(const char *cgroup_dir)
+{
+	struct tst_cgroup_path *a = tst_cgroup_pathes;
+
+	while (strcmp(a->mnt_path, cgroup_dir) != 0){
+		a = a->next;
+	};
+
+	sprintf(tst_cgroup_mnt_path, "%s", a->mnt_path);
+	sprintf(tst_cgroup_new_path, "%s", a->new_path);
+}
+
+static void tst_cgroup_del_path(const char *cgroup_dir)
+{
+	struct tst_cgroup_path *a, *b;
+	a = b = tst_cgroup_pathes;
+
+	while (strcmp(b->mnt_path, cgroup_dir) != 0){
+		a = b;
+		b = b->next;
+	};
+
+	if (b == tst_cgroup_pathes)
+		tst_cgroup_pathes = b->next;
+	else
+		a->next = b->next;
+
+	free(b->mnt_path);
+	free(b->new_path);
+	SAFE_MUNMAP(b, sizeof(b));
+}
+
+void tst_cgroup_mount(enum tst_cgroup_ctrl ctrl, const char *cgroup_dir)
+{
+	tst_cg_ver = tst_cgroup_version();
+
+	tst_cgroup_set_path(cgroup_dir);
+
+	if (tst_cg_ver & TST_CGROUP_V1) {
+		switch(ctrl) {
+		case TST_CGROUP_MEMCG:
+			tst_cgroup1_mount("memcg", "memory", tst_cgroup_mnt_path, tst_cgroup_new_path);
+		break;
+		case TST_CGROUP_CPUSET:
+			tst_cgroup1_mount("cpusetcg", "cpuset", tst_cgroup_mnt_path, tst_cgroup_new_path);
+		break;
+		default:
+			tst_brk(TBROK, "Invalid cgroup controller: %d", ctrl);
+		}
+	}
+
+	if (tst_cg_ver & TST_CGROUP_V2) {
+		tst_cgroup2_mount(tst_cgroup_mnt_path, tst_cgroup_new_path);
+
+		switch(ctrl) {
+		case TST_CGROUP_MEMCG:
+			sprintf(tst_cgroup_ctl_knob, "%s/cgroup.subtree_control", tst_cgroup_mnt_path);
+			SAFE_FILE_PRINTF(tst_cgroup_ctl_knob, "%s", "+memory");
+		break;
+		case TST_CGROUP_CPUSET:
+			tst_brk(TCONF, "Cgroup v2 hasn't achieve cpuset subsystem");
+		break;
+		default:
+			tst_brk(TBROK, "Invalid cgroup controller: %d", ctrl);
+		}
+	}
+}
+
+void tst_cgroup_umount(const char *cgroup_dir)
+{
+	tst_cgroup_get_path(cgroup_dir);
+	tst_cgroup_del_path(cgroup_dir);
+	tst_cgroupN_umount(tst_cgroup_mnt_path, tst_cgroup_new_path);
+}
+
+void tst_cgroup_set_knob(const char *cgroup_dir, const char *knob, long value)
+{
+	tst_cgroup_get_path(cgroup_dir);
+
+	sprintf(tst_cgroup_ctl_knob, "%s/%s", tst_cgroup_new_path, knob);
+	SAFE_FILE_PRINTF(tst_cgroup_ctl_knob, "%ld", value);
+}
+
+void tst_cgroup_move_current(const char *cgroup_dir)
+{
+	if (tst_cg_ver & TST_CGROUP_V1)
+		tst_cgroup_set_knob(cgroup_dir, "tasks", getpid());
+
+	if (tst_cg_ver & TST_CGROUP_V2)
+		tst_cgroup_set_knob(cgroup_dir, "cgroup.procs", getpid());
+}
+
+void tst_cgroup_mem_set_maxbytes(const char *cgroup_dir, long memsz)
+{
+	if (tst_cg_ver & TST_CGROUP_V1)
+		tst_cgroup_set_knob(cgroup_dir, "memory.limit_in_bytes", memsz);
+
+	if (tst_cg_ver & TST_CGROUP_V2)
+		tst_cgroup_set_knob(cgroup_dir, "memory.max", memsz);
+}
+
+int tst_cgroup_mem_swapacct_enabled(const char *cgroup_dir)
+{
+	tst_cgroup_get_path(cgroup_dir);
+
+	if (tst_cg_ver & TST_CGROUP_V1) {
+		sprintf(tst_cgroup_ctl_knob, "%s/%s",
+				tst_cgroup_new_path, "/memory.memsw.limit_in_bytes");
+
+		if ((access(tst_cgroup_ctl_knob, F_OK) == -1)) {
+			if (errno == ENOENT)
+				tst_res(TCONF, "memcg swap accounting is disabled");
+			else
+				tst_brk(TBROK | TERRNO, "failed to access %s", tst_cgroup_ctl_knob);
+		} else {
+			return 1;
+		}
+	}
+
+	if (tst_cg_ver & TST_CGROUP_V2) {
+		sprintf(tst_cgroup_ctl_knob, "%s/%s",
+				tst_cgroup_new_path, "/memory.swap.max");
+
+		if ((access(tst_cgroup_ctl_knob, F_OK) == -1)) {
+			if (errno == ENOENT)
+				tst_res(TCONF, "memcg swap accounting is disabled");
+			else
+				tst_brk(TBROK | TERRNO, "failed to access %s", tst_cgroup_ctl_knob);
+		} else {
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+void tst_cgroup_mem_set_maxswap(const char *cgroup_dir, long memsz)
+{
+	if (tst_cg_ver & TST_CGROUP_V1)
+		tst_cgroup_set_knob(cgroup_dir, "memory.memsw.limit_in_bytes", memsz);
+
+	if (tst_cg_ver & TST_CGROUP_V2)
+		tst_cgroup_set_knob(cgroup_dir, "memory.swap.max", memsz);
+}
-- 
2.21.1

[LTP] [PATCH v2 2/4] mem: take use of new cgroup API

[Li Wang]

For issue #611:
  https://github.com/linux-test-project/ltp/issues/611

Signed-off-by: Li Wang <liwang@redhat.com>
---
 testcases/kernel/mem/cpuset/cpuset01.c | 19 +++++++------
 testcases/kernel/mem/include/mem.h     |  1 +
 testcases/kernel/mem/ksm/ksm02.c       |  8 +++---
 testcases/kernel/mem/ksm/ksm03.c       | 12 +++-----
 testcases/kernel/mem/ksm/ksm04.c       | 16 +++++------
 testcases/kernel/mem/lib/mem.c         |  8 ++----
 testcases/kernel/mem/oom/oom03.c       | 22 ++++-----------
 testcases/kernel/mem/oom/oom04.c       | 12 ++++----
 testcases/kernel/mem/oom/oom05.c       | 38 ++++++++------------------
 9 files changed, 52 insertions(+), 84 deletions(-)

diff --git a/testcases/kernel/mem/cpuset/cpuset01.c b/testcases/kernel/mem/cpuset/cpuset01.c
index 853f7fe55..00b79cf3d 100644
--- a/testcases/kernel/mem/cpuset/cpuset01.c
+++ b/testcases/kernel/mem/cpuset/cpuset01.c
@@ -51,11 +51,11 @@ static void test_cpuset(void)
 	unsigned long nmask[MAXNODES / BITS_PER_LONG] = { 0 };
 	char mems[BUFSIZ], buf[BUFSIZ];
 
-	read_cpuset_files(CPATH, "cpus", buf);
-	write_cpuset_files(CPATH_NEW, "cpus", buf);
-	read_cpuset_files(CPATH, "mems", mems);
-	write_cpuset_files(CPATH_NEW, "mems", mems);
-	SAFE_FILE_PRINTF(CPATH_NEW "/tasks", "%d", getpid());
+	tst_cgroup_move_current(PATH_TMP_CG1_CST);
+	read_cpuset_files(tst_cgroup_mnt_path, "cpus", buf);
+	write_cpuset_files(tst_cgroup_new_path, "cpus", buf);
+	read_cpuset_files(tst_cgroup_mnt_path, "mems", mems);
+	write_cpuset_files(tst_cgroup_new_path, "mems", mems);
 
 	child = SAFE_FORK();
 	if (child == 0) {
@@ -70,9 +70,9 @@ static void test_cpuset(void)
 	}
 
 	snprintf(buf, BUFSIZ, "%d", nodes[0]);
-	write_cpuset_files(CPATH_NEW, "mems", buf);
+	write_cpuset_files(tst_cgroup_new_path, "mems", buf);
 	snprintf(buf, BUFSIZ, "%d", nodes[1]);
-	write_cpuset_files(CPATH_NEW, "mems", buf);
+	write_cpuset_files(tst_cgroup_new_path, "mems", buf);
 
 	SAFE_WAITPID(child, &status, WUNTRACED | WCONTINUED);
 	if (WEXITSTATUS(status) != 0) {
@@ -85,7 +85,7 @@ static void test_cpuset(void)
 
 static void setup(void)
 {
-	mount_mem("cpuset", "cpuset", NULL, CPATH, CPATH_NEW);
+	tst_cgroup_mount(TST_CGROUP_CPUSET, PATH_TMP_CG1_CST);
 	ncpus = count_cpu();
 	if (get_allowed_nodes_arr(NH_MEMS | NH_CPUS, &nnodes, &nodes) < 0)
 		tst_brk(TBROK | TERRNO, "get_allowed_nodes_arr");
@@ -95,7 +95,7 @@ static void setup(void)
 
 static void cleanup(void)
 {
-	umount_mem(CPATH, CPATH_NEW);
+	tst_cgroup_umount(PATH_TMP_CG1_CST);
 }
 
 static void sighandler(int signo LTP_ATTRIBUTE_UNUSED)
@@ -183,6 +183,7 @@ static long count_cpu(void)
 
 static struct tst_test test = {
 	.needs_root = 1,
+	.forks_child = 1,
 	.setup = setup,
 	.cleanup = cleanup,
 	.test_all = test_cpuset,
diff --git a/testcases/kernel/mem/include/mem.h b/testcases/kernel/mem/include/mem.h
index cce9c0497..f553651c2 100644
--- a/testcases/kernel/mem/include/mem.h
+++ b/testcases/kernel/mem/include/mem.h
@@ -2,6 +2,7 @@
 #define _MEM_H
 #include "config.h"
 #include "tst_test.h"
+#include "tst_cgroup.h"
 #include "ksm_helper.h"
 
 #if defined(__powerpc__) || defined(__powerpc64__)
diff --git a/testcases/kernel/mem/ksm/ksm02.c b/testcases/kernel/mem/ksm/ksm02.c
index 3d4c19deb..d680db247 100644
--- a/testcases/kernel/mem/ksm/ksm02.c
+++ b/testcases/kernel/mem/ksm/ksm02.c
@@ -66,6 +66,8 @@ static void verify_ksm(void)
 	unsigned long nmask[MAXNODES / BITS_PER_LONG] = { 0 };
 	unsigned int node;
 
+	tst_cgroup_move_current(PATH_TMP_CG1_CST);
+
 	node = get_a_numa_node();
 	set_node(nmask, node);
 
@@ -88,8 +90,7 @@ static void cleanup(void)
 		FILE_PRINTF(PATH_KSM "merge_across_nodes",
 				 "%d", merge_across_nodes);
 
-	if (cpuset_mounted)
-		umount_mem(CPATH, CPATH_NEW);
+	tst_cgroup_umount(PATH_TMP_CG1_CST);
 }
 
 static void setup(void)
@@ -105,8 +106,7 @@ static void setup(void)
 		SAFE_FILE_PRINTF(PATH_KSM "merge_across_nodes", "1");
 	}
 
-	mount_mem("cpuset", "cpuset", NULL, CPATH, CPATH_NEW);
-	cpuset_mounted = 1;
+	tst_cgroup_mount(TST_CGROUP_CPUSET, PATH_TMP_CG1_CST);
 }
 
 static struct tst_test test = {
diff --git a/testcases/kernel/mem/ksm/ksm03.c b/testcases/kernel/mem/ksm/ksm03.c
index 0e5131654..649203595 100644
--- a/testcases/kernel/mem/ksm/ksm03.c
+++ b/testcases/kernel/mem/ksm/ksm03.c
@@ -59,11 +59,10 @@
 #include "mem.h"
 #include "ksm_common.h"
 
-static int memcg_mounted;
-
 static void verify_ksm(void)
 {
-	write_memcg();
+	tst_cgroup_move_current(PATH_TMP_CG1_MEM);
+	tst_cgroup_mem_set_maxbytes(PATH_TMP_CG1_MEM, TESTMEM);
 	create_same_memory(size, num, unit);
 }
 
@@ -79,8 +78,7 @@ static void setup(void)
 	}
 
 	parse_ksm_options(opt_sizestr, &size, opt_numstr, &num, opt_unitstr, &unit);
-	mount_mem("memcg", "cgroup", "memory", MEMCG_PATH, MEMCG_PATH_NEW);
-	memcg_mounted = 1;
+	tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_TMP_CG1_MEM);
 }
 
 static void cleanup(void)
@@ -88,9 +86,7 @@ static void cleanup(void)
 	if (access(PATH_KSM "merge_across_nodes", F_OK) == 0)
 		FILE_PRINTF(PATH_KSM "merge_across_nodes",
 				 "%d", merge_across_nodes);
-
-	if (memcg_mounted)
-		umount_mem(MEMCG_PATH, MEMCG_PATH_NEW);
+	tst_cgroup_umount(PATH_TMP_CG1_MEM);
 }
 
 static struct tst_test test = {
diff --git a/testcases/kernel/mem/ksm/ksm04.c b/testcases/kernel/mem/ksm/ksm04.c
index e393dbd40..abd3e0978 100644
--- a/testcases/kernel/mem/ksm/ksm04.c
+++ b/testcases/kernel/mem/ksm/ksm04.c
@@ -70,7 +70,8 @@ static void verify_ksm(void)
 	node = get_a_numa_node();
 	set_node(nmask, node);
 
-	write_memcg();
+	tst_cgroup_move_current(PATH_TMP_CG1_MEM);
+	tst_cgroup_mem_set_maxbytes(PATH_TMP_CG1_MEM, TESTMEM);
 
 	if (set_mempolicy(MPOL_BIND, nmask, MAXNODES) == -1) {
 		if (errno != ENOSYS)
@@ -81,6 +82,7 @@ static void verify_ksm(void)
 	}
 	create_same_memory(size, num, unit);
 
+	tst_cgroup_move_current(PATH_TMP_CG1_CST);
 	write_cpusets(node);
 	create_same_memory(size, num, unit);
 }
@@ -91,10 +93,8 @@ static void cleanup(void)
 		FILE_PRINTF(PATH_KSM "merge_across_nodes",
 				 "%d", merge_across_nodes);
 
-	if (cpuset_mounted)
-		umount_mem(CPATH, CPATH_NEW);
-	if (memcg_mounted)
-		umount_mem(MEMCG_PATH, MEMCG_PATH_NEW);
+	tst_cgroup_umount(PATH_TMP_CG1_MEM);
+	tst_cgroup_umount(PATH_TMP_CG1_CST);
 }
 
 static void setup(void)
@@ -110,10 +110,8 @@ static void setup(void)
 
 	parse_ksm_options(opt_sizestr, &size, opt_numstr, &num, opt_unitstr, &unit);
 
-	mount_mem("cpuset", "cpuset", NULL, CPATH, CPATH_NEW);
-	cpuset_mounted = 1;
-	mount_mem("memcg", "cgroup", "memory", MEMCG_PATH, MEMCG_PATH_NEW);
-	memcg_mounted = 1;
+	tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_TMP_CG1_MEM);
+	tst_cgroup_mount(TST_CGROUP_CPUSET, PATH_TMP_CG1_CST);
 }
 
 static struct tst_test test = {
diff --git a/testcases/kernel/mem/lib/mem.c b/testcases/kernel/mem/lib/mem.c
index eca4c61c8..45f155922 100644
--- a/testcases/kernel/mem/lib/mem.c
+++ b/testcases/kernel/mem/lib/mem.c
@@ -692,7 +692,7 @@ void write_cpusets(long nd)
 	char cpus[BUFSIZ] = "";
 
 	snprintf(buf, BUFSIZ, "%ld", nd);
-	write_cpuset_files(CPATH_NEW, "mems", buf);
+	write_cpuset_files(tst_cgroup_new_path, "mems", buf);
 
 	gather_node_cpus(cpus, nd);
 	/*
@@ -701,14 +701,12 @@ void write_cpusets(long nd)
 	 * the value of cpuset.cpus.
 	 */
 	if (strlen(cpus) != 0) {
-		write_cpuset_files(CPATH_NEW, "cpus", cpus);
+		write_cpuset_files(tst_cgroup_new_path, "cpus", cpus);
 	} else {
 		tst_res(TINFO, "No CPUs in the node%ld; "
 				"using only CPU0", nd);
-		write_cpuset_files(CPATH_NEW, "cpus", "0");
+		write_cpuset_files(tst_cgroup_new_path, "cpus", "0");
 	}
-
-	SAFE_FILE_PRINTF(CPATH_NEW "/tasks", "%d", getpid());
 }
 
 void umount_mem(char *path, char *path_new)
diff --git a/testcases/kernel/mem/oom/oom03.c b/testcases/kernel/mem/oom/oom03.c
index ce0b34c31..150824003 100644
--- a/testcases/kernel/mem/oom/oom03.c
+++ b/testcases/kernel/mem/oom/oom03.c
@@ -36,27 +36,19 @@
 
 #ifdef HAVE_NUMA_V2
 
-static int memcg_mounted;
-
 static void verify_oom(void)
 {
 #ifdef TST_ABI32
 	tst_brk(TCONF, "test is not designed for 32-bit system.");
 #endif
 
-	SAFE_FILE_PRINTF(MEMCG_PATH_NEW "/tasks", "%d", getpid());
-	SAFE_FILE_PRINTF(MEMCG_LIMIT, "%ld", TESTMEM);
+	tst_cgroup_move_current(PATH_TMP_CG1_MEM);
+	tst_cgroup_mem_set_maxbytes(PATH_TMP_CG1_MEM, TESTMEM);
 
 	testoom(0, 0, ENOMEM, 1);
 
-	if (access(MEMCG_SW_LIMIT, F_OK) == -1) {
-		if (errno == ENOENT)
-			tst_res(TCONF,
-				"memcg swap accounting is disabled");
-		else
-			tst_brk(TBROK | TERRNO, "access");
-	} else {
-		SAFE_FILE_PRINTF(MEMCG_SW_LIMIT, "%ld", TESTMEM);
+	if (tst_cgroup_mem_swapacct_enabled(PATH_TMP_CG1_MEM)) {
+		tst_cgroup_mem_set_maxswap(PATH_TMP_CG1_MEM, TESTMEM);
 		testoom(0, 1, ENOMEM, 1);
 	}
 
@@ -73,16 +65,14 @@ static void setup(void)
 {
 	overcommit = get_sys_tune("overcommit_memory");
 	set_sys_tune("overcommit_memory", 1, 1);
-	mount_mem("memcg", "cgroup", "memory", MEMCG_PATH, MEMCG_PATH_NEW);
-	memcg_mounted = 1;
+	tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_TMP_CG1_MEM);
 }
 
 static void cleanup(void)
 {
 	if (overcommit != -1)
 		set_sys_tune("overcommit_memory", overcommit, 0);
-	if (memcg_mounted)
-		umount_mem(MEMCG_PATH, MEMCG_PATH_NEW);
+	tst_cgroup_umount(PATH_TMP_CG1_MEM);
 }
 
 static struct tst_test test = {
diff --git a/testcases/kernel/mem/oom/oom04.c b/testcases/kernel/mem/oom/oom04.c
index 7185ef973..bff3faf97 100644
--- a/testcases/kernel/mem/oom/oom04.c
+++ b/testcases/kernel/mem/oom/oom04.c
@@ -36,14 +36,14 @@
 
 #ifdef HAVE_NUMA_V2
 
-static int cpuset_mounted;
-
 static void verify_oom(void)
 {
 #ifdef TST_ABI32
 	tst_brk(TCONF, "test is not designed for 32-bit system.");
 #endif
 
+	tst_cgroup_move_current(PATH_TMP_CG1_CST);
+
 	tst_res(TINFO, "OOM on CPUSET...");
 	testoom(0, 0, ENOMEM, 1);
 
@@ -53,7 +53,7 @@ static void verify_oom(void)
 		 * is in charge of cpuset.memory_migrate, we can write
 		 * 1 to cpuset.memory_migrate to enable the migration.
 		 */
-		write_cpuset_files(CPATH_NEW, "memory_migrate", "1");
+		write_cpuset_files(tst_cgroup_new_path, "memory_migrate", "1");
 		tst_res(TINFO, "OOM on CPUSET with mem migrate:");
 		testoom(0, 0, ENOMEM, 1);
 	}
@@ -69,8 +69,7 @@ static void setup(void)
 	overcommit = get_sys_tune("overcommit_memory");
 	set_sys_tune("overcommit_memory", 1, 1);
 
-	mount_mem("cpuset", "cpuset", NULL, CPATH, CPATH_NEW);
-	cpuset_mounted = 1;
+	tst_cgroup_mount(TST_CGROUP_CPUSET, PATH_TMP_CG1_CST);
 
 	/*
 	 * Some nodes do not contain memory, so use
@@ -89,8 +88,7 @@ static void cleanup(void)
 {
 	if (overcommit != -1)
 		set_sys_tune("overcommit_memory", overcommit, 0);
-	if (cpuset_mounted)
-		umount_mem(CPATH, CPATH_NEW);
+	tst_cgroup_umount(PATH_TMP_CG1_CST);
 }
 
 static struct tst_test test = {
diff --git a/testcases/kernel/mem/oom/oom05.c b/testcases/kernel/mem/oom/oom05.c
index db24df6de..aacbbc220 100644
--- a/testcases/kernel/mem/oom/oom05.c
+++ b/testcases/kernel/mem/oom/oom05.c
@@ -36,18 +36,16 @@
 
 #ifdef HAVE_NUMA_V2
 
-static int memcg_mounted;
-static int cpuset_mounted;
-
 static void verify_oom(void)
 {
-	int swap_acc_on = 1;
-
 #ifdef TST_ABI32
 	tst_brk(TCONF, "test is not designed for 32-bit system.");
 #endif
 
 	tst_res(TINFO, "OOM on CPUSET & MEMCG...");
+	tst_cgroup_move_current(PATH_TMP_CG1_MEM);
+	tst_cgroup_move_current(PATH_TMP_CG1_CST);
+	tst_cgroup_mem_set_maxbytes(PATH_TMP_CG1_MEM, TESTMEM);
 	testoom(0, 0, ENOMEM, 1);
 
 	/*
@@ -56,29 +54,22 @@ static void verify_oom(void)
 	 * 1 to cpuset.memory_migrate to enable the migration.
 	 */
 	if (is_numa(NULL, NH_MEMS, 2)) {
-		write_cpuset_files(CPATH_NEW, "memory_migrate", "1");
+		tst_cgroup_move_current(PATH_TMP_CG1_CST);
+		write_cpuset_files(tst_cgroup_new_path, "memory_migrate", "1");
 		tst_res(TINFO, "OOM on CPUSET & MEMCG with "
 				"cpuset.memory_migrate=1");
 		testoom(0, 0, ENOMEM, 1);
 	}
 
-	if (access(MEMCG_SW_LIMIT, F_OK) == -1) {
-		if (errno == ENOENT) {
-			tst_res(TCONF, "memcg swap accounting is disabled");
-			swap_acc_on = 0;
-		} else
-			tst_brk(TBROK|TERRNO, "access");
-	}
-
-	if (swap_acc_on) {
+	if (tst_cgroup_mem_swapacct_enabled(PATH_TMP_CG1_MEM)) {
 		tst_res(TINFO, "OOM on CPUSET & MEMCG with "
 				"special memswap limitation:");
-		SAFE_FILE_PRINTF(MEMCG_SW_LIMIT, "%ld", TESTMEM);
+		tst_cgroup_mem_set_maxswap(PATH_TMP_CG1_MEM, TESTMEM);
 		testoom(0, 0, ENOMEM, 1);
 
 		tst_res(TINFO, "OOM on CPUSET & MEMCG with "
 				"disabled memswap limitation:");
-		SAFE_FILE_PRINTF(MEMCG_SW_LIMIT, "-1");
+		tst_cgroup_mem_set_maxswap(PATH_TMP_CG1_MEM, -1);
 		testoom(0, 0, ENOMEM, 1);
 	}
 }
@@ -93,11 +84,8 @@ void setup(void)
 	overcommit = get_sys_tune("overcommit_memory");
 	set_sys_tune("overcommit_memory", 1, 1);
 
-	mount_mem("memcg", "cgroup", "memory", MEMCG_PATH, MEMCG_PATH_NEW);
-	memcg_mounted = 1;
-	mount_mem("cpuset", "cpuset", NULL, CPATH, CPATH_NEW);
-	cpuset_mounted = 1;
-	write_memcg();
+	tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_TMP_CG1_MEM);
+	tst_cgroup_mount(TST_CGROUP_CPUSET, PATH_TMP_CG1_CST);
 
 	/*
 	 * Some nodes do not contain memory, so use
@@ -116,10 +104,8 @@ void cleanup(void)
 {
 	if (overcommit != -1)
 		set_sys_tune("overcommit_memory", overcommit, 0);
-	if (cpuset_mounted)
-		umount_mem(CPATH, CPATH_NEW);
-	if (memcg_mounted)
-		umount_mem(MEMCG_PATH, MEMCG_PATH_NEW);
+	tst_cgroup_umount(PATH_TMP_CG1_MEM);
+	tst_cgroup_umount(PATH_TMP_CG1_CST);
 }
 
 static struct tst_test test = {
-- 
2.21.1

[LTP] [PATCH v2 3/4] mem: remove the old cgroup API

[Li Wang]

Signed-off-by: Li Wang <liwang@redhat.com>
---
 testcases/kernel/mem/include/mem.h | 16 +--------
 testcases/kernel/mem/lib/mem.c     | 58 ------------------------------
 2 files changed, 1 insertion(+), 73 deletions(-)

diff --git a/testcases/kernel/mem/include/mem.h b/testcases/kernel/mem/include/mem.h
index f553651c2..0e089fae2 100644
--- a/testcases/kernel/mem/include/mem.h
+++ b/testcases/kernel/mem/include/mem.h
@@ -60,24 +60,10 @@ void test_ksm_merge_across_nodes(unsigned long nr_pages);
 void check_hugepage(void);
 void write_memcg(void);
 
-/* cpuset/memcg */
-
-#define CPATH			"/dev/cpuset"
-#define CPATH_NEW		CPATH "/1"
-#define MEMCG_PATH		"/dev/cgroup"
-#define MEMCG_PATH_NEW		MEMCG_PATH "/1"
-#define MEMCG_LIMIT		MEMCG_PATH_NEW "/memory.limit_in_bytes"
-#define MEMCG_SW_LIMIT		MEMCG_PATH_NEW "/memory.memsw.limit_in_bytes"
-#if HAVE_SYS_EVENTFD_H
-#define PATH_OOMCTRL		MEMCG_PATH_NEW "/memory.oom_control"
-#define PATH_EVTCTRL		MEMCG_PATH_NEW "/cgroup.event_control"
-#endif
-
+/* cpuset/memcg - include/tst_cgroup.h */
 void read_cpuset_files(char *prefix, char *filename, char *retbuf);
 void write_cpuset_files(char *prefix, char *filename, char *buf);
 void write_cpusets(long nd);
-void mount_mem(char *name, char *fs, char *options, char *path, char *path_new);
-void umount_mem(char *path, char *path_new);
 
 /* shared */
 unsigned int get_a_numa_node(void);
diff --git a/testcases/kernel/mem/lib/mem.c b/testcases/kernel/mem/lib/mem.c
index 45f155922..7a911f8b3 100644
--- a/testcases/kernel/mem/lib/mem.c
+++ b/testcases/kernel/mem/lib/mem.c
@@ -312,13 +312,6 @@ void check_hugepage(void)
 		tst_brk(TCONF, "Huge page is not supported.");
 }
 
-void write_memcg(void)
-{
-	SAFE_FILE_PRINTF(MEMCG_LIMIT, "%ld", TESTMEM);
-
-	SAFE_FILE_PRINTF(MEMCG_PATH_NEW "/tasks", "%d", getpid());
-}
-
 struct ksm_merge_data {
 	char data;
 	unsigned int mergeable_size;
@@ -709,57 +702,6 @@ void write_cpusets(long nd)
 	}
 }
 
-void umount_mem(char *path, char *path_new)
-{
-	FILE *fp;
-	int fd;
-	char s_new[BUFSIZ], s[BUFSIZ], value[BUFSIZ];
-
-	/* Move all processes in task to its parent node. */
-	sprintf(s, "%s/tasks", path);
-	fd = open(s, O_WRONLY);
-	if (fd == -1)
-		tst_res(TWARN | TERRNO, "open %s", s);
-
-	snprintf(s_new, BUFSIZ, "%s/tasks", path_new);
-	fp = fopen(s_new, "r");
-	if (fp == NULL)
-		tst_res(TWARN | TERRNO, "fopen %s", s_new);
-	if ((fd != -1) && (fp != NULL)) {
-		while (fgets(value, BUFSIZ, fp) != NULL)
-			if (write(fd, value, strlen(value) - 1)
-			    != (ssize_t)strlen(value) - 1)
-				tst_res(TWARN | TERRNO, "write %s", s);
-	}
-	if (fd != -1)
-		close(fd);
-	if (fp != NULL)
-		fclose(fp);
-	if (rmdir(path_new) == -1)
-		tst_res(TWARN | TERRNO, "rmdir %s", path_new);
-	if (umount(path) == -1)
-		tst_res(TWARN | TERRNO, "umount %s", path);
-	if (rmdir(path) == -1)
-		tst_res(TWARN | TERRNO, "rmdir %s", path);
-}
-
-void mount_mem(char *name, char *fs, char *options, char *path, char *path_new)
-{
-	SAFE_MKDIR(path, 0777);
-	if (mount(name, path, fs, 0, options) == -1) {
-		if (errno == ENODEV) {
-			if (rmdir(path) == -1)
-				tst_res(TWARN | TERRNO, "rmdir %s failed",
-					 path);
-			tst_brk(TCONF,
-				 "file system %s is not configured in kernel",
-				 fs);
-		}
-		tst_brk(TBROK | TERRNO, "mount %s", path);
-	}
-	SAFE_MKDIR(path_new, 0777);
-}
-
 /* shared */
 
 /* Warning: *DO NOT* use this function in child */
-- 
2.21.1

[LTP] [PATCH v2 4/4] mm: add cpuset01 to runtest file

[Li Wang]

This test was moved to stress test since commit ae8fa55a8, but stress
runtest has been removed from commit e752f7c19674 :).

Here adding back it as a general memory test without '-I 3600'.

Signed-off-by: Li Wang <liwang@redhat.com>
---
 runtest/mm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/runtest/mm b/runtest/mm
index a09f39c1e..612a4d066 100644
--- a/runtest/mm
+++ b/runtest/mm
@@ -73,6 +73,8 @@ ksm06 ksm06
 ksm06_1 ksm06 -n 10
 ksm06_2 ksm06 -n 10000
 
+cpuset01 cpuset01
+
 oom01 oom01
 oom02 oom02
 oom03 oom03
-- 
2.21.1

[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Li Wang]

Li Wang <liwang@redhat.com> wrote:

...
>
> +#define PATH_SYS_CGROUP                "/sys/fs/cgroup"
> +#define PATH_TMP_CG1_MEM       "/tmp/cg1_mem"
> +#define PATH_TMP_CG1_CST       "/tmp/cg1_cst"
> +#define PATH_TMP_CGROUP2       "/tmp/cgroup2" /* cgroup v2 has only
> single hierarchy */
> +
>

Seems there is no need to distinguish CG1 and CG2 mount directory
in tst_cgroup.h, because we have introduced the 'tst_cgroup_mnt_path'
to mount cgroup unified, it will choose cgroup versions automatically.

So maybe we can change these above definitions to:

-#define PATH_SYS_CGROUP                "/sys/fs/cgroup"
-#define PATH_TMP_CG1_MEM       "/tmp/cg1_mem"
-#define PATH_TMP_CG1_CST       "/tmp/cg1_cst"
-#define PATH_TMP_CGROUP2       "/tmp/cgroup2" /* cgroup v2 has only single
hierarchy */
+#define PATH_TMP_CGROUP_MEM    "/tmp/cgroup_mem"
+#define PATH_TMP_CGROUP_CST    "/tmp/cgroup_cst"

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200601/8375be37/attachment.htm>

[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Jan Stancek]

----- Original Message -----
> +
> +[source,c]
> +-------------------------------------------------------------------------------
> +#include "tst_test.h"
> +
> +static void run(void)
> +{
> +	...
> +
> +	tst_cgroup_move_current(PATH_TMP_CG1_MEM);
> +	tst_cgroup_mem_set_maxbytes(PATH_TMP_CG1_MEM, MEMSIZE);

Goal for API is to hide differences between cgroup 1/2, but example above
is passing cgroup specific directory.

My suggestion was to have directory parameter relative to cgroup mount,
I didn't consider there would be need for mounting cgroup more than once
from single process. Is there such need?

Since there's only one global 'tst_cgroup_mnt_path', is there need to have
paths absolute? If we assume that single process will mount cgroup only once,
then all paths could be relative to 'tst_cgroup_mnt_path', and test doesn't
need to even use 'tst_cgroup_mnt_path'.

> +
> +static void tst_cgroup_set_path(const char *cgroup_dir)
> +{
> +	struct tst_cgroup_path *tst_cgroup_path, *a;
> +
> +	if (!cgroup_dir)
> +		tst_brk(TBROK, "Invalid cgroup dir, plese check cgroup_dir");
> +
> +	sprintf(tst_cgroup_mnt_path, "%s", cgroup_dir);
> +	sprintf(tst_cgroup_new_path, "%s/ltp_%d", cgroup_dir, rand());
> +
> +	/* To store cgroup path in the shared 'path' list */
> +	tst_cgroup_path = SAFE_MMAP(NULL, (sizeof(struct tst_cgroup_path)),
> +			PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_SHARED, -1, 0);

I'm not sure I understand what is the reason to have tst_cgroup_path. Is it expected,
that mount and umount are called by different processes? It might be easier
to define API as per-process and require same process to call mount and umount.

> +	tst_cgroup_path->mnt_path = SAFE_MALLOC(strlen(tst_cgroup_mnt_path));
> +	tst_cgroup_path->new_path = SAFE_MALLOC(strlen(tst_cgroup_new_path));

Pointers are in shared memory, but content they point to is not, so it's accessible
only from process that called tst_cgroup_set_path().

Can you describe all different scenarios you wanted to support?

[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Li Wang]

Hi Jan,

On Mon, Jun 1, 2020 at 9:57 PM Jan Stancek <jstancek@redhat.com> wrote:

>
>
> ----- Original Message -----
> > +
> > +[source,c]
> >
> +-------------------------------------------------------------------------------
> > +#include "tst_test.h"
> > +
> > +static void run(void)
> > +{
> > +     ...
> > +
> > +     tst_cgroup_move_current(PATH_TMP_CG1_MEM);
> > +     tst_cgroup_mem_set_maxbytes(PATH_TMP_CG1_MEM, MEMSIZE);
>
> Goal for API is to hide differences between cgroup 1/2, but example above
> is passing cgroup specific directory.
>

Sorry for the misleading info, actually that's no different to use any
directory whatever you pass via parameter, it will recognize cgroup
version to be mounted intelligently.

Here as I commented in the last email. the specific dir are typos which
inherent from patch v1. We should fix them.


>
> My suggestion was to have directory parameter relative to cgroup mount,
>

Patch v2 is already achieved this even includes more functions(i.e mount
many same cgroup).



> I didn't consider there would be need for mounting cgroup more than once
> from single process. Is there such need?
>

Yes right, the test21.c mounting many cgroups in a single process is just a
test
 to verify these APIs works fine, it meaningless in real situations.


>
> Since there's only one global 'tst_cgroup_mnt_path', is there need to have
> paths absolute? If we assume that single process will mount cgroup only
> once,
> then all paths could be relative to 'tst_cgroup_mnt_path', and test doesn't
> need to even use 'tst_cgroup_mnt_path'.
>

No, the global 'tst_cgroup_mnt_path' is not only to pass mount directory but
also for storing path when searching from get_cgroup_get_path().

Why we need this? Because, if a testcase(i.e oom05.c) needs more than one
cgroup
subsystem(memory, cpuset) on RHEL7(cgroup-v1), it should mount two
different
directories and do some knob setting.


>
> > +
> > +static void tst_cgroup_set_path(const char *cgroup_dir)
> > +{
> > +     struct tst_cgroup_path *tst_cgroup_path, *a;
> > +
> > +     if (!cgroup_dir)
> > +             tst_brk(TBROK, "Invalid cgroup dir, plese check
> cgroup_dir");
> > +
> > +     sprintf(tst_cgroup_mnt_path, "%s", cgroup_dir);
> > +     sprintf(tst_cgroup_new_path, "%s/ltp_%d", cgroup_dir, rand());
> > +
> > +     /* To store cgroup path in the shared 'path' list */
> > +     tst_cgroup_path = SAFE_MMAP(NULL, (sizeof(struct tst_cgroup_path)),
> > +                     PROT_READ | PROT_WRITE, MAP_ANONYMOUS |
> MAP_SHARED, -1, 0);
>
> I'm not sure I understand what is the reason to have tst_cgroup_path. Is
> it expected,
> that mount and umount are called by different processes? It might be easier
>

The shared 'tst_cgroup_path' is necessary especially for mounting
different cgoups in setup(). Otherwise, it would be easy to get lost
which directory for kind of cgroup type.

And the worth to say, the random directory name for same cgroup
mounting is also on purpose, though we mount same(i.e memory)
cgroup in two places it still belongs to the one hierarchy, and create
the same name of the new directory will be hit an error in EEXIST.

static void tst_cgroup_set_path(const char *cgroup_dir)
{
    ...
    sprintf(tst_cgroup_mnt_path, "%s", cgroup_dir);
    sprintf(tst_cgroup_new_path, "%s/ltp_%d", cgroup_dir, rand());
}



> to define API as per-process and require same process to call mount and
> umount.
>
> > +     tst_cgroup_path->mnt_path =
> SAFE_MALLOC(strlen(tst_cgroup_mnt_path));
> > +     tst_cgroup_path->new_path =
> SAFE_MALLOC(strlen(tst_cgroup_new_path));
>
> Pointers are in shared memory, but content they point to is not, so it's
> accessible
> only from process that called tst_cgroup_set_path().
>

Good catch. This should be also use shared memory.


>
> Can you describe all different scenarios you wanted to support?
>

Sure, we have to consider many scenarios:

1. mount only one cgroup in a single process on the system support cgroup
v1 or v2 (i.e ksm03.c)

#include "tst_cgroup.h"
#define PATH_CGROUP  /tmp/cgroup

static void run(void)
{
    tst_cgroup_move_current(PATH_CGROUP);
    tst_cgroup_mem_set_maxbytes(PATH_CGROUP, 1024*1024*1024);
    // do your test
}

static void setup(void)
{
    tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_CGROUP);
}

static void cleanup(void)
{
    tst_cgroup_umount(PATH_CGROUP);
}

static struct tst_test test = {
    ...
    .test_all = run,
};


2. mount different cgroups in a single process on the system only support
cgroup-v1 (i.e ksm04.c, oom05.c)

#include "tst_cgroup.h"
#define PATH_CGROUP1  /tmp/cgroup1
#define PATH_CGROUP2  /tmp/cgroup2

static void run(void)
{
    tst_cgroup_move_current(PATH_CGROUP1);
    tst_cgroup_move_current(PATH_CGROUP2);

    // we have to recognize the correct cgroup path if we
    // mount two or more cgroup subsystems in a single
    // process, in case we get lost in knob setting

    // the tst_cgroup_get_path() helps to find and get
    // correct path in tst_cgroup_mnt_path, tst_cgroup_new_path

    // that's also the reason why we need a shared list to
    // store many cgoup pathes. And, this is extendible for
    // mounting more cgroup subsystems in the future.

    tst_cgroup_mem_set_maxbytes(PATH_CGROUP1, 1024*1024*1024);
    // some cpuset configration
}

static void setup(void)
{
    tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_CGROUP1);
    tst_cgroup_mount(TST_CGROUP_CPUSET, PATH_CGROUP2);
}

static void cleanup(void)
{
    tst_cgroup_umount(PATH_CGROUP1);
    tst_cgroup_umount(PATH_CGROUP2);
}

static struct tst_test test = {
    ...
    .test_all = run,
};


3. mount different cgroups in different process on the system support v1 or
v2

#include "tst_cgroup.h"
#define PATH_CGROUP1  /tmp/cgroup1
#define PATH_CGROUP2  /tmp/cgroup2

static void run(void)
{
    if (fork() == 0) {
        tst_cgroup_move_current(PATH_CGROUP1);
        tst_cgroup_mem_set_maxbytes(PATH_CGROUP1, 1024*1024*1024);
        // test code
    }

    tst_cgroup_move_current(PATH_CGROUP2);
    // some cpuset configuration
    // and test code
}

static void setup(void)
{
    // we do cgroup mount work unified in setup()
    // whatever the cgroup is being used in the parent
    // or children, there is also no need to care about the
    // details of cgroup v1 or v2.

    tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_CGROUP1);
    tst_cgroup_mount(TST_CGROUP_CPUSET, PATH_CGROUP2);
}

static void cleanup(void)
{
    tst_cgroup_umount(PATH_CGROUP1);
    tst_cgroup_umount(PATH_CGROUP2);
}

static struct tst_test test = {
    ...
    .test_all = run,
};


4. mount same cgroup in different process on the system support v1 or v2

#include "tst_cgroup.h"
#define PATH_CGROUP1  /tmp/cgroup1
#define PATH_CGROUP2  /tmp/cgroup2

static void run(void)
{
    if (fork() == 0) {
        tst_cgroup_move_current(PATH_CGROUP1);
        tst_cgroup_mem_set_maxbytes(PATH_CGROUP1, 1024*1024*1024);
        // test code1
    }

        tst_cgroup_move_current(PATH_CGROUP2);
        tst_cgroup_mem_set_maxbytes(PATH_CGROUP2, 2048*2048);
        // test code2
}

static void setup(void)
{
    // we mount two memory cgroup in the parent
    // but setting different value in corresponding
    // knob for different process to test more configration

    tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_CGROUP1);
    tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_CGROUP2);
}

static void cleanup(void)
{
    tst_cgroup_umount(PATH_CGROUP1);
    tst_cgroup_umount(PATH_CGROUP2);
}

static struct tst_test test = {
    ...
    .test_all = run,
};


5. mount many cgroups on different process for future extendible work
i.e.

// tst_cgroup_mount(TST_CGROUP_MEMCG, PATH_CGROUP1);
// tst_cgroup_mount(TST_CGROUP_CPUSET, PATH_CGROUP2);
// tst_cgroup_mount(TST_CGROUP_CPUACT, PATH_CGROUP3);
// tst_cgroup_mount(TST_CGROUP_PIDSCG,  PATH_CGROUP4);
// tst_cgroup_mount(TST_CGROUP_HUGTLB, PATH_CGROUP5);
// ...


Btw, I attach the patch v2.1 for better readable.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200602/9e622499/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lib-add-new-cgroup-test-API.patch
Type: text/x-patch
Size: 15933 bytes
Desc: not available
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200602/9e622499/attachment-0001.bin>

[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Jan Stancek]

Hi Li,

>Why we need this? Because, if a testcase(i.e oom05.c) needs more than one
>cgroup
>subsystem(memory, cpuset) on RHEL7(cgroup-v1), it should mount two
>different
>directories and do some knob setting.

Mounting with different controllers is fine, I meant do we have a case for mounting
same controller multiple times? We might have, because current design allows
only for single directory (tst_cgroup_new_path), that's automatically created on mount.
(This is your example 4)

>
>
>>
>> > +
>> > +static void tst_cgroup_set_path(const char *cgroup_dir)
>> > +{
>> > +     struct tst_cgroup_path *tst_cgroup_path, *a;
>> > +
>> > +     if (!cgroup_dir)
>> > +             tst_brk(TBROK, "Invalid cgroup dir, plese check
>> cgroup_dir");
>> > +
>> > +     sprintf(tst_cgroup_mnt_path, "%s", cgroup_dir);
>> > +     sprintf(tst_cgroup_new_path, "%s/ltp_%d", cgroup_dir, rand());
>> > +
>> > +     /* To store cgroup path in the shared 'path' list */
>> > +     tst_cgroup_path = SAFE_MMAP(NULL, (sizeof(struct tst_cgroup_path)),
>> > +                     PROT_READ | PROT_WRITE, MAP_ANONYMOUS |
>> MAP_SHARED, -1, 0);
>>
>> I'm not sure I understand what is the reason to have tst_cgroup_path. Is
>> it expected,
>> that mount and umount are called by different processes? It might be easier
>>
>
>The shared 'tst_cgroup_path' is necessary especially for mounting
>different cgoups in setup(). Otherwise, it would be easy to get lost
>which directory for kind of cgroup type.

But why is it shared? Is cleanup going to run in different process context?
Which one of your examples needs shared memory?

>
>And the worth to say, the random directory name for same cgroup
>mounting is also on purpose, though we mount same(i.e memory)
>cgroup in two places it still belongs to the one hierarchy, and create
>the same name of the new directory will be hit an error in EEXIST.
>
>static void tst_cgroup_set_path(const char *cgroup_dir)
>{
>    ...
>    sprintf(tst_cgroup_mnt_path, "%s", cgroup_dir);
>    sprintf(tst_cgroup_new_path, "%s/ltp_%d", cgroup_dir, rand());

I see why you are tracking this in list, but this exchange of state through
global variables does seem bit unclear.

Could we leave "new_path" creation to testcase itself? It would give
us more flexibility and we don't have to worry about name collisions.
It also avoids need to mount same controller multiple times
(example 4 in your reply).

Let's assume this is API:

#include "tst_cgroup.h"
#define MEM_MNT  "/tmp/cgroup1"
#define CPUSET_MNT  "/tmp/cgroup2"
#define DIR1 "ltp_test_blah_dir1"
#define DIR2 "ltp_test_blah_dir2"
#define DIR3 "ltp_test_blah_dir3"

static void run(void)
{
    if (fork() == 0) {
        tst_cgroup_move_current(MEM_MNT, DIR2);
        // do your test
        exit(0);
    }
    tst_cgroup_move_current(MEM_MNT, DIR1);
    // do your test
}

static void setup(void)
{
    tst_cgroup_mount(TST_CGROUP_MEMCG, MEM_MNT);
    tst_cgroup_mkdir(MEM_MNT, DIR1);
    tst_cgroup_mem_set_maxbytes(MEM_MNT, DIR1, 1*1024*1024);
    tst_cgroup_mkdir(MEM_MNT, DIR2);
    tst_cgroup_mem_set_maxbytes(MEM_MNT, DIR2, 2*1024*1024);
    
    tst_cgroup_mount(TST_CGROUP_CPUSET, CPUSET_MNT);
    tst_cgroup_mkdir(CPUSET_MNT, DIR3);
    tst_cgroup_move_current(CPUSET_MNT, DIR3);
}

static void cleanup(void)
{
    tst_cgroup_umount(MEM_MNT);
    tst_cgroup_umount(CPUSET_MNT);
}

static struct tst_test test = {
    ...
    .test_all = run,
};

On library side we would have a list that tracks all mounts. And every mount
record would have list that tracks all mkdir() operations, so we can
cleanup anything that test creates. I think tracking per-process would be sufficient.

(without spinning v3) What are your thoughts?

Regards,
Jan

[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Li Wang]

On Tue, Jun 2, 2020 at 8:12 PM Jan Stancek <jstancek@redhat.com> wrote:

> Hi Li,
>
> >Why we need this? Because, if a testcase(i.e oom05.c) needs more than one
> >cgroup
> >subsystem(memory, cpuset) on RHEL7(cgroup-v1), it should mount two
> >different
> >directories and do some knob setting.
>
> Mounting with different controllers is fine, I meant do we have a case for
> mounting
> same controller multiple times? We might have, because current design
> allows
> only for single directory (tst_cgroup_new_path), that's automatically
> created on mount.
> (This is your example 4)
>
> >
> >
> >>
> >> > +
> >> > +static void tst_cgroup_set_path(const char *cgroup_dir)
> >> > +{
> >> > +     struct tst_cgroup_path *tst_cgroup_path, *a;
> >> > +
> >> > +     if (!cgroup_dir)
> >> > +             tst_brk(TBROK, "Invalid cgroup dir, plese check
> >> cgroup_dir");
> >> > +
> >> > +     sprintf(tst_cgroup_mnt_path, "%s", cgroup_dir);
> >> > +     sprintf(tst_cgroup_new_path, "%s/ltp_%d", cgroup_dir, rand());
> >> > +
> >> > +     /* To store cgroup path in the shared 'path' list */
> >> > +     tst_cgroup_path = SAFE_MMAP(NULL, (sizeof(struct
> tst_cgroup_path)),
> >> > +                     PROT_READ | PROT_WRITE, MAP_ANONYMOUS |
> >> MAP_SHARED, -1, 0);
> >>
> >> I'm not sure I understand what is the reason to have tst_cgroup_path. Is
> >> it expected,
> >> that mount and umount are called by different processes? It might be
> easier
> >>
> >
> >The shared 'tst_cgroup_path' is necessary especially for mounting
> >different cgoups in setup(). Otherwise, it would be easy to get lost
> >which directory for kind of cgroup type.
>
> But why is it shared? Is cleanup going to run in different process context?
> Which one of your examples needs shared memory?
>

My original thought was that if children want to remove some paths
from the list, so this shared memory was made sense. But in the
signed-off patch v2, it seems we only do the deletion in cleanup(),
so private memory is enough.

Another slight reason to keep using share memory is to avoid the
children have a new copy of the list, that will allocate more memory
in testing, but that's fine, I have no objection to using private memory.


>
> >
> >And the worth to say, the random directory name for same cgroup
> >mounting is also on purpose, though we mount same(i.e memory)
> >cgroup in two places it still belongs to the one hierarchy, and create
> >the same name of the new directory will be hit an error in EEXIST.
> >
> >static void tst_cgroup_set_path(const char *cgroup_dir)
> >{
> >    ...
> >    sprintf(tst_cgroup_mnt_path, "%s", cgroup_dir);
> >    sprintf(tst_cgroup_new_path, "%s/ltp_%d", cgroup_dir, rand());
>
> I see why you are tracking this in list, but this exchange of state through
> global variables does seem bit unclear.
>

Yes, that's a compromise to support more usage of the APIs.


>
> Could we leave "new_path" creation to testcase itself? It would give
> us more flexibility and we don't have to worry about name collisions.
>

Hmm, more flexibility sometimes means more complicated to use, I don't
want these APIs to become difficult for newbies of LTP.


> It also avoids need to mount same controller multiple times
> (example 4 in your reply).
>

To be honest, most situations we have are examples 1 and 2, the
others(ex3,4,5) might rarely happen in our test. So maybe leave too
much work(i.e "new_path" creation) in testcase is not wise, and I don't
want to make usage complicated just for ingratiating the rare situations.


>
> Let's assume this is API:
>
> #include "tst_cgroup.h"
> #define MEM_MNT  "/tmp/cgroup1"
> #define CPUSET_MNT  "/tmp/cgroup2"
> #define DIR1 "ltp_test_blah_dir1"
> #define DIR2 "ltp_test_blah_dir2"
> #define DIR3 "ltp_test_blah_dir3"
>
> static void run(void)
> {
>     if (fork() == 0) {
>         tst_cgroup_move_current(MEM_MNT, DIR2);
>         // do your test
>         exit(0);
>     }
>     tst_cgroup_move_current(MEM_MNT, DIR1);
>     // do your test
> }
>
> static void setup(void)
> {
>     tst_cgroup_mount(TST_CGROUP_MEMCG, MEM_MNT);
>     tst_cgroup_mkdir(MEM_MNT, DIR1);
>     tst_cgroup_mem_set_maxbytes(MEM_MNT, DIR1, 1*1024*1024);
>     tst_cgroup_mkdir(MEM_MNT, DIR2);
>     tst_cgroup_mem_set_maxbytes(MEM_MNT, DIR2, 2*1024*1024);
>
>     tst_cgroup_mount(TST_CGROUP_CPUSET, CPUSET_MNT);
>     tst_cgroup_mkdir(CPUSET_MNT, DIR3);
>     tst_cgroup_move_current(CPUSET_MNT, DIR3);

}
>
> static void cleanup(void)
> {
>     tst_cgroup_umount(MEM_MNT);
>     tst_cgroup_umount(CPUSET_MNT);
> }
>
> static struct tst_test test = {
>     ...
>     .test_all = run,
> };
>
> On library side we would have a list that tracks all mounts. And every
> mount
> record would have list that tracks all mkdir() operations, so we can
> cleanup anything that test creates. I think tracking per-process would be
> sufficient.
>

To compare with my v2, this design leaves simple code in lib, and flexible
in usage. The only disadvantage is that people need to define more macros
and mkdir() by themself.

I have no strong preference to shift from v2 to this method, or maybe we
can try
to combine together and do more optimize work in the lib side.


>
> (without spinning v3) What are your thoughts?
>
> Regards,
> Jan
>
>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200603/83db11d8/attachment.htm>

[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Jan Stancek]

>> >And the worth to say, the random directory name for same cgroup
>> >mounting is also on purpose, though we mount same(i.e memory)
>> >cgroup in two places it still belongs to the one hierarchy, and create
>> >the same name of the new directory will be hit an error in EEXIST.
>> >
>> >static void tst_cgroup_set_path(const char *cgroup_dir)
>> >{
>> >    ...
>> >    sprintf(tst_cgroup_mnt_path, "%s", cgroup_dir);
>> >    sprintf(tst_cgroup_new_path, "%s/ltp_%d", cgroup_dir, rand());
>>
>> I see why you are tracking this in list, but this exchange of state through
>> global variables does seem bit unclear.
>>
>
>Yes, that's a compromise to support more usage of the APIs.

I don't get why global variables are necessary.
tst_cgroup_mnt_path is always same as cgroup_dir parameter passed to all functions.
tst_cgroup_get_path() could return pointer to tst_cgroup_pathes->new_path,
  (you just need you call umount before del_path)
tst_cgroup_ctl_knob is set before each use, so it could be local variable too.

>I have no strong preference to shift from v2 to this method, or maybe we
>can try
>to combine together and do more optimize work in the lib side.

Sounds like we could use 3rd opinion.

[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Li Wang]

On Wed, Jun 3, 2020 at 6:43 PM Jan Stancek <jstancek@redhat.com> wrote:

> >> >And the worth to say, the random directory name for same cgroup
> >> >mounting is also on purpose, though we mount same(i.e memory)
> >> >cgroup in two places it still belongs to the one hierarchy, and create
> >> >the same name of the new directory will be hit an error in EEXIST.
> >> >
> >> >static void tst_cgroup_set_path(const char *cgroup_dir)
> >> >{
> >> >    ...
> >> >    sprintf(tst_cgroup_mnt_path, "%s", cgroup_dir);
> >> >    sprintf(tst_cgroup_new_path, "%s/ltp_%d", cgroup_dir, rand());
> >>
> >> I see why you are tracking this in list, but this exchange of state
> through
> >> global variables does seem bit unclear.
> >>
> >
> >Yes, that's a compromise to support more usage of the APIs.
>
> I don't get why global variables are necessary.
>

The only reason to export them as global variables is to make the legacy
read/write_cpuse_files() happy. So that I said it is a compromise.

$ git grep tst_cgroup_new_path
cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "cpus",
buf);
cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
mems);
cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
buf);
cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
buf);
lib/mem.c:      write_cpuset_files(tst_cgroup_new_path, "mems", buf);
lib/mem.c:              write_cpuset_files(tst_cgroup_new_path, "cpus",
cpus);
lib/mem.c:              write_cpuset_files(tst_cgroup_new_path, "cpus",
"0");
oom/oom04.c:            write_cpuset_files(tst_cgroup_new_path,
"memory_migrate", "1");
oom/oom05.c:            write_cpuset_files(tst_cgroup_new_path,
"memory_migrate", "1");



> tst_cgroup_mnt_path is always same as cgroup_dir parameter passed to all
> functions.
> tst_cgroup_get_path() could return pointer to tst_cgroup_pathes->new_path,
>   (you just need you call umount before del_path)
> tst_cgroup_ctl_knob is set before each use, so it could be local variable
> too.
>

tst_cgroup_ctl_knob can be localized since no other places need it.


> >I have no strong preference to shift from v2 to this method, or maybe we
> >can try
> >to combine together and do more optimize work in the lib side.
>
> Sounds like we could use 3rd opinion.
>
>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200603/e64a2a62/attachment.htm>

[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Jan Stancek]

On Wed, Jun 03, 2020 at 08:51:37PM +0800, Li Wang wrote:
>> I don't get why global variables are necessary.
>>
>
>The only reason to export them as global variables is to make the legacy
>read/write_cpuse_files() happy. So that I said it is a compromise.
>
>$ git grep tst_cgroup_new_path
>cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "cpus",
>buf);
>cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
>mems);
>cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
>buf);
>cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
>buf);
>lib/mem.c:      write_cpuset_files(tst_cgroup_new_path, "mems", buf);
>lib/mem.c:              write_cpuset_files(tst_cgroup_new_path, "cpus",
>cpus);
>lib/mem.c:              write_cpuset_files(tst_cgroup_new_path, "cpus",
>"0");
>oom/oom04.c:            write_cpuset_files(tst_cgroup_new_path,
>"memory_migrate", "1");
>oom/oom05.c:            write_cpuset_files(tst_cgroup_new_path,
>"memory_migrate", "1");

What if we provided access to it via API? Would we still need these
global variables?

  char *tst_cgroup_get_path(const char *cgroup_mnt)
      // return ptr to tst_cgroup_paths->new_path

mount path is always known to test, because it passes it to tst_cgroup_mount(),
so it just needs to find out "new path".

Would that satisfy the need of this legacy test?

[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Li Wang]

On Fri, Jun 5, 2020 at 6:14 PM Jan Stancek <jstancek@redhat.com> wrote:

> On Wed, Jun 03, 2020 at 08:51:37PM +0800, Li Wang wrote:
> >> I don't get why global variables are necessary.
> >>
> >
> >The only reason to export them as global variables is to make the legacy
> >read/write_cpuse_files() happy. So that I said it is a compromise.
> >
> >$ git grep tst_cgroup_new_path
> >cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "cpus",
> >buf);
> >cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
> >mems);
> >cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
> >buf);
> >cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
> >buf);
> >lib/mem.c:      write_cpuset_files(tst_cgroup_new_path, "mems", buf);
> >lib/mem.c:              write_cpuset_files(tst_cgroup_new_path, "cpus",
> >cpus);
> >lib/mem.c:              write_cpuset_files(tst_cgroup_new_path, "cpus",
> >"0");
> >oom/oom04.c:            write_cpuset_files(tst_cgroup_new_path,
> >"memory_migrate", "1");
> >oom/oom05.c:            write_cpuset_files(tst_cgroup_new_path,
> >"memory_migrate", "1");
>
> What if we provided access to it via API? Would we still need these
> global variables?
>
>   char *tst_cgroup_get_path(const char *cgroup_mnt)
>       // return ptr to tst_cgroup_paths->new_path
>

The series of list operating function are hiding in the library. My thought
is
to make the list transparent to users.

In your method, we have to export the tst_cgroup_get_path() as an external
function, it stills needs an extra local pointer in testcase to store the
got new_path,
it doesn't seem tidier too.


> mount path is always known to test, because it passes it to
> tst_cgroup_mount(),
> so it just needs to find out "new path".
>
> Would that satisfy the need of this legacy test?


How about moving the cpuset legacy code to the library as part of APIs?
That'd
help to capsulate all the operation of cgroup control in lib, and people
just need
to invoke the related function as what he/she wants.

+void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char
*filename, char *buf);
+void tst_cgroup_cpuset_write_files(const char *cgroup_dir, const char
*filename, const char *buf);

Then 'tst_cgroup_new_path' searching work will all located internally. And
'tst_cgroup_ctl_knob' can
be local variable too.

Any other comments? (attach the v3.1)

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200608/dcd2e1ac/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lib-add-new-cgroup-test-API.patch
Type: text/x-patch
Size: 17454 bytes
Desc: not available
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200608/dcd2e1ac/attachment-0001.bin>

[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Jan Stancek]

----- Original Message -----
> On Fri, Jun 5, 2020 at 6:14 PM Jan Stancek <jstancek@redhat.com> wrote:
> 
> > On Wed, Jun 03, 2020 at 08:51:37PM +0800, Li Wang wrote:
> > >> I don't get why global variables are necessary.
> > >>
> > >
> > >The only reason to export them as global variables is to make the legacy
> > >read/write_cpuse_files() happy. So that I said it is a compromise.
> > >
> > >$ git grep tst_cgroup_new_path
> > >cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "cpus",
> > >buf);
> > >cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
> > >mems);
> > >cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
> > >buf);
> > >cpuset/cpuset01.c:      write_cpuset_files(tst_cgroup_new_path, "mems",
> > >buf);
> > >lib/mem.c:      write_cpuset_files(tst_cgroup_new_path, "mems", buf);
> > >lib/mem.c:              write_cpuset_files(tst_cgroup_new_path, "cpus",
> > >cpus);
> > >lib/mem.c:              write_cpuset_files(tst_cgroup_new_path, "cpus",
> > >"0");
> > >oom/oom04.c:            write_cpuset_files(tst_cgroup_new_path,
> > >"memory_migrate", "1");
> > >oom/oom05.c:            write_cpuset_files(tst_cgroup_new_path,
> > >"memory_migrate", "1");
> >
> > What if we provided access to it via API? Would we still need these
> > global variables?
> >
> >   char *tst_cgroup_get_path(const char *cgroup_mnt)
> >       // return ptr to tst_cgroup_paths->new_path
> >
> 
> The series of list operating function are hiding in the library. My thought
> is
> to make the list transparent to users.
> 
> In your method, we have to export the tst_cgroup_get_path() as an external
> function, it stills needs an extra local pointer in testcase to store the
> got new_path,
> it doesn't seem tidier too.

But there would be clear connection between function and variable.
  new_path = tst_cgroup_get_path(cgroup_dir);
vs.
  tst_cgroup_get_path(cgroup_dir);
  // fyi, tst_cgroup_new_path is updated as side-effect of call above
  // What other calls do update tst_cgroup_new_path? Have a look at implementation.

> 
> 
> > mount path is always known to test, because it passes it to
> > tst_cgroup_mount(),
> > so it just needs to find out "new path".
> >
> > Would that satisfy the need of this legacy test?
> 
> 
> How about moving the cpuset legacy code to the library as part of APIs?
> That'd
> help to capsulate all the operation of cgroup control in lib, and people
> just need
> to invoke the related function as what he/she wants.
> 
> +void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char
> *filename, char *buf);
> +void tst_cgroup_cpuset_write_files(const char *cgroup_dir, const char
> *filename, const char *buf);
> 
> Then 'tst_cgroup_new_path' searching work will all located internally. And
> 'tst_cgroup_ctl_knob' can
> be local variable too.
> 
> Any other comments? (attach the v3.1)

That makes it somewhat better, since it's only concern of library code now.
But since there are no tests using "tst_cgroup_new_path", does it still
need to be global variable?

[LTP] [PATCH v2 1/4] lib: add new cgroup test API

[Li Wang]

On Mon, Jun 8, 2020 at 5:48 PM Jan Stancek <jstancek@redhat.com> wrote:

> ...
>
> But there would be clear connection between function and variable.
>   new_path = tst_cgroup_get_path(cgroup_dir);
>

Yes, that's easier to understand the code workflow.

> vs.
>   tst_cgroup_get_path(cgroup_dir);
>   // fyi, tst_cgroup_new_path is updated as side-effect of call above
>   // What other calls do update tst_cgroup_new_path? Have a look at
> implementation.

...
>
> That makes it somewhat better, since it's only concern of library code now.
> But since there are no tests using "tst_cgroup_new_path", does it still
> need to be global variable?
>

Sure, we could delete tst_cgorup_mnt/new_path variables, since we have
tracking all mount paths in the list, so it will be easy to find new_path
in each
function.

Hope v4 is the final version:).

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200608/5ecc6f74/attachment.htm>