Skip to contentSkip to conversation

perf tools: callchain: Warn only once in empty node detection

PATCH New Frederic Weisbecker 3 participants 25 messages Aug 8, 2009 – Aug 10, 2009

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/4 perf tools: callchain: Warn only once in empty node detection

When we fill a new node that is found to be empty (sign of a bug), we print a message each time that happens. Sometimes we reach thousands of lines printed.

Just warn only once in that case (and use stderr instead of stdout).

Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
---
 tools/perf/util/callchain.c |    3 ++-
 tools/perf/util/util.h      |    3 +++
 tools/perf/util/wrapper.c   |   16 ++++++++++++++++
 3 files changed, 21 insertions(+), 1 deletions(-)
tools/perf/util/callchain.c +2 -1
index 98c5627..ce0c6d4 100644
@@ -13,6 +13,7 @@
 #include <stdio.h>
 #include <stdbool.h>
 #include <errno.h>
+#include "util.h"
 
 #include "callchain.h"
 
@@ -203,7 +204,7 @@ fill_node(struct callchain_node *node, struct ip_callchain *chain,
 	}
 	node->val_nr = chain->nr - start;
 	if (!node->val_nr)
-		printf("Warning: empty node in callchain tree\n");
+		fprintf_once(stderr, "Warning: empty node in callchain tree\n");
 }
 
 static void
tools/perf/util/util.h +3 -0
index 68fe157..386ea40 100644
@@ -148,6 +148,9 @@ static inline const char *skip_prefix(const char *str, const char *prefix)
 	return strncmp(str, prefix, len) ? NULL : str + len;
 }
 
+extern int fprintf_once(FILE *fp, const char *fmt, ...)
+	__attribute__((format (printf, 2, 3)));
+
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
 
 #ifndef PROT_READ
tools/perf/util/wrapper.c +16 -0
index 4574ac2..026c87c 100644
@@ -205,3 +205,19 @@ int xmkstemp(char *template)
 		die("Unable to create temporary file: %s", strerror(errno));
 	return fd;
 }
+
+int fprintf_once(FILE *fp, const char *fmt, ...)
+{
+	va_list args;
+	int ret;
+	static int once;
+
+	if (once++)
+		return 1;
+
+	va_start(args, fmt);
+	ret = vfprintf(fp, fmt, args);
+	va_end(args);
+
+	return ret;
+}
Signature · 1 line
1.6.2.3
#1 lore raw

PATCH 1/3 perfcounter: Initialize tracepoint record before any use

Despite that the tracepoint record is always present when the PERF_SAMPLE_TP_RECORD flag is set, gcc whines, thinking it might not be initialized:

kernel/perf_counter.c: In function ‘perf_counter_output’:
kernel/perf_counter.c:2650: warning: ‘tp’ may be used uninitialized in this function

Then, initialize it to NULL and always check if it's not NULL before dereference it.

Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Paul Mackerras <[email protected]>
---
 kernel/perf_counter.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
kernel/perf_counter.c +4 -3
index 142ae5a..a6963e2 100644
@@ -2647,7 +2647,7 @@ void perf_counter_output(struct perf_counter *counter, int nmi,
 		u64 counter;
 	} group_entry;
 	struct perf_callchain_entry *callchain = NULL;
-	struct perf_tracepoint_record *tp;
+	struct perf_tracepoint_record *tp = NULL;
 	int callchain_size = 0;
 	u64 time;
 	struct {
@@ -2718,7 +2718,8 @@ void perf_counter_output(struct perf_counter *counter, int nmi,
 
 	if (sample_type & PERF_SAMPLE_TP_RECORD) {
 		tp = data->private;
-		header.size += tp->size;
+		if (tp)
+			header.size += tp->size;
 	}
 
 	ret = perf_output_begin(&handle, counter, header.size, nmi, 1);
@@ -2784,7 +2785,7 @@ void perf_counter_output(struct perf_counter *counter, int nmi,
 		}
 	}
 
-	if (sample_type & PERF_SAMPLE_TP_RECORD)
+	if ((sample_type & PERF_SAMPLE_TP_RECORD) && tp)
 		perf_output_copy(&handle, tp->record, tp->size);
 
 	perf_output_end(&handle);
Signature · 1 line
1.6.2.3
#2 lore raw

Re: [PATCH 1/3] perfcounter: Initialize tracepoint record before any use

Quoted 10 lines · Frederic Weisbecker

Despite that the tracepoint record is always present when the PERF_SAMPLE_TP_RECORD flag is set, gcc whines, thinking it might not be initialized:

kernel/perf_counter.c: In function ‘perf_counter_output’:
kernel/perf_counter.c:2650: warning: ‘tp’ may be used uninitialized in this function

Then, initialize it to NULL and always check if it's not NULL before dereference it.

Quoted 7 lines
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Paul Mackerras <[email protected]>

---

Hmm, sorry I've resent the perf tools series by mistake. Please ignore the [PATCH */4] "perf tools: *" series that has already been sent.

The only ones are [PATCH */3] perfcounter: *

Thanks.

Quoted 38 lines
 kernel/perf_counter.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
kernel/perf_counter.c +4 -3
index 142ae5a..a6963e2 100644
@@ -2647,7 +2647,7 @@ void perf_counter_output(struct perf_counter *counter, int nmi,
 		u64 counter;
 	} group_entry;
 	struct perf_callchain_entry *callchain = NULL;
-	struct perf_tracepoint_record *tp;
+	struct perf_tracepoint_record *tp = NULL;
 	int callchain_size = 0;
 	u64 time;
 	struct {
@@ -2718,7 +2718,8 @@ void perf_counter_output(struct perf_counter *counter, int nmi,
 
 	if (sample_type & PERF_SAMPLE_TP_RECORD) {
 		tp = data->private;
-		header.size += tp->size;
+		if (tp)
+			header.size += tp->size;
 	}
 
 	ret = perf_output_begin(&handle, counter, header.size, nmi, 1);
@@ -2784,7 +2785,7 @@ void perf_counter_output(struct perf_counter *counter, int nmi,
 		}
 	}
 
-	if (sample_type & PERF_SAMPLE_TP_RECORD)
+	if ((sample_type & PERF_SAMPLE_TP_RECORD) && tp)
 		perf_output_copy(&handle, tp->record, tp->size);
 
 	perf_output_end(&handle);
Signature · 1 line
1.6.2.3
maintainer
#3 lore raw

[PATCH 4/3] perf_counter: Correct PERF_SAMPLE_RAW output

In reply to Frederic Weisbecker

Subject: perf_counter: Correct PERF_SAMPLE_RAW output
From: Peter Zijlstra <[email protected]>
Date: Mon Aug 10 11:16:52 CEST 2009

PERF_SAMPLE_* output switches should unconditionally output the correct format, as they are the only way to unambiguously parse the PERF_EVENT_SAMPLE data.

---
 include/linux/perf_counter.h |    2 ++
 include/trace/ftrace.h       |    3 ++-
 kernel/perf_counter.c        |   30 ++++++++++++++++++++++++------
 3 files changed, 28 insertions(+), 7 deletions(-)
include/linux/perf_counter.h +2 -0
Index: linux-2.6/include/linux/perf_counter.h
===================================================================
@@ -369,6 +369,8 @@ enum perf_event_type {
 	 *
 	 *	{ u64			nr,
 	 *	  u64			ips[nr];  } && PERF_SAMPLE_CALLCHAIN
+	 *	{ u32			size;
+	 *	  char                  data[size];}&& PERF_SAMPLE_RAW
 	 * };
 	 */
 	PERF_EVENT_SAMPLE		= 9,
include/trace/ftrace.h +2 -1
Index: linux-2.6/include/trace/ftrace.h
===================================================================
@@ -687,7 +687,8 @@ static void ftrace_profile_##call(proto)
 	pc = preempt_count();						\
 									\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
-	__entry_size = ALIGN(__data_size + sizeof(*entry), sizeof(u64));\
+	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\
+			     sizeof(u64));				\
 									\
 	do {								\
 		char raw_data[__entry_size];				\
kernel/perf_counter.c +24 -6
Index: linux-2.6/kernel/perf_counter.c
===================================================================
@@ -2647,7 +2647,6 @@ void perf_counter_output(struct perf_cou
 		u64 counter;
 	} group_entry;
 	struct perf_callchain_entry *callchain = NULL;
-	struct perf_raw_record *raw = NULL;
 	int callchain_size = 0;
 	u64 time;
 	struct {
@@ -2717,9 +2716,15 @@ void perf_counter_output(struct perf_cou
 	}
 
 	if (sample_type & PERF_SAMPLE_RAW) {
-		raw = data->raw;
-		if (raw)
-			header.size += raw->size;
+		int size = sizeof(u32);
+
+		if (data->raw)
+			size += data->raw->size;
+		else
+			size += sizeof(u32);
+
+		WARN_ON_ONCE(size & (sizeof(u64)-1));
+		header.size += size;
 	}
 
 	ret = perf_output_begin(&handle, counter, header.size, nmi, 1);
@@ -2785,8 +2790,21 @@ void perf_counter_output(struct perf_cou
 		}
 	}
 
-	if ((sample_type & PERF_SAMPLE_RAW) && raw)
-		perf_output_copy(&handle, raw->data, raw->size);
+	if (sample_type & PERF_SAMPLE_RAW) {
+		if (data->raw) {
+			perf_output_put(&handle, data->raw->size);
+			perf_output_copy(&handle, data->raw->data, data->raw->size);
+		} else {
+			struct {
+				u32	size;
+				u32	data;
+			} raw = {
+				.size = sizeof(u32),
+				.data = 0,
+			};
+			perf_output_put(&handle, raw);
+		}
+	}
 
 	perf_output_end(&handle);
 }
#4 lore raw

[tip:perfcounters/urgent] perf_counter: Correct PERF_SAMPLE_RAW output

Commit-ID:  a044560c3a1f0ad75ce685c1ed7604820b9ed319
Gitweb:     http://git.kernel.org/tip/a044560c3a1f0ad75ce685c1ed7604820b9ed319
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 10 Aug 2009 11:16:52 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 10 Aug 2009 11:33:09 +0200

perf_counter: Correct PERF_SAMPLE_RAW output

PERF_SAMPLE_* output switches should unconditionally output the correct format, as they are the only way to unambiguously parse the PERF_EVENT_SAMPLE data.

Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Paul Mackerras <[email protected]>

LKML-Reference: <1249896447.17467.74.camel@twins>

---
 include/linux/perf_counter.h |    2 ++
 include/trace/ftrace.h       |    3 ++-
 kernel/perf_counter.c        |   30 ++++++++++++++++++++++++------
 3 files changed, 28 insertions(+), 7 deletions(-)
include/linux/perf_counter.h +2 -0
index 2aabe43..a9d823a 100644
@@ -369,6 +369,8 @@ enum perf_event_type {
 	 *
 	 *	{ u64			nr,
 	 *	  u64			ips[nr];  } && PERF_SAMPLE_CALLCHAIN
+	 *	{ u32			size;
+	 *	  char                  data[size];}&& PERF_SAMPLE_RAW
 	 * };
 	 */
 	PERF_EVENT_SAMPLE		= 9,
include/trace/ftrace.h +2 -1
index 7fb16d9..7167b9b 100644
@@ -685,7 +685,8 @@ static void ftrace_profile_##call(proto)				\
 	pc = preempt_count();						\
 									\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
-	__entry_size = ALIGN(__data_size + sizeof(*entry), sizeof(u64));\
+	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\
+			     sizeof(u64));				\
 									\
 	do {								\
 		char raw_data[__entry_size];				\
kernel/perf_counter.c +24 -6
index 546e62d..5229d16 100644
@@ -2646,7 +2646,6 @@ static void perf_counter_output(struct perf_counter *counter, int nmi,
 		u64 counter;
 	} group_entry;
 	struct perf_callchain_entry *callchain = NULL;
-	struct perf_raw_record *raw = NULL;
 	int callchain_size = 0;
 	u64 time;
 	struct {
@@ -2716,9 +2715,15 @@ static void perf_counter_output(struct perf_counter *counter, int nmi,
 	}
 
 	if (sample_type & PERF_SAMPLE_RAW) {
-		raw = data->raw;
-		if (raw)
-			header.size += raw->size;
+		int size = sizeof(u32);
+
+		if (data->raw)
+			size += data->raw->size;
+		else
+			size += sizeof(u32);
+
+		WARN_ON_ONCE(size & (sizeof(u64)-1));
+		header.size += size;
 	}
 
 	ret = perf_output_begin(&handle, counter, header.size, nmi, 1);
@@ -2784,8 +2789,21 @@ static void perf_counter_output(struct perf_counter *counter, int nmi,
 		}
 	}
 
-	if ((sample_type & PERF_SAMPLE_RAW) && raw)
-		perf_output_copy(&handle, raw->data, raw->size);
+	if (sample_type & PERF_SAMPLE_RAW) {
+		if (data->raw) {
+			perf_output_put(&handle, data->raw->size);
+			perf_output_copy(&handle, data->raw->data, data->raw->size);
+		} else {
+			struct {
+				u32	size;
+				u32	data;
+			} raw = {
+				.size = sizeof(u32),
+				.data = 0,
+			};
+			perf_output_put(&handle, raw);
+		}
+	}
 
 	perf_output_end(&handle);
 }
