linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/2] perf report: Change sort order by a specified event in group
@ 2019-12-10  8:32 Jin Yao
  2019-12-10  8:32 ` [PATCH v1 2/2] perf report: support hotkey to let user select any event in group for sorting Jin Yao
  0 siblings, 1 reply; 4+ messages in thread
From: Jin Yao @ 2019-12-10  8:32 UTC (permalink / raw)
  To: acme, jolsa, peterz, mingo, alexander.shishkin
  Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao

When performing "perf report --group", it shows the event group information
together. By default, the output is sorted by the first event in group.

It would be nice for user to select any event for sorting. This patch
introduces a new option "--group-sort-idx" to sort the output by the
event at the index n in event group.

For example,

Before:

  # perf report --group --stdio

  # To display the perf.data header info, please use --header/--header-only options.
  #
  #
  # Total Lost Samples: 0
  #
  # Samples: 12K of events 'cpu/instructions,period=2000003/, cpu/cpu-cycles,period=200003/, BR_MISP_RETIRED.ALL_BRANCHES:pp, cpu/event=0xc0,umask=1,cmask=1,
  # Event count (approx.): 6451235635
  #
  #                         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
       ...

After:

  # perf report --group --stdio --group-sort-idx 3

  # To display the perf.data header info, please use --header/--header-only options.
  #
  #
  # Total Lost Samples: 0
  #
  # Samples: 12K of events 'cpu/instructions,period=2000003/, cpu/cpu-cycles,period=200003/, BR_MISP_RETIRED.ALL_BRANCHES:pp, cpu/event=0xc0,umask=1,cmask=1,
  # Event count (approx.): 6451235635
  #
  #                         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
       0.00%   0.00%   0.00%   0.02%  mgen       [kernel.kallsyms]        [k] scheduler_tick

Now the output is sorted by the fourth event in group.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/Documentation/perf-report.txt |   4 +
 tools/perf/builtin-report.c              |  10 +++
 tools/perf/ui/hist.c                     | 110 +++++++++++++++++++----
 tools/perf/util/symbol_conf.h            |   1 +
 4 files changed, 110 insertions(+), 15 deletions(-)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index 8dbe2119686a..9ade613ef020 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -371,6 +371,10 @@ OPTIONS
 	Show event group information together. It forces group output also
 	if there are no groups defined in data file.
 
+--group-sort-idx::
+	Sort the output by the event at the index n in group. If n is invalid,
+	sort by the first event. WARNING: This should be used with --group.
+
 --demangle::
 	Demangle symbol names to human readable form. It's enabled by default,
 	disable with --no-demangle.
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 387311c67264..729cf7611d8a 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1210,6 +1210,10 @@ int cmd_report(int argc, const char **argv)
 		    "Show a column with the sum of periods"),
 	OPT_BOOLEAN_SET(0, "group", &symbol_conf.event_group, &report.group_set,
 		    "Show event group information together"),
+	OPT_INTEGER(0, "group-sort-idx", &symbol_conf.group_sort_idx,
+		    "Sort the output by the event at the index n in group. "
+		    "If n is invalid, sort by the first event. "
+		    "WARNING: use only with --group."),
 	OPT_CALLBACK_NOOPT('b', "branch-stack", &branch_mode, "",
 		    "use branch records for per branch histogram filling",
 		    parse_branch_mode),
@@ -1302,6 +1306,12 @@ int cmd_report(int argc, const char **argv)
 		return -EINVAL;
 	}
 
+	if (symbol_conf.group_sort_idx &&
+	    (!symbol_conf.event_group || !report.group_set)) {
+		parse_options_usage(NULL, options, "group-sort-idx", 0);
+		return -EINVAL;
+	}
+
 	if (report.inverted_callchain)
 		callchain_param.order = ORDER_CALLER;
 	if (symbol_conf.cumulate_callchain && !callchain_param.order_set)
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index f73675500061..8991beca60cb 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -151,15 +151,106 @@ static int field_cmp(u64 field_a, u64 field_b)
 	return 0;
 }
 
+static int pair_fields_alloc(struct hist_entry *a, struct hist_entry *b,
+			     hpp_field_fn get_field, int nr_members,
+			     u64 **fields_a, u64 **fields_b)
+{
+	struct evsel *evsel;
+	struct hist_entry *pair;
+	u64 *fa, *fb;
+	int ret = -1;
+
+	fa = calloc(nr_members, sizeof(*fa));
+	fb = calloc(nr_members, sizeof(*fb));
+
+	if (!fa || !fb)
+		goto out;
+
+	list_for_each_entry(pair, &a->pairs.head, pairs.node) {
+		evsel = hists_to_evsel(pair->hists);
+		fa[perf_evsel__group_idx(evsel)] = get_field(pair);
+	}
+
+	list_for_each_entry(pair, &b->pairs.head, pairs.node) {
+		evsel = hists_to_evsel(pair->hists);
+		fb[perf_evsel__group_idx(evsel)] = get_field(pair);
+	}
+
+	*fields_a = fa;
+	*fields_b = fb;
+	ret = 0;
+
+out:
+	if (ret != 0) {
+		free(fa);
+		free(fb);
+		*fields_a = NULL;
+		*fields_b = NULL;
+	}
+
+	return ret;
+}
+
+static int __hpp__group_sort_idx(struct hist_entry *a, struct hist_entry *b,
+				 hpp_field_fn get_field, int idx)
+{
+	struct evsel *evsel = hists_to_evsel(a->hists);
+	u64 *fields_a, *fields_b;
+	int cmp, nr_members, ret, i;
+
+	cmp = field_cmp(get_field(a), get_field(b));
+	if (!perf_evsel__is_group_event(evsel))
+		return cmp;
+
+	nr_members = evsel->core.nr_members;
+	ret = pair_fields_alloc(a, b, get_field, nr_members,
+			      &fields_a, &fields_b);
+	if (ret) {
+		ret = cmp;
+		goto out;
+	}
+
+	for (i = 1; i < nr_members; i++) {
+		if (i == idx) {
+			ret = field_cmp(fields_a[i], fields_b[i]);
+			if (ret)
+				goto out;
+		}
+	}
+
+	if (cmp) {
+		ret = cmp;
+		goto out;
+	}
+
+	for (i = 1; i < nr_members; i++) {
+		if (i != idx) {
+			ret = field_cmp(fields_a[i], fields_b[i]);
+			if (ret)
+				goto out;
+		}
+	}
+
+out:
+	free(fields_a);
+	free(fields_b);
+
+	return ret;
+}
+
 static int __hpp__sort(struct hist_entry *a, struct hist_entry *b,
 		       hpp_field_fn get_field)
 {
 	s64 ret;
 	int i, nr_members;
 	struct evsel *evsel;
-	struct hist_entry *pair;
 	u64 *fields_a, *fields_b;
 
+	if (symbol_conf.group_sort_idx && symbol_conf.event_group) {
+		return __hpp__group_sort_idx(a, b, get_field,
+					     symbol_conf.group_sort_idx);
+	}
+
 	ret = field_cmp(get_field(a), get_field(b));
 	if (ret || !symbol_conf.event_group)
 		return ret;
@@ -169,22 +260,11 @@ static int __hpp__sort(struct hist_entry *a, struct hist_entry *b,
 		return ret;
 
 	nr_members = evsel->core.nr_members;
-	fields_a = calloc(nr_members, sizeof(*fields_a));
-	fields_b = calloc(nr_members, sizeof(*fields_b));
-
-	if (!fields_a || !fields_b)
+	i = pair_fields_alloc(a, b, get_field, nr_members,
+			      &fields_a, &fields_b);
+	if (i)
 		goto out;
 
-	list_for_each_entry(pair, &a->pairs.head, pairs.node) {
-		evsel = hists_to_evsel(pair->hists);
-		fields_a[perf_evsel__group_idx(evsel)] = get_field(pair);
-	}
-
-	list_for_each_entry(pair, &b->pairs.head, pairs.node) {
-		evsel = hists_to_evsel(pair->hists);
-		fields_b[perf_evsel__group_idx(evsel)] = get_field(pair);
-	}
-
 	for (i = 1; i < nr_members; i++) {
 		ret = field_cmp(fields_a[i], fields_b[i]);
 		if (ret)
diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
index 10f1ec3e0349..b916afb95ec5 100644
--- a/tools/perf/util/symbol_conf.h
+++ b/tools/perf/util/symbol_conf.h
@@ -73,6 +73,7 @@ struct symbol_conf {
 	const char	*symfs;
 	int		res_sample;
 	int		pad_output_len_dso;
+	int		group_sort_idx;
 };
 
 extern struct symbol_conf symbol_conf;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v1 2/2] perf report: support hotkey to let user select any event in group for sorting
  2019-12-10  8:32 [PATCH v1 1/2] perf report: Change sort order by a specified event in group Jin Yao
@ 2019-12-10  8:32 ` Jin Yao
  2019-12-10 16:00   ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 4+ messages in thread
