Add a shortcut to run annotation browser for selected symbol from
c2c report TUI.
Signed-off-by: Artem Savkov <[email protected]>
---
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;
};
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':
+ 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);
+ if ((err == 'q' || err == CTRL('c')) && browser->he_selection->branch_info)
+ continue;
+ if (err)
+ ui_browser__handle_resize(&browser->b);
+
+ continue;
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);
return err;
}
--
2.40.1
Hello,
On Wed, May 31, 2023 at 4:50 AM Artem Savkov <[email protected]> wrote:
>
> Add a shortcut to run annotation browser for selected symbol from
> c2c report TUI.
>
> Signed-off-by: Artem Savkov <[email protected]>
> ---
> 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
>
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 <[email protected]> wrote:
> >
> > Add a shortcut to run annotation browser for selected symbol from
> > c2c report TUI.
> >
> > Signed-off-by: Artem Savkov <[email protected]>
> > ---
> > 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
Hello,
Sorry for the late reply.
On Fri, Jun 2, 2023 at 1:18 AM Artem Savkov <[email protected]> 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 <[email protected]> wrote:
> > >
> > > Add a shortcut to run annotation browser for selected symbol from
> > > c2c report TUI.
> > >
> > > Signed-off-by: Artem Savkov <[email protected]>
> > > ---
> > > 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
>