* [PATCH] perf test: iterate over shell tests in alphabetical order
@ 2021-05-25 23:05 Riccardo Mancini
2021-05-25 23:25 ` Ian Rogers
2021-05-26 12:33 ` Jiri Olsa
0 siblings, 2 replies; 6+ messages in thread
From: Riccardo Mancini @ 2021-05-25 23:05 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers
Cc: Riccardo Mancini, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Leo Yan, Fabian Hemmer,
Tommi Rantala, Stephane Eranian, linux-perf-users, linux-kernel
for_each_shell_test macro iterated over all shell tests in the directory
using readdir, which does not guarantee any ordering, causing
problems on certain fs. However, the order in which they are visited
determines the id of the test, in case one wants to run a single test.
This patch replaces readdir with scandir using alphabetical sorting.
This guarantees that, given the same set of tests, all machines will
see the tests in the same order, and, thus, that test ids are
consistent.
Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Ian Rogers <irogers@google.com>
---
tools/perf/tests/builtin-test.c | 38 ++++++++++++++++++---------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index c4b888f18e9ca..cbbfe48ab8029 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -510,8 +510,8 @@ static const char *shell_test__description(char *description, size_t size,
return description ? strim(description + 1) : NULL;
}
-#define for_each_shell_test(dir, base, ent) \
- while ((ent = readdir(dir)) != NULL) \
+#define for_each_shell_test(entlist, nr, base, ent) \
+ for (int __i = 0; __i < nr && (ent = entlist[__i]); __i++) \
if (!is_directory(base, ent) && ent->d_name[0] != '.')
static const char *shell_tests__dir(char *path, size_t size)
@@ -538,8 +538,9 @@ static const char *shell_tests__dir(char *path, size_t size)
static int shell_tests__max_desc_width(void)
{
- DIR *dir;
+ struct dirent **entlist;
struct dirent *ent;
+ int n_dirs;
char path_dir[PATH_MAX];
const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
int width = 0;
@@ -547,11 +548,11 @@ static int shell_tests__max_desc_width(void)
if (path == NULL)
return -1;
- dir = opendir(path);
- if (!dir)
+ n_dirs = scandir(path, &entlist, NULL, alphasort);
+ if (n_dirs == -1)
return -1;
- for_each_shell_test(dir, path, ent) {
+ for_each_shell_test(entlist, n_dirs, path, ent) {
char bf[256];
const char *desc = shell_test__description(bf, sizeof(bf), path, ent->d_name);
@@ -563,7 +564,8 @@ static int shell_tests__max_desc_width(void)
}
}
- closedir(dir);
+ free(entlist);
+
return width;
}
@@ -589,8 +591,9 @@ static int shell_test__run(struct test *test, int subdir __maybe_unused)
static int run_shell_tests(int argc, const char *argv[], int i, int width)
{
- DIR *dir;
+ struct dirent **entlist;
struct dirent *ent;
+ int n_dirs;
char path_dir[PATH_MAX];
struct shell_test st = {
.dir = shell_tests__dir(path_dir, sizeof(path_dir)),
@@ -599,14 +602,14 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
if (st.dir == NULL)
return -1;
- dir = opendir(st.dir);
- if (!dir) {
+ n_dirs = scandir(st.dir, &entlist, NULL, alphasort);
+ if (n_dirs == -1) {
pr_err("failed to open shell test directory: %s\n",
st.dir);
return -1;
}
- for_each_shell_test(dir, st.dir, ent) {
+ for_each_shell_test(entlist, n_dirs, st.dir, ent) {
int curr = i++;
char desc[256];
struct test test = {
@@ -623,7 +626,7 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
test_and_print(&test, false, -1);
}
- closedir(dir);
+ free(entlist);
return 0;
}
@@ -722,19 +725,20 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
static int perf_test__list_shell(int argc, const char **argv, int i)
{
- DIR *dir;
+ struct dirent **entlist;
struct dirent *ent;
+ int n_dirs;
char path_dir[PATH_MAX];
const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
if (path == NULL)
return -1;
- dir = opendir(path);
- if (!dir)
+ n_dirs = scandir(path, &entlist, NULL, alphasort);
+ if (n_dirs == -1)
return -1;
- for_each_shell_test(dir, path, ent) {
+ for_each_shell_test(entlist, n_dirs, path, ent) {
int curr = i++;
char bf[256];
struct test t = {
@@ -747,7 +751,7 @@ static int perf_test__list_shell(int argc, const char **argv, int i)
pr_info("%2d: %s\n", i, t.desc);
}
- closedir(dir);
+ free(entlist);
return 0;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] perf test: iterate over shell tests in alphabetical order
2021-05-25 23:05 [PATCH] perf test: iterate over shell tests in alphabetical order Riccardo Mancini
@ 2021-05-25 23:25 ` Ian Rogers
2021-05-26 12:36 ` Jiri Olsa
` (2 more replies)
2021-05-26 12:33 ` Jiri Olsa
1 sibling, 3 replies; 6+ messages in thread
From: Ian Rogers @ 2021-05-25 23:25 UTC (permalink / raw)
To: Riccardo Mancini
Cc: Arnaldo Carvalho de Melo, Namhyung Kim, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Leo Yan, Fabian Hemmer, Tommi Rantala, Stephane Eranian,
linux-perf-users, LKML
On Tue, May 25, 2021 at 4:08 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
>
> for_each_shell_test macro iterated over all shell tests in the directory
> using readdir, which does not guarantee any ordering, causing
> problems on certain fs. However, the order in which they are visited
> determines the id of the test, in case one wants to run a single test.
>
> This patch replaces readdir with scandir using alphabetical sorting.
> This guarantees that, given the same set of tests, all machines will
> see the tests in the same order, and, thus, that test ids are
> consistent.
>
> Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Ian Rogers <irogers@google.com>
Acked-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/tests/builtin-test.c | 38 ++++++++++++++++++---------------
> 1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index c4b888f18e9ca..cbbfe48ab8029 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -510,8 +510,8 @@ static const char *shell_test__description(char *description, size_t size,
> return description ? strim(description + 1) : NULL;
> }
>
> -#define for_each_shell_test(dir, base, ent) \
> - while ((ent = readdir(dir)) != NULL) \
> +#define for_each_shell_test(entlist, nr, base, ent) \
> + for (int __i = 0; __i < nr && (ent = entlist[__i]); __i++) \
I think this declaration of __i is okay as C99 allows declarations
here, but generally I don't see this style in the kernel.
Thanks,
Ian
> if (!is_directory(base, ent) && ent->d_name[0] != '.')
>
> static const char *shell_tests__dir(char *path, size_t size)
> @@ -538,8 +538,9 @@ static const char *shell_tests__dir(char *path, size_t size)
>
> static int shell_tests__max_desc_width(void)
> {
> - DIR *dir;
> + struct dirent **entlist;
> struct dirent *ent;
> + int n_dirs;
> char path_dir[PATH_MAX];
> const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
> int width = 0;
> @@ -547,11 +548,11 @@ static int shell_tests__max_desc_width(void)
> if (path == NULL)
> return -1;
>
> - dir = opendir(path);
> - if (!dir)
> + n_dirs = scandir(path, &entlist, NULL, alphasort);
> + if (n_dirs == -1)
> return -1;
>
> - for_each_shell_test(dir, path, ent) {
> + for_each_shell_test(entlist, n_dirs, path, ent) {
> char bf[256];
> const char *desc = shell_test__description(bf, sizeof(bf), path, ent->d_name);
>
> @@ -563,7 +564,8 @@ static int shell_tests__max_desc_width(void)
> }
> }
>
> - closedir(dir);
> + free(entlist);
> +
> return width;
> }
>
> @@ -589,8 +591,9 @@ static int shell_test__run(struct test *test, int subdir __maybe_unused)
>
> static int run_shell_tests(int argc, const char *argv[], int i, int width)
> {
> - DIR *dir;
> + struct dirent **entlist;
> struct dirent *ent;
> + int n_dirs;
> char path_dir[PATH_MAX];
> struct shell_test st = {
> .dir = shell_tests__dir(path_dir, sizeof(path_dir)),
> @@ -599,14 +602,14 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
> if (st.dir == NULL)
> return -1;
>
> - dir = opendir(st.dir);
> - if (!dir) {
> + n_dirs = scandir(st.dir, &entlist, NULL, alphasort);
> + if (n_dirs == -1) {
> pr_err("failed to open shell test directory: %s\n",
> st.dir);
> return -1;
> }
>
> - for_each_shell_test(dir, st.dir, ent) {
> + for_each_shell_test(entlist, n_dirs, st.dir, ent) {
> int curr = i++;
> char desc[256];
> struct test test = {
> @@ -623,7 +626,7 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
> test_and_print(&test, false, -1);
> }
>
> - closedir(dir);
> + free(entlist);
> return 0;
> }
>
> @@ -722,19 +725,20 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
>
> static int perf_test__list_shell(int argc, const char **argv, int i)
> {
> - DIR *dir;
> + struct dirent **entlist;
> struct dirent *ent;
> + int n_dirs;
> char path_dir[PATH_MAX];
> const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
>
> if (path == NULL)
> return -1;
>
> - dir = opendir(path);
> - if (!dir)
> + n_dirs = scandir(path, &entlist, NULL, alphasort);
> + if (n_dirs == -1)
> return -1;
>
> - for_each_shell_test(dir, path, ent) {
> + for_each_shell_test(entlist, n_dirs, path, ent) {
> int curr = i++;
> char bf[256];
> struct test t = {
> @@ -747,7 +751,7 @@ static int perf_test__list_shell(int argc, const char **argv, int i)
> pr_info("%2d: %s\n", i, t.desc);
> }
>
> - closedir(dir);
> + free(entlist);
> return 0;
> }
>
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf test: iterate over shell tests in alphabetical order
2021-05-25 23:05 [PATCH] perf test: iterate over shell tests in alphabetical order Riccardo Mancini
2021-05-25 23:25 ` Ian Rogers
@ 2021-05-26 12:33 ` Jiri Olsa
1 sibling, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2021-05-26 12:33 UTC (permalink / raw)
To: Riccardo Mancini
Cc: Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers,
Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Leo Yan, Fabian Hemmer, Tommi Rantala, Stephane Eranian,
linux-perf-users, linux-kernel
On Wed, May 26, 2021 at 01:05:17AM +0200, Riccardo Mancini wrote:
> for_each_shell_test macro iterated over all shell tests in the directory
> using readdir, which does not guarantee any ordering, causing
> problems on certain fs. However, the order in which they are visited
> determines the id of the test, in case one wants to run a single test.
>
> This patch replaces readdir with scandir using alphabetical sorting.
> This guarantees that, given the same set of tests, all machines will
> see the tests in the same order, and, thus, that test ids are
> consistent.
>
> Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Ian Rogers <irogers@google.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
thanks,
jirka
> ---
> tools/perf/tests/builtin-test.c | 38 ++++++++++++++++++---------------
> 1 file changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index c4b888f18e9ca..cbbfe48ab8029 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -510,8 +510,8 @@ static const char *shell_test__description(char *description, size_t size,
> return description ? strim(description + 1) : NULL;
> }
>
> -#define for_each_shell_test(dir, base, ent) \
> - while ((ent = readdir(dir)) != NULL) \
> +#define for_each_shell_test(entlist, nr, base, ent) \
> + for (int __i = 0; __i < nr && (ent = entlist[__i]); __i++) \
> if (!is_directory(base, ent) && ent->d_name[0] != '.')
>
> static const char *shell_tests__dir(char *path, size_t size)
> @@ -538,8 +538,9 @@ static const char *shell_tests__dir(char *path, size_t size)
>
> static int shell_tests__max_desc_width(void)
> {
> - DIR *dir;
> + struct dirent **entlist;
> struct dirent *ent;
> + int n_dirs;
> char path_dir[PATH_MAX];
> const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
> int width = 0;
> @@ -547,11 +548,11 @@ static int shell_tests__max_desc_width(void)
> if (path == NULL)
> return -1;
>
> - dir = opendir(path);
> - if (!dir)
> + n_dirs = scandir(path, &entlist, NULL, alphasort);
> + if (n_dirs == -1)
> return -1;
>
> - for_each_shell_test(dir, path, ent) {
> + for_each_shell_test(entlist, n_dirs, path, ent) {
> char bf[256];
> const char *desc = shell_test__description(bf, sizeof(bf), path, ent->d_name);
>
> @@ -563,7 +564,8 @@ static int shell_tests__max_desc_width(void)
> }
> }
>
> - closedir(dir);
> + free(entlist);
> +
> return width;
> }
>
> @@ -589,8 +591,9 @@ static int shell_test__run(struct test *test, int subdir __maybe_unused)
>
> static int run_shell_tests(int argc, const char *argv[], int i, int width)
> {
> - DIR *dir;
> + struct dirent **entlist;
> struct dirent *ent;
> + int n_dirs;
> char path_dir[PATH_MAX];
> struct shell_test st = {
> .dir = shell_tests__dir(path_dir, sizeof(path_dir)),
> @@ -599,14 +602,14 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
> if (st.dir == NULL)
> return -1;
>
> - dir = opendir(st.dir);
> - if (!dir) {
> + n_dirs = scandir(st.dir, &entlist, NULL, alphasort);
> + if (n_dirs == -1) {
> pr_err("failed to open shell test directory: %s\n",
> st.dir);
> return -1;
> }
>
> - for_each_shell_test(dir, st.dir, ent) {
> + for_each_shell_test(entlist, n_dirs, st.dir, ent) {
> int curr = i++;
> char desc[256];
> struct test test = {
> @@ -623,7 +626,7 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
> test_and_print(&test, false, -1);
> }
>
> - closedir(dir);
> + free(entlist);
> return 0;
> }
>
> @@ -722,19 +725,20 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
>
> static int perf_test__list_shell(int argc, const char **argv, int i)
> {
> - DIR *dir;
> + struct dirent **entlist;
> struct dirent *ent;
> + int n_dirs;
> char path_dir[PATH_MAX];
> const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
>
> if (path == NULL)
> return -1;
>
> - dir = opendir(path);
> - if (!dir)
> + n_dirs = scandir(path, &entlist, NULL, alphasort);
> + if (n_dirs == -1)
> return -1;
>
> - for_each_shell_test(dir, path, ent) {
> + for_each_shell_test(entlist, n_dirs, path, ent) {
> int curr = i++;
> char bf[256];
> struct test t = {
> @@ -747,7 +751,7 @@ static int perf_test__list_shell(int argc, const char **argv, int i)
> pr_info("%2d: %s\n", i, t.desc);
> }
>
> - closedir(dir);
> + free(entlist);
> return 0;
> }
>
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf test: iterate over shell tests in alphabetical order
2021-05-25 23:25 ` Ian Rogers
@ 2021-05-26 12:36 ` Jiri Olsa
2021-05-26 13:57 ` Arnaldo Carvalho de Melo
2021-05-26 13:58 ` Arnaldo Carvalho de Melo
2 siblings, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2021-05-26 12:36 UTC (permalink / raw)
To: Ian Rogers
Cc: Riccardo Mancini, Arnaldo Carvalho de Melo, Namhyung Kim,
Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Leo Yan, Fabian Hemmer, Tommi Rantala, Stephane Eranian,
linux-perf-users, LKML
On Tue, May 25, 2021 at 04:25:48PM -0700, Ian Rogers wrote:
> On Tue, May 25, 2021 at 4:08 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
> >
> > for_each_shell_test macro iterated over all shell tests in the directory
> > using readdir, which does not guarantee any ordering, causing
> > problems on certain fs. However, the order in which they are visited
> > determines the id of the test, in case one wants to run a single test.
> >
> > This patch replaces readdir with scandir using alphabetical sorting.
> > This guarantees that, given the same set of tests, all machines will
> > see the tests in the same order, and, thus, that test ids are
> > consistent.
> >
> > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Ian Rogers <irogers@google.com>
>
> Acked-by: Ian Rogers <irogers@google.com>
>
> > ---
> > tools/perf/tests/builtin-test.c | 38 ++++++++++++++++++---------------
> > 1 file changed, 21 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index c4b888f18e9ca..cbbfe48ab8029 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -510,8 +510,8 @@ static const char *shell_test__description(char *description, size_t size,
> > return description ? strim(description + 1) : NULL;
> > }
> >
> > -#define for_each_shell_test(dir, base, ent) \
> > - while ((ent = readdir(dir)) != NULL) \
> > +#define for_each_shell_test(entlist, nr, base, ent) \
> > + for (int __i = 0; __i < nr && (ent = entlist[__i]); __i++) \
>
> I think this declaration of __i is okay as C99 allows declarations
> here, but generally I don't see this style in the kernel.
I think it's ok, we have some instances of this in perf already..
otherwise we'd need to pass extra argument to the macro
jirka
>
> Thanks,
> Ian
>
> > if (!is_directory(base, ent) && ent->d_name[0] != '.')
> >
> > static const char *shell_tests__dir(char *path, size_t size)
> > @@ -538,8 +538,9 @@ static const char *shell_tests__dir(char *path, size_t size)
> >
> > static int shell_tests__max_desc_width(void)
> > {
> > - DIR *dir;
> > + struct dirent **entlist;
> > struct dirent *ent;
> > + int n_dirs;
> > char path_dir[PATH_MAX];
> > const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
> > int width = 0;
> > @@ -547,11 +548,11 @@ static int shell_tests__max_desc_width(void)
> > if (path == NULL)
> > return -1;
> >
> > - dir = opendir(path);
> > - if (!dir)
> > + n_dirs = scandir(path, &entlist, NULL, alphasort);
> > + if (n_dirs == -1)
> > return -1;
> >
> > - for_each_shell_test(dir, path, ent) {
> > + for_each_shell_test(entlist, n_dirs, path, ent) {
> > char bf[256];
> > const char *desc = shell_test__description(bf, sizeof(bf), path, ent->d_name);
> >
> > @@ -563,7 +564,8 @@ static int shell_tests__max_desc_width(void)
> > }
> > }
> >
> > - closedir(dir);
> > + free(entlist);
> > +
> > return width;
> > }
> >
> > @@ -589,8 +591,9 @@ static int shell_test__run(struct test *test, int subdir __maybe_unused)
> >
> > static int run_shell_tests(int argc, const char *argv[], int i, int width)
> > {
> > - DIR *dir;
> > + struct dirent **entlist;
> > struct dirent *ent;
> > + int n_dirs;
> > char path_dir[PATH_MAX];
> > struct shell_test st = {
> > .dir = shell_tests__dir(path_dir, sizeof(path_dir)),
> > @@ -599,14 +602,14 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
> > if (st.dir == NULL)
> > return -1;
> >
> > - dir = opendir(st.dir);
> > - if (!dir) {
> > + n_dirs = scandir(st.dir, &entlist, NULL, alphasort);
> > + if (n_dirs == -1) {
> > pr_err("failed to open shell test directory: %s\n",
> > st.dir);
> > return -1;
> > }
> >
> > - for_each_shell_test(dir, st.dir, ent) {
> > + for_each_shell_test(entlist, n_dirs, st.dir, ent) {
> > int curr = i++;
> > char desc[256];
> > struct test test = {
> > @@ -623,7 +626,7 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
> > test_and_print(&test, false, -1);
> > }
> >
> > - closedir(dir);
> > + free(entlist);
> > return 0;
> > }
> >
> > @@ -722,19 +725,20 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
> >
> > static int perf_test__list_shell(int argc, const char **argv, int i)
> > {
> > - DIR *dir;
> > + struct dirent **entlist;
> > struct dirent *ent;
> > + int n_dirs;
> > char path_dir[PATH_MAX];
> > const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
> >
> > if (path == NULL)
> > return -1;
> >
> > - dir = opendir(path);
> > - if (!dir)
> > + n_dirs = scandir(path, &entlist, NULL, alphasort);
> > + if (n_dirs == -1)
> > return -1;
> >
> > - for_each_shell_test(dir, path, ent) {
> > + for_each_shell_test(entlist, n_dirs, path, ent) {
> > int curr = i++;
> > char bf[256];
> > struct test t = {
> > @@ -747,7 +751,7 @@ static int perf_test__list_shell(int argc, const char **argv, int i)
> > pr_info("%2d: %s\n", i, t.desc);
> > }
> >
> > - closedir(dir);
> > + free(entlist);
> > return 0;
> > }
> >
> > --
> > 2.31.1
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf test: iterate over shell tests in alphabetical order
2021-05-25 23:25 ` Ian Rogers
2021-05-26 12:36 ` Jiri Olsa
@ 2021-05-26 13:57 ` Arnaldo Carvalho de Melo
2021-05-26 13:58 ` Arnaldo Carvalho de Melo
2 siblings, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-05-26 13:57 UTC (permalink / raw)
To: Ian Rogers
Cc: Riccardo Mancini, Namhyung Kim, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Leo Yan,
Fabian Hemmer, Tommi Rantala, Stephane Eranian, linux-perf-users,
LKML
Em Tue, May 25, 2021 at 04:25:48PM -0700, Ian Rogers escreveu:
> On Tue, May 25, 2021 at 4:08 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
> >
> > for_each_shell_test macro iterated over all shell tests in the directory
> > using readdir, which does not guarantee any ordering, causing
> > problems on certain fs. However, the order in which they are visited
> > determines the id of the test, in case one wants to run a single test.
> >
> > This patch replaces readdir with scandir using alphabetical sorting.
> > This guarantees that, given the same set of tests, all machines will
> > see the tests in the same order, and, thus, that test ids are
> > consistent.
> >
> > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Ian Rogers <irogers@google.com>
>
> Acked-by: Ian Rogers <irogers@google.com>
>
> > ---
> > tools/perf/tests/builtin-test.c | 38 ++++++++++++++++++---------------
> > 1 file changed, 21 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index c4b888f18e9ca..cbbfe48ab8029 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -510,8 +510,8 @@ static const char *shell_test__description(char *description, size_t size,
> > return description ? strim(description + 1) : NULL;
> > }
> >
> > -#define for_each_shell_test(dir, base, ent) \
> > - while ((ent = readdir(dir)) != NULL) \
[acme@five perf]$ grep '(int i =' */*/*.c
tools/perf/builtin-diff.c: for (int i = 0; i < num; i++)
tools/perf/builtin-diff.c: for (int i = 0; i < pair->block_info->num; i++) {
tools/perf/builtin-stat.c: for (int i = 0; i < a->core.cpus->nr; i++) {
[acme@five perf]$ grep '(int i =' */*/*/*.c
tools/lib/subcmd/parse-options.c: for (int i = 0; subcommands[i]; i++) {
tools/lib/subcmd/parse-options.c: for (int i = 0; subcommands[i]; i++)
tools/perf/util/block-info.c: for (int i = 0; i < nr_hpps; i++)
tools/perf/util/block-info.c: for (int i = 0; i < nr_hpps; i++) {
tools/perf/util/block-info.c: for (int i = 0; i < nr_reps; i++)
tools/perf/util/header.c: for (int i = 0; i < ff->ph->env.nr_hybrid_cpc_nodes; i++) {
tools/perf/util/stat-display.c: for (int i = 0; i < evsel__nr_cpus(counter); i++) {
tools/perf/util/stream.c: for (int i = 0; i < nr_evsel; i++)
tools/perf/util/stream.c: for (int i = 0; i < nr_evsel; i++) {
tools/perf/util/stream.c: for (int i = 0; i < els->nr_evsel; i++) {
tools/perf/util/stream.c: for (int i = 0; i < es_pair->nr_streams; i++) {
tools/perf/util/stream.c: for (int i = 0; i < es_base->nr_streams; i++) {
[acme@five perf]$ > > +#define for_each_shell_test(entlist, nr, base, ent) \
And this has been building OK since forever, it seems, in all the test
build containers, with tons of gcc and clang versions, so I don't think
this is a problem.
- Arnaldo
> > + for (int __i = 0; __i < nr && (ent = entlist[__i]); __i++) \
>
> I think this declaration of __i is okay as C99 allows declarations
> here, but generally I don't see this style in the kernel.
Yeah, but there is precendence in.. tools/perf/:
> Thanks,
> Ian
>
> > if (!is_directory(base, ent) && ent->d_name[0] != '.')
> >
> > static const char *shell_tests__dir(char *path, size_t size)
> > @@ -538,8 +538,9 @@ static const char *shell_tests__dir(char *path, size_t size)
> >
> > static int shell_tests__max_desc_width(void)
> > {
> > - DIR *dir;
> > + struct dirent **entlist;
> > struct dirent *ent;
> > + int n_dirs;
> > char path_dir[PATH_MAX];
> > const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
> > int width = 0;
> > @@ -547,11 +548,11 @@ static int shell_tests__max_desc_width(void)
> > if (path == NULL)
> > return -1;
> >
> > - dir = opendir(path);
> > - if (!dir)
> > + n_dirs = scandir(path, &entlist, NULL, alphasort);
> > + if (n_dirs == -1)
> > return -1;
> >
> > - for_each_shell_test(dir, path, ent) {
> > + for_each_shell_test(entlist, n_dirs, path, ent) {
> > char bf[256];
> > const char *desc = shell_test__description(bf, sizeof(bf), path, ent->d_name);
> >
> > @@ -563,7 +564,8 @@ static int shell_tests__max_desc_width(void)
> > }
> > }
> >
> > - closedir(dir);
> > + free(entlist);
> > +
> > return width;
> > }
> >
> > @@ -589,8 +591,9 @@ static int shell_test__run(struct test *test, int subdir __maybe_unused)
> >
> > static int run_shell_tests(int argc, const char *argv[], int i, int width)
> > {
> > - DIR *dir;
> > + struct dirent **entlist;
> > struct dirent *ent;
> > + int n_dirs;
> > char path_dir[PATH_MAX];
> > struct shell_test st = {
> > .dir = shell_tests__dir(path_dir, sizeof(path_dir)),
> > @@ -599,14 +602,14 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
> > if (st.dir == NULL)
> > return -1;
> >
> > - dir = opendir(st.dir);
> > - if (!dir) {
> > + n_dirs = scandir(st.dir, &entlist, NULL, alphasort);
> > + if (n_dirs == -1) {
> > pr_err("failed to open shell test directory: %s\n",
> > st.dir);
> > return -1;
> > }
> >
> > - for_each_shell_test(dir, st.dir, ent) {
> > + for_each_shell_test(entlist, n_dirs, st.dir, ent) {
> > int curr = i++;
> > char desc[256];
> > struct test test = {
> > @@ -623,7 +626,7 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
> > test_and_print(&test, false, -1);
> > }
> >
> > - closedir(dir);
> > + free(entlist);
> > return 0;
> > }
> >
> > @@ -722,19 +725,20 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
> >
> > static int perf_test__list_shell(int argc, const char **argv, int i)
> > {
> > - DIR *dir;
> > + struct dirent **entlist;
> > struct dirent *ent;
> > + int n_dirs;
> > char path_dir[PATH_MAX];
> > const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
> >
> > if (path == NULL)
> > return -1;
> >
> > - dir = opendir(path);
> > - if (!dir)
> > + n_dirs = scandir(path, &entlist, NULL, alphasort);
> > + if (n_dirs == -1)
> > return -1;
> >
> > - for_each_shell_test(dir, path, ent) {
> > + for_each_shell_test(entlist, n_dirs, path, ent) {
> > int curr = i++;
> > char bf[256];
> > struct test t = {
> > @@ -747,7 +751,7 @@ static int perf_test__list_shell(int argc, const char **argv, int i)
> > pr_info("%2d: %s\n", i, t.desc);
> > }
> >
> > - closedir(dir);
> > + free(entlist);
> > return 0;
> > }
> >
> > --
> > 2.31.1
> >
--
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf test: iterate over shell tests in alphabetical order
2021-05-25 23:25 ` Ian Rogers
2021-05-26 12:36 ` Jiri Olsa
2021-05-26 13:57 ` Arnaldo Carvalho de Melo
@ 2021-05-26 13:58 ` Arnaldo Carvalho de Melo
2 siblings, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-05-26 13:58 UTC (permalink / raw)
To: Ian Rogers
Cc: Riccardo Mancini, Namhyung Kim, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Leo Yan,
Fabian Hemmer, Tommi Rantala, Stephane Eranian, linux-perf-users,
LKML
Em Tue, May 25, 2021 at 04:25:48PM -0700, Ian Rogers escreveu:
> On Tue, May 25, 2021 at 4:08 PM Riccardo Mancini <rickyman7@gmail.com> wrote:
> >
> > for_each_shell_test macro iterated over all shell tests in the directory
> > using readdir, which does not guarantee any ordering, causing
> > problems on certain fs. However, the order in which they are visited
> > determines the id of the test, in case one wants to run a single test.
> >
> > This patch replaces readdir with scandir using alphabetical sorting.
> > This guarantees that, given the same set of tests, all machines will
> > see the tests in the same order, and, thus, that test ids are
> > consistent.
> >
> > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Ian Rogers <irogers@google.com>
>
> Acked-by: Ian Rogers <irogers@google.com>
Thanks, applied and also added this, which I think it appropriate in
this case, no?
Reported-by: Ian Rogers <irogers@google.com>
- Arnaldo
> > ---
> > tools/perf/tests/builtin-test.c | 38 ++++++++++++++++++---------------
> > 1 file changed, 21 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index c4b888f18e9ca..cbbfe48ab8029 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -510,8 +510,8 @@ static const char *shell_test__description(char *description, size_t size,
> > return description ? strim(description + 1) : NULL;
> > }
> >
> > -#define for_each_shell_test(dir, base, ent) \
> > - while ((ent = readdir(dir)) != NULL) \
> > +#define for_each_shell_test(entlist, nr, base, ent) \
> > + for (int __i = 0; __i < nr && (ent = entlist[__i]); __i++) \
>
> I think this declaration of __i is okay as C99 allows declarations
> here, but generally I don't see this style in the kernel.
>
> Thanks,
> Ian
>
> > if (!is_directory(base, ent) && ent->d_name[0] != '.')
> >
> > static const char *shell_tests__dir(char *path, size_t size)
> > @@ -538,8 +538,9 @@ static const char *shell_tests__dir(char *path, size_t size)
> >
> > static int shell_tests__max_desc_width(void)
> > {
> > - DIR *dir;
> > + struct dirent **entlist;
> > struct dirent *ent;
> > + int n_dirs;
> > char path_dir[PATH_MAX];
> > const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
> > int width = 0;
> > @@ -547,11 +548,11 @@ static int shell_tests__max_desc_width(void)
> > if (path == NULL)
> > return -1;
> >
> > - dir = opendir(path);
> > - if (!dir)
> > + n_dirs = scandir(path, &entlist, NULL, alphasort);
> > + if (n_dirs == -1)
> > return -1;
> >
> > - for_each_shell_test(dir, path, ent) {
> > + for_each_shell_test(entlist, n_dirs, path, ent) {
> > char bf[256];
> > const char *desc = shell_test__description(bf, sizeof(bf), path, ent->d_name);
> >
> > @@ -563,7 +564,8 @@ static int shell_tests__max_desc_width(void)
> > }
> > }
> >
> > - closedir(dir);
> > + free(entlist);
> > +
> > return width;
> > }
> >
> > @@ -589,8 +591,9 @@ static int shell_test__run(struct test *test, int subdir __maybe_unused)
> >
> > static int run_shell_tests(int argc, const char *argv[], int i, int width)
> > {
> > - DIR *dir;
> > + struct dirent **entlist;
> > struct dirent *ent;
> > + int n_dirs;
> > char path_dir[PATH_MAX];
> > struct shell_test st = {
> > .dir = shell_tests__dir(path_dir, sizeof(path_dir)),
> > @@ -599,14 +602,14 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
> > if (st.dir == NULL)
> > return -1;
> >
> > - dir = opendir(st.dir);
> > - if (!dir) {
> > + n_dirs = scandir(st.dir, &entlist, NULL, alphasort);
> > + if (n_dirs == -1) {
> > pr_err("failed to open shell test directory: %s\n",
> > st.dir);
> > return -1;
> > }
> >
> > - for_each_shell_test(dir, st.dir, ent) {
> > + for_each_shell_test(entlist, n_dirs, st.dir, ent) {
> > int curr = i++;
> > char desc[256];
> > struct test test = {
> > @@ -623,7 +626,7 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width)
> > test_and_print(&test, false, -1);
> > }
> >
> > - closedir(dir);
> > + free(entlist);
> > return 0;
> > }
> >
> > @@ -722,19 +725,20 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
> >
> > static int perf_test__list_shell(int argc, const char **argv, int i)
> > {
> > - DIR *dir;
> > + struct dirent **entlist;
> > struct dirent *ent;
> > + int n_dirs;
> > char path_dir[PATH_MAX];
> > const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
> >
> > if (path == NULL)
> > return -1;
> >
> > - dir = opendir(path);
> > - if (!dir)
> > + n_dirs = scandir(path, &entlist, NULL, alphasort);
> > + if (n_dirs == -1)
> > return -1;
> >
> > - for_each_shell_test(dir, path, ent) {
> > + for_each_shell_test(entlist, n_dirs, path, ent) {
> > int curr = i++;
> > char bf[256];
> > struct test t = {
> > @@ -747,7 +751,7 @@ static int perf_test__list_shell(int argc, const char **argv, int i)
> > pr_info("%2d: %s\n", i, t.desc);
> > }
> >
> > - closedir(dir);
> > + free(entlist);
> > return 0;
> > }
> >
> > --
> > 2.31.1
> >
--
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-26 13:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-25 23:05 [PATCH] perf test: iterate over shell tests in alphabetical order Riccardo Mancini
2021-05-25 23:25 ` Ian Rogers
2021-05-26 12:36 ` Jiri Olsa
2021-05-26 13:57 ` Arnaldo Carvalho de Melo
2021-05-26 13:58 ` Arnaldo Carvalho de Melo
2021-05-26 12:33 ` Jiri Olsa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).