[PATCH] perf report: Provide libtraceevent with a kernel symbol resolver

[PATCH] perf report: Provide libtraceevent with a kernel symbol resolver

[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 <udknight@gmail.com>
---
 tools/perf/builtin-report.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index dd4df9a..7b65100 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -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);
-- 
1.8.5.6.2.g3d8a54e.dirty

Re: [PATCH] perf report: Provide libtraceevent with a kernel symbol resolver

[Jiri Olsa]

On Mon, Jan 15, 2018 at 12:47:32PM +0800, Wang YanQing wrote:
> 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 <udknight@gmail.com>
> ---
>  tools/perf/builtin-report.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index dd4df9a..7b65100 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -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;
> +	}

why not calling the wrapper trace_event__register_resolver?

jirka

> +
>  	sort__setup_elide(stdout);
>  
>  	ret = __cmd_report(&report);
> -- 
> 1.8.5.6.2.g3d8a54e.dirty
> 

Re: [PATCH] perf report: Provide libtraceevent with a kernel symbol resolver

[Wang YanQing]

On Mon, Jan 15, 2018 at 11:06:11AM +0100, Jiri Olsa wrote:
> On Mon, Jan 15, 2018 at 12:47:32PM +0800, Wang YanQing wrote:
> > 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 <udknight@gmail.com>
> > ---
> >  tools/perf/builtin-report.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > index dd4df9a..7b65100 100644
> > --- a/tools/perf/builtin-report.c
> > +++ b/tools/perf/builtin-report.c
> > @@ -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;
> > +	}
> 
> 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.
> 
> jirka
> 
> > +
> >  	sort__setup_elide(stdout);
> >  
> >  	ret = __cmd_report(&report);
> > -- 
> > 1.8.5.6.2.g3d8a54e.dirty
> > 

Re: [PATCH] perf report: Provide libtraceevent with a kernel symbol resolver

[Jiri Olsa]

On Wed, Jan 17, 2018 at 12:48:12AM +0800, Wang YanQing wrote:
> On Mon, Jan 15, 2018 at 11:06:11AM +0100, Jiri Olsa wrote:
> > On Mon, Jan 15, 2018 at 12:47:32PM +0800, Wang YanQing wrote:
> > > 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 <udknight@gmail.com>
> > > ---
> > >  tools/perf/builtin-report.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > > index dd4df9a..7b65100 100644
> > > --- a/tools/perf/builtin-report.c
> > > +++ b/tools/perf/builtin-report.c
> > > @@ -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;
> > > +	}
> > 
> > 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.

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

Re: [PATCH] perf report: Provide libtraceevent with a kernel symbol resolver

[Wang YanQing]

On Thu, Jan 18, 2018 at 10:43:18AM +0100, Jiri Olsa wrote:
> On Wed, Jan 17, 2018 at 12:48:12AM +0800, Wang YanQing wrote:
> > On Mon, Jan 15, 2018 at 11:06:11AM +0100, Jiri Olsa wrote:
> > > On Mon, Jan 15, 2018 at 12:47:32PM +0800, Wang YanQing wrote:
> > > > 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 <udknight@gmail.com>
> > > > ---
> > > >  tools/perf/builtin-report.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > > > index dd4df9a..7b65100 100644
> > > > --- a/tools/perf/builtin-report.c
> > > > +++ b/tools/perf/builtin-report.c
> > > > @@ -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;
> > > > +	}
> > > 
> > > 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.
> 
> 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.
> 
> 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

Re: [PATCH] perf report: Provide libtraceevent with a kernel symbol resolver

[Jiri Olsa]

On Thu, Jan 18, 2018 at 10:22:30PM +0800, Wang YanQing wrote:
> On Thu, Jan 18, 2018 at 10:43:18AM +0100, Jiri Olsa wrote:
> > On Wed, Jan 17, 2018 at 12:48:12AM +0800, Wang YanQing wrote:
> > > On Mon, Jan 15, 2018 at 11:06:11AM +0100, Jiri Olsa wrote:
> > > > On Mon, Jan 15, 2018 at 12:47:32PM +0800, Wang YanQing wrote:
> > > > > 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 <udknight@gmail.com>
> > > > > ---
> > > > >  tools/perf/builtin-report.c | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > > > > index dd4df9a..7b65100 100644
> > > > > --- a/tools/perf/builtin-report.c
> > > > > +++ b/tools/perf/builtin-report.c
> > > > > @@ -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;
> > > > > +	}
> > > > 
> > > > 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.
> > 
> > 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.

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

jirka

Re: [PATCH] perf report: Provide libtraceevent with a kernel symbol resolver

[Wang YanQing]

On Fri, Jan 19, 2018 at 07:57:59PM +0100, Jiri Olsa wrote:
> On Thu, Jan 18, 2018 at 10:22:30PM +0800, Wang YanQing wrote:
> > On Thu, Jan 18, 2018 at 10:43:18AM +0100, Jiri Olsa wrote:
> > > On Wed, Jan 17, 2018 at 12:48:12AM +0800, Wang YanQing wrote:
> > > > On Mon, Jan 15, 2018 at 11:06:11AM +0100, Jiri Olsa wrote:
> > > > > On Mon, Jan 15, 2018 at 12:47:32PM +0800, Wang YanQing wrote:
> > > > > > 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 <udknight@gmail.com>
> > > > > > ---
> > > > > >  tools/perf/builtin-report.c | 9 +++++++++
> > > > > >  1 file changed, 9 insertions(+)
> > > > > > 
> > > > > > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > > > > > index dd4df9a..7b65100 100644
> > > > > > --- a/tools/perf/builtin-report.c
> > > > > > +++ b/tools/perf/builtin-report.c
> > > > > > @@ -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;
> > > > > > +	}
> > > > > 
> > > > > 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.
> > > 
> > > 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.
> 
> 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.
> jirka

Re: [PATCH] perf report: Provide libtraceevent with a kernel symbol resolver

[Jiri Olsa]

On Mon, Jan 22, 2018 at 01:25:03AM +0800, Wang YanQing wrote:
> On Fri, Jan 19, 2018 at 07:57:59PM +0100, Jiri Olsa wrote:
> > On Thu, Jan 18, 2018 at 10:22:30PM +0800, Wang YanQing wrote:
> > > On Thu, Jan 18, 2018 at 10:43:18AM +0100, Jiri Olsa wrote:
> > > > On Wed, Jan 17, 2018 at 12:48:12AM +0800, Wang YanQing wrote:
> > > > > On Mon, Jan 15, 2018 at 11:06:11AM +0100, Jiri Olsa wrote:
> > > > > > On Mon, Jan 15, 2018 at 12:47:32PM +0800, Wang YanQing wrote:
> > > > > > > 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 <udknight@gmail.com>
> > > > > > > ---
> > > > > > >  tools/perf/builtin-report.c | 9 +++++++++
> > > > > > >  1 file changed, 9 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > > > > > > index dd4df9a..7b65100 100644
> > > > > > > --- a/tools/perf/builtin-report.c
> > > > > > > +++ b/tools/perf/builtin-report.c
> > > > > > > @@ -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;
> > > > > > > +	}
> > > > > > 
> > > > > > 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.
> > > > 
> > > > 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.
> > 
> > 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.

ok

jirka

Re: [PATCH] perf report: Provide libtraceevent with a kernel symbol resolver

[Jiri Olsa]

On Mon, Jan 15, 2018 at 12:47:32PM +0800, Wang YanQing wrote:
> 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 <udknight@gmail.com>

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

thanks,
jirka

> ---
>  tools/perf/builtin-report.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index dd4df9a..7b65100 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -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);
> -- 
> 1.8.5.6.2.g3d8a54e.dirty
> 

Re: [PATCH] perf report: Provide libtraceevent with a kernel symbol resolver

[Wang YanQing]

On Thu, Feb 08, 2018 at 01:20:31PM +0100, Jiri Olsa wrote:
> On Mon, Jan 15, 2018 at 12:47:32PM +0800, Wang YanQing wrote:
> > 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 <udknight@gmail.com>
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Hi! Arnaldo Carvalho de Melo

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

Thanks!

Re: [PATCH] perf report: Provide libtraceevent with a kernel symbol resolver

[Arnaldo Carvalho de Melo]

Em Tue, Mar 06, 2018 at 03:36:58PM +0800, Wang YanQing escreveu:
> On Thu, Feb 08, 2018 at 01:20:31PM +0100, Jiri Olsa wrote:
> > On Mon, Jan 15, 2018 at 12:47:32PM +0800, Wang YanQing wrote:
> > > 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 <udknight@gmail.com>
> > 
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> 
> 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