[LTP] [PATCH 0/6] Expand Cgroup shell library

[LTP] [PATCH 0/6] Expand Cgroup shell library

[Luke Nowakowski-Krijger]

This patchset aims to expand the cgroup_lib shell library to simplify
and centralize the whole mounting and cleanup process that, with all the
different versions and mounting schemes, can get confusing and rather
redundant.

So the aim here is to centralize all the functionality to the already
existing cgroup C API which handles a lot of the corner cases and
complexity while still getting to use it from a shell environment which
some developers probably prefer to write their tests in. This also
seems important to make it easier to reconfigure the current tests
to work under cgroup v2 controllers, as well as for developers to write
tests for cgroup v2 controllers for which there doesen't seem to be very
much coverage at the moment.

Luke Nowakowski-Krijger (6):
  API/cgroup: Modify tst_cgroup_print_config for easier parsing and
    consumption
  API/cgroup: Add cgroup_find_root helper function
  API/cgroup: Add option for specific pid to tst_cgroup_opts
  API/cgroup: Implement tst_cgroup_load_config()
  tools: Implement tst_cgctl binary utility
  controllers: Expand cgroup_lib shell library

 include/tst_cgroup.h                       |   7 +-
 lib/tst_cgroup.c                           | 165 ++++++++++++++++++++-
 testcases/kernel/controllers/cgroup_lib.sh | 129 ++++++++++++++--
 tools/cgroup/Makefile                      |   7 +
 tools/cgroup/tst_cgctl.c                   |  69 +++++++++
 5 files changed, 355 insertions(+), 22 deletions(-)
 create mode 100644 tools/cgroup/Makefile
 create mode 100644 tools/cgroup/tst_cgctl.c

-- 
2.32.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 1/6] API/cgroup: Modify tst_cgroup_print_config for easier parsing and consumption

[Luke Nowakowski-Krijger]

Prepare tst_cgroup_print_config to be more easily parsed and consumed by
shell scripts and other programs.

Also add any detected root information as well as the relevant state
associated with the roots that would be needed by the test to properly
cleanup.

Signed-off-by: Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com>
---
 lib/tst_cgroup.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index c08ff2f20..166d0f97e 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -300,12 +300,27 @@ opendir:
 				  O_PATH | O_DIRECTORY);
 }
 
+#define CONFIG_LTPDIR_KEY "Created_Ltp_Dir"
+#define CONFIG_MOUNTROOT_KEY "Mounted_Root"
+#define CONFIG_DRAINDIR_KEY "Created_Drain_Dir"
+#define CONFIG_TESTID_KEY "Test_Id"
+#define CONFIG_CTRL_REQUIRED "Required"
+#define CONFIG_CTRL_HEADER "Detected Controllers:"
+#define CONFIG_ROOT_HEADER "Detected Roots:"
+
+/* Prints out the minimal internal state of the test to be consumed
+ * by tst_cgroup_load_config().
+ *
+ * The config keeps track of the minimal state needed for tst_cgroup_cleanup
+ * to cleanup after a test and does not keep track of the creation of
+ * test cgroups that might be created through tst_cgroup_group_mk().
+ */
 void tst_cgroup_print_config(void)
 {
 	struct cgroup_root *root;
 	const struct cgroup_ctrl *ctrl;
 
-	tst_res(TINFO, "Detected Controllers:");
+	printf("%s\n", CONFIG_CTRL_HEADER);
 
 	for_each_ctrl(ctrl) {
 		root = ctrl->ctrl_root;
@@ -313,11 +328,26 @@ void tst_cgroup_print_config(void)
 		if (!root)
 			continue;
 
-		tst_res(TINFO, "\t%.10s %s @ %s:%s",
+		printf("%s %s @ %s %s\n",
 			ctrl->ctrl_name,
-			root->no_cpuset_prefix ? "[noprefix]" : "",
 			root->ver == TST_CGROUP_V1 ? "V1" : "V2",
-			root->mnt_path);
+			root->mnt_path,
+			ctrl->we_require_it ? CONFIG_CTRL_REQUIRED : "");
+	}
+
+	printf("%s\n", CONFIG_ROOT_HEADER);
+
+	for_each_root(root) {
+		printf("%s %s=%s %s=%s %s=%s %s=%s\n",
+			root->mnt_path,
+			CONFIG_MOUNTROOT_KEY,
+			root->we_mounted_it ? "yes" : "no",
+			CONFIG_LTPDIR_KEY,
+			root->ltp_dir.we_created_it ? "yes" : "no",
+			CONFIG_DRAINDIR_KEY,
+			root->drain_dir.we_created_it ? "yes" : "no",
+			CONFIG_TESTID_KEY,
+			root->test_dir.dir_name ? root->test_dir.dir_name : "NULL");
 	}
 }
 
-- 
2.32.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 2/6] API/cgroup: Add cgroup_find_root helper function

[Luke Nowakowski-Krijger]

Add a helper function similar to cgroup_find_ctrl to make matching paths
to roots more convenient.

Signed-off-by: Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com>
---
 lib/tst_cgroup.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 166d0f97e..b06ae6ab7 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -365,6 +365,19 @@ static struct cgroup_ctrl *cgroup_find_ctrl(const char *const ctrl_name)
 	return ctrl;
 }
 
+static struct cgroup_root *cgroup_find_root(const char *const mnt_path)
+{
+	struct cgroup_root *root = roots;
+
+	while (root->ver && strcmp(root->mnt_path, mnt_path))
+		root++;
+
+	if (!root->ver)
+		root = NULL;
+
+	return root;
+}
+
 /* Determine if a mounted cgroup hierarchy is unique and record it if so.
  *
  * For CGroups V2 this is very simple as there is only one
-- 
2.32.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 3/6] API/cgroup: Add option for specific pid to tst_cgroup_opts

[Luke Nowakowski-Krijger]

Add an option that would allow to create a test directory with a
specified pid, as opposed to the calling processes pid.

Signed-off-by: Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com>
---
 include/tst_cgroup.h | 3 +++
 lib/tst_cgroup.c     | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
index 632050e86..cfcc189ee 100644
--- a/include/tst_cgroup.h
+++ b/include/tst_cgroup.h
@@ -98,6 +98,9 @@ struct tst_cgroup_opts {
 	 * only indicates that we should mount V1 controllers if
 	 * nothing is present. By default we try to mount V2 first. */
 	int only_mount_v1:1;
