[PATCH v7 0/3] perf report: Support sorting by a given event in group

[PATCH v7 0/3] perf report: Support sorting by a given event in group

[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.

The patch 1/3 introduces a new option "--group-sort-idx" to sort the
output by the event at the index n in event group.

The patch 2/3 creates a new key K_RELOAD to reload the browser.

The patch 3/3 supports hotkeys in browser to select a event to
sort.

 v7:
 ---
 v6 was posted two months ago and all comments were fixed.

 v7 just rebases to perf/core, no other change.

Jin Yao (3):
  perf report: Change sort order by a specified event in group
  perf report: Support a new key to reload the browser
  perf report: support hotkey to let user select any event for sorting

 tools/perf/Documentation/perf-report.txt |   5 ++
 tools/perf/builtin-report.c              |  16 +++-
 tools/perf/ui/browsers/hists.c           |  29 ++++++-
 tools/perf/ui/hist.c                     | 104 +++++++++++++++++++----
 tools/perf/ui/keysyms.h                  |   1 +
 tools/perf/util/hist.h                   |   1 +
 tools/perf/util/symbol_conf.h            |   1 +
 7 files changed, 138 insertions(+), 19 deletions(-)

-- 
2.17.1

[PATCH v7 1/3] perf report: Change sort order by a specified event in group

[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.

 v7:
 ---
 Rebase to latest perf/core, no other change.

 v6:
 ---
 No change.

 v5:
 ---
 No change in v5.
 v4 has been ACKed by Jiri.

 v4:
 ---
 1. Update Documentation/perf-report.txt to mention
    '--group-sort-idx' support multiple groups with different
    amount of events and it should be used on grouped events.

 2. Update __hpp__group_sort_idx(), just return when the
    idx is out of limit.

 3. Return failure on symbol_conf.group_sort_idx && !session->evlist->nr_groups.
    So now we don't need to use together with --group.

 v3:
 ---
 Refine the code in __hpp__group_sort_idx().

 Before:
   for (i = 1; i < nr_members; i++) {
        if (i == idx) {
                ret = field_cmp(fields_a[i], fields_b[i]);
                if (ret)
                        goto out;
        }
   }

 After:
   if (idx >= 1 && idx < nr_members) {
        ret = field_cmp(fields_a[idx], fields_b[idx]);
        if (ret)
                goto out;
   }

 v2:
 ---
 No change

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

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index db61f16ffa56..c964d4bd419d 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -377,6 +377,11 @@ 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. It can support multiple groups with different
+	amount of events. WARNING: This should be used on grouped events.
+
 --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 9483b3f0cae3..862c7f8853dc 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1216,6 +1216,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: should be used on grouped events."),
 	OPT_CALLBACK_NOOPT('b', "branch-stack", &branch_mode, "",
 		    "use branch records for per branch histogram filling",
 		    parse_branch_mode),
@@ -1358,6 +1362,12 @@ int cmd_report(int argc, const char **argv)
 
 	setup_forced_leader(&report, session->evlist);
 
+	if (symbol_conf.group_sort_idx && !session->evlist->nr_groups) {
+		parse_options_usage(NULL, options, "group-sort-idx", 0);
+		ret = -EINVAL;
+		goto error;
+	}
+
 	if (itrace_synth_opts.last_branch)
 		has_br_stack = true;
 
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index f73675500061..35224b366305 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -151,15 +151,100 @@ 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;
+	if (idx < 1 || idx >= nr_members)
+		return cmp;
+
+	ret = pair_fields_alloc(a, b, get_field, nr_members,
+				&fields_a, &fields_b);
+	if (ret) {
+		ret = cmp;
+		goto out;
+	}
+
+	ret = field_cmp(fields_a[idx], fields_b[idx]);
+	if (ret)
+		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 +254,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

[PATCH v7 2/3] perf report: Support a new key to reload the browser

[Jin Yao]

Sometimes we may need to reload the browser to update the output since
some options are changed.

This patch creates a new key K_RELOAD. Once the __cmd_report() returns
K_RELOAD, it would repeat the whole process, such as, read samples from
data file, sort the data and display in the browser.

 v7:
 ---
 Rebase to perf/core, no other change.

 v6:
 ---
 No change.

 v5:
 ---
 1. Fix the 'make NO_SLANG=1' error. Define K_RELOAD in util/hist.h.
 2. Skip setup_sorting() in repeat path if last key is K_RELOAD.

 v4:
 ---
 Need to quit in perf_evsel_menu__run if key is K_RELOAD.

 v3:
 ---
 No change.

 v2:
 ---
 This is a new patch created in v2.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/builtin-report.c    | 6 +++---
 tools/perf/ui/browsers/hists.c | 1 +
 tools/perf/ui/keysyms.h        | 1 +
 tools/perf/util/hist.h         | 1 +
 4 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 862c7f8853dc..842ef92c3598 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:
@@ -1469,7 +1469,7 @@ int cmd_report(int argc, const char **argv)
 		sort_order = sort_tmp;
 	}
 
-	if ((last_key != K_SWITCH_INPUT_DATA) &&
+	if ((last_key != K_SWITCH_INPUT_DATA && last_key != K_RELOAD) &&
 	    (setup_sorting(session->evlist) < 0)) {
 		if (sort_order)
 			parse_options_usage(report_usage, options, "s", 1);
@@ -1548,7 +1548,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);
 		last_key = K_SWITCH_INPUT_DATA;
 		goto repeat;
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f36dee499320..7c091fa51a5c 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3440,6 +3440,7 @@ static int perf_evsel_menu__run(struct evsel_menu *menu,
 					pos = perf_evsel__prev(pos);
 				goto browse_hists;
 			case K_SWITCH_INPUT_DATA:
+			case K_RELOAD:
 			case 'q':
 			case CTRL('c'):
 				goto out;
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_ */
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 0aa63aeb58ec..bb994e030495 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -536,6 +536,7 @@ static inline int block_hists_tui_browse(struct block_hist *bh __maybe_unused,
 #define K_LEFT  -1000
 #define K_RIGHT -2000
 #define K_SWITCH_INPUT_DATA -3000
+#define K_RELOAD -4000
 #endif
 
 unsigned int hists__sort_list_width(struct hists *hists);
-- 
2.17.1

[PATCH v7 3/3] perf report: support hotkey to let user select any event for sorting

[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
to sort.

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 forth 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

 v7:
 ---
 Rebase to perf/core, no other change.

 v6:
 ---
 Jiri provided a good improvement to eliminate unneeded refresh.
 This improvement is added to v6.

 v5:
 ---
 No change

 v4:
 ---
 No change

 v3:
 ---
 No change

 v2:
 ---
 1. Report warning at helpline when index is invalid.
 2. Report warning at helpline when it's not group event.
 3. Use "case '0' ... '9'" to refine the code
 4. Split K_RELOAD implementation to another patch.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/ui/browsers/hists.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 7c091fa51a5c..fa4548459c46 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2964,7 +2964,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"
@@ -3025,6 +3026,31 @@ static int perf_evsel__hists_browse(struct evsel *evsel, int nr_events,
 			 * go to the next or previous
 			 */
 			goto out_free_stack;
+		case '0' ... '9':
+			if (!symbol_conf.event_group ||
+			    evsel->core.nr_members < 2) {
+				snprintf(buf, sizeof(buf),
+					 "Sort by index only available with group events!");
+				helpline = buf;
+				continue;
+			}
+
+			if (key - '0' == symbol_conf.group_sort_idx)
+				continue;
+
+			symbol_conf.group_sort_idx = key - '0';
+
+			if (symbol_conf.group_sort_idx >= evsel->core.nr_members) {
+				snprintf(buf, sizeof(buf),
+					 "Max event group index to sort is %d (index from 0 to %d)",
+					 evsel->core.nr_members - 1,
+					 evsel->core.nr_members - 1);
+				helpline = buf;
+				continue;
+			}
+
+			key = K_RELOAD;
+			goto out_free_stack;
 		case 'a':
 			if (!hists__has(hists, sym)) {
 				ui_browser__warning(&browser->b, delay_secs * 2,
-- 
2.17.1

Re: [PATCH v7 1/3] perf report: Change sort order by a specified event in group

[Arnaldo Carvalho de Melo]

Em Thu, Feb 20, 2020 at 09:36:14AM +0800, Jin Yao escreveu:
> 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:

Tested and applied, I also did the following simplifications, to try and
follow naming conventions and use less lines to do the same thing:

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 35224b366305..025f4c7f96bf 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -151,44 +151,35 @@ 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)
+static int hist_entry__new_pair(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;
+	u64 *fa = calloc(nr_members, sizeof(*fa)),
+	    *fb = calloc(nr_members, sizeof(*fb));
 	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;
+		goto out_free;
 
 	list_for_each_entry(pair, &a->pairs.head, pairs.node) {
-		evsel = hists_to_evsel(pair->hists);
+		struct evsel *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);
+		struct evsel *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;
+	return 0;
+out_free:
+	free(fa);
+	free(fb);
+	*fields_a = *fields_b = NULL;
+	return -1;
 }
 
 static int __hpp__group_sort_idx(struct hist_entry *a, struct hist_entry *b,
@@ -206,8 +197,7 @@ static int __hpp__group_sort_idx(struct hist_entry *a, struct hist_entry *b,
 	if (idx < 1 || idx >= nr_members)
 		return cmp;
 
-	ret = pair_fields_alloc(a, b, get_field, nr_members,
-				&fields_a, &fields_b);
+	ret = hist_entry__new_pair(a, b, get_field, nr_members, &fields_a, &fields_b);
 	if (ret) {
 		ret = cmp;
 		goto out;
@@ -254,8 +244,7 @@ static int __hpp__sort(struct hist_entry *a, struct hist_entry *b,
 		return ret;
 
 	nr_members = evsel->core.nr_members;
-	i = pair_fields_alloc(a, b, get_field, nr_members,
-			      &fields_a, &fields_b);
+	i = hist_entry__new_pair(a, b, get_field, nr_members, &fields_a, &fields_b);
 	if (i)
 		goto out;
 

Re: [PATCH v7 2/3] perf report: Support a new key to reload the browser

[Arnaldo Carvalho de Melo]

Em Thu, Feb 20, 2020 at 09:36:15AM +0800, Jin Yao escreveu:
> Sometimes we may need to reload the browser to update the output since
> some options are changed.
> 
> This patch creates a new key K_RELOAD. Once the __cmd_report() returns
> K_RELOAD, it would repeat the whole process, such as, read samples from
> data file, sort the data and display in the browser.
> 
>  v7:
>  ---
>  Rebase to perf/core, no other change.
> 
>  v6:
>  ---
>  No change.
> 
>  v5:
>  ---
>  1. Fix the 'make NO_SLANG=1' error. Define K_RELOAD in util/hist.h.
>  2. Skip setup_sorting() in repeat path if last key is K_RELOAD.
> 
>  v4:
>  ---
>  Need to quit in perf_evsel_menu__run if key is K_RELOAD.
> 
>  v3:
>  ---
>  No change.
> 
>  v2:
>  ---
>  This is a new patch created in v2.
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/builtin-report.c    | 6 +++---
>  tools/perf/ui/browsers/hists.c | 1 +
>  tools/perf/ui/keysyms.h        | 1 +
>  tools/perf/util/hist.h         | 1 +
>  4 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 862c7f8853dc..842ef92c3598 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:
> @@ -1469,7 +1469,7 @@ int cmd_report(int argc, const char **argv)
>  		sort_order = sort_tmp;
>  	}
>  
> -	if ((last_key != K_SWITCH_INPUT_DATA) &&
> +	if ((last_key != K_SWITCH_INPUT_DATA && last_key != K_RELOAD) &&
>  	    (setup_sorting(session->evlist) < 0)) {
>  		if (sort_order)
>  			parse_options_usage(report_usage, options, "s", 1);
> @@ -1548,7 +1548,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);
>  		last_key = K_SWITCH_INPUT_DATA;

Are you sure this shouldn't be:
	
		last_key = ret;

?

I'm applying it to test now anyway,

- Arnaldo

>  		goto repeat;
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index f36dee499320..7c091fa51a5c 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3440,6 +3440,7 @@ static int perf_evsel_menu__run(struct evsel_menu *menu,
>  					pos = perf_evsel__prev(pos);
>  				goto browse_hists;
>  			case K_SWITCH_INPUT_DATA:
> +			case K_RELOAD:
>  			case 'q':
>  			case CTRL('c'):
>  				goto out;
> 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_ */
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 0aa63aeb58ec..bb994e030495 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -536,6 +536,7 @@ static inline int block_hists_tui_browse(struct block_hist *bh __maybe_unused,
>  #define K_LEFT  -1000
>  #define K_RIGHT -2000
>  #define K_SWITCH_INPUT_DATA -3000
> +#define K_RELOAD -4000
>  #endif
>  
>  unsigned int hists__sort_list_width(struct hists *hists);
> -- 
> 2.17.1
> 

-- 

- Arnaldo

Re: [PATCH v7 0/3] perf report: Support sorting by a given event in group

[Arnaldo Carvalho de Melo]

Em Thu, Feb 20, 2020 at 09:36:13AM +0800, Jin Yao escreveu:
> 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.
> 
> The patch 1/3 introduces a new option "--group-sort-idx" to sort the
> output by the event at the index n in event group.
> 
> The patch 2/3 creates a new key K_RELOAD to reload the browser.
> 
> The patch 3/3 supports hotkeys in browser to select a event to
> sort.

Thanks, applied.

- Arnaldo
 
>  v7:
>  ---
>  v6 was posted two months ago and all comments were fixed.
> 
>  v7 just rebases to perf/core, no other change.
> 
> Jin Yao (3):
>   perf report: Change sort order by a specified event in group
>   perf report: Support a new key to reload the browser
>   perf report: support hotkey to let user select any event for sorting
> 
>  tools/perf/Documentation/perf-report.txt |   5 ++
>  tools/perf/builtin-report.c              |  16 +++-
>  tools/perf/ui/browsers/hists.c           |  29 ++++++-
>  tools/perf/ui/hist.c                     | 104 +++++++++++++++++++----
>  tools/perf/ui/keysyms.h                  |   1 +
>  tools/perf/util/hist.h                   |   1 +
>  tools/perf/util/symbol_conf.h            |   1 +
>  7 files changed, 138 insertions(+), 19 deletions(-)
> 
> -- 
> 2.17.1
> 

-- 

- Arnaldo

Re: [PATCH v7 0/3] perf report: Support sorting by a given event in group

[Arnaldo Carvalho de Melo]

Em Wed, Mar 18, 2020 at 04:01:16PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Feb 20, 2020 at 09:36:13AM +0800, Jin Yao escreveu:
> > 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.
> > 
> > The patch 1/3 introduces a new option "--group-sort-idx" to sort the
> > output by the event at the index n in event group.
> > 
> > The patch 2/3 creates a new key K_RELOAD to reload the browser.
> > 
> > The patch 3/3 supports hotkeys in browser to select a event to
> > sort.
> 
> Thanks, applied.

Doesn't work with 'perf top', should, investigating,

- Arnaldo
 
> - Arnaldo
>  
> >  v7:
> >  ---
> >  v6 was posted two months ago and all comments were fixed.
> > 
> >  v7 just rebases to perf/core, no other change.
> > 
> > Jin Yao (3):
> >   perf report: Change sort order by a specified event in group
> >   perf report: Support a new key to reload the browser
> >   perf report: support hotkey to let user select any event for sorting
> > 
> >  tools/perf/Documentation/perf-report.txt |   5 ++
> >  tools/perf/builtin-report.c              |  16 +++-
> >  tools/perf/ui/browsers/hists.c           |  29 ++++++-
> >  tools/perf/ui/hist.c                     | 104 +++++++++++++++++++----
> >  tools/perf/ui/keysyms.h                  |   1 +
> >  tools/perf/util/hist.h                   |   1 +
> >  tools/perf/util/symbol_conf.h            |   1 +
> >  7 files changed, 138 insertions(+), 19 deletions(-)
> > 
> > -- 
> > 2.17.1
> > 
> 
> -- 
> 
> - Arnaldo

-- 

- Arnaldo

Re: [PATCH v7 0/3] perf report: Support sorting by a given event in group

[Arnaldo Carvalho de Melo]

Em Wed, Mar 18, 2020 at 04:03:25PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Mar 18, 2020 at 04:01:16PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Feb 20, 2020 at 09:36:13AM +0800, Jin Yao escreveu:
> > > 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.
> > > 
> > > The patch 1/3 introduces a new option "--group-sort-idx" to sort the
> > > output by the event at the index n in event group.
> > > 
> > > The patch 2/3 creates a new key K_RELOAD to reload the browser.
> > > 
> > > The patch 3/3 supports hotkeys in browser to select a event to
> > > sort.
> > 
> > Thanks, applied.
> 
> Doesn't work with 'perf top', should, investigating,

So needs a bit of work, but is doable and would be a great addition,
since we do support:

  # perf top --group -e instructions,cycles,cache-misses

And we should strive to make 'perf top' to be a perf.data-less, dynamic
version of 'perf record + perf report'.

Can you please take a look at supporting this? And that --group-sort-idx
too,

I'll push my perf/core branch after a few tests,

Thanks,

- Arnaldo

Re: [PATCH v7 1/3] perf report: Change sort order by a specified event in group

[Jin, Yao]

On 3/19/2020 2:45 AM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Feb 20, 2020 at 09:36:14AM +0800, Jin Yao escreveu:
>> 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:
> 
> Tested and applied, I also did the following simplifications, to try and
> follow naming conventions and use less lines to do the same thing:
> 
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 35224b366305..025f4c7f96bf 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -151,44 +151,35 @@ 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)
> +static int hist_entry__new_pair(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;
> +	u64 *fa = calloc(nr_members, sizeof(*fa)),
> +	    *fb = calloc(nr_members, sizeof(*fb));
>   	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;
> +		goto out_free;
>   
>   	list_for_each_entry(pair, &a->pairs.head, pairs.node) {
> -		evsel = hists_to_evsel(pair->hists);
> +		struct evsel *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);
> +		struct evsel *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;
> +	return 0;
> +out_free:
> +	free(fa);
> +	free(fb);
> +	*fields_a = *fields_b = NULL;
> +	return -1;
>   }
>   
>   static int __hpp__group_sort_idx(struct hist_entry *a, struct hist_entry *b,
> @@ -206,8 +197,7 @@ static int __hpp__group_sort_idx(struct hist_entry *a, struct hist_entry *b,
>   	if (idx < 1 || idx >= nr_members)
>   		return cmp;
>   
> -	ret = pair_fields_alloc(a, b, get_field, nr_members,
> -				&fields_a, &fields_b);
> +	ret = hist_entry__new_pair(a, b, get_field, nr_members, &fields_a, &fields_b);
>   	if (ret) {
>   		ret = cmp;
>   		goto out;
> @@ -254,8 +244,7 @@ static int __hpp__sort(struct hist_entry *a, struct hist_entry *b,
>   		return ret;
>   
>   	nr_members = evsel->core.nr_members;
> -	i = pair_fields_alloc(a, b, get_field, nr_members,
> -			      &fields_a, &fields_b);
> +	i = hist_entry__new_pair(a, b, get_field, nr_members, &fields_a, &fields_b);
>   	if (i)
>   		goto out;
>   
> 

Thanks for refining the patch!

Thanks
Jin Yao

Re: [PATCH v7 2/3] perf report: Support a new key to reload the browser

[Jin, Yao]

On 3/19/2020 2:52 AM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Feb 20, 2020 at 09:36:15AM +0800, Jin Yao escreveu:
>> Sometimes we may need to reload the browser to update the output since
>> some options are changed.
>>
>> This patch creates a new key K_RELOAD. Once the __cmd_report() returns
>> K_RELOAD, it would repeat the whole process, such as, read samples from
>> data file, sort the data and display in the browser.
>>
>>   v7:
>>   ---
>>   Rebase to perf/core, no other change.
>>
>>   v6:
>>   ---
>>   No change.
>>
>>   v5:
>>   ---
>>   1. Fix the 'make NO_SLANG=1' error. Define K_RELOAD in util/hist.h.
>>   2. Skip setup_sorting() in repeat path if last key is K_RELOAD.
>>
>>   v4:
>>   ---
>>   Need to quit in perf_evsel_menu__run if key is K_RELOAD.
>>
>>   v3:
>>   ---
>>   No change.
>>
>>   v2:
>>   ---
>>   This is a new patch created in v2.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>>   tools/perf/builtin-report.c    | 6 +++---
>>   tools/perf/ui/browsers/hists.c | 1 +
>>   tools/perf/ui/keysyms.h        | 1 +
>>   tools/perf/util/hist.h         | 1 +
>>   4 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
>> index 862c7f8853dc..842ef92c3598 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:
>> @@ -1469,7 +1469,7 @@ int cmd_report(int argc, const char **argv)
>>   		sort_order = sort_tmp;
>>   	}
>>   
>> -	if ((last_key != K_SWITCH_INPUT_DATA) &&
>> +	if ((last_key != K_SWITCH_INPUT_DATA && last_key != K_RELOAD) &&
>>   	    (setup_sorting(session->evlist) < 0)) {
>>   		if (sort_order)
>>   			parse_options_usage(report_usage, options, "s", 1);
>> @@ -1548,7 +1548,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);
>>   		last_key = K_SWITCH_INPUT_DATA;
> 
> Are you sure this shouldn't be:
> 	
> 		last_key = ret;
> 
> ?
> 

Yes, it should not be. We can reuse original flow for processing 
K_SWITCH_INPUT_DATA.

> I'm applying it to test now anyway,
> 
> - Arnaldo
> 

Thanks!

Thanks
Jin Yao

>>   		goto repeat;
>> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
>> index f36dee499320..7c091fa51a5c 100644
>> --- a/tools/perf/ui/browsers/hists.c
>> +++ b/tools/perf/ui/browsers/hists.c
>> @@ -3440,6 +3440,7 @@ static int perf_evsel_menu__run(struct evsel_menu *menu,
>>   					pos = perf_evsel__prev(pos);
>>   				goto browse_hists;
>>   			case K_SWITCH_INPUT_DATA:
>> +			case K_RELOAD:
>>   			case 'q':
>>   			case CTRL('c'):
>>   				goto out;
>> 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_ */
>> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
>> index 0aa63aeb58ec..bb994e030495 100644
>> --- a/tools/perf/util/hist.h
>> +++ b/tools/perf/util/hist.h
>> @@ -536,6 +536,7 @@ static inline int block_hists_tui_browse(struct block_hist *bh __maybe_unused,
>>   #define K_LEFT  -1000
>>   #define K_RIGHT -2000
>>   #define K_SWITCH_INPUT_DATA -3000
>> +#define K_RELOAD -4000
>>   #endif
>>   
>>   unsigned int hists__sort_list_width(struct hists *hists);
>> -- 
>> 2.17.1
>>
> 

Re: [PATCH v7 0/3] perf report: Support sorting by a given event in group

[Jin, Yao]

On 3/19/2020 3:08 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 18, 2020 at 04:03:25PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Wed, Mar 18, 2020 at 04:01:16PM -0300, Arnaldo Carvalho de Melo escreveu:
>>> Em Thu, Feb 20, 2020 at 09:36:13AM +0800, Jin Yao escreveu:
>>>> 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.
>>>>
>>>> The patch 1/3 introduces a new option "--group-sort-idx" to sort the
>>>> output by the event at the index n in event group.
>>>>
>>>> The patch 2/3 creates a new key K_RELOAD to reload the browser.
>>>>
>>>> The patch 3/3 supports hotkeys in browser to select a event to
>>>> sort.
>>>
>>> Thanks, applied.
>>
>> Doesn't work with 'perf top', should, investigating,
> 
> So needs a bit of work, but is doable and would be a great addition,
> since we do support:
> 
>    # perf top --group -e instructions,cycles,cache-misses
> 
> And we should strive to make 'perf top' to be a perf.data-less, dynamic
> version of 'perf record + perf report'.
> 
> Can you please take a look at supporting this? And that --group-sort-idx
> too,
> 
> I'll push my perf/core branch after a few tests,
> 
> Thanks,
> 
> - Arnaldo
> 

I will check that.

Thanks
Jin Yao

[tip: perf/urgent] perf report: Support a new key to reload the browser

[tip-bot2 for Jin Yao]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     5e3b810aac49e960c80529e2c9aa8c101c5848ce
Gitweb:        https://git.kernel.org/tip/5e3b810aac49e960c80529e2c9aa8c101c5848ce
Author:        Jin Yao <yao.jin@linux.intel.com>
AuthorDate:    Thu, 20 Feb 2020 09:36:15 +08:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 24 Mar 2020 09:37:27 -03:00

perf report: Support a new key to reload the browser

Sometimes we may need to reload the browser to update the output since
some options are changed.

This patch creates a new key K_RELOAD. Once the __cmd_report() returns
K_RELOAD, it would repeat the whole process, such as, read samples from
data file, sort the data and display in the browser.

 v5:
 ---
 1. Fix the 'make NO_SLANG=1' error. Define K_RELOAD in util/hist.h.
 2. Skip setup_sorting() in repeat path if last key is K_RELOAD.

 v4:
 ---
 Need to quit in perf_evsel_menu__run if key is K_RELOAD.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/20200220013616.19916-3-yao.jin@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-report.c    | 6 +++---
 tools/perf/ui/browsers/hists.c | 1 +
 tools/perf/ui/keysyms.h        | 1 +
 tools/perf/util/hist.h         | 1 +
 4 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index fa21c5a..ea673b7 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:
@@ -1480,7 +1480,7 @@ repeat:
 		sort_order = sort_tmp;
 	}
 
-	if ((last_key != K_SWITCH_INPUT_DATA) &&
+	if ((last_key != K_SWITCH_INPUT_DATA && last_key != K_RELOAD) &&
 	    (setup_sorting(session->evlist) < 0)) {
 		if (sort_order)
 			parse_options_usage(report_usage, options, "s", 1);
@@ -1559,7 +1559,7 @@ repeat:
 	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);
 		last_key = K_SWITCH_INPUT_DATA;
 		goto repeat;
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 1103a01..9f3401f 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3495,6 +3495,7 @@ browse_hists:
 					pos = perf_evsel__prev(pos);
 				goto browse_hists;
 			case K_SWITCH_INPUT_DATA:
+			case K_RELOAD:
 			case 'q':
 			case CTRL('c'):
 				goto out;
diff --git a/tools/perf/ui/keysyms.h b/tools/perf/ui/keysyms.h
index fbfac29..04cc4e5 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_ */
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 0aa63ae..bb994e0 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -536,6 +536,7 @@ static inline int block_hists_tui_browse(struct block_hist *bh __maybe_unused,
 #define K_LEFT  -1000
 #define K_RIGHT -2000
 #define K_SWITCH_INPUT_DATA -3000
+#define K_RELOAD -4000
 #endif
 
 unsigned int hists__sort_list_width(struct hists *hists);

[tip: perf/urgent] perf report/top TUI: Support hotkeys to let user select any event for sorting

[tip-bot2 for Jin Yao]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     dbddf17474411f5725fc76fc7e507410f0d4077f
Gitweb:        https://git.kernel.org/tip/dbddf17474411f5725fc76fc7e507410f0d4077f
Author:        Jin Yao <yao.jin@linux.intel.com>
AuthorDate:    Thu, 20 Feb 2020 09:36:16 +08:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 24 Mar 2020 09:37:27 -03:00

perf report/top TUI: Support hotkeys to let user select any event for sorting

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
to sort.

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 forth 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

 v6:
 ---
 Jiri provided a good improvement to eliminate unneeded refresh.
 This improvement is added to v6.

 v2:
 ---
 1. Report warning at helpline when index is invalid.
 2. Report warning at helpline when it's not group event.
 3. Use "case '0' ... '9'" to refine the code
 4. Split K_RELOAD implementation to another patch.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/20200220013616.19916-4-yao.jin@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/browsers/hists.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 9f3401f..95ac5e2 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2992,7 +2992,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"
@@ -3053,6 +3054,31 @@ do_hotkey:		 // key came straight from options ui__popup_menu()
 			 * go to the next or previous
 			 */
 			goto out_free_stack;
+		case '0' ... '9':
+			if (!symbol_conf.event_group ||
+			    evsel->core.nr_members < 2) {
+				snprintf(buf, sizeof(buf),
+					 "Sort by index only available with group events!");
+				helpline = buf;
+				continue;
+			}
+
+			if (key - '0' == symbol_conf.group_sort_idx)
+				continue;
+
+			symbol_conf.group_sort_idx = key - '0';
+
+			if (symbol_conf.group_sort_idx >= evsel->core.nr_members) {
+				snprintf(buf, sizeof(buf),
+					 "Max event group index to sort is %d (index from 0 to %d)",
+					 evsel->core.nr_members - 1,
+					 evsel->core.nr_members - 1);
+				helpline = buf;
+				continue;
+			}
+
+			key = K_RELOAD;
+			goto out_free_stack;
 		case 'a':
 			if (!hists__has(hists, sym)) {
 				ui_browser__warning(&browser->b, delay_secs * 2,

[tip: perf/urgent] perf report: Allow specifying event to be used as sort key in --group output

[tip-bot2 for Jin Yao]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     429a5f9d89fc52256b2a59cd1277afbfafb739b3
Gitweb:        https://git.kernel.org/tip/429a5f9d89fc52256b2a59cd1277afbfafb739b3
Author:        Jin Yao <yao.jin@linux.intel.com>
AuthorDate:    Thu, 20 Feb 2020 09:36:14 +08:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 24 Mar 2020 09:37:27 -03:00

perf report: Allow specifying event to be used as sort key in --group output

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.

 v7:
 ---
 Rebase to latest perf/core, no other change.

 v4:
 ---
 1. Update Documentation/perf-report.txt to mention
    '--group-sort-idx' support multiple groups with different
    amount of events and it should be used on grouped events.

 2. Update __hpp__group_sort_idx(), just return when the
    idx is out of limit.

 3. Return failure on symbol_conf.group_sort_idx && !session->evlist->nr_groups.
    So now we don't need to use together with --group.

 v3:
 ---
 Refine the code in __hpp__group_sort_idx().

 Before:
   for (i = 1; i < nr_members; i++) {
        if (i == idx) {
                ret = field_cmp(fields_a[i], fields_b[i]);
                if (ret)
                        goto out;
        }
   }

 After:
   if (idx >= 1 && idx < nr_members) {
        ret = field_cmp(fields_a[idx], fields_b[idx]);
        if (ret)
                goto out;
   }

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/20200220013616.19916-2-yao.jin@linux.intel.com
[ Renamed pair_fields_alloc() to hist_entry__new_pair() and combined decl + assignment of vars ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Documentation/perf-report.txt |  5 +-
 tools/perf/builtin-report.c              | 10 ++-
 tools/perf/ui/hist.c                     | 93 +++++++++++++++++++----
 tools/perf/util/symbol_conf.h            |  1 +-
 4 files changed, 94 insertions(+), 15 deletions(-)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index bd0a029..e56e3f1 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -377,6 +377,11 @@ 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. It can support multiple groups with different
+	amount of events. WARNING: This should be used on grouped events.
+
 --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 5f4045d..fa21c5a 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1227,6 +1227,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: should be used on grouped events."),
 	OPT_CALLBACK_NOOPT('b', "branch-stack", &branch_mode, "",
 		    "use branch records for per branch histogram filling",
 		    parse_branch_mode),
@@ -1369,6 +1373,12 @@ repeat:
 
 	setup_forced_leader(&report, session->evlist);
 
+	if (symbol_conf.group_sort_idx && !session->evlist->nr_groups) {
+		parse_options_usage(NULL, options, "group-sort-idx", 0);
+		ret = -EINVAL;
+		goto error;
+	}
+
 	if (itrace_synth_opts.last_branch)
 		has_br_stack = true;
 
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index f736755..025f4c7 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -151,15 +151,90 @@ static int field_cmp(u64 field_a, u64 field_b)
 	return 0;
 }
 
+static int hist_entry__new_pair(struct hist_entry *a, struct hist_entry *b,
+				hpp_field_fn get_field, int nr_members,
+				u64 **fields_a, u64 **fields_b)
+{
+	u64 *fa = calloc(nr_members, sizeof(*fa)),
+	    *fb = calloc(nr_members, sizeof(*fb));
+	struct hist_entry *pair;
+
+	if (!fa || !fb)
+		goto out_free;
+
+	list_for_each_entry(pair, &a->pairs.head, pairs.node) {
+		struct evsel *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) {
+		struct evsel *evsel = hists_to_evsel(pair->hists);
+		fb[perf_evsel__group_idx(evsel)] = get_field(pair);
+	}
+
+	*fields_a = fa;
+	*fields_b = fb;
+	return 0;
+out_free:
+	free(fa);
+	free(fb);
+	*fields_a = *fields_b = NULL;
+	return -1;
+}
+
+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;
+	if (idx < 1 || idx >= nr_members)
+		return cmp;
+
+	ret = hist_entry__new_pair(a, b, get_field, nr_members, &fields_a, &fields_b);
+	if (ret) {
+		ret = cmp;
+		goto out;
+	}
+
+	ret = field_cmp(fields_a[idx], fields_b[idx]);
+	if (ret)
+		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 +244,10 @@ 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 = hist_entry__new_pair(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 10f1ec3..b916afb 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;