Re: [PATCH v3 8/8] perf tests: Add option to run tests in parallel

[Namhyung Kim]

On Mon, Feb 12, 2024 at 10:59 AM Ian Rogers <irogers@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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", &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.687.g38aa6559b0-goog
>