[GIT PULL] perf tools fixes

[GIT PULL] perf tools fixes

[Frederic Weisbecker]

Ingo,

Please pull the perf/urgent branch that can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
	perf/urgent

Thanks,
	Frederic
---

Frederic Weisbecker (2):
      perf tools: Fix sample size bit operations
      perf tools: Fix ommitted mmap data update on remap


 tools/perf/util/event.c   |    2 +-
 tools/perf/util/session.c |   39 ++++++++++++++++++++++++++-------------
 2 files changed, 27 insertions(+), 14 deletions(-)

[PATCH 1/2] perf tools: Fix sample size bit operations

[Frederic Weisbecker]

What we want is to count the number of bits in the mask,
not some other random operation written in the middle
of the night.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/util/event.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 17c1c3c..d3fa7e4 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -42,7 +42,7 @@ int perf_sample_size(u64 sample_type)
 	int i;
 
 	for (i = 0; i < 64; i++) {
-		if ((mask << i) & 1)
+		if (mask & (1 << i))
 			size++;
 	}
 
-- 
1.7.3.2

Re: [PATCH 1/2] perf tools: Fix sample size bit operations

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> What we want is to count the number of bits in the mask,
> not some other random operation written in the middle
> of the night.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Stephane Eranian <eranian@google.com>
> ---
>  tools/perf/util/event.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index 17c1c3c..d3fa7e4 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -42,7 +42,7 @@ int perf_sample_size(u64 sample_type)
>  	int i;
>  
>  	for (i = 0; i < 64; i++) {
> -		if ((mask << i) & 1)
> +		if (mask & (1 << i))
>  			size++;
>  	}

I fixed this to be 1UL and applied your fixes out of email - perf top and perf 
report works fine now, so this was a 64-bitness bug (you probably used a 32-bit 
system for testing?).

Btw., shouldnt this use hweight() or such?

Thanks,

	Ingo

Re: [PATCH 1/2] perf tools: Fix sample size bit operations

[Frederic Weisbecker]

On Mon, May 23, 2011 at 01:28:15PM +0200, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > What we want is to count the number of bits in the mask,
> > not some other random operation written in the middle
> > of the night.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: Stephane Eranian <eranian@google.com>
> > ---
> >  tools/perf/util/event.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> > index 17c1c3c..d3fa7e4 100644
> > --- a/tools/perf/util/event.c
> > +++ b/tools/perf/util/event.c
> > @@ -42,7 +42,7 @@ int perf_sample_size(u64 sample_type)
> >  	int i;
> >  
> >  	for (i = 0; i < 64; i++) {
> > -		if ((mask << i) & 1)
> > +		if (mask & (1 << i))
> >  			size++;
> >  	}
> 
> I fixed this to be 1UL and applied your fixes out of email - perf top and perf 
> report works fine now, so this was a 64-bitness bug (you probably used a 32-bit 
> system for testing?).

Nope I was using a 64 machine for testing, but I only tested sched_switch
events with callchains on perf report and perf script.

Perhaps that bug triggered only on perf top or with default cpu-cycle events.

> 
> Btw., shouldnt this use hweight() or such?

A real proper way would better have that yeah :)

[tip:perf/urgent] perf tools: Fix sample size bit operations

[tip-bot for Frederic Weisbecker]

Commit-ID:  3cb6d1540880e767d911b79eb49578de2190f428
Gitweb:     http://git.kernel.org/tip/3cb6d1540880e767d911b79eb49578de2190f428
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Mon, 23 May 2011 13:06:27 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 23 May 2011 13:26:36 +0200

perf tools: Fix sample size bit operations

