perf report: Provide libtraceevent with a kernel symbol resolver

PATCH Superseded Wang YanQing 3 participants 11 messages Jan 15, 2018 – Mar 6, 2018 perf events

Share & tools

Wang YanQing

author

2018-01-15 04:47 UTC #0 lore raw

PATCH 1/1 perf report: Provide libtraceevent with a kernel symbol resolver

So that beautifiers wanting to resolve kernel function addresses to names can do its work, and when we use "perf report" for output of "perf kmem record", we will get kernel symbol output.

Signed-off-by Wang YanQing <[email protected]>

---
 tools/perf/builtin-report.c | 9 +++++++++
 1 file changed, 9 insertions(+)

tools/perf/builtin-report.c +9 -0

index dd4df9a..7b65100 100644
@@ -1317,6 +1317,15 @@ int cmd_report(int argc, const char **argv)
 		report.range_num = 1;
 	}
 
+	if (session->tevent.pevent &&
+	    pevent_set_function_resolver(session->tevent.pevent,
+					 machine__resolve_kernel_addr,
+					 &session->machines.host) < 0) {
+		pr_err("%s: failed to set libtraceevent function resolver\n",
+		       __func__);
+		return -1;
+	}
+
 	sort__setup_elide(stdout);
 
 	ret = __cmd_report(&report);

Signature · 1 line

1.8.5.6.2.g3d8a54e.dirty

Jiri Olsa

2018-01-15 10:06 UTC #1 lore raw

Quoted 15 lines · Wang YanQing

So that beautifiers wanting to resolve kernel function addresses to names can do its work, and when we use "perf report" for output of "perf kmem record", we will get kernel symbol output.
Signed-off-by Wang YanQing <[email protected]>
---
 tools/perf/builtin-report.c | 9 +++++++++
 1 file changed, 9 insertions(+)
tools/perf/builtin-report.c +0 -0
index dd4df9a..7b65100 100644
@@ -1317,6 +1317,15 @@ int cmd_report(int argc, const char **argv)
 		report.range_num = 1;

tools/perf/builtin-report.c int cmd_report(int argc, const char **argv) · line 1327

Quoted 10 lines

tools/perf/builtin-report.c +8 -0

 	}
 
+	if (session->tevent.pevent &&
+	    pevent_set_function_resolver(session->tevent.pevent,
+					 machine__resolve_kernel_addr,
+					 &session->machines.host) < 0) {
+		pr_err("%s: failed to set libtraceevent function resolver\n",
+		       __func__);
+		return -1;
+	}

why not calling the wrapper trace_event__register_resolver?

jirka

Quoted 7 lines

tools/perf/builtin-report.c +1 -0
+
 	sort__setup_elide(stdout);
 
 	ret = __cmd_report(&report);
Signature · 1 line
1.8.5.6.2.g3d8a54e.dirty

Wang YanQing

author

2018-01-16 17:02 UTC #2 lore raw

Quoted 18 lines · Jiri Olsa

Quoted 17 lines · Wang YanQing
So that beautifiers wanting to resolve kernel function addresses to names can do its work, and when we use "perf report" for output of "perf kmem record", we will get kernel symbol output.
Signed-off-by Wang YanQing <[email protected]>
---
 tools/perf/builtin-report.c | 9 +++++++++
 1 file changed, 9 insertions(+)
tools/perf/builtin-report.c +0 -0
index dd4df9a..7b65100 100644
@@ -1317,6 +1317,15 @@ int cmd_report(int argc, const char **argv)
 		report.range_num = 1;
 	}
 

Quoted 10 lines

tools/perf/builtin-report.c int cmd_report(int argc, const char **argv) · line 1327
Quoted 8 lines
tools/perf/builtin-report.c +8 -0
+	if (session->tevent.pevent &&
+	    pevent_set_function_resolver(session->tevent.pevent,
+					 machine__resolve_kernel_addr,
+					 &session->machines.host) < 0) {
+		pr_err("%s: failed to set libtraceevent function resolver\n",
+		       __func__);
+		return -1;
+	}
why not calling the wrapper trace_event__register_resolver?

The reason is the same as builtin-script doesn't use trace_event__register_resolver, because we only use report and script to analyze offline perf.data, and there could be no tracepoints in perf.data.

The wrapper trace_event__register_resolver always initialize tevent.pevent, and calling pevent_set_function_resolver which isn't necessary when there is no tracepoints in the perf.data.

Quoted 10 lines

jirka
Quoted 7 lines
tools/perf/builtin-report.c +1 -0
+
 	sort__setup_elide(stdout);
 
 	ret = __cmd_report(&report);
Signature · 1 line
1.8.5.6.2.g3d8a54e.dirty

Jiri Olsa

2018-01-18 09:43 UTC #3 lore raw