+	/* Pass in a specific pid to create and identify the test
+	 * directory as opposed to the default pid of the calling process. */
+	int test_pid;
 };
 
 /* A Control Group in LTP's aggregated hierarchy */
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index b06ae6ab7..1cec3e722 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -713,7 +713,11 @@ mkdirs:
 
 	cgroup_dir_mk(&root->ltp_dir, cgroup_ltp_drain_dir, &root->drain_dir);
 
-	sprintf(cgroup_test_dir, "test-%d", getpid());
+	if (options->test_pid)
+		sprintf(cgroup_test_dir, "test-%d", options->test_pid);
+	else
+		sprintf(cgroup_test_dir, "test-%d", getpid());
+
 	cgroup_dir_mk(&root->ltp_dir, cgroup_test_dir, &root->test_dir);
 }
 
-- 
2.32.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 4/6] API/cgroup: Implement tst_cgroup_load_config()

[Luke Nowakowski-Krijger]

Implement tst_cgroup_load_config which consumes the state given by
tst_cgroup_print_config() to update the internal test state to reflect
the given config.

This allows for programs using the cgroup C API to load and reload
state, allowing functionality such as calling tst_cgroup_require and
tst_cgroup_cleanup to function properly between programs or between
invocations of a binary using the C API.

Signed-off-by: Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com>
---
 include/tst_cgroup.h |   4 +-
 lib/tst_cgroup.c     | 108 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
index cfcc189ee..9785bf7b9 100644
--- a/include/tst_cgroup.h
+++ b/include/tst_cgroup.h
@@ -112,7 +112,9 @@ struct tst_cgroup_group;
 void tst_cgroup_scan(void);
 /* Print the config detected by tst_cgroup_scan */
 void tst_cgroup_print_config(void);
-
+/* Load the config printed by tst_cgroup_print_config() to update the
+ * the internal state of the test to the given config */
+void tst_cgroup_load_config(const char *const config);
 /* Ensure the specified controller is available in the test's default
  * CGroup, mounting/enabling it if necessary */
 void tst_cgroup_require(const char *const ctrl_name,
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 1cec3e722..9b3c639fc 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -378,6 +378,114 @@ static struct cgroup_root *cgroup_find_root(const char *const mnt_path)
 	return root;
 }
 
