[PATCH v2 0/2] perf tools: annotation browser from c2c tui

[PATCH v2 0/2] perf tools: annotation browser from c2c tui

[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/

v1->v2: Addressed comments from Namhyung Kim
- No longer saving evsel for each hist entry, using evlist__first
  instead.
- Factored out preparations to call annotation browser to do_annotate()
  function
- Other small fixes and adjustments.

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       | 73 +++++++++++++++++++++++++++++++---
 tools/perf/ui/browsers/hists.c | 22 ----------
 tools/perf/util/symbol.c       | 22 ++++++++++
 tools/perf/util/symbol.h       |  1 +
 4 files changed, 91 insertions(+), 27 deletions(-)

-- 
2.40.1

[PATCH v2 1/2] perf util: move symbol__new_unresolved() to util/symbol.c

[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

[PATCH v2 2/2] perf tools: allow running annotation browser from c2c-report

[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 | 73 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 68 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 05dfd98af170b..dce8604837aec 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -24,6 +24,7 @@
 #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 +44,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 {
@@ -111,6 +114,9 @@ struct perf_c2c {
 	char			*cl_sort;
 	char			*cl_resort;
 	char			*cl_output;
+
+	struct evlist		  *evlist;
+	struct annotation_options annotation_opts;
 };
 
 enum {
@@ -327,6 +333,9 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused,
 	c2c_he__set_node(c2c_he, sample);
 
 	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, evsel, al.addr);
+
 	ret = hist_entry__append_callchain(he, sample);
 
 	if (!ret) {
@@ -364,6 +373,9 @@ static int process_sample_event(struct perf_tool *tool __maybe_unused,
 		c2c_he__set_node(c2c_he, sample);
 
 		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, evsel, al.addr);
+
 		ret = hist_entry__append_callchain(he, sample);
 	}
 
@@ -2612,6 +2624,43 @@ c2c_cacheline_browser__new(struct hists *hists, struct hist_entry *he)
 	return browser;
 }
 
+static void do_annotate(struct hist_browser *browser, struct evsel *evsel) {
+	struct map_symbol ms = { NULL, NULL, NULL };
+
+	if (!browser->selection ||
+	    !browser->selection->map ||
+	    !browser->selection->map->dso ||
+	    browser->selection->map->dso->annotate_warned) {
+		return;
+	}
+
+	ms.map = browser->selection->map;
+
+	if (!browser->selection->sym) {
+		if (!browser->he_selection)
+			return;
+
+		ms.sym = symbol__new_unresolved(browser->he_selection->ip,
+						browser->selection->map);
+
+		if (!ms.sym)
+			return;
+	} 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);
+			return;
+		}
+		ms.sym = browser->selection->sym;
+	}
+
+	map_symbol__tui_annotate(&ms, evsel, browser->hbt, &c2c.annotation_opts);
+
+	ui_browser__update_nr_entries(&browser->b, browser->hists->nr_entries);
+	ui_browser__handle_resize(&browser->b);
+}
+
 static int perf_c2c__browse_cacheline(struct hist_entry *he)
 {
 	struct c2c_hist_entry *c2c_he;
@@ -2621,6 +2670,7 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
 	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 +2700,9 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
 		key = hist_browser__run(browser, "? - help", true, 0);
 
 		switch (key) {
+		case 'a':
+			do_annotate(browser, evlist__first(c2c.evlist));
+			break;
 		case 's':
 			c2c.symbol_full = !c2c.symbol_full;
 			break;
@@ -3045,6 +3098,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 +3175,16 @@ static int perf_c2c__report(int argc, const char **argv)
 	if (err)
 		goto out_mem2node;
 
+	c2c.evlist = session->evlist;
+
+	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 +3194,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 +3264,7 @@ static int perf_c2c__report(int argc, const char **argv)
 out_session:
 	perf_session__delete(session);
 out:
+	annotation_options__exit(&c2c.annotation_opts);
 	return err;
 }
 
-- 
2.40.1

Re: [PATCH v2 0/2] perf tools: annotation browser from c2c tui

[Peter Zijlstra]

On Thu, Jun 08, 2023 at 10:44:05AM +0200, Artem Savkov wrote:
> 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/

So what are the plans for doing IP to datatype::member resolution and
deleting this C2C abomination?

/me still regretting ever letting c2c happen..

Re: [PATCH v2 0/2] perf tools: annotation browser from c2c tui

[Namhyung Kim]

Hi Peter,

On Thu, Jun 8, 2023 at 6:25 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Jun 08, 2023 at 10:44:05AM +0200, Artem Savkov wrote:
> > 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/
>
> So what are the plans for doing IP to datatype::member resolution and
> deleting this C2C abomination?

I'm working on it and hoping to post an RFC soonish.. but no plan to kill
c2c entirely :)

