Re: [PATCH v1 2/2] perf report: support hotkey to let user select any event in group for sorting
From: Jin, Yao
Date: Wed Dec 11 2019 - 02:06:31 EST
On 12/11/2019 12:00 AM, Arnaldo Carvalho de Melo wrote:
Em Tue, Dec 10, 2019 at 04:32:07PM +0800, Jin Yao escreveu:
When performing "perf report --group", it shows the event group information
together. In previous patch, we have supported a new option "--group-sort-idx"
to sort the output by the event at the index n in event group.
It would be nice if we can use a hotkey in browser to select a event
for sorting.
For example,
# perf report --group
Samples: 12K of events 'cpu/instructions,period=2000003/, cpu/cpu-cycles,period=200003/, ...
Overhead Command Shared Object Symbol
92.19% 98.68% 0.00% 93.30% mgen mgen [.] LOOP1
3.12% 0.29% 0.00% 0.16% gsd-color libglib-2.0.so.0.5600.4 [.] 0x0000000000049515
1.56% 0.03% 0.00% 0.04% gsd-color libglib-2.0.so.0.5600.4 [.] 0x00000000000494b7
1.56% 0.01% 0.00% 0.00% gsd-color libglib-2.0.so.0.5600.4 [.] 0x00000000000494ce
1.56% 0.00% 0.00% 0.00% mgen [kernel.kallsyms] [k] task_tick_fair
0.00% 0.15% 0.00% 0.04% perf [kernel.kallsyms] [k] smp_call_function_single
0.00% 0.13% 0.00% 6.08% swapper [kernel.kallsyms] [k] intel_idle
0.00% 0.03% 0.00% 0.00% gsd-color libglib-2.0.so.0.5600.4 [.] g_main_context_check
0.00% 0.03% 0.00% 0.00% swapper [kernel.kallsyms] [k] apic_timer_interrupt
0.00% 0.03% 0.00% 0.00% swapper [kernel.kallsyms] [k] check_preempt_curr
When user press hotkey '3' (event index, starting from 0), it indicates
to sort output by the fourth event in group.
Samples: 12K of events 'cpu/instructions,period=2000003/, cpu/cpu-cycles,period=200003/, ...
Overhead Command Shared Object Symbol
92.19% 98.68% 0.00% 93.30% mgen mgen [.] LOOP1
0.00% 0.13% 0.00% 6.08% swapper [kernel.kallsyms] [k] intel_idle
3.12% 0.29% 0.00% 0.16% gsd-color libglib-2.0.so.0.5600.4 [.] 0x0000000000049515
0.00% 0.00% 0.00% 0.06% swapper [kernel.kallsyms] [k] hrtimer_start_range_ns
1.56% 0.03% 0.00% 0.04% gsd-color libglib-2.0.so.0.5600.4 [.] 0x00000000000494b7
0.00% 0.15% 0.00% 0.04% perf [kernel.kallsyms] [k] smp_call_function_single
0.00% 0.00% 0.00% 0.02% mgen [kernel.kallsyms] [k] update_curr
0.00% 0.00% 0.00% 0.02% mgen [kernel.kallsyms] [k] apic_timer_interrupt
0.00% 0.00% 0.00% 0.02% mgen [kernel.kallsyms] [k] native_apic_msr_eoi_write
0.00% 0.00% 0.00% 0.02% mgen [kernel.kallsyms] [k] __update_load_avg_se
Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
---
tools/perf/builtin-report.c | 4 ++--
tools/perf/ui/browsers/hists.c | 21 ++++++++++++++++++++-
tools/perf/ui/keysyms.h | 1 +
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 729cf7611d8a..02178fc54d67 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -635,7 +635,7 @@ static int report__browse_hists(struct report *rep)
* Usually "ret" is the last pressed key, and we only
* care if the key notifies us to switch data file.
*/
- if (ret != K_SWITCH_INPUT_DATA)
+ if (ret != K_SWITCH_INPUT_DATA && ret != K_RELOAD)
ret = 0;
break;
case 2:
@@ -1538,7 +1538,7 @@ int cmd_report(int argc, const char **argv)
sort__setup_elide(stdout);
ret = __cmd_report(&report);
- if (ret == K_SWITCH_INPUT_DATA) {
+ if (ret == K_SWITCH_INPUT_DATA || ret == K_RELOAD) {
perf_session__delete(session);
goto repeat;
} else
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index d4d3558fdef4..1de2456f27c3 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2876,7 +2876,8 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
"s Switch to another data file in PWD\n"
"t Zoom into current Thread\n"
"V Verbose (DSO names in callchains, etc)\n"
- "/ Filter symbol by name";
+ "/ Filter symbol by name\n"
+ "0-9 Sort by event n in group";
static const char top_help[] = HIST_BROWSER_HELP_COMMON
"P Print histograms to perf.hist.N\n"
"t Zoom into current Thread\n"
@@ -2937,6 +2938,24 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
* go to the next or previous
*/
goto out_free_stack;
+ case '0':
case '0' ... '9':
works as well, for instance, in the kernel sources we have:
[acme@quaco perf]$ grep 'case.*\.\..*:' */*.c
block/bio.c: case 2 ... 4:
block/bio.c: case 5 ... 16:
block/bio.c: case 17 ... 64:
block/bio.c: case 65 ... 128:
block/bio.c: case 129 ... BIO_MAX_PAGES:
block/sed-opal.c: case 0xbfff ... 0xffff:
fs/binfmt_elf.c: case PT_LOPROC ... PT_HIPROC:
fs/binfmt_elf.c: case PT_LOPROC ... PT_HIPROC:
kernel/audit.c: case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG:
kernel/audit.c: case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2:
kernel/audit.c: case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG:
kernel/audit.c: case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2:
[acme@quaco perf]$
Yes, your code is much better. :)
+ case '1':
+ case '2':
+ case '3':
+ case '4':
+ case '5':
+ case '6':
+ case '7':
+ case '8':
+ case '9':
+ symbol_conf.group_sort_idx = key - '0';
+ if (!symbol_conf.event_group ||
+ symbol_conf.group_sort_idx >= evsel->core.nr_members) {
+ continue;
Better to put something on the helpline as:
The max event group index to sort is N!
And if symbol_conf.event_group isn't set, something like:
Sort by index only available with group events!
Good idea. I will add warning such as:
"Max event group index to sort is 3 (index from 0 to 3)"
+ }
+
+ key = K_RELOAD;
+ goto out_free_stack;
case 'a':
if (!hists__has(hists, sym)) {
ui_browser__warning(&browser->b, delay_secs * 2,
diff --git a/tools/perf/ui/keysyms.h b/tools/perf/ui/keysyms.h
index fbfac29077f2..04cc4e5c031f 100644
--- a/tools/perf/ui/keysyms.h
+++ b/tools/perf/ui/keysyms.h
@@ -25,5 +25,6 @@
#define K_ERROR -2
#define K_RESIZE -3
#define K_SWITCH_INPUT_DATA -4
+#define K_RELOAD -5
Can you please split the K_RELOAD logic from this patch?
OK, I will split the patch.
Thanks
Jin Yao
#endif /* _PERF_KEYSYMS_H_ */
--
2.17.1