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

[Ian Rogers]

On Tue, Feb 13, 2024 at 5:14 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> 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

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", &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
> >