Thanks,
Namhyung

>
> /me still regretting ever letting c2c happen..

Re: [PATCH v2 0/2] perf tools: annotation browser from c2c tui

[Namhyung Kim]

Hello,

On Thu, Jun 8, 2023 at 1:44 AM Artem Savkov <asavkov@redhat.com> wrote:
>
> These patches add ability to start annotation browser from c2c report
> tui. The idea comes from Arnaldo's "Profiling Data Structures" talk [1].

I was thinking about how it works and realized that it didn't collect
samples by symbol.  Then I'm not sure if the result is meaningful.
I think it'd show a random symbol that touched the cache line
first.  The same cache line can be accessed from other locations
but it cannot know where they are.

Also different instructions in a function (symbol) would access a
different cache line.  The annotate output just shows any memory
access.  So it might be good to check the instruction at the point
but others should not be considered related.

Hmm.. I suspect even the same instruction will hit the different
cache lines at different times.  Then probably the annotation
won't work well in terms of correlating cache lines.

Thanks,
Namhyung

>
> [1]: http://vger.kernel.org/~acme/prez/linux-plumbers-2022/
>
> v1->v2: Addressed comments from Namhyung Kim
> - No longer saving evsel for each hist entry, using evlist__first
>   instead.
> - Factored out preparations to call annotation browser to do_annotate()
>   function
> - Other small fixes and adjustments.
>
> 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       | 73 +++++++++++++++++++++++++++++++---
>  tools/perf/ui/browsers/hists.c | 22 ----------
>  tools/perf/util/symbol.c       | 22 ++++++++++
>  tools/perf/util/symbol.h       |  1 +
>  4 files changed, 91 insertions(+), 27 deletions(-)
>
> --
> 2.40.1
>

Re: [PATCH v2 0/2] perf tools: annotation browser from c2c tui

[Artem Savkov]

On Thu, Jun 08, 2023 at 02:09:06PM -0700, Namhyung Kim wrote:
> Hello,
> 
> On Thu, Jun 8, 2023 at 1:44 AM Artem Savkov <asavkov@redhat.com> wrote:
> >
> > These patches add ability to start annotation browser from c2c report
> > tui. The idea comes from Arnaldo's "Profiling Data Structures" talk [1].
> 
> I was thinking about how it works and realized that it didn't collect
> samples by symbol.  Then I'm not sure if the result is meaningful.
> I think it'd show a random symbol that touched the cache line
> first.  The same cache line can be accessed from other locations
> but it cannot know where they are.
> 
> Also different instructions in a function (symbol) would access a
> different cache line.  The annotate output just shows any memory
> access.  So it might be good to check the instruction at the point
> but others should not be considered related.
> 
> Hmm.. I suspect even the same instruction will hit the different
> cache lines at different times.  Then probably the annotation
> won't work well in terms of correlating cache lines.

The annotation hotkey is only added to the cacheline detailed view where
we do have symbol instruction information. The idea is to give the user
ability to quickly jump to source code/disassembly directly from c2c
TUI.

The hit percentages in annotation view don't make much sense in this
case though, so maybe it is better to use dummy evsel so that none are
shown.

