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

Groups > linux.kernel > #1282831 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] perf stat: Change event enable code Jiri Olsa <jolsa@kernel.org> - 2015-12-03 10:20 +0100
    [PATCH 3/7] perf tools: Factor perf_evlist__(enable|disable) functions Jiri Olsa <jolsa@kernel.org> - 2015-12-03 10:20 +0100
      [tip:perf/core] perf evlist: Factor perf_evlist__(enable|disable)   functions tip-bot for Jiri Olsa <tipbot@zytor.com> - 2015-12-08 05:40 +0100
    Re: [PATCH 0/7] perf stat: Change event enable code Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-07 22:10 +0100
      Re: [PATCH 0/7] perf stat: Change event enable code Adrian Hunter <adrian.hunter@intel.com> - 2015-12-08 08:40 +0100
        Re: [PATCH 0/7] perf stat: Change event enable code Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-12-08 15:00 +0100
          Re: [PATCH 0/7] perf stat: Change event enable code Adrian Hunter <adrian.hunter@intel.com> - 2015-12-09 14:50 +0100
            Re: [PATCH 0/7] perf stat: Change event enable code Adrian Hunter <adrian.hunter@intel.com> - 2015-12-11 13:50 +0100

#1282831 — [PATCH 0/7] 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(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1282833 — [PATCH 3/7] perf tools: 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.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1285928

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1286294

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1286496

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1287505

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1289531

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web