[PATCH 1/3] perf pt: Mark PT return events as "return"

[PATCH 1/3] perf pt: Mark PT return events as "return"

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

With perf script --itrace=cr we can synthesize calls and returns out of
a PT log. However both calls and returns are marked with the same event,
called branches. This makes it difficult to read and post process,
because calls and returns are somewhat diffferent.

Create a separate return event and mark the returns as return.

Cc: adrian.hunter@intel.com
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/util/intel-pt.c | 53 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index 137196990012..4f0b1d7adf50 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -82,9 +82,12 @@ struct intel_pt {
 	u64 instructions_id;
 
 	bool sample_branches;
+	bool sample_returns;
 	u32 branches_filter;
 	u64 branches_sample_type;
+	u64 returns_sample_type;
 	u64 branches_id;
+	u64 returns_id;
 
 	bool sample_transactions;
 	u64 transactions_sample_type;
@@ -960,7 +963,8 @@ static int intel_pt_inject_event(union perf_event *event,
 	return perf_event__synthesize_sample(event, type, 0, sample, swapped);
 }
 
-static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
+static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq,
+					bool is_return)
 {
 	int ret;
 	struct intel_pt *pt = ptq->pt;
@@ -990,8 +994,13 @@ static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
 	sample.pid = ptq->pid;
 	sample.tid = ptq->tid;
 	sample.addr = ptq->state->to_ip;
-	sample.id = ptq->pt->branches_id;
-	sample.stream_id = ptq->pt->branches_id;
+	if (is_return) {
+		sample.id = ptq->pt->returns_id;
+		sample.stream_id = ptq->pt->returns_id;
+	} else {
+		sample.id = ptq->pt->branches_id;
+		sample.stream_id = ptq->pt->branches_id;
+	}
 	sample.period = 1;
 	sample.cpu = ptq->cpu;
 	sample.flags = ptq->flags;
@@ -1014,6 +1023,8 @@ static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
 
 	if (pt->synth_opts.inject) {
 		ret = intel_pt_inject_event(event, &sample,
+					    is_return ?
+					    pt->returns_sample_type :
 					    pt->branches_sample_type,
 					    pt->synth_needs_swap);
 		if (ret)
@@ -1241,7 +1252,13 @@ static int intel_pt_sample(struct intel_pt_queue *ptq)
 		thread_stack__set_trace_nr(ptq->thread, state->trace_nr);
 
 	if (pt->sample_branches) {
-		err = intel_pt_synth_branch_sample(ptq);
+		err = intel_pt_synth_branch_sample(ptq, false);
+		if (err)
+			return err;
+	}
+
+	if (pt->sample_returns) {
+		err = intel_pt_synth_branch_sample(ptq, true);
 		if (err)
 			return err;
 	}
@@ -1956,7 +1973,33 @@ static int intel_pt_synth_events(struct intel_pt *pt,
 		}
 		pt->sample_branches = true;
 		pt->branches_sample_type = attr.sample_type;
-		pt->branches_id = id;
+		pt->branches_id = id++;
+	}
+	if (pt->synth_opts.returns) {
+		attr.config = PERF_COUNT_HW_BRANCH_INSTRUCTIONS;
+		attr.sample_period = 1;
+		attr.sample_type |= PERF_SAMPLE_ADDR;
+		attr.sample_type &= ~(u64)PERF_SAMPLE_CALLCHAIN;
+		attr.sample_type &= ~(u64)PERF_SAMPLE_BRANCH_STACK;
+		pr_debug("Synthesizing 'return' event with id %" PRIu64 " sample type %#" PRIx64 "\n",
+			 id, (u64)attr.sample_type);
+		err = intel_pt_synth_event(session, &attr, id);
+		if (err) {
+			pr_err("%s: failed to synthesize 'return' event type\n",
+			       __func__);
+			return err;
+		}
+		pt->sample_returns = true;
+		pt->returns_sample_type = attr.sample_type;
+		pt->returns_id = id;
+		evlist__for_each(evlist, evsel) {
+			if (evsel->id && evsel->id[0] == pt->returns_id) {
+				if (evsel->name)
+					zfree(&evsel->name);
+				evsel->name = strdup("return");
+				break;
+			}
+		}
 	}
 
 	pt->synth_needs_swap = evsel->needs_swap;
-- 
2.5.5

[PATCH 2/3] perf util: Move get_main_thread to thread.c

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Move the get_main_thread function from db-export.c to thread.c
so that it can be used elsewhere.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/util/db-export.c | 13 +------------
 tools/perf/util/db-export.h |  2 ++
 tools/perf/util/thread.c    | 11 +++++++++++
 tools/perf/util/thread.h    |  2 ++
 4 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c
index 8d96c80cc67e..0fb0aeb0b09b 100644
--- a/tools/perf/util/db-export.c
+++ b/tools/perf/util/db-export.c
@@ -233,17 +233,6 @@ int db_export__symbol(struct db_export *dbe, struct symbol *sym,
 	return 0;
 }
 
-static struct thread *get_main_thread(struct machine *machine, struct thread *thread)
-{
-	if (thread->pid_ == thread->tid)
-		return thread__get(thread);
-
-	if (thread->pid_ == -1)
-		return NULL;
-
-	return machine__find_thread(machine, thread->pid_, thread->pid_);
-}
-
 static int db_ids_from_al(struct db_export *dbe, struct addr_location *al,
 			  u64 *dso_db_id, u64 *sym_db_id, u64 *offset)
 {
@@ -383,7 +372,7 @@ int db_export__sample(struct db_export *dbe, union perf_event *event,
 	if (err)
 		return err;
 
-	main_thread = get_main_thread(al->machine, thread);
+	main_thread = thread__main_thread(al->machine, thread);
 	if (main_thread)
 		comm = machine__thread_exec_comm(al->machine, main_thread);
 
diff --git a/tools/perf/util/db-export.h b/tools/perf/util/db-export.h
index 67bc6b8ad2d6..1241e5773e13 100644
--- a/tools/perf/util/db-export.h
+++ b/tools/perf/util/db-export.h
@@ -106,4 +106,6 @@ int db_export__branch_types(struct db_export *dbe);
 int db_export__call_path(struct db_export *dbe, struct call_path *cp);
 int db_export__call_return(struct db_export *dbe, struct call_return *cr);
 
+struct thread *db_export__main_thread(struct machine *machine, struct thread *thread);
+
 #endif
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 45fcb715a36b..ada58e6070bf 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -265,3 +265,14 @@ void thread__find_cpumode_addr_location(struct thread *thread,
 			break;
 	}
 }
+
+struct thread *thread__main_thread(struct machine *machine, struct thread *thread)
+{
+	if (thread->pid_ == thread->tid)
+		return thread__get(thread);
+
+	if (thread->pid_ == -1)
+		return NULL;
+
+	return machine__find_thread(machine, thread->pid_, thread->pid_);
+}
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index 45fba13c800b..08fcb14cf637 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -81,6 +81,8 @@ void thread__insert_map(struct thread *thread, struct map *map);
 int thread__fork(struct thread *thread, struct thread *parent, u64 timestamp);
 size_t thread__fprintf(struct thread *thread, FILE *fp);
 
+struct thread *thread__main_thread(struct machine *machine, struct thread *thread);
+
 void thread__find_addr_map(struct thread *thread,
 			   u8 cpumode, enum map_type type, u64 addr,
 			   struct addr_location *al);
-- 
2.5.5

[PATCH 3/3] perf script: Support callindent

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

When printing PT instruction traces with perf script
it is rather useful to see some indentation for the call tree. This
patch adds a new callindent field to perf script that prints
spaces for the function call stack depth.

We already have code to track the function call stack for PT,
previously used for the data base export. We can reuse that code
directly with minor modifications.

The resulting output is not quite as nice as ftrace yet, but
a lot better than what was there before.

Note there are some corner cases when the thread stack gets code confused
and prints incorrect indentation. Even with that it is fairly useful.

When displaying kernel code traces it is recommended to run as root, as
otherwise perf doesn't understand the kernel addresses properly, and may not
reset the call stack correctly on kernel boundaries.

Example output:

$ perf record -a -e intel_pt// sleep 1
$ perf script --ns -F callindent,time,comm,pid,sym,addr,event --itrace=cr
...
         swapper     0 126327.044742970:             return:      => ffffffff810aa999 cpu_idle_loop
         swapper     0 126327.044742970:           branches:          => ffffffff81525400 cpuidle_reflect
         swapper     0 126327.044742970:             return:              => ffffffff81525400 cpuidle_reflect
         swapper     0 126327.044742970:           branches:          => ffffffff810aa9ce cpu_idle_loop
         swapper     0 126327.044742970:             return:      => ffffffff810aa9ce cpu_idle_loop
         swapper     0 126327.044742970:           branches:          => ffffffff810cb0b0 rcu_idle_exit
         swapper     0 126327.044742970:             return:              => ffffffff810cb0b0 rcu_idle_exit
         swapper     0 126327.044742970:           branches:                  => ffffffff810c99d0 rcu_eqs_exit_common.isra.43
         swapper     0 126327.044742970:             return:                      => ffffffff810c99d0 rcu_eqs_exit_common.isra.43
         swapper     0 126327.044742970:           branches:                  => ffffffff810cb124 rcu_idle_exit
         swapper     0 126327.044742970:             return:              => ffffffff810cb124 rcu_idle_exit
         swapper     0 126327.044742970:           branches:          => ffffffff810aa719 cpu_idle_loop
         swapper     0 126327.044742970:             return:      => ffffffff810aa719 cpu_idle_loop

Cc: adrian.hunter@intel.com
v2: Move get_main_thread in separate patch. Add thread__put.
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/builtin-script.c    | 46 ++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/thread-stack.c |  7 +++++++
 tools/perf/util/thread-stack.h |  1 +
 3 files changed, 54 insertions(+)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index efca81679bb3..763f538c9a41 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -21,6 +21,7 @@
 #include "util/cpumap.h"
 #include "util/thread_map.h"
 #include "util/stat.h"
+#include "util/thread-stack.h"
 #include <linux/bitmap.h>
 #include <linux/stringify.h>
 #include "asm/bug.h"
@@ -63,6 +64,7 @@ enum perf_output_field {
 	PERF_OUTPUT_DATA_SRC	    = 1U << 17,
 	PERF_OUTPUT_WEIGHT	    = 1U << 18,
 	PERF_OUTPUT_BPF_OUTPUT	    = 1U << 19,
+	PERF_OUTPUT_CALLINDENT	    = 1U << 20,
 };
 
 struct output_option {
@@ -89,6 +91,7 @@ struct output_option {
 	{.str = "data_src", .field = PERF_OUTPUT_DATA_SRC},
 	{.str = "weight",   .field = PERF_OUTPUT_WEIGHT},
 	{.str = "bpf-output",   .field = PERF_OUTPUT_BPF_OUTPUT},
+	{.str = "callindent", .field = PERF_OUTPUT_CALLINDENT},
 };
 
 /* default set to maintain compatibility with current format */
@@ -261,6 +264,11 @@ static int perf_evsel__check_attr(struct perf_evsel *evsel,
 					PERF_OUTPUT_WEIGHT))
 		return -EINVAL;
 
+	if (PRINT_FIELD(CALLINDENT) && !PRINT_FIELD(ADDR)) {
+		pr_err("Display of callindent requested, but no branch address\n");
+		return -EINVAL;
+	}
+
 	if (PRINT_FIELD(SYM) && !PRINT_FIELD(IP) && !PRINT_FIELD(ADDR)) {
 		pr_err("Display of symbols requested but neither sample IP nor "
 			   "sample address\nis selected. Hence, no addresses to convert "
@@ -559,6 +567,41 @@ static void print_sample_addr(struct perf_sample *sample,
 	}
 }
 
+static int dummy_call_return(struct call_return *cr __maybe_unused,
+				     void *arg __maybe_unused)
+{
+	return 0;
+}
+
+static void print_sample_callindent(struct perf_sample *sample,
+				    struct perf_evsel *evsel,
+				    struct thread *thread,
+				    struct addr_location *al)
+{
+	struct perf_event_attr *attr = &evsel->attr;
+
+	if (sample_addr_correlates_sym(attr)) {
+		static struct call_return_processor *crp;
+		struct addr_location addr_al;
+		struct thread *main_thread;
+		struct comm *comm;
+
+		if (!crp)
+			crp = call_return_processor__new(dummy_call_return,
+							 NULL);
+		thread__resolve(thread, &addr_al, sample);
+		main_thread = thread__main_thread(al->machine, thread);
+		if (main_thread) {
+			comm = machine__thread_exec_comm(al->machine,
+							 main_thread);
+			thread__put(main_thread);
+		}
+		thread_stack__process(thread, comm, sample, al, &addr_al,
+							0, crp);
+	}
+	thread_stack__print_indent(thread);
+}
+
 static void print_sample_bts(struct perf_sample *sample,
 			     struct perf_evsel *evsel,
 			     struct thread *thread,
@@ -567,6 +610,9 @@ static void print_sample_bts(struct perf_sample *sample,
 	struct perf_event_attr *attr = &evsel->attr;
 	bool print_srcline_last = false;
 
+	if (PRINT_FIELD(CALLINDENT))
+		print_sample_callindent(sample, evsel, thread, al);
+
 	/* print branch_from information */
 	if (PRINT_FIELD(IP)) {
 		unsigned int print_opts = output[attr->type].print_ip_opts;
diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
index 825086aa9a08..20e57f263c41 100644
--- a/tools/perf/util/thread-stack.c
+++ b/tools/perf/util/thread-stack.c
@@ -616,3 +616,10 @@ int thread_stack__process(struct thread *thread, struct comm *comm,
 
 	return err;
 }
+
+void thread_stack__print_indent(struct thread *thread)
+{
+	if (!thread->ts)
+		return;
+	printf("%*s", (int)thread->ts->cnt * 4, "");
+}
diff --git a/tools/perf/util/thread-stack.h b/tools/perf/util/thread-stack.h
index ad44c7944b8e..7fad96415f88 100644
--- a/tools/perf/util/thread-stack.h
+++ b/tools/perf/util/thread-stack.h
@@ -87,6 +87,7 @@ void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
 			  size_t sz, u64 ip);
 int thread_stack__flush(struct thread *thread);
 void thread_stack__free(struct thread *thread);
+void thread_stack__print_indent(struct thread *thread);
 
 struct call_return_processor *
 call_return_processor__new(int (*process)(struct call_return *cr, void *data),
-- 
2.5.5

Re: [PATCH 2/3] perf util: Move get_main_thread to thread.c

[Arnaldo Carvalho de Melo]

Em Mon, May 23, 2016 at 05:52:24PM -0700, Andi Kleen escreveu:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Move the get_main_thread function from db-export.c to thread.c
> so that it can be used elsewhere.

After removing the db-export.h leftover junk, applied.

- Arnaldo 
 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  tools/perf/util/db-export.c | 13 +------------
>  tools/perf/util/db-export.h |  2 ++
>  tools/perf/util/thread.c    | 11 +++++++++++
>  tools/perf/util/thread.h    |  2 ++
>  4 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c
> index 8d96c80cc67e..0fb0aeb0b09b 100644
> --- a/tools/perf/util/db-export.c
> +++ b/tools/perf/util/db-export.c
> @@ -233,17 +233,6 @@ int db_export__symbol(struct db_export *dbe, struct symbol *sym,
>  	return 0;
>  }
>  
> -static struct thread *get_main_thread(struct machine *machine, struct thread *thread)
> -{
> -	if (thread->pid_ == thread->tid)
> -		return thread__get(thread);
> -
> -	if (thread->pid_ == -1)
> -		return NULL;
> -
> -	return machine__find_thread(machine, thread->pid_, thread->pid_);
> -}
> -
>  static int db_ids_from_al(struct db_export *dbe, struct addr_location *al,
>  			  u64 *dso_db_id, u64 *sym_db_id, u64 *offset)
>  {
> @@ -383,7 +372,7 @@ int db_export__sample(struct db_export *dbe, union perf_event *event,
>  	if (err)
>  		return err;
>  
> -	main_thread = get_main_thread(al->machine, thread);
> +	main_thread = thread__main_thread(al->machine, thread);
>  	if (main_thread)
>  		comm = machine__thread_exec_comm(al->machine, main_thread);
>  
> diff --git a/tools/perf/util/db-export.h b/tools/perf/util/db-export.h
> index 67bc6b8ad2d6..1241e5773e13 100644
> --- a/tools/perf/util/db-export.h
> +++ b/tools/perf/util/db-export.h
> @@ -106,4 +106,6 @@ int db_export__branch_types(struct db_export *dbe);
>  int db_export__call_path(struct db_export *dbe, struct call_path *cp);
>  int db_export__call_return(struct db_export *dbe, struct call_return *cr);
>  
> +struct thread *db_export__main_thread(struct machine *machine, struct thread *thread);
> +
>  #endif
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index 45fcb715a36b..ada58e6070bf 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -265,3 +265,14 @@ void thread__find_cpumode_addr_location(struct thread *thread,
>  			break;
>  	}
>  }
> +
> +struct thread *thread__main_thread(struct machine *machine, struct thread *thread)
> +{
> +	if (thread->pid_ == thread->tid)
> +		return thread__get(thread);
> +
> +	if (thread->pid_ == -1)
> +		return NULL;
> +
> +	return machine__find_thread(machine, thread->pid_, thread->pid_);
> +}
> diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> index 45fba13c800b..08fcb14cf637 100644
> --- a/tools/perf/util/thread.h
> +++ b/tools/perf/util/thread.h
> @@ -81,6 +81,8 @@ void thread__insert_map(struct thread *thread, struct map *map);
>  int thread__fork(struct thread *thread, struct thread *parent, u64 timestamp);
>  size_t thread__fprintf(struct thread *thread, FILE *fp);
>  
> +struct thread *thread__main_thread(struct machine *machine, struct thread *thread);
> +
>  void thread__find_addr_map(struct thread *thread,
>  			   u8 cpumode, enum map_type type, u64 addr,
>  			   struct addr_location *al);
> -- 
> 2.5.5

Re: [PATCH 1/3] perf pt: Mark PT return events as "return"

[Arnaldo Carvalho de Melo]

Em Mon, May 23, 2016 at 05:52:23PM -0700, Andi Kleen escreveu:
> From: Andi Kleen <ak@linux.intel.com>
> 
> With perf script --itrace=cr we can synthesize calls and returns out of
> a PT log. However both calls and returns are marked with the same event,
> called branches. This makes it difficult to read and post process,
> because calls and returns are somewhat diffferent.
> 
> Create a separate return event and mark the returns as return.

Adrian, you voiced some concerns about this patch, are those settled?
Can I have your Acked-by, please?

- Arnaldo
 
> Cc: adrian.hunter@intel.com
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  tools/perf/util/intel-pt.c | 53 +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 48 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index 137196990012..4f0b1d7adf50 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -82,9 +82,12 @@ struct intel_pt {
>  	u64 instructions_id;
>  
>  	bool sample_branches;
> +	bool sample_returns;
>  	u32 branches_filter;
>  	u64 branches_sample_type;
> +	u64 returns_sample_type;
>  	u64 branches_id;
> +	u64 returns_id;
>  
>  	bool sample_transactions;
>  	u64 transactions_sample_type;
> @@ -960,7 +963,8 @@ static int intel_pt_inject_event(union perf_event *event,
>  	return perf_event__synthesize_sample(event, type, 0, sample, swapped);
>  }
>  
> -static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
> +static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq,
> +					bool is_return)
>  {
>  	int ret;
>  	struct intel_pt *pt = ptq->pt;
> @@ -990,8 +994,13 @@ static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
>  	sample.pid = ptq->pid;
>  	sample.tid = ptq->tid;
>  	sample.addr = ptq->state->to_ip;
> -	sample.id = ptq->pt->branches_id;
> -	sample.stream_id = ptq->pt->branches_id;
> +	if (is_return) {
> +		sample.id = ptq->pt->returns_id;
> +		sample.stream_id = ptq->pt->returns_id;
> +	} else {
> +		sample.id = ptq->pt->branches_id;
> +		sample.stream_id = ptq->pt->branches_id;
> +	}
>  	sample.period = 1;
>  	sample.cpu = ptq->cpu;
>  	sample.flags = ptq->flags;
> @@ -1014,6 +1023,8 @@ static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
>  
>  	if (pt->synth_opts.inject) {
>  		ret = intel_pt_inject_event(event, &sample,
> +					    is_return ?
> +					    pt->returns_sample_type :
>  					    pt->branches_sample_type,
>  					    pt->synth_needs_swap);
>  		if (ret)
> @@ -1241,7 +1252,13 @@ static int intel_pt_sample(struct intel_pt_queue *ptq)
>  		thread_stack__set_trace_nr(ptq->thread, state->trace_nr);
>  
>  	if (pt->sample_branches) {
> -		err = intel_pt_synth_branch_sample(ptq);
> +		err = intel_pt_synth_branch_sample(ptq, false);
> +		if (err)
> +			return err;
> +	}
> +
> +	if (pt->sample_returns) {
> +		err = intel_pt_synth_branch_sample(ptq, true);
>  		if (err)
>  			return err;
>  	}
> @@ -1956,7 +1973,33 @@ static int intel_pt_synth_events(struct intel_pt *pt,
>  		}
>  		pt->sample_branches = true;
>  		pt->branches_sample_type = attr.sample_type;
> -		pt->branches_id = id;
> +		pt->branches_id = id++;
> +	}
> +	if (pt->synth_opts.returns) {
> +		attr.config = PERF_COUNT_HW_BRANCH_INSTRUCTIONS;
> +		attr.sample_period = 1;
> +		attr.sample_type |= PERF_SAMPLE_ADDR;
> +		attr.sample_type &= ~(u64)PERF_SAMPLE_CALLCHAIN;
> +		attr.sample_type &= ~(u64)PERF_SAMPLE_BRANCH_STACK;
> +		pr_debug("Synthesizing 'return' event with id %" PRIu64 " sample type %#" PRIx64 "\n",
> +			 id, (u64)attr.sample_type);
> +		err = intel_pt_synth_event(session, &attr, id);
> +		if (err) {
> +			pr_err("%s: failed to synthesize 'return' event type\n",
> +			       __func__);
> +			return err;
> +		}
> +		pt->sample_returns = true;
> +		pt->returns_sample_type = attr.sample_type;
> +		pt->returns_id = id;
> +		evlist__for_each(evlist, evsel) {
> +			if (evsel->id && evsel->id[0] == pt->returns_id) {
> +				if (evsel->name)
> +					zfree(&evsel->name);
> +				evsel->name = strdup("return");
> +				break;
> +			}
> +		}
>  	}
>  
>  	pt->synth_needs_swap = evsel->needs_swap;
> -- 
> 2.5.5

Re: [PATCH 1/3] perf pt: Mark PT return events as "return"

[Adrian Hunter]

On 24/05/2016 5:57 p.m., Arnaldo Carvalho de Melo wrote:
> Em Mon, May 23, 2016 at 05:52:23PM -0700, Andi Kleen escreveu:
>> From: Andi Kleen <ak@linux.intel.com>
>>
>> With perf script --itrace=cr we can synthesize calls and returns out of
>> a PT log. However both calls and returns are marked with the same event,
>> called branches. This makes it difficult to read and post process,
>> because calls and returns are somewhat diffferent.
>>
>> Create a separate return event and mark the returns as return.
>
> Adrian, you voiced some concerns about this patch, are those settled?

What about beautifying the sample flags i.e. instead of displaying the
letters interpret them into something more human readable e.g.

bc	call
br	return
bo	conditional jump
b	jump
bci	software interrupt
bri	return from interrupt
bcs	system call
brs	return from system call
by	asynchronous branch
bcyi	hardware interrupt
bA	transaction abort
bB	trace begin
bE	trace end

In Tx (x) can turn up on a number of those too.


> Can I have your Acked-by, please?
>
> - Arnaldo
>
>> Cc: adrian.hunter@intel.com
>> Signed-off-by: Andi Kleen <ak@linux.intel.com>
>> ---
>>   tools/perf/util/intel-pt.c | 53 +++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 48 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
>> index 137196990012..4f0b1d7adf50 100644
>> --- a/tools/perf/util/intel-pt.c
>> +++ b/tools/perf/util/intel-pt.c
>> @@ -82,9 +82,12 @@ struct intel_pt {
>>   	u64 instructions_id;
>>
>>   	bool sample_branches;
>> +	bool sample_returns;
>>   	u32 branches_filter;
>>   	u64 branches_sample_type;
>> +	u64 returns_sample_type;
>>   	u64 branches_id;
>> +	u64 returns_id;
>>
>>   	bool sample_transactions;
>>   	u64 transactions_sample_type;
>> @@ -960,7 +963,8 @@ static int intel_pt_inject_event(union perf_event *event,
>>   	return perf_event__synthesize_sample(event, type, 0, sample, swapped);
>>   }
>>
>> -static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
>> +static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq,
>> +					bool is_return)
>>   {
>>   	int ret;
>>   	struct intel_pt *pt = ptq->pt;
>> @@ -990,8 +994,13 @@ static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
>>   	sample.pid = ptq->pid;
>>   	sample.tid = ptq->tid;
>>   	sample.addr = ptq->state->to_ip;
>> -	sample.id = ptq->pt->branches_id;
>> -	sample.stream_id = ptq->pt->branches_id;
>> +	if (is_return) {
>> +		sample.id = ptq->pt->returns_id;
>> +		sample.stream_id = ptq->pt->returns_id;
>> +	} else {
>> +		sample.id = ptq->pt->branches_id;
>> +		sample.stream_id = ptq->pt->branches_id;
>> +	}
>>   	sample.period = 1;
>>   	sample.cpu = ptq->cpu;
>>   	sample.flags = ptq->flags;
>> @@ -1014,6 +1023,8 @@ static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
>>
>>   	if (pt->synth_opts.inject) {
>>   		ret = intel_pt_inject_event(event, &sample,
>> +					    is_return ?
>> +					    pt->returns_sample_type :
>>   					    pt->branches_sample_type,
>>   					    pt->synth_needs_swap);
>>   		if (ret)
>> @@ -1241,7 +1252,13 @@ static int intel_pt_sample(struct intel_pt_queue *ptq)
>>   		thread_stack__set_trace_nr(ptq->thread, state->trace_nr);
>>
>>   	if (pt->sample_branches) {
>> -		err = intel_pt_synth_branch_sample(ptq);
>> +		err = intel_pt_synth_branch_sample(ptq, false);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>> +	if (pt->sample_returns) {
>> +		err = intel_pt_synth_branch_sample(ptq, true);
>>   		if (err)
>>   			return err;
>>   	}
>> @@ -1956,7 +1973,33 @@ static int intel_pt_synth_events(struct intel_pt *pt,
>>   		}
>>   		pt->sample_branches = true;
>>   		pt->branches_sample_type = attr.sample_type;
>> -		pt->branches_id = id;
>> +		pt->branches_id = id++;
>> +	}
>> +	if (pt->synth_opts.returns) {
>> +		attr.config = PERF_COUNT_HW_BRANCH_INSTRUCTIONS;
>> +		attr.sample_period = 1;
>> +		attr.sample_type |= PERF_SAMPLE_ADDR;
>> +		attr.sample_type &= ~(u64)PERF_SAMPLE_CALLCHAIN;
>> +		attr.sample_type &= ~(u64)PERF_SAMPLE_BRANCH_STACK;
>> +		pr_debug("Synthesizing 'return' event with id %" PRIu64 " sample type %#" PRIx64 "\n",
>> +			 id, (u64)attr.sample_type);
>> +		err = intel_pt_synth_event(session, &attr, id);
>> +		if (err) {
>> +			pr_err("%s: failed to synthesize 'return' event type\n",
>> +			       __func__);
>> +			return err;
>> +		}
>> +		pt->sample_returns = true;
>> +		pt->returns_sample_type = attr.sample_type;
>> +		pt->returns_id = id;
>> +		evlist__for_each(evlist, evsel) {
>> +			if (evsel->id && evsel->id[0] == pt->returns_id) {
>> +				if (evsel->name)
>> +					zfree(&evsel->name);
>> +				evsel->name = strdup("return");
>> +				break;
>> +			}
>> +		}
>>   	}
>>
>>   	pt->synth_needs_swap = evsel->needs_swap;
>> --
>> 2.5.5

Re: [PATCH 1/3] perf pt: Mark PT return events as "return"

[Arnaldo Carvalho de Melo]

Em Tue, May 24, 2016 at 07:26:19PM +0300, Adrian Hunter escreveu:
> On 24/05/2016 5:57 p.m., Arnaldo Carvalho de Melo wrote:
> > Em Mon, May 23, 2016 at 05:52:23PM -0700, Andi Kleen escreveu:
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > With perf script --itrace=cr we can synthesize calls and returns out of
> > > a PT log. However both calls and returns are marked with the same event,
> > > called branches. This makes it difficult to read and post process,
> > > because calls and returns are somewhat diffferent.
> > > 
> > > Create a separate return event and mark the returns as return.
> > 
> > Adrian, you voiced some concerns about this patch, are those settled?
> 
> What about beautifying the sample flags i.e. instead of displaying the
> letters interpret them into something more human readable e.g.

Looks like a good idea, but can't this be done on top of what he sent?

- Arnaldo
 
> bc	call
> br	return
> bo	conditional jump
> b	jump
> bci	software interrupt
> bri	return from interrupt
> bcs	system call
> brs	return from system call
> by	asynchronous branch
> bcyi	hardware interrupt
> bA	transaction abort
> bB	trace begin
> bE	trace end
> 
> In Tx (x) can turn up on a number of those too.
> 
> 
> > Can I have your Acked-by, please?
> > 
> > - Arnaldo
> > 
> > > Cc: adrian.hunter@intel.com
> > > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> > > ---
> > >   tools/perf/util/intel-pt.c | 53 +++++++++++++++++++++++++++++++++++++++++-----
> > >   1 file changed, 48 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> > > index 137196990012..4f0b1d7adf50 100644
> > > --- a/tools/perf/util/intel-pt.c
> > > +++ b/tools/perf/util/intel-pt.c
> > > @@ -82,9 +82,12 @@ struct intel_pt {
> > >   	u64 instructions_id;
> > > 
> > >   	bool sample_branches;
> > > +	bool sample_returns;
> > >   	u32 branches_filter;
> > >   	u64 branches_sample_type;
> > > +	u64 returns_sample_type;
> > >   	u64 branches_id;
> > > +	u64 returns_id;
> > > 
> > >   	bool sample_transactions;
> > >   	u64 transactions_sample_type;
> > > @@ -960,7 +963,8 @@ static int intel_pt_inject_event(union perf_event *event,
> > >   	return perf_event__synthesize_sample(event, type, 0, sample, swapped);
> > >   }
> > > 
> > > -static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
> > > +static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq,
> > > +					bool is_return)
> > >   {
> > >   	int ret;
> > >   	struct intel_pt *pt = ptq->pt;
> > > @@ -990,8 +994,13 @@ static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
> > >   	sample.pid = ptq->pid;
> > >   	sample.tid = ptq->tid;
> > >   	sample.addr = ptq->state->to_ip;
> > > -	sample.id = ptq->pt->branches_id;
> > > -	sample.stream_id = ptq->pt->branches_id;
> > > +	if (is_return) {
> > > +		sample.id = ptq->pt->returns_id;
> > > +		sample.stream_id = ptq->pt->returns_id;
> > > +	} else {
> > > +		sample.id = ptq->pt->branches_id;
> > > +		sample.stream_id = ptq->pt->branches_id;
> > > +	}
> > >   	sample.period = 1;
> > >   	sample.cpu = ptq->cpu;
> > >   	sample.flags = ptq->flags;
> > > @@ -1014,6 +1023,8 @@ static int intel_pt_synth_branch_sample(struct intel_pt_queue *ptq)
> > > 
> > >   	if (pt->synth_opts.inject) {
> > >   		ret = intel_pt_inject_event(event, &sample,
> > > +					    is_return ?
> > > +					    pt->returns_sample_type :
> > >   					    pt->branches_sample_type,
> > >   					    pt->synth_needs_swap);
> > >   		if (ret)
> > > @@ -1241,7 +1252,13 @@ static int intel_pt_sample(struct intel_pt_queue *ptq)
> > >   		thread_stack__set_trace_nr(ptq->thread, state->trace_nr);
> > > 
> > >   	if (pt->sample_branches) {
> > > -		err = intel_pt_synth_branch_sample(ptq);
> > > +		err = intel_pt_synth_branch_sample(ptq, false);
> > > +		if (err)
> > > +			return err;
> > > +	}
> > > +
> > > +	if (pt->sample_returns) {
> > > +		err = intel_pt_synth_branch_sample(ptq, true);
> > >   		if (err)
> > >   			return err;
> > >   	}
> > > @@ -1956,7 +1973,33 @@ static int intel_pt_synth_events(struct intel_pt *pt,
> > >   		}
> > >   		pt->sample_branches = true;
> > >   		pt->branches_sample_type = attr.sample_type;
> > > -		pt->branches_id = id;
> > > +		pt->branches_id = id++;
> > > +	}
> > > +	if (pt->synth_opts.returns) {
> > > +		attr.config = PERF_COUNT_HW_BRANCH_INSTRUCTIONS;
> > > +		attr.sample_period = 1;
> > > +		attr.sample_type |= PERF_SAMPLE_ADDR;
> > > +		attr.sample_type &= ~(u64)PERF_SAMPLE_CALLCHAIN;
> > > +		attr.sample_type &= ~(u64)PERF_SAMPLE_BRANCH_STACK;
> > > +		pr_debug("Synthesizing 'return' event with id %" PRIu64 " sample type %#" PRIx64 "\n",
> > > +			 id, (u64)attr.sample_type);
> > > +		err = intel_pt_synth_event(session, &attr, id);
> > > +		if (err) {
> > > +			pr_err("%s: failed to synthesize 'return' event type\n",
> > > +			       __func__);
> > > +			return err;
> > > +		}
> > > +		pt->sample_returns = true;
> > > +		pt->returns_sample_type = attr.sample_type;
> > > +		pt->returns_id = id;
> > > +		evlist__for_each(evlist, evsel) {
> > > +			if (evsel->id && evsel->id[0] == pt->returns_id) {
> > > +				if (evsel->name)
> > > +					zfree(&evsel->name);
> > > +				evsel->name = strdup("return");
> > > +				break;
> > > +			}
> > > +		}
> > >   	}
> > > 
> > >   	pt->synth_needs_swap = evsel->needs_swap;
> > > --
> > > 2.5.5

Re: [PATCH 1/3] perf pt: Mark PT return events as "return"

[Andi Kleen]

> > What about beautifying the sample flags i.e. instead of displaying the
> > letters interpret them into something more human readable e.g.
> 
> Looks like a good idea, but can't this be done on top of what he sent?

Yes it both makes sense together.
-Andi

Re: [PATCH 1/3] perf pt: Mark PT return events as "return"

[Adrian Hunter]

On 24/05/2016 10:05 p.m., Andi Kleen wrote:
>>> What about beautifying the sample flags i.e. instead of displaying the
>>> letters interpret them into something more human readable e.g.
>>
>> Looks like a good idea, but can't this be done on top of what he sent?
>
> Yes it both makes sense together.

OK I will look at the patch tomorrow.

It looks like it synths 2 samples for each return (one "branches" and
one "return"), which I would expect would mess up the thread_stack pairing
calls and returns.  Did you intend to have 2 samples?

Re: [PATCH 1/3] perf pt: Mark PT return events as "return"

[Andi Kleen]

> It looks like it synths 2 samples for each return (one "branches" and
> one "return"), which I would expect would mess up the thread_stack pairing
> calls and returns.  Did you intend to have 2 samples?

No it should be only one. I'll check.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

[tip:perf/core] perf thread: Adopt get_main_thread from db-export.c

[tip-bot for Andi Kleen]

Commit-ID:  480ca357fd7f86a381a5b35a8157aa176eddbed4
Gitweb:     http://git.kernel.org/tip/480ca357fd7f86a381a5b35a8157aa176eddbed4
Author:     Andi Kleen <ak@linux.intel.com>
AuthorDate: Mon, 23 May 2016 17:52:24 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 30 May 2016 12:41:43 -0300

perf thread: Adopt get_main_thread from db-export.c

Move the get_main_thread function from db-export.c to thread.c so that
it can be used elsewhere.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Link: http://lkml.kernel.org/r/1464051145-19968-2-git-send-email-andi@firstfloor.org
[ Removed leftover bits from db-export.h ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/db-export.c | 13 +------------
 tools/perf/util/thread.c    | 11 +++++++++++
 tools/perf/util/thread.h    |  2 ++
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c
index c9a6dc1..b0c2b5c 100644
--- a/tools/perf/util/db-export.c
+++ b/tools/perf/util/db-export.c
@@ -233,17 +233,6 @@ int db_export__symbol(struct db_export *dbe, struct symbol *sym,
 	return 0;
 }
 
-static struct thread *get_main_thread(struct machine *machine, struct thread *thread)
-{
-	if (thread->pid_ == thread->tid)
-		return thread__get(thread);
-
-	if (thread->pid_ == -1)
-		return NULL;
-
-	return machine__find_thread(machine, thread->pid_, thread->pid_);
-}
-
 static int db_ids_from_al(struct db_export *dbe, struct addr_location *al,
 			  u64 *dso_db_id, u64 *sym_db_id, u64 *offset)
 {
@@ -382,7 +371,7 @@ int db_export__sample(struct db_export *dbe, union perf_event *event,
 	if (err)
 		return err;
 
-	main_thread = get_main_thread(al->machine, thread);
+	main_thread = thread__main_thread(al->machine, thread);
 	if (main_thread)
 		comm = machine__thread_exec_comm(al->machine, main_thread);
 
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 45fcb71..ada58e6 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -265,3 +265,14 @@ void thread__find_cpumode_addr_location(struct thread *thread,
 			break;
 	}
 }
+
+struct thread *thread__main_thread(struct machine *machine, struct thread *thread)
+{
+	if (thread->pid_ == thread->tid)
+		return thread__get(thread);
+
+	if (thread->pid_ == -1)
+		return NULL;
+
+	return machine__find_thread(machine, thread->pid_, thread->pid_);
+}
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index 45fba13..08fcb14 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -81,6 +81,8 @@ void thread__insert_map(struct thread *thread, struct map *map);
 int thread__fork(struct thread *thread, struct thread *parent, u64 timestamp);
 size_t thread__fprintf(struct thread *thread, FILE *fp);
 
+struct thread *thread__main_thread(struct machine *machine, struct thread *thread);
+
 void thread__find_addr_map(struct thread *thread,
 			   u8 cpumode, enum map_type type, u64 addr,
 			   struct addr_location *al);