[PATCH 0/5] perf tools: Fix for cases where cpumode is incorrect or insufficient

[PATCH 0/5] perf tools: Fix for cases where cpumode is incorrect or insufficient

[Adrian Hunter]

Hi

These patches probably deal with most cases except the one fixed by David
Miller's "perf callchain: Honour the ordering of
PERF_CONTEXT_{USER,KERNEL,etc}" patch, and also cat_backtrace() which looks
like it has the same problem, and the cs-etm fix.


Adrian Hunter (5):
      perf tools: Add fallback functions for cases where cpumode is insufficient
      perf tools: Use fallback for sample_addr_correlates_sym() cases
      perf tools: Use fallbacks for branch stacks
      perf intel-pt: Insert callchain context into synthesized callchains
      perf intel-pt/bts: Calculate cpumode for synthesized samples

 tools/perf/builtin-script.c                        | 12 +++---
 tools/perf/util/event.c                            | 29 +++++++++++++-
 tools/perf/util/intel-bts.c                        | 17 ++++++---
 tools/perf/util/intel-pt.c                         | 28 ++++++++------
 tools/perf/util/machine.c                          | 40 ++++++++++++++++++++
 tools/perf/util/machine.h                          |  2 +
 .../util/scripting-engines/trace-event-python.c    | 16 ++++----
 tools/perf/util/thread-stack.c                     | 44 +++++++++++++++++-----
 tools/perf/util/thread-stack.h                     |  2 +-
 tools/perf/util/thread.h                           |  4 ++
 10 files changed, 153 insertions(+), 41 deletions(-)


Regards
Adrian

[PATCH 1/5] perf tools: Add fallback functions for cases where cpumode is insufficient

[Adrian Hunter]

For branch stacks or branch samples, the sample cpumode might not be
correct because it applies only to the sample 'ip' and not necessary to
'addr' or branch stack addresses. Add fallback functions that can be used
to deal with those cases

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: stable@vger.kernel.org # 4.19
---
 tools/perf/util/event.c   | 27 ++++++++++++++++++++++++++
 tools/perf/util/machine.c | 40 +++++++++++++++++++++++++++++++++++++++
 tools/perf/util/machine.h |  2 ++
 tools/perf/util/thread.h  |  4 ++++
 4 files changed, 73 insertions(+)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index bc646185f8d9..52b24b8ce29e 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -1576,6 +1576,24 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
 	return al->map;
 }
 
