Skip to contentSkip to conversation

perf: inherit_stat related fixes

PATCH In review Jiri Olsa 6 participants 24 messages Aug 24, 2017 – Sep 1, 2017

Subsystems (2)
lore
Share & tools
lore

Jiri Olsa

author reviewer
#0 lore raw

cover perf: inherit_stat related fixes

hi,
Mark reported an issue with inherit_stat code
and posted a fix [1].

I found we have some issues with that in perf
tool as well, so sending his fix together with
perf tool fixes.

The patchset is also available in:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
perf/values

thanks,
jirka

[1] https://marc.info/?l=linux-kernel&m=150238662928203&w=2

---
Jiri Olsa (10):
perf/x86: Add warning on proper cpu during event's update
perf: Fix leader for removed sibling event in perf_group_detach
perf: Make sure we read only scheduled events
perf record: Set read_format for inherit_stat
perf report: Add dump_read function
perf values: Fix thread index bug
perf values: Fix allocation check
perf values: Zero value buffers
perf report: Group stat values on global event id perf stat: Support inherit/no-inherit terms

 arch/x86/events/core.c         |  2 ++
 kernel/events/core.c           | 11 ++++++++---
 tools/perf/builtin-report.c    |  6 +-----
 tools/perf/builtin-stat.c      | 19 +++++++++++++++++++
 tools/perf/util/evsel.c        |  7 ++++++-
 tools/perf/util/parse-events.c |  2 ++
 tools/perf/util/session.c      | 25 +++++++++++++++++++++++++
 tools/perf/util/values.c       | 17 +++++++++++------
 8 files changed, 74 insertions(+), 15 deletions(-)
author reviewer
#1 lore raw

PATCH 1/10 perf/x86: Add warning on proper cpu during event's update

Mark reported that we could actually call pmu->read on unscheduled event. I think it's good idea to keep a warning here to see if we've get it wrong again in future.

---
 arch/x86/events/core.c | 2 ++
 1 file changed, 2 insertions(+)
arch/x86/events/core.c +2 -0
index af12e294caed..b8e394d9f7f2 100644
@@ -72,6 +72,8 @@ u64 x86_perf_event_update(struct perf_event *event)
 	int idx = hwc->idx;
 	u64 delta;
 
+	WARN_ON_ONCE(event->oncpu != smp_processor_id());
+
 	if (idx == INTEL_PMC_IDX_FIXED_BTS)
 		return 0;
 
Signature · 1 line
2.9.5
maintainer
#2 lore raw

Re: [PATCH 01/10] perf/x86: Add warning on proper cpu during event's update

Quoted 7 lines · Jiri Olsa

Mark reported that we could actually call pmu->read on unscheduled event. I think it's good idea to keep a warning here to see if we've get it wrong again in future.

Yeah, no objection to this one.

Ack

Quoted 20 lines
---
 arch/x86/events/core.c | 2 ++
 1 file changed, 2 insertions(+)
arch/x86/events/core.c +2 -0
index af12e294caed..b8e394d9f7f2 100644
@@ -72,6 +72,8 @@ u64 x86_perf_event_update(struct perf_event *event)
 	int idx = hwc->idx;
 	u64 delta;
 
+	WARN_ON_ONCE(event->oncpu != smp_processor_id());
+
 	if (idx == INTEL_PMC_IDX_FIXED_BTS)
 		return 0;
 
Signature · 1 line
2.9.5
author reviewer
#3 lore raw

PATCH 2/10 perf: Fix leader for removed sibling event in perf_group_detach

In reply to Jiri Olsa

Mark reported warning in x86_perf_event_update caused by reading unscheduled leader of an event that was already removed from the group.

As he pointed out we don't properly reset event's leader once it's been detached from the group and he posted the attached fix.

[1] https://marc.info/?l=linux-kernel&m=150238662928203&w=2

Originally-From: Mark Rutland <[email protected]>

---
 kernel/events/core.c | 1 +
 1 file changed, 1 insertion(+)
kernel/events/core.c +1 -0
index d704e23914bf..30e30e94ea32 100644
@@ -1727,6 +1727,7 @@ static void perf_group_detach(struct perf_event *event)
 	if (event->group_leader != event) {
 		list_del_init(&event->group_entry);
 		event->group_leader->nr_siblings--;
+		event->group_leader = event;
 		goto out;
 	}
 
Signature · 1 line
2.9.5
maintainer
#4 lore raw

Re: [PATCH 02/10] perf: Fix leader for removed sibling event in perf_group_detach

