* [PATCH v3 1/8] perf thread_map: Skip exited threads when scanning /proc
2024-02-12 18:58 [PATCH v3 0/8] Run tests in parallel Ian Rogers
@ 2024-02-12 18:58 ` Ian Rogers
2024-02-12 18:58 ` [PATCH v3 2/8] perf list: Add scandirat compatibility function Ian Rogers
` (6 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Ian Rogers @ 2024-02-12 18:58 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, James Clark, Athira Jajeev,
Kan Liang, Yang Jihong, linux-kernel, linux-perf-users, llvm
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 ea3b431b9783..b5f12390c355 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.687.g38aa6559b0-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 2/8] perf list: Add scandirat compatibility function
2024-02-12 18:58 [PATCH v3 0/8] Run tests in parallel Ian Rogers
2024-02-12 18:58 ` [PATCH v3 1/8] perf thread_map: Skip exited threads when scanning /proc Ian Rogers
@ 2024-02-12 18:58 ` Ian Rogers
2024-02-12 18:58 ` [PATCH v3 3/8] perf tests: Avoid fork in perf_has_symbol test Ian Rogers
` (5 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Ian Rogers @ 2024-02-12 18:58 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, James Clark, Athira Jajeev,
Kan Liang, Yang Jihong, linux-kernel, linux-perf-users, llvm
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.687.g38aa6559b0-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 3/8] perf tests: Avoid fork in perf_has_symbol test
2024-02-12 18:58 [PATCH v3 0/8] Run tests in parallel Ian Rogers
2024-02-12 18:58 ` [PATCH v3 1/8] perf thread_map: Skip exited threads when scanning /proc Ian Rogers
2024-02-12 18:58 ` [PATCH v3 2/8] perf list: Add scandirat compatibility function Ian Rogers
@ 2024-02-12 18:58 ` Ian Rogers
2024-02-12 18:58 ` [PATCH v3 4/8] tools subcmd: Add a no exec function call option Ian Rogers
` (4 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Ian Rogers @ 2024-02-12 18:58 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, James Clark, Athira Jajeev,
Kan Liang, Yang Jihong, linux-kernel, linux-perf-users, llvm
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
--
2.43.0.687.g38aa6559b0-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 4/8] tools subcmd: Add a no exec function call option
2024-02-12 18:58 [PATCH v3 0/8] Run tests in parallel Ian Rogers
` (2 preceding siblings ...)
2024-02-12 18:58 ` [PATCH v3 3/8] perf tests: Avoid fork in perf_has_symbol test Ian Rogers
@ 2024-02-12 18:58 ` Ian Rogers
2024-02-12 18:58 ` [PATCH v3 5/8] perf test: Rename builtin-test-list and add missed header guard Ian Rogers
` (3 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Ian Rogers @ 2024-02-12 18:58 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, James Clark, Athira Jajeev,
Kan Liang, Yang Jihong, linux-kernel, linux-perf-users, llvm
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.687.g38aa6559b0-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 5/8] perf test: Rename builtin-test-list and add missed header guard
2024-02-12 18:58 [PATCH v3 0/8] Run tests in parallel Ian Rogers
` (3 preceding siblings ...)
2024-02-12 18:58 ` [PATCH v3 4/8] tools subcmd: Add a no exec function call option Ian Rogers
@ 2024-02-12 18:58 ` Ian Rogers
2024-02-12 18:58 ` [PATCH v3 6/8] perf tests: Use scandirat for shell script finding Ian Rogers
` (2 subsequent siblings)
7 siblings, 0 replies; 11+ messages in thread
From: Ian Rogers @ 2024-02-12 18:58 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, James Clark, Athira Jajeev,
Kan Liang, Yang Jihong, linux-kernel, linux-perf-users, llvm
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 53ba9c3e20e0..c7f9d9676095 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 4a5973f9bb9b..eff3c62e9b47 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.687.g38aa6559b0-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 6/8] perf tests: Use scandirat for shell script finding
2024-02-12 18:58 [PATCH v3 0/8] Run tests in parallel Ian Rogers
` (4 preceding siblings ...)
2024-02-12 18:58 ` [PATCH v3 5/8] perf test: Rename builtin-test-list and add missed header guard Ian Rogers
@ 2024-02-12 18:58 ` Ian Rogers
2024-02-12 18:58 ` [PATCH v3 7/8] perf tests: Run time generate shell test suites Ian Rogers
2024-02-12 18:58 ` [PATCH v3 8/8] perf tests: Add option to run tests in parallel Ian Rogers
7 siblings, 0 replies; 11+ messages in thread
From: Ian Rogers @ 2024-02-12 18:58 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, James Clark, Athira Jajeev,
Kan Liang, Yang Jihong, linux-kernel, linux-perf-users, llvm
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 eff3c62e9b47..6d5001daaf63 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -300,22 +300,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;
@@ -331,7 +329,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[] = {
{
@@ -345,13 +343,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)) {
@@ -455,7 +452,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.687.g38aa6559b0-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 7/8] perf tests: Run time generate shell test suites
2024-02-12 18:58 [PATCH v3 0/8] Run tests in parallel Ian Rogers
` (5 preceding siblings ...)
2024-02-12 18:58 ` [PATCH v3 6/8] perf tests: Use scandirat for shell script finding Ian Rogers
@ 2024-02-12 18:58 ` Ian Rogers
2024-02-12 18:58 ` [PATCH v3 8/8] perf tests: Add option to run tests in parallel Ian Rogers
7 siblings, 0 replies; 11+ messages in thread
From: Ian Rogers @ 2024-02-12 18:58 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, James Clark, Athira Jajeev,
Kan Liang, Yang Jihong, linux-kernel, linux-perf-users, llvm
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 | 130 ++++++++++++++++++-------------
tools/perf/tests/tests-scripts.h | 10 +--
3 files changed, 81 insertions(+), 150 deletions(-)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 6d5001daaf63..c42cb40fc242 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -130,6 +130,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[] = {
@@ -299,74 +300,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));
@@ -441,28 +380,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;
}
@@ -489,9 +406,6 @@ static int perf_test__list(int argc, const char **argv)
test_description(t, subi));
}
}
-
- perf_test__list_shell(argc, argv, i);
-
return 0;
}
@@ -551,6 +465,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..e60867b1e5ce 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,35 @@ 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 */
-
- dir_fd = shell_tests__dir_fd(); /* Walk dir */
- if (dir_fd < 0)
- return NULL;
+ struct test_suite **result = NULL, **result_tmp;
+ size_t result_sz = 0;
+ int dir_fd = shell_tests__dir_fd(); /* Walk dir */
- append_scripts_in_dir(dir_fd);
- close(dir_fd);
+ /*
+ * Append scripts if fd is good, otherwise return a NULL terminated zero
+ * length array.
+ */
+ if (dir_fd >= 0)
+ append_scripts_in_dir(dir_fd, &result, &result_sz);
- return files;
-}
-
-int list_script_max_width(void)
-{
- list_script_files(); /* Ensure we have scanned all scripts */
- return files_max_width;
+ 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;
+ if (dir_fd >= 0)
+ close(dir_fd);
+ 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.687.g38aa6559b0-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 8/8] perf tests: Add option to run tests in parallel
2024-02-12 18:58 [PATCH v3 0/8] Run tests in parallel Ian Rogers
` (6 preceding siblings ...)
2024-02-12 18:58 ` [PATCH v3 7/8] perf tests: Run time generate shell test suites Ian Rogers
@ 2024-02-12 18:58 ` Ian Rogers
2024-02-14 1:14 ` Namhyung Kim
7 siblings, 1 reply; 11+ messages in thread
From: Ian Rogers @ 2024-02-12 18:58 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Nathan Chancellor, Nick Desaulniers,
Bill Wendling, Justin Stitt, James Clark, Athira Jajeev,
Kan Liang, Yang Jihong, linux-kernel, linux-perf-users, llvm
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. The child stderr and stdout are always read to avoid the
child blocking. If verbose is 1 (-v) then if the test fails the child
stdout and stderr are displayed. If the verbose is >1 (e.g. -vv) then
the stdout and stderr from the child are immediately displayed.
An unscientific test on my laptop shows the wall clock time for perf
test without parallel being 5 minutes 21 seconds and with parallel
(-p) being 1 minute 50 seconds.
Signed-off-by: Ian Rogers <irogers@google.com>
---
v1 of this code had a bug where stdout/stderr weren't read fully. This
and additional issues/improvements are dealt with in v2.
---
tools/perf/tests/builtin-test.c | 305 ++++++++++++++++++++++----------
1 file changed, 213 insertions(+), 92 deletions(-)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index c42cb40fc242..b815db8ebf79 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -6,6 +6,7 @@
*/
#include <fcntl.h>
#include <errno.h>
+#include <poll.h>
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
@@ -21,9 +22,11 @@
#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"
+#include "util/strbuf.h"
#include <linux/kernel.h>
#include <linux/string.h>
#include <subcmd/exec-cmd.h>
@@ -31,7 +34,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";
@@ -209,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;
@@ -297,22 +266,177 @@ 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;
+ bool out_done = out <= 0;
+ bool err_done = err <= 0;
+ struct strbuf out_output = STRBUF_INIT;
+ struct strbuf err_output = STRBUF_INIT;
+ int ret;
+
+ /*
+ * For test suites with subtests, display the suite name ahead of the
+ * sub test names.
+ */
+ if (has_subtests(t) && subi == 0)
+ pr_info("%3d: %-*s:\n", i + 1, width, test_description(t, -1));
+
+ /*
+ * Busy loop reading from the child's stdout and stderr that are set to
+ * be non-blocking until EOF.
+ */
+ if (!out_done)
+ fcntl(out, F_SETFL, O_NONBLOCK);
+ if (!err_done)
+ fcntl(err, F_SETFL, O_NONBLOCK);
+ if (verbose > 1) {
+ 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) {
+ struct pollfd pfds[2] = {
+ { .fd = out,
+ .events = POLLIN | POLLERR | POLLHUP | POLLNVAL,
+ },
+ { .fd = err,
+ .events = POLLIN | POLLERR | POLLHUP | POLLNVAL,
+ },
+ };
+ char buf[512];
+ ssize_t len;
+
+ /* Poll to avoid excessive spinning, timeout set for 1000ms. */
+ poll(pfds, ARRAY_SIZE(pfds), /*timeout=*/1000);
+ if (!out_done && pfds[0].revents) {
+ errno = 0;
+ len = read(out, buf, sizeof(buf) - 1);
+
+ if (len <= 0) {
+ out_done = errno != EAGAIN;
+ } else {
+ buf[len] = '\0';
+ if (verbose > 1)
+ fprintf(stdout, "%s", buf);
+ else
+ strbuf_addstr(&out_output, buf);
+ }
+ }
+ if (!err_done && pfds[1].revents) {
+ errno = 0;
+ len = read(err, buf, sizeof(buf) - 1);
+
+ if (len <= 0) {
+ err_done = errno != EAGAIN;
+ } else {
+ buf[len] = '\0';
+ if (verbose > 1)
+ fprintf(stdout, "%s", buf);
+ else
+ strbuf_addstr(&err_output, buf);
+ }
+ }
+ }
+ /* Clean up child process. */
+ ret = finish_command(&child_test->process);
+ if (verbose == 1 && ret == TEST_FAIL) {
+ /* Add header for test that was skipped above. */
+ 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));
+ fprintf(stdout, "%s", out_output.buf);
+ fprintf(stderr, "%s", err_output.buf);
+ }
+ strbuf_release(&out_output);
+ strbuf_release(&err_output);
+ 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++;
@@ -334,52 +458,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));
+ int err;
- if (subw < len)
- subw = len;
- }
-
- 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;
}
@@ -447,6 +566,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", ¶llel,
+ "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.687.g38aa6559b0-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 8/8] perf tests: Add option to run tests in parallel
2024-02-12 18:58 ` [PATCH v3 8/8] perf tests: Add option to run tests in parallel Ian Rogers
@ 2024-02-14 1:14 ` Namhyung Kim
2024-02-14 4:33 ` Ian Rogers
0 siblings, 1 reply; 11+ messages in thread
From: Namhyung Kim @ 2024-02-14 1:14 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
James Clark, Athira Jajeev, Kan Liang, Yang Jihong, linux-kernel,
linux-perf-users, llvm
On Mon, Feb 12, 2024 at 10:59 AM 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. The child stderr and stdout are always read to avoid the
> child blocking. If verbose is 1 (-v) then if the test fails the child
> stdout and stderr are displayed. If the verbose is >1 (e.g. -vv) then
> the stdout and stderr from the child are immediately displayed.
>
> An unscientific test on my laptop shows the wall clock time for perf
> test without parallel being 5 minutes 21 seconds and with parallel
> (-p) being 1 minute 50 seconds.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> v1 of this code had a bug where stdout/stderr weren't read fully. This
> and additional issues/improvements are dealt with in v2.
> ---
[SNIP]
> 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));
Shouldn't it be strlen(test_description(t, i)) ? Looks like it has len
of parent test already.
Thanks,
Namhyung
> + 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++;
> @@ -334,52 +458,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));
> + int err;
>
> - if (subw < len)
> - subw = len;
> - }
> -
> - 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;
> }
>
> @@ -447,6 +566,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", ¶llel,
> + "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.687.g38aa6559b0-goog
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 8/8] perf tests: Add option to run tests in parallel
2024-02-14 1:14 ` Namhyung Kim
@ 2024-02-14 4:33 ` Ian Rogers
0 siblings, 0 replies; 11+ messages in thread
From: Ian Rogers @ 2024-02-14 4:33 UTC (permalink / raw)
To: Namhyung Kim
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
James Clark, Athira Jajeev, Kan Liang, Yang Jihong, linux-kernel,
linux-perf-users, llvm
On Tue, Feb 13, 2024 at 5:14 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Feb 12, 2024 at 10:59 AM 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. The child stderr and stdout are always read to avoid the
> > child blocking. If verbose is 1 (-v) then if the test fails the child
> > stdout and stderr are displayed. If the verbose is >1 (e.g. -vv) then
> > the stdout and stderr from the child are immediately displayed.
> >
> > An unscientific test on my laptop shows the wall clock time for perf
> > test without parallel being 5 minutes 21 seconds and with parallel
> > (-p) being 1 minute 50 seconds.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > v1 of this code had a bug where stdout/stderr weren't read fully. This
> > and additional issues/improvements are dealt with in v2.
> > ---
> [SNIP]
> > 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));
>
> Shouldn't it be strlen(test_description(t, i)) ? Looks like it has len
> of parent test already.
>
> Thanks,
> Namhyung
Thanks Namhyung, will fix in v4.
Ian
> > + 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++;
> > @@ -334,52 +458,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));
> > + int err;
> >
> > - if (subw < len)
> > - subw = len;
> > - }
> > -
> > - 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;
> > }
> >
> > @@ -447,6 +566,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", ¶llel,
> > + "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.687.g38aa6559b0-goog
> >
^ permalink raw reply [flat|nested] 11+ messages in thread