+/*
+ * For branch stacks or branch samples, the sample cpumode might not be correct
+ * because it applies only to the sample 'ip' and not necessary to 'addr' or
+ * branch stack addresses. If possible, use a fallback to deal with those cases.
+ */
+struct map *thread__find_map_fallback(struct thread *thread, u8 cpumode,
+				      u64 addr, struct addr_location *al)
+{
+	struct map *map = thread__find_map(thread, cpumode, addr, al);
+	struct machine *machine = thread->mg->machine;
+	u8 addr_cpumode = machine__addr_cpumode(machine, cpumode, addr);
+
+	if (map || addr_cpumode == cpumode)
+		return map;
+
+	return thread__find_map(thread, addr_cpumode, addr, al);
+}
+
 struct symbol *thread__find_symbol(struct thread *thread, u8 cpumode,
 				   u64 addr, struct addr_location *al)
 {
@@ -1585,6 +1603,15 @@ struct symbol *thread__find_symbol(struct thread *thread, u8 cpumode,
 	return al->sym;
 }
 
+struct symbol *thread__find_symbol_fb(struct thread *thread, u8 cpumode,
+				      u64 addr, struct addr_location *al)
+{
+	al->sym = NULL;
+	if (thread__find_map_fallback(thread, cpumode, addr, al))
+		al->sym = map__find_symbol(al->map, al->addr);
+	return al->sym;
+}
+
 /*
  * Callers need to drop the reference to al->thread, obtained in
  * machine__findnew_thread()
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 111ae858cbcb..04edc0eac376 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -2542,6 +2542,46 @@ int machine__get_kernel_start(struct machine *machine)
 	return err;
 }
 
+/*
+ * machine__single_ku_as - Machine has same address space for kernel and user.
+ * @machine: machine object
+ *
+ * Some architectures have a single address space for kernel and user addresses,
+ * which makes it possible to determine if an address is in kernel space or user
+ * space.
+ */
+static bool machine__single_ku_as(struct machine *machine)
+{
+	return strcmp(perf_env__arch(machine->env), "sparc");
+}
+
+u8 machine__addr_cpumode(struct machine *machine, u8 cpumode, u64 addr)
+{
+	u8 addr_cpumode = cpumode;
+	bool kernel_ip;
+
+	if (!machine__single_ku_as(machine))
+		goto out;
+
+	kernel_ip = machine__kernel_ip(machine, addr);
+	switch (cpumode) {
+	case PERF_RECORD_MISC_KERNEL:
+	case PERF_RECORD_MISC_USER:
+		addr_cpumode = kernel_ip ? PERF_RECORD_MISC_KERNEL :
+					   PERF_RECORD_MISC_USER;
+		break;
+	case PERF_RECORD_MISC_GUEST_KERNEL:
+	case PERF_RECORD_MISC_GUEST_USER:
+		addr_cpumode = kernel_ip ? PERF_RECORD_MISC_GUEST_KERNEL :
+					   PERF_RECORD_MISC_GUEST_USER;
+		break;
+	default:
+		break;
+	}
+out:
+	return addr_cpumode;
+}
+
 struct dso *machine__findnew_dso(struct machine *machine, const char *filename)
 {
 	return dsos__findnew(&machine->dsos, filename);
diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
index d856b85862e2..20d464135ed1 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -99,6 +99,8 @@ static inline bool machine__kernel_ip(struct machine *machine, u64 ip)
 	return ip >= kernel_start;
 }
 
+u8 machine__addr_cpumode(struct machine *machine, u8 cpumode, u64 addr);
+
 struct thread *machine__find_thread(struct machine *machine, pid_t pid,
 				    pid_t tid);
 struct comm *machine__thread_exec_comm(struct machine *machine,
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index 36c09a9904e6..e28f9dc1110a 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -96,9 +96,13 @@ struct thread *thread__main_thread(struct machine *machine, struct thread *threa
 
 struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
 			     struct addr_location *al);
+struct map *thread__find_map_fallback(struct thread *thread, u8 cpumode,
+				      u64 addr, struct addr_location *al);
 
 struct symbol *thread__find_symbol(struct thread *thread, u8 cpumode,
 				   u64 addr, struct addr_location *al);
+struct symbol *thread__find_symbol_fb(struct thread *thread, u8 cpumode,
+				      u64 addr, struct addr_location *al);
 
 void thread__find_cpumode_addr_location(struct thread *thread, u64 addr,
 					struct addr_location *al);
-- 
2.17.1

[PATCH 2/5] perf tools: Use fallback for sample_addr_correlates_sym() cases

[Adrian Hunter]

thread__resolve() is used in the sample_addr_correlates_sym() cases where
'addr' is a destination of a branch which does not necessarily have the
same cpumode as the 'ip'. Use the fallback function in that case.

This patch depends on patch "perf tools: Add fallback functions for cases
where cpumode is insufficient".

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: stable@vger.kernel.org # 4.19
---
 tools/perf/util/event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 52b24b8ce29e..316f62ffa27b 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -1705,7 +1705,7 @@ bool sample_addr_correlates_sym(struct perf_event_attr *attr)
 void thread__resolve(struct thread *thread, struct addr_location *al,
 		     struct perf_sample *sample)
 {
-	thread__find_map(thread, sample->cpumode, sample->addr, al);
+	thread__find_map_fallback(thread, sample->cpumode, sample->addr, al);
 
 	al->cpu = sample->cpu;
 	al->sym = NULL;
-- 
2.17.1

[PATCH 3/5] perf tools: Use fallbacks for branch stacks

[Adrian Hunter]

Branch stacks do not necessarily have the same cpumode as the 'ip'. Use the
fallback functions in those cases.

This patch depends on patch "perf tools: Add fallback functions for cases
where cpumode is insufficient".

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: stable@vger.kernel.org # 4.19
---
 tools/perf/builtin-script.c                      | 12 ++++++------
 .../util/scripting-engines/trace-event-python.c  | 16 ++++++++--------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index b5bc85bd0bbe..996317d8e183 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -728,8 +728,8 @@ static int perf_sample__fprintf_brstack(struct perf_sample *sample,
 		if (PRINT_FIELD(DSO)) {
 			memset(&alf, 0, sizeof(alf));
 			memset(&alt, 0, sizeof(alt));
-			thread__find_map(thread, sample->cpumode, from, &alf);
-			thread__find_map(thread, sample->cpumode, to, &alt);
+			thread__find_map_fallback(thread, sample->cpumode, from, &alf);
+			thread__find_map_fallback(thread, sample->cpumode, to, &alt);
 		}
 
 		printed += fprintf(fp, " 0x%"PRIx64, from);
@@ -775,8 +775,8 @@ static int perf_sample__fprintf_brstacksym(struct perf_sample *sample,
 		from = br->entries[i].from;
 		to   = br->entries[i].to;
 
-		thread__find_symbol(thread, sample->cpumode, from, &alf);
-		thread__find_symbol(thread, sample->cpumode, to, &alt);
+		thread__find_symbol_fb(thread, sample->cpumode, from, &alf);
+		thread__find_symbol_fb(thread, sample->cpumode, to, &alt);
 
 		printed += symbol__fprintf_symname_offs(alf.sym, &alf, fp);
 		if (PRINT_FIELD(DSO)) {
@@ -820,11 +820,11 @@ static int perf_sample__fprintf_brstackoff(struct perf_sample *sample,
 		from = br->entries[i].from;
 		to   = br->entries[i].to;
 
-		if (thread__find_map(thread, sample->cpumode, from, &alf) &&
+		if (thread__find_map_fallback(thread, sample->cpumode, from, &alf) &&
 		    !alf.map->dso->adjust_symbols)
 			from = map__map_ip(alf.map, from);
 
-		if (thread__find_map(thread, sample->cpumode, to, &alt) &&
+		if (thread__find_map_fallback(thread, sample->cpumode, to, &alt) &&
 		    !alt.map->dso->adjust_symbols)
 			to = map__map_ip(alt.map, to);
 
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index 69aa93d4ee99..244aeeee9c7a 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -494,14 +494,14 @@ static PyObject *python_process_brstack(struct perf_sample *sample,
 		pydict_set_item_string_decref(pyelem, "cycles",
 		    PyLong_FromUnsignedLongLong(br->entries[i].flags.cycles));
 
-		thread__find_map(thread, sample->cpumode,
-				 br->entries[i].from, &al);
+		thread__find_map_fallback(thread, sample->cpumode,
+					  br->entries[i].from, &al);
 		dsoname = get_dsoname(al.map);
 		pydict_set_item_string_decref(pyelem, "from_dsoname",
 					      _PyUnicode_FromString(dsoname));
 
-		thread__find_map(thread, sample->cpumode,
-				 br->entries[i].to, &al);
+		thread__find_map_fallback(thread, sample->cpumode,
+					  br->entries[i].to, &al);
 		dsoname = get_dsoname(al.map);
 		pydict_set_item_string_decref(pyelem, "to_dsoname",
 					      _PyUnicode_FromString(dsoname));
@@ -576,14 +576,14 @@ static PyObject *python_process_brstacksym(struct perf_sample *sample,
 		if (!pyelem)
 			Py_FatalError("couldn't create Python dictionary");
 
-		thread__find_symbol(thread, sample->cpumode,
-				    br->entries[i].from, &al);
+		thread__find_symbol_fb(thread, sample->cpumode,
+				       br->entries[i].from, &al);
 		get_symoff(al.sym, &al, true, bf, sizeof(bf));
 		pydict_set_item_string_decref(pyelem, "from",
 					      _PyUnicode_FromString(bf));
 
-		thread__find_symbol(thread, sample->cpumode,
-				    br->entries[i].to, &al);
+		thread__find_symbol_fb(thread, sample->cpumode,
+				       br->entries[i].to, &al);
 		get_symoff(al.sym, &al, true, bf, sizeof(bf));
 		pydict_set_item_string_decref(pyelem, "to",
 					      _PyUnicode_FromString(bf));
-- 
2.17.1

[PATCH 4/5] perf intel-pt: Insert callchain context into synthesized callchains

[Adrian Hunter]

In the absence of a fallback, callchains must encode also the callchain
context. Do that now there is no fallback.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: stable@vger.kernel.org # 4.19
---
 tools/perf/util/intel-pt.c     |  6 +++--
 tools/perf/util/thread-stack.c | 44 +++++++++++++++++++++++++++-------
 tools/perf/util/thread-stack.h |  2 +-
 3 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index ffa385a029b3..60732213d16a 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -759,7 +759,8 @@ static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
 	if (pt->synth_opts.callchain) {
 		size_t sz = sizeof(struct ip_callchain);
 
-		sz += pt->synth_opts.callchain_sz * sizeof(u64);
+		/* Add 1 to callchain_sz for callchain context */
+		sz += (pt->synth_opts.callchain_sz + 1) * sizeof(u64);
 		ptq->chain = zalloc(sz);
 		if (!ptq->chain)
 			goto out_free;
@@ -1160,7 +1161,8 @@ static void intel_pt_prep_sample(struct intel_pt *pt,
 
 	if (pt->synth_opts.callchain) {
 		thread_stack__sample(ptq->thread, ptq->chain,
-				     pt->synth_opts.callchain_sz, sample->ip);
+				     pt->synth_opts.callchain_sz + 1,
+				     sample->ip, pt->kernel_start);
 		sample->callchain = ptq->chain;
 	}
 
diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
index c091635bf7dc..afdf36852ac8 100644
--- a/tools/perf/util/thread-stack.c
+++ b/tools/perf/util/thread-stack.c
@@ -310,20 +310,46 @@ void thread_stack__free(struct thread *thread)
 	}
 }
 
+static inline u64 callchain_context(u64 ip, u64 kernel_start)
+{
+	return ip < kernel_start ? PERF_CONTEXT_USER : PERF_CONTEXT_KERNEL;
+}
+
 void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
-			  size_t sz, u64 ip)
+			  size_t sz, u64 ip, u64 kernel_start)
 {
-	size_t i;
+	u64 context = callchain_context(ip, kernel_start);
+	u64 last_context;
+	size_t i, j;
 
-	if (!thread || !thread->ts)
-		chain->nr = 1;
-	else
-		chain->nr = min(sz, thread->ts->cnt + 1);
+	if (sz < 2) {
+		chain->nr = 0;
+		return;
+	}
 
-	chain->ips[0] = ip;
+	chain->ips[0] = context;
+	chain->ips[1] = ip;
+
+	if (!thread || !thread->ts) {
+		chain->nr = 2;
+		return;
+	}
+
+	last_context = context;
+
+	for (i = 2, j = 0; i < sz && j < thread->ts->cnt; i++, j++) {
+		ip = thread->ts->stack[thread->ts->cnt - j].ret_addr;
+		context = callchain_context(ip, kernel_start);
+		if (context != last_context) {
+			if (i >= sz - 1)
+				break;
+			chain->ips[i++] = context;
+			last_context = context;
+		}
+		chain->ips[i] = ip;
+	}
 
-	for (i = 1; i < chain->nr; i++)
-		chain->ips[i] = thread->ts->stack[thread->ts->cnt - i].ret_addr;
+	chain->nr = i;
 }
 
 struct call_return_processor *
diff --git a/tools/perf/util/thread-stack.h b/tools/perf/util/thread-stack.h
index b7e41c4ebfdd..f97c00a8c251 100644
--- a/tools/perf/util/thread-stack.h
+++ b/tools/perf/util/thread-stack.h
@@ -84,7 +84,7 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
 			u64 to_ip, u16 insn_len, u64 trace_nr);
 void thread_stack__set_trace_nr(struct thread *thread, u64 trace_nr);
 void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
-			  size_t sz, u64 ip);
+			  size_t sz, u64 ip, u64 kernel_start);
 int thread_stack__flush(struct thread *thread);
 void thread_stack__free(struct thread *thread);
 size_t thread_stack__depth(struct thread *thread);
-- 
2.17.1

[PATCH 5/5] perf intel-pt/bts: Calculate cpumode for synthesized samples

[Adrian Hunter]

In the absence of a fallback, samples must provide a correct cpumode for
the 'ip'. Do that now there is no fallback.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: stable@vger.kernel.org # 4.19
---
 tools/perf/util/intel-bts.c | 17 ++++++++++++-----
 tools/perf/util/intel-pt.c  | 22 +++++++++++++---------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
index 3b3a3d55dca1..7b27d77306c2 100644
--- a/tools/perf/util/intel-bts.c
+++ b/tools/perf/util/intel-bts.c
@@ -269,6 +269,13 @@ static int intel_bts_do_fix_overlap(struct auxtrace_queue *queue,
 	return 0;
 }
 
+static inline u8 intel_bts_cpumode(struct intel_bts *bts, uint64_t ip)
+{
+	return machine__kernel_ip(bts->machine, ip) ?
+	       PERF_RECORD_MISC_KERNEL :
+	       PERF_RECORD_MISC_USER;
+}
+
 static int intel_bts_synth_branch_sample(struct intel_bts_queue *btsq,
 					 struct branch *branch)
 {
@@ -281,12 +288,8 @@ static int intel_bts_synth_branch_sample(struct intel_bts_queue *btsq,
 	    bts->num_events++ <= bts->synth_opts.initial_skip)
 		return 0;
 
-	event.sample.header.type = PERF_RECORD_SAMPLE;
-	event.sample.header.misc = PERF_RECORD_MISC_USER;
-	event.sample.header.size = sizeof(struct perf_event_header);
-
-	sample.cpumode = PERF_RECORD_MISC_USER;
 	sample.ip = le64_to_cpu(branch->from);
+	sample.cpumode = intel_bts_cpumode(bts, sample.ip);
 	sample.pid = btsq->pid;
 	sample.tid = btsq->tid;
 	sample.addr = le64_to_cpu(branch->to);
@@ -298,6 +301,10 @@ static int intel_bts_synth_branch_sample(struct intel_bts_queue *btsq,
 	sample.insn_len = btsq->intel_pt_insn.length;
 	memcpy(sample.insn, btsq->intel_pt_insn.buf, INTEL_PT_INSN_BUF_SZ);
 
+	event.sample.header.type = PERF_RECORD_SAMPLE;
+	event.sample.header.misc = sample.cpumode;
+	event.sample.header.size = sizeof(struct perf_event_header);
+
 	if (bts->synth_opts.inject) {
 		event.sample.header.size = bts->branches_event_size;
 		ret = perf_event__synthesize_sample(&event,
diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index 60732213d16a..86cc9a64e982 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -407,6 +407,13 @@ intel_pt_cache_lookup(struct dso *dso, struct machine *machine, u64 offset)
 	return auxtrace_cache__lookup(dso->auxtrace_cache, offset);
 }
 
+static inline u8 intel_pt_cpumode(struct intel_pt *pt, uint64_t ip)
+{
+	return ip >= pt->kernel_start ?
+	       PERF_RECORD_MISC_KERNEL :
+	       PERF_RECORD_MISC_USER;
+}
+
 static int intel_pt_walk_next_insn(struct intel_pt_insn *intel_pt_insn,
 				   uint64_t *insn_cnt_ptr, uint64_t *ip,
 				   uint64_t to_ip, uint64_t max_insn_cnt,
@@ -429,10 +436,7 @@ static int intel_pt_walk_next_insn(struct intel_pt_insn *intel_pt_insn,
 	if (to_ip && *ip == to_ip)
 		goto out_no_cache;
 
-	if (*ip >= ptq->pt->kernel_start)
-		cpumode = PERF_RECORD_MISC_KERNEL;
-	else
-		cpumode = PERF_RECORD_MISC_USER;
+	cpumode = intel_pt_cpumode(ptq->pt, *ip);
 
 	thread = ptq->thread;
 	if (!thread) {
@@ -1059,15 +1063,11 @@ static void intel_pt_prep_b_sample(struct intel_pt *pt,
 				   union perf_event *event,
 				   struct perf_sample *sample)
 {
-	event->sample.header.type = PERF_RECORD_SAMPLE;
-	event->sample.header.misc = PERF_RECORD_MISC_USER;
-	event->sample.header.size = sizeof(struct perf_event_header);
-
 	if (!pt->timeless_decoding)
 		sample->time = tsc_to_perf_time(ptq->timestamp, &pt->tc);
 
-	sample->cpumode = PERF_RECORD_MISC_USER;
 	sample->ip = ptq->state->from_ip;
+	sample->cpumode = intel_pt_cpumode(pt, sample->ip);
 	sample->pid = ptq->pid;
 	sample->tid = ptq->tid;
 	sample->addr = ptq->state->to_ip;
@@ -1076,6 +1076,10 @@ static void intel_pt_prep_b_sample(struct intel_pt *pt,
 	sample->flags = ptq->flags;
 	sample->insn_len = ptq->insn_len;
 	memcpy(sample->insn, ptq->insn, INTEL_PT_INSN_BUF_SZ);
+
+	event->sample.header.type = PERF_RECORD_SAMPLE;
+	event->sample.header.misc = sample->cpumode;
+	event->sample.header.size = sizeof(struct perf_event_header);
 }
 
 static int intel_pt_inject_event(union perf_event *event,
-- 
2.17.1

Re: [PATCH 4/5] perf intel-pt: Insert callchain context into synthesized callchains

[Arnaldo Carvalho de Melo]

Em Wed, Oct 31, 2018 at 11:10:42AM +0200, Adrian Hunter escreveu:
> In the absence of a fallback, callchains must encode also the callchain
> context. Do that now there is no fallback.

So, this one is independent of the first 3 patches, right? Ok, applying
it first, I'll relook the first ones next.

- Arnaldo
 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Cc: stable@vger.kernel.org # 4.19
> ---
>  tools/perf/util/intel-pt.c     |  6 +++--
>  tools/perf/util/thread-stack.c | 44 +++++++++++++++++++++++++++-------
>  tools/perf/util/thread-stack.h |  2 +-
>  3 files changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index ffa385a029b3..60732213d16a 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -759,7 +759,8 @@ static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
>  	if (pt->synth_opts.callchain) {
>  		size_t sz = sizeof(struct ip_callchain);
>  
> -		sz += pt->synth_opts.callchain_sz * sizeof(u64);
> +		/* Add 1 to callchain_sz for callchain context */
> +		sz += (pt->synth_opts.callchain_sz + 1) * sizeof(u64);
>  		ptq->chain = zalloc(sz);
>  		if (!ptq->chain)
>  			goto out_free;
> @@ -1160,7 +1161,8 @@ static void intel_pt_prep_sample(struct intel_pt *pt,
>  
>  	if (pt->synth_opts.callchain) {
>  		thread_stack__sample(ptq->thread, ptq->chain,
> -				     pt->synth_opts.callchain_sz, sample->ip);
> +				     pt->synth_opts.callchain_sz + 1,
> +				     sample->ip, pt->kernel_start);
>  		sample->callchain = ptq->chain;
>  	}
>  
> diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
> index c091635bf7dc..afdf36852ac8 100644
> --- a/tools/perf/util/thread-stack.c
> +++ b/tools/perf/util/thread-stack.c
> @@ -310,20 +310,46 @@ void thread_stack__free(struct thread *thread)
>  	}
>  }
>  
> +static inline u64 callchain_context(u64 ip, u64 kernel_start)
> +{
> +	return ip < kernel_start ? PERF_CONTEXT_USER : PERF_CONTEXT_KERNEL;
> +}
> +
>  void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
> -			  size_t sz, u64 ip)
> +			  size_t sz, u64 ip, u64 kernel_start)
>  {
> -	size_t i;
> +	u64 context = callchain_context(ip, kernel_start);
> +	u64 last_context;
> +	size_t i, j;
>  
> -	if (!thread || !thread->ts)
> -		chain->nr = 1;
> -	else
> -		chain->nr = min(sz, thread->ts->cnt + 1);
> +	if (sz < 2) {
> +		chain->nr = 0;
> +		return;
> +	}
>  
> -	chain->ips[0] = ip;
> +	chain->ips[0] = context;
> +	chain->ips[1] = ip;
> +
> +	if (!thread || !thread->ts) {
> +		chain->nr = 2;
> +		return;
> +	}
> +
> +	last_context = context;
> +
> +	for (i = 2, j = 0; i < sz && j < thread->ts->cnt; i++, j++) {
> +		ip = thread->ts->stack[thread->ts->cnt - j].ret_addr;
> +		context = callchain_context(ip, kernel_start);
> +		if (context != last_context) {
> +			if (i >= sz - 1)
> +				break;
> +			chain->ips[i++] = context;
> +			last_context = context;
> +		}
> +		chain->ips[i] = ip;
> +	}
>  
> -	for (i = 1; i < chain->nr; i++)
> -		chain->ips[i] = thread->ts->stack[thread->ts->cnt - i].ret_addr;
> +	chain->nr = i;
>  }
>  
>  struct call_return_processor *
> diff --git a/tools/perf/util/thread-stack.h b/tools/perf/util/thread-stack.h
> index b7e41c4ebfdd..f97c00a8c251 100644
> --- a/tools/perf/util/thread-stack.h
> +++ b/tools/perf/util/thread-stack.h
> @@ -84,7 +84,7 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
>  			u64 to_ip, u16 insn_len, u64 trace_nr);
>  void thread_stack__set_trace_nr(struct thread *thread, u64 trace_nr);
>  void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
> -			  size_t sz, u64 ip);
> +			  size_t sz, u64 ip, u64 kernel_start);
>  int thread_stack__flush(struct thread *thread);
>  void thread_stack__free(struct thread *thread);
>  size_t thread_stack__depth(struct thread *thread);
> -- 
> 2.17.1

Re: [PATCH 0/5] perf tools: Fix for cases where cpumode is incorrect or insufficient

[Jiri Olsa]

On Wed, Oct 31, 2018 at 11:10:38AM +0200, Adrian Hunter wrote:
> Hi
> 
> These patches probably deal with most cases except the one fixed by David
> Miller's "perf callchain: Honour the ordering of
> PERF_CONTEXT_{USER,KERNEL,etc}" patch, and also cat_backtrace() which looks
> like it has the same problem, and the cs-etm fix.
> 
> 
> Adrian Hunter (5):
>       perf tools: Add fallback functions for cases where cpumode is insufficient
>       perf tools: Use fallback for sample_addr_correlates_sym() cases
>       perf tools: Use fallbacks for branch stacks
>       perf intel-pt: Insert callchain context into synthesized callchains
>       perf intel-pt/bts: Calculate cpumode for synthesized samples

Reviewed-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> 
>  tools/perf/builtin-script.c                        | 12 +++---
>  tools/perf/util/event.c                            | 29 +++++++++++++-
>  tools/perf/util/intel-bts.c                        | 17 ++++++---
>  tools/perf/util/intel-pt.c                         | 28 ++++++++------
>  tools/perf/util/machine.c                          | 40 ++++++++++++++++++++
>  tools/perf/util/machine.h                          |  2 +
>  .../util/scripting-engines/trace-event-python.c    | 16 ++++----
>  tools/perf/util/thread-stack.c                     | 44 +++++++++++++++++-----
>  tools/perf/util/thread-stack.h                     |  2 +-
>  tools/perf/util/thread.h                           |  4 ++
>  10 files changed, 153 insertions(+), 41 deletions(-)
> 
> 
> Regards
> Adrian

Re: [PATCH 4/5] perf intel-pt: Insert callchain context into synthesized callchains

[Adrian Hunter]

On 31/10/18 3:28 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Oct 31, 2018 at 11:10:42AM +0200, Adrian Hunter escreveu:
>> In the absence of a fallback, callchains must encode also the callchain
>> context. Do that now there is no fallback.
> 
> So, this one is independent of the first 3 patches, right?

Yes.  I was just going to test it separately when I noticed I had
screwed up my earlier testing.  When I re-tested I discovered this patch
has an off-by-one error:

diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
index afdf36852ac8..61a4286a74dc 100644
--- a/tools/perf/util/thread-stack.c
+++ b/tools/perf/util/thread-stack.c
@@ -337,7 +337,7 @@ void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
 
 	last_context = context;
 
-	for (i = 2, j = 0; i < sz && j < thread->ts->cnt; i++, j++) {
+	for (i = 2, j = 1; i < sz && j <= thread->ts->cnt; i++, j++) {
 		ip = thread->ts->stack[thread->ts->cnt - j].ret_addr;
 		context = callchain_context(ip, kernel_start);
 		if (context != last_context) {

Shall I send V2?

> So, this one is independent of the first 3 patches, right? Ok, applying
> it first, I'll relook the first ones next.
> 
> - Arnaldo
>  
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: stable@vger.kernel.org # 4.19
>> ---
>>  tools/perf/util/intel-pt.c     |  6 +++--
>>  tools/perf/util/thread-stack.c | 44 +++++++++++++++++++++++++++-------
>>  tools/perf/util/thread-stack.h |  2 +-
>>  3 files changed, 40 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
>> index ffa385a029b3..60732213d16a 100644
>> --- a/tools/perf/util/intel-pt.c
>> +++ b/tools/perf/util/intel-pt.c
>> @@ -759,7 +759,8 @@ static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
>>  	if (pt->synth_opts.callchain) {
>>  		size_t sz = sizeof(struct ip_callchain);
>>  
>> -		sz += pt->synth_opts.callchain_sz * sizeof(u64);
>> +		/* Add 1 to callchain_sz for callchain context */
>> +		sz += (pt->synth_opts.callchain_sz + 1) * sizeof(u64);
>>  		ptq->chain = zalloc(sz);
>>  		if (!ptq->chain)
>>  			goto out_free;
>> @@ -1160,7 +1161,8 @@ static void intel_pt_prep_sample(struct intel_pt *pt,
>>  
>>  	if (pt->synth_opts.callchain) {
>>  		thread_stack__sample(ptq->thread, ptq->chain,
>> -				     pt->synth_opts.callchain_sz, sample->ip);
>> +				     pt->synth_opts.callchain_sz + 1,
>> +				     sample->ip, pt->kernel_start);
>>  		sample->callchain = ptq->chain;
>>  	}
>>  
>> diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
>> index c091635bf7dc..afdf36852ac8 100644
>> --- a/tools/perf/util/thread-stack.c
>> +++ b/tools/perf/util/thread-stack.c
>> @@ -310,20 +310,46 @@ void thread_stack__free(struct thread *thread)
>>  	}
>>  }
>>  
>> +static inline u64 callchain_context(u64 ip, u64 kernel_start)
>> +{
>> +	return ip < kernel_start ? PERF_CONTEXT_USER : PERF_CONTEXT_KERNEL;
>> +}
>> +
>>  void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
>> -			  size_t sz, u64 ip)
>> +			  size_t sz, u64 ip, u64 kernel_start)
>>  {
>> -	size_t i;
>> +	u64 context = callchain_context(ip, kernel_start);
>> +	u64 last_context;
>> +	size_t i, j;
>>  
>> -	if (!thread || !thread->ts)
>> -		chain->nr = 1;
>> -	else
>> -		chain->nr = min(sz, thread->ts->cnt + 1);
>> +	if (sz < 2) {
>> +		chain->nr = 0;
>> +		return;
>> +	}
>>  
>> -	chain->ips[0] = ip;
>> +	chain->ips[0] = context;
>> +	chain->ips[1] = ip;
>> +
>> +	if (!thread || !thread->ts) {
>> +		chain->nr = 2;
>> +		return;
>> +	}
>> +
>> +	last_context = context;
>> +
>> +	for (i = 2, j = 0; i < sz && j < thread->ts->cnt; i++, j++) {
>> +		ip = thread->ts->stack[thread->ts->cnt - j].ret_addr;
>> +		context = callchain_context(ip, kernel_start);
>> +		if (context != last_context) {
>> +			if (i >= sz - 1)
>> +				break;
>> +			chain->ips[i++] = context;
>> +			last_context = context;
>> +		}
>> +		chain->ips[i] = ip;
>> +	}
>>  
>> -	for (i = 1; i < chain->nr; i++)
>> -		chain->ips[i] = thread->ts->stack[thread->ts->cnt - i].ret_addr;
>> +	chain->nr = i;
>>  }
>>  
>>  struct call_return_processor *
>> diff --git a/tools/perf/util/thread-stack.h b/tools/perf/util/thread-stack.h
>> index b7e41c4ebfdd..f97c00a8c251 100644
>> --- a/tools/perf/util/thread-stack.h
>> +++ b/tools/perf/util/thread-stack.h
>> @@ -84,7 +84,7 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
>>  			u64 to_ip, u16 insn_len, u64 trace_nr);
>>  void thread_stack__set_trace_nr(struct thread *thread, u64 trace_nr);
>>  void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
>> -			  size_t sz, u64 ip);
>> +			  size_t sz, u64 ip, u64 kernel_start);
>>  int thread_stack__flush(struct thread *thread);
>>  void thread_stack__free(struct thread *thread);
>>  size_t thread_stack__depth(struct thread *thread);
>> -- 
>> 2.17.1
> 

Re: [PATCH 4/5] perf intel-pt: Insert callchain context into synthesized callchains

[Adrian Hunter]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On 31/10/18 4:15 PM, Adrian Hunter wrote:
> On 31/10/18 3:28 PM, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Oct 31, 2018 at 11:10:42AM +0200, Adrian Hunter escreveu:
>>> In the absence of a fallback, callchains must encode also the callchain
>>> context. Do that now there is no fallback.
>>
>> So, this one is independent of the first 3 patches, right?
> 
> Yes.  I was just going to test it separately when I noticed I had
> screwed up my earlier testing.  When I re-tested I discovered this patch
> has an off-by-one error:
> 
> diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
> index afdf36852ac8..61a4286a74dc 100644
> --- a/tools/perf/util/thread-stack.c
> +++ b/tools/perf/util/thread-stack.c
> @@ -337,7 +337,7 @@ void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
>  
>  	last_context = context;
>  
> -	for (i = 2, j = 0; i < sz && j < thread->ts->cnt; i++, j++) {
> +	for (i = 2, j = 1; i < sz && j <= thread->ts->cnt; i++, j++) {
>  		ip = thread->ts->stack[thread->ts->cnt - j].ret_addr;
>  		context = callchain_context(ip, kernel_start);
>  		if (context != last_context) {
> 
> Shall I send V2?

I have attached it

[-- Attachment #2: 0004-perf-intel-pt-Insert-callchain-context-into-synthesi.patch --]
[-- Type: text/x-patch, Size: 3796 bytes --]

From 19a9fbdbd3729c7f14c9c1f0ac79c18cce91ff60 Mon Sep 17 00:00:00 2001
From: Adrian Hunter <adrian.hunter@intel.com>
Date: Wed, 31 Oct 2018 09:31:25 +0200
Subject: [PATCH V2 4/5] perf intel-pt: Insert callchain context into
 synthesized callchains

In the absence of a fallback, callchains must encode also the callchain
context. Do that now there is no fallback.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: stable@vger.kernel.org # 4.19
---


Changes in V2:

	Fix off-by-one error


 tools/perf/util/intel-pt.c     |  6 +++--
 tools/perf/util/thread-stack.c | 44 +++++++++++++++++++++++++++-------
 tools/perf/util/thread-stack.h |  2 +-
 3 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index ffa385a029b3..60732213d16a 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -759,7 +759,8 @@ static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
 	if (pt->synth_opts.callchain) {
 		size_t sz = sizeof(struct ip_callchain);
 
-		sz += pt->synth_opts.callchain_sz * sizeof(u64);
+		/* Add 1 to callchain_sz for callchain context */
+		sz += (pt->synth_opts.callchain_sz + 1) * sizeof(u64);
 		ptq->chain = zalloc(sz);
 		if (!ptq->chain)
 			goto out_free;
@@ -1160,7 +1161,8 @@ static void intel_pt_prep_sample(struct intel_pt *pt,
 
 	if (pt->synth_opts.callchain) {
 		thread_stack__sample(ptq->thread, ptq->chain,
-				     pt->synth_opts.callchain_sz, sample->ip);
+				     pt->synth_opts.callchain_sz + 1,
+				     sample->ip, pt->kernel_start);
 		sample->callchain = ptq->chain;
 	}
 
diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
index c091635bf7dc..61a4286a74dc 100644
--- a/tools/perf/util/thread-stack.c
+++ b/tools/perf/util/thread-stack.c
@@ -310,20 +310,46 @@ void thread_stack__free(struct thread *thread)
 	}
 }
 
+static inline u64 callchain_context(u64 ip, u64 kernel_start)
+{
+	return ip < kernel_start ? PERF_CONTEXT_USER : PERF_CONTEXT_KERNEL;
+}
+
 void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
-			  size_t sz, u64 ip)
+			  size_t sz, u64 ip, u64 kernel_start)
 {
-	size_t i;
+	u64 context = callchain_context(ip, kernel_start);
+	u64 last_context;
+	size_t i, j;
 
-	if (!thread || !thread->ts)
-		chain->nr = 1;
-	else
-		chain->nr = min(sz, thread->ts->cnt + 1);
+	if (sz < 2) {
+		chain->nr = 0;
+		return;
+	}
 
-	chain->ips[0] = ip;
+	chain->ips[0] = context;
+	chain->ips[1] = ip;
+
+	if (!thread || !thread->ts) {
+		chain->nr = 2;
+		return;
+	}
+
+	last_context = context;
+
+	for (i = 2, j = 1; i < sz && j <= thread->ts->cnt; i++, j++) {
+		ip = thread->ts->stack[thread->ts->cnt - j].ret_addr;
+		context = callchain_context(ip, kernel_start);
+		if (context != last_context) {
+			if (i >= sz - 1)
+				break;
+			chain->ips[i++] = context;
+			last_context = context;
+		}
+		chain->ips[i] = ip;
+	}
 
-	for (i = 1; i < chain->nr; i++)
-		chain->ips[i] = thread->ts->stack[thread->ts->cnt - i].ret_addr;
+	chain->nr = i;
 }
 
 struct call_return_processor *
diff --git a/tools/perf/util/thread-stack.h b/tools/perf/util/thread-stack.h
index b7e41c4ebfdd..f97c00a8c251 100644
--- a/tools/perf/util/thread-stack.h
+++ b/tools/perf/util/thread-stack.h
@@ -84,7 +84,7 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
 			u64 to_ip, u16 insn_len, u64 trace_nr);
 void thread_stack__set_trace_nr(struct thread *thread, u64 trace_nr);
 void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
-			  size_t sz, u64 ip);
+			  size_t sz, u64 ip, u64 kernel_start);
 int thread_stack__flush(struct thread *thread);
 void thread_stack__free(struct thread *thread);
 size_t thread_stack__depth(struct thread *thread);
-- 
2.17.1

Re: [PATCH 4/5] perf intel-pt: Insert callchain context into synthesized callchains

[Arnaldo Carvalho de Melo]

Em Wed, Oct 31, 2018 at 04:15:26PM +0200, Adrian Hunter escreveu:
> On 31/10/18 3:28 PM, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Oct 31, 2018 at 11:10:42AM +0200, Adrian Hunter escreveu:
> >> In the absence of a fallback, callchains must encode also the callchain
> >> context. Do that now there is no fallback.
> > 
> > So, this one is independent of the first 3 patches, right?
> 
> Yes.  I was just going to test it separately when I noticed I had
> screwed up my earlier testing.  When I re-tested I discovered this patch
> has an off-by-one error:
> 
> diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
> index afdf36852ac8..61a4286a74dc 100644
> --- a/tools/perf/util/thread-stack.c
> +++ b/tools/perf/util/thread-stack.c
> @@ -337,7 +337,7 @@ void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
>  
>  	last_context = context;
>  
> -	for (i = 2, j = 0; i < sz && j < thread->ts->cnt; i++, j++) {
> +	for (i = 2, j = 1; i < sz && j <= thread->ts->cnt; i++, j++) {
>  		ip = thread->ts->stack[thread->ts->cnt - j].ret_addr;
>  		context = callchain_context(ip, kernel_start);
>  		if (context != last_context) {
> 
> Shall I send V2?

No need, I can fix this here
 
> > So, this one is independent of the first 3 patches, right? Ok, applying
> > it first, I'll relook the first ones next.
> > 
> > - Arnaldo
> >  
> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >> Cc: stable@vger.kernel.org # 4.19
> >> ---
> >>  tools/perf/util/intel-pt.c     |  6 +++--
> >>  tools/perf/util/thread-stack.c | 44 +++++++++++++++++++++++++++-------
> >>  tools/perf/util/thread-stack.h |  2 +-
> >>  3 files changed, 40 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> >> index ffa385a029b3..60732213d16a 100644
> >> --- a/tools/perf/util/intel-pt.c
> >> +++ b/tools/perf/util/intel-pt.c
> >> @@ -759,7 +759,8 @@ static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
> >>  	if (pt->synth_opts.callchain) {
> >>  		size_t sz = sizeof(struct ip_callchain);
> >>  
> >> -		sz += pt->synth_opts.callchain_sz * sizeof(u64);
> >> +		/* Add 1 to callchain_sz for callchain context */
> >> +		sz += (pt->synth_opts.callchain_sz + 1) * sizeof(u64);
> >>  		ptq->chain = zalloc(sz);
> >>  		if (!ptq->chain)
> >>  			goto out_free;
> >> @@ -1160,7 +1161,8 @@ static void intel_pt_prep_sample(struct intel_pt *pt,
> >>  
> >>  	if (pt->synth_opts.callchain) {
> >>  		thread_stack__sample(ptq->thread, ptq->chain,
> >> -				     pt->synth_opts.callchain_sz, sample->ip);
> >> +				     pt->synth_opts.callchain_sz + 1,
> >> +				     sample->ip, pt->kernel_start);
> >>  		sample->callchain = ptq->chain;
> >>  	}
> >>  
> >> diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
> >> index c091635bf7dc..afdf36852ac8 100644
> >> --- a/tools/perf/util/thread-stack.c
> >> +++ b/tools/perf/util/thread-stack.c
> >> @@ -310,20 +310,46 @@ void thread_stack__free(struct thread *thread)
> >>  	}
> >>  }
> >>  
> >> +static inline u64 callchain_context(u64 ip, u64 kernel_start)
> >> +{
> >> +	return ip < kernel_start ? PERF_CONTEXT_USER : PERF_CONTEXT_KERNEL;
> >> +}
> >> +
> >>  void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
> >> -			  size_t sz, u64 ip)
> >> +			  size_t sz, u64 ip, u64 kernel_start)
> >>  {
> >> -	size_t i;
> >> +	u64 context = callchain_context(ip, kernel_start);
> >> +	u64 last_context;
> >> +	size_t i, j;
> >>  
> >> -	if (!thread || !thread->ts)
> >> -		chain->nr = 1;
> >> -	else
> >> -		chain->nr = min(sz, thread->ts->cnt + 1);
> >> +	if (sz < 2) {
> >> +		chain->nr = 0;
> >> +		return;
> >> +	}
> >>  
> >> -	chain->ips[0] = ip;
> >> +	chain->ips[0] = context;
> >> +	chain->ips[1] = ip;
> >> +
> >> +	if (!thread || !thread->ts) {
> >> +		chain->nr = 2;
> >> +		return;
> >> +	}
> >> +
> >> +	last_context = context;
> >> +
> >> +	for (i = 2, j = 0; i < sz && j < thread->ts->cnt; i++, j++) {
> >> +		ip = thread->ts->stack[thread->ts->cnt - j].ret_addr;
> >> +		context = callchain_context(ip, kernel_start);
> >> +		if (context != last_context) {
> >> +			if (i >= sz - 1)
> >> +				break;
> >> +			chain->ips[i++] = context;
> >> +			last_context = context;
> >> +		}
> >> +		chain->ips[i] = ip;
> >> +	}
> >>  
> >> -	for (i = 1; i < chain->nr; i++)
> >> -		chain->ips[i] = thread->ts->stack[thread->ts->cnt - i].ret_addr;
> >> +	chain->nr = i;
> >>  }
> >>  
> >>  struct call_return_processor *
> >> diff --git a/tools/perf/util/thread-stack.h b/tools/perf/util/thread-stack.h
> >> index b7e41c4ebfdd..f97c00a8c251 100644
> >> --- a/tools/perf/util/thread-stack.h
> >> +++ b/tools/perf/util/thread-stack.h
> >> @@ -84,7 +84,7 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
> >>  			u64 to_ip, u16 insn_len, u64 trace_nr);
> >>  void thread_stack__set_trace_nr(struct thread *thread, u64 trace_nr);
> >>  void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
> >> -			  size_t sz, u64 ip);
> >> +			  size_t sz, u64 ip, u64 kernel_start);
> >>  int thread_stack__flush(struct thread *thread);
> >>  void thread_stack__free(struct thread *thread);
> >>  size_t thread_stack__depth(struct thread *thread);
> >> -- 
> >> 2.17.1
> > 

