Message-ID: <20230602081811.GA240769@samus.usersys.redhat.com>
Date:   Fri, 2 Jun 2023 10:18:11 +0200
From:   Artem Savkov <asavkov@...hat.com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     linux-perf-users@...r.kernel.org,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] perf tools: allow running annotation browser from
 c2c-report

Hello,

Thank you for the review.

On Thu, Jun 01, 2023 at 02:26:48PM -0700, Namhyung Kim wrote:
> Hello,
> 
> On Wed, May 31, 2023 at 4:50 AM Artem Savkov <asavkov@...hat.com> wrote:
> >
> > Add a shortcut to run annotation browser for selected symbol from
> > c2c report TUI.
> >
> > Signed-off-by: Artem Savkov <asavkov@...hat.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 @@

snip

> >  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.
> 

Right, but annotation browser does use it for line usage percentage
calculation. So does this mean it won't be showing correct percentages
whatever evsel is chosen and that's why it is possible to just select a
random one?

As far as I can tell evlist is not currently available in
perf_c2c__browse_cacheline(), but it can be added to struct perf_c2c.

> >  };

snip

> > +       " 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.

Ok, will do.

> > +                       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() ?

Will it change from the previous call before the while loop? This part
was mostly copied over from do_annotate() in ui/browsers/hists.c so I am
a bit hazy here. My understanding is that update_nr_entries and handle
resize are called here mostly for refresh/reset of the ui and
recalculation of number of entries is not needed

> 
> > +                       if ((err == 'q' || err == CTRL('c')) && browser->he_selection->branch_info)
> 
> Why check branch_info?

This was copied over as well and the comment in hists.c states "offer
option to annotate the other branch source or target (if they exists)
when returning from annotate". So I now think this might not be needed
here at all?

> > +                                       continue;
> > +                       if (err)
> > +                               ui_browser__handle_resize(&browser->b);
> > +
> > +                       continue;
> 
> It'd be natural to use 'break' instead of 'continue' here.

Yes, will change this.

> >                 case 's':
> >                         c2c.symbol_full = !c2c.symbol_full;

snip

> >         perf_session__delete(session);
> >  out:
> > +       annotation_options__init(&c2c.annotation_opts);
> 
> __exit() ?

Ouch, thanks for noticing!

-- 
 Artem

Powered by blists - more mailing lists