perf stat: Change event enable code

PATCH In review Jiri Olsa 5 participants 21 messages Dec 3, 2015 – Dec 16, 2015 perf events

Share & tools

Jiri Olsa

author reviewer

2015-12-03 09:12 UTC #0 lore raw

cover perf stat: Change event enable code

hi,
while testing ftrace:function event I noticed we create stat counters as enabled (except for enable_on_exec couters).

This way we count also filter setup and other config code which might be crucial for some events.

Posponing the events enable once everything is ready.

The last patch is RFC as I wasn't sure there's some hidden catch about perf_evlist__(enable|disable)_event functions I missed.. Adrian?

thanks,
jirka

---
Jiri Olsa (7):
perf tools: Use event maps directly in perf_evsel__enable perf tools: Introduce perf_evsel__disable function perf tools: Factor perf_evlist__(enable|disable) functions perf stat: Use perf_evlist__enable in handle_initial_delay perf stat: Create events as disabled
perf stat: Move enable_on_exec setup under earlier code perf tools: Remove perf_evlist__(enable|disable)_event functions

 tools/perf/arch/x86/util/intel-bts.c |  4 ++--
 tools/perf/arch/x86/util/intel-pt.c  |  4 ++--
 tools/perf/builtin-stat.c            | 44 +++++++++++++++++++++++++++-----------------
 tools/perf/tests/keep-tracking.c     |  2 +-
 tools/perf/tests/switch-tracking.c   |  6 +++---
 tools/perf/util/evlist.c             | 74 ++++++++------------------------------------------------------------------
 tools/perf/util/evlist.h             |  4 ----
 tools/perf/util/evsel.c              | 15 ++++++++++++++-
 tools/perf/util/evsel.h              |  3 ++-
 9 files changed, 59 insertions(+), 97 deletions(-)

Jiri Olsa

author reviewer

2015-12-03 09:07 UTC #1 lore raw

PATCH 1/7 perf tools: Use event maps directly in perf_evsel__enable

All events now share proper cpu and thread maps. There's no need to pass those maps from evlist, it's safe to use evsel maps for enabling event.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/builtin-stat.c | 5 +----
 tools/perf/util/evsel.c   | 5 ++++-
 tools/perf/util/evsel.h   | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

tools/perf/builtin-stat.c +1 -4

index df2fbf046ee2..813c52ad9303 100644
@@ -256,12 +256,9 @@ static void handle_initial_delay(void)
 	struct perf_evsel *counter;
 
 	if (initial_delay) {
-		const int ncpus = cpu_map__nr(evsel_list->cpus),
-			nthreads = thread_map__nr(evsel_list->threads);
-
 		usleep(initial_delay * 1000);
 		evlist__for_each(evsel_list, counter)
-			perf_evsel__enable(counter, ncpus, nthreads);
+			perf_evsel__enable(counter);
 	}
 }

tools/perf/util/evsel.c +4 -1

index 0a1f4d9e52fc..3a9b5068667d 100644
@@ -981,8 +981,11 @@ int perf_evsel__append_filter(struct perf_evsel *evsel,
 	return -1;
 }
 
-int perf_evsel__enable(struct perf_evsel *evsel, int ncpus, int nthreads)
+int perf_evsel__enable(struct perf_evsel *evsel)
 {
+	int nthreads = thread_map__nr(evsel->threads);
+	int ncpus = cpu_map__nr(evsel->cpus);
+
 	return perf_evsel__run_ioctl(evsel, ncpus, nthreads,
 				     PERF_EVENT_IOC_ENABLE,
 				     0);

tools/perf/util/evsel.h +1 -1

index 0e49bd742c63..a721592a3200 100644
@@ -227,7 +227,7 @@ int perf_evsel__append_filter(struct perf_evsel *evsel,
 			      const char *op, const char *filter);
 int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int nthreads,
 			     const char *filter);
-int perf_evsel__enable(struct perf_evsel *evsel, int ncpus, int nthreads);
+int perf_evsel__enable(struct perf_evsel *evsel);
 
 int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
 			     struct cpu_map *cpus);

Signature · 1 line

2.4.3

tip-bot for Jiri Olsa

merge bot

2015-12-08 04:33 UTC #2 lore raw

[tip:perf/core] perf evsel: Use event maps directly in perf_evsel__enable

perf evsel: Use event maps directly in perf_evsel__enable

Commit-ID:  5cd95fc3f8d84a8bb256838fa3b6b59e9095eaa2
Gitweb:     http://git.kernel.org/tip/5cd95fc3f8d84a8bb256838fa3b6b59e9095eaa2
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 3 Dec 2015 10:06:40 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 7 Dec 2015 18:12:57 -0300

perf evsel: Use event maps directly in perf_evsel__enable

All events now share proper cpu and thread maps. There's no need to pass those maps from evlist, it's safe to use evsel maps for enabling event.

Signed-off-by Jiri Olsa <[email protected]>

Cc Adrian Hunter <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/builtin-stat.c | 5 +----
 tools/perf/util/evsel.c   | 5 ++++-
 tools/perf/util/evsel.h   | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

tools/perf/builtin-stat.c +1 -4

index df2fbf0..813c52a 100644
@@ -256,12 +256,9 @@ static void handle_initial_delay(void)
 	struct perf_evsel *counter;
 
 	if (initial_delay) {
-		const int ncpus = cpu_map__nr(evsel_list->cpus),
-			nthreads = thread_map__nr(evsel_list->threads);
-
 		usleep(initial_delay * 1000);
 		evlist__for_each(evsel_list, counter)
-			perf_evsel__enable(counter, ncpus, nthreads);
+			perf_evsel__enable(counter);
 	}
 }

tools/perf/util/evsel.c +4 -1

index 0a1f4d9..3a9b506 100644
@@ -981,8 +981,11 @@ int perf_evsel__append_filter(struct perf_evsel *evsel,
 	return -1;
 }
 
-int perf_evsel__enable(struct perf_evsel *evsel, int ncpus, int nthreads)
+int perf_evsel__enable(struct perf_evsel *evsel)
 {
+	int nthreads = thread_map__nr(evsel->threads);
+	int ncpus = cpu_map__nr(evsel->cpus);
+
 	return perf_evsel__run_ioctl(evsel, ncpus, nthreads,
 				     PERF_EVENT_IOC_ENABLE,
 				     0);

tools/perf/util/evsel.h +1 -1

index 0e49bd7..a721592 100644
@@ -227,7 +227,7 @@ int perf_evsel__append_filter(struct perf_evsel *evsel,
 			      const char *op, const char *filter);
 int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int nthreads,
 			     const char *filter);
-int perf_evsel__enable(struct perf_evsel *evsel, int ncpus, int nthreads);
+int perf_evsel__enable(struct perf_evsel *evsel);
 
 int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
 			     struct cpu_map *cpus);

Jiri Olsa

author reviewer

2015-12-03 09:07 UTC #3 lore raw

PATCH 2/7 perf tools: Introduce perf_evsel__disable function

In reply to Jiri Olsa

Adding perf_evsel__disable function to have complement for perf_evsel__enable function. Both will be used in following patch to factor perf_evlist__(enable|disable).

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/evsel.c | 10 ++++++++++
 tools/perf/util/evsel.h |  1 +
 2 files changed, 11 insertions(+)

tools/perf/util/evsel.c +10 -0

index 3a9b5068667d..47f033089349 100644
@@ -991,6 +991,16 @@ int perf_evsel__enable(struct perf_evsel *evsel)
 				     0);
 }
 
+int perf_evsel__disable(struct perf_evsel *evsel)
+{
+	int nthreads = thread_map__nr(evsel->threads);
+	int ncpus = cpu_map__nr(evsel->cpus);
+
+	return perf_evsel__run_ioctl(evsel, ncpus, nthreads,
+				     PERF_EVENT_IOC_DISABLE,
+				     0);
+}
+
 int perf_evsel__alloc_id(struct perf_evsel *evsel, int ncpus, int nthreads)
 {
 	if (ncpus == 0 || nthreads == 0)

tools/perf/util/evsel.h +1 -0

index a721592a3200..5ded1fc0341e 100644
@@ -228,6 +228,7 @@ int perf_evsel__append_filter(struct perf_evsel *evsel,
 int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int nthreads,
 			     const char *filter);
 int perf_evsel__enable(struct perf_evsel *evsel);
+int perf_evsel__disable(struct perf_evsel *evsel);
 
 int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
 			     struct cpu_map *cpus);

Signature · 1 line

2.4.3

tip-bot for Jiri Olsa

merge bot

2015-12-08 04:33 UTC #4 lore raw

[tip:perf/core] perf evsel: Introduce disable() method

perf evsel: Introduce disable() method

Commit-ID:  e98a4cbb01e0ba1110eba5166a425b3eab9b2244
Gitweb:     http://git.kernel.org/tip/e98a4cbb01e0ba1110eba5166a425b3eab9b2244
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 3 Dec 2015 10:06:41 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 7 Dec 2015 18:12:57 -0300

perf evsel: Introduce disable() method

Adding perf_evsel__disable function to have complement for perf_evsel__enable function. Both will be used in following patch to factor perf_evlist__(enable|disable).

Signed-off-by Jiri Olsa <[email protected]>

Cc Adrian Hunter <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/evsel.c | 10 ++++++++++
 tools/perf/util/evsel.h |  1 +
 2 files changed, 11 insertions(+)

tools/perf/util/evsel.c +10 -0

index 3a9b506..47f0330 100644
@@ -991,6 +991,16 @@ int perf_evsel__enable(struct perf_evsel *evsel)
 				     0);
 }
 
+int perf_evsel__disable(struct perf_evsel *evsel)
+{
+	int nthreads = thread_map__nr(evsel->threads);
+	int ncpus = cpu_map__nr(evsel->cpus);
+
+	return perf_evsel__run_ioctl(evsel, ncpus, nthreads,
+				     PERF_EVENT_IOC_DISABLE,
+				     0);
+}
+
 int perf_evsel__alloc_id(struct perf_evsel *evsel, int ncpus, int nthreads)
 {
 	if (ncpus == 0 || nthreads == 0)

tools/perf/util/evsel.h +1 -0

index a721592..5ded1fc 100644
@@ -228,6 +228,7 @@ int perf_evsel__append_filter(struct perf_evsel *evsel,
 int perf_evsel__apply_filter(struct perf_evsel *evsel, int ncpus, int nthreads,
 			     const char *filter);
 int perf_evsel__enable(struct perf_evsel *evsel);
+int perf_evsel__disable(struct perf_evsel *evsel);
 
 int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
 			     struct cpu_map *cpus);

Jiri Olsa

author reviewer

2015-12-03 09:12 UTC #5 lore raw

PATCH 3/7 perf tools: Factor perf_evlist__(enable|disable) functions

In reply to Jiri Olsa

Use perf_evsel__(enable|disable) functions in perf_evlist__(enable|disable) functions in order to centralize ioctl enable/disable calls. This way we eliminate 2 places calling directly ioctl.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/evlist.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

tools/perf/util/evlist.c +8 ~~-24~~

index d1392194a9a9..d1b6c206bb93 100644
@@ -336,20 +336,12 @@ static int perf_evlist__nr_threads(struct perf_evlist *evlist,
 
 void perf_evlist__disable(struct perf_evlist *evlist)
 {
-	int cpu, thread;
 	struct perf_evsel *pos;
-	int nr_cpus = cpu_map__nr(evlist->cpus);
-	int nr_threads;
 
-	for (cpu = 0; cpu < nr_cpus; cpu++) {
-		evlist__for_each(evlist, pos) {
-			if (!perf_evsel__is_group_leader(pos) || !pos->fd)
-				continue;
-			nr_threads = perf_evlist__nr_threads(evlist, pos);
-			for (thread = 0; thread < nr_threads; thread++)
-				ioctl(FD(pos, cpu, thread),
-				      PERF_EVENT_IOC_DISABLE, 0);
-		}
+	evlist__for_each(evlist, pos) {
+		if (!perf_evsel__is_group_leader(pos) || !pos->fd)
+			continue;
+		perf_evsel__disable(pos);
 	}
 
 	evlist->enabled = false;

@@ -357,20 +349,12 @@ void perf_evlist__disable(struct perf_evlist *evlist)
 
 void perf_evlist__enable(struct perf_evlist *evlist)
 {
-	int cpu, thread;
 	struct perf_evsel *pos;
-	int nr_cpus = cpu_map__nr(evlist->cpus);
-	int nr_threads;
 
-	for (cpu = 0; cpu < nr_cpus; cpu++) {
-		evlist__for_each(evlist, pos) {
-			if (!perf_evsel__is_group_leader(pos) || !pos->fd)
-				continue;
-			nr_threads = perf_evlist__nr_threads(evlist, pos);
-			for (thread = 0; thread < nr_threads; thread++)
-				ioctl(FD(pos, cpu, thread),
-				      PERF_EVENT_IOC_ENABLE, 0);
-		}
+	evlist__for_each(evlist, pos) {
+		if (!perf_evsel__is_group_leader(pos) || !pos->fd)
+			continue;
+		perf_evsel__enable(pos);
 	}
 
 	evlist->enabled = true;

Signature · 1 line

2.4.3

tip-bot for Jiri Olsa

merge bot

2015-12-08 04:34 UTC #6 lore raw

[tip:perf/core] perf evlist: Factor perf_evlist__(enable|disable) functions

perf evlist: Factor perf_evlist__(enable|disable) functions

Commit-ID:  3e27c92081131738fa4d7dd71673aa6e8c24866d
Gitweb:     http://git.kernel.org/tip/3e27c92081131738fa4d7dd71673aa6e8c24866d
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 3 Dec 2015 10:06:42 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 7 Dec 2015 18:12:58 -0300

perf evlist: Factor perf_evlist__(enable|disable) functions

Use perf_evsel__(enable|disable) functions in perf_evlist__(enable|disable) functions in order to centralize ioctl enable/disable calls. This way we eliminate 2 places calling directly ioctl.

Signed-off-by Jiri Olsa <[email protected]>

Cc Adrian Hunter <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/evlist.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

tools/perf/util/evlist.c +8 ~~-24~~

index d139219..d1b6c20 100644
@@ -336,20 +336,12 @@ static int perf_evlist__nr_threads(struct perf_evlist *evlist,
 
 void perf_evlist__disable(struct perf_evlist *evlist)
 {
-	int cpu, thread;
 	struct perf_evsel *pos;
-	int nr_cpus = cpu_map__nr(evlist->cpus);
-	int nr_threads;
 
-	for (cpu = 0; cpu < nr_cpus; cpu++) {
-		evlist__for_each(evlist, pos) {
-			if (!perf_evsel__is_group_leader(pos) || !pos->fd)
-				continue;
-			nr_threads = perf_evlist__nr_threads(evlist, pos);
-			for (thread = 0; thread < nr_threads; thread++)
-				ioctl(FD(pos, cpu, thread),
-				      PERF_EVENT_IOC_DISABLE, 0);
-		}
+	evlist__for_each(evlist, pos) {
+		if (!perf_evsel__is_group_leader(pos) || !pos->fd)
+			continue;
+		perf_evsel__disable(pos);
 	}
 
 	evlist->enabled = false;

@@ -357,20 +349,12 @@ void perf_evlist__disable(struct perf_evlist *evlist)
 
 void perf_evlist__enable(struct perf_evlist *evlist)
 {
-	int cpu, thread;
 	struct perf_evsel *pos;
-	int nr_cpus = cpu_map__nr(evlist->cpus);
-	int nr_threads;
 
-	for (cpu = 0; cpu < nr_cpus; cpu++) {
-		evlist__for_each(evlist, pos) {
-			if (!perf_evsel__is_group_leader(pos) || !pos->fd)
-				continue;
-			nr_threads = perf_evlist__nr_threads(evlist, pos);
-			for (thread = 0; thread < nr_threads; thread++)
-				ioctl(FD(pos, cpu, thread),
-				      PERF_EVENT_IOC_ENABLE, 0);
-		}
+	evlist__for_each(evlist, pos) {
+		if (!perf_evsel__is_group_leader(pos) || !pos->fd)
+			continue;
+		perf_evsel__enable(pos);
 	}
 
 	evlist->enabled = true;

Jiri Olsa

author reviewer

2015-12-03 09:11 UTC #7 lore raw

PATCH 4/7 perf stat: Use perf_evlist__enable in handle_initial_delay

In reply to Jiri Olsa

No need to mimic the behaviour of perf_evlist__enable, we can use it directly.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/builtin-stat.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

tools/perf/builtin-stat.c +1 -4

index 813c52ad9303..8ca40deaa728 100644
@@ -253,12 +253,9 @@ static void process_interval(void)
 
 static void handle_initial_delay(void)
 {
-	struct perf_evsel *counter;
-
 	if (initial_delay) {
 		usleep(initial_delay * 1000);
-		evlist__for_each(evsel_list, counter)
-			perf_evsel__enable(counter);
+		perf_evlist__enable(evsel_list);
 	}
 }

Signature · 1 line

2.4.3

tip-bot for Jiri Olsa

merge bot

2015-12-08 04:34 UTC #8 lore raw

[tip:perf/core] perf stat: Use perf_evlist__enable in handle_initial_delay

perf stat: Use perf_evlist__enable in handle_initial_delay

Commit-ID:  ab46db0a3325a064bb24e826b12995d157565efb
Gitweb:     http://git.kernel.org/tip/ab46db0a3325a064bb24e826b12995d157565efb
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 3 Dec 2015 10:06:43 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 7 Dec 2015 18:12:58 -0300

perf stat: Use perf_evlist__enable in handle_initial_delay

No need to mimic the behaviour of perf_evlist__enable, we can use it directly.

Signed-off-by Jiri Olsa <[email protected]>

Tested-by Arnaldo Carvalho de Melo <[email protected]>

Cc Adrian Hunter <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/builtin-stat.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

tools/perf/builtin-stat.c +1 -4

index 813c52a..8ca40de 100644
@@ -253,12 +253,9 @@ static void process_interval(void)
 
 static void handle_initial_delay(void)
 {
-	struct perf_evsel *counter;
-
 	if (initial_delay) {
 		usleep(initial_delay * 1000);
-		evlist__for_each(evsel_list, counter)
-			perf_evsel__enable(counter);
+		perf_evlist__enable(evsel_list);
 	}
 }

Jiri Olsa

author reviewer

2015-12-03 09:09 UTC #9 lore raw

PATCH 5/7 perf stat: Create events as disabled

In reply to Jiri Olsa

Currently we have 2 kinds of stat counters based on when the event is enabled:
1) tracee command events, which are enable once the tracee executes exec syscall (enable_on_exec bit)
2) all other events which get alive within the perf_event_open syscall

Ad 2) case could raise a problem in case we want additional filter to be attached for event. In this case we want the event to be enabled after it's configured with filter.

Changing the behaviour of ad 2) events, so they all are created as disabled (disabled bit). Adding extra enable call to make them alive once they finish setup.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/builtin-stat.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

tools/perf/builtin-stat.c +19 -6

index 8ca40deaa728..2e70610649a1 100644
@@ -168,11 +168,18 @@ static int create_perf_stat_counter(struct perf_evsel *evsel)
 	attr->sample_period = 0;
 	attr->sample_type   = 0;
 
+	/*
+	 * Disabling all counters initially, they will be enabled
+	 * either manually by us or by kernel via enable_on_exec
+	 * set later.
+	 */
+	if (perf_evsel__is_group_leader(evsel))
+		attr->disabled = 1;
+
 	if (target__has_cpu(&target))
 		return perf_evsel__open_per_cpu(evsel, perf_evsel__cpus(evsel));
 
 	if (!target__has_task(&target) && perf_evsel__is_group_leader(evsel)) {
-		attr->disabled = 1;
 		if (!initial_delay)
 			attr->enable_on_exec = 1;
 	}

@@ -251,12 +258,18 @@ static void process_interval(void)
 	print_counters(&rs, 0, NULL);
 }
 
-static void handle_initial_delay(void)
+static void enable_counters(void)
 {
-	if (initial_delay) {
+	if (initial_delay)
 		usleep(initial_delay * 1000);
+
+	/*
+	 * We need to enable counters only if:
+	 * - we don't have tracee (attaching to task or cpu)
+	 * - we have initial delay configured
+	 */
+	if (!target__none(&target) || initial_delay)
 		perf_evlist__enable(evsel_list);
-	}
 }
 
 static volatile int workload_exec_errno;

@@ -353,7 +366,7 @@ static int __run_perf_stat(int argc, const char **argv)
 
 	if (forks) {
 		perf_evlist__start_workload(evsel_list);
-		handle_initial_delay();
+		enable_counters();
 
 		if (interval) {
 			while (!waitpid(child_pid, &status, WNOHANG)) {

@@ -372,7 +385,7 @@ static int __run_perf_stat(int argc, const char **argv)
 		if (WIFSIGNALED(status))
 			psignal(WTERMSIG(status), argv[0]);
 	} else {
-		handle_initial_delay();
+		enable_counters();
 		while (!done) {
 			nanosleep(&ts, NULL);
 			if (interval)

Signature · 1 line

2.4.3

tip-bot for Jiri Olsa

merge bot

2015-12-08 04:34 UTC #10 lore raw

[tip:perf/core] perf stat: Create events as disabled

perf stat: Create events as disabled

Commit-ID:  67ccdecd09cac818146b1e153ff901cb67570012
Gitweb:     http://git.kernel.org/tip/67ccdecd09cac818146b1e153ff901cb67570012
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 3 Dec 2015 10:06:44 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 7 Dec 2015 18:12:59 -0300

perf stat: Create events as disabled

Currently we have 2 kinds of stat counters based on when the event is enabled:

1) tracee command events, which are enable once the tracee executes exec syscall (enable_on_exec bit)
2) all other events which get alive within the perf_event_open syscall

And 2) case could raise a problem in case we want additional filter to be attached for event. In this case we want the event to be enabled after it's configured with filter.

Changing the behaviour of 2) events, so they all are created as disabled (disabled bit). Adding extra enable call to make them alive once they finish setup.

Signed-off-by Jiri Olsa <[email protected]>

Cc Adrian Hunter <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/builtin-stat.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

tools/perf/builtin-stat.c +19 -6

index 8ca40de..2e70610 100644
@@ -168,11 +168,18 @@ static int create_perf_stat_counter(struct perf_evsel *evsel)
 	attr->sample_period = 0;
 	attr->sample_type   = 0;
 
+	/*
+	 * Disabling all counters initially, they will be enabled
+	 * either manually by us or by kernel via enable_on_exec
+	 * set later.
+	 */
+	if (perf_evsel__is_group_leader(evsel))
+		attr->disabled = 1;
+
 	if (target__has_cpu(&target))
 		return perf_evsel__open_per_cpu(evsel, perf_evsel__cpus(evsel));
 
 	if (!target__has_task(&target) && perf_evsel__is_group_leader(evsel)) {
-		attr->disabled = 1;
 		if (!initial_delay)
 			attr->enable_on_exec = 1;
 	}

@@ -251,12 +258,18 @@ static void process_interval(void)
 	print_counters(&rs, 0, NULL);
 }
 
-static void handle_initial_delay(void)
+static void enable_counters(void)
 {
-	if (initial_delay) {
+	if (initial_delay)
 		usleep(initial_delay * 1000);
+
+	/*
+	 * We need to enable counters only if:
+	 * - we don't have tracee (attaching to task or cpu)
+	 * - we have initial delay configured
+	 */
+	if (!target__none(&target) || initial_delay)
 		perf_evlist__enable(evsel_list);
-	}
 }
 
 static volatile int workload_exec_errno;

@@ -353,7 +366,7 @@ static int __run_perf_stat(int argc, const char **argv)
 
 	if (forks) {
 		perf_evlist__start_workload(evsel_list);
-		handle_initial_delay();
+		enable_counters();
 
 		if (interval) {
 			while (!waitpid(child_pid, &status, WNOHANG)) {

@@ -372,7 +385,7 @@ static int __run_perf_stat(int argc, const char **argv)
 		if (WIFSIGNALED(status))
 			psignal(WTERMSIG(status), argv[0]);
 	} else {
-		handle_initial_delay();
+		enable_counters();
 		while (!done) {
 			nanosleep(&ts, NULL);
 			if (interval)

Jiri Olsa

2015-12-03 09:09 UTC #11 lore raw

In reply to Jiri Olsa

Quoted 7 lines · Jiri Olsa

hi,
while testing ftrace:function event I noticed we create stat counters as enabled (except for enable_on_exec couters).
This way we count also filter setup and other config code which might be crucial for some events.

Quoted 8 lines

Posponing the events enable once everything is ready.
The last patch is RFC as I wasn't sure there's some hidden catch about perf_evlist__(enable|disable)_event functions I missed.. Adrian?
thanks,
jirka

Also available in:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
perf/fixes

jirka

Jiri Olsa

author reviewer

2015-12-03 09:08 UTC #12 lore raw

PATCH 6/7 perf stat: Move enable_on_exec setup under earlier code

In reply to Jiri Olsa

It's more readable this way and we can save one perf_evsel__is_group_leader condition in current code.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/builtin-stat.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

tools/perf/builtin-stat.c +9 -6

index 2e70610649a1..e74712dee242 100644
@@ -173,17 +173,20 @@ static int create_perf_stat_counter(struct perf_evsel *evsel)
 	 * either manually by us or by kernel via enable_on_exec
 	 * set later.
 	 */
-	if (perf_evsel__is_group_leader(evsel))
+	if (perf_evsel__is_group_leader(evsel)) {
 		attr->disabled = 1;
 
-	if (target__has_cpu(&target))
-		return perf_evsel__open_per_cpu(evsel, perf_evsel__cpus(evsel));
-
-	if (!target__has_task(&target) && perf_evsel__is_group_leader(evsel)) {
-		if (!initial_delay)
+		/*
+		 * In case of initial_delay we enable tracee
+		 * events manually.
+		 */
+		if (target__none(&target) && !initial_delay)
 			attr->enable_on_exec = 1;
 	}
 
+	if (target__has_cpu(&target))
+		return perf_evsel__open_per_cpu(evsel, perf_evsel__cpus(evsel));
+
 	return perf_evsel__open_per_thread(evsel, evsel_list->threads);
 }

Signature · 1 line

2.4.3

tip-bot for Jiri Olsa

merge bot

2015-12-08 04:34 UTC #13 lore raw

[tip:perf/core] perf stat: Move enable_on_exec setup under earlier code

perf stat: Move enable_on_exec setup under earlier code

Commit-ID:  c8280cec2a196f2ffea83dd755b17eb020ca1b83
Gitweb:     http://git.kernel.org/tip/c8280cec2a196f2ffea83dd755b17eb020ca1b83
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 3 Dec 2015 10:06:45 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 7 Dec 2015 18:12:59 -0300

perf stat: Move enable_on_exec setup under earlier code

It's more readable this way and we can save one perf_evsel__is_group_leader condition in current code.

Signed-off-by Jiri Olsa <[email protected]>

Cc Adrian Hunter <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/builtin-stat.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

tools/perf/builtin-stat.c +9 -6

index 2e70610..e74712d 100644
@@ -173,17 +173,20 @@ static int create_perf_stat_counter(struct perf_evsel *evsel)
 	 * either manually by us or by kernel via enable_on_exec
 	 * set later.
 	 */
-	if (perf_evsel__is_group_leader(evsel))
+	if (perf_evsel__is_group_leader(evsel)) {
 		attr->disabled = 1;
 
-	if (target__has_cpu(&target))
-		return perf_evsel__open_per_cpu(evsel, perf_evsel__cpus(evsel));
-
-	if (!target__has_task(&target) && perf_evsel__is_group_leader(evsel)) {
-		if (!initial_delay)
+		/*
+		 * In case of initial_delay we enable tracee
+		 * events manually.
+		 */
+		if (target__none(&target) && !initial_delay)
 			attr->enable_on_exec = 1;
 	}
 
+	if (target__has_cpu(&target))
+		return perf_evsel__open_per_cpu(evsel, perf_evsel__cpus(evsel));
+
 	return perf_evsel__open_per_thread(evsel, evsel_list->threads);
 }

Jiri Olsa

author reviewer

2015-12-03 09:07 UTC #14 lore raw

RFC 7/7 perf tools: Remove perf_evlist__(enable|disable)_event functions

In reply to Jiri Olsa

Replacing them with perf_evsel__(enable|disable).

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/arch/x86/util/intel-bts.c |  4 ++--
 tools/perf/arch/x86/util/intel-pt.c  |  4 ++--
 tools/perf/tests/keep-tracking.c     |  2 +-
 tools/perf/tests/switch-tracking.c   |  6 +++---
 tools/perf/util/evlist.c             | 42 ------------------------------------
 tools/perf/util/evlist.h             |  4 ----
 6 files changed, 8 insertions(+), 54 deletions(-)

tools/perf/arch/x86/util/intel-bts.c +2 -2

index 9b94ce520917..8d8150f1cf9b 100644
@@ -327,7 +327,7 @@ static int intel_bts_snapshot_start(struct auxtrace_record *itr)
 
 	evlist__for_each(btsr->evlist, evsel) {
 		if (evsel->attr.type == btsr->intel_bts_pmu->type)
-			return perf_evlist__disable_event(btsr->evlist, evsel);
+			return perf_evsel__disable(evsel);
 	}
 	return -EINVAL;
 }

@@ -340,7 +340,7 @@ static int intel_bts_snapshot_finish(struct auxtrace_record *itr)
 
 	evlist__for_each(btsr->evlist, evsel) {
 		if (evsel->attr.type == btsr->intel_bts_pmu->type)
-			return perf_evlist__enable_event(btsr->evlist, evsel);
+			return perf_evsel__enable(evsel);
 	}
 	return -EINVAL;
 }

tools/perf/arch/x86/util/intel-pt.c +2 -2

index b02af064f0f9..7cb3339a95e1 100644
@@ -725,7 +725,7 @@ static int intel_pt_snapshot_start(struct auxtrace_record *itr)
 
 	evlist__for_each(ptr->evlist, evsel) {
 		if (evsel->attr.type == ptr->intel_pt_pmu->type)
-			return perf_evlist__disable_event(ptr->evlist, evsel);
+			return perf_evsel__disable(evsel);
 	}
 	return -EINVAL;
 }

@@ -738,7 +738,7 @@ static int intel_pt_snapshot_finish(struct auxtrace_record *itr)
 
 	evlist__for_each(ptr->evlist, evsel) {
 		if (evsel->attr.type == ptr->intel_pt_pmu->type)
-			return perf_evlist__enable_event(ptr->evlist, evsel);
+			return perf_evsel__enable(evsel);
 	}
 	return -EINVAL;
 }

tools/perf/tests/keep-tracking.c +1 -1

index a337a6da1f39..7b27943ecfef 100644
@@ -124,7 +124,7 @@ int test__keep_tracking(int subtest __maybe_unused)
 
 	evsel = perf_evlist__last(evlist);
 
-	CHECK__(perf_evlist__disable_event(evlist, evsel));
+	CHECK__(perf_evsel__disable(evsel));
 
 	comm = "Test COMM 2";
 	CHECK__(prctl(PR_SET_NAME, (unsigned long)comm, 0, 0, 0));

tools/perf/tests/switch-tracking.c +3 -3

index dfbd8d69ce89..ebd80168d51e 100644
@@ -455,7 +455,7 @@ int test__switch_tracking(int subtest __maybe_unused)
 
 	perf_evlist__enable(evlist);
 
-	err = perf_evlist__disable_event(evlist, cpu_clocks_evsel);
+	err = perf_evsel__disable(cpu_clocks_evsel);
 	if (err) {
 		pr_debug("perf_evlist__disable_event failed!\n");
 		goto out_err;

@@ -474,7 +474,7 @@ int test__switch_tracking(int subtest __maybe_unused)
 		goto out_err;
 	}
 
-	err = perf_evlist__disable_event(evlist, cycles_evsel);
+	err = perf_evsel__disable(cycles_evsel);
 	if (err) {
 		pr_debug("perf_evlist__disable_event failed!\n");
 		goto out_err;

@@ -500,7 +500,7 @@ int test__switch_tracking(int subtest __maybe_unused)
 		goto out_err;
 	}
 
-	err = perf_evlist__enable_event(evlist, cycles_evsel);
+	err = perf_evsel__enable(cycles_evsel);
 	if (err) {
 		pr_debug("perf_evlist__disable_event failed!\n");
 		goto out_err;

tools/perf/util/evlist.c +0 ~~-42~~

48 lines not on this page · raw message

tools/perf/util/evlist.h +0 -4

index a459fe71b452..decec632a6f1 100644
@@ -149,10 +149,6 @@ void perf_evlist__disable(struct perf_evlist *evlist);
 void perf_evlist__enable(struct perf_evlist *evlist);
 void perf_evlist__toggle_enable(struct perf_evlist *evlist);
 
-int perf_evlist__disable_event(struct perf_evlist *evlist,
-			       struct perf_evsel *evsel);
-int perf_evlist__enable_event(struct perf_evlist *evlist,
-			      struct perf_evsel *evsel);
 int perf_evlist__enable_event_idx(struct perf_evlist *evlist,
 				  struct perf_evsel *evsel, int idx);

Signature · 1 line

2.4.3

Arnaldo Carvalho de Melo

maintainer

2015-12-07 21:09 UTC #15 lore raw

In reply to Jiri Olsa

Em Thu, Dec 03, 2015 at 10:06:39AM +0100, Jiri Olsa escreveu:

Quoted 4 lines

hi,
while testing ftrace:function event I noticed we create stat counters as enabled (except for enable_on_exec couters).

Quoted 8 lines

This way we count also filter setup and other config code which might be crucial for some events.
Posponing the events enable once everything is ready.
The last patch is RFC as I wasn't sure there's some hidden catch about perf_evlist__(enable|disable)_event functions I missed.. Adrian?

They look the same, Adrian?

Applied the first 6, will give some more time to Adrian to chime in.

- Arnaldo

Adrian Hunter

reviewer

2015-12-08 07:33 UTC #16 lore raw

Quoted 10 lines · Arnaldo Carvalho de Melo

Em Thu, Dec 03, 2015 at 10:06:39AM +0100, Jiri Olsa escreveu:
Quoted 9 lines
hi,
while testing ftrace:function event I noticed we create stat counters as enabled (except for enable_on_exec couters).
This way we count also filter setup and other config code which might be crucial for some events.
Posponing the events enable once everything is ready.

Quoted 7 lines

Quoted 3 lines
The last patch is RFC as I wasn't sure there's some hidden catch about perf_evlist__(enable|disable)_event functions I missed.. Adrian?
They look the same, Adrian?
Applied the first 6, will give some more time to Adrian to chime in.

Looks like there might already be a problem using evsel->threads instead of evlist->threads with the logic relating to evsel->system_wide getting lost - but that happened already in "perf evlist: Factor perf_evlist__(enable|disable) functions". Probably the threads should not be propagated in that case, but it needs more investigation. I will try to look at it today.

Arnaldo Carvalho de Melo

maintainer

2015-12-08 13:53 UTC #17 lore raw

Em Tue, Dec 08, 2015 at 09:29:51AM +0200, Adrian Hunter escreveu:

Quoted 16 lines · Arnaldo Carvalho de Melo

Quoted 14 lines
Em Thu, Dec 03, 2015 at 10:06:39AM +0100, Jiri Olsa escreveu:
Quoted 11 lines
while testing ftrace:function event I noticed we create stat counters as enabled (except for enable_on_exec couters).
This way we count also filter setup and other config code which might be crucial for some events.
Posponing the events enable once everything is ready.
The last patch is RFC as I wasn't sure there's some hidden catch about perf_evlist__(enable|disable)_event functions I missed.. Adrian?
They look the same, Adrian?

Quoted 8 lines

Quoted 1 line
Applied the first 6, will give some more time to Adrian to chime in.
Looks like there might already be a problem using evsel->threads instead of evlist->threads with the logic relating to evsel->system_wide getting lost - but that happened already in "perf evlist: Factor perf_evlist__(enable|disable) functions". Probably the threads should not be propagated in that case, but it needs more investigation. I will try to look at it today.

Thanks! Is that covered by any 'perf test' entry? Do you think having some sort of Intel PT test to run on capable machines would be feasible?

- Arnaldo

Adrian Hunter

reviewer

2015-12-09 13:49 UTC #18 lore raw

Quoted 26 lines · Arnaldo Carvalho de Melo

Em Tue, Dec 08, 2015 at 09:29:51AM +0200, Adrian Hunter escreveu:
Quoted 16 lines · Arnaldo Carvalho de Melo
Quoted 14 lines
Em Thu, Dec 03, 2015 at 10:06:39AM +0100, Jiri Olsa escreveu:
Quoted 11 lines
while testing ftrace:function event I noticed we create stat counters as enabled (except for enable_on_exec couters).
This way we count also filter setup and other config code which might be crucial for some events.
Posponing the events enable once everything is ready.
The last patch is RFC as I wasn't sure there's some hidden catch about perf_evlist__(enable|disable)_event functions I missed.. Adrian?
They look the same, Adrian?
Quoted 8 lines
Quoted 1 line
Applied the first 6, will give some more time to Adrian to chime in.
Looks like there might already be a problem using evsel->threads instead of evlist->threads with the logic relating to evsel->system_wide getting lost - but that happened already in "perf evlist: Factor perf_evlist__(enable|disable) functions". Probably the threads should not be propagated in that case, but it needs more investigation. I will try to look at it today.

Quoted 2 lines

Thanks! Is that covered by any 'perf test' entry? Do you think having some sort of Intel PT test to run on capable machines would be feasible?

There is "Test tracking with sched_switch". There was an issue where 'perf record' was working differently to the tests. I will try to find where the gaps are. Seems I have run out of time again today though.

Adrian Hunter

reviewer

2015-12-11 12:45 UTC #19 lore raw

Quoted 27 lines · Adrian Hunter

Quoted 26 lines · Arnaldo Carvalho de Melo
Em Tue, Dec 08, 2015 at 09:29:51AM +0200, Adrian Hunter escreveu:
Quoted 24 lines · Arnaldo Carvalho de Melo
Quoted 9 lines
Em Thu, Dec 03, 2015 at 10:06:39AM +0100, Jiri Olsa escreveu:
Quoted 8 lines
while testing ftrace:function event I noticed we create stat counters as enabled (except for enable_on_exec couters).
This way we count also filter setup and other config code which might be crucial for some events.
Posponing the events enable once everything is ready.
Quoted 7 lines
Quoted 3 lines
The last patch is RFC as I wasn't sure there's some hidden catch about perf_evlist__(enable|disable)_event functions I missed.. Adrian?
They look the same, Adrian?
Applied the first 6, will give some more time to Adrian to chime in.
Looks like there might already be a problem using evsel->threads instead of evlist->threads with the logic relating to evsel->system_wide getting lost - but that happened already in "perf evlist: Factor perf_evlist__(enable|disable) functions". Probably the threads should not be propagated in that case, but it needs more investigation. I will try to look at it today.

Quoted 6 lines

Quoted 2 lines
Thanks! Is that covered by any 'perf test' entry? Do you think having some sort of Intel PT test to run on capable machines would be feasible?
There is "Test tracking with sched_switch". There was an issue where 'perf record' was working differently to the tests. I will try to find where the gaps are. Seems I have run out of time again today though.

I was wrong about there being any problem using evsel->threads. While the patch "perf evlist: Factor perf_evlist__(enable|disable) function" changes the number of threads (from perf_evlist__nr_threads() to thread_map__nr()), the system_wide check is still done in perf_evsel__run_ioctl(), so everything is fine.

WRT "[RFC 7/7] perf tools: Remove perf_evlist__(enable|disable)_event functions" it might be worth putting the evsel->fd checks that perf_evlist__[enable|disable]_event() have into perf_evsel__[enable|disable](). But otherwise it looks fine.

The gap in testing that I was thinking of is below:

From: Adrian Hunter <[email protected]>
Date: Fri, 11 Dec 2015 11:05:11 +0200
Subject: [PATCH] perf tools: Make perf_evlist__open() open evsels with their
cpus and threads (like perf record does)

'perf record' uses perf_evsel__open() to open events and passes the evsel->cpus and evsel->threads. Many tests and some tools instead use perf_evlist__open() which passes instead evlist->cpus and evlist->threads.

Make perf_evlist__open() follow the 'perf record' behaviour so that a consistent approach is taken.

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/evlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/perf/util/evlist.c +1 -1

index d1b6c206bb93..306dacb33d8e 100644
@@ -1470,7 +1470,7 @@ int perf_evlist__open(struct perf_evlist *evlist)
 	perf_evlist__update_id_pos(evlist);
 
 	evlist__for_each(evlist, evsel) {
-		err = perf_evsel__open(evsel, evlist->cpus, evlist->threads);
+		err = perf_evsel__open(evsel, evsel->cpus, evsel->threads);
 		if (err < 0)
 			goto out_err;
 	}

Signature · 1 line

1.9.1

Jiri Olsa

2015-12-16 15:55 UTC #20 lore raw

On Fri, Dec 11, 2015 at 02:42:22PM +0200, Adrian Hunter wrote:

SNIP

Quoted 19 lines

I was wrong about there being any problem using evsel->threads. While the patch "perf evlist: Factor perf_evlist__(enable|disable) function" changes the number of threads (from perf_evlist__nr_threads() to thread_map__nr()), the system_wide check is still done in perf_evsel__run_ioctl(), so everything is fine.
WRT "[RFC 7/7] perf tools: Remove perf_evlist__(enable|disable)_event functions" it might be worth putting the evsel->fd checks that perf_evlist__[enable|disable]_event() have into perf_evsel__[enable|disable](). But otherwise it looks fine.
The gap in testing that I was thinking of is below:
From: Adrian Hunter <[email protected]>
Date: Fri, 11 Dec 2015 11:05:11 +0200
Subject: [PATCH] perf tools: Make perf_evlist__open() open evsels with their
cpus and threads (like perf record does)

Quoted 6 lines

'perf record' uses perf_evsel__open() to open events and passes the evsel->cpus and evsel->threads. Many tests and some tools instead use perf_evlist__open() which passes instead evlist->cpus and evlist->threads.
Make perf_evlist__open() follow the 'perf record' behaviour so that a consistent approach is taken.

coo, I'll queue this one for Arnaldo to pick up together with:
[RFC 7/7] perf tools: Remove perf_evlist__(enable|disable)_event

thanks,
jirka

Quoted 23 lines

Signed-off-by Adrian Hunter <[email protected]>

---
 tools/perf/util/evlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/perf/util/evlist.c +1 -1

index d1b6c206bb93..306dacb33d8e 100644
@@ -1470,7 +1470,7 @@ int perf_evlist__open(struct perf_evlist *evlist)
 	perf_evlist__update_id_pos(evlist);
 
 	evlist__for_each(evlist, evsel) {
-		err = perf_evsel__open(evsel, evlist->cpus, evlist->threads);
+		err = perf_evsel__open(evsel, evsel->cpus, evsel->threads);
 		if (err < 0)
 			goto out_err;
 	}

Signature · 1 line

1.9.1