[PATCH v3 1/5] perf tools: Add all matching dynamic sort keys for field name

[PATCH v3 1/5] perf tools: Add all matching dynamic sort keys for field name

[Namhyung Kim]

When a perf.data file has multiple events, it's likely to be similar
(tracepoint) events.  In that case, they might have same field name so
add all of them to sort keys instead of bailing out.

In addition, it contains a trivial whitespace fix at callsite of
add_all_dynamic_fields().

Acked-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 59c4c8586d79..9618a64875c0 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1965,6 +1965,28 @@ static int add_all_dynamic_fields(struct perf_evlist *evlist, bool raw_trace)
 	return 0;
 }
 
+static int add_all_matching_fields(struct perf_evlist *evlist,
+				   char *field_name, bool raw_trace)
+{
+	int ret = -ESRCH;
+	struct perf_evsel *evsel;
+	struct format_field *field;
+
+	evlist__for_each(evlist, evsel) {
+		if (evsel->attr.type != PERF_TYPE_TRACEPOINT)
+			continue;
+
+		field = pevent_find_any_field(evsel->tp_format, field_name);
+		if (field == NULL)
+			continue;
+
+		ret = __dynamic_dimension__add(evsel, field, raw_trace);
+		if (ret < 0)
+			break;
+	}
+	return ret;
+}
+
 static int add_dynamic_entry(struct perf_evlist *evlist, const char *tok)
 {
 	char *str, *event_name, *field_name, *opt_name;
@@ -1995,7 +2017,12 @@ static int add_dynamic_entry(struct perf_evlist *evlist, const char *tok)
 	}
 
 	if (!strcmp(field_name, "trace_fields")) {
-		ret = add_all_dynamic_fields(evlist ,raw_trace);
+		ret = add_all_dynamic_fields(evlist, raw_trace);
+		goto out;
+	}
+
+	if (event_name == NULL) {
+		ret = add_all_matching_fields(evlist, field_name, raw_trace);
 		goto out;
 	}
 
-- 
2.6.4

[PATCH v3 2/5] perf tools: Add document for dynamic sort keys

[Namhyung Kim]

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-report.txt | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index ae7cd91727f6..8a301f6afb37 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -117,6 +117,30 @@ OPTIONS
 	And default sort keys are changed to comm, dso_from, symbol_from, dso_to
 	and symbol_to, see '--branch-stack'.
 
+	If the data file has tracepoint event(s), following (dynamic) sort keys
+	are also available:
+	trace, trace_fields, [<event>.]<field>[/raw]
+
+	- trace: pretty printed trace output in a single column
+	- trace_fields: fields in tracepoints in separate columns
+	- <field name>: optional event and field name for a specific field
+
+	The last form consists of event and field names.  If event name is
+	omitted, it searches all events for matching field name.  The matched
+	field will be shown only for the event has the field.  The event name
+	supports substring match so user doesn't need to specify full subsystem
+	and event name everytime.  For example, 'sched:sched_switch' event can
+	be shortened to 'switch' as long as it's not ambiguous.  Also event can
+	be specified by its index (starting from 1) preceded by the '%'.
+	So '%1' is the first event, '%2' is the second, and so on.
+
+	The field name can have '/raw' suffix which disables pretty printing
+	and shows raw field value like hex numbers.  The --raw-trace option
+	has the same effect for all dynamic sort keys.
+
+	The default sort keys are changed to 'trace' if all events in the data
+	file are tracepoint.
+
 -F::
 --fields=::
 	Specify output field - multiple keys can be specified in CSV format.
-- 
2.6.4

[PATCH v3 3/5] perf tools: Fix dynamic sort keys to sort properly

[Namhyung Kim]

Currently, the dynamic sort keys compares trace data using memcmp().
But for output sorting, it should check data size and compare by word.
Also it sorted strings in reverse order, fix it.

Before)

  $ perf report -F overhead -s prev_pid,next_pid
  ...
  # Overhead    prev_pid    next_pid
  # ........  ..........  ..........
  #
       0.39%         490           0
       9.12%         225           0
       0.04%         224           0
       0.51%         731         189
       0.08%         731           3
       0.12%         731           0
       4.82%         729           0
       0.08%        1229           0
       0.20%         715           0
       4.78%         189         225
  ...

After)

  $ perf report -F overhead -s prev_pid,next_pid
  ...
  # Overhead    prev_pid    next_pid
  # ........  ..........  ..........
  #
       0.43%           0           7
       0.04%           0          11
       0.04%           0          12
       0.08%           0          14
       0.04%           0          17
       0.08%           0          19
       0.04%           0          22
       0.04%           0          27
       0.04%           0          37
       0.04%           0          42
  ...

Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 9618a64875c0..264d2b630549 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1798,6 +1798,51 @@ static int64_t __sort__hde_cmp(struct perf_hpp_fmt *fmt,
 	return memcmp(a->raw_data + offset, b->raw_data + offset, size);
 }
 
+static int64_t __sort__hde_sort(struct perf_hpp_fmt *fmt,
+				struct hist_entry *a, struct hist_entry *b)
+{
+	struct hpp_dynamic_entry *hde;
+	struct format_field *field;
+	unsigned offset, size;
+	int64_t *a64, *b64;
+	int32_t *a32, *b32;
+	int16_t *a16, *b16;
+
+	hde = container_of(fmt, struct hpp_dynamic_entry, hpp);
+
+	field = hde->field;
+	if (field->flags & FIELD_IS_DYNAMIC) {
+		unsigned long long dyn;
+
+		pevent_read_number_field(field, a->raw_data, &dyn);
+		offset = dyn & 0xffff;
+		size = (dyn >> 16) & 0xffff;
+	} else {
+		offset = field->offset;
+		size = field->size;
+	}
+
+	if (field->flags & FIELD_IS_STRING)
+		return strcmp(b->raw_data + offset, a->raw_data + offset);
+
+	switch (size) {
+	case 8:
+		a64 = a->raw_data + offset;
+		b64 = b->raw_data + offset;
+		return *b64 - *a64;
+	case 4:
+		a32 = a->raw_data + offset;
+		b32 = b->raw_data + offset;
+		return *b32 - *a32;
+	case 2:
+		a16 = a->raw_data + offset;
+		b16 = b->raw_data + offset;
+		return *b16 - *a16;
+	default:
+		return memcmp(b->raw_data + offset, a->raw_data + offset, size);
+	}
+}
+
 bool perf_hpp__is_dynamic_entry(struct perf_hpp_fmt *fmt)
 {
 	return fmt->cmp == __sort__hde_cmp;
@@ -1826,7 +1871,7 @@ __alloc_dynamic_entry(struct perf_evsel *evsel, struct format_field *field)
 
 	hde->hpp.cmp = __sort__hde_cmp;
 	hde->hpp.collapse = __sort__hde_cmp;
-	hde->hpp.sort = __sort__hde_cmp;
+	hde->hpp.sort = __sort__hde_sort;
 
 	INIT_LIST_HEAD(&hde->hpp.list);
 	INIT_LIST_HEAD(&hde->hpp.sort_list);
-- 
2.6.4

[PATCH v3 4/5] perf tools: Support dynamic sort keys for -F/--fields

[Namhyung Kim]

Now dynamic sort keys are supported for tracepoint events, add it to
output fields too.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/sort.c | 51 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 264d2b630549..67a92039a2e6 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1964,7 +1964,7 @@ static struct perf_evsel *find_evsel(struct perf_evlist *evlist, char *event_nam
 
 static int __dynamic_dimension__add(struct perf_evsel *evsel,
 				    struct format_field *field,
-				    bool raw_trace)
+				    bool raw_trace, bool is_sort_key)
 {
 	struct hpp_dynamic_entry *hde;
 
@@ -1974,18 +1974,24 @@ static int __dynamic_dimension__add(struct perf_evsel *evsel,
 
 	hde->raw_trace = raw_trace;
 
-	perf_hpp__register_sort_field(&hde->hpp);
+	if (is_sort_key)
+		perf_hpp__register_sort_field(&hde->hpp);
+	else
+		perf_hpp__column_register(&hde->hpp);
+
 	return 0;
 }
 
-static int add_evsel_fields(struct perf_evsel *evsel, bool raw_trace)
+static int add_evsel_fields(struct perf_evsel *evsel, bool raw_trace,
+			    bool is_sort_key)
 {
 	int ret;
 	struct format_field *field;
 
 	field = evsel->tp_format->format.fields;
 	while (field) {
-		ret = __dynamic_dimension__add(evsel, field, raw_trace);
+		ret = __dynamic_dimension__add(evsel, field, raw_trace,
+					       is_sort_key);
 		if (ret < 0)
 			return ret;
 
@@ -1994,7 +2000,8 @@ static int add_evsel_fields(struct perf_evsel *evsel, bool raw_trace)
 	return 0;
 }
 
-static int add_all_dynamic_fields(struct perf_evlist *evlist, bool raw_trace)
+static int add_all_dynamic_fields(struct perf_evlist *evlist, bool raw_trace,
+				  bool is_sort_key)
 {
 	int ret;
 	struct perf_evsel *evsel;
@@ -2003,7 +2010,7 @@ static int add_all_dynamic_fields(struct perf_evlist *evlist, bool raw_trace)
 		if (evsel->attr.type != PERF_TYPE_TRACEPOINT)
 			continue;
 
-		ret = add_evsel_fields(evsel, raw_trace);
+		ret = add_evsel_fields(evsel, raw_trace, is_sort_key);
 		if (ret < 0)
 			return ret;
 	}
@@ -2011,7 +2018,8 @@ static int add_all_dynamic_fields(struct perf_evlist *evlist, bool raw_trace)
 }
 
 static int add_all_matching_fields(struct perf_evlist *evlist,
-				   char *field_name, bool raw_trace)
+				   char *field_name, bool raw_trace,
+				   bool is_sort_key)
 {
 	int ret = -ESRCH;
 	struct perf_evsel *evsel;
@@ -2025,14 +2033,16 @@ static int add_all_matching_fields(struct perf_evlist *evlist,
 		if (field == NULL)
 			continue;
 
-		ret = __dynamic_dimension__add(evsel, field, raw_trace);
+		ret = __dynamic_dimension__add(evsel, field, raw_trace,
+					       is_sort_key);
 		if (ret < 0)
 			break;
 	}
 	return ret;
 }
 
-static int add_dynamic_entry(struct perf_evlist *evlist, const char *tok)
+static int add_dynamic_entry(struct perf_evlist *evlist, const char *tok,
+			     bool is_sort_key)
 {
 	char *str, *event_name, *field_name, *opt_name;
 	struct perf_evsel *evsel;
@@ -2062,12 +2072,13 @@ static int add_dynamic_entry(struct perf_evlist *evlist, const char *tok)
 	}
 
 	if (!strcmp(field_name, "trace_fields")) {
-		ret = add_all_dynamic_fields(evlist, raw_trace);
+		ret = add_all_dynamic_fields(evlist, raw_trace, is_sort_key);
 		goto out;
 	}
 
 	if (event_name == NULL) {
-		ret = add_all_matching_fields(evlist, field_name, raw_trace);
+		ret = add_all_matching_fields(evlist, field_name, raw_trace,
+					      is_sort_key);
 		goto out;
 	}
 
@@ -2085,7 +2096,7 @@ static int add_dynamic_entry(struct perf_evlist *evlist, const char *tok)
 	}
 
 	if (!strcmp(field_name, "*")) {
-		ret = add_evsel_fields(evsel, raw_trace);
+		ret = add_evsel_fields(evsel, raw_trace, is_sort_key);
 	} else {
 		field = pevent_find_any_field(evsel->tp_format, field_name);
 		if (field == NULL) {
@@ -2094,7 +2105,8 @@ static int add_dynamic_entry(struct perf_evlist *evlist, const char *tok)
 			return -ENOENT;
 		}
 
-		ret = __dynamic_dimension__add(evsel, field, raw_trace);
+		ret = __dynamic_dimension__add(evsel, field, raw_trace,
+					       is_sort_key);
 	}
 
 out:
@@ -2238,7 +2250,7 @@ static int sort_dimension__add(const char *tok,
 		return 0;
 	}
 
-	if (!add_dynamic_entry(evlist, tok))
+	if (!add_dynamic_entry(evlist, tok, true))
 		return 0;
 
 	return -ESRCH;
@@ -2443,7 +2455,7 @@ void sort__setup_elide(FILE *output)
 	}
 }
 
-static int output_field_add(char *tok)
+static int output_field_add(struct perf_evlist *evlist, char *tok)
 {
 	unsigned int i;
 
@@ -2483,6 +2495,9 @@ static int output_field_add(char *tok)
 		return __sort_dimension__add_output(sd);
 	}
 
+	if (!add_dynamic_entry(evlist, tok, false))
+		return 0;
+
 	return -ESRCH;
 }
 
@@ -2508,7 +2523,7 @@ bool is_strict_order(const char *order)
 	return order && (*order != '+');
 }
 
-static int __setup_output_field(void)
+static int __setup_output_field(struct perf_evlist *evlist)
 {
 	char *tmp, *tok, *str, *strp;
 	int ret = -EINVAL;
@@ -2532,7 +2547,7 @@ static int __setup_output_field(void)
 
 	for (tok = strtok_r(strp, ", ", &tmp);
 			tok; tok = strtok_r(NULL, ", ", &tmp)) {
-		ret = output_field_add(tok);
+		ret = output_field_add(evlist, tok);
 		if (ret == -EINVAL) {
 			error("Invalid --fields key: `%s'", tok);
 			break;
@@ -2569,7 +2584,7 @@ int setup_sorting(struct perf_evlist *evlist)
 	if (sort__mode != SORT_MODE__DIFF)
 		perf_hpp__init();
 
-	err = __setup_output_field();
+	err = __setup_output_field(evlist);
 	if (err < 0)
 		return err;
 
-- 
2.6.4

[PATCH v3 5/5] perf evlist: Add --trace-fields option to show trace fields

[Namhyung Kim]

To use dynamic sort keys, it might be good to add an option to see the
list of field names.

  $ perf evlist -i perf.data.sched
  sched:sched_switch
  sched:sched_stat_wait
  sched:sched_stat_sleep
  sched:sched_stat_iowait
  sched:sched_stat_runtime
  sched:sched_process_fork
  sched:sched_wakeup
  sched:sched_wakeup_new
  sched:sched_migrate_task
  # Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events

  $ perf evlist -i perf.data.sched --trace-fields
  sched:sched_switch: trace_fields=prev_comm,prev_pid,prev_prio,prev_state,next_comm,next_pid,next_prio
  sched:sched_stat_wait: trace_fields=comm,pid,delay
  sched:sched_stat_sleep: trace_fields=comm,pid,delay
  sched:sched_stat_iowait: trace_fields=comm,pid,delay
  sched:sched_stat_runtime: trace_fields=comm,pid,runtime,vruntime
  sched:sched_process_fork: trace_fields=parent_comm,parent_pid,child_comm,child_pid
  sched:sched_wakeup: trace_fields=comm,pid,prio,success,target_cpu
  sched:sched_wakeup_new: trace_fields=comm,pid,prio,success,target_cpu
  sched:sched_migrate_task: trace_fields=comm,pid,prio,orig_cpu,dest_cpu

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-evlist.txt |  3 +++
 tools/perf/builtin-evlist.c              | 11 ++++++++++-
 tools/perf/util/evsel.c                  | 23 +++++++++++++++++++++++
 tools/perf/util/evsel.h                  |  1 +
 4 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/tools/perf/Documentation/perf-evlist.txt b/tools/perf/Documentation/perf-evlist.txt
index 1ceb3700ffbb..6f7200fb85cf 100644
--- a/tools/perf/Documentation/perf-evlist.txt
+++ b/tools/perf/Documentation/perf-evlist.txt
@@ -32,6 +32,9 @@ OPTIONS
 --group::
 	Show event group information.
 
+--trace-fields::
+	Show tracepoint field names.
+
 SEE ALSO
 --------
 linkperf:perf-record[1], linkperf:perf-list[1],
diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
index 08a7d36a2cf8..8a31f511e1a0 100644
--- a/tools/perf/builtin-evlist.c
+++ b/tools/perf/builtin-evlist.c
@@ -26,14 +26,22 @@ static int __cmd_evlist(const char *file_name, struct perf_attr_details *details
 		.mode = PERF_DATA_MODE_READ,
 		.force = details->force,
 	};
+	bool has_tracepoint = false;
 
 	session = perf_session__new(&file, 0, NULL);
 	if (session == NULL)
 		return -1;
 
-	evlist__for_each(session->evlist, pos)
+	evlist__for_each(session->evlist, pos) {
 		perf_evsel__fprintf(pos, details, stdout);
 
+		if (pos->attr.type == PERF_TYPE_TRACEPOINT)
+			has_tracepoint = true;
+	}
+
+	if (has_tracepoint && !details->trace_fields)
+		printf("# Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events\n");
+
 	perf_session__delete(session);
 	return 0;
 }
@@ -49,6 +57,7 @@ int cmd_evlist(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_BOOLEAN('g', "group", &details.event_group,
 		    "Show event group information"),
 	OPT_BOOLEAN('f', "force", &details.force, "don't complain, do it"),
+	OPT_BOOLEAN(0, "trace-fields", &details.trace_fields, "Show tracepoint fields"),
 	OPT_END()
 	};
 	const char * const evlist_usage[] = {
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 544e4400de13..b7822c98fcca 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2298,6 +2298,29 @@ int perf_evsel__fprintf(struct perf_evsel *evsel,
 		printed += comma_fprintf(fp, &first, " %s=%" PRIu64,
 					 term, (u64)evsel->attr.sample_freq);
 	}
+
+	if (details->trace_fields) {
+		struct format_field *field;
+
+		if (evsel->attr.type != PERF_TYPE_TRACEPOINT) {
+			printed += comma_fprintf(fp, &first, " (not a tracepoint)");
+			goto out;
+		}
+
+		field = evsel->tp_format->format.fields;
+		if (field == NULL) {
+			printed += comma_fprintf(fp, &first, " (no trace field)");
+			goto out;
+		}
+
+		printed += comma_fprintf(fp, &first, " trace_fields=%s", field->name);
+
+		field = field->next;
+		while (field) {
+			printed += comma_fprintf(fp, &first, "%s", field->name);
+			field = field->next;
+		}
+	}
 out:
 	fputc('\n', fp);
 	return ++printed;
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 5ded1fc0341e..8e75434bd01c 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -369,6 +369,7 @@ struct perf_attr_details {
 	bool verbose;
 	bool event_group;
 	bool force;
+	bool trace_fields;
 };
 
 int perf_evsel__fprintf(struct perf_evsel *evsel,
-- 
2.6.4

Re: [PATCH v3 1/5] perf tools: Add all matching dynamic sort keys for field name

[Jiri Olsa]

On Wed, Jan 06, 2016 at 09:54:57AM +0900, Namhyung Kim wrote:
> When a perf.data file has multiple events, it's likely to be similar
> (tracepoint) events.  In that case, they might have same field name so
> add all of them to sort keys instead of bailing out.
> 
> In addition, it contains a trivial whitespace fix at callsite of
> add_all_dynamic_fields().
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

hum, I haven't tried with this last version but I get all
hist tests failing on acme's perf/core and it seems to be
related to sorting changes:

[jolsa@krava perf]$ ./perf test hist 
15: Test matching and linking multiple hists                 : FAILED!
25: Test filtering hist entries                              : FAILED!
28: Test output sorting of hist entries                      : FAILED!
29: Test cumulation of child hist entries                    : FAILED!


[jolsa@krava perf]$ ./perf test 15 -v
15: Test matching and linking multiple hists                 :
--- start ---
test child forked, pid 10676
perf: Segmentation fault
Obtained 16 stack frames.
./perf(dump_stack+0x2d) [0x50f1d7]
./perf(sighandler_dump_stack+0x2d) [0x50f2b7]
/lib64/libc.so.6(+0x34a4f) [0x7fb0b1178a4f]
./perf() [0x508c40]
./perf() [0x508e23]
./perf(setup_sorting+0x26) [0x5097b0]
./perf(test__hists_link+0xb6) [0x487f43]
./perf() [0x4725c0]
./perf() [0x4726ff]
./perf() [0x472986]
./perf(cmd_test+0x1fe) [0x472ddc]
./perf() [0x49b2ab]
./perf() [0x49b513]
./perf() [0x49b661]
./perf(main+0x258) [0x49b9e2]
/lib64/libc.so.6(__libc_start_main+0xef) [0x7fb0b11646ff]
test child interrupted
---- end ----
Test matching and linking multiple hists: FAILED!


thanks,
jirka

Re: [PATCH v3 1/5] perf tools: Add all matching dynamic sort keys for field name

[Arnaldo Carvalho de Melo]

Em Wed, Jan 06, 2016 at 12:19:39PM +0100, Jiri Olsa escreveu:
> On Wed, Jan 06, 2016 at 09:54:57AM +0900, Namhyung Kim wrote:
> > When a perf.data file has multiple events, it's likely to be similar
> > (tracepoint) events.  In that case, they might have same field name so
> > add all of them to sort keys instead of bailing out.
> > 
> > In addition, it contains a trivial whitespace fix at callsite of
> > add_all_dynamic_fields().
> > 
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> 
> hum, I haven't tried with this last version but I get all
> hist tests failing on acme's perf/core and it seems to be
> related to sorting changes:

Bisected it down to

0337e6473845 ("perf tools: Make 'trace' or 'trace_fields' sort key default for tracepoint events")

Fixed with the following patch, which I'm folding into the above commit,
thanks for the report!

- Arnaldo

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index acd222907bd6..4b4b1c5cccef 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -2187,6 +2187,9 @@ static const char *get_default_sort_order(struct perf_evlist *evlist)
 
 	BUG_ON(sort__mode >= ARRAY_SIZE(default_sort_orders));
 
+	if (evlist == NULL)
+		goto out_no_evlist;
+
 	evlist__for_each(evlist, evsel) {
 		if (evsel->attr.type != PERF_TYPE_TRACEPOINT) {
 			use_trace = false;
@@ -2199,7 +2202,7 @@ static const char *get_default_sort_order(struct perf_evlist *evlist)
 		if (symbol_conf.raw_trace)
 			return "trace_fields";
 	}
-
+out_no_evlist:
 	return default_sort_orders[sort__mode];
 }
 
 
> [jolsa@krava perf]$ ./perf test hist 
> 15: Test matching and linking multiple hists                 : FAILED!
> 25: Test filtering hist entries                              : FAILED!
> 28: Test output sorting of hist entries                      : FAILED!
> 29: Test cumulation of child hist entries                    : FAILED!
> 
> 
> [jolsa@krava perf]$ ./perf test 15 -v
> 15: Test matching and linking multiple hists                 :
> --- start ---
> test child forked, pid 10676
> perf: Segmentation fault
> Obtained 16 stack frames.
> ./perf(dump_stack+0x2d) [0x50f1d7]
> ./perf(sighandler_dump_stack+0x2d) [0x50f2b7]
> /lib64/libc.so.6(+0x34a4f) [0x7fb0b1178a4f]
> ./perf() [0x508c40]
> ./perf() [0x508e23]
> ./perf(setup_sorting+0x26) [0x5097b0]
> ./perf(test__hists_link+0xb6) [0x487f43]
> ./perf() [0x4725c0]
> ./perf() [0x4726ff]
> ./perf() [0x472986]
> ./perf(cmd_test+0x1fe) [0x472ddc]
> ./perf() [0x49b2ab]
> ./perf() [0x49b513]
> ./perf() [0x49b661]
> ./perf(main+0x258) [0x49b9e2]
> /lib64/libc.so.6(__libc_start_main+0xef) [0x7fb0b11646ff]
> test child interrupted
> ---- end ----
> Test matching and linking multiple hists: FAILED!
> 
> 
> thanks,
> jirka

Re: [PATCH v3 3/5] perf tools: Fix dynamic sort keys to sort properly

[Arnaldo Carvalho de Melo]

Em Wed, Jan 06, 2016 at 09:54:59AM +0900, Namhyung Kim escreveu:
> Currently, the dynamic sort keys compares trace data using memcmp().
> But for output sorting, it should check data size and compare by word.
> Also it sorted strings in reverse order, fix it.

Can this be broken down in two patches? This is complex code, lets try
to make it as bisectable as possible.

- Arnaldo

> 
> Before)
> 
>   $ perf report -F overhead -s prev_pid,next_pid
>   ...
>   # Overhead    prev_pid    next_pid
>   # ........  ..........  ..........
>   #
>        0.39%         490           0
>        9.12%         225           0
>        0.04%         224           0
>        0.51%         731         189
>        0.08%         731           3
>        0.12%         731           0
>        4.82%         729           0
>        0.08%        1229           0
>        0.20%         715           0
>        4.78%         189         225
>   ...
> 
> After)
> 
>   $ perf report -F overhead -s prev_pid,next_pid
>   ...
>   # Overhead    prev_pid    next_pid
>   # ........  ..........  ..........
>   #
>        0.43%           0           7
>        0.04%           0          11
>        0.04%           0          12
>        0.08%           0          14
>        0.04%           0          17
>        0.08%           0          19
>        0.04%           0          22
>        0.04%           0          27
>        0.04%           0          37
>        0.04%           0          42
>   ...
> 
> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/sort.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 9618a64875c0..264d2b630549 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -1798,6 +1798,51 @@ static int64_t __sort__hde_cmp(struct perf_hpp_fmt *fmt,
>  	return memcmp(a->raw_data + offset, b->raw_data + offset, size);
>  }
>  
> +static int64_t __sort__hde_sort(struct perf_hpp_fmt *fmt,
> +				struct hist_entry *a, struct hist_entry *b)
> +{
> +	struct hpp_dynamic_entry *hde;
> +	struct format_field *field;
> +	unsigned offset, size;
> +	int64_t *a64, *b64;
> +	int32_t *a32, *b32;
> +	int16_t *a16, *b16;
> +
> +	hde = container_of(fmt, struct hpp_dynamic_entry, hpp);
> +
> +	field = hde->field;
> +	if (field->flags & FIELD_IS_DYNAMIC) {
> +		unsigned long long dyn;
> +
> +		pevent_read_number_field(field, a->raw_data, &dyn);
> +		offset = dyn & 0xffff;
> +		size = (dyn >> 16) & 0xffff;
> +	} else {
> +		offset = field->offset;
> +		size = field->size;
> +	}
> +
> +	if (field->flags & FIELD_IS_STRING)
> +		return strcmp(b->raw_data + offset, a->raw_data + offset);
> +
> +	switch (size) {
> +	case 8:
> +		a64 = a->raw_data + offset;
> +		b64 = b->raw_data + offset;
> +		return *b64 - *a64;
> +	case 4:
> +		a32 = a->raw_data + offset;
> +		b32 = b->raw_data + offset;
> +		return *b32 - *a32;
> +	case 2:
> +		a16 = a->raw_data + offset;
> +		b16 = b->raw_data + offset;
> +		return *b16 - *a16;
> +	default:
> +		return memcmp(b->raw_data + offset, a->raw_data + offset, size);
> +	}
> +}
> +
>  bool perf_hpp__is_dynamic_entry(struct perf_hpp_fmt *fmt)
>  {
>  	return fmt->cmp == __sort__hde_cmp;
> @@ -1826,7 +1871,7 @@ __alloc_dynamic_entry(struct perf_evsel *evsel, struct format_field *field)
>  
>  	hde->hpp.cmp = __sort__hde_cmp;
>  	hde->hpp.collapse = __sort__hde_cmp;
> -	hde->hpp.sort = __sort__hde_cmp;
> +	hde->hpp.sort = __sort__hde_sort;
>  
>  	INIT_LIST_HEAD(&hde->hpp.list);
>  	INIT_LIST_HEAD(&hde->hpp.sort_list);
> -- 
> 2.6.4

Re: [PATCH v3 1/5] perf tools: Add all matching dynamic sort keys for field name

[Namhyung Kim]

Hi Arnaldo and Jiri,

On Wed, Jan 06, 2016 at 01:29:48PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jan 06, 2016 at 12:19:39PM +0100, Jiri Olsa escreveu:
> > On Wed, Jan 06, 2016 at 09:54:57AM +0900, Namhyung Kim wrote:
> > > When a perf.data file has multiple events, it's likely to be similar
> > > (tracepoint) events.  In that case, they might have same field name so
> > > add all of them to sort keys instead of bailing out.
> > > 
> > > In addition, it contains a trivial whitespace fix at callsite of
> > > add_all_dynamic_fields().
> > > 
> > > Acked-by: Jiri Olsa <jolsa@kernel.org>
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > 
> > hum, I haven't tried with this last version but I get all
> > hist tests failing on acme's perf/core and it seems to be
> > related to sorting changes:
> 
> Bisected it down to
> 
> 0337e6473845 ("perf tools: Make 'trace' or 'trace_fields' sort key default for tracepoint events")
> 
> Fixed with the following patch, which I'm folding into the above commit,
> thanks for the report!

Ouch, thank you for the fix! :)

Thanks,
Namhyung


> 
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index acd222907bd6..4b4b1c5cccef 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -2187,6 +2187,9 @@ static const char *get_default_sort_order(struct perf_evlist *evlist)
>  
>  	BUG_ON(sort__mode >= ARRAY_SIZE(default_sort_orders));
>  
> +	if (evlist == NULL)
> +		goto out_no_evlist;
> +
>  	evlist__for_each(evlist, evsel) {
>  		if (evsel->attr.type != PERF_TYPE_TRACEPOINT) {
>  			use_trace = false;
> @@ -2199,7 +2202,7 @@ static const char *get_default_sort_order(struct perf_evlist *evlist)
>  		if (symbol_conf.raw_trace)
>  			return "trace_fields";
>  	}
> -
> +out_no_evlist:
>  	return default_sort_orders[sort__mode];
>  }
>  
>  
> > [jolsa@krava perf]$ ./perf test hist 
> > 15: Test matching and linking multiple hists                 : FAILED!
> > 25: Test filtering hist entries                              : FAILED!
> > 28: Test output sorting of hist entries                      : FAILED!
> > 29: Test cumulation of child hist entries                    : FAILED!
> > 
> > 
> > [jolsa@krava perf]$ ./perf test 15 -v
> > 15: Test matching and linking multiple hists                 :
> > --- start ---
> > test child forked, pid 10676
> > perf: Segmentation fault
> > Obtained 16 stack frames.
> > ./perf(dump_stack+0x2d) [0x50f1d7]
> > ./perf(sighandler_dump_stack+0x2d) [0x50f2b7]
> > /lib64/libc.so.6(+0x34a4f) [0x7fb0b1178a4f]
> > ./perf() [0x508c40]
> > ./perf() [0x508e23]
> > ./perf(setup_sorting+0x26) [0x5097b0]
> > ./perf(test__hists_link+0xb6) [0x487f43]
> > ./perf() [0x4725c0]
> > ./perf() [0x4726ff]
> > ./perf() [0x472986]
> > ./perf(cmd_test+0x1fe) [0x472ddc]
> > ./perf() [0x49b2ab]
> > ./perf() [0x49b513]
> > ./perf() [0x49b661]
> > ./perf(main+0x258) [0x49b9e2]
> > /lib64/libc.so.6(__libc_start_main+0xef) [0x7fb0b11646ff]
> > test child interrupted
> > ---- end ----
> > Test matching and linking multiple hists: FAILED!
> > 
> > 
> > thanks,
> > jirka

Re: [PATCH v3 5/5] perf evlist: Add --trace-fields option to show trace fields

[Arnaldo Carvalho de Melo]

Em Wed, Jan 06, 2016 at 09:55:01AM +0900, Namhyung Kim escreveu:
> To use dynamic sort keys, it might be good to add an option to see the
> list of field names.
> 
>   $ perf evlist -i perf.data.sched
>   sched:sched_switch
>   sched:sched_stat_wait
>   sched:sched_stat_sleep
>   sched:sched_stat_iowait
>   sched:sched_stat_runtime
>   sched:sched_process_fork
>   sched:sched_wakeup
>   sched:sched_wakeup_new
>   sched:sched_migrate_task
>   # Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events

Ok, almost there, question is: if I ask explicitely for
"--trace-fields", why should we have the "trace_fields=" in all lines,
instead of just:

 
   $ perf evlist -i perf.data.sched --trace-fields
   sched:sched_switch: prev_comm,prev_pid,prev_prio,prev_state,next_comm,next_pid,next_prio
   sched:sched_stat_wait: comm,pid,delay

?

I like the lack of spaces after commans, this way we can double click
and select the whole list, then edit it, etc.

- Arnaldo
 
>   $ perf evlist -i perf.data.sched --trace-fields
>   sched:sched_switch: trace_fields=prev_comm,prev_pid,prev_prio,prev_state,next_comm,next_pid,next_prio
>   sched:sched_stat_wait: trace_fields=comm,pid,delay
>   sched:sched_stat_sleep: trace_fields=comm,pid,delay
>   sched:sched_stat_iowait: trace_fields=comm,pid,delay
>   sched:sched_stat_runtime: trace_fields=comm,pid,runtime,vruntime
>   sched:sched_process_fork: trace_fields=parent_comm,parent_pid,child_comm,child_pid
>   sched:sched_wakeup: trace_fields=comm,pid,prio,success,target_cpu
>   sched:sched_wakeup_new: trace_fields=comm,pid,prio,success,target_cpu
>   sched:sched_migrate_task: trace_fields=comm,pid,prio,orig_cpu,dest_cpu
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/Documentation/perf-evlist.txt |  3 +++
>  tools/perf/builtin-evlist.c              | 11 ++++++++++-
>  tools/perf/util/evsel.c                  | 23 +++++++++++++++++++++++
>  tools/perf/util/evsel.h                  |  1 +
>  4 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/Documentation/perf-evlist.txt b/tools/perf/Documentation/perf-evlist.txt
> index 1ceb3700ffbb..6f7200fb85cf 100644
> --- a/tools/perf/Documentation/perf-evlist.txt
> +++ b/tools/perf/Documentation/perf-evlist.txt
> @@ -32,6 +32,9 @@ OPTIONS
>  --group::
>  	Show event group information.
>  
> +--trace-fields::
> +	Show tracepoint field names.
> +
>  SEE ALSO
>  --------
>  linkperf:perf-record[1], linkperf:perf-list[1],
> diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
> index 08a7d36a2cf8..8a31f511e1a0 100644
> --- a/tools/perf/builtin-evlist.c
> +++ b/tools/perf/builtin-evlist.c
> @@ -26,14 +26,22 @@ static int __cmd_evlist(const char *file_name, struct perf_attr_details *details
>  		.mode = PERF_DATA_MODE_READ,
>  		.force = details->force,
>  	};
> +	bool has_tracepoint = false;
>  
>  	session = perf_session__new(&file, 0, NULL);
>  	if (session == NULL)
>  		return -1;
>  
> -	evlist__for_each(session->evlist, pos)
> +	evlist__for_each(session->evlist, pos) {
>  		perf_evsel__fprintf(pos, details, stdout);
>  
> +		if (pos->attr.type == PERF_TYPE_TRACEPOINT)
> +			has_tracepoint = true;
> +	}
> +
> +	if (has_tracepoint && !details->trace_fields)
> +		printf("# Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events\n");
> +
>  	perf_session__delete(session);
>  	return 0;
>  }
> @@ -49,6 +57,7 @@ int cmd_evlist(int argc, const char **argv, const char *prefix __maybe_unused)
>  	OPT_BOOLEAN('g', "group", &details.event_group,
>  		    "Show event group information"),
>  	OPT_BOOLEAN('f', "force", &details.force, "don't complain, do it"),
> +	OPT_BOOLEAN(0, "trace-fields", &details.trace_fields, "Show tracepoint fields"),
>  	OPT_END()
>  	};
>  	const char * const evlist_usage[] = {
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 544e4400de13..b7822c98fcca 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2298,6 +2298,29 @@ int perf_evsel__fprintf(struct perf_evsel *evsel,
>  		printed += comma_fprintf(fp, &first, " %s=%" PRIu64,
>  					 term, (u64)evsel->attr.sample_freq);
>  	}
> +
> +	if (details->trace_fields) {
> +		struct format_field *field;
> +
> +		if (evsel->attr.type != PERF_TYPE_TRACEPOINT) {
> +			printed += comma_fprintf(fp, &first, " (not a tracepoint)");
> +			goto out;
> +		}
> +
> +		field = evsel->tp_format->format.fields;
> +		if (field == NULL) {
> +			printed += comma_fprintf(fp, &first, " (no trace field)");
> +			goto out;
> +		}
> +
> +		printed += comma_fprintf(fp, &first, " trace_fields=%s", field->name);
> +
> +		field = field->next;
> +		while (field) {
> +			printed += comma_fprintf(fp, &first, "%s", field->name);
> +			field = field->next;
> +		}
> +	}
>  out:
>  	fputc('\n', fp);
>  	return ++printed;
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 5ded1fc0341e..8e75434bd01c 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -369,6 +369,7 @@ struct perf_attr_details {
>  	bool verbose;
>  	bool event_group;
>  	bool force;
> +	bool trace_fields;
>  };
>  
>  int perf_evsel__fprintf(struct perf_evsel *evsel,
> -- 
> 2.6.4

Re: [PATCH v3 5/5] perf evlist: Add --trace-fields option to show trace fields

[Namhyung Kim]

On Wed, Jan 06, 2016 at 08:10:51PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jan 06, 2016 at 09:55:01AM +0900, Namhyung Kim escreveu:
> > To use dynamic sort keys, it might be good to add an option to see the
> > list of field names.
> > 
> >   $ perf evlist -i perf.data.sched
> >   sched:sched_switch
> >   sched:sched_stat_wait
> >   sched:sched_stat_sleep
> >   sched:sched_stat_iowait
> >   sched:sched_stat_runtime
> >   sched:sched_process_fork
> >   sched:sched_wakeup
> >   sched:sched_wakeup_new
> >   sched:sched_migrate_task
> >   # Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events
> 
> Ok, almost there, question is: if I ask explicitely for
> "--trace-fields", why should we have the "trace_fields=" in all lines,
> instead of just:
> 
>  
>    $ perf evlist -i perf.data.sched --trace-fields
>    sched:sched_switch: prev_comm,prev_pid,prev_prio,prev_state,next_comm,next_pid,next_prio
>    sched:sched_stat_wait: comm,pid,delay
> 
> ?

I made it to work with other options too, like 'perf evlist --freq --trace-fields'.
In that case you may want to see it. :)

Thanks,
Namhyung


> 
> I like the lack of spaces after commans, this way we can double click
> and select the whole list, then edit it, etc.
> 
> - Arnaldo
>  
> >   $ perf evlist -i perf.data.sched --trace-fields
> >   sched:sched_switch: trace_fields=prev_comm,prev_pid,prev_prio,prev_state,next_comm,next_pid,next_prio
> >   sched:sched_stat_wait: trace_fields=comm,pid,delay
> >   sched:sched_stat_sleep: trace_fields=comm,pid,delay
> >   sched:sched_stat_iowait: trace_fields=comm,pid,delay
> >   sched:sched_stat_runtime: trace_fields=comm,pid,runtime,vruntime
> >   sched:sched_process_fork: trace_fields=parent_comm,parent_pid,child_comm,child_pid
> >   sched:sched_wakeup: trace_fields=comm,pid,prio,success,target_cpu
> >   sched:sched_wakeup_new: trace_fields=comm,pid,prio,success,target_cpu
> >   sched:sched_migrate_task: trace_fields=comm,pid,prio,orig_cpu,dest_cpu
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/Documentation/perf-evlist.txt |  3 +++
> >  tools/perf/builtin-evlist.c              | 11 ++++++++++-
> >  tools/perf/util/evsel.c                  | 23 +++++++++++++++++++++++
> >  tools/perf/util/evsel.h                  |  1 +
> >  4 files changed, 37 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/Documentation/perf-evlist.txt b/tools/perf/Documentation/perf-evlist.txt
> > index 1ceb3700ffbb..6f7200fb85cf 100644
> > --- a/tools/perf/Documentation/perf-evlist.txt
> > +++ b/tools/perf/Documentation/perf-evlist.txt
> > @@ -32,6 +32,9 @@ OPTIONS
> >  --group::
> >  	Show event group information.
> >  
> > +--trace-fields::
> > +	Show tracepoint field names.
> > +
> >  SEE ALSO
> >  --------
> >  linkperf:perf-record[1], linkperf:perf-list[1],
> > diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
> > index 08a7d36a2cf8..8a31f511e1a0 100644
> > --- a/tools/perf/builtin-evlist.c
> > +++ b/tools/perf/builtin-evlist.c
> > @@ -26,14 +26,22 @@ static int __cmd_evlist(const char *file_name, struct perf_attr_details *details
> >  		.mode = PERF_DATA_MODE_READ,
> >  		.force = details->force,
> >  	};
> > +	bool has_tracepoint = false;
> >  
> >  	session = perf_session__new(&file, 0, NULL);
> >  	if (session == NULL)
> >  		return -1;
> >  
> > -	evlist__for_each(session->evlist, pos)
> > +	evlist__for_each(session->evlist, pos) {
> >  		perf_evsel__fprintf(pos, details, stdout);
> >  
> > +		if (pos->attr.type == PERF_TYPE_TRACEPOINT)
> > +			has_tracepoint = true;
> > +	}
> > +
> > +	if (has_tracepoint && !details->trace_fields)
> > +		printf("# Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events\n");
> > +
> >  	perf_session__delete(session);
> >  	return 0;
> >  }
> > @@ -49,6 +57,7 @@ int cmd_evlist(int argc, const char **argv, const char *prefix __maybe_unused)
> >  	OPT_BOOLEAN('g', "group", &details.event_group,
> >  		    "Show event group information"),
> >  	OPT_BOOLEAN('f', "force", &details.force, "don't complain, do it"),
> > +	OPT_BOOLEAN(0, "trace-fields", &details.trace_fields, "Show tracepoint fields"),
> >  	OPT_END()
> >  	};
> >  	const char * const evlist_usage[] = {
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index 544e4400de13..b7822c98fcca 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -2298,6 +2298,29 @@ int perf_evsel__fprintf(struct perf_evsel *evsel,
> >  		printed += comma_fprintf(fp, &first, " %s=%" PRIu64,
> >  					 term, (u64)evsel->attr.sample_freq);
> >  	}
> > +
> > +	if (details->trace_fields) {
> > +		struct format_field *field;
> > +
> > +		if (evsel->attr.type != PERF_TYPE_TRACEPOINT) {
> > +			printed += comma_fprintf(fp, &first, " (not a tracepoint)");
> > +			goto out;
> > +		}
> > +
> > +		field = evsel->tp_format->format.fields;
> > +		if (field == NULL) {
> > +			printed += comma_fprintf(fp, &first, " (no trace field)");
> > +			goto out;
> > +		}
> > +
> > +		printed += comma_fprintf(fp, &first, " trace_fields=%s", field->name);
> > +
> > +		field = field->next;
> > +		while (field) {
> > +			printed += comma_fprintf(fp, &first, "%s", field->name);
> > +			field = field->next;
> > +		}
> > +	}
> >  out:
> >  	fputc('\n', fp);
> >  	return ++printed;
> > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > index 5ded1fc0341e..8e75434bd01c 100644
> > --- a/tools/perf/util/evsel.h
> > +++ b/tools/perf/util/evsel.h
> > @@ -369,6 +369,7 @@ struct perf_attr_details {
> >  	bool verbose;
> >  	bool event_group;
> >  	bool force;
> > +	bool trace_fields;
> >  };
> >  
> >  int perf_evsel__fprintf(struct perf_evsel *evsel,
> > -- 
> > 2.6.4

Re: [PATCH v3 3/5] perf tools: Fix dynamic sort keys to sort properly

[Namhyung Kim]

On Wed, Jan 06, 2016 at 08:06:43PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jan 06, 2016 at 09:54:59AM +0900, Namhyung Kim escreveu:
> > Currently, the dynamic sort keys compares trace data using memcmp().
> > But for output sorting, it should check data size and compare by word.
> > Also it sorted strings in reverse order, fix it.
> 
> Can this be broken down in two patches? This is complex code, lets try
> to make it as bisectable as possible.

OK, I'll break out the string part then.  But I think it doesn't help
much to reduce the complexity.

Thanks,
Namhyung


> 
> - Arnaldo
> 
> > 
> > Before)
> > 
> >   $ perf report -F overhead -s prev_pid,next_pid
> >   ...
> >   # Overhead    prev_pid    next_pid
> >   # ........  ..........  ..........
> >   #
> >        0.39%         490           0
> >        9.12%         225           0
> >        0.04%         224           0
> >        0.51%         731         189
> >        0.08%         731           3
> >        0.12%         731           0
> >        4.82%         729           0
> >        0.08%        1229           0
> >        0.20%         715           0
> >        4.78%         189         225
> >   ...
> > 
> > After)
> > 
> >   $ perf report -F overhead -s prev_pid,next_pid
> >   ...
> >   # Overhead    prev_pid    next_pid
> >   # ........  ..........  ..........
> >   #
> >        0.43%           0           7
> >        0.04%           0          11
> >        0.04%           0          12
> >        0.08%           0          14
> >        0.04%           0          17
> >        0.08%           0          19
> >        0.04%           0          22
> >        0.04%           0          27
> >        0.04%           0          37
> >        0.04%           0          42
> >   ...
> > 
> > Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/sort.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 46 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> > index 9618a64875c0..264d2b630549 100644
> > --- a/tools/perf/util/sort.c
> > +++ b/tools/perf/util/sort.c
> > @@ -1798,6 +1798,51 @@ static int64_t __sort__hde_cmp(struct perf_hpp_fmt *fmt,
> >  	return memcmp(a->raw_data + offset, b->raw_data + offset, size);
> >  }
> >  
> > +static int64_t __sort__hde_sort(struct perf_hpp_fmt *fmt,
> > +				struct hist_entry *a, struct hist_entry *b)
> > +{
> > +	struct hpp_dynamic_entry *hde;
> > +	struct format_field *field;
> > +	unsigned offset, size;
> > +	int64_t *a64, *b64;
> > +	int32_t *a32, *b32;
> > +	int16_t *a16, *b16;
> > +
> > +	hde = container_of(fmt, struct hpp_dynamic_entry, hpp);
> > +
> > +	field = hde->field;
> > +	if (field->flags & FIELD_IS_DYNAMIC) {
> > +		unsigned long long dyn;
> > +
> > +		pevent_read_number_field(field, a->raw_data, &dyn);
> > +		offset = dyn & 0xffff;
> > +		size = (dyn >> 16) & 0xffff;
> > +	} else {
> > +		offset = field->offset;
> > +		size = field->size;
> > +	}
> > +
> > +	if (field->flags & FIELD_IS_STRING)
> > +		return strcmp(b->raw_data + offset, a->raw_data + offset);
> > +
> > +	switch (size) {
> > +	case 8:
> > +		a64 = a->raw_data + offset;
> > +		b64 = b->raw_data + offset;
> > +		return *b64 - *a64;
> > +	case 4:
> > +		a32 = a->raw_data + offset;
> > +		b32 = b->raw_data + offset;
> > +		return *b32 - *a32;
> > +	case 2:
> > +		a16 = a->raw_data + offset;
> > +		b16 = b->raw_data + offset;
> > +		return *b16 - *a16;
> > +	default:
> > +		return memcmp(b->raw_data + offset, a->raw_data + offset, size);
> > +	}
> > +}
> > +
> >  bool perf_hpp__is_dynamic_entry(struct perf_hpp_fmt *fmt)
> >  {
> >  	return fmt->cmp == __sort__hde_cmp;
> > @@ -1826,7 +1871,7 @@ __alloc_dynamic_entry(struct perf_evsel *evsel, struct format_field *field)
> >  
> >  	hde->hpp.cmp = __sort__hde_cmp;
> >  	hde->hpp.collapse = __sort__hde_cmp;
> > -	hde->hpp.sort = __sort__hde_cmp;
> > +	hde->hpp.sort = __sort__hde_sort;
> >  
> >  	INIT_LIST_HEAD(&hde->hpp.list);
> >  	INIT_LIST_HEAD(&hde->hpp.sort_list);
> > -- 
> > 2.6.4

Re: [PATCH v3 5/5] perf evlist: Add --trace-fields option to show trace fields

[Arnaldo Carvalho de Melo]

Em Thu, Jan 07, 2016 at 08:21:44AM +0900, Namhyung Kim escreveu:
> On Wed, Jan 06, 2016 at 08:10:51PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Jan 06, 2016 at 09:55:01AM +0900, Namhyung Kim escreveu:
> > > To use dynamic sort keys, it might be good to add an option to see the
> > > list of field names.
> > > 
> > >   $ perf evlist -i perf.data.sched
> > >   sched:sched_switch
> > >   sched:sched_stat_wait
> > >   sched:sched_stat_sleep
> > >   sched:sched_stat_iowait
> > >   sched:sched_stat_runtime
> > >   sched:sched_process_fork
> > >   sched:sched_wakeup
> > >   sched:sched_wakeup_new
> > >   sched:sched_migrate_task
> > >   # Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events
> > 
> > Ok, almost there, question is: if I ask explicitely for
> > "--trace-fields", why should we have the "trace_fields=" in all lines,
> > instead of just:
> > 
> >  
> >    $ perf evlist -i perf.data.sched --trace-fields
> >    sched:sched_switch: prev_comm,prev_pid,prev_prio,prev_state,next_comm,next_pid,next_prio
> >    sched:sched_stat_wait: comm,pid,delay
> > 
> > ?
> 
> I made it to work with other options too, like 'perf evlist --freq --trace-fields'.
> In that case you may want to see it. :)

