All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting
@ 2014-05-13 16:48 Don Zickus
  2014-05-13 16:48 ` [PATCH 1/6] events, perf: Pass protection and flags bits through mmap2 interface Don Zickus
                   ` (6 more replies)
  0 siblings, 7 replies; 30+ messages in thread
From: Don Zickus @ 2014-05-13 16:48 UTC (permalink / raw)
  To: acme, peterz; +Cc: LKML, jolsa, namhyung, eranian, Andi Kleen, Don Zickus

These patches enable mmap2 support in the kernel and perf to
allow support for data cacheline sorting.

V2: change cacheline to dcacheline (Namhyung Kim)
    change dcacheline alogorithm a bit
    re-order mmap2 patches (Namhyung Kim)
    provide documentation (Andi Kleen)
 
Don Zickus (6):
  events, perf: Pass protection and flags bits through mmap2 interface
  Revert "perf: Disable PERF_RECORD_MMAP2 support"
  perf: Update mmap2 interface with protection and flag bits
  perf report: Add mem-mode documentation to report command
  perf: Add cpumode to struct hist_entry
  perf: Add dcacheline sort

 include/uapi/linux/perf_event.h          |   1 +
 kernel/events/core.c                     |  37 +++++++++--
 tools/perf/Documentation/perf-report.txt |  23 +++++++
 tools/perf/builtin-report.c              |   2 +-
 tools/perf/util/event.c                  |  59 +++++++++++------
 tools/perf/util/event.h                  |   2 +
 tools/perf/util/evsel.c                  |   1 +
 tools/perf/util/hist.c                   |   7 ++-
 tools/perf/util/hist.h                   |   1 +
 tools/perf/util/machine.c                |   4 +-
 tools/perf/util/map.c                    |   4 +-
 tools/perf/util/map.h                    |   4 +-
 tools/perf/util/sort.c                   | 105 +++++++++++++++++++++++++++++++
 tools/perf/util/sort.h                   |   2 +
 14 files changed, 222 insertions(+), 30 deletions(-)

-- 
1.7.11.7


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/6] events, perf: Pass protection and flags bits through mmap2 interface
  2014-05-13 16:48 [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting Don Zickus
@ 2014-05-13 16:48 ` Don Zickus
  2014-05-16 12:22   ` Peter Zijlstra
  2014-05-13 16:48 ` [PATCH 2/6] Revert "perf: Disable PERF_RECORD_MMAP2 support" Don Zickus
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 30+ messages in thread
From: Don Zickus @ 2014-05-13 16:48 UTC (permalink / raw)
  To: acme, peterz; +Cc: LKML, jolsa, namhyung, eranian, Andi Kleen, Don Zickus

From: Peter Zijlstra <a.p.zijlstra@chello.nl>

The mmap2 interface was missing the protection and flags bits needed to
accurately determine if a mmap memory area was shared or private and
if it was readable or not.

[tweaked patch to compile and wrote changelog]
Signed-off-by: Don Zickus <dzickus@redhat.com>

--
Peter you mentioned writing up a changelog for this patch, but you
seemed busy, so I jumped the gun a little bit.  Let me know how you
want to do this patch if this approach is wrong.
---
 include/uapi/linux/perf_event.h |  1 +
 kernel/events/core.c            | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 853bc1c..2ed502f 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -699,6 +699,7 @@ enum perf_event_type {
 	 *	u32				min;
 	 *	u64				ino;
 	 *	u64				ino_generation;
+	 *	u32				prot, flags;
 	 *	char				filename[];
 	 * 	struct sample_id		sample_id;
 	 * };
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 5129b12..0711cc9 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -40,6 +40,7 @@
 #include <linux/mm_types.h>
 #include <linux/cgroup.h>
 #include <linux/module.h>
+#include <linux/mman.h>
 
 #include "internal.h"
 
@@ -5089,6 +5090,7 @@ struct perf_mmap_event {
 	int			maj, min;
 	u64			ino;
 	u64			ino_generation;
+	u32			prot, flags;
 
 	struct {
 		struct perf_event_header	header;
@@ -5130,6 +5132,8 @@ static void perf_event_mmap_output(struct perf_event *event,
 		mmap_event->event_id.header.size += sizeof(mmap_event->min);
 		mmap_event->event_id.header.size += sizeof(mmap_event->ino);
 		mmap_event->event_id.header.size += sizeof(mmap_event->ino_generation);
+		mmap_event->event_id.header.size += sizeof(mmap_event->prot);
+		mmap_event->event_id.header.size += sizeof(mmap_event->flags);
 	}
 
 	perf_event_header__init_id(&mmap_event->event_id.header, &sample, event);
@@ -5148,6 +5152,8 @@ static void perf_event_mmap_output(struct perf_event *event,
 		perf_output_put(&handle, mmap_event->min);
 		perf_output_put(&handle, mmap_event->ino);
 		perf_output_put(&handle, mmap_event->ino_generation);
+		perf_output_put(&handle, mmap_event->prot);
+		perf_output_put(&handle, mmap_event->flags);
 	}
 
 	__output_copy(&handle, mmap_event->file_name,
@@ -5166,6 +5172,7 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 	struct file *file = vma->vm_file;
 	int maj = 0, min = 0;
 	u64 ino = 0, gen = 0;
+	u32 prot = 0, flags = 0;
 	unsigned int size;
 	char tmp[16];
 	char *buf = NULL;
@@ -5196,6 +5203,28 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		gen = inode->i_generation;
 		maj = MAJOR(dev);
 		min = MINOR(dev);
+
+		if (vma->vm_flags & VM_READ)
+			prot |= PROT_READ;
+		if (vma->vm_flags & VM_WRITE)
+			prot |= PROT_WRITE;
+		if (vma->vm_flags & VM_EXEC)
+			prot |= PROT_EXEC;
+
+		if (vma->vm_flags & VM_MAYSHARE)
+			flags = MAP_SHARED;
+		else
+			flags = MAP_PRIVATE;
+
+		if (vma->vm_flags & VM_DENYWRITE)
+			flags |= MAP_DENYWRITE;
+		if (vma->vm_flags & VM_MAYEXEC)
+			flags |= MAP_EXECUTABLE;
+		if (vma->vm_flags & VM_LOCKED)
+			flags |= MAP_LOCKED;
+		if (vma->vm_flags & VM_HUGETLB)
+			flags |= MAP_HUGETLB;
+
 		goto got_name;
 	} else {
 		name = (char *)arch_vma_name(vma);
@@ -5236,6 +5265,8 @@ got_name:
 	mmap_event->min = min;
 	mmap_event->ino = ino;
 	mmap_event->ino_generation = gen;
+	mmap_event->prot = prot;
+	mmap_event->flags = flags;
 
 	if (!(vma->vm_flags & VM_EXEC))
 		mmap_event->event_id.header.misc |= PERF_RECORD_MISC_MMAP_DATA;
@@ -5276,6 +5307,8 @@ void perf_event_mmap(struct vm_area_struct *vma)
 		/* .min (attr_mmap2 only) */
 		/* .ino (attr_mmap2 only) */
 		/* .ino_generation (attr_mmap2 only) */
+		/* .prot (attr_mmap2 only) */
+		/* .flags (attr_mmap2 only) */
 	};
 
 	perf_event_mmap_event(&mmap_event);
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 2/6] Revert "perf: Disable PERF_RECORD_MMAP2 support"
  2014-05-13 16:48 [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting Don Zickus
  2014-05-13 16:48 ` [PATCH 1/6] events, perf: Pass protection and flags bits through mmap2 interface Don Zickus
@ 2014-05-13 16:48 ` Don Zickus
  2014-05-16 11:25   ` Jiri Olsa
  2014-05-19 11:21   ` Jiri Olsa
  2014-05-13 16:48 ` [PATCH 3/6] perf: Update mmap2 interface with protection and flag bits Don Zickus
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 30+ messages in thread
From: Don Zickus @ 2014-05-13 16:48 UTC (permalink / raw)
  To: acme, peterz; +Cc: LKML, jolsa, namhyung, eranian, Andi Kleen, Don Zickus

This reverts commit 3090ffb5a2515990182f3f55b0688a7817325488.

Conflicts:
	tools/perf/util/event.c
---
 kernel/events/core.c    |  4 ----
 tools/perf/util/event.c | 36 +++++++++++++++++++-----------------
 tools/perf/util/evsel.c |  1 +
 3 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 0711cc9..c793cb0 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6882,10 +6882,6 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
 	if (ret)
 		return -EFAULT;
 
-	/* disabled for now */
-	if (attr->mmap2)
-		return -EINVAL;
-
 	if (attr->__reserved_1)
 		return -EINVAL;
 
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 65795b8..5e7c722 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -178,13 +178,14 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
 		return -1;
 	}
 
-	event->header.type = PERF_RECORD_MMAP;
+	event->header.type = PERF_RECORD_MMAP2;
 
 	while (1) {
 		char bf[BUFSIZ];
 		char prot[5];
 		char execname[PATH_MAX];
 		char anonstr[] = "//anon";
+		unsigned int ino;
 		size_t size;
 		ssize_t n;
 
@@ -195,14 +196,15 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
 		strcpy(execname, "");
 
 		/* 00400000-0040c000 r-xp 00000000 fd:01 41038  /bin/cat */
-		n = sscanf(bf, "%"PRIx64"-%"PRIx64" %s %"PRIx64" %*x:%*x %*u %s\n",
-		       &event->mmap.start, &event->mmap.len, prot,
-		       &event->mmap.pgoff,
-		       execname);
-		/*
- 		 * Anon maps don't have the execname.
- 		 */
-		if (n < 4)
+		n = sscanf(bf, "%"PRIx64"-%"PRIx64" %s %"PRIx64" %x:%x %u %s\n",
+		       &event->mmap2.start, &event->mmap2.len, prot,
+		       &event->mmap2.pgoff, &event->mmap2.maj,
+		       &event->mmap2.min,
+		       &ino, execname);
+
+		event->mmap2.ino = (u64)ino;
+
+		if (n < 7)
 			continue;
 		/*
 		 * Just like the kernel, see __perf_event_mmap in kernel/perf_event.c
@@ -223,15 +225,15 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
 			strcpy(execname, anonstr);
 
 		size = strlen(execname) + 1;
-		memcpy(event->mmap.filename, execname, size);
+		memcpy(event->mmap2.filename, execname, size);
 		size = PERF_ALIGN(size, sizeof(u64));
-		event->mmap.len -= event->mmap.start;
-		event->mmap.header.size = (sizeof(event->mmap) -
-					(sizeof(event->mmap.filename) - size));
-		memset(event->mmap.filename + size, 0, machine->id_hdr_size);
-		event->mmap.header.size += machine->id_hdr_size;
-		event->mmap.pid = tgid;
-		event->mmap.tid = pid;
+		event->mmap2.len -= event->mmap.start;
+		event->mmap2.header.size = (sizeof(event->mmap2) -
+					(sizeof(event->mmap2.filename) - size));
+		memset(event->mmap2.filename + size, 0, machine->id_hdr_size);
+		event->mmap2.header.size += machine->id_hdr_size;
+		event->mmap2.pid = tgid;
+		event->mmap2.tid = pid;
 
 		if (process(tool, event, &synth_sample, machine) != 0) {
 			rc = -1;
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 5c28d82..21154da 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -659,6 +659,7 @@ void perf_evsel__config(struct perf_evsel *evsel, struct record_opts *opts)
 		perf_evsel__set_sample_bit(evsel, WEIGHT);
 
 	attr->mmap  = track;
+	attr->mmap2 = track && !perf_missing_features.mmap2;
 	attr->comm  = track;
 
 	if (opts->sample_transaction)
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 3/6] perf: Update mmap2 interface with protection and flag bits
  2014-05-13 16:48 [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting Don Zickus
  2014-05-13 16:48 ` [PATCH 1/6] events, perf: Pass protection and flags bits through mmap2 interface Don Zickus
  2014-05-13 16:48 ` [PATCH 2/6] Revert "perf: Disable PERF_RECORD_MMAP2 support" Don Zickus
@ 2014-05-13 16:48 ` Don Zickus
  2014-05-13 16:48 ` [PATCH 4/6] perf report: Add mem-mode documentation to report command Don Zickus
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 30+ messages in thread
From: Don Zickus @ 2014-05-13 16:48 UTC (permalink / raw)
  To: acme, peterz; +Cc: LKML, jolsa, namhyung, eranian, Andi Kleen, Don Zickus

The kernel piece passes more info now.  Update the perf tool to reflect
that and adjust the synthesized maps to play along.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 tools/perf/util/event.c   | 23 +++++++++++++++++++++--
 tools/perf/util/event.h   |  2 ++
 tools/perf/util/machine.c |  4 +++-
 tools/perf/util/map.c     |  4 +++-
 tools/perf/util/map.h     |  4 +++-
 5 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 5e7c722..e91d28e 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -1,4 +1,5 @@
 #include <linux/types.h>
+#include <sys/mman.h>
 #include "event.h"
 #include "debug.h"
 #include "hist.h"
@@ -214,6 +215,21 @@ int perf_event__synthesize_mmap_events(struct perf_tool *tool,
 		else
 			event->header.misc = PERF_RECORD_MISC_GUEST_USER;
 
+		/* map protection and flags bits */
+		event->mmap2.prot = 0;
+		event->mmap2.flags = 0;
+		if (prot[0] == 'r')
+			event->mmap2.prot |= PROT_READ;
+		if (prot[1] == 'w')
+			event->mmap2.prot |= PROT_WRITE;
+		if (prot[2] == 'x')
+			event->mmap2.prot |= PROT_EXEC;
+
+		if (prot[3] == 's')
+			event->mmap2.flags |= MAP_SHARED;
+		else
+			event->mmap2.flags |= MAP_PRIVATE;
+
 		if (prot[2] != 'x') {
 			if (!mmap_data || prot[0] != 'r')
 				continue;
@@ -614,12 +630,15 @@ size_t perf_event__fprintf_mmap(union perf_event *event, FILE *fp)
 size_t perf_event__fprintf_mmap2(union perf_event *event, FILE *fp)
 {
 	return fprintf(fp, " %d/%d: [%#" PRIx64 "(%#" PRIx64 ") @ %#" PRIx64
-			   " %02x:%02x %"PRIu64" %"PRIu64"]: %c %s\n",
+			   " %02x:%02x %"PRIu64" %"PRIu64"]: %c%c%c%c %s\n",
 		       event->mmap2.pid, event->mmap2.tid, event->mmap2.start,
 		       event->mmap2.len, event->mmap2.pgoff, event->mmap2.maj,
 		       event->mmap2.min, event->mmap2.ino,
 		       event->mmap2.ino_generation,
-		       (event->header.misc & PERF_RECORD_MISC_MMAP_DATA) ? 'r' : 'x',
+		       (event->mmap2.prot & PROT_READ) ? 'r' : '-',
+		       (event->mmap2.prot & PROT_WRITE) ? 'w' : '-',
+		       (event->mmap2.prot & PROT_EXEC) ? 'x' : '-',
+		       (event->mmap2.flags & MAP_SHARED) ? 's' : 'p',
 		       event->mmap2.filename);
 }
 
diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index d970232..9ba2eb3 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -27,6 +27,8 @@ struct mmap2_event {
 	u32 min;
 	u64 ino;
 	u64 ino_generation;
+	u32 prot;
+	u32 flags;
 	char filename[PATH_MAX];
 };
 
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 7409ac8..0e5fea9 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1060,6 +1060,8 @@ int machine__process_mmap2_event(struct machine *machine,
 			event->mmap2.pid, event->mmap2.maj,
 			event->mmap2.min, event->mmap2.ino,
 			event->mmap2.ino_generation,
+			event->mmap2.prot,
+			event->mmap2.flags,
 			event->mmap2.filename, type);
 
 	if (map == NULL)
@@ -1105,7 +1107,7 @@ int machine__process_mmap_event(struct machine *machine, union perf_event *event
 
 	map = map__new(&machine->user_dsos, event->mmap.start,
 			event->mmap.len, event->mmap.pgoff,
-			event->mmap.pid, 0, 0, 0, 0,
+			event->mmap.pid, 0, 0, 0, 0, 0, 0,
 			event->mmap.filename,
 			type);
 
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index ba5f5c0c..7ca1d7e 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -51,7 +51,7 @@ void map__init(struct map *map, enum map_type type,
 
 struct map *map__new(struct list_head *dsos__list, u64 start, u64 len,
 		     u64 pgoff, u32 pid, u32 d_maj, u32 d_min, u64 ino,
-		     u64 ino_gen, char *filename,
+		     u64 ino_gen, u32 prot, u32 flags, char *filename,
 		     enum map_type type)
 {
 	struct map *map = malloc(sizeof(*map));
@@ -69,6 +69,8 @@ struct map *map__new(struct list_head *dsos__list, u64 start, u64 len,
 		map->min = d_min;
 		map->ino = ino;
 		map->ino_generation = ino_gen;
+		map->prot = prot;
+		map->flags = flags;
 
 		if ((anon || no_dso) && type == MAP__FUNCTION) {
 			snprintf(newfilename, sizeof(newfilename), "/tmp/perf-%d.map", pid);
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index ae2d451..7758c72 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -35,6 +35,8 @@ struct map {
 	bool			referenced;
 	bool			erange_warned;
 	u32			priv;
+	u32			prot;
+	u32			flags;
 	u64			pgoff;
 	u64			reloc;
 	u32			maj, min; /* only valid for MMAP2 record */
@@ -118,7 +120,7 @@ void map__init(struct map *map, enum map_type type,
 	       u64 start, u64 end, u64 pgoff, struct dso *dso);
 struct map *map__new(struct list_head *dsos__list, u64 start, u64 len,
 		     u64 pgoff, u32 pid, u32 d_maj, u32 d_min, u64 ino,
-		     u64 ino_gen,
+		     u64 ino_gen, u32 prot, u32 flags,
 		     char *filename, enum map_type type);
 struct map *map__new2(u64 start, struct dso *dso, enum map_type type);
 void map__delete(struct map *map);
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 4/6] perf report: Add mem-mode documentation to report command
  2014-05-13 16:48 [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting Don Zickus
                   ` (2 preceding siblings ...)
  2014-05-13 16:48 ` [PATCH 3/6] perf: Update mmap2 interface with protection and flag bits Don Zickus
@ 2014-05-13 16:48 ` Don Zickus
  2014-05-13 16:48 ` [PATCH 5/6] perf: Add cpumode to struct hist_entry Don Zickus
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 30+ messages in thread
From: Don Zickus @ 2014-05-13 16:48 UTC (permalink / raw)
  To: acme, peterz; +Cc: LKML, jolsa, namhyung, eranian, Andi Kleen, Don Zickus

Add mem-mode sorting types and mem-mode itself to perf-report documentation.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 tools/perf/Documentation/perf-report.txt | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index 09af662..bea1c83 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -98,6 +98,21 @@ OPTIONS
 	And default sort keys are changed to comm, dso_from, symbol_from, dso_to
 	and symbol_to, see '--branch-stack'.
 
+	If --mem-mode option is used, following sort keys are also available
+	(incompatible with --branch-stack):
+	symbol_daddr, dso_daddr, locked, tlb, mem, snoop.
+
+	- symbol_daddr: name of data symbol being executed on at the time of sample
+	- dso_daddr: name of library or module containing the data being executed
+	on at the time of sample
+	- locked: whether the bus was locked at the time of sample
+	- tlb: type of tlb access for the data at the time of sample
+	- mem: type of memory access for the data at the time of sample
+	- snoop: type of snoop (if any) for the data at the time of sample
+
+	And default sort keys are changed to local_weight, mem, sym, dso,
+	symbol_daddr, dso_daddr, snoop, tlb, locked, see '--mem-mode'.
+
 -p::
 --parent=<regex>::
         A regex filter to identify parent. The parent is a caller of this
@@ -236,6 +251,13 @@ OPTIONS
 	Demangle symbol names to human readable form. It's enabled by default,
 	disable with --no-demangle.
 
+--mem-mode::
+	Use the data addresses of samples in addition to instruction addresses
+	to build the histograms.  To generate meaningful output, the perf.data
+	file must have been obtained using perf record -d -W and using a 
+	special event -e cpu/mem-loads/ or -e cpu/mem-stores/. See
+	'perf mem' for simpler access.
+
 --percent-limit::
 	Do not show entries which have an overhead under that percent.
 	(Default: 0).
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 5/6] perf: Add cpumode to struct hist_entry
  2014-05-13 16:48 [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting Don Zickus
                   ` (3 preceding siblings ...)
  2014-05-13 16:48 ` [PATCH 4/6] perf report: Add mem-mode documentation to report command Don Zickus
@ 2014-05-13 16:48 ` Don Zickus
  2014-05-13 16:48 ` [PATCH 6/6] perf: Add dcacheline sort Don Zickus
  2014-05-13 16:52 ` [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting Don Zickus
  6 siblings, 0 replies; 30+ messages in thread
From: Don Zickus @ 2014-05-13 16:48 UTC (permalink / raw)
  To: acme, peterz; +Cc: LKML, jolsa, namhyung, eranian, Andi Kleen, Don Zickus

The next patch needs to sort on cpumode, so add it to hist_entry to be tracked.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 tools/perf/util/hist.c | 7 ++++---
 tools/perf/util/sort.h | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 7f0236c..b7160b1c 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -410,9 +410,10 @@ struct hist_entry *__hists__add_entry(struct hists *hists,
 			.map	= al->map,
 			.sym	= al->sym,
 		},
-		.cpu	= al->cpu,
-		.ip	= al->addr,
-		.level	= al->level,
+		.cpu	 = al->cpu,
+		.cpumode = al->cpumode,
+		.ip	 = al->addr,
+		.level	 = al->level,
 		.stat = {
 			.nr_events = 1,
 			.period	= period,
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 43e5ff4..22cf912 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -87,6 +87,7 @@ struct hist_entry {
 	u64			ip;
 	u64			transaction;
 	s32			cpu;
+	u8			cpumode;
 
 	struct hist_entry_diff	diff;
 
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 6/6] perf: Add dcacheline sort
  2014-05-13 16:48 [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting Don Zickus
                   ` (4 preceding siblings ...)
  2014-05-13 16:48 ` [PATCH 5/6] perf: Add cpumode to struct hist_entry Don Zickus
@ 2014-05-13 16:48 ` Don Zickus
  2014-05-16 11:47   ` Jiri Olsa
  2014-05-16 14:09   ` Stephane Eranian
  2014-05-13 16:52 ` [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting Don Zickus
  6 siblings, 2 replies; 30+ messages in thread
From: Don Zickus @ 2014-05-13 16:48 UTC (permalink / raw)
  To: acme, peterz; +Cc: LKML, jolsa, namhyung, eranian, Andi Kleen, Don Zickus

In perf's 'mem-mode', one can get access to a whole bunch of details specific to a
particular sample instruction.  A bunch of those details relate to the data
address.

One interesting thing you can do with data addresses is to convert them into a unique
cacheline they belong too.  Organizing these data cachelines into similar groups and sorting
them can reveal cache contention.

This patch creates an alogorithm based on various sample details that can help group
entries together into data cachelines and allows 'perf report' to sort on it.

The algorithm relies on having proper mmap2 support in the kernel to help determine
if the memory map the data address belongs to is private to a pid or globally shared.

The alogortithm is as follows:

o group cpumodes together
o group entries with discovered maps together
o sort on major, minor, inode and inode generation numbers
o if userspace anon, then sort on pid
o sort on cachelines based on data addresses

The 'dcacheline' sort option in 'perf report' only works in 'mem-mode'.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 tools/perf/Documentation/perf-report.txt |   3 +-
 tools/perf/builtin-report.c              |   2 +-
 tools/perf/util/hist.h                   |   1 +
 tools/perf/util/sort.c                   | 105 +++++++++++++++++++++++++++++++
 tools/perf/util/sort.h                   |   1 +
 5 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index bea1c83..9115d02 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -100,7 +100,7 @@ OPTIONS
 
 	If --mem-mode option is used, following sort keys are also available
 	(incompatible with --branch-stack):
-	symbol_daddr, dso_daddr, locked, tlb, mem, snoop.
+	symbol_daddr, dso_daddr, locked, tlb, mem, snoop, dcacheline.
 
 	- symbol_daddr: name of data symbol being executed on at the time of sample
 	- dso_daddr: name of library or module containing the data being executed
@@ -109,6 +109,7 @@ OPTIONS
 	- tlb: type of tlb access for the data at the time of sample
 	- mem: type of memory access for the data at the time of sample
 	- snoop: type of snoop (if any) for the data at the time of sample
+	- dcacheline: the cacheline the data address is on at the time of sample
 
 	And default sort keys are changed to local_weight, mem, sym, dso,
 	symbol_daddr, dso_daddr, snoop, tlb, locked, see '--mem-mode'.
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 89c9528..387459f 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -702,7 +702,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		   "sort by key(s): pid, comm, dso, symbol, parent, cpu, srcline,"
 		   " dso_to, dso_from, symbol_to, symbol_from, mispredict,"
 		   " weight, local_weight, mem, symbol_daddr, dso_daddr, tlb, "
-		   "snoop, locked, abort, in_tx, transaction"),
+		   "snoop, locked, abort, in_tx, transaction, dcacheline"),
 	OPT_BOOLEAN(0, "showcpuutilization", &symbol_conf.show_cpu_utilization,
 		    "Show sample percentage for different cpu modes"),
 	OPT_STRING('p', "parent", &parent_pattern, "regex",
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 38c3e87..14d1dc1 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -72,6 +72,7 @@ enum hist_column {
 	HISTC_MEM_TLB,
 	HISTC_MEM_LVL,
 	HISTC_MEM_SNOOP,
+	HISTC_MEM_DCACHELINE,
 	HISTC_TRANSACTION,
 	HISTC_NR_COLS, /* Last entry */
 };
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 635cd8f..0e91ba9 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1,3 +1,4 @@
+#include <sys/mman.h>
 #include "sort.h"
 #include "hist.h"
 #include "comm.h"
@@ -764,6 +765,102 @@ static int hist_entry__snoop_snprintf(struct hist_entry *he, char *bf,
 	return repsep_snprintf(bf, size, "%-*s", width, out);
 }
 
+#define CACHE_LINESIZE       64
+#define CLINE_OFFSET_MSK     (CACHE_LINESIZE - 1)
+#define CLADRS(a)            ((a) & ~(CLINE_OFFSET_MSK))
+
+static int64_t
+sort__dcacheline_cmp(struct hist_entry *left, struct hist_entry *right)
+{
+	u64 l, r;
+	struct map *l_map, *r_map;
+
+	if (!left->mem_info)  return -1;
+	if (!right->mem_info) return 1;
+
+	/* group event types together */
+	if (left->cpumode > right->cpumode) return -1;
+	if (left->cpumode < right->cpumode) return 1;
+
+	l_map = left->mem_info->daddr.map;
+	r_map = right->mem_info->daddr.map;
+
+	/* if both are NULL, jump to sort on al_addr instead */
+	if (!l_map && !r_map)
+		goto addr;
+
+	if (!l_map) return -1;
+	if (!r_map) return 1;
+
+	if (l_map->maj > r_map->maj) return -1;
+	if (l_map->maj < r_map->maj) return 1;
+
+	if (l_map->min > r_map->min) return -1;
+	if (l_map->min < r_map->min) return 1;
+
+	if (l_map->ino > r_map->ino) return -1;
+	if (l_map->ino < r_map->ino) return 1;
+
+	if (l_map->ino_generation > r_map->ino_generation) return -1;
+	if (l_map->ino_generation < r_map->ino_generation) return 1;
+
+	/*
+	 * Addresses with no major/minor numbers are assumed to be
+	 * anonymous in userspace.  Sort those on pid then address.
+	 *
+	 * The kernel and non-zero major/minor mapped areas are
+	 * assumed to be unity mapped.  Sort those on address.
+	 */
+
+	if ((left->cpumode != PERF_RECORD_MISC_KERNEL) &&
+	    (!(l_map->flags & MAP_SHARED)) &&
+	    !l_map->maj && !l_map->min && !l_map->ino &&
+	    !l_map->ino_generation) {
+		/* userspace anonymous */
+
+		if (left->thread->pid_ > right->thread->pid_) return -1;
+		if (left->thread->pid_ < right->thread->pid_) return 1;
+	}
+
+addr:
+	/* al_addr does all the right addr - start + offset calculations */
+	l = CLADRS(left->mem_info->daddr.al_addr);
+	r = CLADRS(right->mem_info->daddr.al_addr);
+
+	if (l > r) return -1;
+	if (l < r) return 1;
+
+	return 0;
+}
+
+static int hist_entry__dcacheline_snprintf(struct hist_entry *he, char *bf,
+					  size_t size, unsigned int width)
+{
+
+	uint64_t addr = 0;
+	struct map *map = NULL;
+	struct symbol *sym = NULL;
+	char level = he->level;
+
+	if (he->mem_info) {
+		addr = CLADRS(he->mem_info->daddr.al_addr);
+		map = he->mem_info->daddr.map;
+		sym = he->mem_info->daddr.sym;
+
+		/* print [s] for shared data mmaps */
+		if ((he->cpumode != PERF_RECORD_MISC_KERNEL) &&
+		     map && (map->type == MAP__VARIABLE) &&
+		    (map->flags & MAP_SHARED) &&
+		    (map->maj || map->min || map->ino ||
+		     map->ino_generation))
+			level = 's';
+		else if (!map)
+			level = 'X';
+	}
+	return _hist_entry__sym_snprintf(map, sym, addr, level, bf, size,
+					 width);
+}
+
 struct sort_entry sort_mispredict = {
 	.se_header	= "Branch Mispredicted",
 	.se_cmp		= sort__mispredict_cmp,
@@ -856,6 +953,13 @@ struct sort_entry sort_mem_snoop = {
 	.se_width_idx	= HISTC_MEM_SNOOP,
 };
 
+struct sort_entry sort_mem_dcacheline = {
+	.se_header	= "Data Cacheline         ",
+	.se_cmp		= sort__dcacheline_cmp,
+	.se_snprintf	= hist_entry__dcacheline_snprintf,
+	.se_width_idx	= HISTC_MEM_DCACHELINE,
+};
+
 static int64_t
 sort__abort_cmp(struct hist_entry *left, struct hist_entry *right)
 {
@@ -1023,6 +1127,7 @@ static struct sort_dimension memory_sort_dimensions[] = {
 	DIM(SORT_MEM_TLB, "tlb", sort_mem_tlb),
 	DIM(SORT_MEM_LVL, "mem", sort_mem_lvl),
 	DIM(SORT_MEM_SNOOP, "snoop", sort_mem_snoop),
+	DIM(SORT_MEM_DCACHELINE, "dcacheline", sort_mem_dcacheline),
 };
 
 #undef DIM
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 22cf912..460ec9c 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -167,6 +167,7 @@ enum sort_type {
 	SORT_MEM_TLB,
 	SORT_MEM_LVL,
 	SORT_MEM_SNOOP,
+	SORT_MEM_DCACHELINE,
 };
 
 /*
-- 
1.7.11.7


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting
  2014-05-13 16:48 [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting Don Zickus
                   ` (5 preceding siblings ...)
  2014-05-13 16:48 ` [PATCH 6/6] perf: Add dcacheline sort Don Zickus
@ 2014-05-13 16:52 ` Don Zickus
  6 siblings, 0 replies; 30+ messages in thread
From: Don Zickus @ 2014-05-13 16:52 UTC (permalink / raw)
  To: acme, peterz; +Cc: LKML, jolsa, namhyung, eranian, Andi Kleen

On Tue, May 13, 2014 at 12:48:11PM -0400, Don Zickus wrote:
> These patches enable mmap2 support in the kernel and perf to
> allow support for data cacheline sorting.
> 
> V2: change cacheline to dcacheline (Namhyung Kim)
>     change dcacheline alogorithm a bit
>     re-order mmap2 patches (Namhyung Kim)
>     provide documentation (Andi Kleen)

One other thing, this is based off of Jiri's perf/core branch
13ce34df11833482cd698331fdbb3f8ced06340d.  I think that is the new/temp
perf tip for now.

Cheers,
Don

>  
> Don Zickus (6):
>   events, perf: Pass protection and flags bits through mmap2 interface
>   Revert "perf: Disable PERF_RECORD_MMAP2 support"
>   perf: Update mmap2 interface with protection and flag bits
>   perf report: Add mem-mode documentation to report command
>   perf: Add cpumode to struct hist_entry
>   perf: Add dcacheline sort
> 
>  include/uapi/linux/perf_event.h          |   1 +
>  kernel/events/core.c                     |  37 +++++++++--
>  tools/perf/Documentation/perf-report.txt |  23 +++++++
>  tools/perf/builtin-report.c              |   2 +-
>  tools/perf/util/event.c                  |  59 +++++++++++------
>  tools/perf/util/event.h                  |   2 +
>  tools/perf/util/evsel.c                  |   1 +
>  tools/perf/util/hist.c                   |   7 ++-
>  tools/perf/util/hist.h                   |   1 +
>  tools/perf/util/machine.c                |   4 +-
>  tools/perf/util/map.c                    |   4 +-
>  tools/perf/util/map.h                    |   4 +-
>  tools/perf/util/sort.c                   | 105 +++++++++++++++++++++++++++++++
>  tools/perf/util/sort.h                   |   2 +
>  14 files changed, 222 insertions(+), 30 deletions(-)
> 
> -- 
> 1.7.11.7
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/6] Revert "perf: Disable PERF_RECORD_MMAP2 support"
  2014-05-13 16:48 ` [PATCH 2/6] Revert "perf: Disable PERF_RECORD_MMAP2 support" Don Zickus
@ 2014-05-16 11:25   ` Jiri Olsa
  2014-05-16 13:26     ` Don Zickus
  2014-05-19 11:21   ` Jiri Olsa
  1 sibling, 1 reply; 30+ messages in thread
From: Jiri Olsa @ 2014-05-16 11:25 UTC (permalink / raw)
  To: Don Zickus; +Cc: acme, peterz, LKML, namhyung, eranian, Andi Kleen

On Tue, May 13, 2014 at 12:48:13PM -0400, Don Zickus wrote:

SNIP

> -		/*
> - 		 * Anon maps don't have the execname.
> - 		 */
> -		if (n < 4)
> +		n = sscanf(bf, "%"PRIx64"-%"PRIx64" %s %"PRIx64" %x:%x %u %s\n",
> +		       &event->mmap2.start, &event->mmap2.len, prot,
> +		       &event->mmap2.pgoff, &event->mmap2.maj,
> +		       &event->mmap2.min,
> +		       &ino, execname);
> +
> +		event->mmap2.ino = (u64)ino;
> +
> +		if (n < 7)
>  			continue;

any reason for changing this from 'if (n != 8)' ?

jirka

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-13 16:48 ` [PATCH 6/6] perf: Add dcacheline sort Don Zickus
@ 2014-05-16 11:47   ` Jiri Olsa
  2014-05-16 13:30     ` Don Zickus
  2014-05-16 14:09   ` Stephane Eranian
  1 sibling, 1 reply; 30+ messages in thread
From: Jiri Olsa @ 2014-05-16 11:47 UTC (permalink / raw)
  To: Don Zickus; +Cc: acme, peterz, LKML, namhyung, eranian, Andi Kleen

On Tue, May 13, 2014 at 12:48:17PM -0400, Don Zickus wrote:
> In perf's 'mem-mode', one can get access to a whole bunch of details specific to a
> particular sample instruction.  A bunch of those details relate to the data
> address.
> 
> One interesting thing you can do with data addresses is to convert them into a unique
> cacheline they belong too.  Organizing these data cachelines into similar groups and sorting
> them can reveal cache contention.
> 
> This patch creates an alogorithm based on various sample details that can help group
> entries together into data cachelines and allows 'perf report' to sort on it.
> 
> The algorithm relies on having proper mmap2 support in the kernel to help determine
> if the memory map the data address belongs to is private to a pid or globally shared.
> 
> The alogortithm is as follows:
> 
> o group cpumodes together
> o group entries with discovered maps together
> o sort on major, minor, inode and inode generation numbers
> o if userspace anon, then sort on pid
> o sort on cachelines based on data addresses

needs some collumn width refresh or something..? ;-)

# Overhead  Data Cacheline         
# ........  .......................
#
     5.42%  [k] 0xffff8801ed832c40 
     5.29%  [.] sys_errlist@@GLIBC_2.12+0xffffffcbf7dfc1ff                       
     3.16%  [k] 0xffffffffff5690c0 


also I've got again perf hanged up on opening device file

[jolsa@krava perf]$ sudo strace -p 29445
Process 29445 attached
open("/dev/snd/pcmC0D0p", O_RDONLY^CProcess 29445 detached

another one I recall was /dev/dri/card0 touched by X server

I guess those device files allow to mmap memory and we recorded
memory access there.. we need check for this and do not try to
open device files


jirka

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/6] events, perf: Pass protection and flags bits through mmap2 interface
  2014-05-13 16:48 ` [PATCH 1/6] events, perf: Pass protection and flags bits through mmap2 interface Don Zickus
@ 2014-05-16 12:22   ` Peter Zijlstra
  2014-05-16 13:33     ` Don Zickus
  0 siblings, 1 reply; 30+ messages in thread
From: Peter Zijlstra @ 2014-05-16 12:22 UTC (permalink / raw)
  To: Don Zickus; +Cc: acme, LKML, jolsa, namhyung, eranian, Andi Kleen

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

On Tue, May 13, 2014 at 12:48:12PM -0400, Don Zickus wrote:
> From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> 
> The mmap2 interface was missing the protection and flags bits needed to
> accurately determine if a mmap memory area was shared or private and
> if it was readable or not.
> 
> [tweaked patch to compile and wrote changelog]
> Signed-off-by: Don Zickus <dzickus@redhat.com>
> 
> --
> Peter you mentioned writing up a changelog for this patch, but you
> seemed busy, so I jumped the gun a little bit.  Let me know how you
> want to do this patch if this approach is wrong.

Oh, right, totally forgot about that. yeah I think the blub you have is
adequate. 

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/6] Revert "perf: Disable PERF_RECORD_MMAP2 support"
  2014-05-16 11:25   ` Jiri Olsa
@ 2014-05-16 13:26     ` Don Zickus
  2014-05-19 11:45       ` Jiri Olsa
  0 siblings, 1 reply; 30+ messages in thread
From: Don Zickus @ 2014-05-16 13:26 UTC (permalink / raw)
  To: Jiri Olsa; +Cc: acme, peterz, LKML, namhyung, eranian, Andi Kleen

On Fri, May 16, 2014 at 01:25:14PM +0200, Jiri Olsa wrote:
> On Tue, May 13, 2014 at 12:48:13PM -0400, Don Zickus wrote:
> 
> SNIP
> 
> > -		/*
> > - 		 * Anon maps don't have the execname.
> > - 		 */
> > -		if (n < 4)
> > +		n = sscanf(bf, "%"PRIx64"-%"PRIx64" %s %"PRIx64" %x:%x %u %s\n",
> > +		       &event->mmap2.start, &event->mmap2.len, prot,
> > +		       &event->mmap2.pgoff, &event->mmap2.maj,
> > +		       &event->mmap2.min,
> > +		       &ino, execname);
> > +
> > +		event->mmap2.ino = (u64)ino;
> > +
> > +		if (n < 7)
> >  			continue;
> 
> any reason for changing this from 'if (n != 8)' ?

Yes, read 9d4ecc8893832337daf241236841db966fa53489. :-)

This wasn't a clean revert because of the change for (n != 5) -> (n < 4).
So I maintainted the spirit of the upstream code with this revert.  Let me
know if that is wrong.

Cheers,
Don

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-16 11:47   ` Jiri Olsa
@ 2014-05-16 13:30     ` Don Zickus
  2014-05-16 14:05       ` Jiri Olsa
  0 siblings, 1 reply; 30+ messages in thread
From: Don Zickus @ 2014-05-16 13:30 UTC (permalink / raw)
  To: Jiri Olsa; +Cc: acme, peterz, LKML, namhyung, eranian, Andi Kleen

On Fri, May 16, 2014 at 01:47:57PM +0200, Jiri Olsa wrote:
> On Tue, May 13, 2014 at 12:48:17PM -0400, Don Zickus wrote:
> > In perf's 'mem-mode', one can get access to a whole bunch of details specific to a
> > particular sample instruction.  A bunch of those details relate to the data
> > address.
> > 
> > One interesting thing you can do with data addresses is to convert them into a unique
> > cacheline they belong too.  Organizing these data cachelines into similar groups and sorting
> > them can reveal cache contention.
> > 
> > This patch creates an alogorithm based on various sample details that can help group
> > entries together into data cachelines and allows 'perf report' to sort on it.
> > 
> > The algorithm relies on having proper mmap2 support in the kernel to help determine
> > if the memory map the data address belongs to is private to a pid or globally shared.
> > 
> > The alogortithm is as follows:
> > 
> > o group cpumodes together
> > o group entries with discovered maps together
> > o sort on major, minor, inode and inode generation numbers
> > o if userspace anon, then sort on pid
> > o sort on cachelines based on data addresses
> 
> needs some collumn width refresh or something..? ;-)

Not sure what you mean here.

> 
> # Overhead  Data Cacheline         
> # ........  .......................
> #
>      5.42%  [k] 0xffff8801ed832c40 
>      5.29%  [.] sys_errlist@@GLIBC_2.12+0xffffffcbf7dfc1ff                       
>      3.16%  [k] 0xffffffffff5690c0 
> 
> 
> also I've got again perf hanged up on opening device file
> 
> [jolsa@krava perf]$ sudo strace -p 29445
> Process 29445 attached
> open("/dev/snd/pcmC0D0p", O_RDONLY^CProcess 29445 detached
> 
> another one I recall was /dev/dri/card0 touched by X server
> 
> I guess those device files allow to mmap memory and we recorded
> memory access there.. we need check for this and do not try to
> open device files

Ok.  And that problem doesn't happen when my patch is not applied?  I am
not sure how this patch causes open device hangs.  I'll try to run this on
a box with X server running to duplicate.

Cheers,
Don

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/6] events, perf: Pass protection and flags bits through mmap2 interface
  2014-05-16 12:22   ` Peter Zijlstra
@ 2014-05-16 13:33     ` Don Zickus
  2014-05-16 15:45       ` Peter Zijlstra
  0 siblings, 1 reply; 30+ messages in thread
From: Don Zickus @ 2014-05-16 13:33 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: acme, LKML, jolsa, namhyung, eranian, Andi Kleen

On Fri, May 16, 2014 at 02:22:19PM +0200, Peter Zijlstra wrote:
> On Tue, May 13, 2014 at 12:48:12PM -0400, Don Zickus wrote:
> > From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > 
> > The mmap2 interface was missing the protection and flags bits needed to
> > accurately determine if a mmap memory area was shared or private and
> > if it was readable or not.
> > 
> > [tweaked patch to compile and wrote changelog]
> > Signed-off-by: Don Zickus <dzickus@redhat.com>
> > 
> > --
> > Peter you mentioned writing up a changelog for this patch, but you
> > seemed busy, so I jumped the gun a little bit.  Let me know how you
> > want to do this patch if this approach is wrong.
> 
> Oh, right, totally forgot about that. yeah I think the blub you have is
> adequate. 

I realized I didn't put your sign-off even though I said the patch is from
you.  Should I repost with that?  I don't know who is going to take what
patches from this series.

Cheers,
Don


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-16 13:30     ` Don Zickus
@ 2014-05-16 14:05       ` Jiri Olsa
  2014-05-16 14:30         ` Don Zickus
  0 siblings, 1 reply; 30+ messages in thread
From: Jiri Olsa @ 2014-05-16 14:05 UTC (permalink / raw)
  To: Don Zickus; +Cc: acme, peterz, LKML, namhyung, eranian, Andi Kleen

On Fri, May 16, 2014 at 09:30:58AM -0400, Don Zickus wrote:
> On Fri, May 16, 2014 at 01:47:57PM +0200, Jiri Olsa wrote:
> > On Tue, May 13, 2014 at 12:48:17PM -0400, Don Zickus wrote:
> > > In perf's 'mem-mode', one can get access to a whole bunch of details specific to a
> > > particular sample instruction.  A bunch of those details relate to the data
> > > address.
> > > 
> > > One interesting thing you can do with data addresses is to convert them into a unique
> > > cacheline they belong too.  Organizing these data cachelines into similar groups and sorting
> > > them can reveal cache contention.
> > > 
> > > This patch creates an alogorithm based on various sample details that can help group
> > > entries together into data cachelines and allows 'perf report' to sort on it.
> > > 
> > > The algorithm relies on having proper mmap2 support in the kernel to help determine
> > > if the memory map the data address belongs to is private to a pid or globally shared.
> > > 
> > > The alogortithm is as follows:
> > > 
> > > o group cpumodes together
> > > o group entries with discovered maps together
> > > o sort on major, minor, inode and inode generation numbers
> > > o if userspace anon, then sort on pid
> > > o sort on cachelines based on data addresses
> > 
> > needs some collumn width refresh or something..? ;-)
> 
> Not sure what you mean here.
> 
> > 
> > # Overhead  Data Cacheline         
> > # ........  .......................

header not being wide enough to cover the longest data


> > #
> >      5.42%  [k] 0xffff8801ed832c40 
> >      5.29%  [.] sys_errlist@@GLIBC_2.12+0xffffffcbf7dfc1ff                       
> >      3.16%  [k] 0xffffffffff5690c0 
> > 
> > 
> > also I've got again perf hanged up on opening device file
> > 
> > [jolsa@krava perf]$ sudo strace -p 29445
> > Process 29445 attached
> > open("/dev/snd/pcmC0D0p", O_RDONLY^CProcess 29445 detached
> > 
> > another one I recall was /dev/dri/card0 touched by X server
> > 
> > I guess those device files allow to mmap memory and we recorded
> > memory access there.. we need check for this and do not try to
> > open device files
> 
> Ok.  And that problem doesn't happen when my patch is not applied?  I am
> not sure how this patch causes open device hangs.  I'll try to run this on
> a box with X server running to duplicate.

I think it came with the memory profiling, because we treat
data areas as dsos.. open and look for symbols

jirka

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-13 16:48 ` [PATCH 6/6] perf: Add dcacheline sort Don Zickus
  2014-05-16 11:47   ` Jiri Olsa
@ 2014-05-16 14:09   ` Stephane Eranian
  2014-05-16 15:18     ` Don Zickus
  2014-05-16 15:59     ` Peter Zijlstra
  1 sibling, 2 replies; 30+ messages in thread
From: Stephane Eranian @ 2014-05-16 14:09 UTC (permalink / raw)
  To: Don Zickus
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, LKML, Jiri Olsa,
	Namhyung Kim, Andi Kleen

On Tue, May 13, 2014 at 6:48 PM, Don Zickus <dzickus@redhat.com> wrote:
> In perf's 'mem-mode', one can get access to a whole bunch of details specific to a
> particular sample instruction.  A bunch of those details relate to the data
> address.
>
> One interesting thing you can do with data addresses is to convert them into a unique
> cacheline they belong too.  Organizing these data cachelines into similar groups and sorting
> them can reveal cache contention.
>
> This patch creates an alogorithm based on various sample details that can help group
> entries together into data cachelines and allows 'perf report' to sort on it.
>
> The algorithm relies on having proper mmap2 support in the kernel to help determine
> if the memory map the data address belongs to is private to a pid or globally shared.
>
> The alogortithm is as follows:
>
> o group cpumodes together
> o group entries with discovered maps together
> o sort on major, minor, inode and inode generation numbers
> o if userspace anon, then sort on pid
> o sort on cachelines based on data addresses
>
> The 'dcacheline' sort option in 'perf report' only works in 'mem-mode'.
>
> Signed-off-by: Don Zickus <dzickus@redhat.com>
> ---
>  tools/perf/Documentation/perf-report.txt |   3 +-
>  tools/perf/builtin-report.c              |   2 +-
>  tools/perf/util/hist.h                   |   1 +
>  tools/perf/util/sort.c                   | 105 +++++++++++++++++++++++++++++++
>  tools/perf/util/sort.h                   |   1 +
>  5 files changed, 110 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
> index bea1c83..9115d02 100644
> --- a/tools/perf/Documentation/perf-report.txt
> +++ b/tools/perf/Documentation/perf-report.txt
> @@ -100,7 +100,7 @@ OPTIONS
>
>         If --mem-mode option is used, following sort keys are also available
>         (incompatible with --branch-stack):
> -       symbol_daddr, dso_daddr, locked, tlb, mem, snoop.
> +       symbol_daddr, dso_daddr, locked, tlb, mem, snoop, dcacheline.
>
>         - symbol_daddr: name of data symbol being executed on at the time of sample
>         - dso_daddr: name of library or module containing the data being executed
> @@ -109,6 +109,7 @@ OPTIONS
>         - tlb: type of tlb access for the data at the time of sample
>         - mem: type of memory access for the data at the time of sample
>         - snoop: type of snoop (if any) for the data at the time of sample
> +       - dcacheline: the cacheline the data address is on at the time of sample
>
>         And default sort keys are changed to local_weight, mem, sym, dso,
>         symbol_daddr, dso_daddr, snoop, tlb, locked, see '--mem-mode'.
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 89c9528..387459f 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -702,7 +702,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
>                    "sort by key(s): pid, comm, dso, symbol, parent, cpu, srcline,"
>                    " dso_to, dso_from, symbol_to, symbol_from, mispredict,"
>                    " weight, local_weight, mem, symbol_daddr, dso_daddr, tlb, "
> -                  "snoop, locked, abort, in_tx, transaction"),
> +                  "snoop, locked, abort, in_tx, transaction, dcacheline"),
>         OPT_BOOLEAN(0, "showcpuutilization", &symbol_conf.show_cpu_utilization,
>                     "Show sample percentage for different cpu modes"),
>         OPT_STRING('p', "parent", &parent_pattern, "regex",
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 38c3e87..14d1dc1 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -72,6 +72,7 @@ enum hist_column {
>         HISTC_MEM_TLB,
>         HISTC_MEM_LVL,
>         HISTC_MEM_SNOOP,
> +       HISTC_MEM_DCACHELINE,
>         HISTC_TRANSACTION,
>         HISTC_NR_COLS, /* Last entry */
>  };
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 635cd8f..0e91ba9 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -1,3 +1,4 @@
> +#include <sys/mman.h>
>  #include "sort.h"
>  #include "hist.h"
>  #include "comm.h"
> @@ -764,6 +765,102 @@ static int hist_entry__snoop_snprintf(struct hist_entry *he, char *bf,
>         return repsep_snprintf(bf, size, "%-*s", width, out);
>  }
>
> +#define CACHE_LINESIZE       64
I had something similar to your patch here in my original series for
perf mem, but I never pushed it.
I think this is a useful feature to have.
However, I don't think you can hardcode the cache line size to 64.
This is generic
code. There may be architectures where the line size is different from 64.
So I think you should add an option to change the default line size or provide
an arch-specific definition.

> +#define CLINE_OFFSET_MSK     (CACHE_LINESIZE - 1)
> +#define CLADRS(a)            ((a) & ~(CLINE_OFFSET_MSK))
> +
> +static int64_t
> +sort__dcacheline_cmp(struct hist_entry *left, struct hist_entry *right)
> +{
> +       u64 l, r;
> +       struct map *l_map, *r_map;
> +
> +       if (!left->mem_info)  return -1;
> +       if (!right->mem_info) return 1;
> +
> +       /* group event types together */
> +       if (left->cpumode > right->cpumode) return -1;
> +       if (left->cpumode < right->cpumode) return 1;
> +
> +       l_map = left->mem_info->daddr.map;
> +       r_map = right->mem_info->daddr.map;
> +
> +       /* if both are NULL, jump to sort on al_addr instead */
> +       if (!l_map && !r_map)
> +               goto addr;
> +
> +       if (!l_map) return -1;
> +       if (!r_map) return 1;
> +
> +       if (l_map->maj > r_map->maj) return -1;
> +       if (l_map->maj < r_map->maj) return 1;
> +
> +       if (l_map->min > r_map->min) return -1;
> +       if (l_map->min < r_map->min) return 1;
> +
> +       if (l_map->ino > r_map->ino) return -1;
> +       if (l_map->ino < r_map->ino) return 1;
> +
> +       if (l_map->ino_generation > r_map->ino_generation) return -1;
> +       if (l_map->ino_generation < r_map->ino_generation) return 1;
> +
> +       /*
> +        * Addresses with no major/minor numbers are assumed to be
> +        * anonymous in userspace.  Sort those on pid then address.
> +        *
> +        * The kernel and non-zero major/minor mapped areas are
> +        * assumed to be unity mapped.  Sort those on address.
> +        */
> +
> +       if ((left->cpumode != PERF_RECORD_MISC_KERNEL) &&
> +           (!(l_map->flags & MAP_SHARED)) &&
> +           !l_map->maj && !l_map->min && !l_map->ino &&
> +           !l_map->ino_generation) {
> +               /* userspace anonymous */
> +
> +               if (left->thread->pid_ > right->thread->pid_) return -1;
> +               if (left->thread->pid_ < right->thread->pid_) return 1;
> +       }
> +
> +addr:
> +       /* al_addr does all the right addr - start + offset calculations */
> +       l = CLADRS(left->mem_info->daddr.al_addr);
> +       r = CLADRS(right->mem_info->daddr.al_addr);
> +
> +       if (l > r) return -1;
> +       if (l < r) return 1;
> +
> +       return 0;
> +}
> +
> +static int hist_entry__dcacheline_snprintf(struct hist_entry *he, char *bf,
> +                                         size_t size, unsigned int width)
> +{
> +
> +       uint64_t addr = 0;
> +       struct map *map = NULL;
> +       struct symbol *sym = NULL;
> +       char level = he->level;
> +
> +       if (he->mem_info) {
> +               addr = CLADRS(he->mem_info->daddr.al_addr);
> +               map = he->mem_info->daddr.map;
> +               sym = he->mem_info->daddr.sym;
> +
> +               /* print [s] for shared data mmaps */
> +               if ((he->cpumode != PERF_RECORD_MISC_KERNEL) &&
> +                    map && (map->type == MAP__VARIABLE) &&
> +                   (map->flags & MAP_SHARED) &&
> +                   (map->maj || map->min || map->ino ||
> +                    map->ino_generation))
> +                       level = 's';
> +               else if (!map)
> +                       level = 'X';
> +       }
> +       return _hist_entry__sym_snprintf(map, sym, addr, level, bf, size,
> +                                        width);
> +}
> +
>  struct sort_entry sort_mispredict = {
>         .se_header      = "Branch Mispredicted",
>         .se_cmp         = sort__mispredict_cmp,
> @@ -856,6 +953,13 @@ struct sort_entry sort_mem_snoop = {
>         .se_width_idx   = HISTC_MEM_SNOOP,
>  };
>
> +struct sort_entry sort_mem_dcacheline = {
> +       .se_header      = "Data Cacheline         ",
> +       .se_cmp         = sort__dcacheline_cmp,
> +       .se_snprintf    = hist_entry__dcacheline_snprintf,
> +       .se_width_idx   = HISTC_MEM_DCACHELINE,
> +};
> +
>  static int64_t
>  sort__abort_cmp(struct hist_entry *left, struct hist_entry *right)
>  {
> @@ -1023,6 +1127,7 @@ static struct sort_dimension memory_sort_dimensions[] = {
>         DIM(SORT_MEM_TLB, "tlb", sort_mem_tlb),
>         DIM(SORT_MEM_LVL, "mem", sort_mem_lvl),
>         DIM(SORT_MEM_SNOOP, "snoop", sort_mem_snoop),
> +       DIM(SORT_MEM_DCACHELINE, "dcacheline", sort_mem_dcacheline),
>  };
>
>  #undef DIM
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index 22cf912..460ec9c 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -167,6 +167,7 @@ enum sort_type {
>         SORT_MEM_TLB,
>         SORT_MEM_LVL,
>         SORT_MEM_SNOOP,
> +       SORT_MEM_DCACHELINE,
>  };
>
>  /*
> --
> 1.7.11.7
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-16 14:05       ` Jiri Olsa
@ 2014-05-16 14:30         ` Don Zickus
  2014-05-19 13:34           ` Jiri Olsa
  0 siblings, 1 reply; 30+ messages in thread
From: Don Zickus @ 2014-05-16 14:30 UTC (permalink / raw)
  To: Jiri Olsa; +Cc: acme, peterz, LKML, namhyung, eranian, Andi Kleen

On Fri, May 16, 2014 at 04:05:51PM +0200, Jiri Olsa wrote:
> On Fri, May 16, 2014 at 09:30:58AM -0400, Don Zickus wrote:
> > On Fri, May 16, 2014 at 01:47:57PM +0200, Jiri Olsa wrote:
> > > On Tue, May 13, 2014 at 12:48:17PM -0400, Don Zickus wrote:
> > > > In perf's 'mem-mode', one can get access to a whole bunch of details specific to a
> > > > particular sample instruction.  A bunch of those details relate to the data
> > > > address.
> > > > 
> > > > One interesting thing you can do with data addresses is to convert them into a unique
> > > > cacheline they belong too.  Organizing these data cachelines into similar groups and sorting
> > > > them can reveal cache contention.
> > > > 
> > > > This patch creates an alogorithm based on various sample details that can help group
> > > > entries together into data cachelines and allows 'perf report' to sort on it.
> > > > 
> > > > The algorithm relies on having proper mmap2 support in the kernel to help determine
> > > > if the memory map the data address belongs to is private to a pid or globally shared.
> > > > 
> > > > The alogortithm is as follows:
> > > > 
> > > > o group cpumodes together
> > > > o group entries with discovered maps together
> > > > o sort on major, minor, inode and inode generation numbers
> > > > o if userspace anon, then sort on pid
> > > > o sort on cachelines based on data addresses
> > > 
> > > needs some collumn width refresh or something..? ;-)
> > 
> > Not sure what you mean here.
> > 
> > > 
> > > # Overhead  Data Cacheline         
> > > # ........  .......................
> 
> header not being wide enough to cover the longest data

Ah. Ok.  So I am not sure the right way to fix that.  As the current
header seems to be hardcoded with a bunch of spaces.  Is there a trick to
dynamically space it correctly based on the data provided?

> 
> 
> > > #
> > >      5.42%  [k] 0xffff8801ed832c40 
> > >      5.29%  [.] sys_errlist@@GLIBC_2.12+0xffffffcbf7dfc1ff                       
> > >      3.16%  [k] 0xffffffffff5690c0 
> > > 
> > > 
> > > also I've got again perf hanged up on opening device file
> > > 
> > > [jolsa@krava perf]$ sudo strace -p 29445
> > > Process 29445 attached
> > > open("/dev/snd/pcmC0D0p", O_RDONLY^CProcess 29445 detached
> > > 
> > > another one I recall was /dev/dri/card0 touched by X server
> > > 
> > > I guess those device files allow to mmap memory and we recorded
> > > memory access there.. we need check for this and do not try to
> > > open device files
> > 
> > Ok.  And that problem doesn't happen when my patch is not applied?  I am
> > not sure how this patch causes open device hangs.  I'll try to run this on
> > a box with X server running to duplicate.
> 
> I think it came with the memory profiling, because we treat
> data areas as dsos.. open and look for symbols

Yeah, I figured that too.  I guess I was trying to point out this is a
generic memory profiling issue that isn't related to my patch.  But I will
still try to track down the problem as it needs to be fixed. :-)

Cheers,
Don

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-16 14:09   ` Stephane Eranian
@ 2014-05-16 15:18     ` Don Zickus
  2014-05-16 15:59     ` Peter Zijlstra
  1 sibling, 0 replies; 30+ messages in thread
From: Don Zickus @ 2014-05-16 15:18 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, LKML, Jiri Olsa,
	Namhyung Kim, Andi Kleen

On Fri, May 16, 2014 at 04:09:59PM +0200, Stephane Eranian wrote:
> > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> > index 635cd8f..0e91ba9 100644
> > --- a/tools/perf/util/sort.c
> > +++ b/tools/perf/util/sort.c
> > @@ -1,3 +1,4 @@
> > +#include <sys/mman.h>
> >  #include "sort.h"
> >  #include "hist.h"
> >  #include "comm.h"
> > @@ -764,6 +765,102 @@ static int hist_entry__snoop_snprintf(struct hist_entry *he, char *bf,
> >         return repsep_snprintf(bf, size, "%-*s", width, out);
> >  }
> >
> > +#define CACHE_LINESIZE       64
> I had something similar to your patch here in my original series for
> perf mem, but I never pushed it.
> I think this is a useful feature to have.
> However, I don't think you can hardcode the cache line size to 64.
> This is generic
> code. There may be architectures where the line size is different from 64.
> So I think you should add an option to change the default line size or provide
> an arch-specific definition.

I agree.  I'll dig up a way to do that.  Thanks!

Cheers,
Don

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/6] events, perf: Pass protection and flags bits through mmap2 interface
  2014-05-16 13:33     ` Don Zickus
@ 2014-05-16 15:45       ` Peter Zijlstra
  2014-05-16 16:20         ` Don Zickus
  0 siblings, 1 reply; 30+ messages in thread
From: Peter Zijlstra @ 2014-05-16 15:45 UTC (permalink / raw)
  To: Don Zickus; +Cc: acme, LKML, jolsa, namhyung, eranian, Andi Kleen

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

On Fri, May 16, 2014 at 09:33:00AM -0400, Don Zickus wrote:
> On Fri, May 16, 2014 at 02:22:19PM +0200, Peter Zijlstra wrote:
> > On Tue, May 13, 2014 at 12:48:12PM -0400, Don Zickus wrote:
> > > From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > 
> > > The mmap2 interface was missing the protection and flags bits needed to
> > > accurately determine if a mmap memory area was shared or private and
> > > if it was readable or not.
> > > 
> > > [tweaked patch to compile and wrote changelog]
> > > Signed-off-by: Don Zickus <dzickus@redhat.com>
> > > 
> > > --
> > > Peter you mentioned writing up a changelog for this patch, but you
> > > seemed busy, so I jumped the gun a little bit.  Let me know how you
> > > want to do this patch if this approach is wrong.
> > 
> > Oh, right, totally forgot about that. yeah I think the blub you have is
> > adequate. 
> 
> I realized I didn't put your sign-off even though I said the patch is from
> you.  Should I repost with that?  I don't know who is going to take what
> patches from this series.

I asked jolsa to pick the lot up since its mostly userspace, if he
remembers he can put it in.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-16 14:09   ` Stephane Eranian
  2014-05-16 15:18     ` Don Zickus
@ 2014-05-16 15:59     ` Peter Zijlstra
  2014-05-16 16:02       ` Stephane Eranian
  1 sibling, 1 reply; 30+ messages in thread
From: Peter Zijlstra @ 2014-05-16 15:59 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: Don Zickus, Arnaldo Carvalho de Melo, LKML, Jiri Olsa,
	Namhyung Kim, Andi Kleen

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

On Fri, May 16, 2014 at 04:09:59PM +0200, Stephane Eranian wrote:
> > +#define CACHE_LINESIZE       64
> I had something similar to your patch here in my original series for
> perf mem, but I never pushed it.
> I think this is a useful feature to have.
> However, I don't think you can hardcode the cache line size to 64.
> This is generic
> code. There may be architectures where the line size is different from 64.
> So I think you should add an option to change the default line size or provide
> an arch-specific definition.

# cat /sys/devices/system/cpu/cpu0/cache/index0/coherency_line_size     
64


[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-16 15:59     ` Peter Zijlstra
@ 2014-05-16 16:02       ` Stephane Eranian
  2014-05-16 16:24         ` Don Zickus
  0 siblings, 1 reply; 30+ messages in thread
From: Stephane Eranian @ 2014-05-16 16:02 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Don Zickus, Arnaldo Carvalho de Melo, LKML, Jiri Olsa,
	Namhyung Kim, Andi Kleen

On Fri, May 16, 2014 at 5:59 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, May 16, 2014 at 04:09:59PM +0200, Stephane Eranian wrote:
>> > +#define CACHE_LINESIZE       64
>> I had something similar to your patch here in my original series for
>> perf mem, but I never pushed it.
>> I think this is a useful feature to have.
>> However, I don't think you can hardcode the cache line size to 64.
>> This is generic
>> code. There may be architectures where the line size is different from 64.
>> So I think you should add an option to change the default line size or provide
>> an arch-specific definition.
>
> # cat /sys/devices/system/cpu/cpu0/cache/index0/coherency_line_size
> 64
Excellent, then we should use that!

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/6] events, perf: Pass protection and flags bits through mmap2 interface
  2014-05-16 15:45       ` Peter Zijlstra
@ 2014-05-16 16:20         ` Don Zickus
  0 siblings, 0 replies; 30+ messages in thread
From: Don Zickus @ 2014-05-16 16:20 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: acme, LKML, jolsa, namhyung, eranian, Andi Kleen

On Fri, May 16, 2014 at 05:45:38PM +0200, Peter Zijlstra wrote:
> On Fri, May 16, 2014 at 09:33:00AM -0400, Don Zickus wrote:
> > On Fri, May 16, 2014 at 02:22:19PM +0200, Peter Zijlstra wrote:
> > > On Tue, May 13, 2014 at 12:48:12PM -0400, Don Zickus wrote:
> > > > From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > > 
> > > > The mmap2 interface was missing the protection and flags bits needed to
> > > > accurately determine if a mmap memory area was shared or private and
> > > > if it was readable or not.
> > > > 
> > > > [tweaked patch to compile and wrote changelog]
> > > > Signed-off-by: Don Zickus <dzickus@redhat.com>
> > > > 
> > > > --
> > > > Peter you mentioned writing up a changelog for this patch, but you
> > > > seemed busy, so I jumped the gun a little bit.  Let me know how you
> > > > want to do this patch if this approach is wrong.
> > > 
> > > Oh, right, totally forgot about that. yeah I think the blub you have is
> > > adequate. 
> > 
> > I realized I didn't put your sign-off even though I said the patch is from
> > you.  Should I repost with that?  I don't know who is going to take what
> > patches from this series.
> 
> I asked jolsa to pick the lot up since its mostly userspace, if he
> remembers he can put it in.

Yup he told me the samething on irc.  Thanks!!

Cheers,
Don


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-16 16:02       ` Stephane Eranian
@ 2014-05-16 16:24         ` Don Zickus
  2014-05-16 16:27           ` Stephane Eranian
  2014-05-19 11:25           ` Jiri Olsa
  0 siblings, 2 replies; 30+ messages in thread
From: Don Zickus @ 2014-05-16 16:24 UTC (permalink / raw)
  To: Stephane Eranian
  Cc: Peter Zijlstra, Arnaldo Carvalho de Melo, LKML, Jiri Olsa,
	Namhyung Kim, Andi Kleen

On Fri, May 16, 2014 at 06:02:43PM +0200, Stephane Eranian wrote:
> On Fri, May 16, 2014 at 5:59 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Fri, May 16, 2014 at 04:09:59PM +0200, Stephane Eranian wrote:
> >> > +#define CACHE_LINESIZE       64
> >> I had something similar to your patch here in my original series for
> >> perf mem, but I never pushed it.
> >> I think this is a useful feature to have.
> >> However, I don't think you can hardcode the cache line size to 64.
> >> This is generic
> >> code. There may be architectures where the line size is different from 64.
> >> So I think you should add an option to change the default line size or provide
> >> an arch-specific definition.
> >
> > # cat /sys/devices/system/cpu/cpu0/cache/index0/coherency_line_size
> > 64
> Excellent, then we should use that!

Would it make sense to create an accessory function that sets the size in
util/cpumap.c and provides it as an inline from util/cpumap.h?

Cheers,
Don


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-16 16:24         ` Don Zickus
@ 2014-05-16 16:27           ` Stephane Eranian
  2014-05-19 11:25           ` Jiri Olsa
  1 sibling, 0 replies; 30+ messages in thread
From: Stephane Eranian @ 2014-05-16 16:27 UTC (permalink / raw)
  To: Don Zickus
  Cc: Peter Zijlstra, Arnaldo Carvalho de Melo, LKML, Jiri Olsa,
	Namhyung Kim, Andi Kleen

On Fri, May 16, 2014 at 6:24 PM, Don Zickus <dzickus@redhat.com> wrote:
> On Fri, May 16, 2014 at 06:02:43PM +0200, Stephane Eranian wrote:
>> On Fri, May 16, 2014 at 5:59 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Fri, May 16, 2014 at 04:09:59PM +0200, Stephane Eranian wrote:
>> >> > +#define CACHE_LINESIZE       64
>> >> I had something similar to your patch here in my original series for
>> >> perf mem, but I never pushed it.
>> >> I think this is a useful feature to have.
>> >> However, I don't think you can hardcode the cache line size to 64.
>> >> This is generic
>> >> code. There may be architectures where the line size is different from 64.
>> >> So I think you should add an option to change the default line size or provide
>> >> an arch-specific definition.
>> >
>> > # cat /sys/devices/system/cpu/cpu0/cache/index0/coherency_line_size
>> > 64
>> Excellent, then we should use that!
>
> Would it make sense to create an accessory function that sets the size in
> util/cpumap.c and provides it as an inline from util/cpumap.h?
>
I am fine with it.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/6] Revert "perf: Disable PERF_RECORD_MMAP2 support"
  2014-05-13 16:48 ` [PATCH 2/6] Revert "perf: Disable PERF_RECORD_MMAP2 support" Don Zickus
  2014-05-16 11:25   ` Jiri Olsa
@ 2014-05-19 11:21   ` Jiri Olsa
  1 sibling, 0 replies; 30+ messages in thread
From: Jiri Olsa @ 2014-05-19 11:21 UTC (permalink / raw)
  To: Don Zickus; +Cc: acme, peterz, LKML, namhyung, eranian, Andi Kleen

On Tue, May 13, 2014 at 12:48:13PM -0400, Don Zickus wrote:
> This reverts commit 3090ffb5a2515990182f3f55b0688a7817325488.
> 
> Conflicts:
> 	tools/perf/util/event.c

also besides the missing Signed-off ;-) could I have more
info in changelog, like we revert this now etc..?

thanks,
jirka

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-16 16:24         ` Don Zickus
  2014-05-16 16:27           ` Stephane Eranian
@ 2014-05-19 11:25           ` Jiri Olsa
  2014-05-19 13:20             ` Don Zickus
  1 sibling, 1 reply; 30+ messages in thread
From: Jiri Olsa @ 2014-05-19 11:25 UTC (permalink / raw)
  To: Don Zickus
  Cc: Stephane Eranian, Peter Zijlstra, Arnaldo Carvalho de Melo, LKML,
	Namhyung Kim, Andi Kleen

On Fri, May 16, 2014 at 12:24:41PM -0400, Don Zickus wrote:
> On Fri, May 16, 2014 at 06:02:43PM +0200, Stephane Eranian wrote:
> > On Fri, May 16, 2014 at 5:59 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > On Fri, May 16, 2014 at 04:09:59PM +0200, Stephane Eranian wrote:
> > >> > +#define CACHE_LINESIZE       64
> > >> I had something similar to your patch here in my original series for
> > >> perf mem, but I never pushed it.
> > >> I think this is a useful feature to have.
> > >> However, I don't think you can hardcode the cache line size to 64.
> > >> This is generic
> > >> code. There may be architectures where the line size is different from 64.
> > >> So I think you should add an option to change the default line size or provide
> > >> an arch-specific definition.
> > >
> > > # cat /sys/devices/system/cpu/cpu0/cache/index0/coherency_line_size
> > > 64
> > Excellent, then we should use that!
> 
> Would it make sense to create an accessory function that sets the size in
> util/cpumap.c and provides it as an inline from util/cpumap.h?

sounds good,

btw so far I'll take patches 1-4 (after getting changelog update)
and expecting v2 for 5,6 ;-)

thanks,
jirka

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/6] Revert "perf: Disable PERF_RECORD_MMAP2 support"
  2014-05-16 13:26     ` Don Zickus
@ 2014-05-19 11:45       ` Jiri Olsa
  0 siblings, 0 replies; 30+ messages in thread
From: Jiri Olsa @ 2014-05-19 11:45 UTC (permalink / raw)
  To: Don Zickus; +Cc: acme, peterz, LKML, namhyung, eranian, Andi Kleen

On Fri, May 16, 2014 at 09:26:30AM -0400, Don Zickus wrote:
> On Fri, May 16, 2014 at 01:25:14PM +0200, Jiri Olsa wrote:
> > On Tue, May 13, 2014 at 12:48:13PM -0400, Don Zickus wrote:
> > 
> > SNIP
> > 
> > > -		/*
> > > - 		 * Anon maps don't have the execname.
> > > - 		 */
> > > -		if (n < 4)
> > > +		n = sscanf(bf, "%"PRIx64"-%"PRIx64" %s %"PRIx64" %x:%x %u %s\n",
> > > +		       &event->mmap2.start, &event->mmap2.len, prot,
> > > +		       &event->mmap2.pgoff, &event->mmap2.maj,
> > > +		       &event->mmap2.min,
> > > +		       &ino, execname);
> > > +
> > > +		event->mmap2.ino = (u64)ino;
> > > +
> > > +		if (n < 7)
> > >  			continue;
> > 
> > any reason for changing this from 'if (n != 8)' ?
> 
> Yes, read 9d4ecc8893832337daf241236841db966fa53489. :-)
> 
> This wasn't a clean revert because of the change for (n != 5) -> (n < 4).
> So I maintainted the spirit of the upstream code with this revert.  Let me
> know if that is wrong.

ook, I found another issue.. perf test 25 is failing, because
it's using perf_event__synthesize_mmap_events to store map events
plus machine__process_mmap_event to feed them into machine object

it needs to use machine__process_mmap2_event now, because
perf_event__synthesize_mmap_events uses MMAP2 events now
please check attached patch

I think it's better to have this hunk in this patch, so we dont
break tests.. also please run them ebfore posting patch

please update the changelog to have some info about this.. plus
the Revert reason info and plus the 'if (n != 8)' thing ;-)

thanks,
jirka

---
diff --git a/tools/perf/tests/dwarf-unwind.c b/tools/perf/tests/dwarf-unwind.c
index 108f0cd..96adb73 100644
--- a/tools/perf/tests/dwarf-unwind.c
+++ b/tools/perf/tests/dwarf-unwind.c
@@ -15,7 +15,7 @@ static int mmap_handler(struct perf_tool *tool __maybe_unused,
 			struct perf_sample *sample __maybe_unused,
 			struct machine *machine)
 {
-	return machine__process_mmap_event(machine, event, NULL);
+	return machine__process_mmap2_event(machine, event, NULL);
 }
 
 static int init_live_machine(struct machine *machine)

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-19 11:25           ` Jiri Olsa
@ 2014-05-19 13:20             ` Don Zickus
  0 siblings, 0 replies; 30+ messages in thread
From: Don Zickus @ 2014-05-19 13:20 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Stephane Eranian, Peter Zijlstra, Arnaldo Carvalho de Melo, LKML,
	Namhyung Kim, Andi Kleen

On Mon, May 19, 2014 at 01:25:45PM +0200, Jiri Olsa wrote:
> On Fri, May 16, 2014 at 12:24:41PM -0400, Don Zickus wrote:
> > On Fri, May 16, 2014 at 06:02:43PM +0200, Stephane Eranian wrote:
> > > On Fri, May 16, 2014 at 5:59 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > > On Fri, May 16, 2014 at 04:09:59PM +0200, Stephane Eranian wrote:
> > > >> > +#define CACHE_LINESIZE       64
> > > >> I had something similar to your patch here in my original series for
> > > >> perf mem, but I never pushed it.
> > > >> I think this is a useful feature to have.
> > > >> However, I don't think you can hardcode the cache line size to 64.
> > > >> This is generic
> > > >> code. There may be architectures where the line size is different from 64.
> > > >> So I think you should add an option to change the default line size or provide
> > > >> an arch-specific definition.
> > > >
> > > > # cat /sys/devices/system/cpu/cpu0/cache/index0/coherency_line_size
> > > > 64
> > > Excellent, then we should use that!
> > 
> > Would it make sense to create an accessory function that sets the size in
> > util/cpumap.c and provides it as an inline from util/cpumap.h?
> 
> sounds good,
> 
> btw so far I'll take patches 1-4 (after getting changelog update)
> and expecting v2 for 5,6 ;-)

Ok. Thanks!  What needs to be done for patch 5?  I have a patch 5.5?? that
does the cachline calculation that Stephane requested.  I just have to
find a way to dynamically handle the symbol lengths on output (and fix
that hang you see).

Cheers,
Don

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-16 14:30         ` Don Zickus
@ 2014-05-19 13:34           ` Jiri Olsa
  2014-05-19 14:18             ` Don Zickus
  0 siblings, 1 reply; 30+ messages in thread
From: Jiri Olsa @ 2014-05-19 13:34 UTC (permalink / raw)
  To: Don Zickus; +Cc: acme, peterz, LKML, namhyung, eranian, Andi Kleen

On Fri, May 16, 2014 at 10:30:02AM -0400, Don Zickus wrote:
> On Fri, May 16, 2014 at 04:05:51PM +0200, Jiri Olsa wrote:
> > On Fri, May 16, 2014 at 09:30:58AM -0400, Don Zickus wrote:
> > > On Fri, May 16, 2014 at 01:47:57PM +0200, Jiri Olsa wrote:
> > > > On Tue, May 13, 2014 at 12:48:17PM -0400, Don Zickus wrote:
> > > > > In perf's 'mem-mode', one can get access to a whole bunch of details specific to a
> > > > > particular sample instruction.  A bunch of those details relate to the data
> > > > > address.
> > > > > 
> > > > > One interesting thing you can do with data addresses is to convert them into a unique
> > > > > cacheline they belong too.  Organizing these data cachelines into similar groups and sorting
> > > > > them can reveal cache contention.
> > > > > 
> > > > > This patch creates an alogorithm based on various sample details that can help group
> > > > > entries together into data cachelines and allows 'perf report' to sort on it.
> > > > > 
> > > > > The algorithm relies on having proper mmap2 support in the kernel to help determine
> > > > > if the memory map the data address belongs to is private to a pid or globally shared.
> > > > > 
> > > > > The alogortithm is as follows:
> > > > > 
> > > > > o group cpumodes together
> > > > > o group entries with discovered maps together
> > > > > o sort on major, minor, inode and inode generation numbers
> > > > > o if userspace anon, then sort on pid
> > > > > o sort on cachelines based on data addresses
> > > > 
> > > > needs some collumn width refresh or something..? ;-)
> > > 
> > > Not sure what you mean here.
> > > 
> > > > 
> > > > # Overhead  Data Cacheline         
> > > > # ........  .......................
> > 
> > header not being wide enough to cover the longest data
> 
> Ah. Ok.  So I am not sure the right way to fix that.  As the current
> header seems to be hardcoded with a bunch of spaces.  Is there a trick to
> dynamically space it correctly based on the data provided?

it should be enough to update function:
  void hists__calc_col_len(struct hists *hists, struct hist_entry *h)

with HISTC_MEM_DCACHELINE column update code, same as the rest

jirka

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 6/6] perf: Add dcacheline sort
  2014-05-19 13:34           ` Jiri Olsa
@ 2014-05-19 14:18             ` Don Zickus
  0 siblings, 0 replies; 30+ messages in thread
From: Don Zickus @ 2014-05-19 14:18 UTC (permalink / raw)
  To: Jiri Olsa; +Cc: acme, peterz, LKML, namhyung, eranian, Andi Kleen

On Mon, May 19, 2014 at 03:34:14PM +0200, Jiri Olsa wrote:
> On Fri, May 16, 2014 at 10:30:02AM -0400, Don Zickus wrote:
> > On Fri, May 16, 2014 at 04:05:51PM +0200, Jiri Olsa wrote:
> > > On Fri, May 16, 2014 at 09:30:58AM -0400, Don Zickus wrote:
> > > > On Fri, May 16, 2014 at 01:47:57PM +0200, Jiri Olsa wrote:
> > > > > On Tue, May 13, 2014 at 12:48:17PM -0400, Don Zickus wrote:
> > > > > > In perf's 'mem-mode', one can get access to a whole bunch of details specific to a
> > > > > > particular sample instruction.  A bunch of those details relate to the data
> > > > > > address.
> > > > > > 
> > > > > > One interesting thing you can do with data addresses is to convert them into a unique
> > > > > > cacheline they belong too.  Organizing these data cachelines into similar groups and sorting
> > > > > > them can reveal cache contention.
> > > > > > 
> > > > > > This patch creates an alogorithm based on various sample details that can help group
> > > > > > entries together into data cachelines and allows 'perf report' to sort on it.
> > > > > > 
> > > > > > The algorithm relies on having proper mmap2 support in the kernel to help determine
> > > > > > if the memory map the data address belongs to is private to a pid or globally shared.
> > > > > > 
> > > > > > The alogortithm is as follows:
> > > > > > 
> > > > > > o group cpumodes together
> > > > > > o group entries with discovered maps together
> > > > > > o sort on major, minor, inode and inode generation numbers
> > > > > > o if userspace anon, then sort on pid
> > > > > > o sort on cachelines based on data addresses
> > > > > 
> > > > > needs some collumn width refresh or something..? ;-)
> > > > 
> > > > Not sure what you mean here.
> > > > 
> > > > > 
> > > > > # Overhead  Data Cacheline         
> > > > > # ........  .......................
> > > 
> > > header not being wide enough to cover the longest data
> > 
> > Ah. Ok.  So I am not sure the right way to fix that.  As the current
> > header seems to be hardcoded with a bunch of spaces.  Is there a trick to
> > dynamically space it correctly based on the data provided?
> 
> it should be enough to update function:
>   void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
> 
> with HISTC_MEM_DCACHELINE column update code, same as the rest

Ah, yes that worked perfectly.  Will repost soon. :-)

Cheers,
Don

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2014-05-19 14:19 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-13 16:48 [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting Don Zickus
2014-05-13 16:48 ` [PATCH 1/6] events, perf: Pass protection and flags bits through mmap2 interface Don Zickus
2014-05-16 12:22   ` Peter Zijlstra
2014-05-16 13:33     ` Don Zickus
2014-05-16 15:45       ` Peter Zijlstra
2014-05-16 16:20         ` Don Zickus
2014-05-13 16:48 ` [PATCH 2/6] Revert "perf: Disable PERF_RECORD_MMAP2 support" Don Zickus
2014-05-16 11:25   ` Jiri Olsa
2014-05-16 13:26     ` Don Zickus
2014-05-19 11:45       ` Jiri Olsa
2014-05-19 11:21   ` Jiri Olsa
2014-05-13 16:48 ` [PATCH 3/6] perf: Update mmap2 interface with protection and flag bits Don Zickus
2014-05-13 16:48 ` [PATCH 4/6] perf report: Add mem-mode documentation to report command Don Zickus
2014-05-13 16:48 ` [PATCH 5/6] perf: Add cpumode to struct hist_entry Don Zickus
2014-05-13 16:48 ` [PATCH 6/6] perf: Add dcacheline sort Don Zickus
2014-05-16 11:47   ` Jiri Olsa
2014-05-16 13:30     ` Don Zickus
2014-05-16 14:05       ` Jiri Olsa
2014-05-16 14:30         ` Don Zickus
2014-05-19 13:34           ` Jiri Olsa
2014-05-19 14:18             ` Don Zickus
2014-05-16 14:09   ` Stephane Eranian
2014-05-16 15:18     ` Don Zickus
2014-05-16 15:59     ` Peter Zijlstra
2014-05-16 16:02       ` Stephane Eranian
2014-05-16 16:24         ` Don Zickus
2014-05-16 16:27           ` Stephane Eranian
2014-05-19 11:25           ` Jiri Olsa
2014-05-19 13:20             ` Don Zickus
2014-05-13 16:52 ` [PATCH 0/6 V2] perf: Enable mmap2 and add dcacheline sorting Don Zickus

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.