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

Madadi Vineeth Reddy
2024-06-28T07:10:23Z

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