Skip to contentSkip to conversation

perf report/top with callchain fixes improvements

RFC In review Arnaldo Carvalho de Melo 5 participants 28 messages Dec 17, 2019 – Dec 21, 2019 perf events

lore
Share & tools
lore

Arnaldo Carvalho de Melo

author maintainer
#0 lore raw

Hi,

These patches try to address inconsistencies in the navigation
in the 'perf report' and 'perf top' when callchains are present.

Linus reported that when pressing ENTER with callchains enabled
it no longer shows the popup menu, instead it toggles the callchains, which is surprising and rather annoying.

So this patchset makes it consistent by presenting the popup
menu when ENTER is pressed and callchains are present, and then an extra menu entry, associated with the new '+' toggle is added and appears proeminently in the popup menu, together with suggestions about using 'e' to toogle all the callchains for the top level entry,

Other changes in this patchkit allows for using those suggested
hotkeys directly from the popup menu and add a new 'k' hotkey to zoom straight into the main kernel map. Pressing it again works just like using the popup menu via ENTER + "Zoom out of the kernel" or using ESC to Zoom out from the last filtering operation.

While working on this I noticed some other oddities like in
the default --children mode where the Annotation popup option appeared even for entries where no samples were taken, i.e. something that appeared just on some callchain, suppressing the Annotation popup entry for those cases. If the user insists in using the 'a' hotkey on such an entry, a popup warning is showed.

Also a fix for kernel maps for ranges that seldom appear in
samples, like "[kernel.vmlinux].init.text", that didn't a pointer back to the kernel maps data structure and thus was causing an assertion falure were found and fix, please test, available at:

Available at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/hists_browser

Best regards,

- Arnaldo

Arnaldo Carvalho de Melo (12):
perf hists browser: Restore ESC as "Zoom out" of DSO/thread/etc
perf report/top: Make ENTER consistently bring up menu
perf report/top: Add menu entry for toggling callchain expansion
perf report/top: Improve toggle callchain menu option
perf hists browser: Generalize the do_zoom_dso() function
perf report/top: Add 'k' hotkey to zoom directly into the kernel map perf hists browser: Allow passing an initial hotkey
tools ui popup: Allow returning hotkeys
perf report/top: Allow pressing hotkeys in the options popup menu perf report/top: Do not offer annotation for symbols without samples perf report/top: Make 'e' visible in the help and make it toggle showing callchains perf maps: Set maps pointer in the kmap area for kernel maps

 tools/perf/builtin-c2c.c            |   4 +-
 tools/perf/tests/maps.c             |   8 +-
 tools/perf/ui/browsers/hists.c      | 141 ++++++++++++++++++++++++----
 tools/perf/ui/browsers/hists.h      |   2 +-
 tools/perf/ui/browsers/res_sample.c |   2 +-
 tools/perf/ui/browsers/scripts.c    |   2 +-
 tools/perf/ui/tui/util.c            |  12 ++-
 tools/perf/ui/util.h                |   2 +-
 tools/perf/util/auxtrace.c          |   2 +-
 tools/perf/util/dso.c               |   4 +-
 tools/perf/util/dso.h               |   4 +-
 tools/perf/util/machine.c           |  10 +-
 tools/perf/util/map.c               |  15 ++-
 tools/perf/util/map.h               |   2 +-
 tools/perf/util/probe-event.c       |  10 +-
 tools/perf/util/sort.c              |   3 +-
 tools/perf/util/sort.h              |   2 +
 tools/perf/util/symbol-elf.c        |   2 +-
 tools/perf/util/symbol.c            |   6 +-
 19 files changed, 178 insertions(+), 55 deletions(-)
Signature · 1 line
2.21.0
author maintainer
#1 lore raw

PATCH 1/12 perf hists browser: Restore ESC as "Zoom out" of DSO/thread/etc

From: Arnaldo Carvalho de Melo <[email protected]>

We need to set actions->ms.map since 599a2f38a989 ("perf hists browser:
Check sort keys before hot key actions"), as in that patch we bail out if map is NULL.

Cc Adrian Hunter <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Fixes 599a2f38a989 ("perf hists browser: Check sort keys before hot key actions")
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/ui/browsers/hists.c | 1 +
 1 file changed, 1 insertion(+)
tools/perf/ui/browsers/hists.c +1 -0
index d4d3558fdef4..cfc6172ecab7 100644
@@ -3062,6 +3062,7 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 
 				continue;
 			}
+			actions->ms.map = map;
 			top = pstack__peek(browser->pstack);
 			if (top == &browser->hists->dso_filter) {
 				/*
Signature · 1 line
2.21.0
author maintainer
#2 lore raw

PATCH 2/12 perf report/top: Make ENTER consistently bring up menu

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

When callchains are present the ENTER key switches from bringing up the menu that offers Annotation, Zoom by DSO, etc to expanding/collapsing one callchain level, causing confusion, fix it by making it consistently bring up the menu and use '+' to expand/collapse one callchain level.

Next patch will also add an entry to the menu to allow expanding/collapsing, so that people used to ENTER expanding one callchain level can quickly find it and use it instead.

Reported-by Linus Torvalds <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jin Yao <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/ui/browsers/hists.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
tools/perf/ui/browsers/hists.c +2 -1
index cfc6172ecab7..fefa505d4fa8 100644
@@ -706,7 +706,7 @@ int hist_browser__run(struct hist_browser *browser, const char *help,
 			browser->show_headers = !browser->show_headers;
 			hist_browser__update_rows(browser);
 			break;
-		case K_ENTER:
+		case '+':
 			if (hist_browser__toggle_fold(browser))
 				break;
 			/* fall thru */

@@ -2858,6 +2858,7 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 	"For symbolic views (--sort has sym):\n\n"			\
 	"ENTER         Zoom into DSO/Threads & Annotate current symbol\n" \
 	"ESC           Zoom out\n"					\
+	"+             Expand/Collapse one callchain level\n"		\
 	"a             Annotate current symbol\n"			\
 	"C             Collapse all callchains\n"			\
 	"d             Zoom into current DSO\n"				\
Signature · 1 line
2.21.0
author maintainer
#3 lore raw

PATCH 3/12 perf report/top: Add menu entry for toggling callchain expansion

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

Since previously pressing ENTER toggled expansion/collapse of callchain entries and now brings up the same menu used when callchains are not present, add an entry so that users can quickly figure out the change in behaviour.

Its worth mentioning that we also always had 'e'/'c' to expand/collapse all entries in a hist entry and 'E'/'C' for all hist entries.

Suggested-by Linus Torvalds <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jin Yao <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/ui/browsers/hists.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
tools/perf/ui/browsers/hists.c +21 -0
index fefa505d4fa8..1b5a5990dddb 100644
@@ -2527,6 +2527,26 @@ add_dso_opt(struct hist_browser *browser, struct popup_action *act,
 	return 1;
 }
 
+static int do_toggle_callchain(struct hist_browser *browser, struct popup_action *act __maybe_unused)
+{
+	hist_browser__toggle_fold(browser);
+	return 0;
+}
+
+static int add_callchain_toggle_opt(struct hist_browser *browser, struct popup_action *act, char **optstr)
+{
+	struct hist_entry *he = browser->he_selection;
+
+        if (!he->has_children)
+                return 0;
+
+	if (asprintf(optstr, "Expand/Collapse callchain") < 0)
+		return 0;
+
+	act->fn = do_toggle_callchain;
+	return 1;
+}
+
 static int
 do_browse_map(struct hist_browser *browser __maybe_unused,
 	      struct popup_action *act)

@@ -3137,6 +3157,7 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 					     &options[nr_options], thread);
 		nr_options += add_dso_opt(browser, &actions[nr_options],
 					  &options[nr_options], map);
+		nr_options += add_callchain_toggle_opt(browser, &actions[nr_options], &options[nr_options]);
 		nr_options += add_map_opt(browser, &actions[nr_options],
 					  &options[nr_options],
 					  browser->selection ?
Signature · 1 line
2.21.0
author maintainer
#4 lore raw

PATCH 4/12 perf report/top: Improve toggle callchain menu option

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

Taking into account the current status of the callchain, i.e. if folded, show "Expand", otherwise "Collapse", also show the name of the entry that will be affected and mention the hotkeys for expanding/collapsing all callchains below the main entry, the one that appears with/without callchains.

Cc Adrian Hunter <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jin Yao <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/ui/browsers/hists.c | 54 ++++++++++++++++++++++++++++++++--
 tools/perf/util/sort.c         |  3 +-
 tools/perf/util/sort.h         |  2 ++
 3 files changed, 54 insertions(+), 5 deletions(-)
tools/perf/ui/browsers/hists.c +51 -3
index 1b5a5990dddb..a4413d983216 100644
@@ -391,6 +391,52 @@ static void hist_entry__init_have_children(struct hist_entry *he)
 	he->init_have_children = true;
 }
 
