2024-04-27 01:33:07

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 1/1] perf test: Reintroduce -p/--parallel and make -S/--sequential the default

On Fri, Apr 26, 2024 at 03:19:22PM -0700, Ian Rogers wrote:
> On Fri, Apr 26, 2024 at 3:12 PM Arnaldo Carvalho de Melo
> <[email protected]> wrote:
> >
> > We can't default to doing parallel tests as there are tests that compete
> > for the same resources and thus clash, for instance tests that put in
> > place 'perf probe' probes, that clean the probes without regard to other
> > tests needs, ARM64 coresight tests, Intel PT ones, etc.
> >
> > So reintroduce --p/--parallel and make -S/--sequential the default.
> >
> > We need to come up with infrastructure that state which tests can't run
> > in parallel because they need exclusive access to some resource,
> > something as simple as "probes" that would then avoid 'perf probe' tests
> > from running while other such test is running, or make the tests more
> > resilient, till then we can't use parallel mode as default.
> >
> > While at it, document all these options in the 'perf test' man page.
>
> Looks good to me, LKML?

Right, CCed now.

> Reviewed-by: Ian Rogers <[email protected]>

Thanks!

- Arnaldo

> Thanks,
> Ian
>
>
> > Reported-by: Adrian Hunter <[email protected]>
> > Reported-by: Arnaldo Carvalho de Melo <[email protected]>
> > Reported-by: James Clark <[email protected]>
> > Acked-by: Ian Rogers <[email protected]>
> > Cc: Jiri Olsa <[email protected]>
> > Cc: Kan Liang <[email protected]>
> > Cc: Namhyung Kim <[email protected]>
> > Link: https://lore.kernel.org/lkml/
> > Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
> > ---
> > tools/perf/Documentation/perf-test.txt | 13 ++++++++++++-
> > tools/perf/tests/builtin-test.c | 8 +++++++-
> > 2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/Documentation/perf-test.txt b/tools/perf/Documentation/perf-test.txt
> > index 951a2f2628727a95..9acb8d1f658890e9 100644
> > --- a/tools/perf/Documentation/perf-test.txt
> > +++ b/tools/perf/Documentation/perf-test.txt
> > @@ -31,9 +31,20 @@ OPTIONS
> > --verbose::
> > Be more verbose.
> >
> > +-S::
> > +--sequential::
> > + Run tests one after the other, this is the default mode.
> > +
> > +-p::
> > +--parallel::
> > + Run tests in parallel, speeds up the whole process but is not safe with
> > + the current infrastructure, where some tests that compete for some resources,
> > + for instance, 'perf probe' tests that add/remove probes or clean all probes, etc.
> > +
> > -F::
> > --dont-fork::
> > - Do not fork child for each test, run all tests within single process.
> > + Do not fork child for each test, run all tests within single process, this
> > + sets sequential mode.
> >
> > --dso::
> > Specify a DSO for the "Symbols" test.
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index 73f53b02f7334692..c3d84b67ca8e775d 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -40,7 +40,10 @@
> > */
> > static bool dont_fork;
> > /* Don't fork the tests in parallel and wait for their completion. */
> > -static bool sequential;
> > +static bool sequential = true;
> > +/* Do it in parallel, lacks infrastructure to avoid running tests that clash for resources,
> > + * So leave it as the developers choice to enable while working on the needed infra */
> > +static bool parallel;
> > const char *dso_to_test;
> > const char *test_objdump_path = "objdump";
> >
> > @@ -536,6 +539,7 @@ 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 in parallel"),
> > OPT_BOOLEAN('S', "sequential", &sequential,
> > "Run the tests one after another rather than in parallel"),
> > OPT_STRING('w', "workload", &workload, "work", "workload to run for testing"),
> > @@ -566,6 +570,8 @@ int cmd_test(int argc, const char **argv)
> >
> > if (dont_fork)
> > sequential = true;
> > + else if (parallel)
> > + sequential = false;
> >
> > symbol_conf.priv_size = sizeof(int);
> > symbol_conf.try_vmlinux_path = true;
> > --
> > 2.44.0
> >