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

[PATCH v2] 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>
Reviewed-by: James Clark <james.clark@arm.com>

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

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

[Madadi Vineeth Reddy]

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@linux.ibm.com>
> Reviewed-by: James Clark <james.clark@arm.com>
> 

Hi all,
Any suggestions/comments on this patch?

Thanks and Regards
Madadi Vineeth Reddy

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

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

[Namhyung Kim]

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@linux.ibm.com>
> > Reviewed-by: James Clark <james.clark@arm.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[] = {
> 

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

[Madadi Vineeth Reddy]

Hi Namhyung,

On 28/06/24 08:41, Namhyung Kim wrote:
> 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@linux.ibm.com>
>>> Reviewed-by: James Clark <james.clark@arm.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 for the feedback. I will make the change and send a v3.

Thanks and Regards
Madadi Vineeth Reddy

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