author Acked-by
#5 lore raw

Re: [PATCH 4/3] perf_counter: Correct PERF_SAMPLE_RAW output

In reply to Peter Zijlstra
Quoted 9 lines · Peter Zijlstra

Subject: perf_counter: Correct PERF_SAMPLE_RAW output
From: Peter Zijlstra <[email protected]>
Date: Mon Aug 10 11:16:52 CEST 2009

PERF_SAMPLE_* output switches should unconditionally output the correct format, as they are the only way to unambiguously parse the PERF_EVENT_SAMPLE data.

I economized this part because I thought ftrace events could be parsed the same way we parse binary events through debugfs. The size of an ftrace event can be deduced from its format.

But I must confess that would require a pre-parsing of dynamic strings.

Quoted 89 lines
---
 include/linux/perf_counter.h |    2 ++
 include/trace/ftrace.h       |    3 ++-
 kernel/perf_counter.c        |   30 ++++++++++++++++++++++++------
 3 files changed, 28 insertions(+), 7 deletions(-)

Index: linux-2.6/include/linux/perf_counter.h
===================================================================

include/linux/perf_counter.h +2 -0
@@ -369,6 +369,8 @@ enum perf_event_type {
 	 *
 	 *	{ u64			nr,
 	 *	  u64			ips[nr];  } && PERF_SAMPLE_CALLCHAIN
+	 *	{ u32			size;
+	 *	  char                  data[size];}&& PERF_SAMPLE_RAW
 	 * };
 	 */
 	PERF_EVENT_SAMPLE		= 9,

Index: linux-2.6/include/trace/ftrace.h
===================================================================

include/trace/ftrace.h +2 -1
@@ -687,7 +687,8 @@ static void ftrace_profile_##call(proto)
 	pc = preempt_count();						\
 									\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
-	__entry_size = ALIGN(__data_size + sizeof(*entry), sizeof(u64));\
+	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\
+			     sizeof(u64));				\
 									\
 	do {								\
 		char raw_data[__entry_size];				\

Index: linux-2.6/kernel/perf_counter.c
===================================================================

kernel/perf_counter.c +24 -6
@@ -2647,7 +2647,6 @@ void perf_counter_output(struct perf_cou
 		u64 counter;
 	} group_entry;
 	struct perf_callchain_entry *callchain = NULL;
-	struct perf_raw_record *raw = NULL;
 	int callchain_size = 0;
 	u64 time;
 	struct {
@@ -2717,9 +2716,15 @@ void perf_counter_output(struct perf_cou
 	}
 
 	if (sample_type & PERF_SAMPLE_RAW) {
-		raw = data->raw;
-		if (raw)
-			header.size += raw->size;
+		int size = sizeof(u32);
+
+		if (data->raw)
+			size += data->raw->size;
+		else
+			size += sizeof(u32);
+
+		WARN_ON_ONCE(size & (sizeof(u64)-1));
+		header.size += size;
 	}
 
 	ret = perf_output_begin(&handle, counter, header.size, nmi, 1);
@@ -2785,8 +2790,21 @@ void perf_counter_output(struct perf_cou
 		}
 	}
 
-	if ((sample_type & PERF_SAMPLE_RAW) && raw)
-		perf_output_copy(&handle, raw->data, raw->size);
+	if (sample_type & PERF_SAMPLE_RAW) {
+		if (data->raw) {
+			perf_output_put(&handle, data->raw->size);
+			perf_output_copy(&handle, data->raw->data, data->raw->size);
+		} else {
+			struct {
+				u32	size;
+				u32	data;
+			} raw = {
+				.size = sizeof(u32),
+				.data = 0,
+			};
+			perf_output_put(&handle, raw);
+		}
+	}
 
 	perf_output_end(&handle);
 }
