perf tools: fix incorrect ordering of callchain entries

The current implementation of the python database export API only includes call path information when using some form of call/return tracing, but is unable to do so when sampling.

The following API extensions allow exporting of data collected by perf record when using --call-graph.

The additions to the python api include the following:
- add call_path_id to sample_table to allow association of samples with call paths

- add dso_id to call_path_table to more closely align the data with that of a callchain_node

db-export and trace-script-python were both extended to accomidate the API

changes listed above.

Thread-stack's functionality was expanded to allow storage and exporting of callchains that result from individual samples.

- Introduced a new static function (thread_stack__process_callchain) to resolve call paths using the existing callchain resolution provided by thread__resolve_callchain

- The existing call_path tree in call_return_processor is used for storing the data from the resolved callchain.

- Call_return_processor was also extended to allow the ability to export full call paths in addtion to the existing individual call/return pairs, since call/return pairs are not available when doing sampling.

The call_paths table already has symbol_id which belongs uniquely to a DSO, so why do we need dso_id as well?

Why do you need a callback? Seems like the only thing you need from thread-stack.c is the call path tree. You could move that to its own .c/.h files and then process the call chain in db-export.c

Also a list of changes like the one above heavily implies you are not obeying the one patch == one change rule. Please try to make patches that only do one thing and also run checkpatch.

If you don't mind, I'll let you respond to my comments before I comment on any other patches.

Quoted 2 lines · Adrian Hunter

The call_paths table already has symbol_id which belongs uniquely to a DSO, so why do we need dso_id as well?

If the symbol_id is 0 because the IP could not be resolved to a symbol, this is not necessarily a valid assumption. Without a dso_id in the call_paths table, it is not possible to resolve the dso when symbol information is missing. the db_export api currently does not have enough information to match a DSO with an IP.

It is often useful to still have the call path associated with a DSO even if there is no symbol, which i why i recommend keeping the dso_id in the call_paths table.

Quoted 3 lines

Why do you need a callback? Seems like the only thing you need from thread-stack.c is the call path tree. You could move that to its own .c/.h files and then process the call chain in db-export.c

My original intent was to reuse existing code with minimal changes and conform the existing design patterns they used. Thread-stack.c, for example, currently uses a callback to populate the call_return table, so I used a callback as well to populate the call_path table.

I am open to making this change if it is believed it will result in a cleaner implementation.

Quoted 4 lines

Also a list of changes like the one above heavily implies you are not obeying the one patch == one change rule. Please try to make patches that only do one thing and also run checkpatch.

While i can split this into a few smaller patches there is only really justification for applying all of them all together. If this is still preferred i can resubmit this in smaller parts.

Quoted 4 lines

If you don't mind, I'll let you respond to my comments before I comment on any other patches.

Let me know if you have any additional comments.

Thanks,
Chris

The current instructions for setting up an Ubuntu system for using the export-to-postgresql.py script are incorrect.

The instructions in the script have been updated to work on newer versions of ubuntu.

(quoted diff) +0 -2

-Add missing dependencies to apt-get command:
    python-pyside.qtsql, libqt4-sql-psql
-Add '-s' option to createuser command to force the user to be a
    superuser since the command doesn't prompt as indicated in the
    current instructions.

Tested on: Ubuntu 14.04, Ubuntu 16.04(beta)

Signed-off-by Chris Phlipot <[email protected]>

---
 tools/perf/scripts/python/export-to-postgresql.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

tools/perf/scripts/python/export-to-postgresql.py +2 -3

index 1b02cdc..6f0ca68 100644
@@ -34,10 +34,9 @@ import datetime
 #
 # ubuntu:
 #
-#	$ sudo apt-get install postgresql
+#	$ sudo apt-get install postgresql python-pyside.qtsql libqt4-sql-psql
 #	$ sudo su - postgres
-#	$ createuser <your user id here>
-#	Shall the new role be a superuser? (y/n) y
+#	$ createuser -s <your user id here>
 #
 # An example of using this script with Intel PT:
 #

Signature · 1 line

2.7.4

