Re: [PATCH] perf report: Provide libtraceevent with a kernel symbol resolver
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 > > > > Acked-by: Jiri Olsa > > 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
Re: [PATCH] perf report: Provide libtraceevent with a kernel symbol resolver
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 > > Acked-by: Jiri Olsa 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
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
Acked-by: Jiri Olsa
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
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
> > > > > > > ---
> > > > > > > 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
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
> > > > > > ---
> > > > > > 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
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
> > > > > ---
> > > > > 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
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
> > > > ---
> > > > 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
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
> > > ---
> > > 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
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
> > ---
> > 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
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
> ---
> 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
>
