Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1719385 > unrolled thread

[PATCH 00/10] perf: inherit_stat related fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/10] perf: inherit_stat related fixes Jiri Olsa <jolsa@kernel.org> - 2017-08-24 18:30 +0200
    [PATCH 07/10] perf values: Fix allocation check Jiri Olsa <jolsa@kernel.org> - 2017-08-24 18:30 +0200
    [PATCH 08/10] perf values: Zero value buffers Jiri Olsa <jolsa@kernel.org> - 2017-08-24 18:30 +0200
    [PATCH 10/10] perf stat: Support inherit/no-inherit terms Jiri Olsa <jolsa@kernel.org> - 2017-08-24 18:30 +0200
      Re: [PATCH 10/10] perf stat: Support inherit/no-inherit terms Andi Kleen <andi@firstfloor.org> - 2017-08-24 18:40 +0200
        Re: [PATCH 10/10] perf stat: Support inherit/no-inherit terms Jiri Olsa <jolsa@redhat.com> - 2017-08-24 18:50 +0200
    [PATCH 02/10] perf: Fix leader for removed sibling event in perf_group_detach Jiri Olsa <jolsa@kernel.org> - 2017-08-24 18:30 +0200
    [PATCH 09/10] perf report: Group stat values on global event id Jiri Olsa <jolsa@kernel.org> - 2017-08-24 18:30 +0200
    [PATCH 05/10] perf report: Add dump_read function Jiri Olsa <jolsa@kernel.org> - 2017-08-24 18:30 +0200
    [PATCH 06/10] perf values: Fix thread index bug Jiri Olsa <jolsa@kernel.org> - 2017-08-24 18:30 +0200
    [PATCH 01/10] perf/x86: Add warning on proper cpu during event's update Jiri Olsa <jolsa@kernel.org> - 2017-08-24 18:40 +0200
    [PATCH 03/10] perf: Make sure we read only scheduled events Jiri Olsa <jolsa@kernel.org> - 2017-08-24 18:40 +0200

#1719385 — [PATCH 00/10] 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(-)

[toc] | [next] | [standalone]

#1719386 — [PATCH 07/10] perf values: Fix allocation check

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

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/values.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/values.c b/tools/perf/util/values.c
index 9ac36bf2c438..2c4af02f08cd 100644
--- a/tools/perf/util/values.c
+++ b/tools/perf/util/values.c
@@ -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;
 		}
-- 
2.9.5

[toc] | [prev] | [next] | [standalone]

#1719387 — [PATCH 08/10] 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.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/values.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/values.c b/tools/perf/util/values.c
index 2c4af02f08cd..3b56aeaa8cbb 100644
--- a/tools/perf/util/values.c
+++ b/tools/perf/util/values.c
@@ -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;
 	}
 
-- 
2.9.5

[toc] | [prev] | [next] | [standalone]

#1719388 — [PATCH 10/10] perf stat: Support inherit/no-inherit terms

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

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-stat.c      | 19 +++++++++++++++++++
 tools/perf/util/parse-events.c |  2 ++
 2 files changed, 21 insertions(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 866da7aa54bf..92053b93e452 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -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));
 
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f44aeba51d1f..b2b448ff0a36 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -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)
-- 
2.9.5

[toc] | [prev] | [next] | [standalone]

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

On Thu, Aug 24, 2017 at 06:27:37PM +0200, Jiri Olsa wrote:
> 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

[toc] | [prev] | [next] | [standalone]

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

On Thu, Aug 24, 2017 at 09:30:34AM -0700, Andi Kleen wrote:
> On Thu, Aug 24, 2017 at 06:27:37PM +0200, Jiri Olsa wrote:
> > 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.

right, will add

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

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

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 <mark.rutland@arm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index d704e23914bf..30e30e94ea32 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -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;
 	}
 
-- 
2.9.5

[toc] | [prev] | [next] | [standalone]

#1719395 — [PATCH 09/10] 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

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-report.c | 2 +-
 tools/perf/util/values.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 9e4004b08f55..f9dff652dcbd 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -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);
 
diff --git a/tools/perf/util/values.c b/tools/perf/util/values.c
index 3b56aeaa8cbb..8a32bb0095e5 100644
--- a/tools/perf/util/values.c
+++ b/tools/perf/util/values.c
@@ -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;
 }
 
-- 
2.9.5

[toc] | [prev] | [next] | [standalone]

#1719399 — [PATCH 05/10] 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

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-report.c |  4 ----
 tools/perf/util/session.c   | 25 +++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index bace3429c030..9e4004b08f55 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -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;
 }
 
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index dc453f84a14c..4a09604a66a0 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -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);
-- 
2.9.5

[toc] | [prev] | [next] | [standalone]

#1719402 — [PATCH 06/10] perf values: Fix thread index bug

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

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/values.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/values.c b/tools/perf/util/values.c
index 5de2e15e2eda..9ac36bf2c438 100644
--- a/tools/perf/util/values.c
+++ b/tools/perf/util/values.c
@@ -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;
 }
-- 
2.9.5

[toc] | [prev] | [next] | [standalone]

#1719418 — [PATCH 01/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.

Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/events/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index af12e294caed..b8e394d9f7f2 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -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;
 
-- 
2.9.5

[toc] | [prev] | [next] | [standalone]

#1719422 — [PATCH 03/10] perf: Make sure we read only scheduled events

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.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/events/core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 30e30e94ea32..9a2791afe051 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -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);
-- 
2.9.5

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web