From: Namhyung Kim <namhyung@kernel.org>
To: Arnaldo Carvalho de Melo <acme@kernel.org>, Jiri Olsa <jolsa@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
LKML <linux-kernel@vger.kernel.org>,
Ian Rogers <irogers@google.com>,
Adrian Hunter <adrian.hunter@intel.com>,
linux-perf-users@vger.kernel.org,
Sumanth Korikkar <sumanthk@linux.ibm.com>,
Thomas Richter <tmricht@linux.ibm.com>,
Kevin Nomura <nomurak@google.com>
Subject: [PATCH] perf stat: Do not delay the workload with --delay
Date: Mon, 12 Dec 2022 15:08:20 -0800 [thread overview]
Message-ID: <20221212230820.901382-1-namhyung@kernel.org> (raw)
The -D/--delay option is to delay the measure after the program starts.
But the current code goes to sleep before starting the program so the
program is delayed too. This is not the intention, let's fix it.
Before:
$ time sudo ./perf stat -a -e cycles -D 3000 sleep 4
Events disabled
Events enabled
Performance counter stats for 'system wide':
4,326,949,337 cycles
4.007494118 seconds time elapsed
real 0m7.474s
user 0m0.356s
sys 0m0.120s
It ran the workload for 4 seconds and gave the 3 second delay. So it
should skip the first 3 second and measure the last 1 second only. But
as you can see, it delays 3 seconds and ran the workload after that for
4 seconds. So the total time (real) was 7 seconds.
After:
$ time sudo ./perf stat -a -e cycles -D 3000 sleep 4
Events disabled
Events enabled
Performance counter stats for 'system wide':
1,063,551,013 cycles
1.002769510 seconds time elapsed
real 0m4.484s
user 0m0.385s
sys 0m0.086s
The bug was introduced when it changed enablement of system-wide events
with a command line workload. But it should've considered the initial
delay case. The code was reworked since then (in bb8bc52e7578) so I'm
afraid it won't be applied cleanly.
Fixes: d0a0a511493d ("perf stat: Fix forked applications enablement of counters")
Cc: Sumanth Korikkar <sumanthk@linux.ibm.com>
Cc: Thomas Richter <tmricht@linux.ibm.com>
Reported-by: Kevin Nomura <nomurak@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/builtin-stat.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d040fbcdcc5a..b39bf785a16e 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -540,26 +540,14 @@ static int enable_counters(void)
return err;
}
- if (stat_config.initial_delay < 0) {
- pr_info(EVLIST_DISABLED_MSG);
- return 0;
- }
-
- if (stat_config.initial_delay > 0) {
- pr_info(EVLIST_DISABLED_MSG);
- usleep(stat_config.initial_delay * USEC_PER_MSEC);
- }
-
/*
* We need to enable counters only if:
* - we don't have tracee (attaching to task or cpu)
* - we have initial delay configured
*/
- if (!target__none(&target) || stat_config.initial_delay) {
+ if (!target__none(&target)) {
if (!all_counters_use_bpf)
evlist__enable(evsel_list);
- if (stat_config.initial_delay > 0)
- pr_info(EVLIST_ENABLED_MSG);
}
return 0;
}
@@ -930,14 +918,27 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
return err;
}
- err = enable_counters();
- if (err)
- return -1;
+ if (stat_config.initial_delay) {
+ pr_info(EVLIST_DISABLED_MSG);
+ } else {
+ err = enable_counters();
+ if (err)
+ return -1;
+ }
/* Exec the command, if any */
if (forks)
evlist__start_workload(evsel_list);
+ if (stat_config.initial_delay > 0) {
+ usleep(stat_config.initial_delay * USEC_PER_MSEC);
+ err = enable_counters();
+ if (err)
+ return -1;
+
+ pr_info(EVLIST_ENABLED_MSG);
+ }
+
t0 = rdclock();
clock_gettime(CLOCK_MONOTONIC, &ref_time);
--
2.39.0.rc1.256.g54fd8350bd-goog
next reply other threads:[~2022-12-12 23:08 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-12 23:08 Namhyung Kim [this message]
2022-12-13 13:13 ` [PATCH] perf stat: Do not delay the workload with --delay Thomas Richter
2022-12-13 13:39 ` James Clark
2022-12-14 0:18 ` Ian Rogers
2022-12-14 14:41 ` Arnaldo Carvalho de Melo
2022-12-15 1:43 ` Leo Yan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221212230820.901382-1-namhyung@kernel.org \
--to=namhyung@kernel.org \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=nomurak@google.com \
--cc=peterz@infradead.org \
--cc=sumanthk@linux.ibm.com \
--cc=tmricht@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.