linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Adrian Hunter <adrian.hunter@intel.com>
To: Andi Kleen <andi@firstfloor.org>, acme@kernel.org
Cc: jolsa@kernel.org, linux-kernel@vger.kernel.org,
	kim.phillips@arm.com, Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH v5 6/9] perf, tools, script: Make itrace script default to all calls
Date: Tue, 18 Sep 2018 16:24:55 +0300	[thread overview]
Message-ID: <fa942e47-9654-dfa2-7a57-744b5fd3b790@intel.com> (raw)
In-Reply-To: <20180918123214.26728-7-andi@firstfloor.org>

On 18/09/18 15:32, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> By default perf script for itrace outputs sampled instructions or
> branches. In my experience this is confusing to users because it's
> hard to correlate with real program behavior. The sampling makes
> sense for tools like report that actually sample to reduce
> the run time, but run time is normally not a problem for perf script.
> It's better to give an accurate representation of the program flow.
> 
> Default perf script to output all calls for itrace. That's a much saner
> default. The old behavior can be still requested with
> perf script --itrace=ibxwpe100000
> 
> v2: Fix ETM build failure
> v3: Really fix ETM build failure (Kim Phillips)
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  tools/perf/Documentation/itrace.txt |  7 ++++---
>  tools/perf/builtin-script.c         |  5 ++++-
>  tools/perf/util/auxtrace.c          | 17 ++++++++++++-----
>  tools/perf/util/auxtrace.h          |  5 ++++-
>  tools/perf/util/cs-etm.c            |  3 ++-
>  tools/perf/util/intel-bts.c         |  3 ++-
>  tools/perf/util/intel-pt.c          |  3 ++-
>  7 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/Documentation/itrace.txt b/tools/perf/Documentation/itrace.txt
> index a3abe04c779d..c2182cbabde3 100644
> --- a/tools/perf/Documentation/itrace.txt
> +++ b/tools/perf/Documentation/itrace.txt
> @@ -11,10 +11,11 @@
>  		l	synthesize last branch entries (use with i or x)
>  		s       skip initial number of events
>  
> -	The default is all events i.e. the same as --itrace=ibxwpe
> +	The default is all events i.e. the same as --itrace=ibxwpe,
> +	except for perf script where it is --itrace=ce
>  
> -	In addition, the period (default 100000) for instructions events
> -	can be specified in units of:
> +	In addition, the period (default 100000, except for perf script where it is 1)
> +	for instructions events can be specified in units of:
>  
>  		i	instructions
>  		t	ticks
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 57df6f7cff28..d3a463766e2d 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -3112,7 +3112,10 @@ int cmd_script(int argc, const char **argv)
>  	char *rec_script_path = NULL;
>  	char *rep_script_path = NULL;
>  	struct perf_session *session;
> -	struct itrace_synth_opts itrace_synth_opts = { .set = false, };
> +	struct itrace_synth_opts itrace_synth_opts = {
> +		.set = false,
> +		.default_no_sample = true,
> +	};
>  	char *script_path = NULL;
>  	const char **__argv;
>  	int i, j, err = 0;
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index db1511359c5e..2363a41f0e81 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -964,16 +964,23 @@ s64 perf_event__process_auxtrace(struct perf_tool *tool,
>  #define PERF_ITRACE_DEFAULT_LAST_BRANCH_SZ	64
>  #define PERF_ITRACE_MAX_LAST_BRANCH_SZ		1024
>  
> -void itrace_synth_opts__set_default(struct itrace_synth_opts *synth_opts)
> +void itrace_synth_opts__set_default(struct itrace_synth_opts *synth_opts,
> +				    bool no_sample)

Rather than pass no_sample, what about passing the whole of the tool's
options i.e.

diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index db1511359c5e..d4db61626a5f 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -964,8 +964,14 @@ s64 perf_event__process_auxtrace(struct perf_tool *tool,
 #define PERF_ITRACE_DEFAULT_LAST_BRANCH_SZ	64
 #define PERF_ITRACE_MAX_LAST_BRANCH_SZ		1024
 
-void itrace_synth_opts__set_default(struct itrace_synth_opts *synth_opts)
+bool itrace_synth_opts__set_default(struct itrace_synth_opts *synth_opts,
+				    const struct itrace_synth_opts *tool_synth_opts)
 {
+	if (tool_synth_opts && tool_synth_opts->set) {
+		*synth_opts = *tool_synth_opts;
+		return false;
+	}
+
 	synth_opts->instructions = true;
 	synth_opts->branches = true;
 	synth_opts->transactions = true;
@@ -977,6 +983,8 @@ void itrace_synth_opts__set_default(struct itrace_synth_opts *synth_opts)
 	synth_opts->callchain_sz = PERF_ITRACE_DEFAULT_CALLCHAIN_SZ;
 	synth_opts->last_branch_sz = PERF_ITRACE_DEFAULT_LAST_BRANCH_SZ;
 	synth_opts->initial_skip = 0;
+
+	return true;
 }
 
 /*
@@ -1001,7 +1009,7 @@ int itrace_parse_synth_opts(const struct option *opt, const char *str,
 	}
 
 	if (!str) {
-		itrace_synth_opts__set_default(synth_opts);
+		itrace_synth_opts__set_default(synth_opts, NULL);
 		return 0;
 	}
 
diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
index a516a891d7ea..172c92a8c596 100644
--- a/tools/perf/util/auxtrace.h
+++ b/tools/perf/util/auxtrace.h
@@ -528,7 +528,8 @@ int perf_event__process_auxtrace_error(struct perf_tool *tool,
 				       struct perf_session *session);
 int itrace_parse_synth_opts(const struct option *opt, const char *str,
 			    int unset);
-void itrace_synth_opts__set_default(struct itrace_synth_opts *synth_opts);
+bool itrace_synth_opts__set_default(struct itrace_synth_opts *synth_opts,
+				    const struct itrace_synth_opts *tool_synth_opts);
 
 size_t perf_event__fprintf_auxtrace_error(union perf_event *event, FILE *fp);
 void perf_session__auxtrace_error_inc(struct perf_session *session,
diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
index 2ae640257fdb..45b9a2b9f473 100644
--- a/tools/perf/util/cs-etm.c
+++ b/tools/perf/util/cs-etm.c
@@ -1429,12 +1429,9 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
 		return 0;
 	}
 
-	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
-		etm->synth_opts = *session->itrace_synth_opts;
-	} else {
-		itrace_synth_opts__set_default(&etm->synth_opts);
+	if (itrace_synth_opts__set_default(&etm->synth_opts,
+					   session->itrace_synth_opts))
 		etm->synth_opts.callchain = false;
-	}
 
 	err = cs_etm__synth_events(etm, session);
 	if (err)
diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
index 7f0c83b6332b..2d96f4743789 100644
--- a/tools/perf/util/intel-bts.c
+++ b/tools/perf/util/intel-bts.c
@@ -907,12 +907,9 @@ int intel_bts_process_auxtrace_info(union perf_event *event,
 	if (dump_trace)
 		return 0;
 
-	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
-		bts->synth_opts = *session->itrace_synth_opts;
-	} else {
-		itrace_synth_opts__set_default(&bts->synth_opts);
-		if (session->itrace_synth_opts)
-			bts->synth_opts.thread_stack =
+	if (itrace_synth_opts__set_default(&bts->synth_opts,
+					   session->itrace_synth_opts)) {
+		bts->synth_opts.thread_stack =
 				session->itrace_synth_opts->thread_stack;
 	}
 
diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index aec68908d604..b6ac8ac2f4c3 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -2551,10 +2551,8 @@ int intel_pt_process_auxtrace_info(union perf_event *event,
 		goto err_delete_thread;
 	}
 
-	if (session->itrace_synth_opts && session->itrace_synth_opts->set) {
-		pt->synth_opts = *session->itrace_synth_opts;
-	} else {
-		itrace_synth_opts__set_default(&pt->synth_opts);
+	if (itrace_synth_opts__set_default(&pt->synth_opts,
+					   session->itrace_synth_opts)) {
 		if (use_browser != -1) {
 			pt->synth_opts.branches = false;
 			pt->synth_opts.callchain = true;

  reply	other threads:[~2018-09-18 13:26 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-18 12:32 Make perf script easier to use for itrace Andi Kleen
2018-09-18 12:32 ` [PATCH v5 1/9] perf tools: Report itrace options in help Andi Kleen
2018-09-19 18:13   ` Arnaldo Carvalho de Melo
2018-09-18 12:32 ` [PATCH v5 2/9] tools, pager: Support overwriting the pager Andi Kleen
2018-09-19 18:17   ` Arnaldo Carvalho de Melo
2018-09-26  8:44   ` [tip:perf/core] tools lib subcmd: " tip-bot for Andi Kleen
2018-09-18 12:32 ` [PATCH v5 3/9] tools, perf, script: Add --insn-trace for instruction decoding Andi Kleen
2018-09-18 12:32 ` [PATCH v5 4/9] perf, tools, script: Allow sym and dso without ip, addr Andi Kleen
2018-09-19 18:24   ` Arnaldo Carvalho de Melo
2018-09-26  8:44   ` [tip:perf/core] perf " tip-bot for Andi Kleen
2018-09-18 12:32 ` [PATCH v5 5/9] perf, tools, script: Print DSO for callindent Andi Kleen
2018-09-19 18:26   ` Arnaldo Carvalho de Melo
2018-09-26  8:45   ` [tip:perf/core] perf " tip-bot for Andi Kleen
2018-09-18 12:32 ` [PATCH v5 6/9] perf, tools, script: Make itrace script default to all calls Andi Kleen
2018-09-18 13:24   ` Adrian Hunter [this message]
2018-09-19 18:28     ` Arnaldo Carvalho de Melo
2018-09-18 12:32 ` [PATCH v5 7/9] tools, perf, script: Add --call-trace and --call-ret-trace Andi Kleen
2018-09-18 12:32 ` [PATCH v5 8/9] tools, perf, script: Implement --graph-function Andi Kleen
2018-09-18 12:32 ` [PATCH v5 9/9] perf, tools, script: Support total cycles count Andi Kleen
2018-09-19 18:31   ` Arnaldo Carvalho de Melo

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=fa942e47-9654-dfa2-7a57-744b5fd3b790@intel.com \
    --to=adrian.hunter@intel.com \
    --cc=acme@kernel.org \
    --cc=ak@linux.intel.com \
    --cc=andi@firstfloor.org \
    --cc=jolsa@kernel.org \
    --cc=kim.phillips@arm.com \
    --cc=linux-kernel@vger.kernel.org \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).