[PATCH v1 1/9] perf thread_map: Skip exited threads when scanning /proc

[PATCH v1 1/9] perf thread_map: Skip exited threads when scanning /proc

[Ian Rogers]

Scanning /proc is inherently racy. Scanning /proc/pid/task within that
is also racy as the pid can terminate. Rather than failing in
__thread_map__new_all_cpus, skip pids for such failures.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/thread_map.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c
index e848579e61a8..18fbc41d09f3 100644
--- a/tools/perf/util/thread_map.c
+++ b/tools/perf/util/thread_map.c
@@ -109,9 +109,10 @@ static struct perf_thread_map *__thread_map__new_all_cpus(uid_t uid)
 
 		snprintf(path, sizeof(path), "/proc/%d/task", pid);
 		items = scandir(path, &namelist, filter, NULL);
-		if (items <= 0)
-			goto out_free_closedir;
-
+		if (items <= 0) {
+			pr_debug("scandir for %d returned empty, skipping\n", pid);
+			continue;
+		}
 		while (threads->nr + items >= max_threads) {
 			max_threads *= 2;
 			grow = true;
@@ -152,8 +153,6 @@ static struct perf_thread_map *__thread_map__new_all_cpus(uid_t uid)
 	for (i = 0; i < items; i++)
 		zfree(&namelist[i]);
 	free(namelist);
-
-out_free_closedir:
 	zfree(&threads);
 	goto out_closedir;
 }
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v1 2/9] perf list: Add scandirat compatibility function

[Ian Rogers]

scandirat is used during the printing of tracepoint events but may be
missing from certain libcs. Add a compatibility implementation that
uses the symlink of an fd in /proc as a path for the reliably present
scandir.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/print-events.c | 12 +++---------
 tools/perf/util/util.c         | 19 +++++++++++++++++++
 tools/perf/util/util.h         |  8 ++++++++
 3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
index b0fc48be623f..15ec55b07bfd 100644
--- a/tools/perf/util/print-events.c
+++ b/tools/perf/util/print-events.c
@@ -63,6 +63,8 @@ void print_tracepoint_events(const struct print_callbacks *print_cb __maybe_unus
 {
 	char *events_path = get_tracing_file("events");
 	int events_fd = open(events_path, O_PATH);
+	struct dirent **sys_namelist = NULL;
+	int sys_items;
 
 	put_tracing_file(events_path);
 	if (events_fd < 0) {
@@ -70,10 +72,7 @@ void print_tracepoint_events(const struct print_callbacks *print_cb __maybe_unus
 		return;
 	}
 
-#ifdef HAVE_SCANDIRAT_SUPPORT
-{
-	struct dirent **sys_namelist = NULL;
-	int sys_items = tracing_events__scandir_alphasort(&sys_namelist);
+	sys_items = tracing_events__scandir_alphasort(&sys_namelist);
 
 	for (int i = 0; i < sys_items; i++) {
 		struct dirent *sys_dirent = sys_namelist[i];
@@ -130,11 +129,6 @@ void print_tracepoint_events(const struct print_callbacks *print_cb __maybe_unus
 	}
 
 	free(sys_namelist);
-}
-#else
-	printf("\nWARNING: Your libc doesn't have the scandirat function, please ask its maintainers to implement it.\n"
-	       "         As a rough fallback, please do 'ls %s' to see the available tracepoint events.\n", events_path);
-#endif
 	close(events_fd);
 }
 
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index c1fd9ba6d697..4f561e5e4162 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -552,3 +552,22 @@ int sched_getcpu(void)
 	return -1;
 }
 #endif
+
+#ifndef HAVE_SCANDIRAT_SUPPORT
+int scandirat(int dirfd, const char *dirp,
+	      struct dirent ***namelist,
+	      int (*filter)(const struct dirent *),
+	      int (*compar)(const struct dirent **, const struct dirent **))
+{
+	char path[PATH_MAX];
+	int err, fd = openat(dirfd, dirp, O_PATH);
+
+	if (fd < 0)
+		return fd;
+
+	snprintf(path, sizeof(path), "/proc/%d/fd/%d", getpid(), fd);
+	err = scandir(path, namelist, filter, compar);
+	close(fd);
+	return err;
+}
+#endif
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 7c8915d92dca..9966c21aaf04 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -6,6 +6,7 @@
 /* glibc 2.20 deprecates _BSD_SOURCE in favour of _DEFAULT_SOURCE */
 #define _DEFAULT_SOURCE 1
 
+#include <dirent.h>
 #include <fcntl.h>
 #include <stdbool.h>
 #include <stddef.h>
@@ -56,6 +57,13 @@ int perf_tip(char **strp, const char *dirpath);
 int sched_getcpu(void);
 #endif
 
+#ifndef HAVE_SCANDIRAT_SUPPORT
+int scandirat(int dirfd, const char *dirp,
+	      struct dirent ***namelist,
+	      int (*filter)(const struct dirent *),
+	      int (*compar)(const struct dirent **, const struct dirent **));
+#endif
+
 extern bool perf_singlethreaded;
 
 void perf_set_singlethreaded(void);
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v1 3/9] perf tests: Avoid fork in perf_has_symbol test

[Ian Rogers]

perf test -vv Symbols is used to indentify symbols within the perf
binary. Add the -F flag so that the test command doesn't fork the test
before running. This removes a little overhead.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/lib/perf_has_symbol.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/tests/shell/lib/perf_has_symbol.sh b/tools/perf/tests/shell/lib/perf_has_symbol.sh
index 5d59c32ae3e7..561c93b75d77 100644
--- a/tools/perf/tests/shell/lib/perf_has_symbol.sh
+++ b/tools/perf/tests/shell/lib/perf_has_symbol.sh
@@ -3,7 +3,7 @@
 
 perf_has_symbol()
 {
-	if perf test -vv "Symbols" 2>&1 | grep "[[:space:]]$1$"; then
+	if perf test -vv -F "Symbols" 2>&1 | grep "[[:space:]]$1$"; then
 		echo "perf does have symbol '$1'"
 		return 0
 	fi
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v1 4/9] tools subcmd: Add a no exec function call option

[Ian Rogers]

Tools like perf fork tests in case they crash, but they don't want to
exec a full binary. Add an option to call a function rather than do an
exec. The child process exits with the result of the function call and
is passed the struct of the run_command, things like container_of can
then allow the child process function to determine additional
arguments.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/subcmd/run-command.c | 2 ++
 tools/lib/subcmd/run-command.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/tools/lib/subcmd/run-command.c b/tools/lib/subcmd/run-command.c
index 5cdac2162532..d435eb42354b 100644
--- a/tools/lib/subcmd/run-command.c
+++ b/tools/lib/subcmd/run-command.c
@@ -122,6 +122,8 @@ int start_command(struct child_process *cmd)
 		}
 		if (cmd->preexec_cb)
 			cmd->preexec_cb();
+		if (cmd->no_exec_cmd)
+			exit(cmd->no_exec_cmd(cmd));
 		if (cmd->exec_cmd) {
 			execv_cmd(cmd->argv);
 		} else {
diff --git a/tools/lib/subcmd/run-command.h b/tools/lib/subcmd/run-command.h
index 17d969c6add3..d794138a797f 100644
--- a/tools/lib/subcmd/run-command.h
+++ b/tools/lib/subcmd/run-command.h
@@ -47,6 +47,8 @@ struct child_process {
 	unsigned exec_cmd:1; /* if this is to be external sub-command */
 	unsigned stdout_to_stderr:1;
 	void (*preexec_cb)(void);
+	 /* If set, call function in child rather than doing an exec. */
+	int (*no_exec_cmd)(struct child_process *process);
 };
 
 int start_command(struct child_process *);
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v1 5/9] perf test: Rename builtin-test-list and add missed header guard

[Ian Rogers]

builtin-test-list is primarily concerned with shell script
tests. Rename the file to better reflect this and add a missed header
guard.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/Build                                    | 2 +-
 tools/perf/tests/builtin-test.c                           | 2 +-
 tools/perf/tests/{builtin-test-list.c => tests-scripts.c} | 2 +-
 tools/perf/tests/{builtin-test-list.h => tests-scripts.h} | 4 ++++
 4 files changed, 7 insertions(+), 3 deletions(-)
 rename tools/perf/tests/{builtin-test-list.c => tests-scripts.c} (99%)
 rename tools/perf/tests/{builtin-test-list.h => tests-scripts.h} (79%)

diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index 2b45ffa462a6..5cc814af8e37 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 perf-y += builtin-test.o
-perf-y += builtin-test-list.o
+perf-y += tests-scripts.o
 perf-y += parse-events.o
 perf-y += dso-data.o
 perf-y += attr.o
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index b8c21e81a021..fab49a60d508 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -29,7 +29,7 @@
 #include <subcmd/exec-cmd.h>
 #include <linux/zalloc.h>
 
-#include "builtin-test-list.h"
+#include "tests-scripts.h"
 
 static bool dont_fork;
 const char *dso_to_test;
diff --git a/tools/perf/tests/builtin-test-list.c b/tools/perf/tests/tests-scripts.c
similarity index 99%
rename from tools/perf/tests/builtin-test-list.c
rename to tools/perf/tests/tests-scripts.c
index a65b9e547d82..4ebd841da05b 100644
--- a/tools/perf/tests/builtin-test-list.c
+++ b/tools/perf/tests/tests-scripts.c
@@ -15,7 +15,7 @@
 #include <sys/wait.h>
 #include <sys/stat.h>
 #include "builtin.h"
-#include "builtin-test-list.h"
+#include "tests-scripts.h"
 #include "color.h"
 #include "debug.h"
 #include "hist.h"
diff --git a/tools/perf/tests/builtin-test-list.h b/tools/perf/tests/tests-scripts.h
similarity index 79%
rename from tools/perf/tests/builtin-test-list.h
rename to tools/perf/tests/tests-scripts.h
index eb81f3aa6683..3a3ec6191848 100644
--- a/tools/perf/tests/builtin-test-list.h
+++ b/tools/perf/tests/tests-scripts.h
@@ -1,4 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 */
+#ifndef TESTS_SCRIPTS_H
+#define TESTS_SCRIPTS_H
 
 struct script_file {
 	char *dir;
@@ -10,3 +12,5 @@ struct script_file {
 const struct script_file *list_script_files(void);
 /* Get maximum width of description string */
 int list_script_max_width(void);
+
+#endif /* TESTS_SCRIPTS_H */
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v1 6/9] perf tests: Use scandirat for shell script finding

[Ian Rogers]

Avoid filename appending buffers by using openat, faccessat and
scandirat more widely. Turn the script's path back to a file name
using readlink from /proc/<pid>/fd/<fd>.

Read the script's description using api/io.h to avoid fdopen
conversions. Whilst reading perform additional sanity checks on the
script's contents.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/builtin-test.c  |  21 ++---
 tools/perf/tests/tests-scripts.c | 144 ++++++++++++++++++-------------
 tools/perf/tests/tests-scripts.h |   1 -
 3 files changed, 95 insertions(+), 71 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index fab49a60d508..2669d1d66ed8 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -302,22 +302,20 @@ static int test_and_print(struct test_suite *t, int subtest)
 }
 
 struct shell_test {
-	const char *dir;
 	const char *file;
 };
 
 static int shell_test__run(struct test_suite *test, int subdir __maybe_unused)
 {
 	int err;
-	char script[PATH_MAX];
 	struct shell_test *st = test->priv;
+	char *cmd;
 
-	path__join(script, sizeof(script) - 3, st->dir, st->file);
-
-	if (verbose > 0)
-		strncat(script, " -v", sizeof(script) - strlen(script) - 1);
-
-	err = system(script);
+	asprintf(&cmd, "%s%s", st->file, verbose ? " -v" : "");
+	if (!cmd)
+		return TEST_FAIL;
+	err = system(cmd);
+	free(cmd);
 	if (!err)
 		return TEST_OK;
 
@@ -333,7 +331,7 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width,
 	files = list_script_files();
 	if (!files)
 		return 0;
-	for (file = files; file->dir; file++) {
+	for (file = files; file->file; file++) {
 		int curr = i++;
 		struct test_case test_cases[] = {
 			{
@@ -347,13 +345,12 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width,
 			.test_cases = test_cases,
 			.priv = &st,
 		};
-		st.dir = file->dir;
+		st.file = file->file;
 
 		if (test_suite.desc == NULL ||
 		    !perf_test__matches(test_suite.desc, curr, argc, argv))
 			continue;
 
-		st.file = file->file;
 		pr_info("%3d: %-*s:", i, width, test_suite.desc);
 
 		if (intlist__find(skiplist, i)) {
@@ -457,7 +454,7 @@ static int perf_test__list_shell(int argc, const char **argv, int i)
 	files = list_script_files();
 	if (!files)
 		return 0;
-	for (file = files; file->dir; file++) {
+	for (file = files; file->file; file++) {
 		int curr = i++;
 		struct test_suite t = {
 			.desc = file->desc
diff --git a/tools/perf/tests/tests-scripts.c b/tools/perf/tests/tests-scripts.c
index 4ebd841da05b..9b3b66dd5508 100644
--- a/tools/perf/tests/tests-scripts.c
+++ b/tools/perf/tests/tests-scripts.c
@@ -14,6 +14,7 @@
 #include <subcmd/parse-options.h>
 #include <sys/wait.h>
 #include <sys/stat.h>
+#include <api/io.h>
 #include "builtin.h"
 #include "tests-scripts.h"
 #include "color.h"
@@ -35,55 +36,69 @@ static size_t files_num = 0;
 static struct script_file *files = NULL;
 static int files_max_width = 0;
 
-static const char *shell_tests__dir(char *path, size_t size)
+static int shell_tests__dir_fd(void)
 {
-	const char *devel_dirs[] = { "./tools/perf/tests", "./tests", };
-	char *exec_path;
-	unsigned int i;
+	char path[PATH_MAX], *exec_path;
+	static const char * const devel_dirs[] = { "./tools/perf/tests/shell", "./tests/shell", };
 
-	for (i = 0; i < ARRAY_SIZE(devel_dirs); ++i) {
-		struct stat st;
+	for (size_t i = 0; i < ARRAY_SIZE(devel_dirs); ++i) {
+		int fd = open(devel_dirs[i], O_PATH);
 
-		if (!lstat(devel_dirs[i], &st)) {
-			scnprintf(path, size, "%s/shell", devel_dirs[i]);
-			if (!lstat(devel_dirs[i], &st))
-				return path;
-		}
+		if (fd >= 0)
+			return fd;
 	}
 
 	/* Then installed path. */
 	exec_path = get_argv_exec_path();
-	scnprintf(path, size, "%s/tests/shell", exec_path);
+	scnprintf(path, sizeof(path), "%s/tests/shell", exec_path);
 	free(exec_path);
-	return path;
+	return open(path, O_PATH);
 }
 
-static const char *shell_test__description(char *description, size_t size,
-                                           const char *path, const char *name)
+static char *shell_test__description(int dir_fd, const char *name)
 {
-	FILE *fp;
-	char filename[PATH_MAX];
-	int ch;
+	struct io io;
+	char buf[128], desc[256];
+	int ch, pos = 0;
 
-	path__join(filename, sizeof(filename), path, name);
-	fp = fopen(filename, "r");
-	if (!fp)
+	io__init(&io, openat(dir_fd, name, O_RDONLY), buf, sizeof(buf));
+	if (io.fd < 0)
 		return NULL;
 
 	/* Skip first line - should be #!/bin/sh Shebang */
+	if (io__get_char(&io) != '#')
+		goto err_out;
+	if (io__get_char(&io) != '!')
+		goto err_out;
 	do {
-		ch = fgetc(fp);
-	} while (ch != EOF && ch != '\n');
-
-	description = fgets(description, size, fp);
-	fclose(fp);
+		ch = io__get_char(&io);
+		if (ch < 0)
+			goto err_out;
+	} while (ch != '\n');
 
-	/* Assume first char on line is omment everything after that desc */
-	return description ? strim(description + 1) : NULL;
+	do {
+		ch = io__get_char(&io);
+		if (ch < 0)
+			goto err_out;
+	} while (ch == '#' || isspace(ch));
+	while (ch > 0 && ch != '\n') {
+		desc[pos++] = ch;
+		if (pos >= (int)sizeof(desc) - 1)
+			break;
+		ch = io__get_char(&io);
+	}
+	while (pos > 0 && isspace(desc[--pos]))
+		;
+	desc[++pos] = '\0';
+	close(io.fd);
+	return strdup(desc);
+err_out:
+	close(io.fd);
+	return NULL;
 }
 
 /* Is this full file path a shell script */
-static bool is_shell_script(const char *path)
+static bool is_shell_script(int dir_fd, const char *path)
 {
 	const char *ext;
 
@@ -91,20 +106,16 @@ static bool is_shell_script(const char *path)
 	if (!ext)
 		return false;
 	if (!strcmp(ext, ".sh")) { /* Has .sh extension */
-		if (access(path, R_OK | X_OK) == 0) /* Is executable */
+		if (faccessat(dir_fd, path, R_OK | X_OK, 0) == 0) /* Is executable */
 			return true;
 	}
 	return false;
 }
 
 /* Is this file in this dir a shell script (for test purposes) */
-static bool is_test_script(const char *path, const char *name)
+static bool is_test_script(int dir_fd, const char *name)
 {
-	char filename[PATH_MAX];
-
-	path__join(filename, sizeof(filename), path, name);
-	if (!is_shell_script(filename)) return false;
-	return true;
+	return is_shell_script(dir_fd, name);
 }
 
 /* Duplicate a string and fall over and die if we run out of memory */
@@ -120,12 +131,21 @@ static char *strdup_check(const char *str)
 	return newstr;
 }
 
-static void append_script(const char *dir, const char *file, const char *desc)
+static void append_script(int dir_fd, const char *name, char *desc)
 {
+	char filename[PATH_MAX], link[128];
 	struct script_file *files_tmp;
-	size_t files_num_tmp;
+	size_t files_num_tmp, len;
 	int width;
 
+	snprintf(link, sizeof(link), "/proc/%d/fd/%d", getpid(), dir_fd);
+	len = readlink(link, filename, sizeof(filename));
+	if (len < 0) {
+		pr_err("Failed to readlink %s", link);
+		return;
+	}
+	filename[len++] = '/';
+	strcpy(&filename[len], name);
 	files_num_tmp = files_num + 1;
 	if (files_num_tmp >= SIZE_MAX) {
 		pr_err("Too many script files\n");
@@ -142,10 +162,8 @@ static void append_script(const char *dir, const char *file, const char *desc)
 	/* Add file to end and NULL terminate the struct array */
 	files = files_tmp;
 	files_num = files_num_tmp;
-	files[files_num - 1].dir = strdup_check(dir);
-	files[files_num - 1].file = strdup_check(file);
-	files[files_num - 1].desc = strdup_check(desc);
-	files[files_num].dir = NULL;
+	files[files_num - 1].file = strdup_check(filename);
+	files[files_num - 1].desc = desc;
 	files[files_num].file = NULL;
 	files[files_num].desc = NULL;
 
@@ -154,32 +172,39 @@ static void append_script(const char *dir, const char *file, const char *desc)
 		files_max_width = width;
 }
 
-static void append_scripts_in_dir(const char *path)
+static void append_scripts_in_dir(int dir_fd)
 {
 	struct dirent **entlist;
 	struct dirent *ent;
 	int n_dirs, i;
-	char filename[PATH_MAX];
 
 	/* List files, sorted by alpha */
-	n_dirs = scandir(path, &entlist, NULL, alphasort);
+	n_dirs = scandirat(dir_fd, ".", &entlist, NULL, alphasort);
 	if (n_dirs == -1)
 		return;
 	for (i = 0; i < n_dirs && (ent = entlist[i]); i++) {
+		int fd;
+
 		if (ent->d_name[0] == '.')
 			continue; /* Skip hidden files */
-		if (is_test_script(path, ent->d_name)) { /* It's a test */
-			char bf[256];
-			const char *desc = shell_test__description
-				(bf, sizeof(bf), path, ent->d_name);
+		if (is_test_script(dir_fd, ent->d_name)) { /* It's a test */
+			char *desc = shell_test__description(dir_fd, ent->d_name);
 
 			if (desc) /* It has a desc line - valid script */
-				append_script(path, ent->d_name, desc);
-		} else if (is_directory(path, ent)) { /* Scan the subdir */
-			path__join(filename, sizeof(filename),
-				   path, ent->d_name);
-			append_scripts_in_dir(filename);
+				append_script(dir_fd, ent->d_name, desc);
+			continue;
+		}
+		if (ent->d_type != DT_DIR) {
+			struct stat st;
+
+			if (ent->d_type != DT_UNKNOWN)
+				continue;
+			fstatat(dir_fd, ent->d_name, &st, 0);
+			if (!S_ISDIR(st.st_mode))
+				continue;
 		}
+		fd = openat(dir_fd, ent->d_name, O_PATH);
+		append_scripts_in_dir(fd);
 	}
 	for (i = 0; i < n_dirs; i++) /* Clean up */
 		zfree(&entlist[i]);
@@ -188,14 +213,17 @@ static void append_scripts_in_dir(const char *path)
 
 const struct script_file *list_script_files(void)
 {
-	char path_dir[PATH_MAX];
-	const char *path;
+	int dir_fd;
 
 	if (files)
 		return files; /* Singleton - we already know our list */
 
-	path = shell_tests__dir(path_dir, sizeof(path_dir)); /* Walk  dir */
-	append_scripts_in_dir(path);
+	dir_fd = shell_tests__dir_fd(); /* Walk  dir */
+	if (dir_fd < 0)
+		return NULL;
+
+	append_scripts_in_dir(dir_fd);
+	close(dir_fd);
 
 	return files;
 }
diff --git a/tools/perf/tests/tests-scripts.h b/tools/perf/tests/tests-scripts.h
index 3a3ec6191848..3508a293aaf9 100644
--- a/tools/perf/tests/tests-scripts.h
+++ b/tools/perf/tests/tests-scripts.h
@@ -3,7 +3,6 @@
 #define TESTS_SCRIPTS_H
 
 struct script_file {
-	char *dir;
 	char *file;
 	char *desc;
 };
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v1 7/9] perf tests: Run time generate shell test suites

[Ian Rogers]

Rather than special shell test logic, do a single pass to create an
array of test suites. Hold the shell test file name in the test suite
priv field. This makes the special shell test logic in builtin-test.c
redundant so remove it.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/builtin-test.c  |  91 +----------------------
 tools/perf/tests/tests-scripts.c | 120 ++++++++++++++++++-------------
 tools/perf/tests/tests-scripts.h |  10 +--
 3 files changed, 74 insertions(+), 147 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 2669d1d66ed8..54b11c23e863 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -132,6 +132,7 @@ static struct test_suite *generic_tests[] = {
 static struct test_suite **tests[] = {
 	generic_tests,
 	arch_tests,
+	NULL, /* shell tests created at runtime. */
 };
 
 static struct test_workload *workloads[] = {
@@ -301,74 +302,12 @@ static int test_and_print(struct test_suite *t, int subtest)
 	return err;
 }
 
-struct shell_test {
-	const char *file;
-};
-
-static int shell_test__run(struct test_suite *test, int subdir __maybe_unused)
-{
-	int err;
-	struct shell_test *st = test->priv;
-	char *cmd;
-
-	asprintf(&cmd, "%s%s", st->file, verbose ? " -v" : "");
-	if (!cmd)
-		return TEST_FAIL;
-	err = system(cmd);
-	free(cmd);
-	if (!err)
-		return TEST_OK;
-
-	return WEXITSTATUS(err) == 2 ? TEST_SKIP : TEST_FAIL;
-}
-
-static int run_shell_tests(int argc, const char *argv[], int i, int width,
-				struct intlist *skiplist)
-{
-	struct shell_test st;
-	const struct script_file *files, *file;
-
-	files = list_script_files();
-	if (!files)
-		return 0;
-	for (file = files; file->file; file++) {
-		int curr = i++;
-		struct test_case test_cases[] = {
-			{
-				.desc = file->desc,
-				.run_case = shell_test__run,
-			},
-			{ .name = NULL, }
-		};
-		struct test_suite test_suite = {
-			.desc = test_cases[0].desc,
-			.test_cases = test_cases,
-			.priv = &st,
-		};
-		st.file = file->file;
-
-		if (test_suite.desc == NULL ||
-		    !perf_test__matches(test_suite.desc, curr, argc, argv))
-			continue;
-
-		pr_info("%3d: %-*s:", i, width, test_suite.desc);
-
-		if (intlist__find(skiplist, i)) {
-			color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip (user override)\n");
-			continue;
-		}
-
-		test_and_print(&test_suite, 0);
-	}
-	return 0;
-}
-
 static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
 {
 	struct test_suite *t;
 	unsigned int j, k;
 	int i = 0;
-	int width = list_script_max_width();
+	int width = 0;
 
 	for_each_test(j, k, t) {
 		int len = strlen(test_description(t, -1));
@@ -443,28 +382,6 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
 			}
 		}
 	}
-
-	return run_shell_tests(argc, argv, i, width, skiplist);
-}
-
-static int perf_test__list_shell(int argc, const char **argv, int i)
-{
-	const struct script_file *files, *file;
-
-	files = list_script_files();
-	if (!files)
-		return 0;
-	for (file = files; file->file; file++) {
-		int curr = i++;
-		struct test_suite t = {
-			.desc = file->desc
-		};
-
-		if (!perf_test__matches(t.desc, curr, argc, argv))
-			continue;
-
-		pr_info("%3d: %s\n", i, t.desc);
-	}
 	return 0;
 }
 
@@ -491,9 +408,6 @@ static int perf_test__list(int argc, const char **argv)
 					test_description(t, subi));
 		}
 	}
-
-	perf_test__list_shell(argc, argv, i);
-
 	return 0;
 }
 
@@ -553,6 +467,7 @@ int cmd_test(int argc, const char **argv)
 	/* Unbuffered output */
 	setvbuf(stdout, NULL, _IONBF, 0);
 
+	tests[2] = create_script_test_suites();
 	argc = parse_options_subcommand(argc, argv, test_options, test_subcommands, test_usage, 0);
 	if (argc >= 1 && !strcmp(argv[0], "list"))
 		return perf_test__list(argc - 1, argv + 1);
diff --git a/tools/perf/tests/tests-scripts.c b/tools/perf/tests/tests-scripts.c
index 9b3b66dd5508..b92a93c251c6 100644
--- a/tools/perf/tests/tests-scripts.c
+++ b/tools/perf/tests/tests-scripts.c
@@ -26,16 +26,6 @@
 #include "tests.h"
 #include "util/rlimit.h"
 
-
-/*
- * As this is a singleton built once for the run of the process, there is
- * no value in trying to free it and just let it stay around until process
- * exits when it's cleaned up.
- */
-static size_t files_num = 0;
-static struct script_file *files = NULL;
-static int files_max_width = 0;
-
 static int shell_tests__dir_fd(void)
 {
 	char path[PATH_MAX], *exec_path;
@@ -131,12 +121,31 @@ static char *strdup_check(const char *str)
 	return newstr;
 }
 
-static void append_script(int dir_fd, const char *name, char *desc)
+static int shell_test__run(struct test_suite *test, int subtest __maybe_unused)
+{
+	const char *file = test->priv;
+	int err;
+	char *cmd;
+
+	asprintf(&cmd, "%s%s", file, verbose ? " -v" : "");
+	if (!cmd)
+		return TEST_FAIL;
+	err = system(cmd);
+	free(cmd);
+	if (!err)
+		return TEST_OK;
+
+	return WEXITSTATUS(err) == 2 ? TEST_SKIP : TEST_FAIL;
+}
+
+static void append_script(int dir_fd, const char *name, char *desc,
+			  struct test_suite ***result,
+			  size_t *result_sz)
 {
 	char filename[PATH_MAX], link[128];
-	struct script_file *files_tmp;
-	size_t files_num_tmp, len;
-	int width;
+	struct test_suite *test_suite, **result_tmp;
+	struct test_case *tests;
+	size_t len;
 
 	snprintf(link, sizeof(link), "/proc/%d/fd/%d", getpid(), dir_fd);
 	len = readlink(link, filename, sizeof(filename));
@@ -146,33 +155,43 @@ static void append_script(int dir_fd, const char *name, char *desc)
 	}
 	filename[len++] = '/';
 	strcpy(&filename[len], name);
-	files_num_tmp = files_num + 1;
-	if (files_num_tmp >= SIZE_MAX) {
-		pr_err("Too many script files\n");
-		abort();
+
+	tests = calloc(2, sizeof(*tests));
+	if (!tests) {
+		pr_err("Out of memory while building script test suite list\n");
+		return;
+	}
+	tests[0].name = strdup_check(name);
+	tests[0].desc = strdup_check(desc);
+	tests[0].run_case = shell_test__run;
+
+	test_suite = zalloc(sizeof(*test_suite));
+	if (!test_suite) {
+		pr_err("Out of memory while building script test suite list\n");
+		free(tests);
+		return;
 	}
+	test_suite->desc = desc;
+	test_suite->test_cases = tests;
+	test_suite->priv = strdup_check(filename);
 	/* Realloc is good enough, though we could realloc by chunks, not that
 	 * anyone will ever measure performance here */
-	files_tmp = realloc(files,
-			    (files_num_tmp + 1) * sizeof(struct script_file));
-	if (files_tmp == NULL) {
-		pr_err("Out of memory while building test list\n");
-		abort();
+	result_tmp = realloc(*result, (*result_sz + 1) * sizeof(*result_tmp));
+	if (result_tmp == NULL) {
+		pr_err("Out of memory while building script test suite list\n");
+		free(tests);
+		free(test_suite);
+		return;
 	}
 	/* Add file to end and NULL terminate the struct array */
-	files = files_tmp;
-	files_num = files_num_tmp;
-	files[files_num - 1].file = strdup_check(filename);
-	files[files_num - 1].desc = desc;
-	files[files_num].file = NULL;
-	files[files_num].desc = NULL;
-
-	width = strlen(desc); /* Track max width of desc */
-	if (width > files_max_width)
-		files_max_width = width;
+	*result = result_tmp;
+	(*result)[*result_sz] = test_suite;
+	(*result_sz)++;
 }
 
-static void append_scripts_in_dir(int dir_fd)
+static void append_scripts_in_dir(int dir_fd,
+				  struct test_suite ***result,
+				  size_t *result_sz)
 {
 	struct dirent **entlist;
 	struct dirent *ent;
@@ -191,7 +210,7 @@ static void append_scripts_in_dir(int dir_fd)
 			char *desc = shell_test__description(dir_fd, ent->d_name);
 
 			if (desc) /* It has a desc line - valid script */
-				append_script(dir_fd, ent->d_name, desc);
+				append_script(dir_fd, ent->d_name, desc, result, result_sz);
 			continue;
 		}
 		if (ent->d_type != DT_DIR) {
@@ -204,32 +223,31 @@ static void append_scripts_in_dir(int dir_fd)
 				continue;
 		}
 		fd = openat(dir_fd, ent->d_name, O_PATH);
-		append_scripts_in_dir(fd);
+		append_scripts_in_dir(fd, result, result_sz);
 	}
 	for (i = 0; i < n_dirs; i++) /* Clean up */
 		zfree(&entlist[i]);
 	free(entlist);
 }
 
-const struct script_file *list_script_files(void)
+struct test_suite **create_script_test_suites(void)
 {
-	int dir_fd;
-
-	if (files)
-		return files; /* Singleton - we already know our list */
+	struct test_suite **result = NULL, **result_tmp;
+	size_t result_sz = 0;
+	int dir_fd = shell_tests__dir_fd(); /* Walk  dir */
 
-	dir_fd = shell_tests__dir_fd(); /* Walk  dir */
 	if (dir_fd < 0)
 		return NULL;
 
-	append_scripts_in_dir(dir_fd);
+	append_scripts_in_dir(dir_fd, &result, &result_sz);
+	result_tmp = realloc(result, (result_sz + 1) * sizeof(*result_tmp));
+	if (result_tmp == NULL) {
+		pr_err("Out of memory while building script test suite list\n");
+		abort();
+	}
+	/* NULL terminate the test suite array. */
+	result = result_tmp;
+	result[result_sz] = NULL;
 	close(dir_fd);
-
-	return files;
-}
-
-int list_script_max_width(void)
-{
-	list_script_files(); /* Ensure we have scanned all scripts */
-	return files_max_width;
+	return result;
 }
diff --git a/tools/perf/tests/tests-scripts.h b/tools/perf/tests/tests-scripts.h
index 3508a293aaf9..b553ad26ea17 100644
--- a/tools/perf/tests/tests-scripts.h
+++ b/tools/perf/tests/tests-scripts.h
@@ -2,14 +2,8 @@
 #ifndef TESTS_SCRIPTS_H
 #define TESTS_SCRIPTS_H
 
-struct script_file {
-	char *file;
-	char *desc;
-};
+#include "tests.h"
 
-/* List available script tests to run - singleton - never freed */
-const struct script_file *list_script_files(void);
-/* Get maximum width of description string */
-int list_script_max_width(void);
+struct test_suite **create_script_test_suites(void);
 
 #endif /* TESTS_SCRIPTS_H */
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v1 8/9] perf srcline: Add missed addr2line closes

[Ian Rogers]

The child_process for addr2line sets in and out to -1 so that pipes
get created. It is the caller's responsibility to close the pipes,
finish_command doesn't do it. Add the missed closes.

Fixes: b3801e791231 ("perf srcline: Simplify addr2line subprocess")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/srcline.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index 034b496df297..7addc34afcf5 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -399,6 +399,8 @@ static void addr2line_subprocess_cleanup(struct child_process *a2l)
 		kill(a2l->pid, SIGKILL);
 		finish_command(a2l); /* ignore result, we don't care */
 		a2l->pid = -1;
+		close(a2l->in);
+		close(a2l->out);
 	}
 
 	free(a2l);
-- 
2.43.0.rc2.451.g8631bc7472-goog

[PATCH v1 9/9] perf tests: Add option to run tests in parallel

[Ian Rogers]

By default tests are forked, add an option (-p or --parallel) so that
the forked tests are all started in parallel and then their output
gathered serially. This is opt-in as running in parallel can cause
test flakes.

Rather than fork within the code, the start_command/finish_command
from libsubcmd are used. This changes how stderr and stdout are
handled.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/builtin-test.c | 261 +++++++++++++++++++++-----------
 1 file changed, 169 insertions(+), 92 deletions(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 54b11c23e863..91c32b477cbb 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -21,6 +21,7 @@
 #include "debug.h"
 #include "color.h"
 #include <subcmd/parse-options.h>
+#include <subcmd/run-command.h>
 #include "string2.h"
 #include "symbol.h"
 #include "util/rlimit.h"
@@ -31,7 +32,13 @@
 
 #include "tests-scripts.h"
 
+/*
+ * Command line option to not fork the test running in the same process and
+ * making them easier to debug.
+ */
 static bool dont_fork;
+/* Fork the tests in parallel and then wait for their completion. */
+static bool parallel;
 const char *dso_to_test;
 const char *test_objdump_path = "objdump";
 
@@ -211,76 +218,36 @@ static bool perf_test__matches(const char *desc, int curr, int argc, const char
 	return false;
 }
 
-static int run_test(struct test_suite *test, int subtest)
-{
-	int status, err = -1, child = dont_fork ? 0 : fork();
-	char sbuf[STRERR_BUFSIZE];
-
-	if (child < 0) {
-		pr_err("failed to fork test: %s\n",
-			str_error_r(errno, sbuf, sizeof(sbuf)));
-		return -1;
-	}
-
-	if (!child) {
-		if (!dont_fork) {
-			pr_debug("test child forked, pid %d\n", getpid());
-
-			if (verbose <= 0) {
-				int nullfd = open("/dev/null", O_WRONLY);
-
-				if (nullfd >= 0) {
-					close(STDERR_FILENO);
-					close(STDOUT_FILENO);
-
-					dup2(nullfd, STDOUT_FILENO);
-					dup2(STDOUT_FILENO, STDERR_FILENO);
-					close(nullfd);
-				}
-			} else {
-				signal(SIGSEGV, sighandler_dump_stack);
-				signal(SIGFPE, sighandler_dump_stack);
-			}
-		}
-
-		err = test_function(test, subtest)(test, subtest);
-		if (!dont_fork)
-			exit(err);
-	}
-
-	if (!dont_fork) {
-		wait(&status);
+struct child_test {
+	struct child_process process;
+	struct test_suite *test;
+	int test_num;
+	int subtest;
+};
 
-		if (WIFEXITED(status)) {
-			err = (signed char)WEXITSTATUS(status);
-			pr_debug("test child finished with %d\n", err);
-		} else if (WIFSIGNALED(status)) {
-			err = -1;
-			pr_debug("test child interrupted\n");
-		}
-	}
+static int run_test_child(struct child_process *process)
+{
+	struct child_test *child = container_of(process, struct child_test, process);
+	int err;
 
-	return err;
+	pr_debug("--- start ---\n");
+	pr_debug("test child forked, pid %d\n", getpid());
+	err = test_function(child->test, child->subtest)(child->test, child->subtest);
+	pr_debug("---- end(%d) ----\n", err);
+	fflush(NULL);
+	return -err;
 }
 
-#define for_each_test(j, k, t)			\
-	for (j = 0, k = 0; j < ARRAY_SIZE(tests); j++, k = 0)	\
-		while ((t = tests[j][k++]) != NULL)
-
-static int test_and_print(struct test_suite *t, int subtest)
+static int print_test_result(struct test_suite *t, int i, int subtest, int result, int width)
 {
-	int err;
-
-	pr_debug("\n--- start ---\n");
-	err = run_test(t, subtest);
-	pr_debug("---- end ----\n");
+	if (has_subtests(t)) {
+		int subw = width > 2 ? width - 2 : width;
 
-	if (!has_subtests(t))
-		pr_debug("%s:", t->desc);
-	else
-		pr_debug("%s subtest %d:", t->desc, subtest + 1);
+		pr_info("%3d.%1d: %-*s:", i + 1, subtest + 1, subw, test_description(t, subtest));
+	} else
+		pr_info("%3d: %-*s:", i + 1, width, test_description(t, subtest));
 
-	switch (err) {
+	switch (result) {
 	case TEST_OK:
 		pr_info(" Ok\n");
 		break;
@@ -299,22 +266,135 @@ static int test_and_print(struct test_suite *t, int subtest)
 		break;
 	}
 
-	return err;
+	return 0;
+}
+
+static int finish_test(struct child_test *child_test, int width)
+{
+	struct test_suite *t = child_test->test;
+	int i = child_test->test_num;
+	int subi = child_test->subtest;
+	int out = child_test->process.out;
+	int err = child_test->process.err;
+	int ret;
+
+	if (verbose) {
+		bool out_done = false, err_done = false;
+
+		fcntl(out, F_SETFL, O_NONBLOCK);
+		fcntl(err, F_SETFL, O_NONBLOCK);
+		if (has_subtests(t))
+			pr_info("%3d.%1d: %s:\n", i + 1, subi + 1, test_description(t, subi));
+		else
+			pr_info("%3d: %s:\n", i + 1, test_description(t, -1));
+
+		while (!out_done && !err_done) {
+			char buf[512];
+			ssize_t len;
+
+			if (!out_done) {
+				errno = 0;
+				len = read(out, buf, sizeof(buf) - 1);
+
+				if (len <= 0)
+					err_done = errno != EAGAIN;
+				else {
+					buf[len] = '\0';
+					fprintf(stdout, "%s", buf);
+				}
+			}
+			if (!err_done) {
+				errno = 0;
+				len = read(err, buf, sizeof(buf) - 1);
+
+				if (len <= 0)
+					err_done = errno != EAGAIN;
+				else {
+					buf[len] = '\0';
+					fprintf(stderr, "%s", buf);
+				}
+			}
+		}
+	}
+	ret = finish_command(&child_test->process);
+	print_test_result(t, i, subi, ret, width);
+	if (out >= 0)
+		close(out);
+	if (err >= 0)
+		close(err);
+	return 0;
+}
+
+static int start_test(struct test_suite *test, int i, int subi, struct child_test **child,
+		      int width)
+{
+	int err;
+
+	*child = NULL;
+	if (dont_fork) {
+		pr_debug("--- start ---\n");
+		err = test_function(test, subi)(test, subi);
+		pr_debug("---- end ----\n");
+		print_test_result(test, i, subi, err, width);
+		return 0;
+	}
+
+	*child = zalloc(sizeof(**child));
+	if (!*child)
+		return -ENOMEM;
+
+	(*child)->test = test;
+	(*child)->test_num = i;
+	(*child)->subtest = subi;
+	(*child)->process.pid = -1;
+	(*child)->process.no_stdin = 1;
+	if (verbose <= 0) {
+		(*child)->process.no_stdout = 1;
+		(*child)->process.no_stderr = 1;
+	} else {
+		(*child)->process.out = -1;
+		(*child)->process.err = -1;
+	}
+	(*child)->process.no_exec_cmd = run_test_child;
+	err = start_command(&(*child)->process);
+	if (err || parallel)
+		return  err;
+	return finish_test(*child, width);
 }
 
+#define for_each_test(j, k, t)					\
+	for (j = 0, k = 0; j < ARRAY_SIZE(tests); j++, k = 0)	\
+		while ((t = tests[j][k++]) != NULL)
+
 static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
 {
 	struct test_suite *t;
 	unsigned int j, k;
 	int i = 0;
 	int width = 0;
+	size_t num_tests = 0;
+	struct child_test **child_tests;
+	int child_test_num = 0;
 
 	for_each_test(j, k, t) {
 		int len = strlen(test_description(t, -1));
 
 		if (width < len)
 			width = len;
+
+		if (has_subtests(t)) {
+			for (int l = 0, subn = num_subtests(t); l < subn; l++) {
+				len = strlen(test_description(t, -1));
+				if (width < len)
+					width = len;
+				num_tests++;
+			}
+		} else
+			num_tests++;
 	}
+	child_tests = calloc(num_tests, sizeof(*child_tests));
+	if (!child_tests)
+		return -ENOMEM;
 
 	for_each_test(j, k, t) {
 		int curr = i++;
@@ -336,52 +416,47 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
 				continue;
 		}
 
-		pr_info("%3d: %-*s:", i, width, test_description(t, -1));
-
 		if (intlist__find(skiplist, i)) {
+			pr_info("%3d: %-*s:", curr + 1, width, test_description(t, -1));
 			color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip (user override)\n");
 			continue;
 		}
 
 		if (!has_subtests(t)) {
-			test_and_print(t, -1);
+			int err = start_test(t, curr, -1, &child_tests[child_test_num++], width);
+
+			if (err) {
+				/* TODO: if parallel waitpid the already forked children. */
+				free(child_tests);
+				return err;
+			}
 		} else {
 			int subn = num_subtests(t);
-			/*
-			 * minus 2 to align with normal testcases.
-			 * For subtest we print additional '.x' in number.
-			 * for example:
-			 *
-			 * 35: Test LLVM searching and compiling                        :
-			 * 35.1: Basic BPF llvm compiling test                          : Ok
-			 */
-			int subw = width > 2 ? width - 2 : width;
-
-			if (subn <= 0) {
-				color_fprintf(stderr, PERF_COLOR_YELLOW,
-					      " Skip (not compiled in)\n");
-				continue;
-			}
-			pr_info("\n");
 
 			for (subi = 0; subi < subn; subi++) {
-				int len = strlen(test_description(t, subi));
-
-				if (subw < len)
-					subw = len;
-			}
+				int err;
 
-			for (subi = 0; subi < subn; subi++) {
 				if (!perf_test__matches(test_description(t, subi),
 							curr, argc, argv))
 					continue;
 
-				pr_info("%3d.%1d: %-*s:", i, subi + 1, subw,
-					test_description(t, subi));
-				test_and_print(t, subi);
+				err = start_test(t, curr, subi, &child_tests[child_test_num++],
+						 width);
+				if (err)
+					return err;
 			}
 		}
 	}
+	for (i = 0; i < child_test_num; i++) {
+		if (parallel) {
+			int ret  = finish_test(child_tests[i], width);
+
+			if (ret)
+				return ret;
+		}
+		free(child_tests[i]);
+	}
+	free(child_tests);
 	return 0;
 }
 
@@ -449,6 +524,8 @@ int cmd_test(int argc, const char **argv)
 		    "be more verbose (show symbol address, etc)"),
 	OPT_BOOLEAN('F', "dont-fork", &dont_fork,
 		    "Do not fork for testcase"),
+	OPT_BOOLEAN('p', "parallel", &parallel,
+		    "Run the tests altogether in parallel"),
 	OPT_STRING('w', "workload", &workload, "work", "workload to run for testing"),
 	OPT_STRING(0, "dso", &dso_to_test, "dso", "dso to test"),
 	OPT_STRING(0, "objdump", &test_objdump_path, "path",
-- 
2.43.0.rc2.451.g8631bc7472-goog

Re: [PATCH v1 9/9] perf tests: Add option to run tests in parallel

[Ian Rogers]

On Fri, Dec 1, 2023 at 3:50 PM Ian Rogers <irogers@google.com> wrote:
>
> By default tests are forked, add an option (-p or --parallel) so that
> the forked tests are all started in parallel and then their output
> gathered serially. This is opt-in as running in parallel can cause
> test flakes.
>
> Rather than fork within the code, the start_command/finish_command
> from libsubcmd are used. This changes how stderr and stdout are
> handled.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

Actually, I think this patch needs more work. The verbose output is
degraded and missing in some cases. Suggestions on how to fix welcome.
It'd be nice to fix up the tests and allow parallel to be the default.
The first patch in this series is 1 such fix. Another is needed to
make "Couldn't resolve comm name for pid" silent in the cases where it
is racy. I was also noticing hangs on things like the lock contention
test. The good news is the tests are doing their job of finding bugs.

Thanks,
Ian


> ---
>  tools/perf/tests/builtin-test.c | 261 +++++++++++++++++++++-----------
>  1 file changed, 169 insertions(+), 92 deletions(-)
>
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 54b11c23e863..91c32b477cbb 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -21,6 +21,7 @@
>  #include "debug.h"
>  #include "color.h"
>  #include <subcmd/parse-options.h>
> +#include <subcmd/run-command.h>
>  #include "string2.h"
>  #include "symbol.h"
>  #include "util/rlimit.h"
> @@ -31,7 +32,13 @@
>
>  #include "tests-scripts.h"
>
> +/*
> + * Command line option to not fork the test running in the same process and
> + * making them easier to debug.
> + */
>  static bool dont_fork;
> +/* Fork the tests in parallel and then wait for their completion. */
> +static bool parallel;
>  const char *dso_to_test;
>  const char *test_objdump_path = "objdump";
>
> @@ -211,76 +218,36 @@ static bool perf_test__matches(const char *desc, int curr, int argc, const char
>         return false;
>  }
>
> -static int run_test(struct test_suite *test, int subtest)
> -{
> -       int status, err = -1, child = dont_fork ? 0 : fork();
> -       char sbuf[STRERR_BUFSIZE];
> -
> -       if (child < 0) {
> -               pr_err("failed to fork test: %s\n",
> -                       str_error_r(errno, sbuf, sizeof(sbuf)));
> -               return -1;
> -       }
> -
> -       if (!child) {
> -               if (!dont_fork) {
> -                       pr_debug("test child forked, pid %d\n", getpid());
> -
> -                       if (verbose <= 0) {
> -                               int nullfd = open("/dev/null", O_WRONLY);
> -
> -                               if (nullfd >= 0) {
> -                                       close(STDERR_FILENO);
> -                                       close(STDOUT_FILENO);
> -
> -                                       dup2(nullfd, STDOUT_FILENO);
> -                                       dup2(STDOUT_FILENO, STDERR_FILENO);
> -                                       close(nullfd);
> -                               }
> -                       } else {
> -                               signal(SIGSEGV, sighandler_dump_stack);
> -                               signal(SIGFPE, sighandler_dump_stack);
> -                       }
> -               }
> -
> -               err = test_function(test, subtest)(test, subtest);
> -               if (!dont_fork)
> -                       exit(err);
> -       }
> -
> -       if (!dont_fork) {
> -               wait(&status);
> +struct child_test {
> +       struct child_process process;
> +       struct test_suite *test;
> +       int test_num;
> +       int subtest;
> +};
>
> -               if (WIFEXITED(status)) {
> -                       err = (signed char)WEXITSTATUS(status);
> -                       pr_debug("test child finished with %d\n", err);
> -               } else if (WIFSIGNALED(status)) {
> -                       err = -1;
> -                       pr_debug("test child interrupted\n");
> -               }
> -       }
> +static int run_test_child(struct child_process *process)
> +{
> +       struct child_test *child = container_of(process, struct child_test, process);
> +       int err;
>
> -       return err;
> +       pr_debug("--- start ---\n");
> +       pr_debug("test child forked, pid %d\n", getpid());
> +       err = test_function(child->test, child->subtest)(child->test, child->subtest);
> +       pr_debug("---- end(%d) ----\n", err);
> +       fflush(NULL);
> +       return -err;
>  }
>
> -#define for_each_test(j, k, t)                 \
> -       for (j = 0, k = 0; j < ARRAY_SIZE(tests); j++, k = 0)   \
> -               while ((t = tests[j][k++]) != NULL)
> -
> -static int test_and_print(struct test_suite *t, int subtest)
> +static int print_test_result(struct test_suite *t, int i, int subtest, int result, int width)
>  {
> -       int err;
> -
> -       pr_debug("\n--- start ---\n");
> -       err = run_test(t, subtest);
> -       pr_debug("---- end ----\n");
> +       if (has_subtests(t)) {
> +               int subw = width > 2 ? width - 2 : width;
>
> -       if (!has_subtests(t))
> -               pr_debug("%s:", t->desc);
> -       else
> -               pr_debug("%s subtest %d:", t->desc, subtest + 1);
> +               pr_info("%3d.%1d: %-*s:", i + 1, subtest + 1, subw, test_description(t, subtest));
> +       } else
> +               pr_info("%3d: %-*s:", i + 1, width, test_description(t, subtest));
>
> -       switch (err) {
> +       switch (result) {
>         case TEST_OK:
>                 pr_info(" Ok\n");
>                 break;
> @@ -299,22 +266,135 @@ static int test_and_print(struct test_suite *t, int subtest)
>                 break;
>         }
>
> -       return err;
> +       return 0;
> +}
> +
> +static int finish_test(struct child_test *child_test, int width)
> +{
> +       struct test_suite *t = child_test->test;
> +       int i = child_test->test_num;
> +       int subi = child_test->subtest;
> +       int out = child_test->process.out;
> +       int err = child_test->process.err;
> +       int ret;
> +
> +       if (verbose) {
> +               bool out_done = false, err_done = false;
> +
> +               fcntl(out, F_SETFL, O_NONBLOCK);
> +               fcntl(err, F_SETFL, O_NONBLOCK);
> +               if (has_subtests(t))
> +                       pr_info("%3d.%1d: %s:\n", i + 1, subi + 1, test_description(t, subi));
> +               else
> +                       pr_info("%3d: %s:\n", i + 1, test_description(t, -1));
> +
> +               while (!out_done && !err_done) {
> +                       char buf[512];
> +                       ssize_t len;
> +
> +                       if (!out_done) {
> +                               errno = 0;
> +                               len = read(out, buf, sizeof(buf) - 1);
> +
> +                               if (len <= 0)
> +                                       err_done = errno != EAGAIN;
> +                               else {
> +                                       buf[len] = '\0';
> +                                       fprintf(stdout, "%s", buf);
> +                               }
> +                       }
> +                       if (!err_done) {
> +                               errno = 0;
> +                               len = read(err, buf, sizeof(buf) - 1);
> +
> +                               if (len <= 0)
> +                                       err_done = errno != EAGAIN;
> +                               else {
> +                                       buf[len] = '\0';
> +                                       fprintf(stderr, "%s", buf);
> +                               }
> +                       }
> +               }
> +       }
> +       ret = finish_command(&child_test->process);
> +       print_test_result(t, i, subi, ret, width);
> +       if (out >= 0)
> +               close(out);
> +       if (err >= 0)
> +               close(err);
> +       return 0;
> +}
> +
> +static int start_test(struct test_suite *test, int i, int subi, struct child_test **child,
> +                     int width)
> +{
> +       int err;
> +
> +       *child = NULL;
> +       if (dont_fork) {
> +               pr_debug("--- start ---\n");
> +               err = test_function(test, subi)(test, subi);
> +               pr_debug("---- end ----\n");
> +               print_test_result(test, i, subi, err, width);
> +               return 0;
> +       }
> +
> +       *child = zalloc(sizeof(**child));
> +       if (!*child)
> +               return -ENOMEM;
> +
> +       (*child)->test = test;
> +       (*child)->test_num = i;
> +       (*child)->subtest = subi;
> +       (*child)->process.pid = -1;
> +       (*child)->process.no_stdin = 1;
> +       if (verbose <= 0) {
> +               (*child)->process.no_stdout = 1;
> +               (*child)->process.no_stderr = 1;
> +       } else {
> +               (*child)->process.out = -1;
> +               (*child)->process.err = -1;
> +       }
> +       (*child)->process.no_exec_cmd = run_test_child;
> +       err = start_command(&(*child)->process);
> +       if (err || parallel)
> +               return  err;
> +       return finish_test(*child, width);
>  }
>
> +#define for_each_test(j, k, t)                                 \
> +       for (j = 0, k = 0; j < ARRAY_SIZE(tests); j++, k = 0)   \
> +               while ((t = tests[j][k++]) != NULL)
> +
>  static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
>  {
>         struct test_suite *t;
>         unsigned int j, k;
>         int i = 0;
>         int width = 0;
> +       size_t num_tests = 0;
> +       struct child_test **child_tests;
> +       int child_test_num = 0;
>
>         for_each_test(j, k, t) {
>                 int len = strlen(test_description(t, -1));
>
>                 if (width < len)
>                         width = len;
> +
> +               if (has_subtests(t)) {
> +                       for (int l = 0, subn = num_subtests(t); l < subn; l++) {
> +                               len = strlen(test_description(t, -1));
> +                               if (width < len)
> +                                       width = len;
> +                               num_tests++;
> +                       }
> +               } else
> +                       num_tests++;
>         }
> +       child_tests = calloc(num_tests, sizeof(*child_tests));
> +       if (!child_tests)
> +               return -ENOMEM;
>
>         for_each_test(j, k, t) {
>                 int curr = i++;
> @@ -336,52 +416,47 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
>                                 continue;
>                 }
>
> -               pr_info("%3d: %-*s:", i, width, test_description(t, -1));
> -
>                 if (intlist__find(skiplist, i)) {
> +                       pr_info("%3d: %-*s:", curr + 1, width, test_description(t, -1));
>                         color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip (user override)\n");
>                         continue;
>                 }
>
>                 if (!has_subtests(t)) {
> -                       test_and_print(t, -1);
> +                       int err = start_test(t, curr, -1, &child_tests[child_test_num++], width);
> +
> +                       if (err) {
> +                               /* TODO: if parallel waitpid the already forked children. */
> +                               free(child_tests);
> +                               return err;
> +                       }
>                 } else {
>                         int subn = num_subtests(t);
> -                       /*
> -                        * minus 2 to align with normal testcases.
> -                        * For subtest we print additional '.x' in number.
> -                        * for example:
> -                        *
> -                        * 35: Test LLVM searching and compiling                        :
> -                        * 35.1: Basic BPF llvm compiling test                          : Ok
> -                        */
> -                       int subw = width > 2 ? width - 2 : width;
> -
> -                       if (subn <= 0) {
> -                               color_fprintf(stderr, PERF_COLOR_YELLOW,
> -                                             " Skip (not compiled in)\n");
> -                               continue;
> -                       }
> -                       pr_info("\n");
>
>                         for (subi = 0; subi < subn; subi++) {
> -                               int len = strlen(test_description(t, subi));
> -
> -                               if (subw < len)
> -                                       subw = len;
> -                       }
> +                               int err;
>
> -                       for (subi = 0; subi < subn; subi++) {
>                                 if (!perf_test__matches(test_description(t, subi),
>                                                         curr, argc, argv))
>                                         continue;
>
> -                               pr_info("%3d.%1d: %-*s:", i, subi + 1, subw,
> -                                       test_description(t, subi));
> -                               test_and_print(t, subi);
> +                               err = start_test(t, curr, subi, &child_tests[child_test_num++],
> +                                                width);
> +                               if (err)
> +                                       return err;
>                         }
>                 }
>         }
> +       for (i = 0; i < child_test_num; i++) {
> +               if (parallel) {
> +                       int ret  = finish_test(child_tests[i], width);
> +
> +                       if (ret)
> +                               return ret;
> +               }
> +               free(child_tests[i]);
> +       }
> +       free(child_tests);
>         return 0;
>  }
>
> @@ -449,6 +524,8 @@ int cmd_test(int argc, const char **argv)
>                     "be more verbose (show symbol address, etc)"),
>         OPT_BOOLEAN('F', "dont-fork", &dont_fork,
>                     "Do not fork for testcase"),
> +       OPT_BOOLEAN('p', "parallel", &parallel,
> +                   "Run the tests altogether in parallel"),
>         OPT_STRING('w', "workload", &workload, "work", "workload to run for testing"),
>         OPT_STRING(0, "dso", &dso_to_test, "dso", "dso to test"),
>         OPT_STRING(0, "objdump", &test_objdump_path, "path",
> --
> 2.43.0.rc2.451.g8631bc7472-goog
>

Re: [PATCH v1 3/9] perf tests: Avoid fork in perf_has_symbol test

[Adrian Hunter]

On 2/12/23 01:50, Ian Rogers wrote:
> perf test -vv Symbols is used to indentify symbols within the perf
> binary. Add the -F flag so that the test command doesn't fork the test
> before running. This removes a little overhead.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  tools/perf/tests/shell/lib/perf_has_symbol.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/tests/shell/lib/perf_has_symbol.sh b/tools/perf/tests/shell/lib/perf_has_symbol.sh
> index 5d59c32ae3e7..561c93b75d77 100644
> --- a/tools/perf/tests/shell/lib/perf_has_symbol.sh
> +++ b/tools/perf/tests/shell/lib/perf_has_symbol.sh
> @@ -3,7 +3,7 @@
>  
>  perf_has_symbol()
>  {
> -	if perf test -vv "Symbols" 2>&1 | grep "[[:space:]]$1$"; then
> +	if perf test -vv -F "Symbols" 2>&1 | grep "[[:space:]]$1$"; then
>  		echo "perf does have symbol '$1'"
>  		return 0
>  	fi

Re: [PATCH v1 9/9] perf tests: Add option to run tests in parallel

[Arnaldo Carvalho de Melo]

Em Fri, Dec 01, 2023 at 06:06:12PM -0800, Ian Rogers escreveu:
> On Fri, Dec 1, 2023 at 3:50 PM Ian Rogers <irogers@google.com> wrote:
> >
> > By default tests are forked, add an option (-p or --parallel) so that
> > the forked tests are all started in parallel and then their output
> > gathered serially. This is opt-in as running in parallel can cause
> > test flakes.
> >
> > Rather than fork within the code, the start_command/finish_command
> > from libsubcmd are used. This changes how stderr and stdout are
> > handled.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Actually, I think this patch needs more work. The verbose output is
> degraded and missing in some cases. Suggestions on how to fix welcome.

I'll think about, but to make progress I think the first 8 patches in
this series can be considered now?

- Arnaldo

> It'd be nice to fix up the tests and allow parallel to be the default.
> The first patch in this series is 1 such fix. Another is needed to
> make "Couldn't resolve comm name for pid" silent in the cases where it
> is racy. I was also noticing hangs on things like the lock contention
> test. The good news is the tests are doing their job of finding bugs.
> 
> Thanks,
> Ian
> 
> 
> > ---
> >  tools/perf/tests/builtin-test.c | 261 +++++++++++++++++++++-----------
> >  1 file changed, 169 insertions(+), 92 deletions(-)
> >
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index 54b11c23e863..91c32b477cbb 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -21,6 +21,7 @@
> >  #include "debug.h"
> >  #include "color.h"
> >  #include <subcmd/parse-options.h>
> > +#include <subcmd/run-command.h>
> >  #include "string2.h"
> >  #include "symbol.h"
> >  #include "util/rlimit.h"
> > @@ -31,7 +32,13 @@
> >
> >  #include "tests-scripts.h"
> >
> > +/*
> > + * Command line option to not fork the test running in the same process and
> > + * making them easier to debug.
> > + */
> >  static bool dont_fork;
> > +/* Fork the tests in parallel and then wait for their completion. */
> > +static bool parallel;
> >  const char *dso_to_test;
> >  const char *test_objdump_path = "objdump";
> >
> > @@ -211,76 +218,36 @@ static bool perf_test__matches(const char *desc, int curr, int argc, const char
> >         return false;
> >  }
> >
> > -static int run_test(struct test_suite *test, int subtest)
> > -{
> > -       int status, err = -1, child = dont_fork ? 0 : fork();
> > -       char sbuf[STRERR_BUFSIZE];
> > -
> > -       if (child < 0) {
> > -               pr_err("failed to fork test: %s\n",
> > -                       str_error_r(errno, sbuf, sizeof(sbuf)));
> > -               return -1;
> > -       }
> > -
> > -       if (!child) {
> > -               if (!dont_fork) {
> > -                       pr_debug("test child forked, pid %d\n", getpid());
> > -
> > -                       if (verbose <= 0) {
> > -                               int nullfd = open("/dev/null", O_WRONLY);
> > -
> > -                               if (nullfd >= 0) {
> > -                                       close(STDERR_FILENO);
> > -                                       close(STDOUT_FILENO);
> > -
> > -                                       dup2(nullfd, STDOUT_FILENO);
> > -                                       dup2(STDOUT_FILENO, STDERR_FILENO);
> > -                                       close(nullfd);
> > -                               }
> > -                       } else {
> > -                               signal(SIGSEGV, sighandler_dump_stack);
> > -                               signal(SIGFPE, sighandler_dump_stack);
> > -                       }
> > -               }
> > -
> > -               err = test_function(test, subtest)(test, subtest);
> > -               if (!dont_fork)
> > -                       exit(err);
> > -       }
> > -
> > -       if (!dont_fork) {
> > -               wait(&status);
> > +struct child_test {
> > +       struct child_process process;
> > +       struct test_suite *test;
> > +       int test_num;
> > +       int subtest;
> > +};
> >
> > -               if (WIFEXITED(status)) {
> > -                       err = (signed char)WEXITSTATUS(status);
> > -                       pr_debug("test child finished with %d\n", err);
> > -               } else if (WIFSIGNALED(status)) {
> > -                       err = -1;
> > -                       pr_debug("test child interrupted\n");
> > -               }
> > -       }
> > +static int run_test_child(struct child_process *process)
> > +{
> > +       struct child_test *child = container_of(process, struct child_test, process);
> > +       int err;
> >
> > -       return err;
> > +       pr_debug("--- start ---\n");
> > +       pr_debug("test child forked, pid %d\n", getpid());
> > +       err = test_function(child->test, child->subtest)(child->test, child->subtest);
> > +       pr_debug("---- end(%d) ----\n", err);
> > +       fflush(NULL);
> > +       return -err;
> >  }
> >
> > -#define for_each_test(j, k, t)                 \
> > -       for (j = 0, k = 0; j < ARRAY_SIZE(tests); j++, k = 0)   \
> > -               while ((t = tests[j][k++]) != NULL)
> > -
> > -static int test_and_print(struct test_suite *t, int subtest)
> > +static int print_test_result(struct test_suite *t, int i, int subtest, int result, int width)
> >  {
> > -       int err;
> > -
> > -       pr_debug("\n--- start ---\n");
> > -       err = run_test(t, subtest);
> > -       pr_debug("---- end ----\n");
> > +       if (has_subtests(t)) {
> > +               int subw = width > 2 ? width - 2 : width;
> >
> > -       if (!has_subtests(t))
> > -               pr_debug("%s:", t->desc);
> > -       else
> > -               pr_debug("%s subtest %d:", t->desc, subtest + 1);
> > +               pr_info("%3d.%1d: %-*s:", i + 1, subtest + 1, subw, test_description(t, subtest));
> > +       } else
> > +               pr_info("%3d: %-*s:", i + 1, width, test_description(t, subtest));
> >
> > -       switch (err) {
> > +       switch (result) {
> >         case TEST_OK:
> >                 pr_info(" Ok\n");
> >                 break;
> > @@ -299,22 +266,135 @@ static int test_and_print(struct test_suite *t, int subtest)
> >                 break;
> >         }
> >
> > -       return err;
> > +       return 0;
> > +}
> > +
> > +static int finish_test(struct child_test *child_test, int width)
> > +{
> > +       struct test_suite *t = child_test->test;
> > +       int i = child_test->test_num;
> > +       int subi = child_test->subtest;
> > +       int out = child_test->process.out;
> > +       int err = child_test->process.err;
> > +       int ret;
> > +
> > +       if (verbose) {
> > +               bool out_done = false, err_done = false;
> > +
> > +               fcntl(out, F_SETFL, O_NONBLOCK);
> > +               fcntl(err, F_SETFL, O_NONBLOCK);
> > +               if (has_subtests(t))
> > +                       pr_info("%3d.%1d: %s:\n", i + 1, subi + 1, test_description(t, subi));
> > +               else
> > +                       pr_info("%3d: %s:\n", i + 1, test_description(t, -1));
> > +
> > +               while (!out_done && !err_done) {
> > +                       char buf[512];
> > +                       ssize_t len;
> > +
> > +                       if (!out_done) {
> > +                               errno = 0;
> > +                               len = read(out, buf, sizeof(buf) - 1);
> > +
> > +                               if (len <= 0)
> > +                                       err_done = errno != EAGAIN;
> > +                               else {
> > +                                       buf[len] = '\0';
> > +                                       fprintf(stdout, "%s", buf);
> > +                               }
> > +                       }
> > +                       if (!err_done) {
> > +                               errno = 0;
> > +                               len = read(err, buf, sizeof(buf) - 1);
> > +
> > +                               if (len <= 0)
> > +                                       err_done = errno != EAGAIN;
> > +                               else {
> > +                                       buf[len] = '\0';
> > +                                       fprintf(stderr, "%s", buf);
> > +                               }
> > +                       }
> > +               }
> > +       }
> > +       ret = finish_command(&child_test->process);
> > +       print_test_result(t, i, subi, ret, width);
> > +       if (out >= 0)
> > +               close(out);
> > +       if (err >= 0)
> > +               close(err);
> > +       return 0;
> > +}
> > +
> > +static int start_test(struct test_suite *test, int i, int subi, struct child_test **child,
> > +                     int width)
> > +{
> > +       int err;
> > +
> > +       *child = NULL;
> > +       if (dont_fork) {
> > +               pr_debug("--- start ---\n");
> > +               err = test_function(test, subi)(test, subi);
> > +               pr_debug("---- end ----\n");
> > +               print_test_result(test, i, subi, err, width);
> > +               return 0;
> > +       }
> > +
> > +       *child = zalloc(sizeof(**child));
> > +       if (!*child)
> > +               return -ENOMEM;
> > +
> > +       (*child)->test = test;
> > +       (*child)->test_num = i;
> > +       (*child)->subtest = subi;
> > +       (*child)->process.pid = -1;
> > +       (*child)->process.no_stdin = 1;
> > +       if (verbose <= 0) {
> > +               (*child)->process.no_stdout = 1;
> > +               (*child)->process.no_stderr = 1;
> > +       } else {
> > +               (*child)->process.out = -1;
> > +               (*child)->process.err = -1;
> > +       }
> > +       (*child)->process.no_exec_cmd = run_test_child;
> > +       err = start_command(&(*child)->process);
> > +       if (err || parallel)
> > +               return  err;
> > +       return finish_test(*child, width);
> >  }
> >
> > +#define for_each_test(j, k, t)                                 \
> > +       for (j = 0, k = 0; j < ARRAY_SIZE(tests); j++, k = 0)   \
> > +               while ((t = tests[j][k++]) != NULL)
> > +
> >  static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
> >  {
> >         struct test_suite *t;
> >         unsigned int j, k;
> >         int i = 0;
> >         int width = 0;
> > +       size_t num_tests = 0;
> > +       struct child_test **child_tests;
> > +       int child_test_num = 0;
> >
> >         for_each_test(j, k, t) {
> >                 int len = strlen(test_description(t, -1));
> >
> >                 if (width < len)
> >                         width = len;
> > +
> > +               if (has_subtests(t)) {
> > +                       for (int l = 0, subn = num_subtests(t); l < subn; l++) {
> > +                               len = strlen(test_description(t, -1));
> > +                               if (width < len)
> > +                                       width = len;
> > +                               num_tests++;
> > +                       }
> > +               } else
> > +                       num_tests++;
> >         }
> > +       child_tests = calloc(num_tests, sizeof(*child_tests));
> > +       if (!child_tests)
> > +               return -ENOMEM;
> >
> >         for_each_test(j, k, t) {
> >                 int curr = i++;
> > @@ -336,52 +416,47 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
> >                                 continue;
> >                 }
> >
> > -               pr_info("%3d: %-*s:", i, width, test_description(t, -1));
> > -
> >                 if (intlist__find(skiplist, i)) {
> > +                       pr_info("%3d: %-*s:", curr + 1, width, test_description(t, -1));
> >                         color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip (user override)\n");
> >                         continue;
> >                 }
> >
> >                 if (!has_subtests(t)) {
> > -                       test_and_print(t, -1);
> > +                       int err = start_test(t, curr, -1, &child_tests[child_test_num++], width);
> > +
> > +                       if (err) {
> > +                               /* TODO: if parallel waitpid the already forked children. */
> > +                               free(child_tests);
> > +                               return err;
> > +                       }
> >                 } else {
> >                         int subn = num_subtests(t);
> > -                       /*
> > -                        * minus 2 to align with normal testcases.
> > -                        * For subtest we print additional '.x' in number.
> > -                        * for example:
> > -                        *
> > -                        * 35: Test LLVM searching and compiling                        :
> > -                        * 35.1: Basic BPF llvm compiling test                          : Ok
> > -                        */
> > -                       int subw = width > 2 ? width - 2 : width;
> > -
> > -                       if (subn <= 0) {
> > -                               color_fprintf(stderr, PERF_COLOR_YELLOW,
> > -                                             " Skip (not compiled in)\n");
> > -                               continue;
> > -                       }
> > -                       pr_info("\n");
> >
> >                         for (subi = 0; subi < subn; subi++) {
> > -                               int len = strlen(test_description(t, subi));
> > -
> > -                               if (subw < len)
> > -                                       subw = len;
> > -                       }
> > +                               int err;
> >
> > -                       for (subi = 0; subi < subn; subi++) {
> >                                 if (!perf_test__matches(test_description(t, subi),
> >                                                         curr, argc, argv))
> >                                         continue;
> >
> > -                               pr_info("%3d.%1d: %-*s:", i, subi + 1, subw,
> > -                                       test_description(t, subi));
> > -                               test_and_print(t, subi);
> > +                               err = start_test(t, curr, subi, &child_tests[child_test_num++],
> > +                                                width);
> > +                               if (err)
> > +                                       return err;
> >                         }
> >                 }
> >         }
> > +       for (i = 0; i < child_test_num; i++) {
> > +               if (parallel) {
> > +                       int ret  = finish_test(child_tests[i], width);
> > +
> > +                       if (ret)
> > +                               return ret;
> > +               }
> > +               free(child_tests[i]);
> > +       }
> > +       free(child_tests);
> >         return 0;
> >  }
> >
> > @@ -449,6 +524,8 @@ int cmd_test(int argc, const char **argv)
> >                     "be more verbose (show symbol address, etc)"),
> >         OPT_BOOLEAN('F', "dont-fork", &dont_fork,
> >                     "Do not fork for testcase"),
> > +       OPT_BOOLEAN('p', "parallel", &parallel,
> > +                   "Run the tests altogether in parallel"),
> >         OPT_STRING('w', "workload", &workload, "work", "workload to run for testing"),
> >         OPT_STRING(0, "dso", &dso_to_test, "dso", "dso to test"),
> >         OPT_STRING(0, "objdump", &test_objdump_path, "path",
> > --
> > 2.43.0.rc2.451.g8631bc7472-goog
> >

-- 

- Arnaldo

Re: [PATCH v1 9/9] perf tests: Add option to run tests in parallel

[Ian Rogers]

On Mon, Dec 4, 2023 at 12:30 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Fri, Dec 01, 2023 at 06:06:12PM -0800, Ian Rogers escreveu:
> > On Fri, Dec 1, 2023 at 3:50 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > By default tests are forked, add an option (-p or --parallel) so that
> > > the forked tests are all started in parallel and then their output
> > > gathered serially. This is opt-in as running in parallel can cause
> > > test flakes.
> > >
> > > Rather than fork within the code, the start_command/finish_command
> > > from libsubcmd are used. This changes how stderr and stdout are
> > > handled.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> >
> > Actually, I think this patch needs more work. The verbose output is
> > degraded and missing in some cases. Suggestions on how to fix welcome.
>
> I'll think about, but to make progress I think the first 8 patches in
> this series can be considered now?

That would be great. Thanks!

Ian