[PATCH] perf sched replay: Fix -r/--repeat command line option for infinity

[PATCH] perf sched replay: Fix -r/--repeat command line option for infinity

[Madadi Vineeth Reddy]

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@linux.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) {
 		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[] = {
-- 
2.31.1

Re: [PATCH] perf sched replay: Fix -r/--repeat command line option for infinity

[James Clark]

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@linux.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@arm.com>

>  		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[] = {

Re: [PATCH] perf sched replay: Fix -r/--repeat command line option for infinity

[Madadi Vineeth Reddy]

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@linux.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@arm.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[] = {