I see... And then you want to show those at the same time... Would you
have an use case for that? Or would it be better to make them mutually
exclusive? /me unsure...

- Arnaldo
 
> Thanks,
> Namhyung
> 
> 
> > 
> > I like the lack of spaces after commans, this way we can double click
> > and select the whole list, then edit it, etc.
> > 
> > - Arnaldo
> >  
> > >   $ perf evlist -i perf.data.sched --trace-fields
> > >   sched:sched_switch: trace_fields=prev_comm,prev_pid,prev_prio,prev_state,next_comm,next_pid,next_prio
> > >   sched:sched_stat_wait: trace_fields=comm,pid,delay
> > >   sched:sched_stat_sleep: trace_fields=comm,pid,delay
> > >   sched:sched_stat_iowait: trace_fields=comm,pid,delay
> > >   sched:sched_stat_runtime: trace_fields=comm,pid,runtime,vruntime
> > >   sched:sched_process_fork: trace_fields=parent_comm,parent_pid,child_comm,child_pid
> > >   sched:sched_wakeup: trace_fields=comm,pid,prio,success,target_cpu
> > >   sched:sched_wakeup_new: trace_fields=comm,pid,prio,success,target_cpu
> > >   sched:sched_migrate_task: trace_fields=comm,pid,prio,orig_cpu,dest_cpu
> > > 
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  tools/perf/Documentation/perf-evlist.txt |  3 +++
> > >  tools/perf/builtin-evlist.c              | 11 ++++++++++-
> > >  tools/perf/util/evsel.c                  | 23 +++++++++++++++++++++++
> > >  tools/perf/util/evsel.h                  |  1 +
> > >  4 files changed, 37 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/perf/Documentation/perf-evlist.txt b/tools/perf/Documentation/perf-evlist.txt
> > > index 1ceb3700ffbb..6f7200fb85cf 100644
> > > --- a/tools/perf/Documentation/perf-evlist.txt
> > > +++ b/tools/perf/Documentation/perf-evlist.txt
> > > @@ -32,6 +32,9 @@ OPTIONS
> > >  --group::
> > >  	Show event group information.
> > >  
> > > +--trace-fields::
> > > +	Show tracepoint field names.
> > > +
> > >  SEE ALSO
> > >  --------
> > >  linkperf:perf-record[1], linkperf:perf-list[1],
> > > diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
> > > index 08a7d36a2cf8..8a31f511e1a0 100644
> > > --- a/tools/perf/builtin-evlist.c
> > > +++ b/tools/perf/builtin-evlist.c
> > > @@ -26,14 +26,22 @@ static int __cmd_evlist(const char *file_name, struct perf_attr_details *details
> > >  		.mode = PERF_DATA_MODE_READ,
> > >  		.force = details->force,
> > >  	};
> > > +	bool has_tracepoint = false;
> > >  
> > >  	session = perf_session__new(&file, 0, NULL);
> > >  	if (session == NULL)
> > >  		return -1;
> > >  
> > > -	evlist__for_each(session->evlist, pos)
> > > +	evlist__for_each(session->evlist, pos) {
> > >  		perf_evsel__fprintf(pos, details, stdout);
> > >  
> > > +		if (pos->attr.type == PERF_TYPE_TRACEPOINT)
> > > +			has_tracepoint = true;
> > > +	}
> > > +
> > > +	if (has_tracepoint && !details->trace_fields)
> > > +		printf("# Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events\n");
> > > +
> > >  	perf_session__delete(session);
> > >  	return 0;
> > >  }
> > > @@ -49,6 +57,7 @@ int cmd_evlist(int argc, const char **argv, const char *prefix __maybe_unused)
> > >  	OPT_BOOLEAN('g', "group", &details.event_group,
> > >  		    "Show event group information"),
> > >  	OPT_BOOLEAN('f', "force", &details.force, "don't complain, do it"),
> > > +	OPT_BOOLEAN(0, "trace-fields", &details.trace_fields, "Show tracepoint fields"),
> > >  	OPT_END()
> > >  	};
> > >  	const char * const evlist_usage[] = {
> > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > index 544e4400de13..b7822c98fcca 100644
> > > --- a/tools/perf/util/evsel.c
> > > +++ b/tools/perf/util/evsel.c
> > > @@ -2298,6 +2298,29 @@ int perf_evsel__fprintf(struct perf_evsel *evsel,
> > >  		printed += comma_fprintf(fp, &first, " %s=%" PRIu64,
> > >  					 term, (u64)evsel->attr.sample_freq);
> > >  	}
> > > +
> > > +	if (details->trace_fields) {
> > > +		struct format_field *field;
> > > +
> > > +		if (evsel->attr.type != PERF_TYPE_TRACEPOINT) {
> > > +			printed += comma_fprintf(fp, &first, " (not a tracepoint)");
> > > +			goto out;
> > > +		}
> > > +
> > > +		field = evsel->tp_format->format.fields;
> > > +		if (field == NULL) {
> > > +			printed += comma_fprintf(fp, &first, " (no trace field)");
> > > +			goto out;
> > > +		}
> > > +
> > > +		printed += comma_fprintf(fp, &first, " trace_fields=%s", field->name);
> > > +
> > > +		field = field->next;
> > > +		while (field) {
> > > +			printed += comma_fprintf(fp, &first, "%s", field->name);
> > > +			field = field->next;
> > > +		}
> > > +	}
> > >  out:
> > >  	fputc('\n', fp);
> > >  	return ++printed;
> > > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > > index 5ded1fc0341e..8e75434bd01c 100644
> > > --- a/tools/perf/util/evsel.h
> > > +++ b/tools/perf/util/evsel.h
> > > @@ -369,6 +369,7 @@ struct perf_attr_details {
> > >  	bool verbose;
> > >  	bool event_group;
> > >  	bool force;
> > > +	bool trace_fields;
> > >  };
> > >  
> > >  int perf_evsel__fprintf(struct perf_evsel *evsel,
> > > -- 
> > > 2.6.4

Re: [PATCH v3 3/5] perf tools: Fix dynamic sort keys to sort properly

[Arnaldo Carvalho de Melo]

Em Thu, Jan 07, 2016 at 08:26:45AM +0900, Namhyung Kim escreveu:
> On Wed, Jan 06, 2016 at 08:06:43PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Jan 06, 2016 at 09:54:59AM +0900, Namhyung Kim escreveu:
> > > Currently, the dynamic sort keys compares trace data using memcmp().
> > > But for output sorting, it should check data size and compare by word.
> > > Also it sorted strings in reverse order, fix it.
> > 
> > Can this be broken down in two patches? This is complex code, lets try
> > to make it as bisectable as possible.
> 
> OK, I'll break out the string part then.  But I think it doesn't help
> much to reduce the complexity.

Well, number of patches is not a problem, everytime I see a "Also lets
do this other thing" I cringe, it is automatic, sorry :-\

For reviewing its soooo much better to see things nicely separated, and
sometimes I like one part but not the other, so I pick one and continue
discussion on the other, etc.

- Arnaldo
 
> Thanks,
> Namhyung
> 
> 
> > 
> > - Arnaldo
> > 
> > > 
> > > Before)
> > > 
> > >   $ perf report -F overhead -s prev_pid,next_pid
> > >   ...
> > >   # Overhead    prev_pid    next_pid
> > >   # ........  ..........  ..........
> > >   #
> > >        0.39%         490           0
> > >        9.12%         225           0
> > >        0.04%         224           0
> > >        0.51%         731         189
> > >        0.08%         731           3
> > >        0.12%         731           0
> > >        4.82%         729           0
> > >        0.08%        1229           0
> > >        0.20%         715           0
> > >        4.78%         189         225
> > >   ...
> > > 
> > > After)
> > > 
> > >   $ perf report -F overhead -s prev_pid,next_pid
> > >   ...
> > >   # Overhead    prev_pid    next_pid
> > >   # ........  ..........  ..........
> > >   #
> > >        0.43%           0           7
> > >        0.04%           0          11
> > >        0.04%           0          12
> > >        0.08%           0          14
> > >        0.04%           0          17
> > >        0.08%           0          19
> > >        0.04%           0          22
> > >        0.04%           0          27
> > >        0.04%           0          37
> > >        0.04%           0          42
> > >   ...
> > > 
> > > Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  tools/perf/util/sort.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 46 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> > > index 9618a64875c0..264d2b630549 100644
> > > --- a/tools/perf/util/sort.c
> > > +++ b/tools/perf/util/sort.c
> > > @@ -1798,6 +1798,51 @@ static int64_t __sort__hde_cmp(struct perf_hpp_fmt *fmt,
> > >  	return memcmp(a->raw_data + offset, b->raw_data + offset, size);
> > >  }
> > >  
> > > +static int64_t __sort__hde_sort(struct perf_hpp_fmt *fmt,
> > > +				struct hist_entry *a, struct hist_entry *b)
> > > +{
> > > +	struct hpp_dynamic_entry *hde;
> > > +	struct format_field *field;
> > > +	unsigned offset, size;
> > > +	int64_t *a64, *b64;
> > > +	int32_t *a32, *b32;
> > > +	int16_t *a16, *b16;
> > > +
> > > +	hde = container_of(fmt, struct hpp_dynamic_entry, hpp);
> > > +
> > > +	field = hde->field;
> > > +	if (field->flags & FIELD_IS_DYNAMIC) {
> > > +		unsigned long long dyn;
> > > +
> > > +		pevent_read_number_field(field, a->raw_data, &dyn);
> > > +		offset = dyn & 0xffff;
> > > +		size = (dyn >> 16) & 0xffff;
> > > +	} else {
> > > +		offset = field->offset;
> > > +		size = field->size;
> > > +	}
> > > +
> > > +	if (field->flags & FIELD_IS_STRING)
> > > +		return strcmp(b->raw_data + offset, a->raw_data + offset);
> > > +
> > > +	switch (size) {
> > > +	case 8:
> > > +		a64 = a->raw_data + offset;
> > > +		b64 = b->raw_data + offset;
> > > +		return *b64 - *a64;
> > > +	case 4:
> > > +		a32 = a->raw_data + offset;
> > > +		b32 = b->raw_data + offset;
> > > +		return *b32 - *a32;
> > > +	case 2:
> > > +		a16 = a->raw_data + offset;
> > > +		b16 = b->raw_data + offset;
> > > +		return *b16 - *a16;
> > > +	default:
> > > +		return memcmp(b->raw_data + offset, a->raw_data + offset, size);
> > > +	}
> > > +}
> > > +
> > >  bool perf_hpp__is_dynamic_entry(struct perf_hpp_fmt *fmt)
> > >  {
> > >  	return fmt->cmp == __sort__hde_cmp;
> > > @@ -1826,7 +1871,7 @@ __alloc_dynamic_entry(struct perf_evsel *evsel, struct format_field *field)
> > >  
> > >  	hde->hpp.cmp = __sort__hde_cmp;
> > >  	hde->hpp.collapse = __sort__hde_cmp;
> > > -	hde->hpp.sort = __sort__hde_cmp;
> > > +	hde->hpp.sort = __sort__hde_sort;
> > >  
> > >  	INIT_LIST_HEAD(&hde->hpp.list);
> > >  	INIT_LIST_HEAD(&hde->hpp.sort_list);
> > > -- 
> > > 2.6.4

Re: [PATCH v3 5/5] perf evlist: Add --trace-fields option to show trace fields

[Namhyung Kim]

On Wed, Jan 06, 2016 at 08:29:49PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 07, 2016 at 08:21:44AM +0900, Namhyung Kim escreveu:
> > On Wed, Jan 06, 2016 at 08:10:51PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Wed, Jan 06, 2016 at 09:55:01AM +0900, Namhyung Kim escreveu:
> > > > To use dynamic sort keys, it might be good to add an option to see the
> > > > list of field names.
> > > > 
> > > >   $ perf evlist -i perf.data.sched
> > > >   sched:sched_switch
> > > >   sched:sched_stat_wait
> > > >   sched:sched_stat_sleep
> > > >   sched:sched_stat_iowait
> > > >   sched:sched_stat_runtime
> > > >   sched:sched_process_fork
> > > >   sched:sched_wakeup
> > > >   sched:sched_wakeup_new
> > > >   sched:sched_migrate_task
> > > >   # Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events
> > > 
> > > Ok, almost there, question is: if I ask explicitely for
> > > "--trace-fields", why should we have the "trace_fields=" in all lines,
> > > instead of just:
> > > 
> > >  
> > >    $ perf evlist -i perf.data.sched --trace-fields
> > >    sched:sched_switch: prev_comm,prev_pid,prev_prio,prev_state,next_comm,next_pid,next_prio
> > >    sched:sched_stat_wait: comm,pid,delay
> > > 
> > > ?
> > 
> > I made it to work with other options too, like 'perf evlist --freq --trace-fields'.
> > In that case you may want to see it. :)
> 
> I see... And then you want to show those at the same time... Would you
> have an use case for that? Or would it be better to make them mutually
> exclusive? /me unsure...

I don't have one.  But I think I sometimes want to see it with
-v/--verbose option.  Hmm.. do you think --verbose should imply
--trace-fields?

Thanks,
Namhyung


> > > 
> > > I like the lack of spaces after commans, this way we can double click
> > > and select the whole list, then edit it, etc.
> > > 
> > > - Arnaldo
> > >  
> > > >   $ perf evlist -i perf.data.sched --trace-fields
> > > >   sched:sched_switch: trace_fields=prev_comm,prev_pid,prev_prio,prev_state,next_comm,next_pid,next_prio
> > > >   sched:sched_stat_wait: trace_fields=comm,pid,delay
> > > >   sched:sched_stat_sleep: trace_fields=comm,pid,delay
> > > >   sched:sched_stat_iowait: trace_fields=comm,pid,delay
> > > >   sched:sched_stat_runtime: trace_fields=comm,pid,runtime,vruntime
> > > >   sched:sched_process_fork: trace_fields=parent_comm,parent_pid,child_comm,child_pid
> > > >   sched:sched_wakeup: trace_fields=comm,pid,prio,success,target_cpu
> > > >   sched:sched_wakeup_new: trace_fields=comm,pid,prio,success,target_cpu
> > > >   sched:sched_migrate_task: trace_fields=comm,pid,prio,orig_cpu,dest_cpu
> > > > 
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > ---
> > > >  tools/perf/Documentation/perf-evlist.txt |  3 +++
> > > >  tools/perf/builtin-evlist.c              | 11 ++++++++++-
> > > >  tools/perf/util/evsel.c                  | 23 +++++++++++++++++++++++
> > > >  tools/perf/util/evsel.h                  |  1 +
> > > >  4 files changed, 37 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/perf/Documentation/perf-evlist.txt b/tools/perf/Documentation/perf-evlist.txt
> > > > index 1ceb3700ffbb..6f7200fb85cf 100644
> > > > --- a/tools/perf/Documentation/perf-evlist.txt
> > > > +++ b/tools/perf/Documentation/perf-evlist.txt
> > > > @@ -32,6 +32,9 @@ OPTIONS
> > > >  --group::
> > > >  	Show event group information.
> > > >  
> > > > +--trace-fields::
> > > > +	Show tracepoint field names.
> > > > +
> > > >  SEE ALSO
> > > >  --------
> > > >  linkperf:perf-record[1], linkperf:perf-list[1],
> > > > diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
> > > > index 08a7d36a2cf8..8a31f511e1a0 100644
> > > > --- a/tools/perf/builtin-evlist.c
> > > > +++ b/tools/perf/builtin-evlist.c
> > > > @@ -26,14 +26,22 @@ static int __cmd_evlist(const char *file_name, struct perf_attr_details *details
> > > >  		.mode = PERF_DATA_MODE_READ,
> > > >  		.force = details->force,
> > > >  	};
> > > > +	bool has_tracepoint = false;
> > > >  
> > > >  	session = perf_session__new(&file, 0, NULL);
> > > >  	if (session == NULL)
> > > >  		return -1;
> > > >  
> > > > -	evlist__for_each(session->evlist, pos)
> > > > +	evlist__for_each(session->evlist, pos) {
> > > >  		perf_evsel__fprintf(pos, details, stdout);
> > > >  
> > > > +		if (pos->attr.type == PERF_TYPE_TRACEPOINT)
> > > > +			has_tracepoint = true;
> > > > +	}
> > > > +
> > > > +	if (has_tracepoint && !details->trace_fields)
> > > > +		printf("# Tip: use 'perf evlist --trace-fields' to show fields for tracepoint events\n");
> > > > +
> > > >  	perf_session__delete(session);
> > > >  	return 0;
> > > >  }
> > > > @@ -49,6 +57,7 @@ int cmd_evlist(int argc, const char **argv, const char *prefix __maybe_unused)
> > > >  	OPT_BOOLEAN('g', "group", &details.event_group,
> > > >  		    "Show event group information"),
> > > >  	OPT_BOOLEAN('f', "force", &details.force, "don't complain, do it"),
> > > > +	OPT_BOOLEAN(0, "trace-fields", &details.trace_fields, "Show tracepoint fields"),
> > > >  	OPT_END()
> > > >  	};
> > > >  	const char * const evlist_usage[] = {
> > > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > > index 544e4400de13..b7822c98fcca 100644
> > > > --- a/tools/perf/util/evsel.c
> > > > +++ b/tools/perf/util/evsel.c
> > > > @@ -2298,6 +2298,29 @@ int perf_evsel__fprintf(struct perf_evsel *evsel,
> > > >  		printed += comma_fprintf(fp, &first, " %s=%" PRIu64,
> > > >  					 term, (u64)evsel->attr.sample_freq);
> > > >  	}
> > > > +
> > > > +	if (details->trace_fields) {
> > > > +		struct format_field *field;
> > > > +
> > > > +		if (evsel->attr.type != PERF_TYPE_TRACEPOINT) {
> > > > +			printed += comma_fprintf(fp, &first, " (not a tracepoint)");
> > > > +			goto out;
> > > > +		}
> > > > +
> > > > +		field = evsel->tp_format->format.fields;
> > > > +		if (field == NULL) {
> > > > +			printed += comma_fprintf(fp, &first, " (no trace field)");
> > > > +			goto out;
> > > > +		}
> > > > +
> > > > +		printed += comma_fprintf(fp, &first, " trace_fields=%s", field->name);
> > > > +
> > > > +		field = field->next;
> > > > +		while (field) {
> > > > +			printed += comma_fprintf(fp, &first, "%s", field->name);
> > > > +			field = field->next;
> > > > +		}
> > > > +	}
> > > >  out:
> > > >  	fputc('\n', fp);
> > > >  	return ++printed;
> > > > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > > > index 5ded1fc0341e..8e75434bd01c 100644
> > > > --- a/tools/perf/util/evsel.h
> > > > +++ b/tools/perf/util/evsel.h
> > > > @@ -369,6 +369,7 @@ struct perf_attr_details {
> > > >  	bool verbose;
> > > >  	bool event_group;
> > > >  	bool force;
> > > > +	bool trace_fields;
> > > >  };
> > > >  
> > > >  int perf_evsel__fprintf(struct perf_evsel *evsel,
> > > > -- 
> > > > 2.6.4

Re: [PATCH v3 3/5] perf tools: Fix dynamic sort keys to sort properly

[Namhyung Kim]

On Wed, Jan 06, 2016 at 08:31:49PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 07, 2016 at 08:26:45AM +0900, Namhyung Kim escreveu:
> > On Wed, Jan 06, 2016 at 08:06:43PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Wed, Jan 06, 2016 at 09:54:59AM +0900, Namhyung Kim escreveu:
> > > > Currently, the dynamic sort keys compares trace data using memcmp().
> > > > But for output sorting, it should check data size and compare by word.
> > > > Also it sorted strings in reverse order, fix it.
> > > 
> > > Can this be broken down in two patches? This is complex code, lets try
> > > to make it as bisectable as possible.
> > 
> > OK, I'll break out the string part then.  But I think it doesn't help
> > much to reduce the complexity.
> 
> Well, number of patches is not a problem, everytime I see a "Also lets
> do this other thing" I cringe, it is automatic, sorry :-\
> 
> For reviewing its soooo much better to see things nicely separated, and
> sometimes I like one part but not the other, so I pick one and continue
> discussion on the other, etc.

OK, I understand your concern.  I'll try to make it more easier to review.

Thanks,
Namhyung


> > > 
> > > > 
> > > > Before)
> > > > 
> > > >   $ perf report -F overhead -s prev_pid,next_pid
> > > >   ...
> > > >   # Overhead    prev_pid    next_pid
> > > >   # ........  ..........  ..........
> > > >   #
> > > >        0.39%         490           0
> > > >        9.12%         225           0
> > > >        0.04%         224           0
> > > >        0.51%         731         189
> > > >        0.08%         731           3
> > > >        0.12%         731           0
> > > >        4.82%         729           0
> > > >        0.08%        1229           0
> > > >        0.20%         715           0
> > > >        4.78%         189         225
> > > >   ...
> > > > 
> > > > After)
> > > > 
> > > >   $ perf report -F overhead -s prev_pid,next_pid
> > > >   ...
> > > >   # Overhead    prev_pid    next_pid
> > > >   # ........  ..........  ..........
> > > >   #
> > > >        0.43%           0           7
> > > >        0.04%           0          11
> > > >        0.04%           0          12
> > > >        0.08%           0          14
> > > >        0.04%           0          17
> > > >        0.08%           0          19
> > > >        0.04%           0          22
> > > >        0.04%           0          27
> > > >        0.04%           0          37
> > > >        0.04%           0          42
> > > >   ...
> > > > 
> > > > Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > ---
> > > >  tools/perf/util/sort.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 46 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> > > > index 9618a64875c0..264d2b630549 100644
> > > > --- a/tools/perf/util/sort.c
> > > > +++ b/tools/perf/util/sort.c
> > > > @@ -1798,6 +1798,51 @@ static int64_t __sort__hde_cmp(struct perf_hpp_fmt *fmt,
> > > >  	return memcmp(a->raw_data + offset, b->raw_data + offset, size);
> > > >  }
> > > >  
> > > > +static int64_t __sort__hde_sort(struct perf_hpp_fmt *fmt,
> > > > +				struct hist_entry *a, struct hist_entry *b)
> > > > +{
> > > > +	struct hpp_dynamic_entry *hde;
> > > > +	struct format_field *field;
> > > > +	unsigned offset, size;
> > > > +	int64_t *a64, *b64;
> > > > +	int32_t *a32, *b32;
> > > > +	int16_t *a16, *b16;
> > > > +
> > > > +	hde = container_of(fmt, struct hpp_dynamic_entry, hpp);
> > > > +
> > > > +	field = hde->field;
> > > > +	if (field->flags & FIELD_IS_DYNAMIC) {
> > > > +		unsigned long long dyn;
> > > > +
> > > > +		pevent_read_number_field(field, a->raw_data, &dyn);
> > > > +		offset = dyn & 0xffff;
> > > > +		size = (dyn >> 16) & 0xffff;
> > > > +	} else {
> > > > +		offset = field->offset;
> > > > +		size = field->size;
> > > > +	}
> > > > +
> > > > +	if (field->flags & FIELD_IS_STRING)
> > > > +		return strcmp(b->raw_data + offset, a->raw_data + offset);
> > > > +
> > > > +	switch (size) {
> > > > +	case 8:
> > > > +		a64 = a->raw_data + offset;
> > > > +		b64 = b->raw_data + offset;
> > > > +		return *b64 - *a64;
> > > > +	case 4:
> > > > +		a32 = a->raw_data + offset;
> > > > +		b32 = b->raw_data + offset;
> > > > +		return *b32 - *a32;
> > > > +	case 2:
> > > > +		a16 = a->raw_data + offset;
> > > > +		b16 = b->raw_data + offset;
> > > > +		return *b16 - *a16;
> > > > +	default:
> > > > +		return memcmp(b->raw_data + offset, a->raw_data + offset, size);
> > > > +	}
> > > > +}
> > > > +
> > > >  bool perf_hpp__is_dynamic_entry(struct perf_hpp_fmt *fmt)
> > > >  {
> > > >  	return fmt->cmp == __sort__hde_cmp;
> > > > @@ -1826,7 +1871,7 @@ __alloc_dynamic_entry(struct perf_evsel *evsel, struct format_field *field)
> > > >  
> > > >  	hde->hpp.cmp = __sort__hde_cmp;
> > > >  	hde->hpp.collapse = __sort__hde_cmp;
> > > > -	hde->hpp.sort = __sort__hde_cmp;
> > > > +	hde->hpp.sort = __sort__hde_sort;
> > > >  
> > > >  	INIT_LIST_HEAD(&hde->hpp.list);
> > > >  	INIT_LIST_HEAD(&hde->hpp.sort_list);
> > > > -- 
> > > > 2.6.4

Re: [PATCH v3 3/5] perf tools: Fix dynamic sort keys to sort properly

[Arnaldo Carvalho de Melo]

Em Wed, Jan 06, 2016 at 08:31:49PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Jan 07, 2016 at 08:26:45AM +0900, Namhyung Kim escreveu:
> > On Wed, Jan 06, 2016 at 08:06:43PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Wed, Jan 06, 2016 at 09:54:59AM +0900, Namhyung Kim escreveu:
> > > > Currently, the dynamic sort keys compares trace data using memcmp().
> > > > But for output sorting, it should check data size and compare by word.
> > > > Also it sorted strings in reverse order, fix it.
> > > 
> > > Can this be broken down in two patches? This is complex code, lets try
> > > to make it as bisectable as possible.
> > 
> > OK, I'll break out the string part then.  But I think it doesn't help
> > much to reduce the complexity.
> 
> Well, number of patches is not a problem, everytime I see a "Also lets
> do this other thing" I cringe, it is automatic, sorry :-\
> 
> For reviewing its soooo much better to see things nicely separated, and
> sometimes I like one part but not the other, so I pick one and continue
> discussion on the other, etc.

Ah, please rebase from my latest perf/core, I'm still holding on it
since some 'perf test' entries are failing and I want to check first if
its due to bugs introduced in this branch...

- Arnaldo

Re: [PATCH v3 3/5] perf tools: Fix dynamic sort keys to sort properly

[Namhyung Kim]

On Wed, Jan 06, 2016 at 08:50:45PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jan 06, 2016 at 08:31:49PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Jan 07, 2016 at 08:26:45AM +0900, Namhyung Kim escreveu:
> > > On Wed, Jan 06, 2016 at 08:06:43PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Wed, Jan 06, 2016 at 09:54:59AM +0900, Namhyung Kim escreveu:
> > > > > Currently, the dynamic sort keys compares trace data using memcmp().
> > > > > But for output sorting, it should check data size and compare by word.
> > > > > Also it sorted strings in reverse order, fix it.
> > > > 
> > > > Can this be broken down in two patches? This is complex code, lets try
> > > > to make it as bisectable as possible.
> > > 
> > > OK, I'll break out the string part then.  But I think it doesn't help
> > > much to reduce the complexity.
> > 
> > Well, number of patches is not a problem, everytime I see a "Also lets
> > do this other thing" I cringe, it is automatic, sorry :-\
> > 
> > For reviewing its soooo much better to see things nicely separated, and
> > sometimes I like one part but not the other, so I pick one and continue
> > discussion on the other, etc.
> 
> Ah, please rebase from my latest perf/core, I'm still holding on it
> since some 'perf test' entries are failing and I want to check first if
> its due to bugs introduced in this branch...

Will do!

Thanks,
Namhyung