+static bool hist_browser__selection_has_children(struct hist_browser *browser)
+{
+	struct hist_entry *he = browser->he_selection;
+	struct map_symbol *ms = browser->selection;
+
+	if (!he || !ms)
+		return false;
+
+	if (ms == &he->ms)
+	       return he->has_children;
+
+	return container_of(ms, struct callchain_list, ms)->has_children;
+}
+
+static bool hist_browser__selection_unfolded(struct hist_browser *browser)
+{
+	struct hist_entry *he = browser->he_selection;
+	struct map_symbol *ms = browser->selection;
+
+	if (!he || !ms)
+		return false;
+
+	if (ms == &he->ms)
+	       return he->unfolded;
+
+	return container_of(ms, struct callchain_list, ms)->unfolded;
+}
+
+static char *hist_browser__selection_sym_name(struct hist_browser *browser, char *bf, size_t size)
+{
+	struct hist_entry *he = browser->he_selection;
+	struct map_symbol *ms = browser->selection;
+	struct callchain_list *callchain_entry;
+
+	if (!he || !ms)
+		return NULL;
+
+	if (ms == &he->ms) {
+	       hist_entry__sym_snprintf(he, bf, size, 0);
+	       return bf + 4; // skip the level, e.g. '[k] '
+	}
+
+	callchain_entry = container_of(ms, struct callchain_list, ms);
+	return callchain_list__sym_name(callchain_entry, bf, size, browser->show_dso);
+}
+
 static bool hist_browser__toggle_fold(struct hist_browser *browser)
 {
 	struct hist_entry *he = browser->he_selection;

@@ -2535,12 +2581,14 @@ static int do_toggle_callchain(struct hist_browser *browser, struct popup_action
 
 static int add_callchain_toggle_opt(struct hist_browser *browser, struct popup_action *act, char **optstr)
 {
-	struct hist_entry *he = browser->he_selection;
+	char sym_name[512];
 
-        if (!he->has_children)
+        if (!hist_browser__selection_has_children(browser))
                 return 0;
 
-	if (asprintf(optstr, "Expand/Collapse callchain") < 0)
+	if (asprintf(optstr, "%s [%s] callchain (one level, same as '+' hotkey, use 'e'/'c' for the whole main level entry)",
+		     hist_browser__selection_unfolded(browser) ? "Collapse" : "Expand",
+		     hist_browser__selection_sym_name(browser, sym_name, sizeof(sym_name))) < 0)
 		return 0;
 
 	act->fn = do_toggle_callchain;
tools/perf/util/sort.c +1 -2
index 9fcba2872130..ab0cfd790ad0 100644
@@ -324,8 +324,7 @@ static int _hist_entry__sym_snprintf(struct map_symbol *ms,
 	return ret;
 }
 
-static int hist_entry__sym_snprintf(struct hist_entry *he, char *bf,
-				    size_t size, unsigned int width)
+int hist_entry__sym_snprintf(struct hist_entry *he, char *bf, size_t size, unsigned int width)
 {
 	return _hist_entry__sym_snprintf(&he->ms, he->ip,
 					 he->level, bf, size, width);
tools/perf/util/sort.h +2 -0
index 5aff9542d9b7..6c862d62d052 100644
@@ -164,6 +164,8 @@ static __pure inline bool hist_entry__has_callchains(struct hist_entry *he)
 	return he->callchain_size != 0;
 }
 
+int hist_entry__sym_snprintf(struct hist_entry *he, char *bf, size_t size, unsigned int width);
+
 static inline bool hist_entry__has_pairs(struct hist_entry *he)
 {
 	return !list_empty(&he->pairs.node);
Signature · 1 line
2.21.0
author maintainer
#5 lore raw

PATCH 5/12 perf hists browser: Generalize the do_zoom_dso() function

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

We'll use it to provide a top level hotkey to zoom into the kernel dso directly.

Cc Adrian Hunter <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jin Yao <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/ui/browsers/hists.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
tools/perf/ui/browsers/hists.c +7 -4
index a4413d983216..8aba1aeea0eb 100644
@@ -2530,11 +2530,8 @@ add_thread_opt(struct hist_browser *browser, struct popup_action *act,
 	return 1;
 }
 
-static int
-do_zoom_dso(struct hist_browser *browser, struct popup_action *act)
+static int hists_browser__zoom_map(struct hist_browser *browser, struct map *map)
 {
-	struct map *map = act->ms.map;
-
 	if (!hists__has(browser->hists, dso) || map == NULL)
 		return 0;
 

@@ -2556,6 +2553,12 @@ do_zoom_dso(struct hist_browser *browser, struct popup_action *act)
 	return 0;
 }
 
+static int
+do_zoom_dso(struct hist_browser *browser, struct popup_action *act)
+{
+	return hists_browser__zoom_map(browser, act->ms.map);
+}
+
 static int
 add_dso_opt(struct hist_browser *browser, struct popup_action *act,
 	    char **optstr, struct map *map)
Signature · 1 line
2.21.0
author maintainer
#6 lore raw

PATCH 6/12 perf report/top: Add 'k' hotkey to zoom directly into the kernel map

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

As a convenience, equivalent to pressing Enter in a line with a kernel symbol and then selecting "Zoom" into the kernel DSO.

Cc Adrian Hunter <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jin Yao <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/ui/browsers/hists.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
tools/perf/ui/browsers/hists.c +8 -1
index 8aba1aeea0eb..6dfdd8d5a743 100644
@@ -18,7 +18,9 @@
 #include "../../util/evlist.h"
 #include "../../util/header.h"
 #include "../../util/hist.h"
+#include "../../util/machine.h"
 #include "../../util/map.h"
+#include "../../util/maps.h"
 #include "../../util/symbol.h"
 #include "../../util/map_symbol.h"
 #include "../../util/branch.h"

@@ -2566,7 +2568,7 @@ add_dso_opt(struct hist_browser *browser, struct popup_action *act,
 	if (!hists__has(browser->hists, dso) || map == NULL)
 		return 0;
 
-	if (asprintf(optstr, "Zoom %s %s DSO",
+	if (asprintf(optstr, "Zoom %s %s DSO (use the 'k' hotkey to zoom directly into the kernel)",
 		     browser->hists->dso_filter ? "out of" : "into",
 		     __map__is_kernel(map) ? "the Kernel" : map->dso->short_name) < 0)
 		return 0;

@@ -2936,6 +2938,7 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 	"E             Expand all callchains\n"				\
 	"F             Toggle percentage of filtered entries\n"		\
 	"H             Display column headers\n"			\
+	"k             Zoom into the kernel map\n"			\
 	"L             Change percent limit\n"				\
 	"m             Display context menu\n"				\
 	"S             Zoom into current Processor Socket\n"		\

@@ -3033,6 +3036,10 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 			actions->ms.map = map;
 			do_zoom_dso(browser, actions);
 			continue;
+		case 'k':
+			if (browser->selection != NULL)
+				hists_browser__zoom_map(browser, browser->selection->maps->machine->vmlinux_map);
+			continue;
 		case 'V':
 			verbose = (verbose + 1) % 4;
 			browser->show_dso = verbose > 0;
Signature · 1 line
2.21.0
maintainer
#7 lore raw

Re: [PATCH 06/12] perf report/top: Add 'k' hotkey to zoom directly into the kernel map

Hi Arnaldo,

Quoted 5 lines · Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

As a convenience, equivalent to pressing Enter in a line with a kernel symbol and then selecting "Zoom" into the kernel DSO.

We already have 'd' key for 'zoom into current dso'. Do you really want 'k' for kernel specially?

Quoted 29 lines
Cc Adrian Hunter <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jin Yao <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/ui/browsers/hists.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
tools/perf/ui/browsers/hists.c +2 -0
index 8aba1aeea0eb..6dfdd8d5a743 100644
@@ -18,7 +18,9 @@
 #include "../../util/evlist.h"
 #include "../../util/header.h"
 #include "../../util/hist.h"
+#include "../../util/machine.h"
 #include "../../util/map.h"
+#include "../../util/maps.h"
 #include "../../util/symbol.h"
 #include "../../util/map_symbol.h"
 #include "../../util/branch.h"

tools/perf/ui/browsers/hists.c add_dso_opt(struct hist_browser *browser, struct popup_action *act, · line 2571

Quoted 5 lines
tools/perf/ui/browsers/hists.c +1 -1
@@ -2566,7 +2568,7 @@ add_dso_opt(struct hist_browser *browser, struct popup_action *act,
        if (!hists__has(browser->hists, dso) || map == NULL)
                return 0;
 
-       if (asprintf(optstr, "Zoom %s %s DSO",
+       if (asprintf(optstr, "Zoom %s %s DSO (use the 'k' hotkey to zoom directly into the kernel)",

Wouldn't it be better suggesting 'd' key instead?

Thanks
Namhyung

Quoted 25 lines
tools/perf/ui/browsers/hists.c +5 -1
                     browser->hists->dso_filter ? "out of" : "into",
                     __map__is_kernel(map) ? "the Kernel" : map->dso->short_name) < 0)
                return 0;

@@ -2936,6 +2938,7 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
        "E             Expand all callchains\n"                         \
        "F             Toggle percentage of filtered entries\n"         \
        "H             Display column headers\n"                        \
+       "k             Zoom into the kernel map\n"                      \
        "L             Change percent limit\n"                          \
        "m             Display context menu\n"                          \
        "S             Zoom into current Processor Socket\n"            \

@@ -3033,6 +3036,10 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
                        actions->ms.map = map;
                        do_zoom_dso(browser, actions);
                        continue;
+               case 'k':
+                       if (browser->selection != NULL)
+                               hists_browser__zoom_map(browser, browser->selection->maps->machine->vmlinux_map);
+                       continue;
                case 'V':
                        verbose = (verbose + 1) % 4;
                        browser->show_dso = verbose > 0;
--

2.21.0

#8 lore raw

Re: [PATCH 06/12] perf report/top: Add 'k' hotkey to zoom directly into the kernel map

Em Fri, Dec 20, 2019 at 03:48:23PM +0900, Namhyung Kim escreveu:

Quoted 6 lines · Arnaldo Carvalho de Melo
Quoted 3 lines

From: Arnaldo Carvalho de Melo <[email protected]>
As a convenience, equivalent to pressing Enter in a line with a kernel symbol and then selecting "Zoom" into the kernel DSO.

We already have 'd' key for 'zoom into current dso'.

Right, current DSO, 'k' is equivalent to:

1. Navigate to a kernel map entry
2. Press 'd'

And also to:

1. Navigate to a kernel map entry
2. Press ENTER
3. Navigate to "Zoom into Kernel DSO"
4. Press ENTER

One key versus 2 or four.

Quoted 1 line

Do you really want 'k' for kernel specially?

I thought kernel hackers would like the convenience, doing:

perf top + k

To get the main kernel samples looks faster than:

perf top -e cycles:k

And those are not even equivalent, as cycles:k will show everything in ring 0, while 'perf top + k' will show just what is in the kernel _and_ in the main kernel map.

- Arnaldo

maintainer
#9 lore raw

Re: [PATCH 06/12] perf report/top: Add 'k' hotkey to zoom directly into the kernel map

Quoted 29 lines · Arnaldo Carvalho de Melo

Em Fri, Dec 20, 2019 at 03:48:23PM +0900, Namhyung Kim escreveu:

Quoted 6 lines · Arnaldo Carvalho de Melo
Quoted 3 lines

From: Arnaldo Carvalho de Melo <[email protected]>
As a convenience, equivalent to pressing Enter in a line with a kernel symbol and then selecting "Zoom" into the kernel DSO.

We already have 'd' key for 'zoom into current dso'.

Right, current DSO, 'k' is equivalent to:

1. Navigate to a kernel map entry
2. Press 'd'

And also to:

1. Navigate to a kernel map entry
2. Press ENTER
3. Navigate to "Zoom into Kernel DSO"
4. Press ENTER

One key versus 2 or four.

Quoted 1 line

Do you really want 'k' for kernel specially?

I thought kernel hackers would like the convenience, doing:

perf top + k

Quoted 7 lines

To get the main kernel samples looks faster than:

perf top -e cycles:k

And those are not even equivalent, as cycles:k will show everything in ring 0, while 'perf top + k' will show just what is in the kernel _and_ in the main kernel map.

I'm fine with adding 'k' key itself, but I thought a bit strange when it suggests kernel even with user DSOs.

Thanks
Namhyung

author maintainer
#10 lore raw

PATCH 7/12 perf hists browser: Allow passing an initial hotkey

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

Sometimes we're in an outer code, like the main hists browser popup menu and the user follows a suggestion about using some hotkey, and that hotkey is really handled by hists_browser__run(), so allow for calling it with that hotkey, making it handle it instead of waiting for the user to press one.

Cc Adrian Hunter <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jin Yao <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/builtin-c2c.c       |  4 ++--
 tools/perf/ui/browsers/hists.c | 13 +++++++------
 tools/perf/ui/browsers/hists.h |  2 +-
 3 files changed, 10 insertions(+), 9 deletions(-)
tools/perf/builtin-c2c.c +2 -2
index e69f44941aad..346351260c0b 100644
@@ -2384,7 +2384,7 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
 	c2c_browser__update_nr_entries(browser);
 
 	while (1) {
-		key = hist_browser__run(browser, "? - help", true);
+		key = hist_browser__run(browser, "? - help", true, 0);
 
 		switch (key) {
 		case 's':

@@ -2453,7 +2453,7 @@ static int perf_c2c__hists_browse(struct hists *hists)
 	c2c_browser__update_nr_entries(browser);
 
 	while (1) {
-		key = hist_browser__run(browser, "? - help", true);
+		key = hist_browser__run(browser, "? - help", true, 0);
 
 		switch (key) {
 		case 'q':
tools/perf/ui/browsers/hists.c +7 -6
index 6dfdd8d5a743..3b7af5a8d101 100644
@@ -673,9 +673,8 @@ static int hist_browser__title(struct hist_browser *browser, char *bf, size_t si
 }
 
 int hist_browser__run(struct hist_browser *browser, const char *help,
-		      bool warn_lost_event)
+		      bool warn_lost_event, int key)
 {
-	int key;
 	char title[160];
 	struct hist_browser_timer *hbt = browser->hbt;
 	int delay_secs = hbt ? hbt->refresh : 0;

@@ -688,9 +687,12 @@ int hist_browser__run(struct hist_browser *browser, const char *help,
 	if (ui_browser__show(&browser->b, title, "%s", help) < 0)
 		return -1;
 
+	if (key)
+		goto do_hotkey;
+
 	while (1) {
 		key = ui_browser__run(&browser->b, delay_secs);
-
+do_hotkey:
 		switch (key) {
 		case K_TIMER: {
 			u64 nr_entries;

@@ -2994,8 +2996,7 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 
 		nr_options = 0;
 
-		key = hist_browser__run(browser, helpline,
-					warn_lost_event);
+		key = hist_browser__run(browser, helpline, warn_lost_event, 0);
 
 		if (browser->he_selection != NULL) {
 			thread = hist_browser__selected_thread(browser);

@@ -3573,7 +3574,7 @@ int block_hists_tui_browse(struct block_hist *bh, struct evsel *evsel,
 	memset(&action, 0, sizeof(action));
 
 	while (1) {
-		key = hist_browser__run(browser, "? - help", true);
+		key = hist_browser__run(browser, "? - help", true, 0);
 
 		switch (key) {
 		case 'q':
tools/perf/ui/browsers/hists.h +1 -1
index 078f2f2c7abd..1e938d9ffa5e 100644
@@ -34,7 +34,7 @@ struct hist_browser {
 struct hist_browser *hist_browser__new(struct hists *hists);
 void hist_browser__delete(struct hist_browser *browser);
 int hist_browser__run(struct hist_browser *browser, const char *help,
-		      bool warn_lost_event);
+		      bool warn_lost_event, int key);
 void hist_browser__init(struct hist_browser *browser,
 			struct hists *hists);
 #endif /* _PERF_UI_BROWSER_HISTS_H_ */
Signature · 1 line
2.21.0
#11 lore raw

Re: [PATCH 07/12] perf hists browser: Allow passing an initial hotkey

On Tue, Dec 17, 2019 at 11:48:23AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

Quoted 16 lines
tools/perf/ui/browsers/hists.c +1 -2
index 6dfdd8d5a743..3b7af5a8d101 100644
@@ -673,9 +673,8 @@ static int hist_browser__title(struct hist_browser *browser, char *bf, size_t si
 }
 
 int hist_browser__run(struct hist_browser *browser, const char *help,
-		      bool warn_lost_event)
+		      bool warn_lost_event, int key)
 {
-	int key;
 	char title[160];
 	struct hist_browser_timer *hbt = browser->hbt;
 	int delay_secs = hbt ? hbt->refresh : 0;

tools/perf/ui/browsers/hists.c int hist_browser__run(struct hist_browser *browser, const char *help, · line 695

Quoted 10 lines
tools/perf/ui/browsers/hists.c +4 -1
@@ -688,9 +687,12 @@ int hist_browser__run(struct hist_browser *browser, const char *help,
 	if (ui_browser__show(&browser->b, title, "%s", help) < 0)
 		return -1;
 
+	if (key)
+		goto do_hotkey;
+
 	while (1) {
 		key = ui_browser__run(&browser->b, delay_secs);
-
+do_hotkey:

or we could switch the 'swtich' and ui_browser__run, and get rid of the goto, like:

	while (1) {
  		switch (key) {
		...
		}

		key = ui_browser__run(&browser->b, delay_secs);
	}

jirka

Quoted 38 lines
tools/perf/ui/browsers/hists.c +2 -3
 		switch (key) {
 		case K_TIMER: {
 			u64 nr_entries;

@@ -2994,8 +2996,7 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 
 		nr_options = 0;
 
-		key = hist_browser__run(browser, helpline,
-					warn_lost_event);
+		key = hist_browser__run(browser, helpline, warn_lost_event, 0);
 
 		if (browser->he_selection != NULL) {
 			thread = hist_browser__selected_thread(browser);

@@ -3573,7 +3574,7 @@ int block_hists_tui_browse(struct block_hist *bh, struct evsel *evsel,
 	memset(&action, 0, sizeof(action));
 
 	while (1) {
-		key = hist_browser__run(browser, "? - help", true);
+		key = hist_browser__run(browser, "? - help", true, 0);
 
 		switch (key) {
 		case 'q':
tools/perf/ui/browsers/hists.h +1 -1
index 078f2f2c7abd..1e938d9ffa5e 100644
@@ -34,7 +34,7 @@ struct hist_browser {
 struct hist_browser *hist_browser__new(struct hists *hists);
 void hist_browser__delete(struct hist_browser *browser);
 int hist_browser__run(struct hist_browser *browser, const char *help,
-		      bool warn_lost_event);
+		      bool warn_lost_event, int key);
 void hist_browser__init(struct hist_browser *browser,
 			struct hists *hists);
 #endif /* _PERF_UI_BROWSER_HISTS_H_ */
Signature · 1 line
2.21.0
#12 lore raw

Re: [PATCH 07/12] perf hists browser: Allow passing an initial hotkey

Em Wed, Dec 18, 2019 at 09:08:18AM +0100, Jiri Olsa escreveu:

Quoted 33 lines · Arnaldo Carvalho de Melo

SNIP

Quoted 16 lines
tools/perf/ui/browsers/hists.c +1 -2
index 6dfdd8d5a743..3b7af5a8d101 100644
@@ -673,9 +673,8 @@ static int hist_browser__title(struct hist_browser *browser, char *bf, size_t si
 }
 
 int hist_browser__run(struct hist_browser *browser, const char *help,
-		      bool warn_lost_event)
+		      bool warn_lost_event, int key)
 {
-	int key;
 	char title[160];
 	struct hist_browser_timer *hbt = browser->hbt;
 	int delay_secs = hbt ? hbt->refresh : 0;

tools/perf/ui/browsers/hists.c int hist_browser__run(struct hist_browser *browser, const char *help, · line 695

Quoted 10 lines
tools/perf/ui/browsers/hists.c +4 -1
@@ -688,9 +687,12 @@ int hist_browser__run(struct hist_browser *browser, const char *help,
 	if (ui_browser__show(&browser->b, title, "%s", help) < 0)
 		return -1;
 
+	if (key)
+		goto do_hotkey;
+
 	while (1) {
 		key = ui_browser__run(&browser->b, delay_secs);
-
+do_hotkey:

or we could switch the 'swtich' and ui_browser__run, and get rid of the goto, like:

Quoted 7 lines
	while (1) {
  		switch (key) {
		...
		}

		key = ui_browser__run(&browser->b, delay_secs);
	}

I think those are equivalent and having the test like I did is more clear, i.e. "has this key been provided" instead of going to the switch just to hit the default case for the zero in key and call ui_browser__run().

- Arnaldo

Quoted 40 lines

jirka

Quoted 38 lines
tools/perf/ui/browsers/hists.c +2 -3
 		switch (key) {
 		case K_TIMER: {
 			u64 nr_entries;

@@ -2994,8 +2996,7 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 
 		nr_options = 0;
 
-		key = hist_browser__run(browser, helpline,
-					warn_lost_event);
+		key = hist_browser__run(browser, helpline, warn_lost_event, 0);
 
 		if (browser->he_selection != NULL) {
 			thread = hist_browser__selected_thread(browser);

@@ -3573,7 +3574,7 @@ int block_hists_tui_browse(struct block_hist *bh, struct evsel *evsel,
 	memset(&action, 0, sizeof(action));
 
 	while (1) {
-		key = hist_browser__run(browser, "? - help", true);
+		key = hist_browser__run(browser, "? - help", true, 0);
 
 		switch (key) {
 		case 'q':
tools/perf/ui/browsers/hists.h +1 -1
index 078f2f2c7abd..1e938d9ffa5e 100644
@@ -34,7 +34,7 @@ struct hist_browser {
 struct hist_browser *hist_browser__new(struct hists *hists);
 void hist_browser__delete(struct hist_browser *browser);
 int hist_browser__run(struct hist_browser *browser, const char *help,
-		      bool warn_lost_event);
+		      bool warn_lost_event, int key);
 void hist_browser__init(struct hist_browser *browser,
 			struct hists *hists);
 #endif /* _PERF_UI_BROWSER_HISTS_H_ */
Signature · 1 line
2.21.0
Signature · 1 line
- Arnaldo
#13 lore raw

Re: [PATCH 07/12] perf hists browser: Allow passing an initial hotkey

Quoted 39 lines · Arnaldo Carvalho de Melo

Em Wed, Dec 18, 2019 at 09:08:18AM +0100, Jiri Olsa escreveu:

Quoted 38 lines · Arnaldo Carvalho de Melo

SNIP

Quoted 16 lines
tools/perf/ui/browsers/hists.c +1 -2
index 6dfdd8d5a743..3b7af5a8d101 100644
@@ -673,9 +673,8 @@ static int hist_browser__title(struct hist_browser *browser, char *bf, size_t si
 }
 
 int hist_browser__run(struct hist_browser *browser, const char *help,
-		      bool warn_lost_event)
+		      bool warn_lost_event, int key)
 {
-	int key;
 	char title[160];
 	struct hist_browser_timer *hbt = browser->hbt;
 	int delay_secs = hbt ? hbt->refresh : 0;

tools/perf/ui/browsers/hists.c int hist_browser__run(struct hist_browser *browser, const char *help, · line 695

Quoted 10 lines
tools/perf/ui/browsers/hists.c +4 -1
@@ -688,9 +687,12 @@ int hist_browser__run(struct hist_browser *browser, const char *help,
 	if (ui_browser__show(&browser->b, title, "%s", help) < 0)
 		return -1;
 
+	if (key)
+		goto do_hotkey;
+
 	while (1) {
 		key = ui_browser__run(&browser->b, delay_secs);
-
+do_hotkey:

or we could switch the 'swtich' and ui_browser__run, and get rid of the goto, like:

	while (1) {
  		switch (key) {
		...
		}
Quoted 7 lines
Quoted 2 lines
		key = ui_browser__run(&browser->b, delay_secs);
	}

I think those are equivalent and having the test like I did is more clear, i.e. "has this key been provided" instead of going to the switch just to hit the default case for the zero in key and call ui_browser__run().

sure, I just don't like goto other than for error handling, looks too hacky to me ;-) but of course it's your call

jirka

#14 lore raw

Re: [PATCH 07/12] perf hists browser: Allow passing an initial hotkey

Em Wed, Dec 18, 2019 at 03:23:21PM +0100, Jiri Olsa escreveu:

Quoted 21 lines · Arnaldo Carvalho de Melo
Quoted 19 lines

Em Wed, Dec 18, 2019 at 09:08:18AM +0100, Jiri Olsa escreveu:

Quoted 18 lines · Arnaldo Carvalho de Melo

Review

Quoted 7 lines
(quoted diff) +4 -1
+	if (key)
+		goto do_hotkey;
+
 	while (1) {
 		key = ui_browser__run(&browser->b, delay_secs);
-
+do_hotkey:

or we could switch the 'swtich' and ui_browser__run, and get rid of the goto, like:

	while (1) {
  		switch (key) {
		...
		}

		key = ui_browser__run(&browser->b, delay_secs);
	}
Quoted 7 lines
Quoted 4 lines

I think those are equivalent and having the test like I did is more clear, i.e. "has this key been provided" instead of going to the switch just to hit the default case for the zero in key and call ui_browser__run().

sure, I just don't like goto other than for error handling, looks too hacky to me ;-) but of course it's your call

How about the one below?

- Arnaldo

commit 9290297c911ad6c315c5f46f35afa3ce517a4c9f

Author: Arnaldo Carvalho de Melo <[email protected]>
Date:   Thu Dec 12 15:31:40 2019 -0300

perf hists browser: Allow passing an initial hotkey

Sometimes we're in an outer code, like the main hists browser popup menu and the user follows a suggestion about using some hotkey, and that hotkey is really handled by hists_browser__run(), so allow for calling it with that hotkey, making it handle it instead of waiting for the user to press one.

Cc Adrian Hunter <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jin Yao <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
tools/perf/builtin-c2c.c +2 -2
index e69f44941aad..346351260c0b 100644
@@ -2384,7 +2384,7 @@ static int perf_c2c__browse_cacheline(struct hist_entry *he)
 	c2c_browser__update_nr_entries(browser);
 
 	while (1) {
-		key = hist_browser__run(browser, "? - help", true);
+		key = hist_browser__run(browser, "? - help", true, 0);
 
 		switch (key) {
 		case 's':

@@ -2453,7 +2453,7 @@ static int perf_c2c__hists_browse(struct hists *hists)
 	c2c_browser__update_nr_entries(browser);
 
 	while (1) {
-		key = hist_browser__run(browser, "? - help", true);
+		key = hist_browser__run(browser, "? - help", true, 0);
 
 		switch (key) {
 		case 'q':
tools/perf/ui/browsers/hists.c +79 -74

183 lines not on this page · raw message

tools/perf/ui/browsers/hists.h +1 -1
index 078f2f2c7abd..1e938d9ffa5e 100644
@@ -34,7 +34,7 @@ struct hist_browser {
 struct hist_browser *hist_browser__new(struct hists *hists);
 void hist_browser__delete(struct hist_browser *browser);
 int hist_browser__run(struct hist_browser *browser, const char *help,
-		      bool warn_lost_event);
+		      bool warn_lost_event, int key);
 void hist_browser__init(struct hist_browser *browser,
 			struct hists *hists);
 #endif /* _PERF_UI_BROWSER_HISTS_H_ */
#15 lore raw

Re: [PATCH 07/12] perf hists browser: Allow passing an initial hotkey

Quoted 27 lines · Arnaldo Carvalho de Melo

Em Wed, Dec 18, 2019 at 03:23:21PM +0100, Jiri Olsa escreveu:

Quoted 25 lines · Arnaldo Carvalho de Melo
Quoted 24 lines

Em Wed, Dec 18, 2019 at 09:08:18AM +0100, Jiri Olsa escreveu:

Quoted 11 lines · Arnaldo Carvalho de Melo

tools/perf/ui/browsers/hists.c int hist_browser__run(struct hist_browser *browser, const char *help,

Quoted 7 lines
tools/perf/ui/browsers/hists.c +4 -1
+	if (key)
+		goto do_hotkey;
+
 	while (1) {
 		key = ui_browser__run(&browser->b, delay_secs);
-
+do_hotkey:

or we could switch the 'swtich' and ui_browser__run, and get rid of the goto, like:

Quoted 7 lines
	while (1) {
  		switch (key) {
		...
		}

		key = ui_browser__run(&browser->b, delay_secs);
	}

I think those are equivalent and having the test like I did is more clear, i.e. "has this key been provided" instead of going to the switch just to hit the default case for the zero in key and call ui_browser__run().

Quoted 4 lines
Quoted 2 lines

sure, I just don't like goto other than for error handling, looks too hacky to me ;-) but of course it's your call

How about the one below?

looks good, thanks

jirka

author maintainer
#16 lore raw

PATCH 8/12 tools ui popup: Allow returning hotkeys

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

With this patch if an optional pointer is passed to ui__popup_menu() then when any key that is not being handled (ENTER, ESC, etc) is typed, it'll record that key in the pointer and return, allowing for hotkey processing on the caller.

If NULL is passed, no change in logic, unhandled keys continue to be ignored.

Cc Adrian Hunter <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jin Yao <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/ui/browsers/hists.c      |  4 ++--
 tools/perf/ui/browsers/res_sample.c |  2 +-
 tools/perf/ui/browsers/scripts.c    |  2 +-
 tools/perf/ui/tui/util.c            | 12 ++++++++----
 tools/perf/ui/util.h                |  2 +-
 5 files changed, 13 insertions(+), 9 deletions(-)
tools/perf/ui/browsers/hists.c +2 -2
index 3b7af5a8d101..da7de49b3553 100644
@@ -2389,7 +2389,7 @@ static int switch_data_file(void)
 	closedir(pwd_dir);
 
 	if (nr_options) {
-		choice = ui__popup_menu(nr_options, options);
+		choice = ui__popup_menu(nr_options, options, NULL);
 		if (choice < nr_options && choice >= 0) {
 			tmp = strdup(abs_path[choice]);
 			if (tmp) {

@@ -3275,7 +3275,7 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 		do {
 			struct popup_action *act;
 
-			choice = ui__popup_menu(nr_options, options);
+			choice = ui__popup_menu(nr_options, options, NULL);
 			if (choice == -1 || choice >= nr_options)
 				break;
tools/perf/ui/browsers/res_sample.c +1 -1
index 76d356a18790..7cb2d6678039 100644
@@ -56,7 +56,7 @@ int res_sample_browse(struct res_sample *res_samples, int num_res,
 			return -1;
 		}
 	}
-	choice = ui__popup_menu(num_res, names);
+	choice = ui__popup_menu(num_res, names, NULL);
 	for (i = 0; i < num_res; i++)
 		zfree(&names[i]);
 	free(names);
tools/perf/ui/browsers/scripts.c +1 -1
index fc733a6354d4..47d2c7a8cbe1 100644
@@ -126,7 +126,7 @@ static int list_scripts(char *script_name, bool *custom,
 			SCRIPT_FULLPATH_LEN);
 	if (num < 0)
 		num = 0;
-	choice = ui__popup_menu(num + max_std, (char * const *)names);
+	choice = ui__popup_menu(num + max_std, (char * const *)names, NULL);
 	if (choice < 0) {
 		ret = -1;
 		goto out;
tools/perf/ui/tui/util.c +8 -4
index b98dd0e31dc1..0f562e2cb1e8 100644
@@ -23,7 +23,7 @@ static void ui_browser__argv_write(struct ui_browser *browser,
 	ui_browser__write_nstring(browser, *arg, browser->width);
 }
 
-static int popup_menu__run(struct ui_browser *menu)
+static int popup_menu__run(struct ui_browser *menu, int *keyp)
 {
 	int key;
 

@@ -45,6 +45,11 @@ static int popup_menu__run(struct ui_browser *menu)
 			key = -1;
 			break;
 		default:
+			if (keyp) {
+				*keyp = key;
+				key = menu->nr_entries;
+				break;
+			}
 			continue;
 		}
 

@@ -55,7 +60,7 @@ static int popup_menu__run(struct ui_browser *menu)
 	return key;
 }
 
-int ui__popup_menu(int argc, char * const argv[])
+int ui__popup_menu(int argc, char * const argv[], int *keyp)
 {
 	struct ui_browser menu = {
 		.entries    = (void *)argv,

@@ -64,8 +69,7 @@ int ui__popup_menu(int argc, char * const argv[])
 		.write	    = ui_browser__argv_write,
 		.nr_entries = argc,
 	};
-
-	return popup_menu__run(&menu);
+	return popup_menu__run(&menu, keyp);
 }
 
 int ui_browser__input_window(const char *title, const char *text, char *input,
tools/perf/ui/util.h +1 -1
index 40891942f465..e30cea807564 100644
@@ -5,7 +5,7 @@
 #include <stdarg.h>
 
 int ui__getch(int delay_secs);
-int ui__popup_menu(int argc, char * const argv[]);
+int ui__popup_menu(int argc, char * const argv[], int *keyp);
 int ui__help_window(const char *text);
 int ui__dialog_yesno(const char *msg);
 void __ui__info_window(const char *title, const char *text, const char *exit_msg);
Signature · 1 line
2.21.0
author maintainer
#17 lore raw

PATCH 9/12 perf report/top: Allow pressing hotkeys in the options popup menu

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

When the users presses ENTER in the main 'perf report/top' screen a popup menu is presented, in it some hotkeys are suggested as alternatives to using the menu, or for additional features.

At that point the user may try those hotkeys, so allow for that by recording the key used and exiting, the caller then can check for that possibility and process the hotkey.

I.e. try pressing ENTER, and then 'k' to exit and zoom into the kernel map, using ESC then zooms out, etc.

Cc Adrian Hunter <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jin Yao <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/ui/browsers/hists.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
tools/perf/ui/browsers/hists.c +10 -6
index da7de49b3553..7f653b9f5cd8 100644
@@ -2991,12 +2991,13 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 	while (1) {
 		struct thread *thread = NULL;
 		struct map *map = NULL;
-		int choice = 0;
+		int choice;
 		int socked_id = -1;
 
-		nr_options = 0;
-
-		key = hist_browser__run(browser, helpline, warn_lost_event, 0);
+		key = 0; // reset key
+do_hotkey:		 // key came straight from options ui__popup_menu()
+		choice = nr_options = 0;
+		key = hist_browser__run(browser, helpline, warn_lost_event, key);
 
 		if (browser->he_selection != NULL) {
 			thread = hist_browser__selected_thread(browser);

@@ -3275,10 +3276,13 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 		do {
 			struct popup_action *act;
 
-			choice = ui__popup_menu(nr_options, options, NULL);
-			if (choice == -1 || choice >= nr_options)
+			choice = ui__popup_menu(nr_options, options, &key);
+			if (choice == -1)
 				break;
 
+			if (choice == nr_options)
+				goto do_hotkey;
+
 			act = &actions[choice];
 			key = act->fn(browser, act);
 		} while (key == 1);
Signature · 1 line
2.21.0
author maintainer
#18 lore raw

PATCH 10/12 perf report/top: Do not offer annotation for symbols without samples

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

This can happen in the --children mode, i.e. the default mode when callchains are present, where one of the main entries may be a callchain entry with no samples.

So far we were not providing any information about why an annotation couldn't be provided even offering the Annotation option in the popup menu.

Work is needed to allow for no-samples "annotation', i.e. to show the disassembly anyway and allow for navigation, etc.

Cc Adrian Hunter <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jin Yao <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/ui/browsers/hists.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
tools/perf/ui/browsers/hists.c +10 -1
index 7f653b9f5cd8..3f10e1a070c5 100644
@@ -2461,7 +2461,8 @@ add_annotate_opt(struct hist_browser *browser __maybe_unused,
 		 struct popup_action *act, char **optstr,
 		 struct map_symbol *ms)
 {
-	if (ms->sym == NULL || ms->map->dso->annotate_warned)
+	if (ms->sym == NULL || ms->map->dso->annotate_warned ||
+	    symbol__annotation(ms->sym)->src == NULL)
 		return 0;
 
 	if (asprintf(optstr, "Annotate %s", ms->sym->name) < 0)

@@ -3027,6 +3028,14 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 			    browser->selection->map->dso->annotate_warned)
 				continue;
 
+			if (symbol__annotation(browser->selection->sym)->src == NULL) {
+				ui_browser__warning(&browser->b, delay_secs * 2,
+						    "No samples for the \"%s\" symbol.\n\n"
+						    "Probably appeared just in a callchain",
+						    browser->selection->sym->name);
+				continue;
+			}
+
 			actions->ms.map = browser->selection->map;
 			actions->ms.sym = browser->selection->sym;
 			do_annotate(browser, actions);
Signature · 1 line
2.21.0
author maintainer
#19 lore raw

PATCH 11/12 perf report/top: Make 'e' visible in the help and make it toggle showing callchains

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

The 'e' and 'c' hotkeys were present for a long time, but not documented in the help window, change 'e' to be a toggle so that it gets consistent with other toggles like '+' and document it in the help window.

Keep 'c' as is for people used to it but don't document, as it is easier to just use 'e' to show/hide all the callchains for a top level histogram entry.

Cc Adrian Hunter <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jin Yao <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Kan Liang <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/ui/browsers/hists.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
tools/perf/ui/browsers/hists.c +7 -1
index 3f10e1a070c5..fa3a16c49398 100644
@@ -407,6 +407,11 @@ static bool hist_browser__selection_has_children(struct hist_browser *browser)
 	return container_of(ms, struct callchain_list, ms)->has_children;
 }
 
+static bool hist_browser__he_selection_unfolded(struct hist_browser *browser)
+{
+	return browser->he_selection ? browser->he_selection->unfolded : false;
+}
+
 static bool hist_browser__selection_unfolded(struct hist_browser *browser)
 {
 	struct hist_entry *he = browser->he_selection;

@@ -750,7 +755,7 @@ int hist_browser__run(struct hist_browser *browser, const char *help,
 			break;
 		case 'e':
 			/* Expand the selected entry. */
-			hist_browser__set_folding_selected(browser, true);
+			hist_browser__set_folding_selected(browser, !hist_browser__he_selection_unfolded(browser));
 			break;
 		case 'H':
 			browser->show_headers = !browser->show_headers;

@@ -2938,6 +2943,7 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 	"a             Annotate current symbol\n"			\
 	"C             Collapse all callchains\n"			\
 	"d             Zoom into current DSO\n"				\
+	"e             Expand/Collapse main entry callchains\n"	\
 	"E             Expand all callchains\n"				\
 	"F             Toggle percentage of filtered entries\n"		\
 	"H             Display column headers\n"			\
Signature · 1 line
2.21.0
author maintainer
#20 lore raw

PATCH 12/12 perf maps: Set maps pointer in the kmap area for kernel maps

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

When kernel maps are created with map__new2() we allocate an extra area to store a pointer to the 'struct maps' for the kernel maps, and this ends up being used in places like __map__is_kernel() to figure out if a map is the main kernel one.

We were setting this up only in __machine__create_kernel_maps() and machine__create_extra_kernel_map(), but not when splitting kallsyms, kcore address ranges in new kernel maps such as "[kernel.vmlinux].init.text", leading to assertion failures in __map__is_kernel().

So make map__new2() receive the kernel 'struct maps' pointer so that all kernel maps point back to it in its 'struct kmap' area.

Cc Adrian Hunter <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/tests/maps.c       |  8 ++++----
 tools/perf/util/auxtrace.c    |  2 +-
 tools/perf/util/dso.c         |  4 ++--
 tools/perf/util/dso.h         |  4 +++-
 tools/perf/util/machine.c     | 10 ++++------
 tools/perf/util/map.c         | 15 ++++++++++++++-
 tools/perf/util/map.h         |  2 +-
 tools/perf/util/probe-event.c | 10 ++++++----
 tools/perf/util/symbol-elf.c  |  2 +-
 tools/perf/util/symbol.c      |  6 ++++--
 10 files changed, 40 insertions(+), 23 deletions(-)
tools/perf/tests/maps.c +4 -4
index edcbc70ff9d6..7e32b4df8ff1 100644
@@ -69,7 +69,7 @@ int test__maps__merge_in(struct test *t __maybe_unused, int subtest __maybe_unus
 	for (i = 0; i < ARRAY_SIZE(bpf_progs); i++) {
 		struct map *map;
 
-		map = dso__new_map(bpf_progs[i].name);
+		map = dso__new_map(bpf_progs[i].name, &maps);
 		TEST_ASSERT_VAL("failed to create map", map);
 
 		map->start = bpf_progs[i].start;

@@ -78,13 +78,13 @@ int test__maps__merge_in(struct test *t __maybe_unused, int subtest __maybe_unus
 		map__put(map);
 	}
 
-	map_kcore1 = dso__new_map("kcore1");
+	map_kcore1 = dso__new_map("kcore1", &maps);
 	TEST_ASSERT_VAL("failed to create map", map_kcore1);
 
-	map_kcore2 = dso__new_map("kcore2");
+	map_kcore2 = dso__new_map("kcore2", &maps);
 	TEST_ASSERT_VAL("failed to create map", map_kcore2);
 
-	map_kcore3 = dso__new_map("kcore3");
+	map_kcore3 = dso__new_map("kcore3", &maps);
 	TEST_ASSERT_VAL("failed to create map", map_kcore3);
 
 	/* kcore1 map overlaps over all bpf maps */
tools/perf/util/auxtrace.c +1 -1
index eb087e7df6f4..c5be6eeff069 100644
@@ -2250,7 +2250,7 @@ static struct dso *load_dso(const char *name)
 	struct map *map;
 	struct dso *dso;
 
-	map = dso__new_map(name);
+	map = dso__new_map(name, NULL);
 	if (!map)
 		return NULL;
tools/perf/util/dso.c +2 -2
index 91f21239608b..dd5c2e71d8d7 100644
@@ -1118,13 +1118,13 @@ ssize_t dso__data_write_cache_addr(struct dso *dso, struct map *map,
 	return dso__data_write_cache_offs(dso, machine, offset, data, size);
 }
 
-struct map *dso__new_map(const char *name)
+struct map *dso__new_map(const char *name, struct maps *kmaps)
 {
 	struct map *map = NULL;
 	struct dso *dso = dso__new(name);
 
 	if (dso)
-		map = map__new2(0, dso);
+		map = map__new2(0, dso, kmaps);
 
 	return map;
 }
tools/perf/util/dso.h +3 -1
index 2db64b79617a..3b4f690c67e0 100644
@@ -353,7 +353,9 @@ ssize_t dso__data_write_cache_addr(struct dso *dso, struct map *map,
 				   struct machine *machine, u64 addr,
 				   const u8 *data, ssize_t size);
 
-struct map *dso__new_map(const char *name);
+struct maps;
+
+struct map *dso__new_map(const char *name, struct maps *kmaps);
 struct dso *machine__findnew_kernel(struct machine *machine, const char *name,
 				    const char *short_name, int dso_type);
tools/perf/util/machine.c +4 -6
index c8c5410315e8..7ee14963edc9 100644
@@ -726,7 +726,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
 	struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
 
 	if (!map) {
-		map = dso__new_map(event->ksymbol.name);
+		map = dso__new_map(event->ksymbol.name, &machine->kmaps);
 		if (!map)
 			return -ENOMEM;
 

@@ -784,7 +784,7 @@ static struct map *machine__addnew_module_map(struct machine *machine, u64 start
 	if (dso == NULL)
 		goto out;
 
-	map = map__new2(start, dso);
+	map = map__new2(start, dso, &machine->kmaps);
 	if (map == NULL)
 		goto out;
 

@@ -963,7 +963,7 @@ int machine__create_extra_kernel_map(struct machine *machine,
 	struct kmap *kmap;
 	struct map *map;
 
-	map = map__new2(xm->start, kernel);
+	map = map__new2(xm->start, kernel, &machine->kmaps);
 	if (!map)
 		return -1;
 

@@ -972,7 +972,6 @@ int machine__create_extra_kernel_map(struct machine *machine,
 
 	kmap = map__kmap(map);
 
-	kmap->kmaps = &machine->kmaps;
 	strlcpy(kmap->name, xm->name, KMAP_NAME_LEN);
 
 	maps__insert(&machine->kmaps, map);

@@ -1088,7 +1087,7 @@ __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 	/* In case of renewal the kernel map, destroy previous one */
 	machine__destroy_kernel_maps(machine);
 
-	machine->vmlinux_map = map__new2(0, kernel);
+	machine->vmlinux_map = map__new2(0, kernel, &machine->kmaps);
 	if (machine->vmlinux_map == NULL)
 		return -1;
 

@@ -1098,7 +1097,6 @@ __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 	if (!kmap)
 		return -1;
 
-	kmap->kmaps = &machine->kmaps;
 	maps__insert(&machine->kmaps, map);
 
 	return 0;
tools/perf/util/map.c +14 -1
index fdd5bddb3075..a2cdfe62df94 100644
@@ -223,7 +223,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
  * they are loaded) and for vmlinux, where only after we load all the
  * symbols we'll know where it starts and ends.
  */
-struct map *map__new2(u64 start, struct dso *dso)
+struct map *map__new2(u64 start, struct dso *dso, struct maps *kmaps)
 {
 	struct map *map = calloc(1, (sizeof(*map) +
 				     (dso->kernel ? sizeof(struct kmap) : 0)));

@@ -232,6 +232,19 @@ struct map *map__new2(u64 start, struct dso *dso)
 		 * ->end will be filled after we load all the symbols
 		 */
 		map__init(map, start, 0, 0, dso);
+		if (dso->kernel) {
+			/*
+			 * __map__is_kernel() Needs this for in-kernel map ranges
+			 * such as:
+			 * (gdb) print map->dso->name
+			 * $8 = 0x1232d6c "[kernel.vmlinux].init.text"
+			 * (gdb) print map->dso->kernel
+			 * $9 = DSO_TYPE_KERNEL
+			 * (gdb)
+			 */
+			struct kmap *kmap = map__kmap(map);
+			kmap->kmaps = kmaps;
+		}
 	}
 
 	return map;
tools/perf/util/map.h +1 -1
index 067036e8970c..b4531876cd66 100644
@@ -108,7 +108,7 @@ struct dso_id;
 struct map *map__new(struct machine *machine, u64 start, u64 len,
 		     u64 pgoff, struct dso_id *id, u32 prot, u32 flags,
 		     char *filename, struct thread *thread);
-struct map *map__new2(u64 start, struct dso *dso);
+struct map *map__new2(u64 start, struct dso *dso, struct maps *kmaps);
 void map__delete(struct map *map);
 struct map *map__clone(struct map *map);
tools/perf/util/probe-event.c +6 -4
index eea132f512b0..b123a800d260 100644
@@ -146,7 +146,7 @@ static struct map *kernel_get_module_map(const char *module)
 
 	/* A file path -- this is an offline module */
 	if (module && strchr(module, '/'))
-		return dso__new_map(module);
+		return dso__new_map(module, maps);
 
 	if (!module) {
 		pos = machine__kernel_map(host_machine);

@@ -170,7 +170,7 @@ struct map *get_target_map(const char *target, struct nsinfo *nsi, bool user)
 	if (user) {
 		struct map *map;
 
-		map = dso__new_map(target);
+		map = dso__new_map(target, NULL);
 		if (map && map->dso)
 			map->dso->nsinfo = nsinfo__get(nsi);
 		return map;

@@ -651,12 +651,13 @@ static int
 post_process_offline_probe_trace_events(struct probe_trace_event *tevs,
 					int ntevs, const char *pathname)
 {
+	struct maps *maps = machine__kernel_maps(host_machine);
 	struct map *map;
 	unsigned long stext = 0;
 	int i, ret = 0;
 
 	/* Prepare a map for offline binary */
-	map = dso__new_map(pathname);
+	map = dso__new_map(pathname, maps);
 	if (!map || get_text_start_address(pathname, &stext, NULL) < 0) {
 		pr_warning("Failed to get ELF symbols for %s\n", pathname);
 		return -EINVAL;

@@ -2111,7 +2112,8 @@ static int find_perf_probe_point_from_map(struct probe_trace_point *tp,
 	int ret = -ENOENT;
 
 	if (!is_kprobe) {
-		map = dso__new_map(tp->module);
+		struct maps *maps = machine__kernel_maps(host_machine);
+		map = dso__new_map(tp->module, maps);
 		if (!map)
 			goto out;
 		sym = map__find_symbol(map, addr);
tools/perf/util/symbol-elf.c +1 -1
index 6658fbf196e6..cc896ba85a99 100644
@@ -915,7 +915,7 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
 		curr_dso->kernel = dso->kernel;
 		curr_dso->long_name = dso->long_name;
 		curr_dso->long_name_len = dso->long_name_len;
-		curr_map = map__new2(start, curr_dso);
+		curr_map = map__new2(start, curr_dso, kmaps);
 		dso__put(curr_dso);
 		if (curr_map == NULL)
 			return -1;
tools/perf/util/symbol.c +4 -2
index 3b379b1296f1..d166f0fb258c 100644
@@ -856,7 +856,7 @@ static int maps__split_kallsyms(struct maps *kmaps, struct dso *dso, u64 delta,
 
 			ndso->kernel = dso->kernel;
 
-			curr_map = map__new2(pos->start, ndso);
+			curr_map = map__new2(pos->start, ndso, kmaps);
 			if (curr_map == NULL) {
 				dso__put(ndso);
 				return -1;

@@ -1144,6 +1144,7 @@ static int validate_kcore_addresses(const char *kallsyms_filename,
 
 struct kcore_mapfn_data {
 	struct dso *dso;
+	struct maps *kmaps;
 	struct list_head maps;
 };
 

@@ -1152,7 +1153,7 @@ static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data)
 	struct kcore_mapfn_data *md = data;
 	struct map *map;
 
-	map = map__new2(start, md->dso);
+	map = map__new2(start, md->dso, md->kmaps);
 	if (map == NULL)
 		return -ENOMEM;
 

@@ -1271,6 +1272,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 		return -EINVAL;
 
 	md.dso = dso;
+	md.kmaps = kmaps;
 	INIT_LIST_HEAD(&md.maps);
 
 	fd = open(kcore_filename, O_RDONLY);
Signature · 1 line
2.21.0
#21 lore raw

Re: [PATCH 12/12] perf maps: Set maps pointer in the kmap area for kernel maps

On Tue, Dec 17, 2019 at 11:48:28AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

Quoted 26 lines
tools/perf/util/machine.c +1 -1
+	machine->vmlinux_map = map__new2(0, kernel, &machine->kmaps);
 	if (machine->vmlinux_map == NULL)
 		return -1;

@@ -1098,7 +1097,6 @@ __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 	if (!kmap)
 		return -1;
 
-	kmap->kmaps = &machine->kmaps;
 	maps__insert(&machine->kmaps, map);
 
 	return 0;
tools/perf/util/map.c +1 -1
index fdd5bddb3075..a2cdfe62df94 100644
@@ -223,7 +223,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
  * they are loaded) and for vmlinux, where only after we load all the
  * symbols we'll know where it starts and ends.
  */
-struct map *map__new2(u64 start, struct dso *dso)
+struct map *map__new2(u64 start, struct dso *dso, struct maps *kmaps)
 {
 	struct map *map = calloc(1, (sizeof(*map) +
 				     (dso->kernel ? sizeof(struct kmap) : 0)));

tools/perf/util/map.c struct map *map__new2(u64 start, struct dso *dso) · line 234

Quoted 3 lines
tools/perf/util/map.c +0 -0
@@ -232,6 +232,19 @@ struct map *map__new2(u64 start, struct dso *dso)
 		 * ->end will be filled after we load all the symbols
 		 */
 		map__init(map, start, 0, 0, dso);

we are passing NULL for kmaps in some cases,
should we check it in here and warn?

		if (!WARN_ON_ONCE(!kmaps, "too bad..") && dso->kernel)

jirka

#22 lore raw

Re: [PATCH 12/12] perf maps: Set maps pointer in the kmap area for kernel maps

Em Wed, Dec 18, 2019 at 10:05:04AM +0100, Jiri Olsa escreveu:

Quoted 37 lines · Arnaldo Carvalho de Melo

SNIP

Quoted 26 lines
tools/perf/util/machine.c +1 -1
+	machine->vmlinux_map = map__new2(0, kernel, &machine->kmaps);
 	if (machine->vmlinux_map == NULL)
 		return -1;

@@ -1098,7 +1097,6 @@ __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 	if (!kmap)
 		return -1;
 
-	kmap->kmaps = &machine->kmaps;
 	maps__insert(&machine->kmaps, map);
 
 	return 0;
tools/perf/util/map.c +1 -1
index fdd5bddb3075..a2cdfe62df94 100644
@@ -223,7 +223,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
  * they are loaded) and for vmlinux, where only after we load all the
  * symbols we'll know where it starts and ends.
  */
-struct map *map__new2(u64 start, struct dso *dso)
+struct map *map__new2(u64 start, struct dso *dso, struct maps *kmaps)
 {
 	struct map *map = calloc(1, (sizeof(*map) +
 				     (dso->kernel ? sizeof(struct kmap) : 0)));

tools/perf/util/map.c struct map *map__new2(u64 start, struct dso *dso) · line 234

Quoted 3 lines
tools/perf/util/map.c +0 -0
@@ -232,6 +232,19 @@ struct map *map__new2(u64 start, struct dso *dso)
 		 * ->end will be filled after we load all the symbols
 		 */
 		map__init(map, start, 0, 0, dso);

we are passing NULL for kmaps in some cases,
should we check it in here and warn?

Quoted 1 line
		if (!WARN_ON_ONCE(!kmaps, "too bad..") && dso->kernel)

I'll add that, yeah

- Arnaldo

#23 lore raw

Re: [PATCH 12/12] perf maps: Set maps pointer in the kmap area for kernel maps

In reply to Jiri Olsa

Em Wed, Dec 18, 2019 at 10:05:04AM +0100, Jiri Olsa escreveu:

Quoted 37 lines · Arnaldo Carvalho de Melo

SNIP

Quoted 26 lines
(quoted diff) +1 -1
+	machine->vmlinux_map = map__new2(0, kernel, &machine->kmaps);
 	if (machine->vmlinux_map == NULL)
 		return -1;

@@ -1098,7 +1097,6 @@ __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 	if (!kmap)
 		return -1;
 
-	kmap->kmaps = &machine->kmaps;
 	maps__insert(&machine->kmaps, map);
 
 	return 0;
tools/perf/util/map.c +1 -1
index fdd5bddb3075..a2cdfe62df94 100644
@@ -223,7 +223,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
  * they are loaded) and for vmlinux, where only after we load all the
  * symbols we'll know where it starts and ends.
  */
-struct map *map__new2(u64 start, struct dso *dso)
+struct map *map__new2(u64 start, struct dso *dso, struct maps *kmaps)
 {
 	struct map *map = calloc(1, (sizeof(*map) +
 				     (dso->kernel ? sizeof(struct kmap) : 0)));

tools/perf/util/map.c struct map *map__new2(u64 start, struct dso *dso) · line 234

Quoted 3 lines
tools/perf/util/map.c +0 -0
@@ -232,6 +232,19 @@ struct map *map__new2(u64 start, struct dso *dso)
 		 * ->end will be filled after we load all the symbols
 		 */
 		map__init(map, start, 0, 0, dso);

we are passing NULL for kmaps in some cases,
should we check it in here and warn?

Quoted 1 line
		if (!WARN_ON_ONCE(!kmaps, "too bad..") && dso->kernel)

After looking at this some more, I retract this patch and instead the two-liner at the end of this message seems enough.

I forgot the dso->kernel is just set for the main kernel map, and what dso__process_kernel_symbol is doing is to split that map into chunks for the ELF sections for the main kernel map, as an extra tidbit of information, i.e. that symbol is in a specific main kernel section, so its dso->kernel continues being set and thus it expects the kmap area to be in place.

There is space for simplification in this old code, but for now the safest is just the patch below, agreed?

- Arnaldo

tools/perf/util/symbol-elf.c +3 -0
index 6658fbf196e6..1965aefccb02 100644
@@ -920,6 +920,9 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
 		if (curr_map == NULL)
 			return -1;
 
+		if (curr_dso->kernel)
+			map__kmap(curr_map)->kmaps = kmaps;
+
 		if (adjust_kernel_syms) {
 			curr_map->start  = shdr->sh_addr + ref_reloc(kmap);
 			curr_map->end	 = curr_map->start + shdr->sh_size;
#24 lore raw

Re: [PATCH 12/12] perf maps: Set maps pointer in the kmap area for kernel maps

Em Wed, Dec 18, 2019 at 03:22:54PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 3 lines

Em Wed, Dec 18, 2019 at 10:05:04AM +0100, Jiri Olsa escreveu:
After looking at this some more, I retract this patch and instead the two-liner at the end of this message seems enough.

So here is the full cset, I have not made changes to the other patches,

- Arnaldo

commit 178427a192bcbe3ad93dd0637c3dceaa3ccef31e

Author: Arnaldo Carvalho de Melo <[email protected]>
Date:   Wed Dec 18 15:23:14 2019 -0300

perf map: Set kmap->kmaps backpointer for main kernel map chunks

When a map is create to represent the main kernel area (vmlinux) with map__new2() we allocate an extra area to store a pointer to the 'struct maps' for the kernel maps, so that we can access that struct when loading ELF files or kallsyms, as we will need to split it in multiple maps, one per kernel module or ELF section (such as ".init.text").

So when map->dso->kernel is non-zero, it is expected that map__kmap(map)->kmaps to be set to the tree of kernel maps (modules, chunks of the main kernel, bpf progs put in place via PERF_RECORD_KSYMBOL, the main kernel).

This was not the case when we were splitting the main kernel into chunks for its ELF sections, which ended up making 'perf report --children' processing a perf.data file with callchains to trip on __map__is_kernel(), when we press ENTER to see the popup menu for main histogram entries that starts at a symbol in the ".init.text" ELF section, e.g.:

- 8.83% 0.00% swapper [kernel.vmlinux].init.text [k] start_kernel
start_kernel
cpu_startup_entry
do_idle
cpuidle_enter
cpuidle_enter_state
intel_idle

Fix it.

Cc Adrian Hunter <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
tools/perf/util/symbol-elf.c +3 -0
index 6658fbf196e6..1965aefccb02 100644
@@ -920,6 +920,9 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
 		if (curr_map == NULL)
 			return -1;
 
+		if (curr_dso->kernel)
+			map__kmap(curr_map)->kmaps = kmaps;
+
 		if (adjust_kernel_syms) {
 			curr_map->start  = shdr->sh_addr + ref_reloc(kmap);
 			curr_map->end	 = curr_map->start + shdr->sh_size;
#25 lore raw

Re: [PATCH 12/12] perf maps: Set maps pointer in the kmap area for kernel maps

Quoted 55 lines · Arnaldo Carvalho de Melo

Em Wed, Dec 18, 2019 at 03:22:54PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 3 lines

Em Wed, Dec 18, 2019 at 10:05:04AM +0100, Jiri Olsa escreveu:
After looking at this some more, I retract this patch and instead the two-liner at the end of this message seems enough.

So here is the full cset, I have not made changes to the other patches,

- Arnaldo

commit 178427a192bcbe3ad93dd0637c3dceaa3ccef31e

Author: Arnaldo Carvalho de Melo <[email protected]>
Date:   Wed Dec 18 15:23:14 2019 -0300

perf map: Set kmap->kmaps backpointer for main kernel map chunks

When a map is create to represent the main kernel area (vmlinux) with map__new2() we allocate an extra area to store a pointer to the 'struct maps' for the kernel maps, so that we can access that struct when loading ELF files or kallsyms, as we will need to split it in multiple maps, one per kernel module or ELF section (such as ".init.text").

So when map->dso->kernel is non-zero, it is expected that map__kmap(map)->kmaps to be set to the tree of kernel maps (modules, chunks of the main kernel, bpf progs put in place via PERF_RECORD_KSYMBOL, the main kernel).

This was not the case when we were splitting the main kernel into chunks for its ELF sections, which ended up making 'perf report --children' processing a perf.data file with callchains to trip on __map__is_kernel(), when we press ENTER to see the popup menu for main histogram entries that starts at a symbol in the ".init.text" ELF section, e.g.:

- 8.83% 0.00% swapper [kernel.vmlinux].init.text [k] start_kernel
start_kernel
cpu_startup_entry
do_idle
cpuidle_enter
cpuidle_enter_state
intel_idle

Fix it.

Cc Adrian Hunter <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Link https://lkml.kernel.org/n/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
tools/perf/util/symbol-elf.c +0 -0
index 6658fbf196e6..1965aefccb02 100644

tools/perf/util/symbol-elf.c static int dso__process_kernel_symbol(struct dso *dso, struct map *map, · line 928

Quoted 9 lines
tools/perf/util/symbol-elf.c +3 -0
@@ -920,6 +920,9 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
 		if (curr_map == NULL)
 			return -1;
 
+		if (curr_dso->kernel)
+			map__kmap(curr_map)->kmaps = kmaps;
+
 		if (adjust_kernel_syms) {
 			curr_map->start  = shdr->sh_addr + ref_reloc(kmap);
 			curr_map->end	 = curr_map->start + shdr->sh_size;

looks good ;-)

jirka

#26 lore raw

Re: [PATCH 12/12] perf maps: Set maps pointer in the kmap area for kernel maps

In reply to Arnaldo Carvalho de Melo

On Tue, Dec 17, 2019 at 11:48:28AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

Quoted 14 lines
tools/perf/util/machine.c +0 -1
@@ -1098,7 +1097,6 @@ __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 	if (!kmap)
 		return -1;
 
-	kmap->kmaps = &machine->kmaps;
 	maps__insert(&machine->kmaps, map);
 
 	return 0;
tools/perf/util/map.c +0 -0
index fdd5bddb3075..a2cdfe62df94 100644

tools/perf/util/map.c struct map *map__new(struct machine *machine, u64 start, u64 len, · line 226

Quoted 5 lines
tools/perf/util/map.c +1 -1
@@ -223,7 +223,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
  * they are loaded) and for vmlinux, where only after we load all the
  * symbols we'll know where it starts and ends.
  */
-struct map *map__new2(u64 start, struct dso *dso)
+struct map *map__new2(u64 start, struct dso *dso, struct maps *kmaps)

hum, how about map__new? kernel maps could go throught that as well, right?

jirka

#27 lore raw

Re: [PATCH 12/12] perf maps: Set maps pointer in the kmap area for kernel maps

Em Wed, Dec 18, 2019 at 10:07:07AM +0100, Jiri Olsa escreveu:

Quoted 12 lines · Arnaldo Carvalho de Melo

SNIP

Quoted 8 lines
tools/perf/util/machine.c +0 -1
@@ -1098,7 +1097,6 @@ __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 	if (!kmap)
 		return -1;
 
-	kmap->kmaps = &machine->kmaps;
 	maps__insert(&machine->kmaps, map);
Quoted 13 lines

tools/perf/util/map.c struct map *map__new(struct machine *machine, u64 start, u64 len, · line 226

Quoted 11 lines
tools/perf/util/machine.c +0 -0
 	return 0;
tools/perf/util/map.c +1 -1
index fdd5bddb3075..a2cdfe62df94 100644
@@ -223,7 +223,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
  * they are loaded) and for vmlinux, where only after we load all the
  * symbols we'll know where it starts and ends.
  */
-struct map *map__new2(u64 start, struct dso *dso)
+struct map *map__new2(u64 start, struct dso *dso, struct maps *kmaps)

hum, how about map__new? kernel maps could go throught that as well, right?

Nope, I even thought about renaming map__new2() to map__new_kmap() as only it is used to create kernel/modules maps, as is stated above in the comment just before map__new2().

map__new() is only called from machine__process_mmap_event() and machine__process_mmap2_event() and only after checking if it is not a kernel "mmap":

        if (sample->cpumode == PERF_RECORD_MISC_GUEST_KERNEL ||
            sample->cpumode == PERF_RECORD_MISC_KERNEL) {
                ret = machine__process_kernel_mmap_event(machine, event);
		if (ret < 0)
                        goto out_problem;
                return 0;
	...
	map = map__new(...);

- Arnaldo