| 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
| ||
|
Message-ID: <87bocjl00m.fsf@sejong.aot.lge.com>
Date: Mon, 21 Jan 2013 11:53:29 +0900
From: Namhyung Kim <namhyung@...nel.org>
To: Stephane Eranian <eranian@...gle.com>
Cc: linux-kernel@...r.kernel.org, peterz@...radead.org, mingo@...e.hu,
ak@...ux.intel.com, acme@...hat.com, jolsa@...hat.com,
namhyung.kim@....com
Subject: Re: [PATCH v4 2/2] perf stat: add interval printing
Hi Stephane,
On Sat, 19 Jan 2013 00:13:59 +0100, Stephane Eranian wrote:
> This patch adds a new printing mode for perf stat.
> It allows internval printing. That means perf stat
> can now print event deltas at regular time interval.
> This is useful to detect phases in programs.
>
> The -I option enables interval printing. It expects
> an interval duration in milliseconds. Minimum is
> 100ms. Once, activated perf stat prints events deltas
> since last printout. All modes are supported.
>
> $ perf stat -I 1000 -e cycles noploop 10
> noploop for 10 seconds
Is this line an output from perf stat?
In addition, how about adding a head line like:
# time count event
#
> 1.000086918 2385155642 cycles # 0.000 GHz
> 2.000267937 2392279774 cycles # 0.000 GHz
> 3.000385400 2390971450 cycles # 0.000 GHz
> 4.000504408 2390996752 cycles # 0.000 GHz
> 5.000626878 2390853097 cycles # 0.000 GHz
>
> The output format makes it easy to feed into a plotting program
> such as gnuplot when the -I option is used in combination with the -x
> option:
>
> $ perf stat -x, -I 1000 -e cycles noploop 10
> noploop for 10 seconds
> 1.000084113,2378775498,cycles
> 2.000245798,2391056897,cycles
> 3.000354445,2392089414,cycles
> 4.000459115,2390936603,cycles
> 5.000565341,2392108173,cycles
>
> Signed-off-by: Stephane Eranian <eranian@...gle.com>
> ---
[snip]
> @@ -877,6 +977,8 @@ static void print_counter(struct perf_evsel *counter)
> static void print_stat(int argc, const char **argv)
> {
> struct perf_evsel *counter;
> + struct timespec ts, rs;
> + char prefix[64] = { 0, };
> int i;
>
> fflush(stdout);
> @@ -899,12 +1001,18 @@ static void print_stat(int argc, const char **argv)
> fprintf(output, ":\n\n");
> }
>
> + if (interval) {
> + clock_gettime(CLOCK_MONOTONIC, &ts);
> + diff_timespec(&rs, &ts, &ref_time);
> + sprintf(prefix, "%lu.%09lu%s", rs.tv_sec, rs.tv_nsec, csv_sep);
> + }
AFAICS the only caller of print_stat() is cmd_stat() and it'll call this
only if interval is 0. So why not just setting prefix to NULL then?
> +
> if (no_aggr) {
> list_for_each_entry(counter, &evsel_list->entries, node)
> - print_counter(counter);
> + print_counter(counter, prefix);
> } else {
> list_for_each_entry(counter, &evsel_list->entries, node)
> - print_counter_aggr(counter);
> + print_counter_aggr(counter, prefix);
> }
>
> if (!csv_output) {
> @@ -925,7 +1033,7 @@ static volatile int signr = -1;
>
> static void skip_signal(int signo)
> {
> - if(child_pid == -1)
> + if((child_pid == -1) || interval)
Looks like it needs a whitespace :)
> done = 1;
>
> signr = signo;
> @@ -1145,6 +1253,8 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
> "command to run prior to the measured command"),
> OPT_STRING(0, "post", &post_cmd, "command",
> "command to run after to the measured command"),
> + OPT_INTEGER('I', "interval-print", &interval,
> + "print counts at regular interval in ms (>= 100)"),
> OPT_END()
> };
> const char * const stat_usage[] = {
> @@ -1245,12 +1355,23 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
> usage_with_options(stat_usage, options);
> return -1;
> }
> + if (interval < 0 || (interval > 0 && interval < 100)) {
> + pr_err("print interval must be >= 100ms\n");
> + usage_with_options(stat_usage, options);
> + return -1;
> + }
How about making 'interval' unsigned and simplify the condition a bit:
if (interval && interval < 100) {
...
}
Thanks,
Namhyung
>
> list_for_each_entry(pos, &evsel_list->entries, node) {
> if (perf_evsel__alloc_stat_priv(pos) < 0 ||
> perf_evsel__alloc_counts(pos, perf_evsel__nr_cpus(pos)) < 0)
> goto out_free_fd;
> }
> + if (interval) {
> + list_for_each_entry(pos, &evsel_list->entries, node) {
> + if (perf_evsel__alloc_prev_raw_counts(pos) < 0)
> + goto out_free_fd;
> + }
> + }
It's not about your patch, but I can't find where it frees evsel->counts
- a counter part of perf_evsel__alloc_counts(). Seems we leak that?
>
> /*
> * We dont want to block the signals - that would cause
> @@ -1260,6 +1381,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
> */
> atexit(sig_atexit);
> signal(SIGINT, skip_signal);
> + signal(SIGCHLD, skip_signal);
> signal(SIGALRM, skip_signal);
> signal(SIGABRT, skip_signal);
>
> @@ -1272,11 +1394,13 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
> status = run_perf_stat(argc, argv);
> }
>
> - if (status != -1)
> + if (status != -1 && !interval)
> print_stat(argc, argv);
> out_free_fd:
> - list_for_each_entry(pos, &evsel_list->entries, node)
> + list_for_each_entry(pos, &evsel_list->entries, node) {
> perf_evsel__free_stat_priv(pos);
> + perf_evsel__free_prev_raw_counts(pos);
> + }
> perf_evlist__delete_maps(evsel_list);
> out:
> perf_evlist__delete(evsel_list);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists