Messages in this thread | | | From | Namhyung Kim <> | Date | Thu, 1 Jun 2023 14:26:48 -0700 | Subject | Re: [PATCH 2/2] perf tools: allow running annotation browser from c2c-report |
| |
Hello,
On Wed, May 31, 2023 at 4:50 AM Artem Savkov <asavkov@redhat.com> wrote: > > Add a shortcut to run annotation browser for selected symbol from > c2c report TUI. > > Signed-off-by: Artem Savkov <asavkov@redhat.com> > --- > tools/perf/builtin-c2c.c | 76 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 71 insertions(+), 5 deletions(-) > > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c > index 05dfd98af170b..96e66289c2576 100644 > --- a/tools/perf/builtin-c2c.c > +++ b/tools/perf/builtin-c2c.c > @@ -19,11 +19,13 @@ > #include <linux/zalloc.h> > #include <asm/bug.h> > #include <sys/param.h> > +#include <sys/ttydefaults.h> > #include "debug.h" > #include "builtin.h" > #include <perf/cpumap.h> > #include <subcmd/pager.h> > #include <subcmd/parse-options.h> > +#include "map.h" > #include "map_symbol.h" > #include "mem-events.h" > #include "session.h" > @@ -43,6 +45,8 @@ > #include "ui/progress.h" > #include "pmus.h" > #include "string2.h" > +#include "util/annotate.h" > +#include "util/dso.h" > #include "util/util.h" > > struct c2c_hists { > @@ -79,6 +83,7 @@ struct c2c_hist_entry { > * because of its callchain dynamic entry > */ > struct hist_entry he; > + struct evsel *evsel;
I'm not sure if it's needed. It seems c2c command doesn't collect samples per evsel. It uses c2c.hists.hists for all evsels. Then it might not be worth keeping an evsel in a c2c_hist_entry and just use a random evsel in the evlist.
> }; > > static char const *coalesce_default = "iaddr"; > @@ -111,6 +116,8 @@ struct perf_c2c { > char *cl_sort; > char *cl_resort; > char *cl_output; > + > + struct annotation_options annotation_opts; > }; > > enum { > @@ -326,7 +333,12 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused, > c2c_he__set_cpu(c2c_he, sample); > c2c_he__set_node(c2c_he, sample); > > + c2c_he->evsel = evsel; > + > hists__inc_nr_samples(&c2c_hists->hists, he->filtered); > + if (use_browser == 1 && al.map != NULL && al.sym != NULL) { > + hist_entry__inc_addr_samples(he, sample, c2c_he->evsel, al.addr); > + } > ret = hist_entry__append_callchain(he, sample); > > if (!ret) { > @@ -363,7 +375,12 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused, > c2c_he__set_cpu(c2c_he, sample); > c2c_he__set_node(c2c_he, sample); > > + c2c_he->evsel = evsel; > + > hists__inc_nr_samples(&c2c_hists->hists, he->filtered); > + if (use_browser == 1 && al.map != NULL && al.sym != NULL) { > + hist_entry__inc_addr_samples(he, sample, c2c_he->evsel, al.addr); > + } > ret = hist_entry__append_callchain(he, sample); > } > > @@ -2618,9 +2635,12 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he) > struct c2c_hists *c2c_hists; > struct c2c_cacheline_browser *cl_browser; > struct hist_browser *browser; > + struct map_symbol ms = { NULL, NULL, NULL }; > + int err = 0; > int key = -1; > static const char help[] = > " ENTER Toggle callchains (if present) \n" > + " a Annotate current symbol\n" > " n Toggle Node details info \n" > " s Toggle full length of symbol and source line columns \n" > " q Return back to cacheline list \n"; > @@ -2650,6 +2670,44 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he) > key = hist_browser__run(browser, "? - help", true, 0); > > switch (key) { > + case 'a':
I think it's better to factor this code out to a function.
> + if (!browser->selection || > + !browser->selection->map || > + !browser->selection->map->dso || > + browser->selection->map->dso->annotate_warned) { > + continue; > + } > + > + ms.map = browser->selection->map; > + > + if (!browser->selection->sym) { > + if (!browser->he_selection) > + continue; > + > + ms.sym = symbol__new_unresolved(browser->he_selection->ip, > + browser->selection->map); > + > + if (!ms.sym) > + continue; > + } else { > + if (symbol__annotation(browser->selection->sym)->src == NULL) { > + ui_browser__warning(&browser->b, 0, > + "No samples for the \"%s\" symbol.\n\n", > + browser->selection->sym->name); > + continue; > + } > + ms.sym = browser->selection->sym; > + } > + > + err = map_symbol__tui_annotate(&ms, c2c_he->evsel, browser->hbt, &c2c.annotation_opts); > + > + ui_browser__update_nr_entries(&browser->b, browser->hists->nr_entries);
c2c_browser__update_nr_entries() ?
> + if ((err == 'q' || err == CTRL('c')) && browser->he_selection->branch_info)
Why check branch_info?
> + continue; > + if (err) > + ui_browser__handle_resize(&browser->b); > + > + continue;
It'd be natural to use 'break' instead of 'continue' here.
> case 's': > c2c.symbol_full = !c2c.symbol_full; > break; > @@ -3045,6 +3103,10 @@ static int perf_c2c__report(int argc, const char **argv) > int err = 0; > const char *output_str, *sort_str = NULL; > > + annotation_options__init(&c2c.annotation_opts); > + > + hists__init(); > + > argc = parse_options(argc, argv, options, report_c2c_usage, > PARSE_OPT_STOP_AT_NON_OPTION); > if (argc) > @@ -3118,6 +3180,14 @@ static int perf_c2c__report(int argc, const char **argv) > if (err) > goto out_mem2node; > > + if (c2c.use_stdio) { > + use_browser = 0; > + } else { > + use_browser = 1; > + symbol__annotation_init(); > + annotation_config__init(&c2c.annotation_opts); > + } > + > if (symbol__init(&session->header.env) < 0) > goto out_mem2node; > > @@ -3127,11 +3197,6 @@ static int perf_c2c__report(int argc, const char **argv) > goto out_mem2node; > } > > - if (c2c.use_stdio) > - use_browser = 0; > - else > - use_browser = 1; > - > setup_browser(false); > > err = perf_session__process_events(session); > @@ -3202,6 +3267,7 @@ static int perf_c2c__report(int argc, const char **argv) > out_session: > perf_session__delete(session); > out: > + annotation_options__init(&c2c.annotation_opts);
__exit() ?
Thanks, Namhyung
> return err; > } > > -- > 2.40.1 >
| |