+static int parse_ctrl_config(const char *const config_entry)
+{
+	char *token;
+	struct cgroup_ctrl *ctrl = NULL;
+	int we_require_it = 0;
+
+	if (!strcmp(CONFIG_CTRL_HEADER, config_entry))
+		return 0;
+
+	if (!strcmp(CONFIG_ROOT_HEADER, config_entry))
+		return 1;
+
+	for (token = strtok(config_entry, " "); token; token = strtok(NULL, " ")) {
+		if (!ctrl && !(ctrl = cgroup_find_ctrl(token)))
+			tst_brk(TBROK, "Could not find ctrl from config. Ctrls changing between calls?");
+
+		if (ctrl && !strcmp(token, CONFIG_CTRL_REQUIRED))
+			ctrl->we_require_it = 1;
+	}
+
+	return 0;
+}
+
+static int parse_root_config(char *config_entry)
+{
+	char *key;
+	char *value;
+	struct cgroup_root *root;
+
+	for (key = strtok(config_entry, " "); key; key = strtok(NULL, " ")) {
+		if (!(value = strchr(key, '='))) {
+			if (!(root = cgroup_find_root(key)))
+				tst_brk(TBROK, "Could not find root from config. Roots changing between calls?");
+
+			continue;
+		}
+
+		*value = '\0';
+		value = value + 1;
+
+		if (!strcmp(key, CONFIG_MOUNTROOT_KEY) && !strcmp(value, "yes")) {
+			root->we_mounted_it = 1;
+
+		} else if (!strcmp(key, CONFIG_LTPDIR_KEY) && !root->ltp_dir.dir_name) {
+			cgroup_dir_mk(&root->mnt_dir, cgroup_ltp_dir, &root->ltp_dir);
+			if (!strcmp(value, "yes"))
+				root->ltp_dir.we_created_it = 1;
+
+		} else if (!strcmp(key, CONFIG_DRAINDIR_KEY) && !root->drain_dir.dir_name) {
+			cgroup_dir_mk(&root->ltp_dir, cgroup_ltp_drain_dir, &root->drain_dir);
+			if (!strcmp(value, "yes"))
+				root->ltp_dir.we_created_it = 1;
+
+		} else if (!strcmp(key, CONFIG_TESTID_KEY) && strcmp(value, "NULL") &&
+				   !root->test_dir.dir_name) {
+			cgroup_dir_mk(&root->ltp_dir, value, &root->test_dir);
+			root->test_dir.we_created_it = 1;
+		}
+	}
+
+	return 0;
+}
+
+/* Load the test state configuration provided by tst_cgroup_print_config()
+ *
+ * This will reload some internal tst_cgroup state given by the config
+ * that might otherwise have been lost between calls or between different
+ * processes. In particular this is used by tools/cgroup/tst_cgctl to
+ * provide access to this C api to shell scripts.
+ *
+ * The config keeps track of the minimal state needed for tst_cgroup_cleanup
+ * to cleanup after a test and does not keep track of the creation of
+ * test cgroups that might be created through tst_cgroup_group_mk().
+ */
+void tst_cgroup_load_config(const char *const config)
+{
+	char temp_config[BUFSIZ];
+	char *curr_line;
+	char *next_line;
+
+	if (strlen(config) >= BUFSIZ)
+		tst_brk(TBROK, "Config has exceeded buffer size?");
+
+	strncpy(temp_config, config, BUFSIZ);
+
+	for (curr_line = &temp_config[0]; curr_line; curr_line = next_line + 1) {
+		next_line = strchr(curr_line, '\n');
+		if (next_line)
+			*next_line = '\0';
+
+		if (parse_ctrl_config(curr_line))
+			break;
+	}
+
+	for (curr_line = next_line + 1; curr_line; curr_line = next_line + 1) {
+		next_line = strchr(curr_line, '\n');
+		if (next_line)
+			*next_line = '\0';
+
+		parse_root_config(curr_line);
+		/* Bash will truncate the last newline that we are using to deliminate
+		 * the start and end of valid entries, so if we could not detect any more
+		 * newlines we assume that was our last entry. */
+		if (!next_line)
+			break;
+	}
+}
+
 /* Determine if a mounted cgroup hierarchy is unique and record it if so.
  *
  * For CGroups V2 this is very simple as there is only one
-- 
2.32.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 5/6] tools: Implement tst_cgctl binary utility

[Luke Nowakowski-Krijger]

Implement a binary utility that creates an interface to make calls to
the cgroup C API.

This will effectively allow shell scripts to make calls to the cgroup C
api.

Signed-off-by: Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com>
---
 tools/cgroup/Makefile    |  7 ++++
 tools/cgroup/tst_cgctl.c | 69 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)
 create mode 100644 tools/cgroup/Makefile
 create mode 100644 tools/cgroup/tst_cgctl.c

diff --git a/tools/cgroup/Makefile b/tools/cgroup/Makefile
new file mode 100644
index 000000000..81810bf4d
--- /dev/null
+++ b/tools/cgroup/Makefile
@@ -0,0 +1,7 @@
+top_srcdir		?= ../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+
+MAKE_TARGETS	:= tst_cgctl
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
\ No newline at end of file
diff --git a/tools/cgroup/tst_cgctl.c b/tools/cgroup/tst_cgctl.c
new file mode 100644
index 000000000..ef20e7485
--- /dev/null
+++ b/tools/cgroup/tst_cgctl.c
@@ -0,0 +1,69 @@
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <unistd.h>
+#include "tst_cgroup.h"
+
+static int cgctl_require(const char *ctrl, int test_pid)
+{
+    struct tst_cgroup_opts opts;
+
+    memset(&opts, 0, sizeof(opts));
+    opts.test_pid = test_pid;
+
+    tst_cgroup_require(ctrl, &opts);
+    tst_cgroup_print_config();
+
+    return 0;
+}
+
+static int cgctl_cleanup(const char *config)
+{
+    tst_cgroup_scan();
+    tst_cgroup_load_config(config);
+    tst_cgroup_cleanup();
+
+    return 0;
+}
+
+static int cgctl_print(void)
+{
+    tst_cgroup_scan();
+    tst_cgroup_print_config();
+
+    return 0;
+}
+
+static int cgctl_process_cmd(int argc, char *argv[])
+{
+    int test_pid;
+    const char *cmd_name = argv[1];
+
+    if (!strcmp(cmd_name, "require")) {
+        test_pid = atoi(argv[3]);
+        if (!test_pid) {
+            fprintf(stderr, "tst_cgctl: Invalid test_pid '%s' given\n",
+                    argv[3]);
+            return 1;
+        }
+        return cgctl_require(argv[2], test_pid);
+    } else if (!strcmp(cmd_name, "cleanup")) {
+        return cgctl_cleanup(argv[2]);
+    } else if (!strcmp(cmd_name, "print")) {
+        return cgctl_print();
+    }
+
+    fprintf(stderr, "tst_cgctl: Unknown command '%s' given\n", cmd_name);
+    return 1;
+}
+
+int main(int argc, char *argv[])
+{
+    if (argc < 2 || argc > 4) {
+        fprintf(stderr, "tst_cgctl: Invalid number of arguements given");
+        return 1;
+    }
+
+    return cgctl_process_cmd(argc, argv);
+}
\ No newline at end of file
-- 
2.32.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 6/6] controllers: Expand cgroup_lib shell library

[Luke Nowakowski-Krijger]

Expand the cgroup_lib library by using the tools/cgroup/tst_cgctl binary
utility to make calls to the Cgroup C API to simplify and centralize the
mounting and cleanup process of Cgroups.

Signed-off-by: Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com>
---
 testcases/kernel/controllers/cgroup_lib.sh | 129 ++++++++++++++++++---
 1 file changed, 113 insertions(+), 16 deletions(-)

diff --git a/testcases/kernel/controllers/cgroup_lib.sh b/testcases/kernel/controllers/cgroup_lib.sh
index 7918b5636..6ab201b95 100644
--- a/testcases/kernel/controllers/cgroup_lib.sh
+++ b/testcases/kernel/controllers/cgroup_lib.sh
@@ -5,22 +5,7 @@
 
 . tst_test.sh
 
-# Find mountpoint to given subsystem
-# get_cgroup_mountpoint SUBSYSTEM
-# RETURN: 0 if mountpoint found, otherwise 1
-get_cgroup_mountpoint()
-{
-	local subsystem=$1
-	local mntpoint
-
-	[ $# -eq 0 ] && tst_brk TBROK "get_cgroup_mountpoint: subsystem not defined"
-
-	mntpoint=$(grep cgroup /proc/mounts | grep -w $subsystem | awk '{ print $2 }')
-	[ -z "$mntpoint" ] && return 1
-
-	echo $mntpoint
-	return 0
-}
+_cgroup_state=
 
 # Check if given subsystem is supported and enabled
 # is_cgroup_subsystem_available_and_enabled SUBSYSTEM
@@ -37,3 +22,115 @@ is_cgroup_subsystem_available_and_enabled()
 
 	return 1
 }
+
+# Find mountpoint of the given controller
+# USAGE: cgroup_get_mountpoint CONTROLLER
+# RETURNS: Prints the mountpoint of the given controller
+# Must call cgroup_require before calling
+cgroup_get_mountpoint()
+{
+	local ctrl=$1
+	local mountpoint
+
+	[ $# -eq 0 ] && tst_brk TBROK "cgroup_get_mountpoint: controller not defined"
+	[ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_get_mountpoint: No previous state found. Forgot to call cgroup_require?"
+
+	mountpoint=$(echo "$_cgroup_state" | grep -w "$ctrl" | awk '{ print $4 }')
+	echo "$mountpoint"
+
+	return 0
+}
+
+# Get the test path of a given controller that has been created by the cgroup C API
+# USAGE: cgroup_get_test_path CONTROLLER
+# RETURNS: Prints the path to the test direcory
+# Must call cgroup_require before calling
+cgroup_get_test_path()
+{
+	local ctrl="$1"
+	local mountpoint
+	local test_path
+
+	[ $# -eq 0 ] && tst_brk TBROK "cgroup_get_test_path: controller not defined"
+	[ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_get_test_path: No previous state found. Forgot to call cgroup_require?"
+
+	mountpoint=$(cgroup_get_mountpoint "$ctrl")
+
+	test_path="$mountpoint/ltp/test-$$"
+
+	[ ! -d "$test_path" ] && tst_brk TBROK "cgroup_get_test_path: No test path found. Forgot to call cgroup_require?"
+
+	echo "$test_path"
+
+	return 0
+}
+
+# Gets the cgroup version of the given controller
+# USAGE: cgroup_get_version CONTROLLER
+# RETURNS: "V1" if version 1 and "V2" if version 2
+# Must call cgroup_require before calling
+cgroup_get_version()
+{
+	local ctrl="$1"
+
+	[ $# -eq 0 ] && tst_brk TBROK "cgroup_get_version: controller not defined"
+	[ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_get_version: No previous state found. Forgot to call cgroup_require?"
+
+	version=$(echo "$_cgroup_state" | grep -w "$ctrl" | awk '{ print $2 }')
+	[ "$version" = "" ] && tst_brk TBROK "cgroup_get_version: Could not find controller $ctrl"
+
+	echo "$version"
+
+	return 0
+}
+
+# Cleans up any setup done by calling cgroup_require.
+# USAGE: cgroup_cleanup
+# Can be safely called even when no setup has been done
+cgroup_cleanup()
+{
+	[ "$_cgroup_state" = "" ] && return 0
+
+	tst_cgctl cleanup "$_cgroup_state"
+
+	return 0
+}
+
+# Get the task list of the given controller
+# USAGE: cgroup_get_task_list CONTROLLER
+# RETURNS: prints out "cgroup.procs" if version 2 otherwise "tasks"
+# Must call cgroup_require before calling
+cgroup_get_task_list()
+{
+	local ctrl="$1"
+	local version
+
+	version=$(cgroup_get_version "$ctrl")
+
+	if [ "$version" = "V2" ]; then
+		echo "cgroup.procs"
+	else
+		echo "tasks"
+	fi
+
+	return 0
+}
+
+# Mounts and configures the given controller
+# USAGE: cgroup_require CONTROLLER
+cgroup_require()
+{
+	local ctrl="$1"
+
+	[ $# -eq 0 ] && tst_brk TBROK "cgroup_require: controller not defined"
+
+	if ! is_cgroup_subsystem_available_and_enabled "$ctrl"; then
+		tst_brk TBROK "cgroup_require: Controller not available or not enabled"
+	fi
+
+	_cgroup_state=$(tst_cgctl require "$ctrl" $$)
+
+	[ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_require: No state was set after call. Controller '$ctrl' maybe does not exist?"
+
+	return 0
+}
-- 
2.32.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 6/6] controllers: Expand cgroup_lib shell library

[Li Wang]

On Wed, Jan 5, 2022 at 6:01 PM Luke Nowakowski-Krijger
<luke.nowakowskikrijger@canonical.com> wrote:
>
> Expand the cgroup_lib library by using the tools/cgroup/tst_cgctl binary
> utility to make calls to the Cgroup C API to simplify and centralize the
> mounting and cleanup process of Cgroups.
>
> Signed-off-by: Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com>
> ---
>  testcases/kernel/controllers/cgroup_lib.sh | 129 ++++++++++++++++++---
>  1 file changed, 113 insertions(+), 16 deletions(-)
>
> diff --git a/testcases/kernel/controllers/cgroup_lib.sh b/testcases/kernel/controllers/cgroup_lib.sh
> index 7918b5636..6ab201b95 100644
> --- a/testcases/kernel/controllers/cgroup_lib.sh
> +++ b/testcases/kernel/controllers/cgroup_lib.sh
> @@ -5,22 +5,7 @@
>
>  . tst_test.sh
>
> -# Find mountpoint to given subsystem
> -# get_cgroup_mountpoint SUBSYSTEM
> -# RETURN: 0 if mountpoint found, otherwise 1
> -get_cgroup_mountpoint()

As we have renamed this function to cgroup_get_mountpoint(), so the
invoke part in test cases should be updated too. Maybe you're going
to complete that in following up patches, but as a completed patch set,
we'd better keep the code not broken in running.

cgroup/cgroup_regression_test.sh:
cpu_subsys_path=$(get_cgroup_mountpoint "cpu")
cgroup/cgroup_regression_test.sh:       local
subsys_path=$(get_cgroup_mountpoint $subsys)
cpuset/cpuset_regression_test.sh:
root_cpuset_dir=$(get_cgroup_mountpoint cpuset)

Btw, it would be great you can write simple test demos to verify the
new shell API works well.
Just like what people done at: ltp/lib/newlib_tests/shell/*

> -{
> -       local subsystem=$1
> -       local mntpoint
> -
> -       [ $# -eq 0 ] && tst_brk TBROK "get_cgroup_mountpoint: subsystem not defined"
> -
> -       mntpoint=$(grep cgroup /proc/mounts | grep -w $subsystem | awk '{ print $2 }')
> -       [ -z "$mntpoint" ] && return 1
> -
> -       echo $mntpoint
> -       return 0
> -}
> +_cgroup_state=
>
>  # Check if given subsystem is supported and enabled
>  # is_cgroup_subsystem_available_and_enabled SUBSYSTEM
> @@ -37,3 +22,115 @@ is_cgroup_subsystem_available_and_enabled()
>
>         return 1
>  }
> +
> +# Find mountpoint of the given controller
> +# USAGE: cgroup_get_mountpoint CONTROLLER
> +# RETURNS: Prints the mountpoint of the given controller
> +# Must call cgroup_require before calling
> +cgroup_get_mountpoint()
> +{
> +       local ctrl=$1
> +       local mountpoint
> +
> +       [ $# -eq 0 ] && tst_brk TBROK "cgroup_get_mountpoint: controller not defined"
> +       [ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_get_mountpoint: No previous state found. Forgot to call cgroup_require?"
> +
> +       mountpoint=$(echo "$_cgroup_state" | grep -w "$ctrl" | awk '{ print $4 }')
> +       echo "$mountpoint"
> +
> +       return 0
> +}
> +
> +# Get the test path of a given controller that has been created by the cgroup C API
> +# USAGE: cgroup_get_test_path CONTROLLER
> +# RETURNS: Prints the path to the test direcory
> +# Must call cgroup_require before calling
> +cgroup_get_test_path()
> +{
> +       local ctrl="$1"
> +       local mountpoint
> +       local test_path
> +
> +       [ $# -eq 0 ] && tst_brk TBROK "cgroup_get_test_path: controller not defined"
> +       [ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_get_test_path: No previous state found. Forgot to call cgroup_require?"
> +
> +       mountpoint=$(cgroup_get_mountpoint "$ctrl")
> +
> +       test_path="$mountpoint/ltp/test-$$"
> +
> +       [ ! -d "$test_path" ] && tst_brk TBROK "cgroup_get_test_path: No test path found. Forgot to call cgroup_require?"
> +
> +       echo "$test_path"
> +
> +       return 0
> +}
> +
> +# Gets the cgroup version of the given controller
> +# USAGE: cgroup_get_version CONTROLLER
> +# RETURNS: "V1" if version 1 and "V2" if version 2
> +# Must call cgroup_require before calling
> +cgroup_get_version()
> +{
> +       local ctrl="$1"
> +
> +       [ $# -eq 0 ] && tst_brk TBROK "cgroup_get_version: controller not defined"
> +       [ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_get_version: No previous state found. Forgot to call cgroup_require?"
> +
> +       version=$(echo "$_cgroup_state" | grep -w "$ctrl" | awk '{ print $2 }')
> +       [ "$version" = "" ] && tst_brk TBROK "cgroup_get_version: Could not find controller $ctrl"
> +
> +       echo "$version"
> +
> +       return 0
> +}
> +
> +# Cleans up any setup done by calling cgroup_require.
> +# USAGE: cgroup_cleanup
> +# Can be safely called even when no setup has been done
> +cgroup_cleanup()
> +{
> +       [ "$_cgroup_state" = "" ] && return 0
?> +
> +       tst_cgctl cleanup "$_cgroup_state"

I don't understand what exactly the $_cgroup_state do here,
isn't that just 0 or 1 which returned by 'tst_cgctl require "$ctrl" $$' ?
Maybe you can add brief comments on this variable.

> +
> +       return 0
> +}
> +
> +# Get the task list of the given controller
> +# USAGE: cgroup_get_task_list CONTROLLER
> +# RETURNS: prints out "cgroup.procs" if version 2 otherwise "tasks"
> +# Must call cgroup_require before calling
> +cgroup_get_task_list()
> +{
> +       local ctrl="$1"
> +       local version
> +
> +       version=$(cgroup_get_version "$ctrl")
> +
> +       if [ "$version" = "V2" ]; then
> +               echo "cgroup.procs"
> +       else
> +               echo "tasks"
> +       fi
> +
> +       return 0
> +}
> +
> +# Mounts and configures the given controller
> +# USAGE: cgroup_require CONTROLLER
> +cgroup_require()
> +{
> +       local ctrl="$1"
> +
> +       [ $# -eq 0 ] && tst_brk TBROK "cgroup_require: controller not defined"

We have already do parameter "$1" checking in
is_cgroup_subsystem_avalibale_and_enbale,
so here looks reductant.

And, I think we don't need to invoke that independent function at any
other place
if we put that in this cgroup_require() function. It might be good to delete
is_cgroup_subsystem_avalibale_and_enbale and move the process to here.

> +
> +       if ! is_cgroup_subsystem_available_and_enabled "$ctrl"; then
> +               tst_brk TBROK "cgroup_require: Controller not available or not enabled"

Maybe using TCONF here is more elegent.

> +       fi
> +
> +       _cgroup_state=$(tst_cgctl require "$ctrl" $$)
> +
> +       [ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_require: No state was set after call. Controller '$ctrl' maybe does not exist?"
> +
> +       return 0
> +}
> --
> 2.32.0
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>


-- 
Regards,
Li Wang


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 6/6] controllers: Expand cgroup_lib shell library

[Li Wang]

> > +cgroup_cleanup()
> > +{
> > +       [ "$_cgroup_state" = "" ] && return 0
> ?> +
> > +       tst_cgctl cleanup "$_cgroup_state"
>
> I don't understand what exactly the $_cgroup_state do here,
> isn't that just 0 or 1 which returned by 'tst_cgctl require "$ctrl" $$' ?

Ah, I got it, that comes from a string printed by tst_cgroup_print_config.
(Sorry I was read these codes from top to down.)

-- 
Regards,
Li Wang


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 6/6] controllers: Expand cgroup_lib shell library

[Li Wang]

> +# Find mountpoint of the given controller
> +# USAGE: cgroup_get_mountpoint CONTROLLER
> +# RETURNS: Prints the mountpoint of the given controller
> +# Must call cgroup_require before calling
> +cgroup_get_mountpoint()
> +{
> +       local ctrl=$1
> +       local mountpoint
> +
> +       [ $# -eq 0 ] && tst_brk TBROK "cgroup_get_mountpoint: controller not defined"
> +       [ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_get_mountpoint: No previous state found. Forgot to call cgroup_require?"
> +
> +       mountpoint=$(echo "$_cgroup_state" | grep -w "$ctrl" | awk '{ print $4 }')
> +       echo "$mountpoint"
> +
> +       return 0
> +}
> +
> +# Get the test path of a given controller that has been created by the cgroup C API
> +# USAGE: cgroup_get_test_path CONTROLLER
> +# RETURNS: Prints the path to the test direcory
> +# Must call cgroup_require before calling
> +cgroup_get_test_path()
> +{
> +       local ctrl="$1"
> +       local mountpoint
> +       local test_path
> +
> +       [ $# -eq 0 ] && tst_brk TBROK "cgroup_get_test_path: controller not defined"
> +       [ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_get_test_path: No previous state found. Forgot to call cgroup_require?"
> +
> +       mountpoint=$(cgroup_get_mountpoint "$ctrl")
> +
> +       test_path="$mountpoint/ltp/test-$$"
> +
> +       [ ! -d "$test_path" ] && tst_brk TBROK "cgroup_get_test_path: No test path found. Forgot to call cgroup_require?"
> +
> +       echo "$test_path"
> +
> +       return 0
> +}
> +
> +# Gets the cgroup version of the given controller
> +# USAGE: cgroup_get_version CONTROLLER
> +# RETURNS: "V1" if version 1 and "V2" if version 2
> +# Must call cgroup_require before calling
> +cgroup_get_version()
> +{
> +       local ctrl="$1"
> +
> +       [ $# -eq 0 ] && tst_brk TBROK "cgroup_get_version: controller not defined"
> +       [ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_get_version: No previous state found. Forgot to call cgroup_require?"
> +
> +       version=$(echo "$_cgroup_state" | grep -w "$ctrl" | awk '{ print $2 }')

This won't work on my x86_64 KVM platform, maybe we need: grep -w "^$ctrl".

# echo $$
1801

# ./tst_cgctl require memory 1801
Detected Controllers:
memory V1 @ /sys/fs/cgroup/memory Required
cpu V1 @ /sys/fs/cgroup/cpu,cpuacct
cpuset V1 @ /sys/fs/cgroup/cpuset
Detected Roots:
/sys/fs/cgroup/memory Mounted_Root=no Created_Ltp_Dir=no
Created_Drain_Dir=no Test_Id=test-1801
/sys/fs/cgroup/cpu,cpuacct Mounted_Root=no Created_Ltp_Dir=no
Created_Drain_Dir=no Test_Id=NULL
/sys/fs/cgroup/cpuset Mounted_Root=no Created_Ltp_Dir=no
Created_Drain_Dir=no Test_Id=NULL

# _cgroup_state=$(./tst_cgctl require memory 1801)

# echo "$_cgroup_state" | grep -w "memory" | awk '{ print $2 }'
V1
Mounted_Root=no

# echo "$_cgroup_state" | grep -w "memory" | awk '{ print $4 }'
/sys/fs/cgroup/memory
Created_Drain_Dir=no

# ./tst_cgctl cleanup "$_cgroup_state"
tst_cgroup.c:414: TBROK: Could not find root from config. Roots
changing between calls?


> +       [ "$version" = "" ] && tst_brk TBROK "cgroup_get_version: Could not find controller $ctrl"
> +
> +       echo "$version"
> +
> +       return 0
> +}
> +


-- 
Regards,
Li Wang


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 6/6] controllers: Expand cgroup_lib shell library

[Luke Nowakowski-Krijger]

[-- Attachment #1.1: Type: text/plain, Size: 3708 bytes --]

Hi Li,

On Tue, Jan 11, 2022 at 2:51 AM Li Wang <liwang@redhat.com> wrote:

> > +# Find mountpoint of the given controller
> > +# USAGE: cgroup_get_mountpoint CONTROLLER
> > +# RETURNS: Prints the mountpoint of the given controller
> > +# Must call cgroup_require before calling
> > +cgroup_get_mountpoint()
> > +{
> > +       local ctrl=$1
> > +       local mountpoint
> > +
> > +       [ $# -eq 0 ] && tst_brk TBROK "cgroup_get_mountpoint: controller
> not defined"
> > +       [ "$_cgroup_state" = "" ] && tst_brk TBROK
> "cgroup_get_mountpoint: No previous state found. Forgot to call
> cgroup_require?"
> > +
> > +       mountpoint=$(echo "$_cgroup_state" | grep -w "$ctrl" | awk '{
> print $4 }')
> > +       echo "$mountpoint"
> > +
> > +       return 0
> > +}
> > +
> > +# Get the test path of a given controller that has been created by the
> cgroup C API
> > +# USAGE: cgroup_get_test_path CONTROLLER
> > +# RETURNS: Prints the path to the test direcory
> > +# Must call cgroup_require before calling
> > +cgroup_get_test_path()
> > +{
> > +       local ctrl="$1"
> > +       local mountpoint
> > +       local test_path
> > +
> > +       [ $# -eq 0 ] && tst_brk TBROK "cgroup_get_test_path: controller
> not defined"
> > +       [ "$_cgroup_state" = "" ] && tst_brk TBROK
> "cgroup_get_test_path: No previous state found. Forgot to call
> cgroup_require?"
> > +
> > +       mountpoint=$(cgroup_get_mountpoint "$ctrl")
> > +
> > +       test_path="$mountpoint/ltp/test-$$"
> > +
> > +       [ ! -d "$test_path" ] && tst_brk TBROK "cgroup_get_test_path: No
> test path found. Forgot to call cgroup_require?"
> > +
> > +       echo "$test_path"
> > +
> > +       return 0
> > +}
> > +
> > +# Gets the cgroup version of the given controller
> > +# USAGE: cgroup_get_version CONTROLLER
> > +# RETURNS: "V1" if version 1 and "V2" if version 2
> > +# Must call cgroup_require before calling
> > +cgroup_get_version()
> > +{
> > +       local ctrl="$1"
> > +
> > +       [ $# -eq 0 ] && tst_brk TBROK "cgroup_get_version: controller
> not defined"
> > +       [ "$_cgroup_state" = "" ] && tst_brk TBROK "cgroup_get_version:
> No previous state found. Forgot to call cgroup_require?"
> > +
> > +       version=$(echo "$_cgroup_state" | grep -w "$ctrl" | awk '{ print
> $2 }')
>
> This won't work on my x86_64 KVM platform, maybe we need: grep -w "^$ctrl".
>
> # echo $$
> 1801
>
> # ./tst_cgctl require memory 1801
> Detected Controllers:
> memory V1 @ /sys/fs/cgroup/memory Required
> cpu V1 @ /sys/fs/cgroup/cpu,cpuacct
> cpuset V1 @ /sys/fs/cgroup/cpuset
> Detected Roots:
> /sys/fs/cgroup/memory Mounted_Root=no Created_Ltp_Dir=no
> Created_Drain_Dir=no Test_Id=test-1801
> /sys/fs/cgroup/cpu,cpuacct Mounted_Root=no Created_Ltp_Dir=no
> Created_Drain_Dir=no Test_Id=NULL
> /sys/fs/cgroup/cpuset Mounted_Root=no Created_Ltp_Dir=no
> Created_Drain_Dir=no Test_Id=NULL
>
> # _cgroup_state=$(./tst_cgctl require memory 1801)
>
> # echo "$_cgroup_state" | grep -w "memory" | awk '{ print $2 }'
> V1
> Mounted_Root=no
>
> # echo "$_cgroup_state" | grep -w "memory" | awk '{ print $4 }'
> /sys/fs/cgroup/memory
> Created_Drain_Dir=no
>
> # ./tst_cgctl cleanup "$_cgroup_state"
> tst_cgroup.c:414: TBROK: Could not find root from config. Roots
> changing between calls?
>
>
> > +       [ "$version" = "" ] && tst_brk TBROK "cgroup_get_version: Could
> not find controller $ctrl"
> > +
> > +       echo "$version"
> > +
> > +       return 0
> > +}
> > +
>
>
Ah I see that the grepping goes awry when there is already a V1 controller
which has the controller name in the mount path. Thank you for checking
this, I will fix this in the next version.


> --
> Regards,
> Li Wang
>
>
Thanks for the review,
- Luke

[-- Attachment #1.2: Type: text/html, Size: 5237 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 5/6] tools: Implement tst_cgctl binary utility

[Li Wang]

Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com> wrote:

>  tools/cgroup/Makefile    |  7 ++++
>  tools/cgroup/tst_cgctl.c | 69 ++++++++++++++++++++++++++++++++++++++++

Looks like putting this tst_cgctl.c in testcase/lib/ will be better,
we have no necessary to create it under a bit far directory, and
that tool/ is more generic for LTP, but this process is only for
shell tests.

> +static int cgctl_cleanup(const char *config)
> +{
> +    tst_cgroup_scan();
> +    tst_cgroup_load_config(config);

This tst_cgroup_load_config() does not work as expected.
From my manual check, the ltp and drain dir have been created
but it prints " Created_Ltp_Dir=no Created_Drain_Dir=no" strings.

...
Detected Roots:
/sys/fs/cgroup/memory Mounted_Root=no Created_Ltp_Dir=no
Created_Drain_Dir=no Test_Id=test-1801
...


> +    tst_cgroup_cleanup();

This does not work as expected too, but the problem should exist
in previous patches. Anyway, I will look into the details tomorrow.

# ./tst_cgctl cleanup "$_cgroup_state"
tst_cgroup.c:414: TBROK: Could not find root from config. Roots
changing between calls?


> +
> +    return 0;
> +}


--
Regards,
Li Wang


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 5/6] tools: Implement tst_cgctl binary utility

[Luke Nowakowski-Krijger]

[-- Attachment #1.1: Type: text/plain, Size: 1862 bytes --]

Hi Li,

On Tue, Jan 11, 2022 at 4:43 AM Li Wang <liwang@redhat.com> wrote:

> Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com> wrote:
>
> >  tools/cgroup/Makefile    |  7 ++++
> >  tools/cgroup/tst_cgctl.c | 69 ++++++++++++++++++++++++++++++++++++++++
>
> Looks like putting this tst_cgctl.c in testcase/lib/ will be better,
> we have no necessary to create it under a bit far directory, and
> that tool/ is more generic for LTP, but this process is only for
> shell tests.
>
>
Yeah I will move it over here. I was not sure exactly where it should go.

> +static int cgctl_cleanup(const char *config)
> > +{
> > +    tst_cgroup_scan();
> > +    tst_cgroup_load_config(config);
>
> This tst_cgroup_load_config() does not work as expected.
> From my manual check, the ltp and drain dir have been created
> but it prints " Created_Ltp_Dir=no Created_Drain_Dir=no" strings.
>
> ...
> Detected Roots:
> /sys/fs/cgroup/memory Mounted_Root=no Created_Ltp_Dir=no
> Created_Drain_Dir=no Test_Id=test-1801
> ...
>
>
>
Hm, I'm not sure what this could be. Are you sure when you were reading the
printed info it was in the same invocation as when it was being created?
Because the tst_cgroup_print_config is just pretty directly printing out
the state of the cgroup framework.

> +    tst_cgroup_cleanup();
>
> This does not work as expected too, but the problem should exist
> in previous patches. Anyway, I will look into the details tomorrow.
>
> # ./tst_cgctl cleanup "$_cgroup_state"
> tst_cgroup.c:414: TBROK: Could not find root from config. Roots
> changing between calls?
>
>
Was "/sys/fs/cgroup/cpu,cpuacct" one of the roots that was printed out?
Because if so the way I have it now it would not be able to parse that. I
will look into fixing this.


> > +
> > +    return 0;
> > +}
>
>
> --
> Regards,
> Li Wang
>
>
Thanks again for review,

- Luke

[-- Attachment #1.2: Type: text/html, Size: 3036 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 5/6] tools: Implement tst_cgctl binary utility

[Li Wang]

Hi Luke,

>> > +static int cgctl_cleanup(const char *config)
>> > +{
>> > +    tst_cgroup_scan();
>> > +    tst_cgroup_load_config(config);
>>
>> This tst_cgroup_load_config() does not work as expected.
>> From my manual check, the ltp and drain dir have been created
>> but it prints " Created_Ltp_Dir=no Created_Drain_Dir=no" strings.
>>
>> ...
>> Detected Roots:
>> /sys/fs/cgroup/memory Mounted_Root=no Created_Ltp_Dir=no
>> Created_Drain_Dir=no Test_Id=test-1801
>> ...
>>
>>
>
> Hm, I'm not sure what this could be. Are you sure when you were reading the printed info it was in the same invocation as when it was being created? Because the tst_cgroup_print_config is just pretty directly printing out the state of the cgroup framework.

I have got the reason, because I manually run Cgroup test but did
not allow it to do cleanup so leaves the ltp/ and drain/ dir there, so
it will not be recorded as "yes" in Created_Ltp_dir next time, that
behavior is correct.

>
>> > +    tst_cgroup_cleanup();
>>
>> This does not work as expected too, but the problem should exist
>> in previous patches. Anyway, I will look into the details tomorrow.
>>
>> # ./tst_cgctl cleanup "$_cgroup_state"
>> tst_cgroup.c:414: TBROK: Could not find root from config. Roots
>> changing between calls?
>>
>
> Was "/sys/fs/cgroup/cpu,cpuacct" one of the roots that was printed out? Because if so the way I have it now it would not be able to parse that. I will look into fixing this.

Yes, it was, I just cut the message to show the problem.


Detected Controllers:
memory V1 @ /sys/fs/cgroup/memory Required
cpu V1 @ /sys/fs/cgroup/cpu,cpuacct
cpuset V1 @ /sys/fs/cgroup/cpuset
Detected Roots:
/sys/fs/cgroup/memory Mounted_Root=no Created_Ltp_Dir=no
Created_Drain_Dir=no Test_Id=test-5091
/sys/fs/cgroup/cpu,cpuacct Mounted_Root=no Created_Ltp_Dir=no
Created_Drain_Dir=no Test_Id=NULL
/sys/fs/cgroup/cpuset Mounted_Root=no Created_Ltp_Dir=no
Created_Drain_Dir=no Test_Id=NULL


From my test, the progress works well on Cgroup V2,
but now fails on parsing the root config in V1.
I suspect there is a bug in patch 4/6, but still checking
why the "roots" is NULL at that moment.

-- 
Regards,
Li Wang


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/6] API/cgroup: Add cgroup_find_root helper function

[Li Wang]

On Wed, Jan 5, 2022 at 6:00 PM Luke Nowakowski-Krijger
<luke.nowakowskikrijger@canonical.com> wrote:
>
> Add a helper function similar to cgroup_find_ctrl to make matching paths
> to roots more convenient.
>
> Signed-off-by: Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com>
> ---
>  lib/tst_cgroup.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
> index 166d0f97e..b06ae6ab7 100644
> --- a/lib/tst_cgroup.c
> +++ b/lib/tst_cgroup.c
> @@ -365,6 +365,19 @@ static struct cgroup_ctrl *cgroup_find_ctrl(const char *const ctrl_name)
>         return ctrl;
>  }
>
> +static struct cgroup_root *cgroup_find_root(const char *const mnt_path)
> +{
> +       struct cgroup_root *root = roots;
> +
> +       while (root->ver && strcmp(root->mnt_path, mnt_path))
> +               root++;
> +
> +       if (!root->ver)
> +               root = NULL;
> +
> +       return root;
> +}

I suggest using for_each_root() macro here, otherwise as I commented
it can't parse root in V1.

static struct cgroup_root *cgroup_find_root(const char *const mnt_path)
{
        struct cgroup_root *root;

        for_each_root(root) {
                if (!strcmp(root->mnt_path, mnt_path))
                        return root;
        }

        return NULL;
}



> +
>  /* Determine if a mounted cgroup hierarchy is unique and record it if so.
>   *
>   * For CGroups V2 this is very simple as there is only one
> --
> 2.32.0
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>


-- 
Regards,
Li Wang


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/6] API/cgroup: Implement tst_cgroup_load_config()

[Li Wang]

 Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com> wrote:

> +static int parse_root_config(char *config_entry)
> +{
> +       char *key;
> +       char *value;
> +       struct cgroup_root *root;
> +
> +       for (key = strtok(config_entry, " "); key; key = strtok(NULL, " ")) {
> +               if (!(value = strchr(key, '='))) {
> +                       if (!(root = cgroup_find_root(key)))
> +                               tst_brk(TBROK, "Could not find root from config. Roots changing between calls?");
> +
> +                       continue;
> +               }
> +
> +               *value = '\0';
> +               value = value + 1;
> +
> +               if (!strcmp(key, CONFIG_MOUNTROOT_KEY) && !strcmp(value, "yes")) {
> +                       root->we_mounted_it = 1;
> +

> +               } else if (!strcmp(key, CONFIG_LTPDIR_KEY) && !root->ltp_dir.dir_name) {
> +                       cgroup_dir_mk(&root->mnt_dir, cgroup_ltp_dir, &root->ltp_dir);
> +                       if (!strcmp(value, "yes"))
> +                               root->ltp_dir.we_created_it = 1;
> +
> +               } else if (!strcmp(key, CONFIG_DRAINDIR_KEY) && !root->drain_dir.dir_name) {
> +                       cgroup_dir_mk(&root->ltp_dir, cgroup_ltp_drain_dir, &root->drain_dir);
> +                       if (!strcmp(value, "yes"))
> +                               root->ltp_dir.we_created_it = 1;

I think that parsing the  CONFIG_DRAINDIR_KEY from '$_cgroup_state'
is superfluous. Because from the tst_cgroup_cleanup, if
root->ltp_dir.we_created_it
is 1 then both of the two directories will be removed, so just using
CONFIG_LTPDIR_KE
to track the status is enough.

And maybe it is not necessary to print "Created_Drain_Dir=xx" in
tst_cgroup_print_config at all.

Then, the code snippet could be as:

                } else if (!strcmp(key, CONFIG_LTPDIR_KEY) &&
!root->ltp_dir.dir_name) {
                        cgroup_dir_mk(&root->mnt_dir, cgroup_ltp_dir,
&root->ltp_dir);
                        cgroup_dir_mk(&root->ltp_dir,
cgroup_ltp_drain_dir, &root->drain_dir);
                        if (!strcmp(value, "yes"))
                                root->ltp_dir.we_created_it = 1;


> +
> +               } else if (!strcmp(key, CONFIG_TESTID_KEY) && strcmp(value, "NULL") &&
> +                                  !root->test_dir.dir_name) {
> +                       cgroup_dir_mk(&root->ltp_dir, value, &root->test_dir);
> +                       root->test_dir.we_created_it = 1;
> +               }
> +       }
> +
> +       return 0;
> +}




--
Regards,
Li Wang


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/6] API/cgroup: Implement tst_cgroup_load_config()

[Luke Nowakowski-Krijger]

[-- Attachment #1.1: Type: text/plain, Size: 3113 bytes --]

Hi Li,

On Thu, Jan 13, 2022 at 10:57 PM Li Wang <liwang@redhat.com> wrote:

>  Luke Nowakowski-Krijger <luke.nowakowskikrijger@canonical.com> wrote:
>
> > +static int parse_root_config(char *config_entry)
> > +{
> > +       char *key;
> > +       char *value;
> > +       struct cgroup_root *root;
> > +
> > +       for (key = strtok(config_entry, " "); key; key = strtok(NULL, "
> ")) {
> > +               if (!(value = strchr(key, '='))) {
> > +                       if (!(root = cgroup_find_root(key)))
> > +                               tst_brk(TBROK, "Could not find root from
> config. Roots changing between calls?");
> > +
> > +                       continue;
> > +               }
> > +
> > +               *value = '\0';
> > +               value = value + 1;
> > +
> > +               if (!strcmp(key, CONFIG_MOUNTROOT_KEY) && !strcmp(value,
> "yes")) {
> > +                       root->we_mounted_it = 1;
> > +
>
> > +               } else if (!strcmp(key, CONFIG_LTPDIR_KEY) &&
> !root->ltp_dir.dir_name) {
> > +                       cgroup_dir_mk(&root->mnt_dir, cgroup_ltp_dir,
> &root->ltp_dir);
> > +                       if (!strcmp(value, "yes"))
> > +                               root->ltp_dir.we_created_it = 1;
> > +
> > +               } else if (!strcmp(key, CONFIG_DRAINDIR_KEY) &&
> !root->drain_dir.dir_name) {
> > +                       cgroup_dir_mk(&root->ltp_dir,
> cgroup_ltp_drain_dir, &root->drain_dir);
> > +                       if (!strcmp(value, "yes"))
> > +                               root->ltp_dir.we_created_it = 1;
>
> I think that parsing the  CONFIG_DRAINDIR_KEY from '$_cgroup_state'
> is superfluous. Because from the tst_cgroup_cleanup, if
> root->ltp_dir.we_created_it
> is 1 then both of the two directories will be removed, so just using
> CONFIG_LTPDIR_KE
> to track the status is enough.
>
>
This is definitely true and I think it makes it a lot simpler. I will add
this in the next version.

And maybe it is not necessary to print "Created_Drain_Dir=xx" in
> tst_cgroup_print_config at all.
>
> Then, the code snippet could be as:
>
>                 } else if (!strcmp(key, CONFIG_LTPDIR_KEY) &&
> !root->ltp_dir.dir_name) {
>                         cgroup_dir_mk(&root->mnt_dir, cgroup_ltp_dir,
> &root->ltp_dir);
>                         cgroup_dir_mk(&root->ltp_dir,
> cgroup_ltp_drain_dir, &root->drain_dir);
>                         if (!strcmp(value, "yes"))
>                                 root->ltp_dir.we_created_it = 1;
>
>
> > +
> > +               } else if (!strcmp(key, CONFIG_TESTID_KEY) &&
> strcmp(value, "NULL") &&
> > +                                  !root->test_dir.dir_name) {
> > +                       cgroup_dir_mk(&root->ltp_dir, value,
> &root->test_dir);
> > +                       root->test_dir.we_created_it = 1;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
>
>
>
>
I'll be posting the changes along with all the modified tests probably
later today or early next week, and I will definitely need help reviewing
that :)


>
> --
> Regards,
> Li Wang
>
>
Thanks as always,
- Luke

[-- Attachment #1.2: Type: text/html, Size: 4804 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp