[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ff2c43e3-31d3-40c8-a597-cd0be751c0f2@linux.ibm.com>
Date: Tue, 18 Jun 2024 21:39:17 +0530
From: Madadi Vineeth Reddy <vineethr@...ux.ibm.com>
To: James Clark <james.clark@....com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Namhyung Kim <namhyung@...nel.org>, Ian Rogers <irogers@...gle.com>,
Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
Chen Yu <yu.c.chen@...el.com>, 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,
Fernand Sieber <sieberf@...zon.com>,
linux-perf-users <linux-perf-users@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Madadi Vineeth Reddy <vineethr@...ux.ibm.com>
Subject: Re: [PATCH] perf sched replay: Fix -r/--repeat command line option
for infinity
Hi James,
On 18/06/24 18:34, James Clark wrote:
>
>
> On 18/06/2024 12:29, 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)
>>
>
> I was wondering if this was a regression or was always like this but
> it's at least been broken long before it becomes difficult to build
> Perf for me.
>
>> 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.
>>
>
> I also saw an intermittent hang waiting in destroy_tasks() which is now
> fixed.
>
>> 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>
>> ---
>> tools/perf/Documentation/perf-sched.txt | 7 +++++++
>> tools/perf/builtin-sched.c | 8 ++++++--
>> 2 files changed, 13 insertions(+), 2 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..2c4ed5c2d695 100644
>> --- a/tools/perf/builtin-sched.c
>> +++ b/tools/perf/builtin-sched.c
>> @@ -3383,8 +3383,12 @@ 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++)
>> +
>> + i = 0;
>> + while (sched->replay_repeat == 0 || i < sched->replay_repeat) {
>
> Very minor nit, but you don't really need to remove the for loop, just add
> the new condition to the existing one. Not sure if it's worth re-spinning
> for though.
>
> Reviewed-by: James Clark <james.clark@....com>
>
Thank you for taking a look!
Thanks and Regards
Madadi Vineeth Reddy
>> run_one_test(sched);
>> + i++;
>> + }
>>
>> sched->thread_funcs_exit = true;
>> destroy_tasks(sched);
>> @@ -3548,7 +3552,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