Quoted 21 lines · Jiri Olsa

Mark reported warning in x86_perf_event_update caused by reading unscheduled leader of an event that was already removed from the group.

As he pointed out we don't properly reset event's leader once it's been detached from the group and he posted the attached fix.

[1] https://marc.info/?l=linux-kernel&m=150238662928203&w=2

Originally-From: Mark Rutland <[email protected]>

---
 kernel/events/core.c | 1 +
 1 file changed, 1 insertion(+)
kernel/events/core.c +0 -0
index d704e23914bf..30e30e94ea32 100644

kernel/events/core.c static void perf_group_detach(struct perf_event *event) · line 1732

Quoted 6 lines
kernel/events/core.c +1 -0
@@ -1727,6 +1727,7 @@ static void perf_group_detach(struct perf_event *event)
 	if (event->group_leader != event) {
 		list_del_init(&event->group_entry);
 		event->group_leader->nr_siblings--;
+		event->group_leader = event;
 		goto out;
 	}

No, I think this breaks other things... but I again forgot what. The original issue he reported is fixed by patch:

2bcb88eb04d3 ("perf/core: fix group {cpu,task} validation")

Also by Mark.

author reviewer
#5 lore raw

PATCH 3/10 perf: Make sure we read only scheduled events

In reply to Jiri Olsa

Adding leader's state check into perf_output_read_group to ensure we read only leader, which is scheduled in.

Similar check is already there for siblings.

---
 kernel/events/core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
kernel/events/core.c +7 -3
index 30e30e94ea32..9a2791afe051 100644
@@ -5760,6 +5760,11 @@ void perf_event__output_id_sample(struct perf_event *event,
 		__perf_event__output_id_sample(handle, sample);
 }
 
+static bool can_read(struct perf_event *event)
+{
+	return event->state == PERF_EVENT_STATE_ACTIVE;
+}
+
 static void perf_output_read_one(struct perf_output_handle *handle,
 				 struct perf_event *event,
 				 u64 enabled, u64 running)
@@ -5800,7 +5805,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
 		values[n++] = running;
 
-	if (leader != event)
+	if ((leader != event) && can_read(leader))
 		leader->pmu->read(leader);
 
 	values[n++] = perf_event_count(leader);
@@ -5812,8 +5817,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 	list_for_each_entry(sub, &leader->sibling_list, group_entry) {
 		n = 0;
 
-		if ((sub != event) &&
-		    (sub->state == PERF_EVENT_STATE_ACTIVE))
+		if ((sub != event) && can_read(sub))
 			sub->pmu->read(sub);
 
 		values[n++] = perf_event_count(sub);
Signature · 1 line
2.9.5
maintainer
#6 lore raw

Re: [PATCH 03/10] perf: Make sure we read only scheduled events

Quoted 36 lines · Jiri Olsa

Adding leader's state check into perf_output_read_group to ensure we read only leader, which is scheduled in.

Similar check is already there for siblings.

---
 kernel/events/core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
kernel/events/core.c +6 -1
index 30e30e94ea32..9a2791afe051 100644
@@ -5760,6 +5760,11 @@ void perf_event__output_id_sample(struct perf_event *event,
 		__perf_event__output_id_sample(handle, sample);
 }
 
+static bool can_read(struct perf_event *event)
+{
+	return event->state == PERF_EVENT_STATE_ACTIVE;
+}
+
 static void perf_output_read_one(struct perf_output_handle *handle,
 				 struct perf_event *event,
 				 u64 enabled, u64 running)
@@ -5800,7 +5805,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
 		values[n++] = running;
 
-	if (leader != event)
+	if ((leader != event) && can_read(leader))
 		leader->pmu->read(leader);
 
 	values[n++] = perf_event_count(leader);

kernel/events/core.c static void perf_output_read_group(struct perf_output_handle *handle, · line 5823

Quoted 9 lines
kernel/events/core.c +1 -2
@@ -5812,8 +5817,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 	list_for_each_entry(sub, &leader->sibling_list, group_entry) {
 		n = 0;
 
-		if ((sub != event) &&
-		    (sub->state == PERF_EVENT_STATE_ACTIVE))
+		if ((sub != event) && can_read(sub))
 			sub->pmu->read(sub);
 
 		values[n++] = perf_event_count(sub);

I'm not seeing how this makes sense. Groups should either _all_ be scheduled or not at all. Please explain.

#7 lore raw

Re: [PATCH 03/10] perf: Make sure we read only scheduled events

Quoted 45 lines · Peter Zijlstra
Quoted 44 lines · Jiri Olsa

Adding leader's state check into perf_output_read_group to ensure we read only leader, which is scheduled in.

Similar check is already there for siblings.

---
 kernel/events/core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
kernel/events/core.c +7 -3
index 30e30e94ea32..9a2791afe051 100644
@@ -5760,6 +5760,11 @@ void perf_event__output_id_sample(struct perf_event *event,
 		__perf_event__output_id_sample(handle, sample);
 }
 
+static bool can_read(struct perf_event *event)
+{
+	return event->state == PERF_EVENT_STATE_ACTIVE;
+}
+
 static void perf_output_read_one(struct perf_output_handle *handle,
 				 struct perf_event *event,
 				 u64 enabled, u64 running)
@@ -5800,7 +5805,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
 		values[n++] = running;
 
-	if (leader != event)
+	if ((leader != event) && can_read(leader))
 		leader->pmu->read(leader);
 
 	values[n++] = perf_event_count(leader);
@@ -5812,8 +5817,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 	list_for_each_entry(sub, &leader->sibling_list, group_entry) {
 		n = 0;
 
-		if ((sub != event) &&
-		    (sub->state == PERF_EVENT_STATE_ACTIVE))
+		if ((sub != event) && can_read(sub))
 			sub->pmu->read(sub);
 
Quoted 4 lines

kernel/events/core.c static void perf_output_read_group(struct perf_output_handle *handle, · line 5823

Quoted 1 line
kernel/events/core.c +0 -0
 		values[n++] = perf_event_count(sub);

I'm not seeing how this makes sense. Groups should either _all_ be scheduled or not at all. Please explain.

so this could be called for event which is already scheduled out:

perf_event_exit_task_context
task_ctx_sched_out <- unschedules event
perf_event_exit_event
sync_child_event
perf_event_read_event
perf_output_read

if leader != events (which is, if you don't have Mark's fix), we'll call leader->pmu->read(leader) even if it's not scheduled in

jirka

author reviewer
#8 lore raw

PATCH 4/10 perf record: Set read_format for inherit_stat

In reply to Jiri Olsa

Set read_format for what we expect to get from read event generated by perf_event_attr::inherit_stat.

---
 tools/perf/util/evsel.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
tools/perf/util/evsel.c +6 -1
index a5888c704e01..d9bd632ed7db 100644
@@ -902,8 +902,13 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
 	if (opts->no_samples)
 		attr->sample_freq = 0;
 
-	if (opts->inherit_stat)
+	if (opts->inherit_stat) {
+		evsel->attr.read_format |=
+			PERF_FORMAT_TOTAL_TIME_ENABLED |
+			PERF_FORMAT_TOTAL_TIME_RUNNING |
+			PERF_FORMAT_ID;
 		attr->inherit_stat = 1;
+	}
 
 	if (opts->sample_address) {
 		perf_evsel__set_sample_bit(evsel, ADDR);
Signature · 1 line
2.9.5
#9 lore raw

[tip:perf/core] perf record: Set read_format for inherit_stat

perf record: Set read_format for inherit_stat
Commit-ID:  a17f06978769735ab5c7598c46881fa201e9b1a2
Gitweb:     http://git.kernel.org/tip/a17f06978769735ab5c7598c46881fa201e9b1a2
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 24 Aug 2017 18:27:31 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 28 Aug 2017 11:05:10 -0300

perf record: Set read_format for inherit_stat

Set read_format for what we expect to get from read event generated by perf_event_attr::inherit_stat.

---
 tools/perf/util/evsel.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
tools/perf/util/evsel.c +6 -1
index a5888c7..d9bd632 100644
@@ -902,8 +902,13 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts,
 	if (opts->no_samples)
 		attr->sample_freq = 0;
 
-	if (opts->inherit_stat)
+	if (opts->inherit_stat) {
+		evsel->attr.read_format |=
+			PERF_FORMAT_TOTAL_TIME_ENABLED |
+			PERF_FORMAT_TOTAL_TIME_RUNNING |
+			PERF_FORMAT_ID;
 		attr->inherit_stat = 1;
+	}
 
 	if (opts->sample_address) {
 		perf_evsel__set_sample_bit(evsel, ADDR);
author reviewer
#10 lore raw

PATCH 5/10 perf report: Add dump_read function

In reply to Jiri Olsa

Adding dump_read function to gather all the dump output of read function. Adding output of enabled and running times and id if enabled (3 new lines with '...' prefix below).

  $ perf record -s ...
  $ perf report -D

958358311769 0x91f8 [0x40]: PERF_RECORD_READ: 3339 3339 cycles:u 0 ... time enabled : 958358313731
... time running : 958358313731
... id : 80

---
 tools/perf/builtin-report.c |  4 ----
 tools/perf/util/session.c   | 25 +++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)
tools/perf/builtin-report.c +0 -4
index bace3429c030..9e4004b08f55 100644
@@ -249,10 +249,6 @@ static int process_read_event(struct perf_tool *tool,
 			return err;
 	}
 
-	dump_printf(": %d %d %s %" PRIu64 "\n", event->read.pid, event->read.tid,
-		    evsel ? perf_evsel__name(evsel) : "FAIL",
-		    event->read.value);
-
 	return 0;
 }
 
tools/perf/util/session.c +25 -0
index dc453f84a14c..4a09604a66a0 100644
@@ -1127,6 +1127,30 @@ static void dump_sample(struct perf_evsel *evsel, union perf_event *event,
 		sample_read__printf(sample, evsel->attr.read_format);
 }
 
+static void dump_read(struct perf_evsel *evsel, union perf_event *event)
+{
+	struct read_event *read = &event->read;
+	u64 read_format;
+
+	if (!dump_trace)
+		return;
+
+	printf(": %d %d %s %" PRIu64 "\n", event->read.pid, event->read.tid,
+	       evsel ? perf_evsel__name(evsel) : "FAIL",
+	       event->read.value);
+
+	read_format = evsel->attr.read_format;
+
+	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
+		printf("... time enabled : %" PRIu64 "\n", read->time_enabled);
+
+	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
+		printf("... time running : %" PRIu64 "\n", read->time_running);
+
+	if (read_format & PERF_FORMAT_ID)
+		printf("... id           : %" PRIu64 "\n", read->id);
+}
+
 static struct machine *machines__find_for_cpumode(struct machines *machines,
 					       union perf_event *event,
 					       struct perf_sample *sample)
@@ -1271,6 +1295,7 @@ static int machines__deliver_event(struct machines *machines,
 			evlist->stats.total_lost_samples += event->lost_samples.lost;
 		return tool->lost_samples(tool, event, sample, machine);
 	case PERF_RECORD_READ:
+		dump_read(evsel, event);
 		return tool->read(tool, event, sample, evsel, machine);
 	case PERF_RECORD_THROTTLE:
 		return tool->throttle(tool, event, sample, machine);
Signature · 1 line
2.9.5
#11 lore raw

[tip:perf/core] perf report: Add dump_read function

perf report: Add dump_read function
Commit-ID:  dac7f6b7ed1c8601358357f60e9764a4c6a68d71
Gitweb:     http://git.kernel.org/tip/dac7f6b7ed1c8601358357f60e9764a4c6a68d71
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 24 Aug 2017 18:27:32 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 28 Aug 2017 16:43:50 -0300

perf report: Add dump_read function

Adding dump_read function to gather all the dump output of read function. Adding output of enabled and running times and id if enabled (3 new lines with '...' prefix below).

  $ perf record -s ...
  $ perf report -D

958358311769 0x91f8 [0x40]: PERF_RECORD_READ: 3339 3339 cycles:u 0 ... time enabled : 958358313731
... time running : 958358313731
... id : 80

Committer note:

Do not use 'read' as a variable name as it breaks the build on older systems, such as RHEL6:

CC /tmp/build/perf/util/session.o
cc1: warnings being treated as errors
util/session.c: In function 'dump_read':
util/session.c:1132: error: declaration of 'read' shadows a global declaration /usr/include/bits/unistd.h:35: error: shadowed declaration is here
mv: cannot stat `/tmp/build/perf/util/.session.o.tmp': No such file or directory

---
 tools/perf/builtin-report.c |  4 ----
 tools/perf/util/session.c   | 25 +++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)
tools/perf/builtin-report.c +0 -4
index bace342..9e4004b0 100644
@@ -249,10 +249,6 @@ static int process_read_event(struct perf_tool *tool,
 			return err;
 	}
 
-	dump_printf(": %d %d %s %" PRIu64 "\n", event->read.pid, event->read.tid,
-		    evsel ? perf_evsel__name(evsel) : "FAIL",
-		    event->read.value);
-
 	return 0;
 }
 
tools/perf/util/session.c +25 -0
index dc453f8..ac86369 100644
@@ -1127,6 +1127,30 @@ static void dump_sample(struct perf_evsel *evsel, union perf_event *event,
 		sample_read__printf(sample, evsel->attr.read_format);
 }
 
+static void dump_read(struct perf_evsel *evsel, union perf_event *event)
+{
+	struct read_event *read_event = &event->read;
+	u64 read_format;
+
+	if (!dump_trace)
+		return;
+
+	printf(": %d %d %s %" PRIu64 "\n", event->read.pid, event->read.tid,
+	       evsel ? perf_evsel__name(evsel) : "FAIL",
+	       event->read.value);
+
+	read_format = evsel->attr.read_format;
+
+	if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
+		printf("... time enabled : %" PRIu64 "\n", read_event->time_enabled);
+
+	if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
+		printf("... time running : %" PRIu64 "\n", read_event->time_running);
+
+	if (read_format & PERF_FORMAT_ID)
+		printf("... id           : %" PRIu64 "\n", read_event->id);
+}
+
 static struct machine *machines__find_for_cpumode(struct machines *machines,
 					       union perf_event *event,
 					       struct perf_sample *sample)
@@ -1271,6 +1295,7 @@ static int machines__deliver_event(struct machines *machines,
 			evlist->stats.total_lost_samples += event->lost_samples.lost;
 		return tool->lost_samples(tool, event, sample, machine);
 	case PERF_RECORD_READ:
+		dump_read(evsel, event);
 		return tool->read(tool, event, sample, evsel, machine);
 	case PERF_RECORD_THROTTLE:
 		return tool->throttle(tool, event, sample, machine);
author reviewer
#12 lore raw

PATCH 6/10 perf values: Fix thread index bug

In reply to Jiri Olsa

We are taking wrong index (+1) for first thread, which leaves thread with index 0 unused and uninitialized.

---
 tools/perf/util/values.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
tools/perf/util/values.c +2 -2
index 5de2e15e2eda..9ac36bf2c438 100644
@@ -98,7 +98,7 @@ static int perf_read_values__findnew_thread(struct perf_read_values *values,
 			return i;
 	}
 
-	i = values->threads + 1;
+	i = values->threads;
 	values->value[i] = malloc(values->counters_max * sizeof(**values->value));
 	if (!values->value[i]) {
 		pr_debug("failed to allocate read_values counters array");
@@ -106,7 +106,7 @@ static int perf_read_values__findnew_thread(struct perf_read_values *values,
 	}
 	values->pid[i] = pid;
 	values->tid[i] = tid;
-	values->threads = i;
+	values->threads = i + 1;
 
 	return i;
 }
Signature · 1 line
2.9.5
#13 lore raw

[tip:perf/core] perf values: Fix thread index bug

perf values: Fix thread index bug
Commit-ID:  64eed1deb6d87f4c0efe03297f50367a3689eb56
Gitweb:     http://git.kernel.org/tip/64eed1deb6d87f4c0efe03297f50367a3689eb56
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 24 Aug 2017 18:27:33 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 28 Aug 2017 16:44:42 -0300

perf values: Fix thread index bug

We are taking wrong index (+1) for first thread, which leaves thread with index 0 unused and uninitialized.

---
 tools/perf/util/values.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
tools/perf/util/values.c +2 -2
index 5de2e15..9ac36bf 100644
@@ -98,7 +98,7 @@ static int perf_read_values__findnew_thread(struct perf_read_values *values,
 			return i;
 	}
 
-	i = values->threads + 1;
+	i = values->threads;
 	values->value[i] = malloc(values->counters_max * sizeof(**values->value));
 	if (!values->value[i]) {
 		pr_debug("failed to allocate read_values counters array");
@@ -106,7 +106,7 @@ static int perf_read_values__findnew_thread(struct perf_read_values *values,
 	}
 	values->pid[i] = pid;
 	values->tid[i] = tid;
-	values->threads = i;
+	values->threads = i + 1;
 
 	return i;
 }
author reviewer
#14 lore raw

PATCH 7/10 perf values: Fix allocation check

In reply to Jiri Olsa

Bailing out in case the allocation failed,
not the other way round.

---
 tools/perf/util/values.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/util/values.c +1 -1
index 9ac36bf2c438..2c4af02f08cd 100644
@@ -131,7 +131,7 @@ static int perf_read_values__enlarge_counters(struct perf_read_values *values)
 	for (i = 0; i < values->threads; i++) {
 		u64 *value = realloc(values->value[i], counters_max * sizeof(**values->value));
 
-		if (value) {
+		if (!value) {
 			pr_debug("failed to enlarge read_values ->values array");
 			goto out_free_name;
 		}
Signature · 1 line
2.9.5
#15 lore raw

[tip:perf/core] perf values: Fix allocation check

perf values: Fix allocation check
Commit-ID:  f4ef3b7c184c4c269f953f226f7158347d007622
Gitweb:     http://git.kernel.org/tip/f4ef3b7c184c4c269f953f226f7158347d007622
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 24 Aug 2017 18:27:34 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 28 Aug 2017 16:44:43 -0300

perf values: Fix allocation check

Bailing out in case the allocation failed, not the other way round.

---
 tools/perf/util/values.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/util/values.c +1 -1
index 9ac36bf..2c4af02 100644
@@ -131,7 +131,7 @@ static int perf_read_values__enlarge_counters(struct perf_read_values *values)
 	for (i = 0; i < values->threads; i++) {
 		u64 *value = realloc(values->value[i], counters_max * sizeof(**values->value));
 
-		if (value) {
+		if (!value) {
 			pr_debug("failed to enlarge read_values ->values array");
 			goto out_free_name;
 		}
author reviewer
#16 lore raw

PATCH 8/10 perf values: Zero value buffers

In reply to Jiri Olsa

We need to make sure the array of value pointers are zero initialized, because we use them in realloc later on and uninitialized non zero value will cause allocation error and aborted execution.

---
 tools/perf/util/values.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
tools/perf/util/values.c +7 -2
index 2c4af02f08cd..3b56aeaa8cbb 100644
@@ -12,7 +12,7 @@ int perf_read_values_init(struct perf_read_values *values)
 	values->threads_max = 16;
 	values->pid = malloc(values->threads_max * sizeof(*values->pid));
 	values->tid = malloc(values->threads_max * sizeof(*values->tid));
-	values->value = malloc(values->threads_max * sizeof(*values->value));
+	values->value = zalloc(values->threads_max * sizeof(*values->value));
 	if (!values->pid || !values->tid || !values->value) {
 		pr_debug("failed to allocate read_values threads arrays");
 		goto out_free_pid;
@@ -99,7 +99,8 @@ static int perf_read_values__findnew_thread(struct perf_read_values *values,
 	}
 
 	i = values->threads;
-	values->value[i] = malloc(values->counters_max * sizeof(**values->value));
+
+	values->value[i] = zalloc(values->counters_max * sizeof(**values->value));
 	if (!values->value[i]) {
 		pr_debug("failed to allocate read_values counters array");
 		return -ENOMEM;
@@ -130,12 +131,16 @@ static int perf_read_values__enlarge_counters(struct perf_read_values *values)
 
 	for (i = 0; i < values->threads; i++) {
 		u64 *value = realloc(values->value[i], counters_max * sizeof(**values->value));
+		int j;
 
 		if (!value) {
 			pr_debug("failed to enlarge read_values ->values array");
 			goto out_free_name;
 		}
 
+		for (j = values->counters_max; j < counters_max; j++)
+			value[j] = 0;
+
 		values->value[i] = value;
 	}
 
Signature · 1 line
2.9.5
#17 lore raw

[tip:perf/core] perf values: Zero value buffers

perf values: Zero value buffers
Commit-ID:  a1834fc938344dd3015a1df64ee7f2af70ded147
Gitweb:     http://git.kernel.org/tip/a1834fc938344dd3015a1df64ee7f2af70ded147
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 24 Aug 2017 18:27:35 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 28 Aug 2017 16:44:43 -0300

perf values: Zero value buffers

We need to make sure the array of value pointers are zero initialized, because we use them in realloc later on and uninitialized non zero value will cause allocation error and aborted execution.

---
 tools/perf/util/values.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
tools/perf/util/values.c +7 -2
index 2c4af02..3b56aea 100644
@@ -12,7 +12,7 @@ int perf_read_values_init(struct perf_read_values *values)
 	values->threads_max = 16;
 	values->pid = malloc(values->threads_max * sizeof(*values->pid));
 	values->tid = malloc(values->threads_max * sizeof(*values->tid));
-	values->value = malloc(values->threads_max * sizeof(*values->value));
+	values->value = zalloc(values->threads_max * sizeof(*values->value));
 	if (!values->pid || !values->tid || !values->value) {
 		pr_debug("failed to allocate read_values threads arrays");
 		goto out_free_pid;
@@ -99,7 +99,8 @@ static int perf_read_values__findnew_thread(struct perf_read_values *values,
 	}
 
 	i = values->threads;
-	values->value[i] = malloc(values->counters_max * sizeof(**values->value));
+
+	values->value[i] = zalloc(values->counters_max * sizeof(**values->value));
 	if (!values->value[i]) {
 		pr_debug("failed to allocate read_values counters array");
 		return -ENOMEM;
@@ -130,12 +131,16 @@ static int perf_read_values__enlarge_counters(struct perf_read_values *values)
 
 	for (i = 0; i < values->threads; i++) {
 		u64 *value = realloc(values->value[i], counters_max * sizeof(**values->value));
+		int j;
 
 		if (!value) {
 			pr_debug("failed to enlarge read_values ->values array");
 			goto out_free_name;
 		}
 
+		for (j = values->counters_max; j < counters_max; j++)
+			value[j] = 0;
+
 		values->value[i] = value;
 	}
 
author reviewer
#18 lore raw

PATCH 9/10 perf report: Group stat values on global event id

In reply to Jiri Olsa

There's no big value on displaying counts for every event ID, which is one per every CPU. Rather than that, displaying the whole sum for the event.

  $ perf record -c 100000 -e cycles:u -s test
  $ perf report -T

Before:
# PID TID cycles:u cycles:u cycles:u cycles:u ... [20 more columns of 'cycles:u']

    3339  3339         0         0         0         0
    3340  3340         0         0         0         0
    3341  3341         0         0         0         0
    3342  3342         0         0         0         0

Now:
# PID TID cycles:u

    3339  3339     19678
    3340  3340     18744
    3341  3341     17335
    3342  3342     26414
---
 tools/perf/builtin-report.c | 2 +-
 tools/perf/util/values.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
tools/perf/builtin-report.c +1 -1
index 9e4004b08f55..f9dff652dcbd 100644
@@ -241,7 +241,7 @@ static int process_read_event(struct perf_tool *tool,
 		const char *name = evsel ? perf_evsel__name(evsel) : "unknown";
 		int err = perf_read_values_add_value(&rep->show_threads_values,
 					   event->read.pid, event->read.tid,
-					   event->read.id,
+					   evsel->idx,
 					   name,
 					   event->read.value);
 
tools/perf/util/values.c +1 -1
index 3b56aeaa8cbb..8a32bb0095e5 100644
@@ -192,7 +192,7 @@ int perf_read_values_add_value(struct perf_read_values *values,
 	if (cindex < 0)
 		return cindex;
 
-	values->value[tindex][cindex] = value;
+	values->value[tindex][cindex] += value;
 	return 0;
 }
 
Signature · 1 line
2.9.5
#19 lore raw

[tip:perf/core] perf report: Group stat values on global event id

perf report: Group stat values on global event id
Commit-ID:  9933183e365f7dd3a79507f1ffb4bcf9433a73ee
Gitweb:     http://git.kernel.org/tip/9933183e365f7dd3a79507f1ffb4bcf9433a73ee
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 24 Aug 2017 18:27:36 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 28 Aug 2017 16:44:44 -0300

perf report: Group stat values on global event id

There's no big value on displaying counts for every event ID, which is one per every CPU. Rather than that, displaying the whole sum for the event.

  $ perf record -c 100000 -e cycles:u -s test
  $ perf report -T

Before:
# PID TID cycles:u cycles:u cycles:u cycles:u ... [20 more columns of 'cycles:u']

    3339  3339         0         0         0         0
    3340  3340         0         0         0         0
    3341  3341         0         0         0         0
    3342  3342         0         0         0         0

Now:
# PID TID cycles:u

    3339  3339     19678
    3340  3340     18744
    3341  3341     17335
    3342  3342     26414
---
 tools/perf/builtin-report.c | 2 +-
 tools/perf/util/values.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
tools/perf/builtin-report.c +1 -1
index 9e4004b0..f9dff65 100644
@@ -241,7 +241,7 @@ static int process_read_event(struct perf_tool *tool,
 		const char *name = evsel ? perf_evsel__name(evsel) : "unknown";
 		int err = perf_read_values_add_value(&rep->show_threads_values,
 					   event->read.pid, event->read.tid,
-					   event->read.id,
+					   evsel->idx,
 					   name,
 					   event->read.value);
 
tools/perf/util/values.c +1 -1
index 3b56aea..8a32bb0 100644
@@ -192,7 +192,7 @@ int perf_read_values_add_value(struct perf_read_values *values,
 	if (cindex < 0)
 		return cindex;
 
-	values->value[tindex][cindex] = value;
+	values->value[tindex][cindex] += value;
 	return 0;
 }
 
author reviewer
#20 lore raw

PATCH 10/10 perf stat: Support inherit/no-inherit terms

In reply to Jiri Olsa

Adding support to use 'inherit' and 'no-inherit' terms in perf stat command, like:

To disable perf_event_attr::inherit (enabled by default):

  $ perf stat -e cpu/cpu-cycles,no-inherit/u ...

Enable perf_event_attr::inherit (disabled by -i):

  $ perf stat -i -e cpu/cpu-cycles,inherit/u ...
---
 tools/perf/builtin-stat.c      | 19 +++++++++++++++++++
 tools/perf/util/parse-events.c |  2 ++
 2 files changed, 21 insertions(+)
tools/perf/builtin-stat.c +19 -0
index 866da7aa54bf..92053b93e452 100644
@@ -210,6 +210,23 @@ static void perf_stat__reset_stats(void)
 	perf_stat__reset_shadow_stats();
 }
 
+static void apply_stat_config_terms(struct perf_evsel *evsel)
+{
+	struct perf_evsel_config_term *term;
+	struct list_head *config_terms = &evsel->config_terms;
+	struct perf_event_attr *attr   = &evsel->attr;
+
+	list_for_each_entry(term, config_terms, list) {
+		switch (term->type) {
+		case PERF_EVSEL__CONFIG_TERM_INHERIT:
+			attr->inherit = term->val.inherit ? 1 : 0;
+			break;
+		default:
+			break;
+		}
+	}
+}
+
 static int create_perf_stat_counter(struct perf_evsel *evsel)
 {
 	struct perf_event_attr *attr = &evsel->attr;
@@ -264,6 +281,8 @@ static int create_perf_stat_counter(struct perf_evsel *evsel)
 			attr->enable_on_exec = 1;
 	}
 
+	apply_stat_config_terms(evsel);
+
 	if (target__has_cpu(&target))
 		return perf_evsel__open_per_cpu(evsel, perf_evsel__cpus(evsel));
 
tools/perf/util/parse-events.c +2 -0
index f44aeba51d1f..b2b448ff0a36 100644
@@ -934,6 +934,8 @@ config_term_avail(int term_type, struct parse_events_error *err)
 	case PARSE_EVENTS__TERM_TYPE_CONFIG2:
 	case PARSE_EVENTS__TERM_TYPE_NAME:
 	case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
+	case PARSE_EVENTS__TERM_TYPE_INHERIT:
+	case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
 		return true;
 	default:
 		if (!err)
Signature · 1 line
2.9.5
#21 lore raw

Re: [PATCH 10/10] perf stat: Support inherit/no-inherit terms

Quoted 10 lines · Jiri Olsa

Adding support to use 'inherit' and 'no-inherit' terms in perf stat command, like:

To disable perf_event_attr::inherit (enabled by default):

  $ perf stat -e cpu/cpu-cycles,no-inherit/u ...

Enable perf_event_attr::inherit (disabled by -i):

  $ perf stat -i -e cpu/cpu-cycles,inherit/u ...

User documentation is missing.

-Andi

#22 lore raw

Re: [PATCH 10/10] perf stat: Support inherit/no-inherit terms

Quoted 6 lines · Andi Kleen
Quoted 5 lines · Jiri Olsa

Adding support to use 'inherit' and 'no-inherit' terms in perf stat command, like:

To disable perf_event_attr::inherit (enabled by default):

Quoted 7 lines
Quoted 5 lines
  $ perf stat -e cpu/cpu-cycles,no-inherit/u ...

Enable perf_event_attr::inherit (disabled by -i):

  $ perf stat -i -e cpu/cpu-cycles,inherit/u ...

User documentation is missing.

right, will add

thanks,
jirka

#23 lore raw

Re: [PATCH 10/10] perf stat: Support inherit/no-inherit terms

Em Thu, Aug 24, 2017 at 06:45:27PM +0200, Jiri Olsa escreveu:

Quoted 9 lines · Andi Kleen
Quoted 8 lines · Jiri Olsa
Quoted 7 lines

Adding support to use 'inherit' and 'no-inherit' terms in perf stat command, like:

To disable perf_event_attr::inherit (enabled by default):

  $ perf stat -e cpu/cpu-cycles,no-inherit/u ...
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

Enable perf_event_attr::inherit (disabled by -i):

  $ perf stat -i -e cpu/cpu-cycles,inherit/u ...

User documentation is missing.

right, will add

Ok, applied 4-9, Peter, are you ok with the kernel parts?

- Arnaldo