Re: [PATCH 4/5] perf intel-pt: Insert callchain context into synthesized callchains

[Arnaldo Carvalho de Melo]

Em Wed, Oct 31, 2018 at 04:20:45PM +0200, Adrian Hunter escreveu:
> On 31/10/18 4:15 PM, Adrian Hunter wrote:
> > On 31/10/18 3:28 PM, Arnaldo Carvalho de Melo wrote:
> >> Em Wed, Oct 31, 2018 at 11:10:42AM +0200, Adrian Hunter escreveu:
> >>> In the absence of a fallback, callchains must encode also the callchain
> >>> context. Do that now there is no fallback.
> >>
> >> So, this one is independent of the first 3 patches, right?
> > 
> > Yes.  I was just going to test it separately when I noticed I had
> > screwed up my earlier testing.  When I re-tested I discovered this patch
> > has an off-by-one error:
> > 
> > diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
> > index afdf36852ac8..61a4286a74dc 100644
> > --- a/tools/perf/util/thread-stack.c
> > +++ b/tools/perf/util/thread-stack.c
> > @@ -337,7 +337,7 @@ void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
> >  
> >  	last_context = context;
> >  
> > -	for (i = 2, j = 0; i < sz && j < thread->ts->cnt; i++, j++) {
> > +	for (i = 2, j = 1; i < sz && j <= thread->ts->cnt; i++, j++) {
> >  		ip = thread->ts->stack[thread->ts->cnt - j].ret_addr;
> >  		context = callchain_context(ip, kernel_start);
> >  		if (context != last_context) {
> > 
> > Shall I send V2?
> 
> I have attached it

Thanks!

> >From 19a9fbdbd3729c7f14c9c1f0ac79c18cce91ff60 Mon Sep 17 00:00:00 2001
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Wed, 31 Oct 2018 09:31:25 +0200
> Subject: [PATCH V2 4/5] perf intel-pt: Insert callchain context into
>  synthesized callchains
> 
> In the absence of a fallback, callchains must encode also the callchain
> context. Do that now there is no fallback.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Cc: stable@vger.kernel.org # 4.19
> ---
> 
> 
> Changes in V2:
> 
> 	Fix off-by-one error
> 
> 
>  tools/perf/util/intel-pt.c     |  6 +++--
>  tools/perf/util/thread-stack.c | 44 +++++++++++++++++++++++++++-------
>  tools/perf/util/thread-stack.h |  2 +-
>  3 files changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index ffa385a029b3..60732213d16a 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -759,7 +759,8 @@ static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
>  	if (pt->synth_opts.callchain) {
>  		size_t sz = sizeof(struct ip_callchain);
>  
> -		sz += pt->synth_opts.callchain_sz * sizeof(u64);
> +		/* Add 1 to callchain_sz for callchain context */
> +		sz += (pt->synth_opts.callchain_sz + 1) * sizeof(u64);
>  		ptq->chain = zalloc(sz);
>  		if (!ptq->chain)
>  			goto out_free;
> @@ -1160,7 +1161,8 @@ static void intel_pt_prep_sample(struct intel_pt *pt,
>  
>  	if (pt->synth_opts.callchain) {
>  		thread_stack__sample(ptq->thread, ptq->chain,
> -				     pt->synth_opts.callchain_sz, sample->ip);
> +				     pt->synth_opts.callchain_sz + 1,
> +				     sample->ip, pt->kernel_start);
>  		sample->callchain = ptq->chain;
>  	}
>  
> diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
> index c091635bf7dc..61a4286a74dc 100644
> --- a/tools/perf/util/thread-stack.c
> +++ b/tools/perf/util/thread-stack.c
> @@ -310,20 +310,46 @@ void thread_stack__free(struct thread *thread)
>  	}
>  }
>  
> +static inline u64 callchain_context(u64 ip, u64 kernel_start)
> +{
> +	return ip < kernel_start ? PERF_CONTEXT_USER : PERF_CONTEXT_KERNEL;
> +}
> +
>  void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
> -			  size_t sz, u64 ip)
> +			  size_t sz, u64 ip, u64 kernel_start)
>  {
> -	size_t i;
> +	u64 context = callchain_context(ip, kernel_start);
> +	u64 last_context;
> +	size_t i, j;
>  
> -	if (!thread || !thread->ts)
> -		chain->nr = 1;
> -	else
> -		chain->nr = min(sz, thread->ts->cnt + 1);
> +	if (sz < 2) {
> +		chain->nr = 0;
> +		return;
> +	}
>  
> -	chain->ips[0] = ip;
> +	chain->ips[0] = context;
> +	chain->ips[1] = ip;
> +
> +	if (!thread || !thread->ts) {
> +		chain->nr = 2;
> +		return;
> +	}
> +
> +	last_context = context;
> +
> +	for (i = 2, j = 1; i < sz && j <= thread->ts->cnt; i++, j++) {
> +		ip = thread->ts->stack[thread->ts->cnt - j].ret_addr;
> +		context = callchain_context(ip, kernel_start);
> +		if (context != last_context) {
> +			if (i >= sz - 1)
> +				break;
> +			chain->ips[i++] = context;
> +			last_context = context;
> +		}
> +		chain->ips[i] = ip;
> +	}
>  
> -	for (i = 1; i < chain->nr; i++)
> -		chain->ips[i] = thread->ts->stack[thread->ts->cnt - i].ret_addr;
> +	chain->nr = i;
>  }
>  
>  struct call_return_processor *
> diff --git a/tools/perf/util/thread-stack.h b/tools/perf/util/thread-stack.h
> index b7e41c4ebfdd..f97c00a8c251 100644
> --- a/tools/perf/util/thread-stack.h
> +++ b/tools/perf/util/thread-stack.h
> @@ -84,7 +84,7 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
>  			u64 to_ip, u16 insn_len, u64 trace_nr);
>  void thread_stack__set_trace_nr(struct thread *thread, u64 trace_nr);
>  void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
> -			  size_t sz, u64 ip);
> +			  size_t sz, u64 ip, u64 kernel_start);
>  int thread_stack__flush(struct thread *thread);
>  void thread_stack__free(struct thread *thread);
>  size_t thread_stack__depth(struct thread *thread);
> -- 
> 2.17.1
> 