#6 lore raw

Re: [PATCH 4/3] perf_counter: Correct PERF_SAMPLE_RAW output

In reply to Peter Zijlstra
Quoted 33 lines · Peter Zijlstra

Subject: perf_counter: Correct PERF_SAMPLE_RAW output
From: Peter Zijlstra <[email protected]>
Date: Mon Aug 10 11:16:52 CEST 2009

PERF_SAMPLE_* output switches should unconditionally output the correct format, as they are the only way to unambiguously parse the PERF_EVENT_SAMPLE data.

---
 include/linux/perf_counter.h |    2 ++
 include/trace/ftrace.h       |    3 ++-
 kernel/perf_counter.c        |   30 ++++++++++++++++++++++++------
 3 files changed, 28 insertions(+), 7 deletions(-)

Index: linux-2.6/include/linux/perf_counter.h
===================================================================

include/linux/perf_counter.h +2 -0
@@ -369,6 +369,8 @@ enum perf_event_type {
 	 *
 	 *	{ u64			nr,
 	 *	  u64			ips[nr];  } && PERF_SAMPLE_CALLCHAIN
+	 *	{ u32			size;
+	 *	  char                  data[size];}&& PERF_SAMPLE_RAW
 	 * };
 	 */
 	PERF_EVENT_SAMPLE		= 9,

Index: linux-2.6/include/trace/ftrace.h
===================================================================

include/trace/ftrace.h static void ftrace_profile_##call(proto) · line 691

Quoted 6 lines
include/trace/ftrace.h +2 -1
@@ -687,7 +687,8 @@ static void ftrace_profile_##call(proto)
 	pc = preempt_count();						\
 									\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
-	__entry_size = ALIGN(__data_size + sizeof(*entry), sizeof(u64));\
+	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\
+			     sizeof(u64));				\

Here you are reserving a room for the size of the buffer inside the buffer.

Quoted 16 lines
include/trace/ftrace.h +0 -0
 									\
 	do {								\
 		char raw_data[__entry_size];				\

Index: linux-2.6/kernel/perf_counter.c
===================================================================

kernel/perf_counter.c +0 -1
@@ -2647,7 +2647,6 @@ void perf_counter_output(struct perf_cou
 		u64 counter;
 	} group_entry;
 	struct perf_callchain_entry *callchain = NULL;
-	struct perf_raw_record *raw = NULL;
 	int callchain_size = 0;
 	u64 time;
 	struct {

kernel/perf_counter.c void perf_counter_output(struct perf_cou · line 2722

Quoted 10 lines
kernel/perf_counter.c +4 -3
@@ -2717,9 +2716,15 @@ void perf_counter_output(struct perf_cou
 	}
 
 	if (sample_type & PERF_SAMPLE_RAW) {
-		raw = data->raw;
-		if (raw)
-			header.size += raw->size;
+		int size = sizeof(u32);
+
+		if (data->raw)
+			size += data->raw->size;

And here you reserve a size for the buffer once again?

Quoted 9 lines
kernel/perf_counter.c +5 -0
+		else
+			size += sizeof(u32);
+
+		WARN_ON_ONCE(size & (sizeof(u64)-1));
+		header.size += size;
 	}
 
 	ret = perf_output_begin(&handle, counter, header.size, nmi, 1);

kernel/perf_counter.c void perf_counter_output(struct perf_cou · line 2796

Quoted 9 lines
kernel/perf_counter.c +4 -2
@@ -2785,8 +2790,21 @@ void perf_counter_output(struct perf_cou
 		}
 	}
 
-	if ((sample_type & PERF_SAMPLE_RAW) && raw)
-		perf_output_copy(&handle, raw->data, raw->size);
+	if (sample_type & PERF_SAMPLE_RAW) {
+		if (data->raw) {
+			perf_output_put(&handle, data->raw->size);
+			perf_output_copy(&handle, data->raw->data, data->raw->size);

And actually you copy the buffer that has the size of the buffer plus the u32 reserved for the size, whereas the size has already been copied.

When I look at a perf sample raw it gives me the following:

.  0000:  09 00 00 00 01 00 54 00 d0 c7 00 81 ff ff ff ff  ......T........
.  0010:  69 01 00 00 69 01 00 00 e6 15 00 00 00 00 00 00  i...i..........
.  0020:  30 00 00 00 2b 00 01 02 69 01 00 00 69 01 00 00  0...+...i...i..

^ ^ ^ ^ ^ ^ ^ ^ ^ ^ .................
Buf size struct trace_entry

. 0030: 6b 6f 6e 64 65 6d 61 6e 64 2f 31 00 00 00 00 00 kondemand/1....

^ ^ ^ ^ .................................. Rest of struct ftrace_raw_<call>

. 0040: 69 01 00 00 70 82 46 81 ff ff ff ff 00 00 00 00 i...p.F........

.................................. ^ ^ ^ ^
The room you have reserved for the buffer size (zeroed from a tmp patch)
. 0050: 00 00 00 00

          ^  ^  ^  ^
          padding from alignment (ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\
			     sizeof(u64));

I first thought the 0x4c bytes was a padding from gcc because sizeof(struct ftrace_raw_<call>) % 8 != 0
But no, it is equal to 0: It's between 0x4c and 0x24 = 40 bytes.

I'm preparing a patch to fix this. Just wanted to expose my idea here, because I'm perhaps wrong in the middle of this dump.

I'll do the alignment right in the end, just before inserting the entry in the output. That will be more easy. I'll zeroe the rest in the same time.

Quoted 15 lines
kernel/perf_counter.c +11 -0
+		} else {
+			struct {
+				u32	size;
+				u32	data;
+			} raw = {
+				.size = sizeof(u32),
+				.data = 0,
+			};
+			perf_output_put(&handle, raw);
+		}
+	}
 
 	perf_output_end(&handle);
 }
maintainer
#7 lore raw

Re: [PATCH 4/3] perf_counter: Correct PERF_SAMPLE_RAW output

Quoted 7 lines · Frederic Weisbecker
Quoted 7 lines

Index: linux-2.6/include/trace/ftrace.h
===================================================================

include/trace/ftrace.h +0 -0
@@ -687,7 +687,8 @@ static void ftrace_profile_##call(proto)
 	pc = preempt_count();						\
 									\
Quoted 8 lines

include/trace/ftrace.h static void ftrace_profile_##call(proto) · line 691

Quoted 4 lines
include/trace/ftrace.h +2 -1
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
-	__entry_size = ALIGN(__data_size + sizeof(*entry), sizeof(u64));\
+	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\
+			     sizeof(u64));				\

Here you are reserving a room for the size of the buffer inside the buffer.

No, here I align so that __entry_size + the u32 ends up at a u64 boundary.

Oh gah, now I see.. I should have subtracted sizeof(u32) after the alignment.

Quoted 15 lines

include/trace/ftrace.h void perf_counter_output(struct perf_cou · line 2722

Quoted 11 lines
include/trace/ftrace.h +4 -3
@@ -2717,9 +2716,15 @@ void perf_counter_output(struct perf_cou
 	}
 
 	if (sample_type & PERF_SAMPLE_RAW) {
-		raw = data->raw;
-		if (raw)
-			header.size += raw->size;
+		int size = sizeof(u32);
+
+		if (data->raw)
+			size += data->raw->size;

And here you reserve a size for the buffer once again?

This is the actual output buffer reservation code, that needs the u32 above and the data->raw size.

Quoted 5 lines

include/trace/ftrace.h void perf_counter_output(struct perf_cou · line 2726

Quoted 5 lines
include/trace/ftrace.h +4 -0
+		else
+			size += sizeof(u32);
+
+		WARN_ON_ONCE(size & (sizeof(u64)-1));

Which when taken together should be u64 aligned.

So this will always fail with the above.

Quoted 38 lines
Quoted 5 lines
include/trace/ftrace.h +1 -0
+		header.size += size;
 	}
 
 	ret = perf_output_begin(&handle, counter, header.size, nmi, 1);

include/trace/ftrace.h void perf_counter_output(struct perf_cou · line 2796

Quoted 9 lines
include/trace/ftrace.h +4 -2
@@ -2785,8 +2790,21 @@ void perf_counter_output(struct perf_cou
 		}
 	}
 
-	if ((sample_type & PERF_SAMPLE_RAW) && raw)
-		perf_output_copy(&handle, raw->data, raw->size);
+	if (sample_type & PERF_SAMPLE_RAW) {
+		if (data->raw) {
+			perf_output_put(&handle, data->raw->size);
+			perf_output_copy(&handle, data->raw->data, data->raw->size);

And actually you copy the buffer that has the size of the buffer plus the u32 reserved for the size, whereas the size has already been copied.

When I look at a perf sample raw it gives me the following:

..  0000:  09 00 00 00 01 00 54 00 d0 c7 00 81 ff ff ff ff  ......T........
..  0010:  69 01 00 00 69 01 00 00 e6 15 00 00 00 00 00 00  i...i..........
..  0020:  30 00 00 00 2b 00 01 02 69 01 00 00 69 01 00 00  0...+...i...i..

^ ^ ^ ^ ^ ^ ^ ^ ^ ^ .................
Buf size struct trace_entry

.. 0030: 6b 6f 6e 64 65 6d 61 6e 64 2f 31 00 00 00 00 00 kondemand/1....

^ ^ ^ ^ .................................. Rest of struct ftrace_raw_<call>

.. 0040: 69 01 00 00 70 82 46 81 ff ff ff ff 00 00 00 00 i...p.F........

Quoted 8 lines

.................................. ^ ^ ^ ^
The room you have reserved for the buffer size (zeroed from a tmp patch)
.. 0050: 00 00 00 00

          ^  ^  ^  ^
          padding from alignment (ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\
			     sizeof(u64));

Right, one u32 too much :/

Quoted 9 lines

I first thought the 0x4c bytes was a padding from gcc because sizeof(struct ftrace_raw_<call>) % 8 != 0
But no, it is equal to 0: It's between 0x4c and 0x24 = 40 bytes.

I'm preparing a patch to fix this. Just wanted to expose my idea here, because I'm perhaps wrong in the middle of this dump.

I'll do the alignment right in the end, just before inserting the entry in the output. That will be more easy. I'll zeroe the rest in the same time.

OK.

#8 lore raw

[PATCH 6/3] perfcounter: Substract the buffer size field from the event record size

Quoted 4 lines · Peter Zijlstra
Quoted 2 lines

I'll do the alignment right in the end, just before inserting the entry in the output. That will be more easy. I'll zeroe the rest in the same time.

OK.

Actually I shouldn't, the alignment should be computed early, taking the field size into account.

That's not a problem though, I've just substracted the u32 field once I found the final aligned size.

See below:

---
From: Frederic Weisbecker <[email protected]>
Date: Mon, 10 Aug 2009 15:56:46 +0200
Subject: [PATCH 6/3] perfcounter: Substract the buffer size field from the event record size

We compute the perf raw sample size by aligning the raw ftrace event size plus the buffer size field itself. We do that instead of aligning only the perf raw sample size, so that we might economize some in some cases.

But this buffer size field is not stored in the perf raw sample, we must then substract its size from the buffer once we computed the alignment unless we may get a useless u32 field in the buffer.

---
 include/trace/ftrace.h      |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletions(-)
include/trace/ftrace.h +7 -1
index 4dc1a60..f98ff56 100644
@@ -639,7 +639,12 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
  *	pc = preempt_count();
  *
  *	__data_size = ftrace_get_offsets_<call>(&__data_offsets, args);
- *	__entry_size = __data_size + sizeof(*entry);
+ *
+ *	// Below we want to get the aligned size by taking into account
+ *	// the u32 field that will later store the buffer size
+ *	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),
+ *			     sizeof(u64));
+ *	__entry_size -= sizeof(u32);
  *
  *	do {
  *		char raw_data[__entry_size]; <- allocate our sample in the stack
@@ -689,6 +694,7 @@ static void ftrace_profile_##call(proto)				\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
 	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\
 			     sizeof(u64));				\
+	__entry_size -= sizeof(u32);					\
 									\
 	do {								\
 		char raw_data[__entry_size];				\
Signature · 1 line
1.6.2.3
maintainer Acked-by
#9 lore raw

Re: [PATCH 6/3] perfcounter: Substract the buffer size field from the event record size

Quoted 9 lines · Frederic Weisbecker

From: Frederic Weisbecker <[email protected]>
Date: Mon, 10 Aug 2009 15:56:46 +0200
Subject: [PATCH 6/3] perfcounter: Substract the buffer size field from the event record size

We compute the perf raw sample size by aligning the raw ftrace event size plus the buffer size field itself. We do that instead of aligning only the perf raw sample size, so that we might economize some in some cases.

Quoted 5 lines

But this buffer size field is not stored in the perf raw sample, we must then substract its size from the buffer once we computed the alignment unless we may get a useless u32 field in the buffer.

Thanks!

Quoted 30 lines
---
 include/trace/ftrace.h      |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletions(-)
include/trace/ftrace.h +7 -1
index 4dc1a60..f98ff56 100644
@@ -639,7 +639,12 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
  *	pc = preempt_count();
  *
  *	__data_size = ftrace_get_offsets_<call>(&__data_offsets, args);
- *	__entry_size = __data_size + sizeof(*entry);
+ *
+ *	// Below we want to get the aligned size by taking into account
+ *	// the u32 field that will later store the buffer size
+ *	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),
+ *			     sizeof(u64));
+ *	__entry_size -= sizeof(u32);
  *
  *	do {
  *		char raw_data[__entry_size]; <- allocate our sample in the stack
@@ -689,6 +694,7 @@ static void ftrace_profile_##call(proto)				\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
 	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\
 			     sizeof(u64));				\
+	__entry_size -= sizeof(u32);					\
 									\
 	do {								\
 		char raw_data[__entry_size];				\
#10 lore raw

[tip:perfcounters/urgent] perf_counter: Subtract the buffer size field from the event record size

In reply to Frederic Weisbecker
Commit-ID:  304703aba31a87903b8c0db8f5e6890cac2d596d
Gitweb:     http://git.kernel.org/tip/304703aba31a87903b8c0db8f5e6890cac2d596d
Author:     Frederic Weisbecker <[email protected]>
AuthorDate: Mon, 10 Aug 2009 16:11:32 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 10 Aug 2009 16:18:50 +0200

perf_counter: Subtract the buffer size field from the event record size

We compute the perf raw sample size by aligning the raw ftrace event size plus the buffer size field itself. We do that instead of aligning only the perf raw sample size, so that we might economize some in some cases.

But this buffer size field is not stored in the perf raw sample, we must then substract its size from the buffer once we computed the alignment unless we may get a useless u32 field in the buffer.

Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Paul Mackerras <[email protected]>

LKML-Reference: <20090810141129.GA5124@nowhere>

---
 include/trace/ftrace.h |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)
include/trace/ftrace.h +7 -1
index 7167b9b..a05524f 100644
@@ -637,7 +637,12 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
  *	pc = preempt_count();
  *
  *	__data_size = ftrace_get_offsets_<call>(&__data_offsets, args);
- *	__entry_size = __data_size + sizeof(*entry);
+ *
+ *	// Below we want to get the aligned size by taking into account
+ *	// the u32 field that will later store the buffer size
+ *	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),
+ *			     sizeof(u64));
+ *	__entry_size -= sizeof(u32);
  *
  *	do {
  *		char raw_data[__entry_size]; <- allocate our sample in the stack
@@ -687,6 +692,7 @@ static void ftrace_profile_##call(proto)				\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
 	__entry_size = ALIGN(__data_size + sizeof(*entry) + sizeof(u32),\
 			     sizeof(u64));				\
+	__entry_size -= sizeof(u32);					\
 									\
 	do {								\
 		char raw_data[__entry_size];				\
maintainer
#11 lore raw

[PATCH 5/3] perf_counter: Require CAP_SYS_ADMIN for raw tracepoint data

In reply to Frederic Weisbecker

Subject: perf_counter: Require CAP_SYS_ADMIN for raw tracepoint data
From: Peter Zijlstra <[email protected]>
Date: Mon Aug 10 11:20:12 CEST 2009

Raw tracepoint data is a severe data leak, restrict this to root only.

---
 kernel/perf_counter.c |    8 ++++++++
 1 file changed, 8 insertions(+)
kernel/perf_counter.c +8 -0
Index: linux-2.6/kernel/perf_counter.c
===================================================================
@@ -3788,6 +3788,14 @@ static void tp_perf_counter_destroy(stru
 
 static const struct pmu *tp_perf_counter_init(struct perf_counter *counter)
 {
+	/*
+	 * Raw tracepoint data is a severe data leak, only allow root to
+	 * have these.
+	 */
+	if ((counter->attr.sample_type & PERF_SAMPLE_RAW) &&
+			!capable(CAP_SYS_ADMIN))
+		return ERR_PTR(-EPERM);
+
 	if (ftrace_profile_enable(counter->attr.config))
 		return NULL;
 
#12 lore raw

[tip:perfcounters/urgent] perf_counter: Require CAP_SYS_ADMIN for raw tracepoint data

Commit-ID:  a4e95fc2cbb31d70a65beffeaf8773f881328c34
Gitweb:     http://git.kernel.org/tip/a4e95fc2cbb31d70a65beffeaf8773f881328c34
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 10 Aug 2009 11:20:12 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 10 Aug 2009 11:33:09 +0200

perf_counter: Require CAP_SYS_ADMIN for raw tracepoint data

Raw tracepoint data contains various kernel internals and data from other users, so restrict this to CAP_SYS_ADMIN.

Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Paul Mackerras <[email protected]>

LKML-Reference: <1249896452.17467.75.camel@twins>

---
 kernel/perf_counter.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
kernel/perf_counter.c +8 -0
index 5229d16..b0b20a0 100644
@@ -3787,6 +3787,14 @@ static void tp_perf_counter_destroy(struct perf_counter *counter)
 
 static const struct pmu *tp_perf_counter_init(struct perf_counter *counter)
 {
+	/*
+	 * Raw tracepoint data is a severe data leak, only allow root to
+	 * have these.
+	 */
+	if ((counter->attr.sample_type & PERF_SAMPLE_RAW) &&
+			!capable(CAP_SYS_ADMIN))
+		return ERR_PTR(-EPERM);
+
 	if (ftrace_profile_enable(counter->attr.config))
 		return NULL;
 
#13 lore raw

PATCH 2/4 perf tools: callchain: Ignore empty callchains

In reply to Frederic Weisbecker

When the callchain tree comes to insert an empty backtrace, it raises a spurious warning about the fact we are inserting an empty. This is spurious because the radix tree assumes it did something wrong to reach this state. But it didn't, we just met an empty callchain that has to be ignored.

Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
---
 tools/perf/util/callchain.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
tools/perf/util/callchain.c +2 -0
index ce0c6d4..4c09b25 100644
@@ -337,5 +337,7 @@ __append_chain(struct callchain_node *root, struct ip_callchain *chain,
 void append_chain(struct callchain_node *root, struct ip_callchain *chain,
 		  struct symbol **syms)
 {
+	if (!chain->nr)
+		return;
 	__append_chain_children(root, chain, syms, 0);
 }
Signature · 1 line
1.6.2.3
#14 lore raw

PATCH 3/4 perf tools: callchain: Default display callchain from report if recorded with -g

In reply to Frederic Weisbecker

If we recorded with -g option to record the callchain, let's display the callchain by default from perf report.

The user can override this default using "-g none" option from perf report.

Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
---
 tools/perf/builtin-report.c |   16 +++++++++++++++-
 tools/perf/util/callchain.h |    1 +
 2 files changed, 16 insertions(+), 1 deletions(-)
tools/perf/builtin-report.c +15 -1
index a5e2f8d..c4a8e10 100644
@@ -68,7 +68,7 @@ static int		callchain;
 
 static
 struct callchain_param	callchain_param = {
-	.mode	= CHAIN_GRAPH_ABS,
+	.mode	= CHAIN_GRAPH_REL,
 	.min_percent = 0.5
 };
 
@@ -1836,6 +1836,13 @@ static int __cmd_report(void)
 					" -g?\n");
 			exit(-1);
 		}
+	} else if (callchain_param.mode != CHAIN_NONE && !callchain) {
+			callchain = 1;
+			if (register_callchain_param(&callchain_param) < 0) {
+				fprintf(stderr, "Can't register callchain"
+						" params\n");
+				exit(-1);
+			}
 	}
 
 	if (load_kernel() < 0) {
@@ -1974,6 +1981,13 @@ parse_callchain_opt(const struct option *opt __used, const char *arg,
 	else if (!strncmp(tok, "fractal", strlen(arg)))
 		callchain_param.mode = CHAIN_GRAPH_REL;
 
+	else if (!strncmp(tok, "none", strlen(arg))) {
+		callchain_param.mode = CHAIN_NONE;
+		callchain = 0;
+
+		return 0;
+	}
+
 	else
 		return -1;
 
tools/perf/util/callchain.h +1 -0
index b2d128e..a926ae4 100644
@@ -7,6 +7,7 @@
 #include "symbol.h"
 
 enum chain_mode {
+	CHAIN_NONE,
 	CHAIN_FLAT,
 	CHAIN_GRAPH_ABS,
 	CHAIN_GRAPH_REL
Signature · 1 line
1.6.2.3
#15 lore raw

PATCH 2/3 perfcounter: Generalize the tracepoint sampling to generic sampling

In reply to Frederic Weisbecker

Rename:

- PERF_SAMPLE_TP_RECORD to PERF_SAMPLE_RAW
- struct perf_tracepoint_record to perf_raw_record

We want the system in place that transport tracepoints raw samples events into the perf ring buffer to be generalized and usable by any type of counter.

Reported-by; Peter Zijlstra <[email protected]>

Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Paul Mackerras <[email protected]>
---
 include/linux/perf_counter.h |   10 +++++-----
 kernel/perf_counter.c        |   20 ++++++++++----------
 2 files changed, 15 insertions(+), 15 deletions(-)
include/linux/perf_counter.h +5 -5
index c484834..0a6f320 100644
@@ -121,7 +121,7 @@ enum perf_counter_sample_format {
 	PERF_SAMPLE_CPU				= 1U << 7,
 	PERF_SAMPLE_PERIOD			= 1U << 8,
 	PERF_SAMPLE_STREAM_ID			= 1U << 9,
-	PERF_SAMPLE_TP_RECORD			= 1U << 10,
+	PERF_SAMPLE_RAW				= 1U << 10,
 
 	PERF_SAMPLE_MAX = 1U << 11,		/* non-ABI */
 };
@@ -414,9 +414,9 @@ struct perf_callchain_entry {
 	__u64				ip[PERF_MAX_STACK_DEPTH];
 };
 
-struct perf_tracepoint_record {
-	int				size;
-	char				*record;
+struct perf_raw_record {
+	u32				size;
+	void				*data;
 };
 
 struct task_struct;
@@ -687,7 +687,7 @@ struct perf_sample_data {
 	struct pt_regs			*regs;
 	u64				addr;
 	u64				period;
-	void				*private;
+	struct perf_raw_record		*raw;
 };
 
 extern int perf_counter_overflow(struct perf_counter *counter, int nmi,
kernel/perf_counter.c +10 -10
index a6963e2..22e2839 100644
@@ -2647,7 +2647,7 @@ void perf_counter_output(struct perf_counter *counter, int nmi,
 		u64 counter;
 	} group_entry;
 	struct perf_callchain_entry *callchain = NULL;
-	struct perf_tracepoint_record *tp = NULL;
+	struct perf_raw_record *raw = NULL;
 	int callchain_size = 0;
 	u64 time;
 	struct {
@@ -2716,10 +2716,10 @@ void perf_counter_output(struct perf_counter *counter, int nmi,
 			header.size += sizeof(u64);
 	}
 
-	if (sample_type & PERF_SAMPLE_TP_RECORD) {
-		tp = data->private;
-		if (tp)
-			header.size += tp->size;
+	if (sample_type & PERF_SAMPLE_RAW) {
+		raw = data->raw;
+		if (raw)
+			header.size += raw->size;
 	}
 
 	ret = perf_output_begin(&handle, counter, header.size, nmi, 1);
@@ -2785,8 +2785,8 @@ void perf_counter_output(struct perf_counter *counter, int nmi,
 		}
 	}
 
-	if ((sample_type & PERF_SAMPLE_TP_RECORD) && tp)
-		perf_output_copy(&handle, tp->record, tp->size);
+	if ((sample_type & PERF_SAMPLE_RAW) && raw)
+		perf_output_copy(&handle, raw->data, raw->size);
 
 	perf_output_end(&handle);
 }
@@ -3741,15 +3741,15 @@ static const struct pmu perf_ops_task_clock = {
 void perf_tpcounter_event(int event_id, u64 addr, u64 count, void *record,
 			  int entry_size)
 {
-	struct perf_tracepoint_record tp = {
+	struct perf_raw_record raw = {
 		.size = entry_size,
-		.record = record,
+		.data = record,
 	};
 
 	struct perf_sample_data data = {
 		.regs = get_irq_regs(),
 		.addr = addr,
-		.private = &tp,
+		.raw = &raw,
 	};
 
 	if (!data.regs)
Signature · 1 line
1.6.2.3
#16 lore raw

[tip:perfcounters/core] perf_counter: Fix tracepoint sampling to be part of generic sampling

Commit-ID:  6455883ad50c575b0bc5f6d6ab45d42cd56b83c4
Gitweb:     http://git.kernel.org/tip/6455883ad50c575b0bc5f6d6ab45d42cd56b83c4
Author:     Frederic Weisbecker <[email protected]>
AuthorDate: Sat, 8 Aug 2009 04:26:37 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Sat, 8 Aug 2009 13:49:22 +0200

perf_counter: Fix tracepoint sampling to be part of generic sampling

Based on Peter's comments, make tracepoint sampling generic just like all the other sampling bits are. This is a rename with no code changes:

- PERF_SAMPLE_TP_RECORD to PERF_SAMPLE_RAW
- struct perf_tracepoint_record to perf_raw_record

We want the system in place that transport tracepoints raw samples events into the perf ring buffer to be generalized and usable by any type of counter.

Reported-by; Peter Zijlstra <[email protected]>

Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Paul Mackerras <[email protected]>

LKML-Reference: <[email protected]>

---
 include/linux/perf_counter.h |   10 +++++-----
 kernel/perf_counter.c        |   20 ++++++++++----------
 2 files changed, 15 insertions(+), 15 deletions(-)
include/linux/perf_counter.h +5 -5
index a67dd5c..2aabe43 100644
@@ -121,7 +121,7 @@ enum perf_counter_sample_format {
 	PERF_SAMPLE_CPU				= 1U << 7,
 	PERF_SAMPLE_PERIOD			= 1U << 8,
 	PERF_SAMPLE_STREAM_ID			= 1U << 9,
-	PERF_SAMPLE_TP_RECORD			= 1U << 10,
+	PERF_SAMPLE_RAW				= 1U << 10,
 
 	PERF_SAMPLE_MAX = 1U << 11,		/* non-ABI */
 };
@@ -414,9 +414,9 @@ struct perf_callchain_entry {
 	__u64				ip[PERF_MAX_STACK_DEPTH];
 };
 
-struct perf_tracepoint_record {
-	int				size;
-	char				*record;
+struct perf_raw_record {
+	u32				size;
+	void				*data;
 };
 
 struct task_struct;
@@ -687,7 +687,7 @@ struct perf_sample_data {
 	struct pt_regs			*regs;
 	u64				addr;
 	u64				period;
-	void				*private;
+	struct perf_raw_record		*raw;
 };
 
 extern int perf_counter_overflow(struct perf_counter *counter, int nmi,
kernel/perf_counter.c +10 -10
index 117622c..0023105 100644
@@ -2646,7 +2646,7 @@ static void perf_counter_output(struct perf_counter *counter, int nmi,
 		u64 counter;
 	} group_entry;
 	struct perf_callchain_entry *callchain = NULL;
-	struct perf_tracepoint_record *tp = NULL;
+	struct perf_raw_record *raw = NULL;
 	int callchain_size = 0;
 	u64 time;
 	struct {
@@ -2715,10 +2715,10 @@ static void perf_counter_output(struct perf_counter *counter, int nmi,
 			header.size += sizeof(u64);
 	}
 
-	if (sample_type & PERF_SAMPLE_TP_RECORD) {
-		tp = data->private;
-		if (tp)
-			header.size += tp->size;
+	if (sample_type & PERF_SAMPLE_RAW) {
+		raw = data->raw;
+		if (raw)
+			header.size += raw->size;
 	}
 
 	ret = perf_output_begin(&handle, counter, header.size, nmi, 1);
@@ -2784,8 +2784,8 @@ static void perf_counter_output(struct perf_counter *counter, int nmi,
 		}
 	}
 
-	if ((sample_type & PERF_SAMPLE_TP_RECORD) && tp)
-		perf_output_copy(&handle, tp->record, tp->size);
+	if ((sample_type & PERF_SAMPLE_RAW) && raw)
+		perf_output_copy(&handle, raw->data, raw->size);
 
 	perf_output_end(&handle);
 }
@@ -3740,15 +3740,15 @@ static const struct pmu perf_ops_task_clock = {
 void perf_tpcounter_event(int event_id, u64 addr, u64 count, void *record,
 			  int entry_size)
 {
-	struct perf_tracepoint_record tp = {
+	struct perf_raw_record raw = {
 		.size = entry_size,
-		.record = record,
+		.data = record,
 	};
 
 	struct perf_sample_data data = {
 		.regs = get_irq_regs(),
 		.addr = addr,
-		.private = &tp,
+		.raw = &raw,
 	};
 
 	if (!data.regs)
#17 lore raw

[tip:perfcounters/core] perf_counter: Fix tracepoint sampling to be part of generic sampling

In reply to Frederic Weisbecker
Commit-ID:  3a43ce68ae1758fa6a839386025ef45acb6baa22
Gitweb:     http://git.kernel.org/tip/3a43ce68ae1758fa6a839386025ef45acb6baa22
Author:     Frederic Weisbecker <[email protected]>
AuthorDate: Sat, 8 Aug 2009 04:26:37 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Sun, 9 Aug 2009 12:54:45 +0200

perf_counter: Fix tracepoint sampling to be part of generic sampling

Based on Peter's comments, make tracepoint sampling generic just like all the other sampling bits are. This is a rename with no code changes:

- PERF_SAMPLE_TP_RECORD to PERF_SAMPLE_RAW
- struct perf_tracepoint_record to perf_raw_record

We want the system in place that transport tracepoints raw samples events into the perf ring buffer to be generalized and usable by any type of counter.

Reported-by; Peter Zijlstra <[email protected]>

Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Paul Mackerras <[email protected]>

LKML-Reference: <[email protected]>

---
 include/linux/perf_counter.h |   10 +++++-----
 kernel/perf_counter.c        |   20 ++++++++++----------
 2 files changed, 15 insertions(+), 15 deletions(-)
include/linux/perf_counter.h +5 -5
index a67dd5c..2aabe43 100644
@@ -121,7 +121,7 @@ enum perf_counter_sample_format {
 	PERF_SAMPLE_CPU				= 1U << 7,
 	PERF_SAMPLE_PERIOD			= 1U << 8,
 	PERF_SAMPLE_STREAM_ID			= 1U << 9,
-	PERF_SAMPLE_TP_RECORD			= 1U << 10,
+	PERF_SAMPLE_RAW				= 1U << 10,
 
 	PERF_SAMPLE_MAX = 1U << 11,		/* non-ABI */
 };
@@ -414,9 +414,9 @@ struct perf_callchain_entry {
 	__u64				ip[PERF_MAX_STACK_DEPTH];
 };
 
-struct perf_tracepoint_record {
-	int				size;
-	char				*record;
+struct perf_raw_record {
+	u32				size;
+	void				*data;
 };
 
 struct task_struct;
@@ -687,7 +687,7 @@ struct perf_sample_data {
 	struct pt_regs			*regs;
 	u64				addr;
 	u64				period;
-	void				*private;
+	struct perf_raw_record		*raw;
 };
 
 extern int perf_counter_overflow(struct perf_counter *counter, int nmi,
kernel/perf_counter.c +10 -10
index 117622c..0023105 100644
@@ -2646,7 +2646,7 @@ static void perf_counter_output(struct perf_counter *counter, int nmi,
 		u64 counter;
 	} group_entry;
 	struct perf_callchain_entry *callchain = NULL;
-	struct perf_tracepoint_record *tp = NULL;
+	struct perf_raw_record *raw = NULL;
 	int callchain_size = 0;
 	u64 time;
 	struct {
@@ -2715,10 +2715,10 @@ static void perf_counter_output(struct perf_counter *counter, int nmi,
 			header.size += sizeof(u64);
 	}
 
-	if (sample_type & PERF_SAMPLE_TP_RECORD) {
-		tp = data->private;
-		if (tp)
-			header.size += tp->size;
+	if (sample_type & PERF_SAMPLE_RAW) {
+		raw = data->raw;
+		if (raw)
+			header.size += raw->size;
 	}
 
 	ret = perf_output_begin(&handle, counter, header.size, nmi, 1);
@@ -2784,8 +2784,8 @@ static void perf_counter_output(struct perf_counter *counter, int nmi,
 		}
 	}
 
-	if ((sample_type & PERF_SAMPLE_TP_RECORD) && tp)
-		perf_output_copy(&handle, tp->record, tp->size);
+	if ((sample_type & PERF_SAMPLE_RAW) && raw)
+		perf_output_copy(&handle, raw->data, raw->size);
 
 	perf_output_end(&handle);
 }
@@ -3740,15 +3740,15 @@ static const struct pmu perf_ops_task_clock = {
 void perf_tpcounter_event(int event_id, u64 addr, u64 count, void *record,
 			  int entry_size)
 {
-	struct perf_tracepoint_record tp = {
+	struct perf_raw_record raw = {
 		.size = entry_size,
-		.record = record,
+		.data = record,
 	};
 
 	struct perf_sample_data data = {
 		.regs = get_irq_regs(),
 		.addr = addr,
-		.private = &tp,
+		.raw = &raw,
 	};
 
 	if (!data.regs)
#18 lore raw

PATCH 4/4 perf tools: callchain: Display amount of ignored chains in fractal mode

In reply to Frederic Weisbecker

When we filter the callchains below a given percentage, we ignore them and the end result only shows entries that have an upper percentage than the filter threshold. It seems then that we have an imbalance in the percentage, as if the sum inside a profiled branch doesn't reach 100%.

Fix this by displaying the remaining hits that have been filtered but without more detail than their amount in each branches. Example while filtering below 50%:

7.73% [k] delay_tsc

                |
                |--98.22%-- __const_udelay
                |          |
                |          |--86.37%-- ath5k_hw_register_timeout
                |          |          ath5k_hw_noise_floor_calibration
                |          |          ath5k_hw_reset
                |          |          ath5k_reset
                |          |          ath5k_config
                |          |          ieee80211_hw_config
                |          |          |
                |          |          |--88.53%-- ieee80211_scan_work
                |          |          |          worker_thread
                |          |          |          kthread
                |          |          |          child_rip
                |          |           --11.47%-- [...]
                |           --13.63%-- [...]
                 --1.78%-- [...]
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
---
 tools/perf/builtin-report.c |   47 ++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 44 insertions(+), 3 deletions(-)
tools/perf/builtin-report.c +44 -3
index c4a8e10..99274ce 100644
@@ -891,6 +891,21 @@ ipchain__fprintf_graph(FILE *fp, struct callchain_list *chain, int depth,
 	return ret;
 }
 
+static struct symbol *rem_sq_bracket;
+static struct callchain_list rem_hits;
+
+static void init_rem_hits(void)
+{
+	rem_sq_bracket = malloc(sizeof(*rem_sq_bracket) + 6);
+	if (!rem_sq_bracket) {
+		fprintf(stderr, "Not enough memory to display remaining hits\n");
+		return;
+	}
+
+	strcpy(rem_sq_bracket->name, "[...]");
+	rem_hits.sym = rem_sq_bracket;
+}
+
 static size_t
 callchain__fprintf_graph(FILE *fp, struct callchain_node *self,
 			u64 total_samples, int depth, int depth_mask)
@@ -900,6 +915,7 @@ callchain__fprintf_graph(FILE *fp, struct callchain_node *self,
 	struct callchain_list *chain;
 	int new_depth_mask = depth_mask;
 	u64 new_total;
+	u64 remaining;
 	size_t ret = 0;
 	int i;
 
@@ -908,17 +924,25 @@ callchain__fprintf_graph(FILE *fp, struct callchain_node *self,
 	else
 		new_total = total_samples;
 
+	remaining = new_total;
+
 	node = rb_first(&self->rb_root);
 	while (node) {
+		u64 cumul;
+
 		child = rb_entry(node, struct callchain_node, rb_node);
+		cumul = cumul_hits(child);
+		remaining -= cumul;
 
 		/*
 		 * The depth mask manages the output of pipes that show
 		 * the depth. We don't want to keep the pipes of the current
-		 * level for the last child of this depth
+		 * level for the last child of this depth.
+		 * Except if we have remaining filtered hits. They will
+		 * supersede the last child
 		 */
 		next = rb_next(node);
-		if (!next)
+		if (!next && (callchain_param.mode != CHAIN_GRAPH_REL || !remaining))
 			new_depth_mask &= ~(1 << (depth - 1));
 
 		/*
@@ -933,7 +957,7 @@ callchain__fprintf_graph(FILE *fp, struct callchain_node *self,
 			ret += ipchain__fprintf_graph(fp, chain, depth,
 						      new_depth_mask, i++,
 						      new_total,
-						      cumul_hits(child));
+						      cumul);
 		}
 		ret += callchain__fprintf_graph(fp, child, new_total,
 						depth + 1,
@@ -941,6 +965,19 @@ callchain__fprintf_graph(FILE *fp, struct callchain_node *self,
 		node = next;
 	}
 
+	if (callchain_param.mode == CHAIN_GRAPH_REL &&
+		remaining && remaining != new_total) {
+
+		if (!rem_sq_bracket)
+			return ret;
+
+		new_depth_mask &= ~(1 << (depth - 1));
+
+		ret += ipchain__fprintf_graph(fp, &rem_hits, depth,
+					      new_depth_mask, 0, new_total,
+					      remaining);
+	}
+
 	return ret;
 }
 
@@ -1361,6 +1398,8 @@ static size_t output__fprintf(FILE *fp, u64 total_samples)
 	unsigned int width;
 	char *col_width = col_width_list_str;
 
+	init_rem_hits();
+
 	fprintf(fp, "# Samples: %Ld\n", (u64)total_samples);
 	fprintf(fp, "#\n");
 
@@ -1432,6 +1471,8 @@ print_entries:
 	}
 	fprintf(fp, "\n");
 
+	free(rem_sq_bracket);
+
 	return ret;
 }
 
Signature · 1 line
1.6.2.3
#19 lore raw

PATCH 3/3 perfcounter: Align ftrace events raw samples to 8 bytes

In reply to Frederic Weisbecker

Align the ftrace events raw samples to 8 bytes so that they meet the perf output event alignements requirements.

Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Paul Mackerras <[email protected]>
---
 include/trace/ftrace.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/trace/ftrace.h +1 -1
index 80e5f6c..7b0834c 100644
@@ -687,7 +687,7 @@ static void ftrace_profile_##call(proto)				\
 	pc = preempt_count();						\
 									\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
-	__entry_size = __data_size + sizeof(*entry);			\
+	__entry_size = ALIGN(__data_size + sizeof(*entry), sizeof(u64));\
 									\
 	do {								\
 		char raw_data[__entry_size];				\
Signature · 1 line
1.6.2.3
#20 lore raw

[tip:perfcounters/core] perf_counter: Fix ftrace events raw samples to be aligned to 8 bytes

Commit-ID:  eba67b1fd06180d57b94bf97557d89873b0ce61e
Gitweb:     http://git.kernel.org/tip/eba67b1fd06180d57b94bf97557d89873b0ce61e
Author:     Frederic Weisbecker <[email protected]>
AuthorDate: Sat, 8 Aug 2009 04:26:39 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Sat, 8 Aug 2009 13:49:23 +0200

perf_counter: Fix ftrace events raw samples to be aligned to 8 bytes

Align the ftrace events raw samples to 8 bytes so that they meet the perf output event alignment requirements.

Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Paul Mackerras <[email protected]>

LKML-Reference: <[email protected]>

---
 include/trace/ftrace.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/trace/ftrace.h +1 -1
index bbf9042..7fb16d9 100644
@@ -685,7 +685,7 @@ static void ftrace_profile_##call(proto)				\
 	pc = preempt_count();						\
 									\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
-	__entry_size = __data_size + sizeof(*entry);			\
+	__entry_size = ALIGN(__data_size + sizeof(*entry), sizeof(u64));\
 									\
 	do {								\
 		char raw_data[__entry_size];				\
maintainer
#21 lore raw

Re: [PATCH 3/3] perfcounter: Align ftrace events raw samples to 8 bytes

In reply to Frederic Weisbecker
Quoted 18 lines · Frederic Weisbecker

Align the ftrace events raw samples to 8 bytes so that they meet the perf output event alignements requirements.

Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Paul Mackerras <[email protected]>
---
 include/trace/ftrace.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/trace/ftrace.h +0 -0
index 80e5f6c..7b0834c 100644

include/trace/ftrace.h static void ftrace_profile_##call(proto) \ · line 693

Quoted 8 lines
include/trace/ftrace.h +1 -1
@@ -687,7 +687,7 @@ static void ftrace_profile_##call(proto)				\
 	pc = preempt_count();						\
 									\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
-	__entry_size = __data_size + sizeof(*entry);			\
+	__entry_size = ALIGN(__data_size + sizeof(*entry), sizeof(u64));\
 									\
 	do {								\
 		char raw_data[__entry_size];				\

Make sure to clear the trailing few bytes, otherwise we've got a stack data leak here.

Also, tracepoints using __string() could still easily overflow the stack here and cause great havoc, esp since the tracepoint could already be deep into a call-chain.

#22 lore raw

Re: [PATCH 3/3] perfcounter: Align ftrace events raw samples to 8 bytes

Quoted 14 lines · Peter Zijlstra
Quoted 13 lines · Frederic Weisbecker

Align the ftrace events raw samples to 8 bytes so that they meet the perf output event alignements requirements.

Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Paul Mackerras <[email protected]>
---
 include/trace/ftrace.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Quoted 16 lines

include/trace/ftrace.h static void ftrace_profile_##call(proto) \ · line 693

Quoted 13 lines
include/trace/ftrace.h +1 -1
index 80e5f6c..7b0834c 100644
@@ -687,7 +687,7 @@ static void ftrace_profile_##call(proto)				\
 	pc = preempt_count();						\
 									\
 	__data_size = ftrace_get_offsets_##call(&__data_offsets, args); \
-	__entry_size = __data_size + sizeof(*entry);			\
+	__entry_size = ALIGN(__data_size + sizeof(*entry), sizeof(u64));\
 									\
 	do {								\
 		char raw_data[__entry_size];				\

Make sure to clear the trailing few bytes, otherwise we've got a stack data leak here.

Oh, right.

Quoted 3 lines

Also, tracepoints using __string() could still easily overflow the stack here and cause great havoc, esp since the tracepoint could already be deep into a call-chain.

I know yeah, I'll think about a way to store the string parts as pointers and copy their content on perf_output only.

#23 lore raw

[PATCH 7/3] perfcounter: Zeroe dead bytes from ftrace raw samples size alignment

In reply to Peter Zijlstra

After aligning the ftrace raw samples, there are dead bytes storing random data from the stack. We don't want to leak these to userspace, then zeroe these out.

Before:

	0x2de88 [0x50]: event: 9
	.
	. ... raw event: size 80 bytes
	.  0000:  09 00 00 00 01 00 50 00 d0 c7 00 81 ff ff ff ff  ......P........
	.  0010:  68 01 00 00 68 01 00 00 2c 00 00 00 00 00 00 00  h...h...,......
	.  0020:  2c 00 00 00 2b 00 01 02 68 01 00 00 68 01 00 00  ,...+...h...h..
	.  0030:  6b 6f 6e 64 65 6d 61 6e 64 2f 30 00 00 00 00 00  kondemand/0....
	.  0040:  68 01 00 00 40 7f 46 81 ff ff ff ff 00 10 1b 7f  [email protected]........
                                                      ^  ^  ^  ^
                                                         Leak

After:

	0x2d318 [0x50]: event: 9
	.
	. ... raw event: size 80 bytes
	.  0000:  09 00 00 00 01 00 50 00 d0 c7 00 81 ff ff ff ff  ......P........
	.  0010:  68 01 00 00 68 01 00 00 68 14 00 00 00 00 00 00  h...h...h......
	.  0020:  2c 00 00 00 2b 00 01 02 68 01 00 00 68 01 00 00  ,...+...h...h..
	.  0030:  6b 6f 6e 64 65 6d 61 6e 64 2f 30 00 00 00 00 00  kondemand/0....
	.  0040:  68 01 00 00 a0 80 46 81 ff ff ff ff 00 00 00 00  h.....F........
                                                      ^  ^  ^  ^
							 Fixed
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
---
 include/trace/ftrace.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
include/trace/ftrace.h +3 -0
index f98ff56..f2646e0 100644
@@ -650,6 +650,8 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
  *		char raw_data[__entry_size]; <- allocate our sample in the stack
  *		struct trace_entry *ent;
  *
+ *		// zeroe dead bytes from alignment to avoid stack leak to userspace
+ *		*(u64 *)(&raw_data[__entry_size - sizeof(u64)]) = 0ULL;
  *		entry = (struct ftrace_raw_<call> *)raw_data;
  *		ent = &entry->ent;
  *		tracing_generic_entry_update(ent, irq_flags, pc);
@@ -700,6 +702,7 @@ static void ftrace_profile_##call(proto)				\
 		char raw_data[__entry_size];				\
 		struct trace_entry *ent;				\
 									\
+		*(u64 *)(&raw_data[__entry_size - sizeof(u64)]) = 0ULL;	\
 		entry = (struct ftrace_raw_##call *)raw_data;		\
 		ent = &entry->ent;					\
 		tracing_generic_entry_update(ent, irq_flags, pc);	\
Signature · 1 line
1.6.2.3
#24 lore raw

[tip:perfcounters/urgent] perf_counter: Zero dead bytes from ftrace raw samples size alignment

Commit-ID:  1853db0e02ae4088f102b0d8e59e83dc98f93f03
Gitweb:     http://git.kernel.org/tip/1853db0e02ae4088f102b0d8e59e83dc98f93f03
Author:     Frederic Weisbecker <[email protected]>
AuthorDate: Mon, 10 Aug 2009 16:38:36 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 10 Aug 2009 16:51:19 +0200

perf_counter: Zero dead bytes from ftrace raw samples size alignment

After aligning the ftrace raw samples, there are dead bytes storing random data from the stack. We don't want to leak these to userspace, then zero these out.

Before:

	0x2de88 [0x50]: event: 9
	.
	. ... raw event: size 80 bytes
	.  0000:  09 00 00 00 01 00 50 00 d0 c7 00 81 ff ff ff ff  ......P........
	.  0010:  68 01 00 00 68 01 00 00 2c 00 00 00 00 00 00 00  h...h...,......
	.  0020:  2c 00 00 00 2b 00 01 02 68 01 00 00 68 01 00 00  ,...+...h...h..
	.  0030:  6b 6f 6e 64 65 6d 61 6e 64 2f 30 00 00 00 00 00  kondemand/0....
	.  0040:  68 01 00 00 40 7f 46 81 ff ff ff ff 00 10 1b 7f  [email protected]........
                                                      ^  ^  ^  ^
                                                         Leak

After:

	0x2d318 [0x50]: event: 9
	.
	. ... raw event: size 80 bytes
	.  0000:  09 00 00 00 01 00 50 00 d0 c7 00 81 ff ff ff ff  ......P........
	.  0010:  68 01 00 00 68 01 00 00 68 14 00 00 00 00 00 00  h...h...h......
	.  0020:  2c 00 00 00 2b 00 01 02 68 01 00 00 68 01 00 00  ,...+...h...h..
	.  0030:  6b 6f 6e 64 65 6d 61 6e 64 2f 30 00 00 00 00 00  kondemand/0....
	.  0040:  68 01 00 00 a0 80 46 81 ff ff ff ff 00 00 00 00  h.....F........
                                                      ^  ^  ^  ^
							 Fixed
Cc Frederic Weisbecker <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>

LKML-Reference: <[email protected]>

Cc Peter Zijlstra <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Mike Galbraith <[email protected]>
---
 include/trace/ftrace.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
include/trace/ftrace.h +4 -0
index a05524f..f64fbaa 100644
@@ -648,6 +648,9 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
  *		char raw_data[__entry_size]; <- allocate our sample in the stack
  *		struct trace_entry *ent;
  *
+ *		zero dead bytes from alignment to avoid stack leak to userspace:
+ *
+ *		*(u64 *)(&raw_data[__entry_size - sizeof(u64)]) = 0ULL;
  *		entry = (struct ftrace_raw_<call> *)raw_data;
  *		ent = &entry->ent;
  *		tracing_generic_entry_update(ent, irq_flags, pc);
@@ -698,6 +701,7 @@ static void ftrace_profile_##call(proto)				\
 		char raw_data[__entry_size];				\
 		struct trace_entry *ent;				\
 									\
+		*(u64 *)(&raw_data[__entry_size - sizeof(u64)]) = 0ULL;	\
 		entry = (struct ftrace_raw_##call *)raw_data;		\
 		ent = &entry->ent;					\
 		tracing_generic_entry_update(ent, irq_flags, pc);	\