Re: [PATCH v1 2/2] perf report: support hotkey to let user select any event in group for sorting

From: Arnaldo Carvalho de Melo
Date: Tue Dec 10 2019 - 11:00:35 EST

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]$

> + 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!

> + }
> +
> + 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?

> #endif /* _PERF_KEYSYMS_H_ */
> --
> 2.17.1

--

- Arnaldo