[tip:perf/urgent] perf intel-pt: Insert callchain context into synthesized callchains

[tip-bot for Adrian Hunter]

Commit-ID:  242483068b4b9ad02f1653819b6e683577681e0e
Gitweb:     https://git.kernel.org/tip/242483068b4b9ad02f1653819b6e683577681e0e
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Wed, 31 Oct 2018 11:10:42 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 31 Oct 2018 12:54:27 -0300

perf intel-pt: Insert callchain context into synthesized callchains

In the absence of a fallback, callchains must encode also the callchain
context. Do that now there is no fallback.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: stable@vger.kernel.org # 4.19
Link: http://lkml.kernel.org/r/100ea2ec-ed14-b56d-d810-e0a6d2f4b069@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/intel-pt.c     |  6 ++++--
 tools/perf/util/thread-stack.c | 44 +++++++++++++++++++++++++++++++++---------
 tools/perf/util/thread-stack.h |  2 +-
 3 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index ffa385a029b3..60732213d16a 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -759,7 +759,8 @@ static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
 	if (pt->synth_opts.callchain) {
 		size_t sz = sizeof(struct ip_callchain);
 
-		sz += pt->synth_opts.callchain_sz * sizeof(u64);
+		/* Add 1 to callchain_sz for callchain context */
+		sz += (pt->synth_opts.callchain_sz + 1) * sizeof(u64);
 		ptq->chain = zalloc(sz);
 		if (!ptq->chain)
 			goto out_free;
@@ -1160,7 +1161,8 @@ static void intel_pt_prep_sample(struct intel_pt *pt,
 
 	if (pt->synth_opts.callchain) {
 		thread_stack__sample(ptq->thread, ptq->chain,
-				     pt->synth_opts.callchain_sz, sample->ip);
+				     pt->synth_opts.callchain_sz + 1,
+				     sample->ip, pt->kernel_start);
 		sample->callchain = ptq->chain;
 	}
 
diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
index c091635bf7dc..61a4286a74dc 100644
--- a/tools/perf/util/thread-stack.c
+++ b/tools/perf/util/thread-stack.c
@@ -310,20 +310,46 @@ void thread_stack__free(struct thread *thread)
 	}
 }
 
