| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 7 Jun 2023 09:50:01 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Artem Savkov <asavkov@...hat.com>
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,
Sorry for the late reply.
On Fri, Jun 2, 2023 at 1:18 AM Artem Savkov <asavkov@...hat.com> wrote:
>
> 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?
Right, annotation can be correct but c2c hist entry is not tied to an evsel
so there's no need to save the evsel for each hist entry.
>
> 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.
Sure, please do.
>
> > > };
>
> 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
Hmm.. right. I thought you should check the number of entries again but
c2c doesn't have filtering or hierarchy output so you can use
hists->nr_entries always.
But now I wonder if it's really needed as the annotate will use its own
browser so the c2c browser won't be affected.
>
> >
> > > + 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?
Yeah, I don't think we need it.
Thanks,
Namhyung
>
> > > + 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