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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <51e9fd2f-7d86-40cb-b307-0b0b396ca32a@linux.ibm.com>
Date: Fri, 28 Jun 2024 08:00:14 +0530
From: Madadi Vineeth Reddy <vineethr@...ux.ibm.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>
Cc: 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>,
        Madadi Vineeth Reddy <vineethr@...ux.ibm.com>
Subject: Re: [PATCH v2] perf sched replay: Fix -r/--repeat command line option
 for infinity

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?

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