> Thanks,
> Namhyung
> 
> >
> > [1]: http://vger.kernel.org/~acme/prez/linux-plumbers-2022/
> >
> > v1->v2: Addressed comments from Namhyung Kim
> > - No longer saving evsel for each hist entry, using evlist__first
> >   instead.
> > - Factored out preparations to call annotation browser to do_annotate()
> >   function
> > - Other small fixes and adjustments.
> >
> > 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       | 73 +++++++++++++++++++++++++++++++---
> >  tools/perf/ui/browsers/hists.c | 22 ----------
> >  tools/perf/util/symbol.c       | 22 ++++++++++
> >  tools/perf/util/symbol.h       |  1 +
> >  4 files changed, 91 insertions(+), 27 deletions(-)
> >
> > --
> > 2.40.1
> >
> 

-- 
 Artem

Re: [PATCH v2 0/2] perf tools: annotation browser from c2c tui

[Arnaldo Carvalho de Melo]

Em Fri, Jun 09, 2023 at 10:37:31AM +0200, Artem Savkov escreveu:
> On Thu, Jun 08, 2023 at 02:09:06PM -0700, Namhyung Kim wrote:
> > Hello,
> > 
> > On Thu, Jun 8, 2023 at 1:44 AM Artem Savkov <asavkov@redhat.com> wrote:
> > >
> > > These patches add ability to start annotation browser from c2c report
> > > tui. The idea comes from Arnaldo's "Profiling Data Structures" talk [1].
> > 
> > I was thinking about how it works and realized that it didn't collect
> > samples by symbol.  Then I'm not sure if the result is meaningful.
> > I think it'd show a random symbol that touched the cache line
> > first.  The same cache line can be accessed from other locations
> > but it cannot know where they are.
> > 
> > Also different instructions in a function (symbol) would access a
> > different cache line.  The annotate output just shows any memory
> > access.  So it might be good to check the instruction at the point
> > but others should not be considered related.
> > 
> > Hmm.. I suspect even the same instruction will hit the different
> > cache lines at different times.  Then probably the annotation
> > won't work well in terms of correlating cache lines.
> 
> The annotation hotkey is only added to the cacheline detailed view where
> we do have symbol instruction information. The idea is to give the user
> ability to quickly jump to source code/disassembly directly from c2c
> TUI.
> 
> The hit percentages in annotation view don't make much sense in this
> case though, so maybe it is better to use dummy evsel so that none are
> shown.

Yes, the point is just to reuse the source browser, if there is no
annotation data applicable, don't use any.

- Arnaldo
 
> > Thanks,
> > Namhyung
> > 
> > >
> > > [1]: http://vger.kernel.org/~acme/prez/linux-plumbers-2022/
> > >
> > > v1->v2: Addressed comments from Namhyung Kim
> > > - No longer saving evsel for each hist entry, using evlist__first
> > >   instead.
> > > - Factored out preparations to call annotation browser to do_annotate()
> > >   function
> > > - Other small fixes and adjustments.
> > >
> > > 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       | 73 +++++++++++++++++++++++++++++++---
> > >  tools/perf/ui/browsers/hists.c | 22 ----------
> > >  tools/perf/util/symbol.c       | 22 ++++++++++
> > >  tools/perf/util/symbol.h       |  1 +
> > >  4 files changed, 91 insertions(+), 27 deletions(-)
> > >
> > > --
> > > 2.40.1
> > >
> > 
> 
> -- 
>  Artem
> 

-- 

- Arnaldo

Re: [PATCH v2 0/2] perf tools: annotation browser from c2c tui

[Arnaldo Carvalho de Melo]

Em Wed, Jun 14, 2023 at 03:38:35PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Jun 09, 2023 at 10:37:31AM +0200, Artem Savkov escreveu:
> > On Thu, Jun 08, 2023 at 02:09:06PM -0700, Namhyung Kim wrote:
> > > On Thu, Jun 8, 2023 at 1:44 AM Artem Savkov <asavkov@redhat.com> wrote:
> > > > These patches add ability to start annotation browser from c2c report
> > > > tui. The idea comes from Arnaldo's "Profiling Data Structures" talk [1].

