[PATCH] perf: implement recording/reporting per-cpu samples

[PATCH] perf: implement recording/reporting per-cpu samples

[Arun Sharma]

Enable PERF_SAMPLE_CPU by default. Implement --sort cpu.

Signed-off-by: Arun Sharma <aruns@google.com>

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 27f992a..2ac9aaa 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -258,7 +258,8 @@ static void create_counter(int counter, int cpu)
 				  PERF_FORMAT_TOTAL_TIME_RUNNING |
 				  PERF_FORMAT_ID;
 
-	attr->sample_type	|= PERF_SAMPLE_IP | PERF_SAMPLE_TID;
+	attr->sample_type	|= PERF_SAMPLE_IP | PERF_SAMPLE_TID |
+				   PERF_SAMPLE_CPU;
 
 	if (nr_counters > 1)
 		attr->sample_type |= PERF_SAMPLE_ID;
@@ -293,7 +294,6 @@ static void create_counter(int counter, int cpu)
 	if (raw_samples) {
 		attr->sample_type	|= PERF_SAMPLE_TIME;
 		attr->sample_type	|= PERF_SAMPLE_RAW;
-		attr->sample_type	|= PERF_SAMPLE_CPU;
 	}
 
 	attr->mmap		= track;
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 816edae..6014d70 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -190,6 +190,13 @@ static int process_sample_event(event_t *event, struct perf_session *session)
 		return -1;
 	}
 
+	if (session->sample_type & PERF_SAMPLE_CPU) {
+		dump_printf("... cpu: %d\n", data.cpu);
+		al.cpu = data.cpu;
+	} else {
+		al.cpu = -1;
+	}
+
 	if (al.filtered || (hide_unresolved && al.sym == NULL))
 		return 0;
 
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index ad6b22d..4640015 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -52,6 +52,7 @@ struct hist_entry *__perf_session__add_hist_entry(struct rb_root *hists,
 		},
 		.ip	= al->addr,
 		.level	= al->level,
+		.cpu    = al->cpu,
 		.count	= count,
 		.parent = sym_parent,
 	};
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index da30b30..ee70bb8 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -13,6 +13,7 @@ enum sort_type	sort__first_dimension;
 unsigned int dsos__col_width;
 unsigned int comms__col_width;
 unsigned int threads__col_width;
+unsigned int cpu__col_width;
 static unsigned int parent_symbol__col_width;
 char * field_sep;
 
@@ -28,6 +29,8 @@ static int hist_entry__sym_snprintf(struct hist_entry *self, char *bf,
 				    size_t size, unsigned int width);
 static int hist_entry__parent_snprintf(struct hist_entry *self, char *bf,
 				       size_t size, unsigned int width);
+static int hist_entry__cpu_snprintf(struct hist_entry *self, char *bf,
+				    size_t size, unsigned int width);
 
 struct sort_entry sort_thread = {
 	.se_header	= "Command:  Pid",
@@ -64,6 +67,13 @@ struct sort_entry sort_parent = {
 	.se_width	= &parent_symbol__col_width,
 };
 
+struct sort_entry sort_cpu = {
+	.se_header      = "CPU",
+	.se_cmp	        = sort__cpu_cmp,
+	.se_snprintf    = hist_entry__cpu_snprintf,
+	.se_width	= &cpu__col_width,
+};
+
 struct sort_dimension {
 	const char		*name;
 	struct sort_entry	*entry;
@@ -76,6 +86,7 @@ static struct sort_dimension sort_dimensions[] = {
 	{ .name = "dso",	.entry = &sort_dso,	},
 	{ .name = "symbol",	.entry = &sort_sym,	},
 	{ .name = "parent",	.entry = &sort_parent,	},
+	{ .name = "cpu",	.entry = &sort_cpu,	},
 };
 
 int64_t cmp_null(void *l, void *r)
@@ -242,6 +253,20 @@ static int hist_entry__parent_snprintf(struct hist_entry *self, char *bf,
 			      self->parent ? self->parent->name : "[other]");
 }
 
+/* --sort cpu */
+
+int64_t
+sort__cpu_cmp(struct hist_entry *left, struct hist_entry *right)
+{
+	return right->cpu - left->cpu;
+}
+
+static int hist_entry__cpu_snprintf(struct hist_entry *self, char *bf,
+				       size_t size, unsigned int width)
+{
+	return repsep_snprintf(bf, size, "%-*d", width, self->cpu);
+}
+
 int sort_dimension__add(const char *tok)
 {
 	unsigned int i;
@@ -281,6 +306,8 @@ int sort_dimension__add(const char *tok)
 				sort__first_dimension = SORT_SYM;
 			else if (!strcmp(sd->name, "parent"))
 				sort__first_dimension = SORT_PARENT;
+			else if (!strcmp(sd->name, "cpu"))
+				sort__first_dimension = SORT_CPU;
 		}
 
 		list_add_tail(&sd->entry->list, &hist_entry__sort_list);
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index b7c54ee..82c5596 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -60,6 +60,7 @@ struct hist_entry {
 	char			level;
 	u8			filtered;
 	struct symbol		*parent;
+	s32                     cpu;
 	union {
 		unsigned long	  position;
 		struct hist_entry *pair;
@@ -73,7 +74,8 @@ enum sort_type {
 	SORT_COMM,
 	SORT_DSO,
 	SORT_SYM,
-	SORT_PARENT
+	SORT_PARENT,
+	SORT_CPU
 };
 
 /*
@@ -102,6 +104,8 @@ extern size_t sort__thread_print(FILE *, struct hist_entry *, unsigned int);
 extern size_t sort__comm_print(FILE *, struct hist_entry *, unsigned int);
 extern size_t sort__dso_print(FILE *, struct hist_entry *, unsigned int);
 extern size_t sort__sym_print(FILE *, struct hist_entry *, unsigned int __used);
+extern size_t sort__parent_print(FILE *, struct hist_entry *, unsigned int);
+extern size_t sort__cpu_print(FILE *, struct hist_entry *, unsigned int);
 extern int64_t cmp_null(void *, void *);
 extern int64_t sort__thread_cmp(struct hist_entry *, struct hist_entry *);
 extern int64_t sort__comm_cmp(struct hist_entry *, struct hist_entry *);
@@ -109,7 +113,7 @@ extern int64_t sort__comm_collapse(struct hist_entry *, struct hist_entry *);
 extern int64_t sort__dso_cmp(struct hist_entry *, struct hist_entry *);
 extern int64_t sort__sym_cmp(struct hist_entry *, struct hist_entry *);
 extern int64_t sort__parent_cmp(struct hist_entry *, struct hist_entry *);
-extern size_t sort__parent_print(FILE *, struct hist_entry *, unsigned int);
+extern int64_t sort__cpu_cmp(struct hist_entry *, struct hist_entry *);
 extern int sort_dimension__add(const char *);
 void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
 			     const char *list_name, FILE *fp);
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 478f5ab..4b3e09d 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -112,6 +112,7 @@ struct addr_location {
 	char	      level;
 	bool	      filtered;
 	unsigned int  cpumode;
+	s32           cpu;
 };
 
 enum dso_kernel_type {
-- 
1.7.0.1

Re: [PATCH] perf: implement recording/reporting per-cpu samples

[Peter Zijlstra]

On Mon, 2010-05-03 at 13:38 -0700, Arun Sharma wrote:
> Enable PERF_SAMPLE_CPU by default. Implement --sort cpu.

Why? The downside is that you unconditionally grow each sample and thus
increase the overhead for something that doesn't make sense for the
normal (task-inherit) case.

Re: [PATCH] perf: implement recording/reporting per-cpu samples

[Arun Sharma]

On Mon, May 3, 2010 at 1:42 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, 2010-05-03 at 13:38 -0700, Arun Sharma wrote:
> > Enable PERF_SAMPLE_CPU by default. Implement --sort cpu.
>
> Why? The downside is that you unconditionally grow each sample and thus
> increase the overhead for something that doesn't make sense for the
> normal (task-inherit) case.

In a shared multi-core environment, users want to analyze why their
program was slow. In particular, if the code ran slower only on
certain CPUs due to interference from other programs or kernel
threads, they want to know that.

But that's just our use case. The patch is mostly about --sort cpu
option. If you want to drop the part that enables PERF_SAMPLE_CPU by
default, that's fine by me.

 -Arun

Re: [PATCH] perf: implement recording/reporting per-cpu samples

[Peter Zijlstra]

On Mon, 2010-05-03 at 13:53 -0700, Arun Sharma wrote:
> On Mon, May 3, 2010 at 1:42 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, 2010-05-03 at 13:38 -0700, Arun Sharma wrote:
> > > Enable PERF_SAMPLE_CPU by default. Implement --sort cpu.
> >
> > Why? The downside is that you unconditionally grow each sample and thus
> > increase the overhead for something that doesn't make sense for the
> > normal (task-inherit) case.
> 
> In a shared multi-core environment, users want to analyze why their
> program was slow. In particular, if the code ran slower only on
> certain CPUs due to interference from other programs or kernel
> threads, they want to know that.

But for that you use perf record -a, right? So you record all cpus
allways -- otherwise there is no telling what was happening to make it
go slow.

> But that's just our use case. The patch is mostly about --sort cpu
> option. If you want to drop the part that enables PERF_SAMPLE_CPU by
> default, that's fine by me.

Right, it would be very nice if we can avoid growing the default sample
size. Also, your changelog needs work, please explain the full usecase
that goes with this feature.

Explain the thing you're wanting to measure, explain the implementation
and maybe give a short example.

Re: [PATCH] perf: implement recording/reporting per-cpu samples

[Arun Sharma]

On Tue, May 04, 2010 at 11:16:38AM +0200, Peter Zijlstra wrote:
> > In a shared multi-core environment, users want to analyze why their
> > program was slow. In particular, if the code ran slower only on
> > certain CPUs due to interference from other programs or kernel
> > threads, they want to know that.
> 
> But for that you use perf record -a, right? So you record all cpus
> allways -- otherwise there is no telling what was happening to make it
> go slow.

The updated patch records the CPU only in the system_wide mode.

> 
> > But that's just our use case. The patch is mostly about --sort cpu
> > option. If you want to drop the part that enables PERF_SAMPLE_CPU by
> > default, that's fine by me.
> 
> Right, it would be very nice if we can avoid growing the default sample
> size. Also, your changelog needs work, please explain the full usecase
> that goes with this feature.
> 
> Explain the thing you're wanting to measure, explain the implementation
> and maybe give a short example.

Updated changelog as well.

 -Arun

>From 7ae0f1cd7d6ab0d74ab3f8a8a31b11d1cd416f36 Mon Sep 17 00:00:00 2001
From: Arun Sharma <aruns@google.com>
Date: Mon, 3 May 2010 11:45:41 -0700
Subject: [PATCH] Implement --sort cpu

In a shared multi-core environment, users want to analyze why their
program was slow. In particular, if the code ran slower only on
certain CPUs due to interference from other programs or kernel
threads, the user should be able to notice that.

Sample usage:

perf record -f -a -- sleep 3
perf report --sort cpu,comm

Workload:

program is running on 16 CPUs
Experiencing interference from an antagonist only on 4 CPUs.

  Samples: 106218177676 cycles

  Overhead  CPU          Command
  ........  ...  ...............

     6.25%  2            program
     6.24%  6            program
     6.24%  11           program
     6.24%  5            program
     6.24%  9            program
     6.24%  10           program
     6.23%  15           program
     6.23%  7            program
     6.23%  3            program
     6.23%  14           program
     6.22%  1            program
     6.20%  13           program
     3.17%  12           program
     3.15%  8            program
     3.14%  0            program
     3.13%  4            program
     3.11%  4         antagonist
     3.11%  0         antagonist
     3.10%  8         antagonist
     3.07%  12        antagonist

Signed-off-by: Arun Sharma <aruns@google.com>
---
 tools/perf/builtin-record.c |    3 +++
 tools/perf/builtin-report.c |    7 +++++++
 tools/perf/util/hist.c      |    1 +
 tools/perf/util/sort.c      |   27 +++++++++++++++++++++++++++
 tools/perf/util/sort.h      |    8 ++++++--
 tools/perf/util/symbol.h    |    1 +
 6 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 27f992a..66867b8 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -290,6 +290,9 @@ static void create_counter(int counter, int cpu)
 	if (call_graph)
 		attr->sample_type	|= PERF_SAMPLE_CALLCHAIN;
 
+	if (system_wide)
+		attr->sample_type	|= PERF_SAMPLE_CPU;
+
 	if (raw_samples) {
 		attr->sample_type	|= PERF_SAMPLE_TIME;
 		attr->sample_type	|= PERF_SAMPLE_RAW;
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 816edae..6014d70 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -190,6 +190,13 @@ static int process_sample_event(event_t *event, struct perf_session *session)
 		return -1;
 	}
 
+	if (session->sample_type & PERF_SAMPLE_CPU) {
+		dump_printf("... cpu: %d\n", data.cpu);
+		al.cpu = data.cpu;
+	} else {
+		al.cpu = -1;
+	}
+
 	if (al.filtered || (hide_unresolved && al.sym == NULL))
 		return 0;
 
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index ad6b22d..4640015 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -52,6 +52,7 @@ struct hist_entry *__perf_session__add_hist_entry(struct rb_root *hists,
 		},
 		.ip	= al->addr,
 		.level	= al->level,
+		.cpu    = al->cpu,
 		.count	= count,
 		.parent = sym_parent,
 	};
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index da30b30..ee70bb8 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -13,6 +13,7 @@ enum sort_type	sort__first_dimension;
 unsigned int dsos__col_width;
 unsigned int comms__col_width;
 unsigned int threads__col_width;
+unsigned int cpu__col_width;
 static unsigned int parent_symbol__col_width;
 char * field_sep;
 
@@ -28,6 +29,8 @@ static int hist_entry__sym_snprintf(struct hist_entry *self, char *bf,
 				    size_t size, unsigned int width);
 static int hist_entry__parent_snprintf(struct hist_entry *self, char *bf,
 				       size_t size, unsigned int width);
+static int hist_entry__cpu_snprintf(struct hist_entry *self, char *bf,
+				    size_t size, unsigned int width);
 
 struct sort_entry sort_thread = {
 	.se_header	= "Command:  Pid",
@@ -64,6 +67,13 @@ struct sort_entry sort_parent = {
 	.se_width	= &parent_symbol__col_width,
 };
 
+struct sort_entry sort_cpu = {
+	.se_header      = "CPU",
+	.se_cmp	        = sort__cpu_cmp,
+	.se_snprintf    = hist_entry__cpu_snprintf,
+	.se_width	= &cpu__col_width,
+};
+
 struct sort_dimension {
 	const char		*name;
 	struct sort_entry	*entry;
@@ -76,6 +86,7 @@ static struct sort_dimension sort_dimensions[] = {
 	{ .name = "dso",	.entry = &sort_dso,	},
 	{ .name = "symbol",	.entry = &sort_sym,	},
 	{ .name = "parent",	.entry = &sort_parent,	},
+	{ .name = "cpu",	.entry = &sort_cpu,	},
 };
 
 int64_t cmp_null(void *l, void *r)
@@ -242,6 +253,20 @@ static int hist_entry__parent_snprintf(struct hist_entry *self, char *bf,
 			      self->parent ? self->parent->name : "[other]");
 }
 
+/* --sort cpu */
+
+int64_t
+sort__cpu_cmp(struct hist_entry *left, struct hist_entry *right)
+{
+	return right->cpu - left->cpu;
+}
+
+static int hist_entry__cpu_snprintf(struct hist_entry *self, char *bf,
+				       size_t size, unsigned int width)
+{
+	return repsep_snprintf(bf, size, "%-*d", width, self->cpu);
+}
+
 int sort_dimension__add(const char *tok)
 {
 	unsigned int i;
@@ -281,6 +306,8 @@ int sort_dimension__add(const char *tok)
 				sort__first_dimension = SORT_SYM;
 			else if (!strcmp(sd->name, "parent"))
 				sort__first_dimension = SORT_PARENT;
+			else if (!strcmp(sd->name, "cpu"))
+				sort__first_dimension = SORT_CPU;
 		}
 
 		list_add_tail(&sd->entry->list, &hist_entry__sort_list);
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index b7c54ee..82c5596 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -60,6 +60,7 @@ struct hist_entry {
 	char			level;
 	u8			filtered;
 	struct symbol		*parent;
+	s32                     cpu;
 	union {
 		unsigned long	  position;
 		struct hist_entry *pair;
@@ -73,7 +74,8 @@ enum sort_type {
 	SORT_COMM,
 	SORT_DSO,
 	SORT_SYM,
-	SORT_PARENT
+	SORT_PARENT,
+	SORT_CPU
 };
 
 /*
@@ -102,6 +104,8 @@ extern size_t sort__thread_print(FILE *, struct hist_entry *, unsigned int);
 extern size_t sort__comm_print(FILE *, struct hist_entry *, unsigned int);
 extern size_t sort__dso_print(FILE *, struct hist_entry *, unsigned int);
 extern size_t sort__sym_print(FILE *, struct hist_entry *, unsigned int __used);
+extern size_t sort__parent_print(FILE *, struct hist_entry *, unsigned int);
+extern size_t sort__cpu_print(FILE *, struct hist_entry *, unsigned int);
 extern int64_t cmp_null(void *, void *);
 extern int64_t sort__thread_cmp(struct hist_entry *, struct hist_entry *);
 extern int64_t sort__comm_cmp(struct hist_entry *, struct hist_entry *);
@@ -109,7 +113,7 @@ extern int64_t sort__comm_collapse(struct hist_entry *, struct hist_entry *);
 extern int64_t sort__dso_cmp(struct hist_entry *, struct hist_entry *);
 extern int64_t sort__sym_cmp(struct hist_entry *, struct hist_entry *);
 extern int64_t sort__parent_cmp(struct hist_entry *, struct hist_entry *);
-extern size_t sort__parent_print(FILE *, struct hist_entry *, unsigned int);
+extern int64_t sort__cpu_cmp(struct hist_entry *, struct hist_entry *);
 extern int sort_dimension__add(const char *);
 void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
 			     const char *list_name, FILE *fp);
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 478f5ab..4b3e09d 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -112,6 +112,7 @@ struct addr_location {
 	char	      level;
 	bool	      filtered;
 	unsigned int  cpumode;
+	s32           cpu;
 };
 
 enum dso_kernel_type {
-- 
1.7.0.1

Re: [PATCH] perf: implement recording/reporting per-cpu samples

[Peter Zijlstra]

Arnaldo, could you pick this up?

On Wed, 2010-05-05 at 11:16 -0700, Arun Sharma wrote:

> From 7ae0f1cd7d6ab0d74ab3f8a8a31b11d1cd416f36 Mon Sep 17 00:00:00 2001
> From: Arun Sharma <aruns@google.com>
> Date: Mon, 3 May 2010 11:45:41 -0700
> Subject: [PATCH] Implement --sort cpu
> 
> In a shared multi-core environment, users want to analyze why their
> program was slow. In particular, if the code ran slower only on
> certain CPUs due to interference from other programs or kernel
> threads, the user should be able to notice that.
> 
> Sample usage:
> 
> perf record -f -a -- sleep 3
> perf report --sort cpu,comm
> 
> Workload:
> 
> program is running on 16 CPUs
> Experiencing interference from an antagonist only on 4 CPUs.
> 
>   Samples: 106218177676 cycles
> 
>   Overhead  CPU          Command
>   ........  ...  ...............
> 
>      6.25%  2            program
>      6.24%  6            program
>      6.24%  11           program
>      6.24%  5            program
>      6.24%  9            program
>      6.24%  10           program
>      6.23%  15           program
>      6.23%  7            program
>      6.23%  3            program
>      6.23%  14           program
>      6.22%  1            program
>      6.20%  13           program
>      3.17%  12           program
>      3.15%  8            program
>      3.14%  0            program
>      3.13%  4            program
>      3.11%  4         antagonist
>      3.11%  0         antagonist
>      3.10%  8         antagonist
>      3.07%  12        antagonist
> 
> Signed-off-by: Arun Sharma <aruns@google.com>
> ---
>  tools/perf/builtin-record.c |    3 +++
>  tools/perf/builtin-report.c |    7 +++++++
>  tools/perf/util/hist.c      |    1 +
>  tools/perf/util/sort.c      |   27 +++++++++++++++++++++++++++
>  tools/perf/util/sort.h      |    8 ++++++--
>  tools/perf/util/symbol.h    |    1 +
>  6 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 27f992a..66867b8 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -290,6 +290,9 @@ static void create_counter(int counter, int cpu)
>  	if (call_graph)
>  		attr->sample_type	|= PERF_SAMPLE_CALLCHAIN;
>  
> +	if (system_wide)
> +		attr->sample_type	|= PERF_SAMPLE_CPU;
> +
>  	if (raw_samples) {
>  		attr->sample_type	|= PERF_SAMPLE_TIME;
>  		attr->sample_type	|= PERF_SAMPLE_RAW;
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 816edae..6014d70 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -190,6 +190,13 @@ static int process_sample_event(event_t *event, struct perf_session *session)
>  		return -1;
>  	}
>  
> +	if (session->sample_type & PERF_SAMPLE_CPU) {
> +		dump_printf("... cpu: %d\n", data.cpu);
> +		al.cpu = data.cpu;
> +	} else {
> +		al.cpu = -1;
> +	}
> +
>  	if (al.filtered || (hide_unresolved && al.sym == NULL))
>  		return 0;
>  
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index ad6b22d..4640015 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -52,6 +52,7 @@ struct hist_entry *__perf_session__add_hist_entry(struct rb_root *hists,
>  		},
>  		.ip	= al->addr,
>  		.level	= al->level,
> +		.cpu    = al->cpu,
>  		.count	= count,
>  		.parent = sym_parent,
>  	};
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index da30b30..ee70bb8 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -13,6 +13,7 @@ enum sort_type	sort__first_dimension;
>  unsigned int dsos__col_width;
>  unsigned int comms__col_width;
>  unsigned int threads__col_width;
> +unsigned int cpu__col_width;
>  static unsigned int parent_symbol__col_width;
>  char * field_sep;
>  
> @@ -28,6 +29,8 @@ static int hist_entry__sym_snprintf(struct hist_entry *self, char *bf,
>  				    size_t size, unsigned int width);
>  static int hist_entry__parent_snprintf(struct hist_entry *self, char *bf,
>  				       size_t size, unsigned int width);
> +static int hist_entry__cpu_snprintf(struct hist_entry *self, char *bf,
> +				    size_t size, unsigned int width);
>  
>  struct sort_entry sort_thread = {
>  	.se_header	= "Command:  Pid",
> @@ -64,6 +67,13 @@ struct sort_entry sort_parent = {
>  	.se_width	= &parent_symbol__col_width,
>  };
>  
> +struct sort_entry sort_cpu = {
> +	.se_header      = "CPU",
> +	.se_cmp	        = sort__cpu_cmp,
> +	.se_snprintf    = hist_entry__cpu_snprintf,
> +	.se_width	= &cpu__col_width,
> +};
> +
>  struct sort_dimension {
>  	const char		*name;
>  	struct sort_entry	*entry;
> @@ -76,6 +86,7 @@ static struct sort_dimension sort_dimensions[] = {
>  	{ .name = "dso",	.entry = &sort_dso,	},
>  	{ .name = "symbol",	.entry = &sort_sym,	},
>  	{ .name = "parent",	.entry = &sort_parent,	},
> +	{ .name = "cpu",	.entry = &sort_cpu,	},
>  };
>  
>  int64_t cmp_null(void *l, void *r)
> @@ -242,6 +253,20 @@ static int hist_entry__parent_snprintf(struct hist_entry *self, char *bf,
>  			      self->parent ? self->parent->name : "[other]");
>  }
>  
> +/* --sort cpu */
> +
> +int64_t
> +sort__cpu_cmp(struct hist_entry *left, struct hist_entry *right)
> +{
> +	return right->cpu - left->cpu;
> +}
> +
> +static int hist_entry__cpu_snprintf(struct hist_entry *self, char *bf,
> +				       size_t size, unsigned int width)
> +{
> +	return repsep_snprintf(bf, size, "%-*d", width, self->cpu);
> +}
> +
>  int sort_dimension__add(const char *tok)
>  {
>  	unsigned int i;
> @@ -281,6 +306,8 @@ int sort_dimension__add(const char *tok)
>  				sort__first_dimension = SORT_SYM;
>  			else if (!strcmp(sd->name, "parent"))
>  				sort__first_dimension = SORT_PARENT;
> +			else if (!strcmp(sd->name, "cpu"))
> +				sort__first_dimension = SORT_CPU;
>  		}
>  
>  		list_add_tail(&sd->entry->list, &hist_entry__sort_list);
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index b7c54ee..82c5596 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -60,6 +60,7 @@ struct hist_entry {
>  	char			level;
>  	u8			filtered;
>  	struct symbol		*parent;
> +	s32                     cpu;
>  	union {
>  		unsigned long	  position;
>  		struct hist_entry *pair;
> @@ -73,7 +74,8 @@ enum sort_type {
>  	SORT_COMM,
>  	SORT_DSO,
>  	SORT_SYM,
> -	SORT_PARENT
> +	SORT_PARENT,
> +	SORT_CPU
>  };
>  
>  /*
> @@ -102,6 +104,8 @@ extern size_t sort__thread_print(FILE *, struct hist_entry *, unsigned int);
>  extern size_t sort__comm_print(FILE *, struct hist_entry *, unsigned int);
>  extern size_t sort__dso_print(FILE *, struct hist_entry *, unsigned int);
>  extern size_t sort__sym_print(FILE *, struct hist_entry *, unsigned int __used);
> +extern size_t sort__parent_print(FILE *, struct hist_entry *, unsigned int);
> +extern size_t sort__cpu_print(FILE *, struct hist_entry *, unsigned int);
>  extern int64_t cmp_null(void *, void *);
>  extern int64_t sort__thread_cmp(struct hist_entry *, struct hist_entry *);
>  extern int64_t sort__comm_cmp(struct hist_entry *, struct hist_entry *);
> @@ -109,7 +113,7 @@ extern int64_t sort__comm_collapse(struct hist_entry *, struct hist_entry *);
>  extern int64_t sort__dso_cmp(struct hist_entry *, struct hist_entry *);
>  extern int64_t sort__sym_cmp(struct hist_entry *, struct hist_entry *);
>  extern int64_t sort__parent_cmp(struct hist_entry *, struct hist_entry *);
> -extern size_t sort__parent_print(FILE *, struct hist_entry *, unsigned int);
> +extern int64_t sort__cpu_cmp(struct hist_entry *, struct hist_entry *);
>  extern int sort_dimension__add(const char *);
>  void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
>  			     const char *list_name, FILE *fp);
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 478f5ab..4b3e09d 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -112,6 +112,7 @@ struct addr_location {
>  	char	      level;
>  	bool	      filtered;
>  	unsigned int  cpumode;
> +	s32           cpu;
>  };
>  
>  enum dso_kernel_type {

Re: [PATCH] perf: implement recording/reporting per-cpu samples

[Arnaldo Carvalho de Melo]

Em Thu, May 27, 2010 at 08:08:43PM +0200, Peter Zijlstra escreveu:
> Arnaldo, could you pick this up?

I'll review and merge it, thanks,
 
- Arnaldo

Re: [PATCH] perf: implement recording/reporting per-cpu samples

[Arnaldo Carvalho de Melo]

Em Wed, May 05, 2010 at 11:16:12AM -0700, Arun Sharma escreveu:
> On Tue, May 04, 2010 at 11:16:38AM +0200, Peter Zijlstra wrote:
> > > In a shared multi-core environment, users want to analyze why their
> > > program was slow. In particular, if the code ran slower only on
> > > certain CPUs due to interference from other programs or kernel
> > > threads, they want to know that.
> > 
> > But for that you use perf record -a, right? So you record all cpus
> > allways -- otherwise there is no telling what was happening to make it
> > go slow.
> 
> The updated patch records the CPU only in the system_wide mode.

I think this should be done only if you'll actually need it, as in,
"cpu" is one of the sort keys, but that can be done as a followup patch,
but there is another thing I think you need to change, see below.

> > > But that's just our use case. The patch is mostly about --sort cpu
> > > option. If you want to drop the part that enables PERF_SAMPLE_CPU by
> > > default, that's fine by me.
> > 
> > Right, it would be very nice if we can avoid growing the default sample
> > size. Also, your changelog needs work, please explain the full usecase
> > that goes with this feature.
> > 
> > Explain the thing you're wanting to measure, explain the implementation
> > and maybe give a short example.
> 
> Updated changelog as well.
> 
>  -Arun
> 

<SNIP>

> +	if (session->sample_type & PERF_SAMPLE_CPU) {
> +		dump_printf("... cpu: %d\n", data.cpu);
> +		al.cpu = data.cpu;
> +	} else {
> +		al.cpu = -1;
> +	}
> +

This should be in event__parse_sample() that will set a field in
sample_data, just like the others optional fields.

I'll cook up a patch with these changes and post here for your review.

- Arnaldo

Re: [PATCH] perf: implement recording/reporting per-cpu samples

[Arun Sharma]

On Thu, May 27, 2010 at 11:41 AM, Arnaldo Carvalho de Melo
<acme@ghostprotocols.net> wrote:
> Em Wed, May 05, 2010 at 11:16:12AM -0700, Arun Sharma escreveu:
>> On Tue, May 04, 2010 at 11:16:38AM +0200, Peter Zijlstra wrote:
>> > > In a shared multi-core environment, users want to analyze why their
>> > > program was slow. In particular, if the code ran slower only on
>> > > certain CPUs due to interference from other programs or kernel
>> > > threads, they want to know that.
>> >
>> > But for that you use perf record -a, right? So you record all cpus
>> > allways -- otherwise there is no telling what was happening to make it
>> > go slow.
>>
>> The updated patch records the CPU only in the system_wide mode.
>
> I think this should be done only if you'll actually need it, as in,
> "cpu" is one of the sort keys, but that can be done as a followup patch,
> but there is another thing I think you need to change, see below.
>

How would you know if the user is going to sort by cpu at "perf record" time?

Thanks for taking care of the second part.

 -Arun

Re: [PATCH] perf: implement recording/reporting per-cpu samples

[Arnaldo Carvalho de Melo]

Em Thu, May 27, 2010 at 01:54:46PM -0700, Arun Sharma escreveu:
> On Thu, May 27, 2010 at 11:41 AM, Arnaldo Carvalho de Melo
> <acme@ghostprotocols.net> wrote:
> > Em Wed, May 05, 2010 at 11:16:12AM -0700, Arun Sharma escreveu:
> >> On Tue, May 04, 2010 at 11:16:38AM +0200, Peter Zijlstra wrote:
> >> > > In a shared multi-core environment, users want to analyze why their
> >> > > program was slow. In particular, if the code ran slower only on
> >> > > certain CPUs due to interference from other programs or kernel
> >> > > threads, they want to know that.

> >> > But for that you use perf record -a, right? So you record all cpus
> >> > allways -- otherwise there is no telling what was happening to make it
> >> > go slow.

> >> The updated patch records the CPU only in the system_wide mode.

> > I think this should be done only if you'll actually need it, as in,
> > "cpu" is one of the sort keys, but that can be done as a followup patch,
> > but there is another thing I think you need to change, see below.

> How would you know if the user is going to sort by cpu at "perf record" time?

Excellent point, but as time goes on we may end up selecting all the
optionally selectable fields, so perhaps we should tell that to record
and then check at report time if it is present?

For instance, PERF_SAMPLE_TIME would be interesting too to check if
there is no reordering of events, etc, but should we have it always
enabled?

If we used something like:

perf record --sort cpu,comm ...

We would be able for instance, to avoid having MMAP events that wouldn't
be used at all, reducing PERF_SAMPLE_TID too, I guess, and then the per
event cost would be reduced, on the other hand, if we want to have
maximum flexibility at 'report' time, we could use:

perf record --sort all

With the default remaining the one we have.

perf record --sort +cpu

could be used to add one field to the set of fields in place, whatever
we get the default to be at any point in time.

perf record could as well, if no --sort is presented, infer a reasonable
one from the set of fields present in sample_type, etc.

Of course the feature implemented as-is by your patch is useful and we
need to support it, it can even be like you posted, but I wanted to
express this feeling about per event cost.
 
> Thanks for taking care of the second part.

Will try to get it done now and will send for review.

- Arnaldo

Re: [PATCH] perf: implement recording/reporting per-cpu samples

[Arnaldo Carvalho de Melo]

Em Thu, May 27, 2010 at 06:53:33PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, May 27, 2010 at 01:54:46PM -0700, Arun Sharma escreveu:
> > Thanks for taking care of the second part.
> 
> Will try to get it done now and will send for review.

event__preprocess_sample should eventually digest event__parse_sample,
I guess, goal being to separate out what is common to evergrowing set of
perf commands.

Can you see any holes in this one?

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 96db524..9542295 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -65,7 +65,7 @@ static int process_sample_event(event_t *event, struct perf_session *session)
 	dump_printf("(IP, %d): %d: %#Lx\n", event->header.misc,
 		    event->ip.pid, event->ip.ip);
 
-	if (event__preprocess_sample(event, session, &al, NULL) < 0) {
+	if (event__preprocess_sample(event, NULL, session, &al, NULL) < 0) {
 		pr_warning("problem processing %d event, skipping it.\n",
 			   event->header.type);
 		return -1;
diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index a6e2fdc..1608629 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -38,7 +38,9 @@ static int diff__process_sample_event(event_t *event, struct perf_session *sessi
 	dump_printf("(IP, %d): %d: %#Lx\n", event->header.misc,
 		    event->ip.pid, event->ip.ip);
 
-	if (event__preprocess_sample(event, session, &al, NULL) < 0) {
+	event__parse_sample(event, session->sample_type, &data);
+
+	if (event__preprocess_sample(event, &data, session, &al, NULL) < 0) {
 		pr_warning("problem processing %d event, skipping it.\n",
 			   event->header.type);
 		return -1;
@@ -47,8 +49,6 @@ static int diff__process_sample_event(event_t *event, struct perf_session *sessi
 	if (al.filtered || al.sym == NULL)
 		return 0;
 
-	event__parse_sample(event, session->sample_type, &data);
-
 	if (hists__add_entry(&session->hists, &al, data.period)) {
 		pr_warning("problem incrementing symbol period, skipping event\n");
 		return -1;
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 9bc8905..73f8122 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -274,6 +274,9 @@ static void create_counter(int counter, int cpu)
 	if (call_graph)
 		attr->sample_type	|= PERF_SAMPLE_CALLCHAIN;
 
+	if (system_wide)
+		attr->sample_type	|= PERF_SAMPLE_CPU;
+
 	if (raw_samples) {
 		attr->sample_type	|= PERF_SAMPLE_TIME;
 		attr->sample_type	|= PERF_SAMPLE_RAW;
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 3592057..bb0859f 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -157,8 +157,8 @@ static int process_sample_event(event_t *event, struct perf_session *session)
 
 	event__parse_sample(event, session->sample_type, &data);
 
-	dump_printf("(IP, %d): %d/%d: %#Lx period: %Ld\n", event->header.misc,
-		    data.pid, data.tid, data.ip, data.period);
+	dump_printf("(IP, %d): %d %d/%d: %#Lx period: %Ld\n", event->header.misc,
+		    data.cpu, data.pid, data.tid, data.ip, data.period);
 
 	if (session->sample_type & PERF_SAMPLE_CALLCHAIN) {
 		unsigned int i;
@@ -178,7 +178,7 @@ static int process_sample_event(event_t *event, struct perf_session *session)
 		}
 	}
 
-	if (event__preprocess_sample(event, session, &al, NULL) < 0) {
+	if (event__preprocess_sample(event, &data, session, &al, NULL) < 0) {
 		fprintf(stderr, "problem processing %d event, skipping it.\n",
 			event->header.type);
 		return -1;
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index a66f427..fc9849a 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1024,7 +1024,7 @@ static void event__process_sample(const event_t *self,
 	if (self->header.misc & PERF_RECORD_MISC_EXACT_IP)
 		exact_samples++;
 
-	if (event__preprocess_sample(self, session, &al, symbol_filter) < 0 ||
+	if (event__preprocess_sample(self, NULL, session, &al, symbol_filter) < 0 ||
 	    al.filtered)
 		return;
 
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 50771b5..cd8bf65 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -659,7 +659,8 @@ static void dso__calc_col_width(struct dso *self)
 	self->slen_calculated = 1;
 }
 
-int event__preprocess_sample(const event_t *self, struct perf_session *session,
+int event__preprocess_sample(const event_t *self, const struct sample_data *data,
+			     struct perf_session *session,
 			     struct addr_location *al, symbol_filter_t filter)
 {
 	u8 cpumode = self->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
@@ -672,6 +673,9 @@ int event__preprocess_sample(const event_t *self, struct perf_session *session,
 	    !strlist__has_entry(symbol_conf.comm_list, thread->comm))
 		goto out_filtered;
 
+	if (data != NULL)
+		al->cpu = data->cpu;
+
 	dump_printf(" ... thread: %s:%d\n", thread->comm, thread->pid);
 	/*
 	 * Have we already created the kernel maps for the host machine?
diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index 8577085..baedf02 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -156,7 +156,8 @@ int event__process_mmap(event_t *self, struct perf_session *session);
 int event__process_task(event_t *self, struct perf_session *session);
 
 struct addr_location;
-int event__preprocess_sample(const event_t *self, struct perf_session *session,
+int event__preprocess_sample(const event_t *self, const struct sample_data *data,
+			     struct perf_session *session,
 			     struct addr_location *al, symbol_filter_t filter);
 int event__parse_sample(event_t *event, u64 type, struct sample_data *data);
 
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 2316cb5..4f8e0bf 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -13,6 +13,7 @@ enum sort_type	sort__first_dimension;
 unsigned int dsos__col_width;
 unsigned int comms__col_width;
 unsigned int threads__col_width;
+unsigned int cpu__col_width;
 static unsigned int parent_symbol__col_width;
 char * field_sep;
 
@@ -28,6 +29,8 @@ static int hist_entry__sym_snprintf(struct hist_entry *self, char *bf,
 				    size_t size, unsigned int width);
 static int hist_entry__parent_snprintf(struct hist_entry *self, char *bf,
 				       size_t size, unsigned int width);
+static int hist_entry__cpu_snprintf(struct hist_entry *self, char *bf,
+				    size_t size, unsigned int width);
 
 struct sort_entry sort_thread = {
 	.se_header	= "Command:  Pid",
@@ -64,6 +67,13 @@ struct sort_entry sort_parent = {
 	.se_width	= &parent_symbol__col_width,
 };
 
+struct sort_entry sort_cpu = {
+	.se_header      = "CPU",
+	.se_cmp	        = sort__cpu_cmp,
+	.se_snprintf    = hist_entry__cpu_snprintf,
+	.se_width	= &cpu__col_width,
+};
+
 struct sort_dimension {
 	const char		*name;
 	struct sort_entry	*entry;
@@ -76,6 +86,7 @@ static struct sort_dimension sort_dimensions[] = {
 	{ .name = "dso",	.entry = &sort_dso,	},
 	{ .name = "symbol",	.entry = &sort_sym,	},
 	{ .name = "parent",	.entry = &sort_parent,	},
+	{ .name = "cpu",	.entry = &sort_cpu,	},
 };
 
 int64_t cmp_null(void *l, void *r)
@@ -242,6 +253,20 @@ static int hist_entry__parent_snprintf(struct hist_entry *self, char *bf,
 			      self->parent ? self->parent->name : "[other]");
 }
 
+/* --sort cpu */
+
+int64_t
+sort__cpu_cmp(struct hist_entry *left, struct hist_entry *right)
+{
+	return right->cpu - left->cpu;
+}
+
+static int hist_entry__cpu_snprintf(struct hist_entry *self, char *bf,
+				       size_t size, unsigned int width)
+{
+	return repsep_snprintf(bf, size, "%-*d", width, self->cpu);
+}
+
 int sort_dimension__add(const char *tok)
 {
 	unsigned int i;
@@ -281,6 +306,8 @@ int sort_dimension__add(const char *tok)
 				sort__first_dimension = SORT_SYM;
 			else if (!strcmp(sd->name, "parent"))
 				sort__first_dimension = SORT_PARENT;
+			else if (!strcmp(sd->name, "cpu"))
+				sort__first_dimension = SORT_CPU;
 		}
 
 		list_add_tail(&sd->entry->list, &hist_entry__sort_list);
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 0d61c40..dcc6f42 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -55,6 +55,7 @@ struct hist_entry {
 	char			level;
 	u8			filtered;
 	struct symbol		*parent;
+	s32                     cpu;
 	union {
 		unsigned long	  position;
 		struct hist_entry *pair;
@@ -68,7 +69,8 @@ enum sort_type {
 	SORT_COMM,
 	SORT_DSO,
 	SORT_SYM,
-	SORT_PARENT
+	SORT_PARENT,
+	SORT_CPU
 };
 
 /*
@@ -97,6 +99,8 @@ extern size_t sort__thread_print(FILE *, struct hist_entry *, unsigned int);
 extern size_t sort__comm_print(FILE *, struct hist_entry *, unsigned int);
 extern size_t sort__dso_print(FILE *, struct hist_entry *, unsigned int);
 extern size_t sort__sym_print(FILE *, struct hist_entry *, unsigned int __used);
+extern size_t sort__parent_print(FILE *, struct hist_entry *, unsigned int);
+extern size_t sort__cpu_print(FILE *, struct hist_entry *, unsigned int);
 extern int64_t cmp_null(void *, void *);
 extern int64_t sort__thread_cmp(struct hist_entry *, struct hist_entry *);
 extern int64_t sort__comm_cmp(struct hist_entry *, struct hist_entry *);
@@ -104,7 +108,7 @@ extern int64_t sort__comm_collapse(struct hist_entry *, struct hist_entry *);
 extern int64_t sort__dso_cmp(struct hist_entry *, struct hist_entry *);
 extern int64_t sort__sym_cmp(struct hist_entry *, struct hist_entry *);
 extern int64_t sort__parent_cmp(struct hist_entry *, struct hist_entry *);
-extern size_t sort__parent_print(FILE *, struct hist_entry *, unsigned int);
+extern int64_t sort__cpu_cmp(struct hist_entry *, struct hist_entry *);
 extern int sort_dimension__add(const char *);
 void sort_entry__setup_elide(struct sort_entry *self, struct strlist *list,
 			     const char *list_name, FILE *fp);
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 5e02d2c..e9a5848 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -113,6 +113,7 @@ struct addr_location {
 	char	      level;
 	bool	      filtered;
 	unsigned int  cpumode;
+	s32           cpu;
 };
 
 enum dso_kernel_type {