[PATCH 0/7] perf stat: Change event enable code

[PATCH 0/7] perf stat: Change event enable code

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

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(-)

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

[Jiri Olsa]

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/tip-8758blril51rkn4e6wef1gcv@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 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(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index df2fbf046ee2..813c52ad9303 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -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);
 	}
 }
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 0a1f4d9e52fc..3a9b5068667d 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -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);
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 0e49bd742c63..a721592a3200 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -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);
-- 
2.4.3

[PATCH 2/7] perf tools: Introduce perf_evsel__disable function

[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/tip-bbyjpha9wc4ifn0ebfkm0mnl@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/evsel.c | 10 ++++++++++
 tools/perf/util/evsel.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 3a9b5068667d..47f033089349 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -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)
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index a721592a3200..5ded1fc0341e 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -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);
-- 
2.4.3

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

[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/tip-0jrocwelrmn4sflhzjulj0rv@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/evlist.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index d1392194a9a9..d1b6c206bb93 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -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;
-- 
2.4.3

[PATCH 4/7] perf stat: Use perf_evlist__enable in handle_initial_delay

[Jiri Olsa]

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

Link: http://lkml.kernel.org/n/tip-zrqwyndnod6pkxtwb1p3h6j6@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-stat.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 813c52ad9303..8ca40deaa728 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -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);
 	}
 }
 
-- 
2.4.3

[PATCH 5/7] perf stat: Create events as disabled

[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/tip-xwx95lcwjpt40uwzhos3ks9i@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-stat.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 8ca40deaa728..2e70610649a1 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -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)
-- 
2.4.3

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

[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/tip-y9s28dql028b0as614rkeul9@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/builtin-stat.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 2e70610649a1..e74712dee242 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -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);
 }
 
-- 
2.4.3

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

[Jiri Olsa]

Replacing them with perf_evsel__(enable|disable).

Link: http://lkml.kernel.org/n/tip-bbyjpha9wc4ifn0ebfkm0mnl@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 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(-)

diff --git a/tools/perf/arch/x86/util/intel-bts.c b/tools/perf/arch/x86/util/intel-bts.c
index 9b94ce520917..8d8150f1cf9b 100644
--- a/tools/perf/arch/x86/util/intel-bts.c
+++ b/tools/perf/arch/x86/util/intel-bts.c
@@ -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;
 }
diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c
index b02af064f0f9..7cb3339a95e1 100644
--- a/tools/perf/arch/x86/util/intel-pt.c
+++ b/tools/perf/arch/x86/util/intel-pt.c
@@ -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;
 }
diff --git a/tools/perf/tests/keep-tracking.c b/tools/perf/tests/keep-tracking.c
index a337a6da1f39..7b27943ecfef 100644
--- a/tools/perf/tests/keep-tracking.c
+++ b/tools/perf/tests/keep-tracking.c
@@ -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));
diff --git a/tools/perf/tests/switch-tracking.c b/tools/perf/tests/switch-tracking.c
index dfbd8d69ce89..ebd80168d51e 100644
--- a/tools/perf/tests/switch-tracking.c
+++ b/tools/perf/tests/switch-tracking.c
@@ -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;
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index d1b6c206bb93..211f497b7f39 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -365,48 +365,6 @@ void perf_evlist__toggle_enable(struct perf_evlist *evlist)
 	(evlist->enabled ? perf_evlist__disable : perf_evlist__enable)(evlist);
 }
 
-int perf_evlist__disable_event(struct perf_evlist *evlist,
-			       struct perf_evsel *evsel)
-{
-	int cpu, thread, err;
-	int nr_cpus = cpu_map__nr(evlist->cpus);
-	int nr_threads = perf_evlist__nr_threads(evlist, evsel);
-
-	if (!evsel->fd)
-		return 0;
-
-	for (cpu = 0; cpu < nr_cpus; cpu++) {
-		for (thread = 0; thread < nr_threads; thread++) {
-			err = ioctl(FD(evsel, cpu, thread),
-				    PERF_EVENT_IOC_DISABLE, 0);
-			if (err)
-				return err;
-		}
-	}
-	return 0;
-}
-
-int perf_evlist__enable_event(struct perf_evlist *evlist,
-			      struct perf_evsel *evsel)
-{
-	int cpu, thread, err;
-	int nr_cpus = cpu_map__nr(evlist->cpus);
-	int nr_threads = perf_evlist__nr_threads(evlist, evsel);
-
-	if (!evsel->fd)
-		return -EINVAL;
-
-	for (cpu = 0; cpu < nr_cpus; cpu++) {
-		for (thread = 0; thread < nr_threads; thread++) {
-			err = ioctl(FD(evsel, cpu, thread),
-				    PERF_EVENT_IOC_ENABLE, 0);
-			if (err)
-				return err;
-		}
-	}
-	return 0;
-}
-
 static int perf_evlist__enable_event_cpu(struct perf_evlist *evlist,
 					 struct perf_evsel *evsel, int cpu)
 {
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index a459fe71b452..decec632a6f1 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -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);
 
-- 
2.4.3

Re: [PATCH 0/7] perf stat: Change event enable code

[Jiri Olsa]

On Thu, Dec 03, 2015 at 10:06:39AM +0100, Jiri Olsa wrote:
> 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

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

jirka

Re: [PATCH 0/7] perf stat: Change event enable code

[Arnaldo Carvalho de Melo]

Em Thu, Dec 03, 2015 at 10:06:39AM +0100, Jiri Olsa escreveu:
> 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?

They look the same, Adrian?

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

- Arnaldo

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

[tip-bot for Jiri Olsa]

Commit-ID:  5cd95fc3f8d84a8bb256838fa3b6b59e9095eaa2
Gitweb:     http://git.kernel.org/tip/5cd95fc3f8d84a8bb256838fa3b6b59e9095eaa2
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Thu, 3 Dec 2015 10:06:40 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1449133606-14429-2-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index df2fbf0..813c52a 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -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);
 	}
 }
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 0a1f4d9..3a9b506 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -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);
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 0e49bd7..a721592 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -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);

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

[tip-bot for Jiri Olsa]

Commit-ID:  e98a4cbb01e0ba1110eba5166a425b3eab9b2244
Gitweb:     http://git.kernel.org/tip/e98a4cbb01e0ba1110eba5166a425b3eab9b2244
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Thu, 3 Dec 2015 10:06:41 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1449133606-14429-3-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evsel.c | 10 ++++++++++
 tools/perf/util/evsel.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 3a9b506..47f0330 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -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)
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index a721592..5ded1fc 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -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);

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

[tip-bot for Jiri Olsa]

Commit-ID:  3e27c92081131738fa4d7dd71673aa6e8c24866d
Gitweb:     http://git.kernel.org/tip/3e27c92081131738fa4d7dd71673aa6e8c24866d
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Thu, 3 Dec 2015 10:06:42 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1449133606-14429-4-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index d139219..d1b6c20 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -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;

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

[tip-bot for Jiri Olsa]

Commit-ID:  ab46db0a3325a064bb24e826b12995d157565efb
Gitweb:     http://git.kernel.org/tip/ab46db0a3325a064bb24e826b12995d157565efb
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Thu, 3 Dec 2015 10:06:43 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1449133606-14429-5-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 813c52a..8ca40de 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -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);
 	}
 }
 

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

[tip-bot for Jiri Olsa]

Commit-ID:  67ccdecd09cac818146b1e153ff901cb67570012
Gitweb:     http://git.kernel.org/tip/67ccdecd09cac818146b1e153ff901cb67570012
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Thu, 3 Dec 2015 10:06:44 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1449133606-14429-6-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 8ca40de..2e70610 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -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)

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

[tip-bot for Jiri Olsa]

Commit-ID:  c8280cec2a196f2ffea83dd755b17eb020ca1b83
Gitweb:     http://git.kernel.org/tip/c8280cec2a196f2ffea83dd755b17eb020ca1b83
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Thu, 3 Dec 2015 10:06:45 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1449133606-14429-7-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 2e70610..e74712d 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -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);
 }
 

Re: [PATCH 0/7] perf stat: Change event enable code

[Adrian Hunter]

On 07/12/15 23:09, Arnaldo Carvalho de Melo wrote:
> Em Thu, Dec 03, 2015 at 10:06:39AM +0100, Jiri Olsa escreveu:
>> 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?
> 
> 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.

Re: [PATCH 0/7] perf stat: Change event enable code

[Arnaldo Carvalho de Melo]

Em Tue, Dec 08, 2015 at 09:29:51AM +0200, Adrian Hunter escreveu:
> On 07/12/15 23:09, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Dec 03, 2015 at 10:06:39AM +0100, Jiri Olsa escreveu:
> >> 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?

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

Re: [PATCH 0/7] perf stat: Change event enable code

[Adrian Hunter]

On 08/12/15 15:53, Arnaldo Carvalho de Melo wrote:
> Em Tue, Dec 08, 2015 at 09:29:51AM +0200, Adrian Hunter escreveu:
>> On 07/12/15 23:09, Arnaldo Carvalho de Melo wrote:
>>> Em Thu, Dec 03, 2015 at 10:06:39AM +0100, Jiri Olsa escreveu:
>>>> 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?
> 
>>> 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?

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.

Re: [PATCH 0/7] perf stat: Change event enable code

[Adrian Hunter]

On 09/12/15 15:44, Adrian Hunter wrote:
> On 08/12/15 15:53, Arnaldo Carvalho de Melo wrote:
>> Em Tue, Dec 08, 2015 at 09:29:51AM +0200, Adrian Hunter escreveu:
>>> On 07/12/15 23:09, Arnaldo Carvalho de Melo wrote:
>>>> Em Thu, Dec 03, 2015 at 10:06:39AM +0100, Jiri Olsa escreveu:
>>>>> 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?
>>
>>>> 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?
> 
> 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 <adrian.hunter@intel.com>
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 <adrian.hunter@intel.com>
---
 tools/perf/util/evlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index d1b6c206bb93..306dacb33d8e 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -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;
 	}
-- 
1.9.1

Re: [PATCH 0/7] perf stat: Change event enable code

[Jiri Olsa]

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

SNIP

> 
> 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 <adrian.hunter@intel.com>
> 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.

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

> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/util/evlist.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index d1b6c206bb93..306dacb33d8e 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -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;
>  	}
> -- 
> 1.9.1
> 
>