+static inline u64 callchain_context(u64 ip, u64 kernel_start)
+{
+	return ip < kernel_start ? PERF_CONTEXT_USER : PERF_CONTEXT_KERNEL;
+}
+
 void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
-			  size_t sz, u64 ip)
+			  size_t sz, u64 ip, u64 kernel_start)
 {
-	size_t i;
+	u64 context = callchain_context(ip, kernel_start);
+	u64 last_context;
+	size_t i, j;
 
-	if (!thread || !thread->ts)
-		chain->nr = 1;
-	else
-		chain->nr = min(sz, thread->ts->cnt + 1);
+	if (sz < 2) {
+		chain->nr = 0;
+		return;
+	}
 
-	chain->ips[0] = ip;
+	chain->ips[0] = context;
+	chain->ips[1] = ip;
+
+	if (!thread || !thread->ts) {
+		chain->nr = 2;
+		return;
+	}
+
+	last_context = context;
+
+	for (i = 2, j = 1; i < sz && j <= thread->ts->cnt; i++, j++) {
+		ip = thread->ts->stack[thread->ts->cnt - j].ret_addr;
+		context = callchain_context(ip, kernel_start);
+		if (context != last_context) {
+			if (i >= sz - 1)
+				break;
+			chain->ips[i++] = context;
+			last_context = context;
+		}
+		chain->ips[i] = ip;
+	}
 
-	for (i = 1; i < chain->nr; i++)
-		chain->ips[i] = thread->ts->stack[thread->ts->cnt - i].ret_addr;
+	chain->nr = i;
 }
 
 struct call_return_processor *
diff --git a/tools/perf/util/thread-stack.h b/tools/perf/util/thread-stack.h
index b7e41c4ebfdd..f97c00a8c251 100644
--- a/tools/perf/util/thread-stack.h
+++ b/tools/perf/util/thread-stack.h
@@ -84,7 +84,7 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
 			u64 to_ip, u16 insn_len, u64 trace_nr);
 void thread_stack__set_trace_nr(struct thread *thread, u64 trace_nr);
 void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
-			  size_t sz, u64 ip);
+			  size_t sz, u64 ip, u64 kernel_start);
 int thread_stack__flush(struct thread *thread);
 void thread_stack__free(struct thread *thread);
 size_t thread_stack__depth(struct thread *thread);

[tip:perf/urgent] perf intel-pt/bts: Calculate cpumode for synthesized samples

[tip-bot for Adrian Hunter]

Commit-ID:  5d4f0edaa3ac4f1844ed7c64cd2bae6f1912bac5
Gitweb:     https://git.kernel.org/tip/5d4f0edaa3ac4f1844ed7c64cd2bae6f1912bac5
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Wed, 31 Oct 2018 11:10:43 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 31 Oct 2018 12:56:26 -0300

perf intel-pt/bts: Calculate cpumode for synthesized samples

In the absence of a fallback, samples must provide a correct cpumode for
the 'ip'. Do that now there is no fallback.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: stable@vger.kernel.org # 4.19
Link: http://lkml.kernel.org/r/20181031091043.23465-6-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/intel-bts.c | 17 ++++++++++++-----
 tools/perf/util/intel-pt.c  | 22 +++++++++++++---------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/intel-bts.c b/tools/perf/util/intel-bts.c
index 3b3a3d55dca1..7b27d77306c2 100644
--- a/tools/perf/util/intel-bts.c
+++ b/tools/perf/util/intel-bts.c
@@ -269,6 +269,13 @@ static int intel_bts_do_fix_overlap(struct auxtrace_queue *queue,
 	return 0;
 }
 
+static inline u8 intel_bts_cpumode(struct intel_bts *bts, uint64_t ip)
+{
+	return machine__kernel_ip(bts->machine, ip) ?
+	       PERF_RECORD_MISC_KERNEL :
+	       PERF_RECORD_MISC_USER;
+}
+
 static int intel_bts_synth_branch_sample(struct intel_bts_queue *btsq,
 					 struct branch *branch)
 {
@@ -281,12 +288,8 @@ static int intel_bts_synth_branch_sample(struct intel_bts_queue *btsq,
 	    bts->num_events++ <= bts->synth_opts.initial_skip)
 		return 0;
 
-	event.sample.header.type = PERF_RECORD_SAMPLE;
-	event.sample.header.misc = PERF_RECORD_MISC_USER;
-	event.sample.header.size = sizeof(struct perf_event_header);
-
-	sample.cpumode = PERF_RECORD_MISC_USER;
 	sample.ip = le64_to_cpu(branch->from);
+	sample.cpumode = intel_bts_cpumode(bts, sample.ip);
 	sample.pid = btsq->pid;
 	sample.tid = btsq->tid;
 	sample.addr = le64_to_cpu(branch->to);
@@ -298,6 +301,10 @@ static int intel_bts_synth_branch_sample(struct intel_bts_queue *btsq,
 	sample.insn_len = btsq->intel_pt_insn.length;
 	memcpy(sample.insn, btsq->intel_pt_insn.buf, INTEL_PT_INSN_BUF_SZ);
 
+	event.sample.header.type = PERF_RECORD_SAMPLE;
+	event.sample.header.misc = sample.cpumode;
+	event.sample.header.size = sizeof(struct perf_event_header);
+
 	if (bts->synth_opts.inject) {
 		event.sample.header.size = bts->branches_event_size;
 		ret = perf_event__synthesize_sample(&event,
diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index 60732213d16a..86cc9a64e982 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -407,6 +407,13 @@ intel_pt_cache_lookup(struct dso *dso, struct machine *machine, u64 offset)
 	return auxtrace_cache__lookup(dso->auxtrace_cache, offset);
 }
 
+static inline u8 intel_pt_cpumode(struct intel_pt *pt, uint64_t ip)
+{
+	return ip >= pt->kernel_start ?
+	       PERF_RECORD_MISC_KERNEL :
+	       PERF_RECORD_MISC_USER;
+}
+
 static int intel_pt_walk_next_insn(struct intel_pt_insn *intel_pt_insn,
 				   uint64_t *insn_cnt_ptr, uint64_t *ip,
 				   uint64_t to_ip, uint64_t max_insn_cnt,
@@ -429,10 +436,7 @@ static int intel_pt_walk_next_insn(struct intel_pt_insn *intel_pt_insn,
 	if (to_ip && *ip == to_ip)
 		goto out_no_cache;
 
-	if (*ip >= ptq->pt->kernel_start)
-		cpumode = PERF_RECORD_MISC_KERNEL;
-	else
-		cpumode = PERF_RECORD_MISC_USER;
+	cpumode = intel_pt_cpumode(ptq->pt, *ip);
 
 	thread = ptq->thread;
 	if (!thread) {
@@ -1059,15 +1063,11 @@ static void intel_pt_prep_b_sample(struct intel_pt *pt,
 				   union perf_event *event,
 				   struct perf_sample *sample)
 {
-	event->sample.header.type = PERF_RECORD_SAMPLE;
-	event->sample.header.misc = PERF_RECORD_MISC_USER;
-	event->sample.header.size = sizeof(struct perf_event_header);
-
 	if (!pt->timeless_decoding)
 		sample->time = tsc_to_perf_time(ptq->timestamp, &pt->tc);
 
-	sample->cpumode = PERF_RECORD_MISC_USER;
 	sample->ip = ptq->state->from_ip;
+	sample->cpumode = intel_pt_cpumode(pt, sample->ip);
 	sample->pid = ptq->pid;
 	sample->tid = ptq->tid;
 	sample->addr = ptq->state->to_ip;
@@ -1076,6 +1076,10 @@ static void intel_pt_prep_b_sample(struct intel_pt *pt,
 	sample->flags = ptq->flags;
 	sample->insn_len = ptq->insn_len;
 	memcpy(sample->insn, ptq->insn, INTEL_PT_INSN_BUF_SZ);
+
+	event->sample.header.type = PERF_RECORD_SAMPLE;
+	event->sample.header.misc = sample->cpumode;
+	event->sample.header.size = sizeof(struct perf_event_header);
 }
 
 static int intel_pt_inject_event(union perf_event *event,

RE: [PATCH 2/5] perf tools: Use fallback for sample_addr_correlates_sym() cases

[Hunter, Adrian]

> -----Original Message-----
> From: Hunter, Adrian
> Sent: Wednesday, October 31, 2018 11:11 AM
> To: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>; Andi Kleen <ak@linux.intel.com>; linux-
> kernel@vger.kernel.org; leo.yan@linaro.org; David Miller
> <davem@davemloft.net>; Mathieu Poirier <mathieu.poirier@linaro.org>
> Subject: [PATCH 2/5] perf tools: Use fallback for
> sample_addr_correlates_sym() cases
> 
> thread__resolve() is used in the sample_addr_correlates_sym() cases where
> 'addr' is a destination of a branch which does not necessarily have the
> same cpumode as the 'ip'. Use the fallback function in that case.
> 
> This patch depends on patch "perf tools: Add fallback functions for cases
> where cpumode is insufficient".
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Cc: stable@vger.kernel.org # 4.19

Any comments?

> ---
>  tools/perf/util/event.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 52b24b8ce29e..316f62ffa27b 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -1705,7 +1705,7 @@ bool sample_addr_correlates_sym(struct
> perf_event_attr *attr)
>  void thread__resolve(struct thread *thread, struct addr_location *al,
>  		     struct perf_sample *sample)
>  {
> -	thread__find_map(thread, sample->cpumode, sample->addr, al);
> +	thread__find_map_fallback(thread, sample->cpumode, sample-
> >addr, al);
> 
>  	al->cpu = sample->cpu;
>  	al->sym = NULL;
> --
> 2.17.1

Re: [PATCH 1/5] perf tools: Add fallback functions for cases where cpumode is insufficient

[Arnaldo Carvalho de Melo]

Em Wed, Oct 31, 2018 at 11:10:39AM +0200, Adrian Hunter escreveu:
> For branch stacks or branch samples, the sample cpumode might not be
> correct because it applies only to the sample 'ip' and not necessary to
> 'addr' or branch stack addresses. Add fallback functions that can be used
> to deal with those cases
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Cc: stable@vger.kernel.org # 4.19
> ---
>  tools/perf/util/event.c   | 27 ++++++++++++++++++++++++++
>  tools/perf/util/machine.c | 40 +++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/machine.h |  2 ++
>  tools/perf/util/thread.h  |  4 ++++
>  4 files changed, 73 insertions(+)
> 
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index bc646185f8d9..52b24b8ce29e 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -1576,6 +1576,24 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
>  	return al->map;
>  }
>  
> +/*
> + * For branch stacks or branch samples, the sample cpumode might not be correct
> + * because it applies only to the sample 'ip' and not necessary to 'addr' or
> + * branch stack addresses. If possible, use a fallback to deal with those cases.
> + */
> +struct map *thread__find_map_fallback(struct thread *thread, u8 cpumode,
> +				      u64 addr, struct addr_location *al)
> +{

You named one as _fallback...

> +	struct map *map = thread__find_map(thread, cpumode, addr, al);
> +	struct machine *machine = thread->mg->machine;
> +	u8 addr_cpumode = machine__addr_cpumode(machine, cpumode, addr);
> +
> +	if (map || addr_cpumode == cpumode)
> +		return map;
> +
> +	return thread__find_map(thread, addr_cpumode, addr, al);
> +}
> +
>  struct symbol *thread__find_symbol(struct thread *thread, u8 cpumode,
>  				   u64 addr, struct addr_location *al)
>  {
> @@ -1585,6 +1603,15 @@ struct symbol *thread__find_symbol(struct thread *thread, u8 cpumode,
>  	return al->sym;
>  }
>  
> +struct symbol *thread__find_symbol_fb(struct thread *thread, u8 cpumode,
> +				      u64 addr, struct addr_location *al)

... and the other as _fb, make it consistent, please.

> +{
> +	al->sym = NULL;
> +	if (thread__find_map_fallback(thread, cpumode, addr, al))
> +		al->sym = map__find_symbol(al->map, al->addr);
> +	return al->sym;
> +}
> +
>  /*
>   * Callers need to drop the reference to al->thread, obtained in
>   * machine__findnew_thread()
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 111ae858cbcb..04edc0eac376 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -2542,6 +2542,46 @@ int machine__get_kernel_start(struct machine *machine)
>  	return err;
>  }
>  
> +/*
> + * machine__single_ku_as - Machine has same address space for kernel and user.
> + * @machine: machine object
> + *
> + * Some architectures have a single address space for kernel and user addresses,
> + * which makes it possible to determine if an address is in kernel space or user
> + * space.
> + */
> +static bool machine__single_ku_as(struct machine *machine)
> +{
> +	return strcmp(perf_env__arch(machine->env), "sparc");
> +}

Can we avoid having this strcmp be done repeatedly? I.e. just make this
a boolean initialized at session start, when machine->env is setup, so
we'd have:

   machine->single_address_space

Instead of a function?

Also have you considered making this fallback to be performed only from
code that is that arch specific?

I.e. the code that supports branch samples/stacks is x86_86 specific at
this point and thus only in that case we would call the routines that
perform the fallback, which, in turn, wouldn't need to check for
"sparc"?

> +
> +u8 machine__addr_cpumode(struct machine *machine, u8 cpumode, u64 addr)
> +{
> +	u8 addr_cpumode = cpumode;
> +	bool kernel_ip;
> +
> +	if (!machine__single_ku_as(machine))
> +		goto out;
> +
> +	kernel_ip = machine__kernel_ip(machine, addr);
> +	switch (cpumode) {
> +	case PERF_RECORD_MISC_KERNEL:
> +	case PERF_RECORD_MISC_USER:
> +		addr_cpumode = kernel_ip ? PERF_RECORD_MISC_KERNEL :
> +					   PERF_RECORD_MISC_USER;
> +		break;
> +	case PERF_RECORD_MISC_GUEST_KERNEL:
> +	case PERF_RECORD_MISC_GUEST_USER:
> +		addr_cpumode = kernel_ip ? PERF_RECORD_MISC_GUEST_KERNEL :
> +					   PERF_RECORD_MISC_GUEST_USER;
> +		break;
> +	default:
> +		break;
> +	}
> +out:
> +	return addr_cpumode;
> +}
> +
>  struct dso *machine__findnew_dso(struct machine *machine, const char *filename)
>  {
>  	return dsos__findnew(&machine->dsos, filename);
> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> index d856b85862e2..20d464135ed1 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -99,6 +99,8 @@ static inline bool machine__kernel_ip(struct machine *machine, u64 ip)
>  	return ip >= kernel_start;
>  }
>  
> +u8 machine__addr_cpumode(struct machine *machine, u8 cpumode, u64 addr);
> +
>  struct thread *machine__find_thread(struct machine *machine, pid_t pid,
>  				    pid_t tid);
>  struct comm *machine__thread_exec_comm(struct machine *machine,
> diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> index 36c09a9904e6..e28f9dc1110a 100644
> --- a/tools/perf/util/thread.h
> +++ b/tools/perf/util/thread.h
> @@ -96,9 +96,13 @@ struct thread *thread__main_thread(struct machine *machine, struct thread *threa
>  
>  struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
>  			     struct addr_location *al);
> +struct map *thread__find_map_fallback(struct thread *thread, u8 cpumode,
> +				      u64 addr, struct addr_location *al);
>  
>  struct symbol *thread__find_symbol(struct thread *thread, u8 cpumode,
>  				   u64 addr, struct addr_location *al);
> +struct symbol *thread__find_symbol_fb(struct thread *thread, u8 cpumode,
> +				      u64 addr, struct addr_location *al);
>  
>  void thread__find_cpumode_addr_location(struct thread *thread, u64 addr,
>  					struct addr_location *al);
> -- 
> 2.17.1

Re: [PATCH 3/5] perf tools: Use fallbacks for branch stacks

[Arnaldo Carvalho de Melo]

Em Wed, Oct 31, 2018 at 11:10:41AM +0200, Adrian Hunter escreveu:
> Branch stacks do not necessarily have the same cpumode as the 'ip'. Use the
> fallback functions in those cases.
> 
> This patch depends on patch "perf tools: Add fallback functions for cases
>> where cpumode is insufficient".

This one is ok, i.e. only in places where we need a fallback we do it.

- Arnaldo
 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Cc: stable@vger.kernel.org # 4.19
> ---
>  tools/perf/builtin-script.c                      | 12 ++++++------
>  .../util/scripting-engines/trace-event-python.c  | 16 ++++++++--------
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index b5bc85bd0bbe..996317d8e183 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -728,8 +728,8 @@ static int perf_sample__fprintf_brstack(struct perf_sample *sample,
>  		if (PRINT_FIELD(DSO)) {
>  			memset(&alf, 0, sizeof(alf));
>  			memset(&alt, 0, sizeof(alt));
> -			thread__find_map(thread, sample->cpumode, from, &alf);
> -			thread__find_map(thread, sample->cpumode, to, &alt);
> +			thread__find_map_fallback(thread, sample->cpumode, from, &alf);
> +			thread__find_map_fallback(thread, sample->cpumode, to, &alt);
>  		}
>  
>  		printed += fprintf(fp, " 0x%"PRIx64, from);
> @@ -775,8 +775,8 @@ static int perf_sample__fprintf_brstacksym(struct perf_sample *sample,
>  		from = br->entries[i].from;
>  		to   = br->entries[i].to;
>  
> -		thread__find_symbol(thread, sample->cpumode, from, &alf);
> -		thread__find_symbol(thread, sample->cpumode, to, &alt);
> +		thread__find_symbol_fb(thread, sample->cpumode, from, &alf);
> +		thread__find_symbol_fb(thread, sample->cpumode, to, &alt);
>  
>  		printed += symbol__fprintf_symname_offs(alf.sym, &alf, fp);
>  		if (PRINT_FIELD(DSO)) {
> @@ -820,11 +820,11 @@ static int perf_sample__fprintf_brstackoff(struct perf_sample *sample,
>  		from = br->entries[i].from;
>  		to   = br->entries[i].to;
>  
> -		if (thread__find_map(thread, sample->cpumode, from, &alf) &&
> +		if (thread__find_map_fallback(thread, sample->cpumode, from, &alf) &&
>  		    !alf.map->dso->adjust_symbols)
>  			from = map__map_ip(alf.map, from);
>  
> -		if (thread__find_map(thread, sample->cpumode, to, &alt) &&
> +		if (thread__find_map_fallback(thread, sample->cpumode, to, &alt) &&
>  		    !alt.map->dso->adjust_symbols)
>  			to = map__map_ip(alt.map, to);
>  
> diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
> index 69aa93d4ee99..244aeeee9c7a 100644
> --- a/tools/perf/util/scripting-engines/trace-event-python.c
> +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> @@ -494,14 +494,14 @@ static PyObject *python_process_brstack(struct perf_sample *sample,
>  		pydict_set_item_string_decref(pyelem, "cycles",
>  		    PyLong_FromUnsignedLongLong(br->entries[i].flags.cycles));
>  
> -		thread__find_map(thread, sample->cpumode,
> -				 br->entries[i].from, &al);
> +		thread__find_map_fallback(thread, sample->cpumode,
> +					  br->entries[i].from, &al);
>  		dsoname = get_dsoname(al.map);
>  		pydict_set_item_string_decref(pyelem, "from_dsoname",
>  					      _PyUnicode_FromString(dsoname));
>  
> -		thread__find_map(thread, sample->cpumode,
> -				 br->entries[i].to, &al);
> +		thread__find_map_fallback(thread, sample->cpumode,
> +					  br->entries[i].to, &al);
>  		dsoname = get_dsoname(al.map);
>  		pydict_set_item_string_decref(pyelem, "to_dsoname",
>  					      _PyUnicode_FromString(dsoname));
> @@ -576,14 +576,14 @@ static PyObject *python_process_brstacksym(struct perf_sample *sample,
>  		if (!pyelem)
>  			Py_FatalError("couldn't create Python dictionary");
>  
> -		thread__find_symbol(thread, sample->cpumode,
> -				    br->entries[i].from, &al);
> +		thread__find_symbol_fb(thread, sample->cpumode,
> +				       br->entries[i].from, &al);
>  		get_symoff(al.sym, &al, true, bf, sizeof(bf));
>  		pydict_set_item_string_decref(pyelem, "from",
>  					      _PyUnicode_FromString(bf));
>  
> -		thread__find_symbol(thread, sample->cpumode,
> -				    br->entries[i].to, &al);
> +		thread__find_symbol_fb(thread, sample->cpumode,
> +				       br->entries[i].to, &al);
>  		get_symoff(al.sym, &al, true, bf, sizeof(bf));
>  		pydict_set_item_string_decref(pyelem, "to",
>  					      _PyUnicode_FromString(bf));
> -- 
> 2.17.1

Re: [PATCH 1/5] perf tools: Add fallback functions for cases where cpumode is insufficient

[David Miller]

From: Arnaldo Carvalho de Melo <acme@kernel.org>
Date: Mon, 5 Nov 2018 14:29:46 -0300

>> @@ -2542,6 +2542,46 @@ int machine__get_kernel_start(struct machine *machine)
>>  	return err;
>>  }
>>  
>> +/*
>> + * machine__single_ku_as - Machine has same address space for kernel and user.
>> + * @machine: machine object
>> + *
>> + * Some architectures have a single address space for kernel and user addresses,
>> + * which makes it possible to determine if an address is in kernel space or user
>> + * space.
>> + */
>> +static bool machine__single_ku_as(struct machine *machine)
>> +{
>> +	return strcmp(perf_env__arch(machine->env), "sparc");
>> +}
> 
> Can we avoid having this strcmp be done repeatedly? I.e. just make this
> a boolean initialized at session start, when machine->env is setup, so
> we'd have:
> 
>    machine->single_address_space
> 
> Instead of a function?

Agreed, doing this every time is wasteful.

We could also make it a define in some arch/foo file.

RE: [PATCH 1/5] perf tools: Add fallback functions for cases where cpumode is insufficient

[Hunter, Adrian]

> -----Original Message-----
> From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
> Sent: Monday, November 5, 2018 7:30 PM
> To: Hunter, Adrian <adrian.hunter@intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>; Andi Kleen <ak@linux.intel.com>; linux-
> kernel@vger.kernel.org; leo.yan@linaro.org; David Miller
> <davem@davemloft.net>; Mathieu Poirier <mathieu.poirier@linaro.org>
> Subject: Re: [PATCH 1/5] perf tools: Add fallback functions for cases where
> cpumode is insufficient
> 
> Em Wed, Oct 31, 2018 at 11:10:39AM +0200, Adrian Hunter escreveu:
> > For branch stacks or branch samples, the sample cpumode might not be
> > correct because it applies only to the sample 'ip' and not necessary
> > to 'addr' or branch stack addresses. Add fallback functions that can
> > be used to deal with those cases
> >
> > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: stable@vger.kernel.org # 4.19
> > ---
> >  tools/perf/util/event.c   | 27 ++++++++++++++++++++++++++
> >  tools/perf/util/machine.c | 40
> > +++++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/machine.h |  2 ++
> >  tools/perf/util/thread.h  |  4 ++++
> >  4 files changed, 73 insertions(+)
> >
> > diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c index
> > bc646185f8d9..52b24b8ce29e 100644
> > --- a/tools/perf/util/event.c
> > +++ b/tools/perf/util/event.c
> > @@ -1576,6 +1576,24 @@ struct map *thread__find_map(struct thread
> *thread, u8 cpumode, u64 addr,
> >  	return al->map;
> >  }
> >
> > +/*
> > + * For branch stacks or branch samples, the sample cpumode might not
> > +be correct
> > + * because it applies only to the sample 'ip' and not necessary to
> > +'addr' or
> > + * branch stack addresses. If possible, use a fallback to deal with those
> cases.
> > + */
> > +struct map *thread__find_map_fallback(struct thread *thread, u8
> cpumode,
> > +				      u64 addr, struct addr_location *al) {
> 
> You named one as _fallback...
> 
> > +	struct map *map = thread__find_map(thread, cpumode, addr, al);
> > +	struct machine *machine = thread->mg->machine;
> > +	u8 addr_cpumode = machine__addr_cpumode(machine, cpumode,
> addr);
> > +
> > +	if (map || addr_cpumode == cpumode)
> > +		return map;
> > +
> > +	return thread__find_map(thread, addr_cpumode, addr, al); }
> > +
> >  struct symbol *thread__find_symbol(struct thread *thread, u8 cpumode,
> >  				   u64 addr, struct addr_location *al)  { @@ -
> 1585,6 +1603,15 @@
> > struct symbol *thread__find_symbol(struct thread *thread, u8 cpumode,
> >  	return al->sym;
> >  }
> >
> > +struct symbol *thread__find_symbol_fb(struct thread *thread, u8
> cpumode,
> > +				      u64 addr, struct addr_location *al)
> 
> ... and the other as _fb, make it consistent, please.

ok

> 
> > +{
> > +	al->sym = NULL;
> > +	if (thread__find_map_fallback(thread, cpumode, addr, al))
> > +		al->sym = map__find_symbol(al->map, al->addr);
> > +	return al->sym;
> > +}
> > +
> >  /*
> >   * Callers need to drop the reference to al->thread, obtained in
> >   * machine__findnew_thread()
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index 111ae858cbcb..04edc0eac376 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -2542,6 +2542,46 @@ int machine__get_kernel_start(struct machine
> *machine)
> >  	return err;
> >  }
> >
> > +/*
> > + * machine__single_ku_as - Machine has same address space for kernel
> and user.
> > + * @machine: machine object
> > + *
> > + * Some architectures have a single address space for kernel and user
> > +addresses,
> > + * which makes it possible to determine if an address is in kernel
> > +space or user
> > + * space.
> > + */
> > +static bool machine__single_ku_as(struct machine *machine) {
> > +	return strcmp(perf_env__arch(machine->env), "sparc"); }
> 
> Can we avoid having this strcmp be done repeatedly?

It is only done if there are mapping errors

 > Can we avoid having this strcmp be done repeatedly? I.e. just make this a
> boolean initialized at session start, when machine->env is setup, so we'd
> have:
> 
>    machine->single_address_space
> 
> Instead of a function?

Sure  thing.

> 
> Also have you considered making this fallback to be performed only from
> code that is that arch specific?
> 
> I.e. the code that supports branch samples/stacks is x86_86 specific at this
> point and thus only in that case we would call the routines that perform the
> fallback, which, in turn, wouldn't need to check for "sparc"?

I will look at it, but theoretically someone could be processing x86 data but
doing it on a machine of a different architecture.

> 
> > +
> > +u8 machine__addr_cpumode(struct machine *machine, u8 cpumode, u64
> > +addr) {
> > +	u8 addr_cpumode = cpumode;
> > +	bool kernel_ip;
> > +
> > +	if (!machine__single_ku_as(machine))
> > +		goto out;
> > +
> > +	kernel_ip = machine__kernel_ip(machine, addr);
> > +	switch (cpumode) {
> > +	case PERF_RECORD_MISC_KERNEL:
> > +	case PERF_RECORD_MISC_USER:
> > +		addr_cpumode = kernel_ip ? PERF_RECORD_MISC_KERNEL :
> > +					   PERF_RECORD_MISC_USER;
> > +		break;
> > +	case PERF_RECORD_MISC_GUEST_KERNEL:
> > +	case PERF_RECORD_MISC_GUEST_USER:
> > +		addr_cpumode = kernel_ip ?
> PERF_RECORD_MISC_GUEST_KERNEL :
> > +					   PERF_RECORD_MISC_GUEST_USER;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +out:
> > +	return addr_cpumode;
> > +}
> > +
> >  struct dso *machine__findnew_dso(struct machine *machine, const char
> > *filename)  {
> >  	return dsos__findnew(&machine->dsos, filename); diff --git
> > a/tools/perf/util/machine.h b/tools/perf/util/machine.h index
> > d856b85862e2..20d464135ed1 100644
> > --- a/tools/perf/util/machine.h
> > +++ b/tools/perf/util/machine.h
> > @@ -99,6 +99,8 @@ static inline bool machine__kernel_ip(struct machine
> *machine, u64 ip)
> >  	return ip >= kernel_start;
> >  }
> >
> > +u8 machine__addr_cpumode(struct machine *machine, u8 cpumode, u64
> > +addr);
> > +
> >  struct thread *machine__find_thread(struct machine *machine, pid_t pid,
> >  				    pid_t tid);
> >  struct comm *machine__thread_exec_comm(struct machine *machine,
> diff
> > --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h index
> > 36c09a9904e6..e28f9dc1110a 100644
> > --- a/tools/perf/util/thread.h
> > +++ b/tools/perf/util/thread.h
> > @@ -96,9 +96,13 @@ struct thread *thread__main_thread(struct machine
> > *machine, struct thread *threa
> >
> >  struct map *thread__find_map(struct thread *thread, u8 cpumode, u64
> addr,
> >  			     struct addr_location *al);
> > +struct map *thread__find_map_fallback(struct thread *thread, u8
> cpumode,
> > +				      u64 addr, struct addr_location *al);
> >
> >  struct symbol *thread__find_symbol(struct thread *thread, u8 cpumode,
> >  				   u64 addr, struct addr_location *al);
> > +struct symbol *thread__find_symbol_fb(struct thread *thread, u8
> cpumode,
> > +				      u64 addr, struct addr_location *al);
> >
> >  void thread__find_cpumode_addr_location(struct thread *thread, u64
> addr,
> >  					struct addr_location *al);
> > --
> > 2.17.1

Re: [PATCH 1/5] perf tools: Add fallback functions for cases where cpumode is insufficient

[Arnaldo Carvalho de Melo]

Em Mon, Nov 05, 2018 at 07:21:44PM +0000, Hunter, Adrian escreveu:
> > In Monday, November 5, 2018 7:30 PM, Arnaldo Carvalho de Melo wrote
<SNIP>
> > > +struct map *thread__find_map_fallback(struct thread *thread, u8
> > cpumode,
> > > +				      u64 addr, struct addr_location *al) {

> > You named one as _fallback...

> > > +	struct map *map = thread__find_map(thread, cpumode, addr, al);
> > > +	struct machine *machine = thread->mg->machine;
> > > +	u8 addr_cpumode = machine__addr_cpumode(machine, cpumode,
> > addr);
> > > +
> > > +	if (map || addr_cpumode == cpumode)
> > > +		return map;
> > > +
> > > +	return thread__find_map(thread, addr_cpumode, addr, al); }
> > > +
> > >  struct symbol *thread__find_symbol(struct thread *thread, u8 cpumode,
> > >  				   u64 addr, struct addr_location *al)  { @@ -
> > 1585,6 +1603,15 @@
> > > struct symbol *thread__find_symbol(struct thread *thread, u8 cpumode,
> > >  	return al->sym;
> > >  }

> > > +struct symbol *thread__find_symbol_fb(struct thread *thread, u8
> > cpumode,
> > > +				      u64 addr, struct addr_location *al)

> > ... and the other as _fb, make it consistent, please.

> ok

> > > +{
> > > +	al->sym = NULL;
> > > +	if (thread__find_map_fallback(thread, cpumode, addr, al))
> > > +		al->sym = map__find_symbol(al->map, al->addr);
> > > +	return al->sym;
> > > +}

> > >  /*
> > >   * Callers need to drop the reference to al->thread, obtained in
> > >   * machine__findnew_thread()
> > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > > index 111ae858cbcb..04edc0eac376 100644
> > > --- a/tools/perf/util/machine.c
> > > +++ b/tools/perf/util/machine.c
> > > @@ -2542,6 +2542,46 @@ int machine__get_kernel_start(struct machine
> > *machine)
> > >  	return err;
> > >  }

> > > +/*
> > > + * machine__single_ku_as - Machine has same address space for kernel
> > and user.
> > > + * @machine: machine object
> > > + *
> > > + * Some architectures have a single address space for kernel and user
> > > +addresses,
> > > + * which makes it possible to determine if an address is in kernel
> > > +space or user
> > > + * space.
> > > + */
> > > +static bool machine__single_ku_as(struct machine *machine) {
> > > +	return strcmp(perf_env__arch(machine->env), "sparc"); }

> > Can we avoid having this strcmp be done repeatedly?
 
> It is only done if there are mapping errors
 
>  > Can we avoid having this strcmp be done repeatedly? I.e. just make this a
> > boolean initialized at session start, when machine->env is setup, so we'd
> > have:

> >    machine->single_address_space

> > Instead of a function?
 
> Sure  thing.

Thanks
 
> > 
> > Also have you considered making this fallback to be performed only from
> > code that is that arch specific?
> > 
> > I.e. the code that supports branch samples/stacks is x86_86 specific at this
> > point and thus only in that case we would call the routines that perform the
> > fallback, which, in turn, wouldn't need to check for "sparc"?
 
> I will look at it, but theoretically someone could be processing x86 data but
> doing it on a machine of a different architecture.

Right, that should be supported, yes. What I meant was that when
processing perf.data file with samples where the cpumode can't be
inferred, we should use the fallback routines.

It is super unfortunate that we have addresses without a accompanying
cpumode :-\ Don't you think those coulde be fixed somehow? If this comes
from things synthesized from Intel PT traces, then we can use the
address ranges for kernel/userspace to derive that before hitting the
core code, that would be fed with addr/cpumode pairs, just like we have
hdr.misc & USER/KERNEL and the PERF_CONTEXT_ markers in callchains.

- Arnaldo

RE: [PATCH 1/5] perf tools: Add fallback functions for cases where cpumode is insufficient

[Hunter, Adrian]

> -----Original Message-----
> From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
> Sent: Monday, November 5, 2018 9:36 PM
> To: Hunter, Adrian <adrian.hunter@intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>; Andi Kleen <ak@linux.intel.com>; linux-
> kernel@vger.kernel.org; leo.yan@linaro.org; David Miller
> <davem@davemloft.net>; Mathieu Poirier <mathieu.poirier@linaro.org>
> Subject: Re: [PATCH 1/5] perf tools: Add fallback functions for cases where
> cpumode is insufficient
> 
> Em Mon, Nov 05, 2018 at 07:21:44PM +0000, Hunter, Adrian escreveu:
> > > In Monday, November 5, 2018 7:30 PM, Arnaldo Carvalho de Melo wrote
> <SNIP>
> > > > +struct map *thread__find_map_fallback(struct thread *thread, u8
> > > cpumode,
> > > > +				      u64 addr, struct addr_location *al) {
> 
> > > You named one as _fallback...
> 
> > > > +	struct map *map = thread__find_map(thread, cpumode, addr, al);
> > > > +	struct machine *machine = thread->mg->machine;
> > > > +	u8 addr_cpumode = machine__addr_cpumode(machine, cpumode,
> > > addr);
> > > > +
> > > > +	if (map || addr_cpumode == cpumode)
> > > > +		return map;
> > > > +
> > > > +	return thread__find_map(thread, addr_cpumode, addr, al); }
> > > > +
> > > >  struct symbol *thread__find_symbol(struct thread *thread, u8
> cpumode,
> > > >  				   u64 addr, struct addr_location *al)  { @@ -
> > > 1585,6 +1603,15 @@
> > > > struct symbol *thread__find_symbol(struct thread *thread, u8
> cpumode,
> > > >  	return al->sym;
> > > >  }
> 
> > > > +struct symbol *thread__find_symbol_fb(struct thread *thread, u8
> > > cpumode,
> > > > +				      u64 addr, struct addr_location *al)
> 
> > > ... and the other as _fb, make it consistent, please.
> 
> > ok
> 
> > > > +{
> > > > +	al->sym = NULL;
> > > > +	if (thread__find_map_fallback(thread, cpumode, addr, al))
> > > > +		al->sym = map__find_symbol(al->map, al->addr);
> > > > +	return al->sym;
> > > > +}
> 
> > > >  /*
> > > >   * Callers need to drop the reference to al->thread, obtained in
> > > >   * machine__findnew_thread()
> > > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > > > index 111ae858cbcb..04edc0eac376 100644
> > > > --- a/tools/perf/util/machine.c
> > > > +++ b/tools/perf/util/machine.c
> > > > @@ -2542,6 +2542,46 @@ int machine__get_kernel_start(struct
> > > > machine
> > > *machine)
> > > >  	return err;
> > > >  }
> 
> > > > +/*
> > > > + * machine__single_ku_as - Machine has same address space for
> > > > +kernel
> > > and user.
> > > > + * @machine: machine object
> > > > + *
> > > > + * Some architectures have a single address space for kernel and
> > > > +user addresses,
> > > > + * which makes it possible to determine if an address is in
> > > > +kernel space or user
> > > > + * space.
> > > > + */
> > > > +static bool machine__single_ku_as(struct machine *machine) {
> > > > +	return strcmp(perf_env__arch(machine->env), "sparc"); }
> 
> > > Can we avoid having this strcmp be done repeatedly?
> 
> > It is only done if there are mapping errors
> 
> >  > Can we avoid having this strcmp be done repeatedly? I.e. just make
> > this a
> > > boolean initialized at session start, when machine->env is setup, so
> > > we'd
> > > have:
> 
> > >    machine->single_address_space
> 
> > > Instead of a function?
> 
> > Sure  thing.
> 
> Thanks
> 
> > >
> > > Also have you considered making this fallback to be performed only
> > > from code that is that arch specific?
> > >
> > > I.e. the code that supports branch samples/stacks is x86_86 specific
> > > at this point and thus only in that case we would call the routines
> > > that perform the fallback, which, in turn, wouldn't need to check for
> "sparc"?
> 
> > I will look at it, but theoretically someone could be processing x86
> > data but doing it on a machine of a different architecture.
> 
> Right, that should be supported, yes. What I meant was that when
> processing perf.data file with samples where the cpumode can't be inferred,
> we should use the fallback routines.
> 
> It is super unfortunate that we have addresses without a accompanying
> cpumode :-\ Don't you think those coulde be fixed somehow? If this comes
> from things synthesized from Intel PT traces, then we can use the address
> ranges for kernel/userspace to derive that before hitting the core code, that
> would be fed with addr/cpumode pairs, just like we have hdr.misc &
> USER/KERNEL and the PERF_CONTEXT_ markers in callchains.

Yes we will probably need to look at that, but at the moment I would like a fix for stable.

Re: [PATCH 1/5] perf tools: Add fallback functions for cases where cpumode is insufficient

[Arnaldo Carvalho de Melo]

Em Mon, Nov 05, 2018 at 07:53:17PM +0000, Hunter, Adrian escreveu:
> > -----Original Message-----
> > From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
> > Sent: Monday, November 5, 2018 9:36 PM
> > To: Hunter, Adrian <adrian.hunter@intel.com>
> > Cc: Jiri Olsa <jolsa@redhat.com>; Andi Kleen <ak@linux.intel.com>; linux-
> > kernel@vger.kernel.org; leo.yan@linaro.org; David Miller
> > <davem@davemloft.net>; Mathieu Poirier <mathieu.poirier@linaro.org>
> > Subject: Re: [PATCH 1/5] perf tools: Add fallback functions for cases where
> > cpumode is insufficient
> > 
> > Em Mon, Nov 05, 2018 at 07:21:44PM +0000, Hunter, Adrian escreveu:
> > > > In Monday, November 5, 2018 7:30 PM, Arnaldo Carvalho de Melo wrote
> > > > Also have you considered making this fallback to be performed only
> > > > from code that is that arch specific?

> > > > I.e. the code that supports branch samples/stacks is x86_86 specific
> > > > at this point and thus only in that case we would call the routines
> > > > that perform the fallback, which, in turn, wouldn't need to check for
> > "sparc"?
> > 
> > > I will look at it, but theoretically someone could be processing x86
> > > data but doing it on a machine of a different architecture.
> > 
> > Right, that should be supported, yes. What I meant was that when
> > processing perf.data file with samples where the cpumode can't be inferred,
> > we should use the fallback routines.
> > 
> > It is super unfortunate that we have addresses without a accompanying
> > cpumode :-\ Don't you think those coulde be fixed somehow? If this comes
> > from things synthesized from Intel PT traces, then we can use the address
> > ranges for kernel/userspace to derive that before hitting the core code, that
> > would be fed with addr/cpumode pairs, just like we have hdr.misc &
> > USER/KERNEL and the PERF_CONTEXT_ markers in callchains.
> 
> Yes we will probably need to look at that, but at the moment I would like a fix for stable.

Ok, with that check for archs like sparc, fair enough, and its great
that you consider doing the better fix on top of it, later, thanks!

- Arnaldo

Re: [PATCH 1/5] perf tools: Add fallback functions for cases where cpumode is insufficient

[Arnaldo Carvalho de Melo]

Em Mon, Nov 05, 2018 at 07:53:17PM +0000, Hunter, Adrian escreveu:
> > -----Original Message-----
> > From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
> > Sent: Monday, November 5, 2018 9:36 PM
> > To: Hunter, Adrian <adrian.hunter@intel.com>
> > Cc: Jiri Olsa <jolsa@redhat.com>; Andi Kleen <ak@linux.intel.com>; linux-
> > kernel@vger.kernel.org; leo.yan@linaro.org; David Miller
> > <davem@davemloft.net>; Mathieu Poirier <mathieu.poirier@linaro.org>
> > Subject: Re: [PATCH 1/5] perf tools: Add fallback functions for cases where
> > cpumode is insufficient
> > 
> > Em Mon, Nov 05, 2018 at 07:21:44PM +0000, Hunter, Adrian escreveu:
> > > > In Monday, November 5, 2018 7:30 PM, Arnaldo Carvalho de Melo wrote
> > <SNIP>
> > > > > +struct map *thread__find_map_fallback(struct thread *thread, u8
> > > > cpumode,
> > > > > +				      u64 addr, struct addr_location *al) {
> > 
> > > > You named one as _fallback...
> > 
> > > > > +	struct map *map = thread__find_map(thread, cpumode, addr, al);
> > > > > +	struct machine *machine = thread->mg->machine;
> > > > > +	u8 addr_cpumode = machine__addr_cpumode(machine, cpumode,
> > > > addr);
> > > > > +
> > > > > +	if (map || addr_cpumode == cpumode)
> > > > > +		return map;
> > > > > +
> > > > > +	return thread__find_map(thread, addr_cpumode, addr, al); }
> > > > > +
> > > > >  struct symbol *thread__find_symbol(struct thread *thread, u8
> > cpumode,
> > > > >  				   u64 addr, struct addr_location *al)  { @@ -
> > > > 1585,6 +1603,15 @@
> > > > > struct symbol *thread__find_symbol(struct thread *thread, u8
> > cpumode,
> > > > >  	return al->sym;
> > > > >  }
> > 
> > > > > +struct symbol *thread__find_symbol_fb(struct thread *thread, u8
> > > > cpumode,
> > > > > +				      u64 addr, struct addr_location *al)
> > 
> > > > ... and the other as _fb, make it consistent, please.
> > 
> > > ok
> > 
> > > > > +{
> > > > > +	al->sym = NULL;
> > > > > +	if (thread__find_map_fallback(thread, cpumode, addr, al))
> > > > > +		al->sym = map__find_symbol(al->map, al->addr);
> > > > > +	return al->sym;
> > > > > +}
> > 
> > > > >  /*
> > > > >   * Callers need to drop the reference to al->thread, obtained in
> > > > >   * machine__findnew_thread()
> > > > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > > > > index 111ae858cbcb..04edc0eac376 100644
> > > > > --- a/tools/perf/util/machine.c
> > > > > +++ b/tools/perf/util/machine.c
> > > > > @@ -2542,6 +2542,46 @@ int machine__get_kernel_start(struct
> > > > > machine
> > > > *machine)
> > > > >  	return err;
> > > > >  }
> > 
> > > > > +/*
> > > > > + * machine__single_ku_as - Machine has same address space for
> > > > > +kernel
> > > > and user.
> > > > > + * @machine: machine object
> > > > > + *
> > > > > + * Some architectures have a single address space for kernel and
> > > > > +user addresses,
> > > > > + * which makes it possible to determine if an address is in
> > > > > +kernel space or user
> > > > > + * space.
> > > > > + */
> > > > > +static bool machine__single_ku_as(struct machine *machine) {
> > > > > +	return strcmp(perf_env__arch(machine->env), "sparc"); }
> > 
> > > > Can we avoid having this strcmp be done repeatedly?
> > 
> > > It is only done if there are mapping errors
> > 
> > >  > Can we avoid having this strcmp be done repeatedly? I.e. just make
> > > this a
> > > > boolean initialized at session start, when machine->env is setup, so
> > > > we'd
> > > > have:
> > 
> > > >    machine->single_address_space
> > 
> > > > Instead of a function?
> > 
> > > Sure  thing.

Have you sent an update to this patch series addressing the concerns? I
think that renaming that _fb to __fallback and having this
machine->single_address_space is what is missing, I'll try to take a
stab at this now and have it in my perf/core branch soon.

But you have this already done, please send it.

- Arnaldo

Re: [PATCH 1/5] perf tools: Add fallback functions for cases where cpumode is insufficient

[Arnaldo Carvalho de Melo]

Em Mon, Nov 26, 2018 at 04:02:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Nov 05, 2018 at 07:53:17PM +0000, Hunter, Adrian escreveu:
> > > From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
> > > Em Mon, Nov 05, 2018 at 07:21:44PM +0000, Hunter, Adrian escreveu:
> > > > > > +static bool machine__single_ku_as(struct machine *machine) {
> > > > > > +	return strcmp(perf_env__arch(machine->env), "sparc"); }

> > > > > Can we avoid having this strcmp be done repeatedly?

> > > > It is only done if there are mapping errors

> > > > > Can we avoid having this strcmp be done repeatedly? I.e. just
> > > > > make this a boolean initialized at session start, when
> > > > > machine->env is setup, so we'd have:

> > > > >    machine->single_address_space

> > > > > Instead of a function?

> > > > Sure  thing.
> 
> Have you sent an update to this patch series addressing the concerns? I
> think that renaming that _fb to __fallback and having this
> machine->single_address_space is what is missing, I'll try to take a
> stab at this now and have it in my perf/core branch soon.
 
> But you have this already done, please send it.

So, here is the patch on top of yours and at the end a patch that I'm
inserting just before yours, please check and ack, thanks:

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index da523d99be4b..4506aa7fa557 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -2592,25 +2592,13 @@ int machine__get_kernel_start(struct machine *machine)
 	return err;
 }
 
-/*
- * machine__single_ku_as - Machine has same address space for kernel and user.
- * @machine: machine object
- *
- * Some architectures have a single address space for kernel and user addresses,
- * which makes it possible to determine if an address is in kernel space or user
- * space.
- */
-static bool machine__single_ku_as(struct machine *machine)
-{
-	return strcmp(perf_env__arch(machine->env), "sparc");
-}
 
 u8 machine__addr_cpumode(struct machine *machine, u8 cpumode, u64 addr)
 {
 	u8 addr_cpumode = cpumode;
 	bool kernel_ip;
 
-	if (!machine__single_ku_as(machine))
+	if (!machine->env->single_address_space)
 		goto out;
 
 	kernel_ip = machine__kernel_ip(machine, addr);


commit 5aabc18da787f7f6785b4027c63a89592e4e2c03
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Mon Nov 26 16:25:25 2018 -0300

    perf env: Record if a arch has a single user/kernel address space
    
    Some architectures have a single address space for kernel and user
    addresses, which makes it possible to determine if an address is in
    kernel space or user space. Some don't, e.g.: sparc.
    
    Cache that info in perf_env so that, for instance, code needing to
    fallback failed symbol lookups at the kernel space in single address
    space arches can lookup at userspace.
    
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: Andi Kleen <ak@linux.intel.com>
    Cc: David Ahern <dsahern@gmail.com>
    Cc: David Miller <davem@davemloft.net>
    Cc: Jiri Olsa <jolsa@kernel.org>
    Cc: Leo Yan <leo.yan@linaro.org>
    Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Wang Nan <wangnan0@huawei.com>
    Link: https://lkml.kernel.org/n/tip-4jb2rusqml7utgebiaf51k77@git.kernel.org
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index 59f38c7693f8..d1cae06a2dfc 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -8,6 +8,16 @@
 
 struct perf_env perf_env;
 
+/*
+ * Some architectures have a single address space for kernel and user
+ * addresses, which makes it possible to determine if an address is in kernel
+ * space or user space. Some don't, e.g.: sparc
+ */
+void perf_env__init_single_address_space(struct perf_env *env)
+{
+	env->single_address_space = strcmp(perf_env__arch(env), "sparc");
+}
+
 void perf_env__exit(struct perf_env *env)
 {
 	int i;
diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
index d01b8355f4ca..a79898f6417d 100644
--- a/tools/perf/util/env.h
+++ b/tools/perf/util/env.h
@@ -60,6 +60,12 @@ struct perf_env {
 	struct cpu_topology_map	*cpu;
 	struct cpu_cache_level	*caches;
 	int			 caches_cnt;
+	/*
+         * Some architectures have a single address space for kernel and user
+         * addresses, which makes it possible to determine if an address is in
+         * kernel space or user space. Some don't, e.g.: sparc
+         */
+	bool			single_address_space;
 	struct numa_node	*numa_nodes;
 	struct memory_node	*memory_nodes;
 	unsigned long long	 memory_bsize;
@@ -72,6 +78,8 @@ void perf_env__exit(struct perf_env *env);
 
 int perf_env__set_cmdline(struct perf_env *env, int argc, const char *argv[]);
 
+void perf_env__init_single_address_space(struct perf_env *env);
+
 int perf_env__read_cpu_topology_map(struct perf_env *env);
 
 void cpu_cache_level__free(struct cpu_cache_level *cache);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 7d2c8ce6cfad..164d56229135 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -150,6 +150,8 @@ struct perf_session *perf_session__new(struct perf_data *data,
 		session->machines.host.env = &perf_env;
 	}
 
+	perf_env__init_single_address_space(session->machines.host.env);
+
 	if (!data || perf_data__is_write(data)) {
 		/*
 		 * In O_RDONLY mode this will be performed when reading the

Re: [PATCH 1/5] perf tools: Add fallback functions for cases where cpumode is insufficient

[Arnaldo Carvalho de Melo]

Em Mon, Nov 26, 2018 at 04:41:45PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Nov 26, 2018 at 04:02:17PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Nov 05, 2018 at 07:53:17PM +0000, Hunter, Adrian escreveu:
> > > > From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
> > > > Em Mon, Nov 05, 2018 at 07:21:44PM +0000, Hunter, Adrian escreveu:
> > > > > > > +static bool machine__single_ku_as(struct machine *machine) {
> > > > > > > +	return strcmp(perf_env__arch(machine->env), "sparc"); }
> 
> > > > > > Can we avoid having this strcmp be done repeatedly?
> 
> > > > > It is only done if there are mapping errors
> 
> > > > > > Can we avoid having this strcmp be done repeatedly? I.e. just
> > > > > > make this a boolean initialized at session start, when
> > > > > > machine->env is setup, so we'd have:
> 
> > > > > >    machine->single_address_space
> 
> > > > > > Instead of a function?
> 
> > > > > Sure  thing.
> > 
> > Have you sent an update to this patch series addressing the concerns? I
> > think that renaming that _fb to __fallback and having this
> > machine->single_address_space is what is missing, I'll try to take a
> > stab at this now and have it in my perf/core branch soon.
>  
> > But you have this already done, please send it.
> 
> So, here is the patch on top of yours and at the end a patch that I'm
> inserting just before yours, please check and ack, thanks:


Argh, nevermind, just saw your V2, /me goes to read it ;-\

- Arnaldo