[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zn4p0kMIsLNlv04X@google.com>
Date: Thu, 27 Jun 2024 20:11:14 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Madadi Vineeth Reddy <vineethr@...ux.ibm.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>, Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Kan Liang <kan.liang@...ux.intel.com>, acme@...hat.com,
Chen Yu <yu.c.chen@...el.com>, Fernand Sieber <sieberf@...zon.com>,
linux-perf-users <linux-perf-users@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
James Clark <james.clark@....com>,
Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
Ian Rogers <irogers@...gle.com>
Subject: Re: [PATCH v2] perf sched replay: Fix -r/--repeat command line
option for infinity
On Fri, Jun 28, 2024 at 08:00:14AM +0530, Madadi Vineeth Reddy wrote:
> On 21/06/24 22:49, Madadi Vineeth Reddy wrote:
> > Currently, the -r/--repeat option accepts values from 0 and complains
> > for -1. The help section specifies:
> > -r, --repeat <n> repeat the workload replay N times (-1: infinite)
> >
> > The -r -1 option raises an error because replay_repeat is defined as
> > an unsigned int.
> >
> > In the current implementation, the workload is repeated n times when
> > -r <n> is used, except when n is 0.
> >
> > When -r is set to 0, the workload is also repeated once. This happens
> > because when -r=0, the run_one_test function is not called. (Note that
> > mutex unlocking, which is essential for child threads spawned to emulate
> > the workload, happens in run_one_test.) However, mutex unlocking is
> > still performed in the destroy_tasks function. Thus, -r=0 results in the
> > workload running once coincidentally.
> >
> > To clarify and maintain the existing logic for -r >= 1 (which runs the
> > workload the specified number of times) and to fix the issue with infinite
> > runs, make -r=0 perform an infinite run.
> >
> > Signed-off-by: Madadi Vineeth Reddy <vineethr@...ux.ibm.com>
> > Reviewed-by: James Clark <james.clark@....com>
> >
>
> Hi all,
> Any suggestions/comments on this patch?
I think the original intention was to set replay_repeat to UINT_MAX
using -1. But it seems to refuse the negative number now.
Maybe you can simply set it to UNIT_MAX for -r 0.
Thanks,
Namhyung
> > ---
> > Changes in v2:
> > - Kept the existing 'for' loop and added a condition for infinite
> > looping. (James Clark)
> > - This change also ensures that integer overflow doesn't occur when
> > 'replay_repeat' is zero.
> > - Add Reviewed-by tag from James Clark.
> > - Rebase against perf-tools-next commit 788c5160526a ("perf vendor
> > events: Add westmereex counter information")
> > - Link to v1: https://lore.kernel.org/all/20240618112907.15131-1-vineethr@linux.ibm.com/
> >
> > tools/perf/Documentation/perf-sched.txt | 7 +++++++
> > tools/perf/builtin-sched.c | 11 ++++++++---
> > 2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/Documentation/perf-sched.txt b/tools/perf/Documentation/perf-sched.txt
> > index a216d2991b19..f1be8f0b249e 100644
> > --- a/tools/perf/Documentation/perf-sched.txt
> > +++ b/tools/perf/Documentation/perf-sched.txt
> > @@ -202,6 +202,13 @@ OPTIONS for 'perf sched timehist'
> > --state::
> > Show task state when it switched out.
> >
> > +OPTIONS for 'perf sched replay'
> > +------------------------------
> > +
> > +-r::
> > +--repeat <n>::
> > + repeat the workload n times (0: infinite). Default is 10.
> > +
> > SEE ALSO
> > --------
> > linkperf:perf-record[1]
> > diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> > index 8cdf18139a7e..51b3dea404bc 100644
> > --- a/tools/perf/builtin-sched.c
> > +++ b/tools/perf/builtin-sched.c
> > @@ -3383,8 +3383,13 @@ static int perf_sched__replay(struct perf_sched *sched)
> > sched->thread_funcs_exit = false;
> > create_tasks(sched);
> > printf("------------------------------------------------------------\n");
> > - for (i = 0; i < sched->replay_repeat; i++)
> > - run_one_test(sched);
> > + if (sched->replay_repeat == 0) {
> > + while (1)
> > + run_one_test(sched);
> > + } else {
> > + for (i = 0; i < sched->replay_repeat; i++)
> > + run_one_test(sched);
> > + }
> >
> > sched->thread_funcs_exit = true;
> > destroy_tasks(sched);
> > @@ -3548,7 +3553,7 @@ int cmd_sched(int argc, const char **argv)
> > };
> > const struct option replay_options[] = {
> > OPT_UINTEGER('r', "repeat", &sched.replay_repeat,
> > - "repeat the workload replay N times (-1: infinite)"),
> > + "repeat the workload replay N times (0: infinite)"),
> > OPT_PARENT(sched_options)
> > };
> > const struct option map_options[] = {
>
Powered by blists - more mailing lists