What we want is to count the number of bits in the mask,
not some other random operation written in the middle
of the night.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1306148788-6179-2-git-send-email-fweisbec@gmail.com
[ Fixed perf_event__names[] alignment which was nearby and hurting my eyes ... ]
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 tools/perf/util/event.c |   32 ++++++++++++++++----------------
 1 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 17c1c3c..252b72a 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -9,21 +9,21 @@
 #include "thread_map.h"
 
 static const char *perf_event__names[] = {
-	[0]			 = "TOTAL",
-	[PERF_RECORD_MMAP]	 = "MMAP",
-	[PERF_RECORD_LOST]	 = "LOST",
-	[PERF_RECORD_COMM]	 = "COMM",
-	[PERF_RECORD_EXIT]	 = "EXIT",
-	[PERF_RECORD_THROTTLE]	 = "THROTTLE",
-	[PERF_RECORD_UNTHROTTLE] = "UNTHROTTLE",
-	[PERF_RECORD_FORK]	 = "FORK",
-	[PERF_RECORD_READ]	 = "READ",
-	[PERF_RECORD_SAMPLE]	 = "SAMPLE",
-	[PERF_RECORD_HEADER_ATTR]	 = "ATTR",
-	[PERF_RECORD_HEADER_EVENT_TYPE]	 = "EVENT_TYPE",
-	[PERF_RECORD_HEADER_TRACING_DATA]	 = "TRACING_DATA",
-	[PERF_RECORD_HEADER_BUILD_ID]	 = "BUILD_ID",
-	[PERF_RECORD_FINISHED_ROUND]	 = "FINISHED_ROUND",
+	[0]					= "TOTAL",
+	[PERF_RECORD_MMAP]			= "MMAP",
+	[PERF_RECORD_LOST]			= "LOST",
+	[PERF_RECORD_COMM]			= "COMM",
+	[PERF_RECORD_EXIT]			= "EXIT",
+	[PERF_RECORD_THROTTLE]			= "THROTTLE",
+	[PERF_RECORD_UNTHROTTLE]		= "UNTHROTTLE",
+	[PERF_RECORD_FORK]			= "FORK",
+	[PERF_RECORD_READ]			= "READ",
+	[PERF_RECORD_SAMPLE]			= "SAMPLE",
+	[PERF_RECORD_HEADER_ATTR]		= "ATTR",
+	[PERF_RECORD_HEADER_EVENT_TYPE]		= "EVENT_TYPE",
+	[PERF_RECORD_HEADER_TRACING_DATA]	= "TRACING_DATA",
+	[PERF_RECORD_HEADER_BUILD_ID]		= "BUILD_ID",
+	[PERF_RECORD_FINISHED_ROUND]		= "FINISHED_ROUND",
 };
 
 const char *perf_event__name(unsigned int id)
@@ -42,7 +42,7 @@ int perf_sample_size(u64 sample_type)
 	int i;
 
 	for (i = 0; i < 64; i++) {
-		if ((mask << i) & 1)
+		if (mask & (1UL << i))
 			size++;
 	}
 

[PATCH 2/2] perf tools: Fix ommitted mmap data update on remap

[Frederic Weisbecker]

Commit eac9eacee16
"perf tools: Check we are able to read the event size on mmap"
brought a check to ensure we can read the size of the event
before dereferencing it, and do a remap otherwise to move
the buffer forward.

However that remap was ommitting all the necessary work to update
the new page offset, head, and to unmap previous pages, etc...

To fix this, gather all the code that fetches the event in a
seperate helper which does all the necessary checks about the
header/event size and tells us anytime a remap is needed.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/util/session.c |   39 ++++++++++++++++++++++++++-------------
 1 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 948327d..64500fc 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -960,6 +960,30 @@ out_err:
 	return err;
 }
 
+static union perf_event *
+fetch_mmaped_event(struct perf_session *session,
+		   u64 head, size_t mmap_size, char *buf)
+{
+	union perf_event *event;
+
+	/*
+	 * Ensure we have enough space remaining to read
+	 * the size of the event in the headers.
+	 */
+	if (head + sizeof(event->header) > mmap_size)
+		return NULL;
+
+	event = (union perf_event *)(buf + head);
+
+	if (session->header.needs_swap)
+		perf_event_header__bswap(&event->header);
+
+	if (head + event->header.size > mmap_size)
+		return NULL;
+
+	return event;
+}
+
 int __perf_session__process_events(struct perf_session *session,
 				   u64 data_offset, u64 data_size,
 				   u64 file_size, struct perf_event_ops *ops)
@@ -1014,19 +1038,8 @@ remap:
 	file_pos = file_offset + head;
 
 more:
-	/*
-	 * Ensure we have enough space remaining to read
-	 * the size of the event in the headers.
-	 */
-	if (head + sizeof(event->header) > mmap_size)
-		goto remap;
-
-	event = (union perf_event *)(buf + head);
-
-	if (session->header.needs_swap)
-		perf_event_header__bswap(&event->header);
-
-	if (head + event->header.size > mmap_size) {
+	event = fetch_mmaped_event(session, head, mmap_size, buf);
+	if (!event) {
 		if (mmaps[map_idx]) {
 			munmap(mmaps[map_idx], mmap_size);
 			mmaps[map_idx] = NULL;
-- 
1.7.3.2

[tip:perf/urgent] perf tools: Fix ommitted mmap data update on remap

[tip-bot for Frederic Weisbecker]

Commit-ID:  998bedc8c56c6869de457c845cbd328592e5e82e
Gitweb:     http://git.kernel.org/tip/998bedc8c56c6869de457c845cbd328592e5e82e
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Mon, 23 May 2011 13:06:28 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 23 May 2011 13:22:57 +0200

perf tools: Fix ommitted mmap data update on remap

Commit eac9eacee16 "perf tools: Check we are able to read the event
size on mmap" brought a check to ensure we can read the size of the
event before dereferencing it, and do a remap otherwise to move the
buffer forward.

However that remap was ommitting all the necessary work to
update the new page offset, head, and to unmap previous pages,
etc...

To fix this, gather all the code that fetches the event in a
seperate helper which does all the necessary checks about the
header/event size and tells us anytime a remap is needed.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1306148788-6179-3-git-send-email-fweisbec@gmail.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 tools/perf/util/session.c |   39 ++++++++++++++++++++++++++-------------
 1 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 948327d..64500fc 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -960,6 +960,30 @@ out_err:
 	return err;
 }
 
+static union perf_event *
+fetch_mmaped_event(struct perf_session *session,
+		   u64 head, size_t mmap_size, char *buf)
+{
+	union perf_event *event;
+
+	/*
+	 * Ensure we have enough space remaining to read
+	 * the size of the event in the headers.
+	 */
+	if (head + sizeof(event->header) > mmap_size)
+		return NULL;
+
+	event = (union perf_event *)(buf + head);
+
+	if (session->header.needs_swap)
+		perf_event_header__bswap(&event->header);
+
+	if (head + event->header.size > mmap_size)
+		return NULL;
+
+	return event;
+}
+
 int __perf_session__process_events(struct perf_session *session,
 				   u64 data_offset, u64 data_size,
 				   u64 file_size, struct perf_event_ops *ops)
@@ -1014,19 +1038,8 @@ remap:
 	file_pos = file_offset + head;
 
 more:
-	/*
-	 * Ensure we have enough space remaining to read
-	 * the size of the event in the headers.
-	 */
-	if (head + sizeof(event->header) > mmap_size)
-		goto remap;
-
-	event = (union perf_event *)(buf + head);
-
-	if (session->header.needs_swap)
-		perf_event_header__bswap(&event->header);
-
-	if (head + event->header.size > mmap_size) {
+	event = fetch_mmaped_event(session, head, mmap_size, buf);
+	if (!event) {
 		if (mmaps[map_idx]) {
 			munmap(mmaps[map_idx], mmap_size);
 			mmaps[map_idx] = NULL;

Re: [GIT PULL] perf tools fixes

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Ingo,
> 
> Please pull the perf/urgent branch that can be found at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> 	perf/urgent
> 
> Thanks,
> 	Frederic
> ---
> 
> Frederic Weisbecker (2):
>       perf tools: Fix sample size bit operations
>       perf tools: Fix ommitted mmap data update on remap
> 
> 
>  tools/perf/util/event.c   |    2 +-
>  tools/perf/util/session.c |   39 ++++++++++++++++++++++++++-------------
>  2 files changed, 27 insertions(+), 14 deletions(-)

Pulled, thanks a lot Frederic!

	Ingo

Re: [GIT PULL] perf tools fixes

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > Ingo,
> > 
> > Please pull the perf/urgent branch that can be found at:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> > 	perf/urgent
> > 
> > Thanks,
> > 	Frederic
> > ---
> > 
> > Frederic Weisbecker (2):
> >       perf tools: Fix sample size bit operations
> >       perf tools: Fix ommitted mmap data update on remap
> > 
> > 
> >  tools/perf/util/event.c   |    2 +-
> >  tools/perf/util/session.c |   39 ++++++++++++++++++++++++++-------------
> >  2 files changed, 27 insertions(+), 14 deletions(-)

Hm, you broke perf top:

   PerfTop:       0 irqs/sec  kernel:-nan%  exact: -nan% [1000Hz cycles],  (all, 16 CPUs)
-------------------------------------------------------------------------------------------------------

             samples  pcnt  DSO
             _______ _____  

Can't parse sample, err = -14
Can't parse sample, err = -14
Can't parse sample, err = -14
Can't parse sample, err = -14
Can't parse sample, err = -14
Can't parse sample, err = -14

and perf record+report as well:

aldebaran:~/linux/linux> perf record -a sleep 1
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.119 MB perf.data (~5201 samples) ]
aldebaran:~/linux/linux> perf report --stdio
The perf.data file has no samples!

so i'm unpulling these bits.

Thanks,

	Ingo