> > > I was thinking about how it works and realized that it didn't collect
> > > samples by symbol.  Then I'm not sure if the result is meaningful.
> > > I think it'd show a random symbol that touched the cache line
> > > first.  The same cache line can be accessed from other locations
> > > but it cannot know where they are.

> > > Also different instructions in a function (symbol) would access a
> > > different cache line.  The annotate output just shows any memory
> > > access.  So it might be good to check the instruction at the point
> > > but others should not be considered related.

> > > Hmm.. I suspect even the same instruction will hit the different
> > > cache lines at different times.  Then probably the annotation
> > > won't work well in terms of correlating cache lines.

> > The annotation hotkey is only added to the cacheline detailed view where
> > we do have symbol instruction information. The idea is to give the user
> > ability to quickly jump to source code/disassembly directly from c2c
> > TUI.

> > The hit percentages in annotation view don't make much sense in this
> > case though, so maybe it is better to use dummy evsel so that none are
> > shown.

> Yes, the point is just to reuse the source browser, if there is no
> annotation data applicable, don't use any.

I tried to work on having the annotation browser, both --stdio2 and the
TUI to work with evsels without annotation data, just so that we could
simply navigate source code or disassembly functions using the more
compact form we have in perf than the original one from objdump.

- Arnaldo

Re: [PATCH v2 0/2] perf tools: annotation browser from c2c tui

[Artem Savkov]

On Wed, Jun 14, 2023 at 03:38:35PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jun 09, 2023 at 10:37:31AM +0200, Artem Savkov escreveu:
> > On Thu, Jun 08, 2023 at 02:09:06PM -0700, Namhyung Kim wrote:
> > > Hello,
> > > 
> > > On Thu, Jun 8, 2023 at 1:44 AM Artem Savkov <asavkov@redhat.com> wrote:
> > > >
> > > > These patches add ability to start annotation browser from c2c report
> > > > tui. The idea comes from Arnaldo's "Profiling Data Structures" talk [1].
> > > 
> > > I was thinking about how it works and realized that it didn't collect
> > > samples by symbol.  Then I'm not sure if the result is meaningful.
> > > I think it'd show a random symbol that touched the cache line
> > > first.  The same cache line can be accessed from other locations
> > > but it cannot know where they are.
> > > 
> > > Also different instructions in a function (symbol) would access a
> > > different cache line.  The annotate output just shows any memory
> > > access.  So it might be good to check the instruction at the point
> > > but others should not be considered related.
> > > 
> > > Hmm.. I suspect even the same instruction will hit the different
> > > cache lines at different times.  Then probably the annotation
> > > won't work well in terms of correlating cache lines.
> > 
> > The annotation hotkey is only added to the cacheline detailed view where
> > we do have symbol instruction information. The idea is to give the user
> > ability to quickly jump to source code/disassembly directly from c2c
> > TUI.
> > 
> > The hit percentages in annotation view don't make much sense in this
> > case though, so maybe it is better to use dummy evsel so that none are
> > shown.
> 
> Yes, the point is just to reuse the source browser, if there is no
> annotation data applicable, don't use any.
> 

Ok, should be as easy as the following diff. I'll include it in v3 if
there si more feedback.

---

diff --git b/tools/perf/builtin-c2c.c a/tools/perf/builtin-c2c.c
index dce8604837aec..c856ce7a50740 100644
--- b/tools/perf/builtin-c2c.c
+++ a/tools/perf/builtin-c2c.c
@@ -2701,7 +2701,11 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)

                switch (key) {
                case 'a':
-                       do_annotate(browser, evlist__first(c2c.evlist));
+                       /*
+                        * We don't need percentage info so use 'dummy:HG'
+                        * evsel which is last in evlist.
+                        */
+                       do_annotate(browser, evlist__last(c2c.evlist));
                        break;
                case 's':
                        c2c.symbol_full = !c2c.symbol_full;