Quoted 29 lines · Wang YanQing

Quoted 18 lines · Jiri Olsa
Quoted 17 lines · Wang YanQing
So that beautifiers wanting to resolve kernel function addresses to names can do its work, and when we use "perf report" for output of "perf kmem record", we will get kernel symbol output.
Signed-off-by Wang YanQing <[email protected]>
---
 tools/perf/builtin-report.c | 9 +++++++++
 1 file changed, 9 insertions(+)
tools/perf/builtin-report.c +0 -0
index dd4df9a..7b65100 100644
@@ -1317,6 +1317,15 @@ int cmd_report(int argc, const char **argv)
 		report.range_num = 1;
 	}
 
Quoted 10 lines
tools/perf/builtin-report.c int cmd_report(int argc, const char **argv) · line 1327
Quoted 8 lines
tools/perf/builtin-report.c +8 -0
+	if (session->tevent.pevent &&
+	    pevent_set_function_resolver(session->tevent.pevent,
+					 machine__resolve_kernel_addr,
+					 &session->machines.host) < 0) {
+		pr_err("%s: failed to set libtraceevent function resolver\n",
+		       __func__);
+		return -1;
+	}
why not calling the wrapper trace_event__register_resolver?

Quoted 3 lines

The reason is the same as builtin-script doesn't use trace_event__register_resolver, because we only use report and script to analyze offline perf.data, and there could be no tracepoints in perf.data.

hum, I missed this functionality.. so we need this even if there are no tracepoints in the perf.data?

The only related interface function I could found is pevent_find_function and it seems to be called only from within traceevent library.

thanks,
jirka

Wang YanQing

author

2018-01-18 14:23 UTC #4 lore raw

Quoted 30 lines · Jiri Olsa

Quoted 29 lines · Wang YanQing
Quoted 28 lines · Jiri Olsa
Quoted 15 lines · Wang YanQing
So that beautifiers wanting to resolve kernel function addresses to names can do its work, and when we use "perf report" for output of "perf kmem record", we will get kernel symbol output.
Signed-off-by Wang YanQing <[email protected]>
---
 tools/perf/builtin-report.c | 9 +++++++++
 1 file changed, 9 insertions(+)
tools/perf/builtin-report.c +0 -0
index dd4df9a..7b65100 100644
@@ -1317,6 +1317,15 @@ int cmd_report(int argc, const char **argv)
 		report.range_num = 1;
tools/perf/builtin-report.c int cmd_report(int argc, const char **argv) · line 1327
Quoted 10 lines
tools/perf/builtin-report.c +8 -0
 	}
 
+	if (session->tevent.pevent &&
+	    pevent_set_function_resolver(session->tevent.pevent,
+					 machine__resolve_kernel_addr,
+					 &session->machines.host) < 0) {
+		pr_err("%s: failed to set libtraceevent function resolver\n",
+		       __func__);
+		return -1;
+	}
why not calling the wrapper trace_event__register_resolver?

Quoted 6 lines

Quoted 3 lines
The reason is the same as builtin-script doesn't use trace_event__register_resolver, because we only use report and script to analyze offline perf.data, and there could be no tracepoints in perf.data.
hum, I missed this functionality.. so we need this even if there are no tracepoints in the perf.data?

In "perf report", when there are tracepoints in perf.data, session->tevent.pevent will be initialized in trace-event-read.c:trace_report, then "if (session->tevent.pevent" will become true, and we should calling pevent_set_function_resolver.

But if we calling trace_event__register_resolver, then it will initialize tevent.pevent no matter whether there are tracepoints in perf.data.

Thanks.

Quoted 6 lines

The only related interface function I could found is pevent_find_function and it seems to be called only from within traceevent library.
thanks,
jirka

Jiri Olsa

2018-01-19 18:58 UTC #5 lore raw

Quoted 38 lines · Wang YanQing

Quoted 30 lines · Jiri Olsa
Quoted 29 lines · Wang YanQing
Quoted 28 lines · Jiri Olsa
Quoted 15 lines · Wang YanQing
So that beautifiers wanting to resolve kernel function addresses to names can do its work, and when we use "perf report" for output of "perf kmem record", we will get kernel symbol output.
Signed-off-by Wang YanQing <[email protected]>
---
 tools/perf/builtin-report.c | 9 +++++++++
 1 file changed, 9 insertions(+)
tools/perf/builtin-report.c +0 -0
index dd4df9a..7b65100 100644
@@ -1317,6 +1317,15 @@ int cmd_report(int argc, const char **argv)
 		report.range_num = 1;
tools/perf/builtin-report.c int cmd_report(int argc, const char **argv) · line 1327
Quoted 10 lines
tools/perf/builtin-report.c +8 -0
 	}
 
+	if (session->tevent.pevent &&
+	    pevent_set_function_resolver(session->tevent.pevent,
+					 machine__resolve_kernel_addr,
+					 &session->machines.host) < 0) {
+		pr_err("%s: failed to set libtraceevent function resolver\n",
+		       __func__);
+		return -1;
+	}
why not calling the wrapper trace_event__register_resolver?
Quoted 6 lines
Quoted 3 lines
The reason is the same as builtin-script doesn't use trace_event__register_resolver, because we only use report and script to analyze offline perf.data, and there could be no tracepoints in perf.data.
hum, I missed this functionality.. so we need this even if there are no tracepoints in the perf.data?

Quoted 6 lines

In "perf report", when there are tracepoints in perf.data, session->tevent.pevent will be initialized in trace-event-read.c:trace_report, then "if (session->tevent.pevent" will become true, and we should calling pevent_set_function_resolver.
But if we calling trace_event__register_resolver, then it will initialize tevent.pevent no matter whether there are tracepoints in perf.data.

ok, should we call it from perf_session__read_header then? below perf_evlist__prepare_tracepoint_events perhaps

jirka

Wang YanQing

author

2018-01-21 17:41 UTC #6 lore raw

Quoted 43 lines · Jiri Olsa

Quoted 42 lines · Wang YanQing
Quoted 30 lines · Jiri Olsa
Quoted 29 lines · Wang YanQing
Quoted 28 lines · Jiri Olsa
Quoted 15 lines · Wang YanQing
So that beautifiers wanting to resolve kernel function addresses to names can do its work, and when we use "perf report" for output of "perf kmem record", we will get kernel symbol output.
Signed-off-by Wang YanQing <[email protected]>
---
 tools/perf/builtin-report.c | 9 +++++++++
 1 file changed, 9 insertions(+)
tools/perf/builtin-report.c +0 -0
index dd4df9a..7b65100 100644
@@ -1317,6 +1317,15 @@ int cmd_report(int argc, const char **argv)
 		report.range_num = 1;
tools/perf/builtin-report.c int cmd_report(int argc, const char **argv) · line 1327
Quoted 10 lines
tools/perf/builtin-report.c +8 -0
 	}
 
+	if (session->tevent.pevent &&
+	    pevent_set_function_resolver(session->tevent.pevent,
+					 machine__resolve_kernel_addr,
+					 &session->machines.host) < 0) {
+		pr_err("%s: failed to set libtraceevent function resolver\n",
+		       __func__);
+		return -1;
+	}
why not calling the wrapper trace_event__register_resolver?
Quoted 6 lines
Quoted 3 lines
The reason is the same as builtin-script doesn't use trace_event__register_resolver, because we only use report and script to analyze offline perf.data, and there could be no tracepoints in perf.data.
hum, I missed this functionality.. so we need this even if there are no tracepoints in the perf.data?
In "perf report", when there are tracepoints in perf.data, session->tevent.pevent will be initialized in trace-event-read.c:trace_report, then "if (session->tevent.pevent" will become true, and we should calling pevent_set_function_resolver.

Quoted 5 lines

Quoted 2 lines
But if we calling trace_event__register_resolver, then it will initialize tevent.pevent no matter whether there are tracepoints in perf.data.
ok, should we call it from perf_session__read_header then? below perf_evlist__prepare_tracepoint_events perhaps

No, I think we can't do it in perf_session__read_header, because perf_session__new calls perf_session__create_kernel_maps to initialize machines.host after calling perf_session__open, and pevent_set_function_resolver needs it.

Although it is possible to place pevent_set_function_resolver at the tail of perf_session__new, but I don't think it is a better choice for us, because tools like "perf kmem stat" doesn't need it, "perf kmem stat" has itself way to prepare and display the symbols of callsites.

I think the current calling place of pevent_set_function_resolver for "perf report" is the right place.

Thanks.

Quoted 1 line

jirka

Jiri Olsa

2018-01-23 12:41 UTC #7 lore raw

Quoted 55 lines · Wang YanQing

Quoted 43 lines · Jiri Olsa
Quoted 42 lines · Wang YanQing
Quoted 30 lines · Jiri Olsa
Quoted 29 lines · Wang YanQing
Quoted 28 lines · Jiri Olsa
Quoted 15 lines · Wang YanQing
So that beautifiers wanting to resolve kernel function addresses to names can do its work, and when we use "perf report" for output of "perf kmem record", we will get kernel symbol output.
Signed-off-by Wang YanQing <[email protected]>
---
 tools/perf/builtin-report.c | 9 +++++++++
 1 file changed, 9 insertions(+)
tools/perf/builtin-report.c +0 -0
index dd4df9a..7b65100 100644
@@ -1317,6 +1317,15 @@ int cmd_report(int argc, const char **argv)
 		report.range_num = 1;
tools/perf/builtin-report.c int cmd_report(int argc, const char **argv) · line 1327
Quoted 10 lines
tools/perf/builtin-report.c +8 -0
 	}
 
+	if (session->tevent.pevent &&
+	    pevent_set_function_resolver(session->tevent.pevent,
+					 machine__resolve_kernel_addr,
+					 &session->machines.host) < 0) {
+		pr_err("%s: failed to set libtraceevent function resolver\n",
+		       __func__);
+		return -1;
+	}
why not calling the wrapper trace_event__register_resolver?
Quoted 6 lines
Quoted 3 lines
The reason is the same as builtin-script doesn't use trace_event__register_resolver, because we only use report and script to analyze offline perf.data, and there could be no tracepoints in perf.data.
hum, I missed this functionality.. so we need this even if there are no tracepoints in the perf.data?
In "perf report", when there are tracepoints in perf.data, session->tevent.pevent will be initialized in trace-event-read.c:trace_report, then "if (session->tevent.pevent" will become true, and we should calling pevent_set_function_resolver.
Quoted 5 lines
Quoted 2 lines
But if we calling trace_event__register_resolver, then it will initialize tevent.pevent no matter whether there are tracepoints in perf.data.
ok, should we call it from perf_session__read_header then? below perf_evlist__prepare_tracepoint_events perhaps
No, I think we can't do it in perf_session__read_header, because perf_session__new calls perf_session__create_kernel_maps to initialize machines.host after calling perf_session__open, and pevent_set_function_resolver needs it.

Quoted 8 lines

Although it is possible to place pevent_set_function_resolver at the tail of perf_session__new, but I don't think it is a better choice for us, because tools like "perf kmem stat" doesn't need it, "perf kmem stat" has itself way to prepare and display the symbols of callsites.
I think the current calling place of pevent_set_function_resolver for "perf report" is the right place.

jirka

Jiri Olsa

Acked-by on v1 1/1

2018-02-08 12:20 UTC #8 lore raw

In reply to Wang YanQing

Quoted 5 lines · Wang YanQing

So that beautifiers wanting to resolve kernel function addresses to names can do its work, and when we use "perf report" for output of "perf kmem record", we will get kernel symbol output.
Signed-off-by Wang YanQing <[email protected]>

Acked-by Jiri Olsa <[email protected]>

thanks,
jirka

Quoted 27 lines

---
 tools/perf/builtin-report.c | 9 +++++++++
 1 file changed, 9 insertions(+)

tools/perf/builtin-report.c +9 -0

index dd4df9a..7b65100 100644
@@ -1317,6 +1317,15 @@ int cmd_report(int argc, const char **argv)
 		report.range_num = 1;
 	}
 
+	if (session->tevent.pevent &&
+	    pevent_set_function_resolver(session->tevent.pevent,
+					 machine__resolve_kernel_addr,
+					 &session->machines.host) < 0) {
+		pr_err("%s: failed to set libtraceevent function resolver\n",
+		       __func__);
+		return -1;
+	}
+
 	sort__setup_elide(stdout);
 
 	ret = __cmd_report(&report);

Signature · 1 line

1.8.5.6.2.g3d8a54e.dirty

Wang YanQing

author

2018-03-06 07:52 UTC #9 lore raw

Quoted 8 lines · Jiri Olsa

Quoted 5 lines · Wang YanQing
So that beautifiers wanting to resolve kernel function addresses to names can do its work, and when we use "perf report" for output of "perf kmem record", we will get kernel symbol output.
Signed-off-by Wang YanQing <[email protected]>
Acked-by Jiri Olsa <[email protected]>

Hi! Arnaldo Carvalho de Melo

What is the status of this patch now?
Does the patch sanked to the bottom of your mailbox?

Thanks!

Arnaldo Carvalho de Melo

maintainer

2018-03-06 14:01 UTC #10 lore raw

Em Tue, Mar 06, 2018 at 03:36:58PM +0800, Wang YanQing escreveu:

Quoted 6 lines · Jiri Olsa

Quoted 5 lines · Wang YanQing
Quoted 4 lines
So that beautifiers wanting to resolve kernel function addresses to names can do its work, and when we use "perf report" for output of "perf kmem record", we will get kernel symbol output.

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
Signed-off-by Wang YanQing <[email protected]>
Acked-by Jiri Olsa <[email protected]>
Hi! Arnaldo Carvalho de Melo
What is the status of this patch now?
Does the patch sanked to the bottom of your mailbox?

It looks ok, but you forgot to add a before and after output of tools affected by the patch.

Reading the patch one can try and figure out how to test your patch, but if you show how the output is changed, providing the exact command line used to produce the output, then you will make it easier for reviewers to reproduce your results, testing your patch.

I try to do this for every patch, so it all adds up, please help me with reviewing your patch :-)

- Arnaldo