The existing implentation implementation of thread__resolve_callchain,

under certain circumstanes, can assemble callchain entries in the

incorrect order.

A the callchain entries are resolved incorrectly for a sample when all of the following conditions are met:

1. callchain_param.order is set to ORDER_CALLER

2. thread__resolve_callchain_sample is able to resolve callchain entries for the sample.

3. unwind__get_entries is also able to resolve callchain entries for the sample.

The fix is accomplished by reversing the order in which thread__resolve_callchain_sample and unwind__get_entries are called when callchain_param.order is set to ORDER_CALLER.

Unwind specific code from thread__resolve_callchain is also moved into a new static function to improve readability of the fix.

Signed-off-by Chris Phlipot <[email protected]>

---
 tools/perf/util/machine.c | 57 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 16 deletions(-)

tools/perf/util/machine.c +41 -16

index 0c4dabc..dd086c8 100644
@@ -1806,8 +1806,6 @@ static int thread__resolve_callchain_sample(struct thread *thread,
 	int skip_idx = -1;
 	int first_call = 0;
 
-	callchain_cursor_reset(cursor);
-
 	if (has_branch_callstack(evsel)) {
 		err = resolve_lbr_callchain_sample(thread, cursor, sample, parent,
 						   root_al, max_stack);

@@ -1918,20 +1916,12 @@ static int unwind_entry(struct unwind_entry *entry, void *arg)
 				       entry->map, entry->sym);
 }
 
-int thread__resolve_callchain(struct thread *thread,
-			      struct callchain_cursor *cursor,
-			      struct perf_evsel *evsel,
-			      struct perf_sample *sample,
-			      struct symbol **parent,
-			      struct addr_location *root_al,
-			      int max_stack)
-{
-	int ret = thread__resolve_callchain_sample(thread, cursor, evsel,
-						   sample, parent,
-						   root_al, max_stack);
-	if (ret)
-		return ret;
-
+static int thread__resolve_callchain_unwind(struct thread *thread,
+                                            struct callchain_cursor *cursor,
+					    struct perf_evsel *evsel,
+					    struct perf_sample *sample,
+					    int max_stack)
+{
 	/* Can we do dwarf post unwind? */
 	if (!((evsel->attr.sample_type & PERF_SAMPLE_REGS_USER) &&
 	      (evsel->attr.sample_type & PERF_SAMPLE_STACK_USER)))

@@ -1944,7 +1934,42 @@ int thread__resolve_callchain(struct thread *thread,
 
 	return unwind__get_entries(unwind_entry, cursor,
 				   thread, sample, max_stack);
+}
+
+int thread__resolve_callchain(struct thread *thread,
+                              struct callchain_cursor *cursor,
+                              struct perf_evsel *evsel,
+                              struct perf_sample *sample,
+                              struct symbol **parent,
+                              struct addr_location *root_al,
+                              int max_stack)
+{
+	int ret = 0;
+	callchain_cursor_reset(&callchain_cursor);
+
+	if (callchain_param.order == ORDER_CALLEE) {
+                ret = thread__resolve_callchain_sample(thread, cursor,
+                                                       evsel, sample,
+						       parent, root_al,
+						       max_stack);
+		if (ret)
+			return ret;
+                ret = thread__resolve_callchain_unwind(thread, cursor,
+                                                       evsel, sample,
+						       max_stack);
+	} else {
+                ret = thread__resolve_callchain_unwind(thread, cursor,
+                                                       evsel, sample,
+						       max_stack);
+		if (ret)
+			return ret;
+                ret = thread__resolve_callchain_sample(thread, cursor,
+                                                       evsel, sample,
+						       parent, root_al,
+						       max_stack);
+	}
 
+	return ret;
 }
 
 int machine__for_each_thread(struct machine *machine,

Quoted 1 line · Chris Phlipot

The existing implentation implementation of thread__resolve_callchain,

Remove 'implentation'

Quoted 1 line

under certain circumstanes, can assemble callchain entries in the

'circumstanes' -> 'circumstances'

Quoted 10 lines

incorrect order.

A the callchain entries are resolved incorrectly for a sample when all of the following conditions are met:

1. callchain_param.order is set to ORDER_CALLER

2. thread__resolve_callchain_sample is able to resolve callchain entries for the sample.

Quoted 6 lines

3. unwind__get_entries is also able to resolve callchain entries for the sample.

The fix is accomplished by reversing the order in which thread__resolve_callchain_sample and unwind__get_entries are called when callchain_param.order is set to ORDER_CALLER.

Can you give an example of the commands you used and what the call chain looked like before and after.

Also please run ./scripts/checkpatch.pl

Quoted 93 lines

Unwind specific code from thread__resolve_callchain is also moved into a new static function to improve readability of the fix.

Signed-off-by Chris Phlipot <[email protected]>

---
 tools/perf/util/machine.c | 57 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 16 deletions(-)

tools/perf/util/machine.c +41 -16

index 0c4dabc..dd086c8 100644
@@ -1806,8 +1806,6 @@ static int thread__resolve_callchain_sample(struct thread *thread,
 	int skip_idx = -1;
 	int first_call = 0;
 
-	callchain_cursor_reset(cursor);
-
 	if (has_branch_callstack(evsel)) {
 		err = resolve_lbr_callchain_sample(thread, cursor, sample, parent,
 						   root_al, max_stack);

@@ -1918,20 +1916,12 @@ static int unwind_entry(struct unwind_entry *entry, void *arg)
 				       entry->map, entry->sym);
 }
 
-int thread__resolve_callchain(struct thread *thread,
-			      struct callchain_cursor *cursor,
-			      struct perf_evsel *evsel,
-			      struct perf_sample *sample,
-			      struct symbol **parent,
-			      struct addr_location *root_al,
-			      int max_stack)
-{
-	int ret = thread__resolve_callchain_sample(thread, cursor, evsel,
-						   sample, parent,
-						   root_al, max_stack);
-	if (ret)
-		return ret;
-
+static int thread__resolve_callchain_unwind(struct thread *thread,
+                                            struct callchain_cursor *cursor,
+					    struct perf_evsel *evsel,
+					    struct perf_sample *sample,
+					    int max_stack)
+{
 	/* Can we do dwarf post unwind? */
 	if (!((evsel->attr.sample_type & PERF_SAMPLE_REGS_USER) &&
 	      (evsel->attr.sample_type & PERF_SAMPLE_STACK_USER)))

@@ -1944,7 +1934,42 @@ int thread__resolve_callchain(struct thread *thread,
 
 	return unwind__get_entries(unwind_entry, cursor,
 				   thread, sample, max_stack);
+}
+
+int thread__resolve_callchain(struct thread *thread,
+                              struct callchain_cursor *cursor,
+                              struct perf_evsel *evsel,
+                              struct perf_sample *sample,
+                              struct symbol **parent,
+                              struct addr_location *root_al,
+                              int max_stack)
+{
+	int ret = 0;
+	callchain_cursor_reset(&callchain_cursor);
+
+	if (callchain_param.order == ORDER_CALLEE) {
+                ret = thread__resolve_callchain_sample(thread, cursor,
+                                                       evsel, sample,
+						       parent, root_al,
+						       max_stack);
+		if (ret)
+			return ret;
+                ret = thread__resolve_callchain_unwind(thread, cursor,
+                                                       evsel, sample,
+						       max_stack);
+	} else {
+                ret = thread__resolve_callchain_unwind(thread, cursor,
+                                                       evsel, sample,
+						       max_stack);
+		if (ret)
+			return ret;
+                ret = thread__resolve_callchain_sample(thread, cursor,
+                                                       evsel, sample,
+						       parent, root_al,
+						       max_stack);
+	}
 
+	return ret;
 }
 
 int machine__for_each_thread(struct machine *machine,

+Jiri since he wrote the original code

Quoted 121 lines · Adrian Hunter

Quoted 1 line · Chris Phlipot

The existing implentation implementation of thread__resolve_callchain,

Remove 'implentation'

Quoted 1 line

under certain circumstanes, can assemble callchain entries in the

'circumstanes' -> 'circumstances'

Quoted 10 lines

incorrect order.

A the callchain entries are resolved incorrectly for a sample when all of the following conditions are met:

1. callchain_param.order is set to ORDER_CALLER

2. thread__resolve_callchain_sample is able to resolve callchain entries for the sample.

Quoted 6 lines

3. unwind__get_entries is also able to resolve callchain entries for the sample.

The fix is accomplished by reversing the order in which thread__resolve_callchain_sample and unwind__get_entries are called when callchain_param.order is set to ORDER_CALLER.

Can you give an example of the commands you used and what the call chain looked like before and after.

Also please run ./scripts/checkpatch.pl

Quoted 92 lines

Unwind specific code from thread__resolve_callchain is also moved into a new static function to improve readability of the fix.

Signed-off-by Chris Phlipot <[email protected]>

---
  tools/perf/util/machine.c | 57 ++++++++++++++++++++++++++++++++++-------------
  1 file changed, 41 insertions(+), 16 deletions(-)

tools/perf/util/machine.c +41 -16

index 0c4dabc..dd086c8 100644
@@ -1806,8 +1806,6 @@ static int thread__resolve_callchain_sample(struct thread *thread,
  	int skip_idx = -1;
  	int first_call = 0;
 
-	callchain_cursor_reset(cursor);
-
  	if (has_branch_callstack(evsel)) {
  		err = resolve_lbr_callchain_sample(thread, cursor, sample, parent,
  						   root_al, max_stack);

@@ -1918,20 +1916,12 @@ static int unwind_entry(struct unwind_entry *entry, void *arg)
  				       entry->map, entry->sym);
  }
 
-int thread__resolve_callchain(struct thread *thread,
-			      struct callchain_cursor *cursor,
-			      struct perf_evsel *evsel,
-			      struct perf_sample *sample,
-			      struct symbol **parent,
-			      struct addr_location *root_al,
-			      int max_stack)
-{
-	int ret = thread__resolve_callchain_sample(thread, cursor, evsel,
-						   sample, parent,
-						   root_al, max_stack);
-	if (ret)
-		return ret;
-
+static int thread__resolve_callchain_unwind(struct thread *thread,
+                                            struct callchain_cursor *cursor,
+					    struct perf_evsel *evsel,
+					    struct perf_sample *sample,
+					    int max_stack)
+{
  	/* Can we do dwarf post unwind? */
  	if (!((evsel->attr.sample_type & PERF_SAMPLE_REGS_USER) &&
  	      (evsel->attr.sample_type & PERF_SAMPLE_STACK_USER)))

@@ -1944,7 +1934,42 @@ int thread__resolve_callchain(struct thread *thread,
 
  	return unwind__get_entries(unwind_entry, cursor,
  				   thread, sample, max_stack);
+}
+
+int thread__resolve_callchain(struct thread *thread,
+                              struct callchain_cursor *cursor,
+                              struct perf_evsel *evsel,
+                              struct perf_sample *sample,
+                              struct symbol **parent,
+                              struct addr_location *root_al,
+                              int max_stack)
+{
+	int ret = 0;
+	callchain_cursor_reset(&callchain_cursor);
+
+	if (callchain_param.order == ORDER_CALLEE) {
+                ret = thread__resolve_callchain_sample(thread, cursor,
+                                                       evsel, sample,
+						       parent, root_al,
+						       max_stack);
+		if (ret)
+			return ret;
+                ret = thread__resolve_callchain_unwind(thread, cursor,
+                                                       evsel, sample,
+						       max_stack);
+	} else {
+                ret = thread__resolve_callchain_unwind(thread, cursor,
+                                                       evsel, sample,
+						       max_stack);
+		if (ret)
+			return ret;
+                ret = thread__resolve_callchain_sample(thread, cursor,
+                                                       evsel, sample,
+						       parent, root_al,
+						       max_stack);
+	}
 
+	return ret;
  }
 
  int machine__for_each_thread(struct machine *machine,