* [PATCH 0/2] perf tools: annotation browser from c2c tui
@ 2023-05-31 11:50 Artem Savkov
2023-05-31 11:50 ` [PATCH 1/2] perf util: move symbol__new_unresolved() to util/symbol.c Artem Savkov
2023-05-31 11:50 ` [PATCH 2/2] perf tools: allow running annotation browser from c2c-report Artem Savkov
0 siblings, 2 replies; 6+ messages in thread
From: Artem Savkov @ 2023-05-31 11:50 UTC (permalink / raw)
To: linux-perf-users, Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter, linux-kernel,
Artem Savkov
These patches add ability to start annotation browser from c2c report
tui. The idea comes from Arnaldo's "Profiling Data Structures" talk [1].
[1]: http://vger.kernel.org/~acme/prez/linux-plumbers-2022/
Artem Savkov (2):
perf util: move symbol__new_unresolved() to util/symbol.c
perf tools: allow running annotation browser from c2c-report
tools/perf/builtin-c2c.c | 76 +++++++++++++++++++++++++++++++---
tools/perf/ui/browsers/hists.c | 22 ----------
tools/perf/util/symbol.c | 22 ++++++++++
tools/perf/util/symbol.h | 1 +
4 files changed, 94 insertions(+), 27 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] perf util: move symbol__new_unresolved() to util/symbol.c
2023-05-31 11:50 [PATCH 0/2] perf tools: annotation browser from c2c tui Artem Savkov
@ 2023-05-31 11:50 ` Artem Savkov
2023-05-31 11:50 ` [PATCH 2/2] perf tools: allow running annotation browser from c2c-report Artem Savkov
1 sibling, 0 replies; 6+ messages in thread
From: Artem Savkov @ 2023-05-31 11:50 UTC (permalink / raw)
To: linux-perf-users, Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter, linux-kernel,
Artem Savkov
Make symbol__new_unresolved() available through util/symbol.h
so it can be later used from builtin-c2c.
Signed-off-by: Artem Savkov <asavkov@redhat.com>
---
tools/perf/ui/browsers/hists.c | 22 ----------------------
tools/perf/util/symbol.c | 22 ++++++++++++++++++++++
tools/perf/util/symbol.h | 1 +
3 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 69c81759a64f9..10d2243d27504 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2471,28 +2471,6 @@ do_annotate(struct hist_browser *browser, struct popup_action *act)
return 0;
}
-static struct symbol *symbol__new_unresolved(u64 addr, struct map *map)
-{
- struct annotated_source *src;
- struct symbol *sym;
- char name[64];
-
- snprintf(name, sizeof(name), "%.*" PRIx64, BITS_PER_LONG / 4, addr);
-
- sym = symbol__new(addr, ANNOTATION_DUMMY_LEN, 0, 0, name);
- if (sym) {
- src = symbol__hists(sym, 1);
- if (!src) {
- symbol__delete(sym);
- return NULL;
- }
-
- dso__insert_symbol(map__dso(map), sym);
- }
-
- return sym;
-}
-
static int
add_annotate_opt(struct hist_browser *browser __maybe_unused,
struct popup_action *act, char **optstr,
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 6b9c55784b56a..297a903f72777 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -294,6 +294,28 @@ void maps__fixup_end(struct maps *maps)
up_write(maps__lock(maps));
}
+struct symbol *symbol__new_unresolved(u64 addr, struct map *map)
+{
+ struct annotated_source *src;
+ struct symbol *sym;
+ char name[64];
+
+ snprintf(name, sizeof(name), "%.*" PRIx64, BITS_PER_LONG / 4, addr);
+
+ sym = symbol__new(addr, ANNOTATION_DUMMY_LEN, 0, 0, name);
+ if (sym) {
+ src = symbol__hists(sym, 1);
+ if (!src) {
+ symbol__delete(sym);
+ return NULL;
+ }
+
+ dso__insert_symbol(map__dso(map), sym);
+ }
+
+ return sym;
+}
+
struct symbol *symbol__new(u64 start, u64 len, u8 binding, u8 type, const char *name)
{
size_t namelen = strlen(name) + 1;
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 7558735543c25..4d47748716627 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -176,6 +176,7 @@ void symbol__exit(void);
void symbol__elf_init(void);
int symbol__annotation_init(void);
+struct symbol *symbol__new_unresolved(u64 addr, struct map *map);
struct symbol *symbol__new(u64 start, u64 len, u8 binding, u8 type, const char *name);
size_t __symbol__fprintf_symname_offs(const struct symbol *sym,
const struct addr_location *al,
--
2.40.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] perf tools: allow running annotation browser from c2c-report
2023-05-31 11:50 [PATCH 0/2] perf tools: annotation browser from c2c tui Artem Savkov
2023-05-31 11:50 ` [PATCH 1/2] perf util: move symbol__new_unresolved() to util/symbol.c Artem Savkov
@ 2023-05-31 11:50 ` Artem Savkov
2023-06-01 21:26 ` Namhyung Kim
1 sibling, 1 reply; 6+ messages in thread
From: Artem Savkov @ 2023-05-31 11:50 UTC (permalink / raw)
To: linux-perf-users, Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter, linux-kernel,
Artem Savkov
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;
};
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] perf tools: allow running annotation browser from c2c-report
2023-05-31 11:50 ` [PATCH 2/2] perf tools: allow running annotation browser from c2c-report Artem Savkov
@ 2023-06-01 21:26 ` Namhyung Kim
2023-06-02 8:18 ` Artem Savkov
0 siblings, 1 reply; 6+ messages in thread
From: Namhyung Kim @ 2023-06-01 21:26 UTC (permalink / raw)
To: Artem Savkov
Cc: linux-perf-users, Arnaldo Carvalho de Melo, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, linux-kernel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] perf tools: allow running annotation browser from c2c-report
2023-06-01 21:26 ` Namhyung Kim
@ 2023-06-02 8:18 ` Artem Savkov
2023-06-07 16:50 ` Namhyung Kim
0 siblings, 1 reply; 6+ messages in thread
From: Artem Savkov @ 2023-06-02 8:18 UTC (permalink / raw)
To: Namhyung Kim
Cc: linux-perf-users, Arnaldo Carvalho de Melo, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, linux-kernel
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@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 @@
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] perf tools: allow running annotation browser from c2c-report
2023-06-02 8:18 ` Artem Savkov
@ 2023-06-07 16:50 ` Namhyung Kim
0 siblings, 0 replies; 6+ messages in thread
From: Namhyung Kim @ 2023-06-07 16:50 UTC (permalink / raw)
To: Artem Savkov
Cc: linux-perf-users, Arnaldo Carvalho de Melo, Peter Zijlstra,
Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, linux-kernel
Hello,
Sorry for the late reply.
On Fri, Jun 2, 2023 at 1:18 AM Artem Savkov <asavkov@redhat.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@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 @@
>
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-06-07 16:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-31 11:50 [PATCH 0/2] perf tools: annotation browser from c2c tui Artem Savkov
2023-05-31 11:50 ` [PATCH 1/2] perf util: move symbol__new_unresolved() to util/symbol.c Artem Savkov
2023-05-31 11:50 ` [PATCH 2/2] perf tools: allow running annotation browser from c2c-report Artem Savkov
2023-06-01 21:26 ` Namhyung Kim
2023-06-02 8:18 ` Artem Savkov
2023-06-07 16:50 ` Namhyung Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).