Received-SPF: pass (google.com: domain of linux-kernel+bounces-160849-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 26 Apr 2024 22:32:43 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>,
	Arnaldo Carvalho de Melo <acme@redhat.com>,
	James Clark <james.clark@arm.com>, Jiri Olsa <jolsa@kernel.org>,
	Kan Liang <kan.liang@linux.intel.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/1] perf test: Reintroduce -p/--parallel and make
 -S/--sequential the default
Message-ID: <ZixVuyDSvTETMvi6@x1>
References: <Ziwm18BqIn_vc1vn@x1>
 <CAP-5=fXKsW_cD6=k7yAkNwwdmKqOSMga8Y7o4CwMd+9MOSw0zA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAP-5=fXKsW_cD6=k7yAkNwwdmKqOSMga8Y7o4CwMd+9MOSw0zA@mail.gmail.com>

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
> <acme@kernel.org> 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 <irogers@google.com>

Thanks!

- Arnaldo
 
> Thanks,
> Ian
> 
> 
> > Reported-by: Adrian Hunter <adrian.hunter@intel.com>
> > Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Reported-by: James Clark <james.clark@arm.com>
> > Acked-by: Ian Rogers <irogers@google.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Kan Liang <kan.liang@linux.intel.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Link: https://lore.kernel.org/lkml/
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> >  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
> >