| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f3e93abb-e840-281e-0e3a-feb959e897a0@linux.intel.com>
Date: Wed, 11 Dec 2019 15:06:20 +0800
From: "Jin, Yao" <yao.jin@...ux.intel.com>
To: Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc: jolsa@...nel.org, peterz@...radead.org, mingo@...hat.com,
alexander.shishkin@...ux.intel.com, Linux-kernel@...r.kernel.org,
ak@...ux.intel.com, kan.liang@...el.com, yao.jin@...el.com
Subject: Re: [PATCH v1 2/2] perf report: support hotkey to let user select any
event in group for sorting
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@...ux.intel.com>
>> ---
>> 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@...co 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@...co 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
>
Powered by blists - more mailing lists