2020-04-16 00:15:20

by Arnaldo Carvalho de Melo

Subject: [PATCH] perf stat: Honour --timeout for forked workloads

Hi guys,

Please take a look and give this your acks, Tested-by, etc.

Thanks,

- Arnaldo

----

When --timeout is used and a workload is specified to be started by
'perf stat', i.e.

$ perf stat --timeout 1000 sleep 1h

The --timeout wasn't being honoured, i.e. the workload, 'sleep 1h' in
the above example, should be terminated after 1000ms, but it wasn't,
'perf stat' was waiting for it to finish.

Fix it by sending a SIGTERM when the timeout expires.

Now it works:

# perf stat -e cycles --timeout 1234 sleep 1h
sleep: Terminated

Performance counter stats for 'sleep 1h':

1,066,692 cycles

1.234314838 seconds time elapsed

0.000750000 seconds user
0.000000000 seconds sys

#

Reported-by: Konstantin Kharlamov <[email protected]>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207243
Cc: Adrian Hunter <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/builtin-stat.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index ec053dc1e35c..9207b6c45475 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -686,8 +686,11 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
break;
}
}
- if (child_pid != -1)
+ if (child_pid != -1) {
+ if (timeout)
+ kill(child_pid, SIGTERM);
wait4(child_pid, &status, 0, &stat_config.ru_data);
+ }

if (workload_exec_errno) {
const char *emsg = str_error_r(workload_exec_errno, msg, sizeof(msg));
--
2.25.2

2020-04-16 00:20:50

by Jiri Olsa

Subject: Re: [PATCH] perf stat: Honour --timeout for forked workloads

On Wed, Apr 15, 2020 at 12:38:03PM -0300, Arnaldo Carvalho de Melo wrote:
> Hi guys,
>
> Please take a look and give this your acks, Tested-by, etc.
>
> Thanks,
>
> - Arnaldo
>
> ----
>
> When --timeout is used and a workload is specified to be started by
> 'perf stat', i.e.
>
> $ perf stat --timeout 1000 sleep 1h
>
> The --timeout wasn't being honoured, i.e. the workload, 'sleep 1h' in
> the above example, should be terminated after 1000ms, but it wasn't,
> 'perf stat' was waiting for it to finish.
>
> Fix it by sending a SIGTERM when the timeout expires.
>
> Now it works:
>
> # perf stat -e cycles --timeout 1234 sleep 1h
> sleep: Terminated
>
> Performance counter stats for 'sleep 1h':
>
> 1,066,692 cycles
>
> 1.234314838 seconds time elapsed
>
> 0.000750000 seconds user
> 0.000000000 seconds sys
>
> #
>
> Reported-by: Konstantin Kharlamov <[email protected]>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207243
> Cc: Adrian Hunter <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>

Acked/Tested-by: Jiri Olsa <[email protected]>

cc-ing yuzhoujian, author of the --timeout

jirka

> ---
> tools/perf/builtin-stat.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index ec053dc1e35c..9207b6c45475 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -686,8 +686,11 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> break;
> }
> }
> - if (child_pid != -1)
> + if (child_pid != -1) {
> + if (timeout)
> + kill(child_pid, SIGTERM);
> wait4(child_pid, &status, 0, &stat_config.ru_data);
> + }
>
> if (workload_exec_errno) {
> const char *emsg = str_error_r(workload_exec_errno, msg, sizeof(msg));
> --
> 2.25.2
>

2020-04-16 00:25:40

by Konstantin Kharlamov

Subject: Re: [PATCH] perf stat: Honour --timeout for forked workloads

Tested-by: Konstantin Kharlamov <[email protected]>

On среда, 15 апреля 2020 г. 18:38:03 MSK Arnaldo Carvalho de Melo wrote:
> Hi guys,
>
> Please take a look and give this your acks, Tested-by, etc.
>
> Thanks,
>
> - Arnaldo
>
> ----
>
> When --timeout is used and a workload is specified to be started by
> 'perf stat', i.e.
>
> $ perf stat --timeout 1000 sleep 1h
>
> The --timeout wasn't being honoured, i.e. the workload, 'sleep 1h' in
> the above example, should be terminated after 1000ms, but it wasn't,
> 'perf stat' was waiting for it to finish.
>
> Fix it by sending a SIGTERM when the timeout expires.
>
> Now it works:
>
> # perf stat -e cycles --timeout 1234 sleep 1h
> sleep: Terminated
>
> Performance counter stats for 'sleep 1h':
>
> 1,066,692 cycles
>
> 1.234314838 seconds time elapsed
>
> 0.000750000 seconds user
> 0.000000000 seconds sys
>
> #
>
> Reported-by: Konstantin Kharlamov <[email protected]>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207243
> Cc: Adrian Hunter <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
> ---
> tools/perf/builtin-stat.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index ec053dc1e35c..9207b6c45475 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -686,8 +686,11 @@ static int __run_perf_stat(int argc, const char **argv,
> int run_idx) break;
> }
> }
> - if (child_pid != -1)
> + if (child_pid != -1) {
> + if (timeout)
> + kill(child_pid, SIGTERM);
> wait4(child_pid, &status, 0,
&stat_config.ru_data);
> + }
>
> if (workload_exec_errno) {
> const char *emsg =
str_error_r(workload_exec_errno, msg, sizeof(msg));


--
Инженер-разработчик Константин Харламов

2020-04-22 12:27:46

by tip-bot2 for Haifeng Xu

Subject: [tip: perf/core] perf stat: Honour --timeout for forked workloads

The following commit has been merged into the perf/core branch of tip:

Commit-ID: cfbd41b786519d4a15e1c15181556689bcf6635a
Gitweb: https://git.kernel.org/tip/cfbd41b786519d4a15e1c15181556689bcf6635a
Author: Arnaldo Carvalho de Melo <[email protected]>
AuthorDate: Wed, 15 Apr 2020 12:31:26 -03:00
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitterDate: Thu, 16 Apr 2020 12:17:41 -03:00

perf stat: Honour --timeout for forked workloads

When --timeout is used and a workload is specified to be started by
'perf stat', i.e.

$ perf stat --timeout 1000 sleep 1h

The --timeout wasn't being honoured, i.e. the workload, 'sleep 1h' in
the above example, should be terminated after 1000ms, but it wasn't,
'perf stat' was waiting for it to finish.

Fix it by sending a SIGTERM when the timeout expires.

Now it works:

# perf stat -e cycles --timeout 1234 sleep 1h
sleep: Terminated

Performance counter stats for 'sleep 1h':

1,066,692 cycles

1.234314838 seconds time elapsed

0.000750000 seconds user
0.000000000 seconds sys

#

Fixes: f1f8ad52f8bf ("perf stat: Add support to print counts after a period of time")
Reported-by: Konstantin Kharlamov <[email protected]>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207243
Tested-by: Konstantin Kharlamov <[email protected]>
Cc: Adrian Hunter <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
Tested-by: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: yuzhoujian <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/builtin-stat.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index ec053dc..9207b6c 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -686,8 +686,11 @@ try_again_reset:
break;
}
}
- if (child_pid != -1)
+ if (child_pid != -1) {
+ if (timeout)
+ kill(child_pid, SIGTERM);
wait4(child_pid, &status, 0, &stat_config.ru_data);
+ }

if (workload_exec_errno) {
const char *emsg = str_error_r(workload_exec_errno, msg, sizeof(msg));