From: Jin Yao @ 2019-12-10  8:32 UTC (permalink / raw)
  To: acme, jolsa, peterz, mingo, alexander.shishkin
  Cc: Linux-kernel, ak, kan.liang, yao.jin, Jin Yao

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@linux.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 '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;
+			}
+
+			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
 
 #endif /* _PERF_KEYSYMS_H_ */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v1 2/2] perf report: support hotkey to let user select any event in group for sorting
  2019-12-10  8:32 ` [PATCH v1 2/2] perf report: support hotkey to let user select any event in group for sorting Jin Yao
@ 2019-12-10 16:00   ` Arnaldo Carvalho de Melo
  2019-12-11  7:06     ` Jin, Yao
  0 siblings, 1 reply; 4+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-12-10 16:00 UTC (permalink / raw)
  To: Jin Yao
  Cc: jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin

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@linux.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@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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v1 2/2] perf report: support hotkey to let user select any event in group for sorting
  2019-12-10 16:00   ` Arnaldo Carvalho de Melo
@ 2019-12-11  7:06     ` Jin, Yao
  0 siblings, 0 replies; 4+ messages in thread
From: Jin, Yao @ 2019-12-11  7:06 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: jolsa, peterz, mingo, alexander.shishkin, Linux-kernel, ak,
	kan.liang, yao.jin



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@linux.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@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
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-12-11  7:06 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-10  8:32 [PATCH v1 1/2] perf report: Change sort order by a specified event in group Jin Yao
2019-12-10  8:32 ` [PATCH v1 2/2] perf report: support hotkey to let user select any event in group for sorting Jin Yao
2019-12-10 16:00   ` Arnaldo Carvalho de Melo
2019-12-11  7:06     ` Jin, Yao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).