[PATCH 00/11] perf tool: assorted cleanups and bug fixes

[PATCH 00/11] perf tool: assorted cleanups and bug fixes

[David Ahern]

Arnaldo:

Welcome back from vacation. Some cleanups and bug fixes for 3.6.
Patch 8 should apply to stable if Gleb's and Peter's patches
are backported as well.
 

David Ahern (11):
  perf tool: add machine id to modules debug message
  perf kvm: set name for VM process in guest machine
  perf kvm: guest userspace samples should not be lumped with host
    uspace
  perf kvm: fix bug resolving guest kernel syms - v2
  perf kvm: use strtol for walking guestmount directory
  perf kvm: limit repetitive guestmount message to once per directory
  perf tools: dump exclude_{guest,host}, precise_ip header info too
  perf tool: precise mode requires exclude_guest
  perf top: error handling for counter creation should parallel
    perf-record
  perf tool: give user better message if precise is not supported
  perf kvm top: limit guest kernel info message to once

 tools/perf/builtin-record.c    |    4 ++++
 tools/perf/builtin-top.c       |   24 +++++++++++++++++++----
 tools/perf/util/header.c       |    6 ++++++
 tools/perf/util/map.c          |   41 ++++++++++++++++++++++++++++++++++++++--
 tools/perf/util/map.h          |    1 +
 tools/perf/util/parse-events.c |    3 +++
 tools/perf/util/session.c      |    5 ++++-
 tools/perf/util/symbol.c       |   21 +++++++++++++++++---
 8 files changed, 95 insertions(+), 10 deletions(-)

-- 
1.7.10.1

[PATCH 01/11] perf tool: add machine id to modules debug message

[David Ahern]

Current debug message is:
Problems creating module maps, continuing anyway...

When running multiple VMs it would be nice to know which machine the
message is referring to:

$ perf kvm --guest --guestmount=/tmp/guest-mount record -av -- sleep 10
Problems creating module maps for guest 6613, continuing anyway...

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 tools/perf/util/symbol.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 90f6951..6802ef4 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2567,8 +2567,15 @@ int machine__create_kernel_maps(struct machine *machine)
 	    __machine__create_kernel_maps(machine, kernel) < 0)
 		return -1;
 
-	if (symbol_conf.use_modules && machine__create_modules(machine) < 0)
-		pr_debug("Problems creating module maps, continuing anyway...\n");
+	if (symbol_conf.use_modules && machine__create_modules(machine) < 0) {
+		if (machine__is_host(machine))
+			pr_debug("Problems creating module maps, "
+				 "continuing anyway...\n");
+		else
+			pr_debug("Problems creating module maps for guest %d, "
+				 "continuing anyway...\n", machine->pid);
+	}
+
 	/*
 	 * Now that we have all the maps created, just set the ->end of them:
 	 */
-- 
1.7.10.1

[PATCH 02/11] perf kvm: set name for VM process in guest machine

[David Ahern]

COMM events are not generated in the context of a guest machine, so the
thread name is never set for the VMM process. For example, the qemu-kvm
name applies to the process in the host machine, not the guest machine.
So, samples for guest machines are currently displayed as:

    99.67%     :5671  [unknown]         [g] 0xffffffff81366b41

where 5671 is the pid of the VMM. With this patch the samples in the guest
machine are shown as:

    18.43%  [guest/5671]  [unknown]           [g] 0xffffffff810d68b7

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 tools/perf/util/map.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index a1f4e36..8668569 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -7,6 +7,7 @@
 #include <stdio.h>
 #include <unistd.h>
 #include "map.h"
+#include "thread.h"
 
 const char *map_type__name[MAP__NR_TYPES] = {
 	[MAP__FUNCTION] = "Functions",
@@ -585,7 +586,21 @@ int machine__init(struct machine *self, const char *root_dir, pid_t pid)
 	self->kmaps.machine = self;
 	self->pid	    = pid;
 	self->root_dir      = strdup(root_dir);
-	return self->root_dir == NULL ? -ENOMEM : 0;
+	if (self->root_dir == NULL)
+		return -ENOMEM;
+
+	if (pid != HOST_KERNEL_ID) {
+		struct thread *thread = machine__findnew_thread(self, pid);
+		char comm[64];
+
+		if (thread == NULL)
+			return -ENOMEM;
+
+		snprintf(comm, sizeof(comm), "[guest/%d]", pid);
+		thread__set_comm(thread, comm);
+	}
+
+	return 0;
 }
 
 static void dsos__delete(struct list_head *self)
-- 
1.7.10.1

[PATCH 03/11] perf kvm: guest userspace samples should not be lumped with host uspace

[David Ahern]

e.g., perf kvm --host  --guest report -i perf.data --stdio -D
shows:

1 599127912065356 0x143b8 [0x48]: PERF_RECORD_SAMPLE(IP, 5): 5671/5676: 0x7fdf95a061c0 period: 1 addr: 0
... chain: nr:2
.....  0: ffffffffffffff80
.....  1: fffffffffffffe00
 ... thread: qemu-kvm:5671
 ...... dso: <not found>

(IP, 5) means sample in guest userspace. Those samples should not be lumped
into the VMM's host thread. i.e, the report output:

    56.86%  qemu-kvm  [unknown]         [u] 0x00007fdf95a061c0

With this patch the output emphasizes it is a guest userspace hit:

    56.86%  [guest/5671]  [unknown]         [u] 0x00007fdf95a061c0

Looking at 3 VMs (2 64-bit, 1 32-bit) with each running a CPU bound
process (openssl speed), perf report currently shows:

  93.84%  117726   qemu-kvm  [unknown]   [u] 0x00007fd7dcaea8e5

which is wrong. With this patch you get:

  31.50%   39258   [guest/18772]  [unknown]   [u] 0x00007fd7dcaea8e5
  31.50%   39236   [guest/11230]  [unknown]   [u] 0x0000000000a57340
  30.84%   39232   [guest/18395]  [unknown]   [u] 0x00007f66f641e107

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 tools/perf/util/session.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 8e48559..90ee39d 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -918,7 +918,9 @@ static struct machine *
 {
 	const u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
 
-	if (cpumode == PERF_RECORD_MISC_GUEST_KERNEL && perf_guest) {
+	if (perf_guest &&
+	    ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
+	     (cpumode == PERF_RECORD_MISC_GUEST_USER))) {
 		u32 pid;
 
 		if (event->header.type == PERF_RECORD_MMAP)
-- 
1.7.10.1

[PATCH 04/11] perf kvm: fix bug resolving guest kernel syms - v2

[David Ahern]

Guest kernel symbols are not resolved despite passing the information
needed to resolve them. e.g.,

perf kvm --guest --guestmount=/tmp/guest-mount record -a -- sleep 1
perf kvm --guest --guestmount=/tmp/guest-mount report --stdio

    36.55%  [guest/11399]  [unknown]         [g] 0xffffffff81600bc8
    33.19%  [guest/10474]  [unknown]         [g] 0x00000000c0116e00
    30.26%  [guest/11094]  [unknown]         [g] 0xffffffff8100a288
    43.69%  [guest/10474]  [unknown]         [g] 0x00000000c0103d90
    37.38%  [guest/11399]  [unknown]         [g] 0xffffffff81600bc8
    12.24%  [guest/11094]  [unknown]         [g] 0xffffffff810aa91d
     6.69%  [guest/11094]  [unknown]         [u] 0x00007fa784d721c3

which is just pathetic.

After a maddening 2 days sifting through perf minutia I found it --
id_hdr_size is not initialized for guest machines. This shows up on the
report side as random garbage for the cpu and timestamp, e.g.,

29816 7310572949125804849 0x1ac0 [0x50]: PERF_RECORD_MMAP ...

That messes up the sample sorting such that synthesized guest maps are
processed last.

With this patch you get a much more helpful report:

  12.11%  [guest/11399]  [guest.kernel.kallsyms.11399]  [g] irqtime_account_process_tick
  10.58%  [guest/11399]  [guest.kernel.kallsyms.11399]  [g] run_timer_softirq
   6.95%  [guest/11094]  [guest.kernel.kallsyms.11094]  [g] printk_needs_cpu
   6.50%  [guest/11094]  [guest.kernel.kallsyms.11094]  [g] do_timer
   6.45%  [guest/11399]  [guest.kernel.kallsyms.11399]  [g] idle_balance
   4.90%  [guest/11094]  [guest.kernel.kallsyms.11094]  [g] native_read_tsc
    ...

v2:
- changed rbtree walk to use rb_first per Namhyung's suggestion

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 tools/perf/util/map.c     |   13 +++++++++++++
 tools/perf/util/map.h     |    1 +
 tools/perf/util/session.c |    1 +
 3 files changed, 15 insertions(+)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 8668569..16d783d 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -729,3 +729,16 @@ char *machine__mmap_name(struct machine *self, char *bf, size_t size)
 
 	return bf;
 }
+
+void machines__set_id_hdr_size(struct rb_root *machines, u16 id_hdr_size)
+{
+	struct rb_node *node;
+	struct machine *machine;
+
+	for (node = rb_first(machines); node; node = rb_next(node)) {
+		machine = rb_entry(node, struct machine, rb_node);
+		machine->id_hdr_size = id_hdr_size;
+	}
+
+	return;
+}
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index c14c665..03a1e9b 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -151,6 +151,7 @@ struct machine *machines__add(struct rb_root *self, pid_t pid,
 struct machine *machines__find_host(struct rb_root *self);
 struct machine *machines__find(struct rb_root *self, pid_t pid);
 struct machine *machines__findnew(struct rb_root *self, pid_t pid);
+void machines__set_id_hdr_size(struct rb_root *self, u16 id_hdr_size);
 char *machine__mmap_name(struct machine *self, char *bf, size_t size);
 int machine__init(struct machine *self, const char *root_dir, pid_t pid);
 void machine__exit(struct machine *self);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 90ee39d..8e4f075 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -87,6 +87,7 @@ void perf_session__update_sample_type(struct perf_session *self)
 	self->sample_id_all = perf_evlist__sample_id_all(self->evlist);
 	self->id_hdr_size = perf_evlist__id_hdr_size(self->evlist);
 	self->host_machine.id_hdr_size = self->id_hdr_size;
+	machines__set_id_hdr_size(&self->machines, self->id_hdr_size);
 }
 
 int perf_session__create_kernel_maps(struct perf_session *self)
-- 
1.7.10.1

[PATCH 05/11] perf kvm: use strtol for walking guestmount directory

[David Ahern]

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 tools/perf/util/symbol.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 6802ef4..968de79 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2825,6 +2825,7 @@ int machines__create_guest_kernel_maps(struct rb_root *machines)
 	int i, items = 0;
 	char path[PATH_MAX];
 	pid_t pid;
+	char *endp;
 
 	if (symbol_conf.default_guest_vmlinux_name ||
 	    symbol_conf.default_guest_modules ||
@@ -2841,7 +2842,14 @@ int machines__create_guest_kernel_maps(struct rb_root *machines)
 				/* Filter out . and .. */
 				continue;
 			}
-			pid = atoi(namelist[i]->d_name);
+			pid = (pid_t) strtol(namelist[i]->d_name, &endp, 10);
+			if ((*endp != '\0') ||
+			    (endp == namelist[i]->d_name) ||
+			    (errno == ERANGE)) {
+				pr_debug("invalid directory (%s). Skipping.\n",
+					 namelist[i]->d_name);
+				continue;
+			}
 			sprintf(path, "%s/%s/proc/kallsyms",
 				symbol_conf.guestmount,
 				namelist[i]->d_name);
-- 
1.7.10.1

[PATCH 06/11] perf kvm: limit repetitive guestmount message to once per directory

[David Ahern]

After 7ed97ad use of the guestmount option without a subdir for *each*
VM generates an error message for each sample related to that VM. Once
per VM is enough.

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 tools/perf/util/map.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 16d783d..cc33486 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -8,6 +8,7 @@
 #include <unistd.h>
 #include "map.h"
 #include "thread.h"
+#include "strlist.h"
 
 const char *map_type__name[MAP__NR_TYPES] = {
 	[MAP__FUNCTION] = "Functions",
@@ -695,7 +696,15 @@ struct machine *machines__findnew(struct rb_root *self, pid_t pid)
 	    (symbol_conf.guestmount)) {
 		sprintf(path, "%s/%d", symbol_conf.guestmount, pid);
 		if (access(path, R_OK)) {
-			pr_err("Can't access file %s\n", path);
+			static struct strlist *seen;
+
+			if (!seen)
+				seen = strlist__new(true, NULL);
+
+			if (!strlist__has_entry(seen, path)) {
+				pr_err("Can't access file %s\n", path);
+				strlist__add(seen, path);
+			}
 			machine = NULL;
 			goto out;
 		}
-- 
1.7.10.1

[PATCH 07/11] perf tools: dump exclude_{guest,host}, precise_ip header info too

[David Ahern]

Adds the attributes to the event line in the header dump.
From:
 event : name = cycles, type = 0, config = 0x0, config1 = 0x0,
      config2 = 0x0, excl_usr = 0, excl_kern = 0, ...
to
  event : name = cycles, type = 0, config = 0x0, config1 = 0x0,
      config2 = 0x0, excl_usr = 0, excl_kern = 0, excl_host = 0,
      excl_guest = 0, precise_ip = 0, ...

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/util/header.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 5a47aba..3a6d204 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1212,6 +1212,12 @@ static void print_event_desc(struct perf_header *ph, int fd, FILE *fp)
 				attr.exclude_user,
 				attr.exclude_kernel);
 
+		fprintf(fp, ", excl_host = %d, excl_guest = %d",
+				attr.exclude_host,
+				attr.exclude_guest);
+
+		fprintf(fp, ", precise_ip = %d", attr.precise_ip);
+
 		if (nr)
 			fprintf(fp, ", id = {");
 
-- 
1.7.10.1

[PATCH 08/11] perf tool: precise mode requires exclude_guest

[David Ahern]

PEBS cannot be used with guest mode. If user adds :p modifier set
exclude_guest as well.


Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Gleb Natapov <gleb@redhat.com>
Link: https://lkml.org/lkml/2012/7/9/264
---
 tools/perf/util/parse-events.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 1aa721d..f34629b 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -653,6 +653,9 @@ int parse_events_modifier(struct list_head *list, char *str)
 			eH = 0;
 		} else if (*str == 'p') {
 			precise++;
+			/* use of precise requires exclude_guest */
+			if (!exclude_GH)
+				eG = 1;
 		} else
 			break;
 
-- 
1.7.10.1

[PATCH 09/11] perf top: error handling for counter creation should parallel perf-record

[David Ahern]

5a7ed29 fixed up perf-record but not perf-top. Similar argument
holds for it -- fallback to PMU only if it does not exist and handle
invalid attributes separately.

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Robert Richter <robert.richter@amd.com>
---
 tools/perf/builtin-top.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index e3cab5f..0ce665c 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -943,8 +943,10 @@ try_again:
 			 * based cpu-clock-tick sw counter, which
 			 * is always available even if no PMU support:
 			 */
-			if (attr->type == PERF_TYPE_HARDWARE &&
-			    attr->config == PERF_COUNT_HW_CPU_CYCLES) {
+			if ((err == ENOENT || err == ENXIO)
+				&& attr->type == PERF_TYPE_HARDWARE
+				&& attr->config == PERF_COUNT_HW_CPU_CYCLES) {
+
 				if (verbose)
 					ui__warning("Cycles event not supported,\n"
 						    "trying to fall back to cpu-clock-ticks\n");
-- 
1.7.10.1

[PATCH 10/11] perf tool: give user better message if precise is not supported

[David Ahern]

If the PEBS is not supported (e.g., a VM) and the precise modifier is
set the user is given a confusing error message:

  $ perf top -e cycles:p

  The sys_perf_event_open() syscall returned with 95 (Operation not supported).
/bin/dmesg may provide additional information.
  No CONFIG_PERF_EVENTS=y kernel support configured?

  $ perf record -e cycles:p -a -- sleep 1

  Error: sys_perf_event_open() syscall returned with 95 (Operation not
supported).  /bin/dmesg may provide additional information.

  Fatal: No hardware sampling interrupt available. No APIC? If so then you can
boot the kernel with the "lapic" boot parameter to force-enable it.

Which in no way conveys the real error. With this patch:
  $ perf top -e cycles:p
  PEBS request not supported. Try removing 'p' modifier

  $ perf record -e cycles:p -a -- sleep 1
  PEBS request not supported. Try removing 'p' modifier

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 tools/perf/builtin-record.c |    4 ++++
 tools/perf/builtin-top.c    |    4 ++++
 2 files changed, 8 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f5a6452..911e300 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -267,6 +267,10 @@ try_again:
 				ui__error("The %s event is not supported.\n",
 					  perf_evsel__name(pos));
 				exit(EXIT_FAILURE);
+			} else if ((err == ENOTSUP) && (attr->precise_ip)) {
+				ui__error("PEBS request not supported. "
+					  "Try removing 'p' modifier\n");
+				exit(EXIT_FAILURE);
 			}
 
 			printf("\n");
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 0ce665c..1706dc9 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -968,6 +968,10 @@ try_again:
 				ui__error("Too many events are opened.\n"
 					    "Try again after reducing the number of events\n");
 				goto out_err;
+			} else if ((err == ENOTSUP) && (attr->precise_ip)) {
+				ui__error("PEBS request not supported. "
+					    "Try removing 'p' modifier\n");
+				goto out_err;
 			}
 
 			ui__error("The sys_perf_event_open() syscall "
-- 
1.7.10.1

[PATCH 11/11] perf kvm top: limit guest kernel info message to once

[David Ahern]

'perf kvm top' shows a continual flurry of:
    Can't find guest [5201]'s kernel information

if it can't find the guest info and with a lot of VMs running a user
has no chance of reading them all. Limit message to once per guest.

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 tools/perf/builtin-top.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 1706dc9..6285374 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -706,8 +706,18 @@ static void perf_event__process_sample(struct perf_tool *tool,
 	int err;
 
 	if (!machine && perf_guest) {
-		pr_err("Can't find guest [%d]'s kernel information\n",
-			event->ip.pid);
+		static struct strlist *seen;
+		char pidstr[8];
+
+		if (!seen)
+			seen = strlist__new(true, NULL);
+
+		scnprintf(pidstr, sizeof(pidstr), "%d", event->ip.pid);
+		if (!strlist__has_entry(seen, pidstr)) {
+			pr_err("Can't find guest [%d]'s kernel information\n",
+				event->ip.pid);
+			strlist__add(seen, pidstr);
+		}
 		return;
 	}
 
-- 
1.7.10.1

Re: [PATCH 10/11] perf tool: give user better message if precise is not supported

[Namhyung Kim]

Hi, David

On Fri, 20 Jul 2012 17:25:55 -0600, David Ahern wrote:
> If the PEBS is not supported (e.g., a VM) and the precise modifier is
> set the user is given a confusing error message:
>
>   $ perf top -e cycles:p
>
>   The sys_perf_event_open() syscall returned with 95 (Operation not supported).
> /bin/dmesg may provide additional information.
>   No CONFIG_PERF_EVENTS=y kernel support configured?
>
>   $ perf record -e cycles:p -a -- sleep 1
>
>   Error: sys_perf_event_open() syscall returned with 95 (Operation not
> supported).  /bin/dmesg may provide additional information.
>
>   Fatal: No hardware sampling interrupt available. No APIC? If so then you can
> boot the kernel with the "lapic" boot parameter to force-enable it.
>
> Which in no way conveys the real error. With this patch:
>   $ perf top -e cycles:p
>   PEBS request not supported. Try removing 'p' modifier
>
>   $ perf record -e cycles:p -a -- sleep 1
>   PEBS request not supported. Try removing 'p' modifier
>

The name 'PEBS' is intel-specific, so shouldn't we try to avoid using it
directly? How about this:

    Precise event sampling is not supported. Try removing 'p' modifier

Thanks,
Namhyung


> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
>  tools/perf/builtin-record.c |    4 ++++
>  tools/perf/builtin-top.c    |    4 ++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index f5a6452..911e300 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -267,6 +267,10 @@ try_again:
>  				ui__error("The %s event is not supported.\n",
>  					  perf_evsel__name(pos));
>  				exit(EXIT_FAILURE);
> +			} else if ((err == ENOTSUP) && (attr->precise_ip)) {
> +				ui__error("PEBS request not supported. "
> +					  "Try removing 'p' modifier\n");
> +				exit(EXIT_FAILURE);
>  			}
>  
>  			printf("\n");
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 0ce665c..1706dc9 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -968,6 +968,10 @@ try_again:
>  				ui__error("Too many events are opened.\n"
>  					    "Try again after reducing the number of events\n");
>  				goto out_err;
> +			} else if ((err == ENOTSUP) && (attr->precise_ip)) {
> +				ui__error("PEBS request not supported. "
> +					    "Try removing 'p' modifier\n");
> +				goto out_err;
>  			}
>  
>  			ui__error("The sys_perf_event_open() syscall "

Re: [PATCH 10/11] perf tool: give user better message if precise is not supported

[David Ahern]

On 7/21/12 4:40 AM, Namhyung Kim wrote:
> The name 'PEBS' is intel-specific, so shouldn't we try to avoid using it
> directly? How about this:
>
>      Precise event sampling is not supported. Try removing 'p' modifier

Your right. should keep the response consistent with the documentation 
which is generic. I'll wait for responses on the other patches; perhaps 
only have to respin this one.

David

Re: [PATCH 05/11] perf kvm: use strtol for walking guestmount directory

[Arnaldo Carvalho de Melo]

Em Fri, Jul 20, 2012 at 05:25:50PM -0600, David Ahern escreveu:
> Signed-off-by: David Ahern <dsahern@gmail.com>

Why is this patch needed? Rethorical question, please resubmit with an
answer in the changeset comment :-)

- Arnaldo

Re: [PATCH 06/11] perf kvm: limit repetitive guestmount message to once per directory

[Arnaldo Carvalho de Melo]

Em Fri, Jul 20, 2012 at 05:25:51PM -0600, David Ahern escreveu:
> After 7ed97ad use of the guestmount option without a subdir for *each*
> VM generates an error message for each sample related to that VM. Once
> per VM is enough.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
>  tools/perf/util/map.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 16d783d..cc33486 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -8,6 +8,7 @@
>  #include <unistd.h>
>  #include "map.h"
>  #include "thread.h"
> +#include "strlist.h"
>  
>  const char *map_type__name[MAP__NR_TYPES] = {
>  	[MAP__FUNCTION] = "Functions",
> @@ -695,7 +696,15 @@ struct machine *machines__findnew(struct rb_root *self, pid_t pid)
>  	    (symbol_conf.guestmount)) {
>  		sprintf(path, "%s/%d", symbol_conf.guestmount, pid);
>  		if (access(path, R_OK)) {
> -			pr_err("Can't access file %s\n", path);
> +			static struct strlist *seen;
> +
> +			if (!seen)
> +				seen = strlist__new(true, NULL);
> +
> +			if (!strlist__has_entry(seen, path)) {
> +				pr_err("Can't access file %s\n", path);
> +				strlist__add(seen, path);
> +			}

At some point we'll need to have __exit functions in userland perf so
that we can delete this when using mem leak detectors :-\ Applying
anyway.

- Arnaldo

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Arnaldo Carvalho de Melo]

Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:
> PEBS cannot be used with guest mode. If user adds :p modifier set
> exclude_guest as well.

Is this something Intel specific? Or can someone think of an arch where
this limitation wouldn't exist?
 
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Gleb Natapov <gleb@redhat.com>
> Link: https://lkml.org/lkml/2012/7/9/264
> ---
>  tools/perf/util/parse-events.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 1aa721d..f34629b 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -653,6 +653,9 @@ int parse_events_modifier(struct list_head *list, char *str)
>  			eH = 0;
>  		} else if (*str == 'p') {
>  			precise++;
> +			/* use of precise requires exclude_guest */
> +			if (!exclude_GH)
> +				eG = 1;
>  		} else
>  			break;
>  
> -- 
> 1.7.10.1

Re: [PATCH 09/11] perf top: error handling for counter creation should parallel perf-record

[Arnaldo Carvalho de Melo]

Em Fri, Jul 20, 2012 at 05:25:54PM -0600, David Ahern escreveu:
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index e3cab5f..0ce665c 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -943,8 +943,10 @@ try_again:
>  			 * based cpu-clock-tick sw counter, which
>  			 * is always available even if no PMU support:
>  			 */
> -			if (attr->type == PERF_TYPE_HARDWARE &&
> -			    attr->config == PERF_COUNT_HW_CPU_CYCLES) {
> +			if ((err == ENOENT || err == ENXIO)
> +				&& attr->type == PERF_TYPE_HARDWARE
> +				&& attr->config == PERF_COUNT_HW_CPU_CYCLES) {
> +

usual style:

			if ((err == ENOENT || err == ENXIO) &&
			    attr->type == PERF_TYPE_HARDWARE &&
			    attr->config == PERF_COUNT_HW_CPU_CYCLES) {

- Arnaldo

Re: [PATCH 11/11] perf kvm top: limit guest kernel info message to once

[Arnaldo Carvalho de Melo]

Em Fri, Jul 20, 2012 at 05:25:56PM -0600, David Ahern escreveu:
> 'perf kvm top' shows a continual flurry of:
>     Can't find guest [5201]'s kernel information
> 
> if it can't find the guest info and with a lot of VMs running a user
> has no chance of reading them all. Limit message to once per guest.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
>  tools/perf/builtin-top.c |   14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 1706dc9..6285374 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -706,8 +706,18 @@ static void perf_event__process_sample(struct perf_tool *tool,
>  	int err;
>  
>  	if (!machine && perf_guest) {
> -		pr_err("Can't find guest [%d]'s kernel information\n",
> -			event->ip.pid);
> +		static struct strlist *seen;
> +		char pidstr[8];
> +
> +		if (!seen)
> +			seen = strlist__new(true, NULL);
> +
> +		scnprintf(pidstr, sizeof(pidstr), "%d", event->ip.pid);
> +		if (!strlist__has_entry(seen, pidstr)) {
> +			pr_err("Can't find guest [%d]'s kernel information\n",
> +				event->ip.pid);
> +			strlist__add(seen, pidstr);
> +		}

Abuse of strlist? Can't we have an intlist?

- Arnaldo

Re: [PATCH 09/11] perf top: error handling for counter creation should parallel perf-record

[David Ahern]

On 7/23/12 12:15 PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 20, 2012 at 05:25:54PM -0600, David Ahern escreveu:
>> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
>> index e3cab5f..0ce665c 100644
>> --- a/tools/perf/builtin-top.c
>> +++ b/tools/perf/builtin-top.c
>> @@ -943,8 +943,10 @@ try_again:
>>   			 * based cpu-clock-tick sw counter, which
>>   			 * is always available even if no PMU support:
>>   			 */
>> -			if (attr->type == PERF_TYPE_HARDWARE &&
>> -			    attr->config == PERF_COUNT_HW_CPU_CYCLES) {
>> +			if ((err == ENOENT || err == ENXIO)
>> +				&& attr->type == PERF_TYPE_HARDWARE
>> +				&& attr->config == PERF_COUNT_HW_CPU_CYCLES) {
>> +
>
> usual style:
>
> 			if ((err == ENOENT || err == ENXIO) &&
> 			    attr->type == PERF_TYPE_HARDWARE &&
> 			    attr->config == PERF_COUNT_HW_CPU_CYCLES) {
>

Right. I literally made it the same as builtin-record.c. I'll resubmit.

David

Re: [PATCH 11/11] perf kvm top: limit guest kernel info message to once

[David Ahern]

On 7/23/12 12:17 PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 20, 2012 at 05:25:56PM -0600, David Ahern escreveu:
>> 'perf kvm top' shows a continual flurry of:
>>      Can't find guest [5201]'s kernel information
>>
>> if it can't find the guest info and with a lot of VMs running a user
>> has no chance of reading them all. Limit message to once per guest.
>>
>> Signed-off-by: David Ahern <dsahern@gmail.com>
>> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> ---
>>   tools/perf/builtin-top.c |   14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
>> index 1706dc9..6285374 100644
>> --- a/tools/perf/builtin-top.c
>> +++ b/tools/perf/builtin-top.c
>> @@ -706,8 +706,18 @@ static void perf_event__process_sample(struct perf_tool *tool,
>>   	int err;
>>
>>   	if (!machine && perf_guest) {
>> -		pr_err("Can't find guest [%d]'s kernel information\n",
>> -			event->ip.pid);
>> +		static struct strlist *seen;
>> +		char pidstr[8];
>> +
>> +		if (!seen)
>> +			seen = strlist__new(true, NULL);
>> +
>> +		scnprintf(pidstr, sizeof(pidstr), "%d", event->ip.pid);
>> +		if (!strlist__has_entry(seen, pidstr)) {
>> +			pr_err("Can't find guest [%d]'s kernel information\n",
>> +				event->ip.pid);
>> +			strlist__add(seen, pidstr);
>> +		}
>
> Abuse of strlist? Can't we have an intlist?

Use of existing facility. :-) I'll look at adding an intlist facility.

David

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[David Ahern]

On 7/23/12 12:13 PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:
>> PEBS cannot be used with guest mode. If user adds :p modifier set
>> exclude_guest as well.
>
> Is this something Intel specific? Or can someone think of an arch where
> this limitation wouldn't exist?

Good point. So far precise_ip is used by arch/x86 only. I don't have an 
AMD based server so I don't know if there is a conflict between 
virtualization and IBS. Added Robert for advice.

David

>
>>
>> Signed-off-by: David Ahern <dsahern@gmail.com>
>> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Gleb Natapov <gleb@redhat.com>
>> Link: https://lkml.org/lkml/2012/7/9/264
>> ---
>>   tools/perf/util/parse-events.c |    3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 1aa721d..f34629b 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -653,6 +653,9 @@ int parse_events_modifier(struct list_head *list, char *str)
>>   			eH = 0;
>>   		} else if (*str == 'p') {
>>   			precise++;
>> +			/* use of precise requires exclude_guest */
>> +			if (!exclude_GH)
>> +				eG = 1;
>>   		} else
>>   			break;
>>
>> --
>> 1.7.10.1

Re: [PATCH 09/11] perf top: error handling for counter creation should parallel perf-record

[Arnaldo Carvalho de Melo]

Em Tue, Jul 24, 2012 at 08:01:38AM -0600, David Ahern escreveu:
> On 7/23/12 12:15 PM, Arnaldo Carvalho de Melo wrote:
> >Em Fri, Jul 20, 2012 at 05:25:54PM -0600, David Ahern escreveu:
> >>+			if ((err == ENOENT || err == ENXIO)
> >>+				&& attr->type == PERF_TYPE_HARDWARE
> >>+				&& attr->config == PERF_COUNT_HW_CPU_CYCLES) {

> >usual style:
> >			if ((err == ENOENT || err == ENXIO) &&
> >			    attr->type == PERF_TYPE_HARDWARE &&
> >			    attr->config == PERF_COUNT_HW_CPU_CYCLES) {

> Right. I literally made it the same as builtin-record.c. I'll resubmit.

Ok, at some point we should fix that too, lowest prio tho, just trying
to avoid adding more.

- Arnaldo

Re: [PATCH 11/11] perf kvm top: limit guest kernel info message to once

[Arnaldo Carvalho de Melo]

Em Tue, Jul 24, 2012 at 08:02:52AM -0600, David Ahern escreveu:
> On 7/23/12 12:17 PM, Arnaldo Carvalho de Melo wrote:
> >Em Fri, Jul 20, 2012 at 05:25:56PM -0600, David Ahern escreveu:
> >>+		static struct strlist *seen;
> >>+		char pidstr[8];

> >>+		if (!seen)
> >>+			seen = strlist__new(true, NULL);

> >>+		scnprintf(pidstr, sizeof(pidstr), "%d", event->ip.pid);
> >>+		if (!strlist__has_entry(seen, pidstr)) {
> >>+			pr_err("Can't find guest [%d]'s kernel information\n",
> >>+				event->ip.pid);
> >>+			strlist__add(seen, pidstr);

> >Abuse of strlist? Can't we have an intlist?
> 
> Use of existing facility. :-) I'll look at adding an intlist facility.

Thanks!

- Arnaldo

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Robert Richter]

David,

On 24.07.12 08:20:19, David Ahern wrote:
> On 7/23/12 12:13 PM, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:
> >> PEBS cannot be used with guest mode. If user adds :p modifier set
> >> exclude_guest as well.
> >
> > Is this something Intel specific? Or can someone think of an arch where
> > this limitation wouldn't exist?
> 
> Good point. So far precise_ip is used by arch/x86 only. I don't have an 
> AMD based server so I don't know if there is a conflict between 
> virtualization and IBS. Added Robert for advice.

thanks for this hint.

On AMD cpus precise_ip maps to IBS, which does not support hardware
options as perfctrs do. Thus, following attr flags are not supported:

 exclude_user, exclude_kernel, exclude_host, exclude_guest

Counting in guest mode is possible with IBS, but not the exclusion of
a certain mode. If precise_ip counting is enabled on AMD we may not
set the exclude_guest flag.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[David Ahern]

On 7/24/12 10:15 AM, Robert Richter wrote:
> David,
>
> On 24.07.12 08:20:19, David Ahern wrote:
>> On 7/23/12 12:13 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:
>>>> PEBS cannot be used with guest mode. If user adds :p modifier set
>>>> exclude_guest as well.
>>>
>>> Is this something Intel specific? Or can someone think of an arch where
>>> this limitation wouldn't exist?
>>
>> Good point. So far precise_ip is used by arch/x86 only. I don't have an
>> AMD based server so I don't know if there is a conflict between
>> virtualization and IBS. Added Robert for advice.
>
> thanks for this hint.
>
> On AMD cpus precise_ip maps to IBS, which does not support hardware
> options as perfctrs do. Thus, following attr flags are not supported:
>
>   exclude_user, exclude_kernel, exclude_host, exclude_guest
>
> Counting in guest mode is possible with IBS, but not the exclusion of
> a certain mode. If precise_ip counting is enabled on AMD we may not
> set the exclude_guest flag.

Ok, so with AMD precise_ip requires exclude_guest to be unset; for Intel 
we need it set.

So then we look at vendor_id in /proc/cpuinfo?

David

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Arnaldo Carvalho de Melo]

Em Tue, Jul 24, 2012 at 11:28:48AM -0600, David Ahern escreveu:
> On 7/24/12 10:15 AM, Robert Richter wrote:
> >On AMD cpus precise_ip maps to IBS, which does not support hardware
> >options as perfctrs do. Thus, following attr flags are not supported:

> >  exclude_user, exclude_kernel, exclude_host, exclude_guest

> >Counting in guest mode is possible with IBS, but not the exclusion of
> >a certain mode. If precise_ip counting is enabled on AMD we may not
> >set the exclude_guest flag.

> Ok, so with AMD precise_ip requires exclude_guest to be unset; for
> Intel we need it set.

> So then we look at vendor_id in /proc/cpuinfo?

Does it return EOPNOTSUPP or something similar if something not
supported is asked for?

Fallbacking, or capability querying if you will, may be the way to do it
without having to maintain an userland table for what is possible,
leaving it to the kernel drivers for each arch.

We do it now for sample_id_all and some other newer stuff, maybe we can
do it for this as well.

- Arnaldo

Re: [PATCH 04/11] perf kvm: fix bug resolving guest kernel syms - v2

[Jiri Olsa]

On Fri, Jul 20, 2012 at 05:25:49PM -0600, David Ahern wrote:
> Guest kernel symbols are not resolved despite passing the information
> needed to resolve them. e.g.,
> 
> perf kvm --guest --guestmount=/tmp/guest-mount record -a -- sleep 1
> perf kvm --guest --guestmount=/tmp/guest-mount report --stdio
> 
>     36.55%  [guest/11399]  [unknown]         [g] 0xffffffff81600bc8
>     33.19%  [guest/10474]  [unknown]         [g] 0x00000000c0116e00
>     30.26%  [guest/11094]  [unknown]         [g] 0xffffffff8100a288
>     43.69%  [guest/10474]  [unknown]         [g] 0x00000000c0103d90
>     37.38%  [guest/11399]  [unknown]         [g] 0xffffffff81600bc8
>     12.24%  [guest/11094]  [unknown]         [g] 0xffffffff810aa91d
>      6.69%  [guest/11094]  [unknown]         [u] 0x00007fa784d721c3
> 
> which is just pathetic.
> 
> After a maddening 2 days sifting through perf minutia I found it --
> id_hdr_size is not initialized for guest machines. This shows up on the
> report side as random garbage for the cpu and timestamp, e.g.,
> 
> 29816 7310572949125804849 0x1ac0 [0x50]: PERF_RECORD_MMAP ...
> 
> That messes up the sample sorting such that synthesized guest maps are
> processed last.
> 
> With this patch you get a much more helpful report:
> 
>   12.11%  [guest/11399]  [guest.kernel.kallsyms.11399]  [g] irqtime_account_process_tick
>   10.58%  [guest/11399]  [guest.kernel.kallsyms.11399]  [g] run_timer_softirq
>    6.95%  [guest/11094]  [guest.kernel.kallsyms.11094]  [g] printk_needs_cpu
>    6.50%  [guest/11094]  [guest.kernel.kallsyms.11094]  [g] do_timer
>    6.45%  [guest/11399]  [guest.kernel.kallsyms.11399]  [g] idle_balance
>    4.90%  [guest/11094]  [guest.kernel.kallsyms.11094]  [g] native_read_tsc
>     ...
> 
> v2:
> - changed rbtree walk to use rb_first per Namhyung's suggestion
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>

Tested-by: Jiri Olsa <jolsa@redhat.com>

Re: [PATCH 02/11] perf kvm: set name for VM process in guest machine

[Jiri Olsa]

On Fri, Jul 20, 2012 at 05:25:47PM -0600, David Ahern wrote:
> COMM events are not generated in the context of a guest machine, so the
> thread name is never set for the VMM process. For example, the qemu-kvm
> name applies to the process in the host machine, not the guest machine.
> So, samples for guest machines are currently displayed as:
> 
>     99.67%     :5671  [unknown]         [g] 0xffffffff81366b41
> 
> where 5671 is the pid of the VMM. With this patch the samples in the guest
> machine are shown as:
> 
>     18.43%  [guest/5671]  [unknown]           [g] 0xffffffff810d68b7

Tested-by: Jiri Olsa <jolsa@redhat.com>


> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
>  tools/perf/util/map.c |   17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index a1f4e36..8668569 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -7,6 +7,7 @@
>  #include <stdio.h>
>  #include <unistd.h>
>  #include "map.h"
> +#include "thread.h"
>  
>  const char *map_type__name[MAP__NR_TYPES] = {
>  	[MAP__FUNCTION] = "Functions",
> @@ -585,7 +586,21 @@ int machine__init(struct machine *self, const char *root_dir, pid_t pid)
>  	self->kmaps.machine = self;
>  	self->pid	    = pid;
>  	self->root_dir      = strdup(root_dir);
> -	return self->root_dir == NULL ? -ENOMEM : 0;
> +	if (self->root_dir == NULL)
> +		return -ENOMEM;
> +
> +	if (pid != HOST_KERNEL_ID) {
> +		struct thread *thread = machine__findnew_thread(self, pid);
> +		char comm[64];
> +
> +		if (thread == NULL)
> +			return -ENOMEM;
> +
> +		snprintf(comm, sizeof(comm), "[guest/%d]", pid);
> +		thread__set_comm(thread, comm);
> +	}
> +
> +	return 0;
>  }
>  
>  static void dsos__delete(struct list_head *self)
> -- 
> 1.7.10.1
> 

Re: [PATCH 03/11] perf kvm: guest userspace samples should not be lumped with host uspace

[Jiri Olsa]

On Fri, Jul 20, 2012 at 05:25:48PM -0600, David Ahern wrote:
> e.g., perf kvm --host  --guest report -i perf.data --stdio -D
> shows:
> 
> 1 599127912065356 0x143b8 [0x48]: PERF_RECORD_SAMPLE(IP, 5): 5671/5676: 0x7fdf95a061c0 period: 1 addr: 0
> ... chain: nr:2
> .....  0: ffffffffffffff80
> .....  1: fffffffffffffe00
>  ... thread: qemu-kvm:5671
>  ...... dso: <not found>
> 
> (IP, 5) means sample in guest userspace. Those samples should not be lumped
> into the VMM's host thread. i.e, the report output:
> 
>     56.86%  qemu-kvm  [unknown]         [u] 0x00007fdf95a061c0
> 
> With this patch the output emphasizes it is a guest userspace hit:
> 
>     56.86%  [guest/5671]  [unknown]         [u] 0x00007fdf95a061c0
> 
> Looking at 3 VMs (2 64-bit, 1 32-bit) with each running a CPU bound
> process (openssl speed), perf report currently shows:
> 
>   93.84%  117726   qemu-kvm  [unknown]   [u] 0x00007fd7dcaea8e5
> 
> which is wrong. With this patch you get:
> 
>   31.50%   39258   [guest/18772]  [unknown]   [u] 0x00007fd7dcaea8e5
>   31.50%   39236   [guest/11230]  [unknown]   [u] 0x0000000000a57340
>   30.84%   39232   [guest/18395]  [unknown]   [u] 0x00007f66f641e107

Tested-by: Jiri Olsa <jolsa@redhat.com>

[tip:perf/core] perf symbols: Add machine id to modules debug message

[tip-bot for David Ahern]

Commit-ID:  f51304d3fe4fee475991ee424a4b7f85eec65a7b
Gitweb:     http://git.kernel.org/tip/f51304d3fe4fee475991ee424a4b7f85eec65a7b
Author:     David Ahern <dsahern@gmail.com>
AuthorDate: Fri, 20 Jul 2012 17:25:46 -0600
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 23 Jul 2012 15:03:48 -0300

perf symbols: Add machine id to modules debug message

Current debug message is:
Problems creating module maps, continuing anyway...

When running multiple VMs it would be nice to know which machine the
message is referring to:

$ perf kvm --guest --guestmount=/tmp/guest-mount record -av -- sleep 10
Problems creating module maps for guest 6613, continuing anyway...

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1342826756-64663-2-git-send-email-dsahern@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/symbol.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 50958bb..66c132e 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2564,8 +2564,15 @@ int machine__create_kernel_maps(struct machine *machine)
 	    __machine__create_kernel_maps(machine, kernel) < 0)
 		return -1;
 
-	if (symbol_conf.use_modules && machine__create_modules(machine) < 0)
-		pr_debug("Problems creating module maps, continuing anyway...\n");
+	if (symbol_conf.use_modules && machine__create_modules(machine) < 0) {
+		if (machine__is_host(machine))
+			pr_debug("Problems creating module maps, "
+				 "continuing anyway...\n");
+		else
+			pr_debug("Problems creating module maps for guest %d, "
+				 "continuing anyway...\n", machine->pid);
+	}
+
 	/*
 	 * Now that we have all the maps created, just set the ->end of them:
 	 */

[tip:perf/core] perf kvm: Set name for VM process in guest machine

[tip-bot for David Ahern]

Commit-ID:  5cd95c2db479aa7a66f6fa572dfa410c6314c78e
Gitweb:     http://git.kernel.org/tip/5cd95c2db479aa7a66f6fa572dfa410c6314c78e
Author:     David Ahern <dsahern@gmail.com>
AuthorDate: Fri, 20 Jul 2012 17:25:47 -0600
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 25 Jul 2012 11:27:55 -0300

perf kvm: Set name for VM process in guest machine

COMM events are not generated in the context of a guest machine, so the
thread name is never set for the VMM process. For example, the qemu-kvm
name applies to the process in the host machine, not the guest machine.
So, samples for guest machines are currently displayed as:

    99.67%     :5671  [unknown]         [g] 0xffffffff81366b41

where 5671 is the pid of the VMM. With this patch the samples in the guest
machine are shown as:

    18.43%  [guest/5671]  [unknown]           [g] 0xffffffff810d68b7

Tested-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1342826756-64663-3-git-send-email-dsahern@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/map.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index a1f4e36..8668569 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -7,6 +7,7 @@
 #include <stdio.h>
 #include <unistd.h>
 #include "map.h"
+#include "thread.h"
 
 const char *map_type__name[MAP__NR_TYPES] = {
 	[MAP__FUNCTION] = "Functions",
@@ -585,7 +586,21 @@ int machine__init(struct machine *self, const char *root_dir, pid_t pid)
 	self->kmaps.machine = self;
 	self->pid	    = pid;
 	self->root_dir      = strdup(root_dir);
-	return self->root_dir == NULL ? -ENOMEM : 0;
+	if (self->root_dir == NULL)
+		return -ENOMEM;
+
+	if (pid != HOST_KERNEL_ID) {
+		struct thread *thread = machine__findnew_thread(self, pid);
+		char comm[64];
+
+		if (thread == NULL)
+			return -ENOMEM;
+
+		snprintf(comm, sizeof(comm), "[guest/%d]", pid);
+		thread__set_comm(thread, comm);
+	}
+
+	return 0;
 }
 
 static void dsos__delete(struct list_head *self)

[tip:perf/core] perf kvm: Guest userspace samples should not be lumped with host uspace

[tip-bot for David Ahern]

Commit-ID:  7c0f4a4113ba5de7898c246eeaeee4c23d94b887
Gitweb:     http://git.kernel.org/tip/7c0f4a4113ba5de7898c246eeaeee4c23d94b887
Author:     David Ahern <dsahern@gmail.com>
AuthorDate: Fri, 20 Jul 2012 17:25:48 -0600
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 25 Jul 2012 11:29:46 -0300

perf kvm: Guest userspace samples should not be lumped with host uspace

e.g., perf kvm --host  --guest report -i perf.data --stdio -D
shows:

1 599127912065356 0x143b8 [0x48]: PERF_RECORD_SAMPLE(IP, 5): 5671/5676: 0x7fdf95a061c0 period: 1 addr: 0
... chain: nr:2
.....  0: ffffffffffffff80
.....  1: fffffffffffffe00
 ... thread: qemu-kvm:5671
 ...... dso: <not found>

(IP, 5) means sample in guest userspace. Those samples should not be lumped
into the VMM's host thread. i.e, the report output:

    56.86%  qemu-kvm  [unknown]         [u] 0x00007fdf95a061c0

With this patch the output emphasizes it is a guest userspace hit:

    56.86%  [guest/5671]  [unknown]         [u] 0x00007fdf95a061c0

Looking at 3 VMs (2 64-bit, 1 32-bit) with each running a CPU bound
process (openssl speed), perf report currently shows:

  93.84%  117726   qemu-kvm  [unknown]   [u] 0x00007fd7dcaea8e5

which is wrong. With this patch you get:

  31.50%   39258   [guest/18772]  [unknown]   [u] 0x00007fd7dcaea8e5
  31.50%   39236   [guest/11230]  [unknown]   [u] 0x0000000000a57340
  30.84%   39232   [guest/18395]  [unknown]   [u] 0x00007f66f641e107

Tested-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1342826756-64663-4-git-send-email-dsahern@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/session.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 8e48559..90ee39d 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -918,7 +918,9 @@ static struct machine *
 {
 	const u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
 
-	if (cpumode == PERF_RECORD_MISC_GUEST_KERNEL && perf_guest) {
+	if (perf_guest &&
+	    ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
+	     (cpumode == PERF_RECORD_MISC_GUEST_USER))) {
 		u32 pid;
 
 		if (event->header.type == PERF_RECORD_MMAP)

[tip:perf/core] perf kvm: Fix bug resolving guest kernel syms

[tip-bot for David Ahern]

Commit-ID:  adb5d2a487c55e5ca2ecc0b73c8f592e95d292c7
Gitweb:     http://git.kernel.org/tip/adb5d2a487c55e5ca2ecc0b73c8f592e95d292c7
Author:     David Ahern <dsahern@gmail.com>
AuthorDate: Fri, 20 Jul 2012 17:25:49 -0600
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 25 Jul 2012 11:30:13 -0300

perf kvm: Fix bug resolving guest kernel syms

Guest kernel symbols are not resolved despite passing the information
needed to resolve them. e.g.,

perf kvm --guest --guestmount=/tmp/guest-mount record -a -- sleep 1
perf kvm --guest --guestmount=/tmp/guest-mount report --stdio

    36.55%  [guest/11399]  [unknown]         [g] 0xffffffff81600bc8
    33.19%  [guest/10474]  [unknown]         [g] 0x00000000c0116e00
    30.26%  [guest/11094]  [unknown]         [g] 0xffffffff8100a288
    43.69%  [guest/10474]  [unknown]         [g] 0x00000000c0103d90
    37.38%  [guest/11399]  [unknown]         [g] 0xffffffff81600bc8
    12.24%  [guest/11094]  [unknown]         [g] 0xffffffff810aa91d
     6.69%  [guest/11094]  [unknown]         [u] 0x00007fa784d721c3

which is just pathetic.

After a maddening 2 days sifting through perf minutia I found it --
id_hdr_size is not initialized for guest machines. This shows up on the
report side as random garbage for the cpu and timestamp, e.g.,

29816 7310572949125804849 0x1ac0 [0x50]: PERF_RECORD_MMAP ...

That messes up the sample sorting such that synthesized guest maps are
processed last.

With this patch you get a much more helpful report:

  12.11%  [guest/11399]  [guest.kernel.kallsyms.11399]  [g] irqtime_account_process_tick
  10.58%  [guest/11399]  [guest.kernel.kallsyms.11399]  [g] run_timer_softirq
   6.95%  [guest/11094]  [guest.kernel.kallsyms.11094]  [g] printk_needs_cpu
   6.50%  [guest/11094]  [guest.kernel.kallsyms.11094]  [g] do_timer
   6.45%  [guest/11399]  [guest.kernel.kallsyms.11399]  [g] idle_balance
   4.90%  [guest/11094]  [guest.kernel.kallsyms.11094]  [g] native_read_tsc
    ...

v2:
- changed rbtree walk to use rb_first per Namhyung's suggestion

Tested-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1342826756-64663-5-git-send-email-dsahern@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/map.c     |   13 +++++++++++++
 tools/perf/util/map.h     |    1 +
 tools/perf/util/session.c |    1 +
 3 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 8668569..16d783d 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -729,3 +729,16 @@ char *machine__mmap_name(struct machine *self, char *bf, size_t size)
 
 	return bf;
 }
+
+void machines__set_id_hdr_size(struct rb_root *machines, u16 id_hdr_size)
+{
+	struct rb_node *node;
+	struct machine *machine;
+
+	for (node = rb_first(machines); node; node = rb_next(node)) {
+		machine = rb_entry(node, struct machine, rb_node);
+		machine->id_hdr_size = id_hdr_size;
+	}
+
+	return;
+}
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index c14c665..03a1e9b 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -151,6 +151,7 @@ struct machine *machines__add(struct rb_root *self, pid_t pid,
 struct machine *machines__find_host(struct rb_root *self);
 struct machine *machines__find(struct rb_root *self, pid_t pid);
 struct machine *machines__findnew(struct rb_root *self, pid_t pid);
+void machines__set_id_hdr_size(struct rb_root *self, u16 id_hdr_size);
 char *machine__mmap_name(struct machine *self, char *bf, size_t size);
 int machine__init(struct machine *self, const char *root_dir, pid_t pid);
 void machine__exit(struct machine *self);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 90ee39d..8e4f075 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -87,6 +87,7 @@ void perf_session__update_sample_type(struct perf_session *self)
 	self->sample_id_all = perf_evlist__sample_id_all(self->evlist);
 	self->id_hdr_size = perf_evlist__id_hdr_size(self->evlist);
 	self->host_machine.id_hdr_size = self->id_hdr_size;
+	machines__set_id_hdr_size(&self->machines, self->id_hdr_size);
 }
 
 int perf_session__create_kernel_maps(struct perf_session *self)

[tip:perf/core] perf kvm: Limit repetitive guestmount message to once per directory

[tip-bot for David Ahern]

Commit-ID:  c80c3c269011c67b8dabef5238af44a6d94e4d0e
Gitweb:     http://git.kernel.org/tip/c80c3c269011c67b8dabef5238af44a6d94e4d0e
Author:     David Ahern <dsahern@gmail.com>
AuthorDate: Fri, 20 Jul 2012 17:25:51 -0600
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 25 Jul 2012 11:31:03 -0300

perf kvm: Limit repetitive guestmount message to once per directory

After 7ed97ad use of the guestmount option without a subdir for *each*
VM generates an error message for each sample related to that VM. Once
per VM is enough.

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/1342826756-64663-7-git-send-email-dsahern@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/map.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 16d783d..cc33486 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -8,6 +8,7 @@
 #include <unistd.h>
 #include "map.h"
 #include "thread.h"
+#include "strlist.h"
 
 const char *map_type__name[MAP__NR_TYPES] = {
 	[MAP__FUNCTION] = "Functions",
@@ -695,7 +696,15 @@ struct machine *machines__findnew(struct rb_root *self, pid_t pid)
 	    (symbol_conf.guestmount)) {
 		sprintf(path, "%s/%d", symbol_conf.guestmount, pid);
 		if (access(path, R_OK)) {
-			pr_err("Can't access file %s\n", path);
+			static struct strlist *seen;
+
+			if (!seen)
+				seen = strlist__new(true, NULL);
+
+			if (!strlist__has_entry(seen, path)) {
+				pr_err("Can't access file %s\n", path);
+				strlist__add(seen, path);
+			}
 			machine = NULL;
 			goto out;
 		}

[tip:perf/core] perf tools: Dump exclude_{guest,host}, precise_ip header info too

[tip-bot for David Ahern]

Commit-ID:  78b961ff8e67207adb15959526cdea4cc50ae1ed
Gitweb:     http://git.kernel.org/tip/78b961ff8e67207adb15959526cdea4cc50ae1ed
Author:     David Ahern <dsahern@gmail.com>
AuthorDate: Fri, 20 Jul 2012 17:25:52 -0600
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 25 Jul 2012 11:31:19 -0300

perf tools: Dump exclude_{guest,host}, precise_ip header info too

Adds the attributes to the event line in the header dump.
From:
 event : name = cycles, type = 0, config = 0x0, config1 = 0x0,
      config2 = 0x0, excl_usr = 0, excl_kern = 0, ...
to
  event : name = cycles, type = 0, config = 0x0, config1 = 0x0,
      config2 = 0x0, excl_usr = 0, excl_kern = 0, excl_host = 0,
      excl_guest = 0, precise_ip = 0, ...

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1342826756-64663-8-git-send-email-dsahern@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/header.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 5a47aba..3a6d204 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -1212,6 +1212,12 @@ static void print_event_desc(struct perf_header *ph, int fd, FILE *fp)
 				attr.exclude_user,
 				attr.exclude_kernel);
 
+		fprintf(fp, ", excl_host = %d, excl_guest = %d",
+				attr.exclude_host,
+				attr.exclude_guest);
+
+		fprintf(fp, ", precise_ip = %d", attr.precise_ip);
+
 		if (nr)
 			fprintf(fp, ", id = {");
 

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Peter Zijlstra]

On Tue, 2012-07-24 at 18:15 +0200, Robert Richter wrote:
> David,
> 
> On 24.07.12 08:20:19, David Ahern wrote:
> > On 7/23/12 12:13 PM, Arnaldo Carvalho de Melo wrote:
> > > Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:
> > >> PEBS cannot be used with guest mode. If user adds :p modifier set
> > >> exclude_guest as well.
> > >
> > > Is this something Intel specific? Or can someone think of an arch where
> > > this limitation wouldn't exist?
> > 
> > Good point. So far precise_ip is used by arch/x86 only. I don't have an 
> > AMD based server so I don't know if there is a conflict between 
> > virtualization and IBS. Added Robert for advice.
> 
> thanks for this hint.
> 
> On AMD cpus precise_ip maps to IBS, which does not support hardware
> options as perfctrs do. Thus, following attr flags are not supported:
> 
>  exclude_user, exclude_kernel, exclude_host, exclude_guest
> 
> Counting in guest mode is possible with IBS, but not the exclusion of
> a certain mode. If precise_ip counting is enabled on AMD we may not
> set the exclude_guest flag.

IIRC we have SVM enter/exit hooks in kvm/perf already, we could use
those to implement exclude_guest for IBS.

Now I've been trying to find the patches that introduced that, but I'm
failing horridly. Gleb, got us a pointer here?

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[David Ahern]

On 7/24/12 12:03 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jul 24, 2012 at 11:28:48AM -0600, David Ahern escreveu:
>> On 7/24/12 10:15 AM, Robert Richter wrote:
>>> On AMD cpus precise_ip maps to IBS, which does not support hardware
>>> options as perfctrs do. Thus, following attr flags are not supported:
>
>>>   exclude_user, exclude_kernel, exclude_host, exclude_guest
>
>>> Counting in guest mode is possible with IBS, but not the exclusion of
>>> a certain mode. If precise_ip counting is enabled on AMD we may not
>>> set the exclude_guest flag.
>
>> Ok, so with AMD precise_ip requires exclude_guest to be unset; for
>> Intel we need it set.
>
>> So then we look at vendor_id in /proc/cpuinfo?
>
> Does it return EOPNOTSUPP or something similar if something not
> supported is asked for?
>
> Fallbacking, or capability querying if you will, may be the way to do it
> without having to maintain an userland table for what is possible,
> leaving it to the kernel drivers for each arch.

Peter's patch (see https://lkml.org/lkml/2012/7/9/298) changes kernel 
side to require the use of exclude_guest if the precise modifier is 
used, returning -EOPNOTSUPP if exclude_guest is not set. This patch goes 
after the user experience: Today if a user specifies -e <event>:p all 
other modifiers are reset - including exclude_guest. Going forward we 
need :p to imply :pH if a user has not specified a GH modifer.

We could do nothing and handle the unsupported error and try setting the 
exclude_guest option - like perf handles other new parameters. But 
EOPNOTSUPP is not uniquely tied to this error -- e.g., it could be the 
BTS is not supported (:pp). Also, we have no easy way to discriminate :p 
from :pG or :pGH. It seems to me perf should not silently undo a user 
request on the modifier, but inform the user the request is wrong. For 
example if a user request -e cycles:pG it should not be silently turned 
into :pH.

And then yesterday, Robert stated that none of the exclude_xxxx 
modifiers can be set for the AMD if the precise modifier is used, so we 
cannot blindly set exclude_guest if precise_ip is set.

So, seems to me perf need's one action for Intel processors and another 
for AMD.

David

>
> We do it now for sample_id_all and some other newer stuff, maybe we can
> do it for this as well.
>
> - Arnaldo
>

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Gleb Natapov]

On Wed, Jul 25, 2012 at 10:35:46PM +0200, Peter Zijlstra wrote:
> On Tue, 2012-07-24 at 18:15 +0200, Robert Richter wrote:
> > David,
> > 
> > On 24.07.12 08:20:19, David Ahern wrote:
> > > On 7/23/12 12:13 PM, Arnaldo Carvalho de Melo wrote:
> > > > Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:
> > > >> PEBS cannot be used with guest mode. If user adds :p modifier set
> > > >> exclude_guest as well.
> > > >
> > > > Is this something Intel specific? Or can someone think of an arch where
> > > > this limitation wouldn't exist?
> > > 
> > > Good point. So far precise_ip is used by arch/x86 only. I don't have an 
> > > AMD based server so I don't know if there is a conflict between 
> > > virtualization and IBS. Added Robert for advice.
> > 
> > thanks for this hint.
> > 
> > On AMD cpus precise_ip maps to IBS, which does not support hardware
> > options as perfctrs do. Thus, following attr flags are not supported:
> > 
> >  exclude_user, exclude_kernel, exclude_host, exclude_guest
> > 
> > Counting in guest mode is possible with IBS, but not the exclusion of
> > a certain mode. If precise_ip counting is enabled on AMD we may not
> > set the exclude_guest flag.
> 
> IIRC we have SVM enter/exit hooks in kvm/perf already, we could use
> those to implement exclude_guest for IBS.
> 
> Now I've been trying to find the patches that introduced that, but I'm
> failing horridly. Gleb, got us a pointer here?
The commit is 144d31e6, but it introduces hook that is used on VMX only.
SVM does not need it to implement guest/host only counters since it
has HW support for that in the PMU.
 
--
			Gleb.

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Peter Zijlstra]

On Thu, 2012-07-26 at 08:50 +0300, Gleb Natapov wrote:
> On Wed, Jul 25, 2012 at 10:35:46PM +0200, Peter Zijlstra wrote:
> > On Tue, 2012-07-24 at 18:15 +0200, Robert Richter wrote:
> > > David,
> > > 
> > > On 24.07.12 08:20:19, David Ahern wrote:
> > > > On 7/23/12 12:13 PM, Arnaldo Carvalho de Melo wrote:
> > > > > Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:
> > > > >> PEBS cannot be used with guest mode. If user adds :p modifier set
> > > > >> exclude_guest as well.
> > > > >
> > > > > Is this something Intel specific? Or can someone think of an arch where
> > > > > this limitation wouldn't exist?
> > > > 
> > > > Good point. So far precise_ip is used by arch/x86 only. I don't have an 
> > > > AMD based server so I don't know if there is a conflict between 
> > > > virtualization and IBS. Added Robert for advice.
> > > 
> > > thanks for this hint.
> > > 
> > > On AMD cpus precise_ip maps to IBS, which does not support hardware
> > > options as perfctrs do. Thus, following attr flags are not supported:
> > > 
> > >  exclude_user, exclude_kernel, exclude_host, exclude_guest
> > > 
> > > Counting in guest mode is possible with IBS, but not the exclusion of
> > > a certain mode. If precise_ip counting is enabled on AMD we may not
> > > set the exclude_guest flag.
> > 
> > IIRC we have SVM enter/exit hooks in kvm/perf already, we could use
> > those to implement exclude_guest for IBS.
> > 
> > Now I've been trying to find the patches that introduced that, but I'm
> > failing horridly. Gleb, got us a pointer here?

> The commit is 144d31e6, but it introduces hook that is used on VMX only.
> SVM does not need it to implement guest/host only counters since it
> has HW support for that in the PMU.

Right, seems that has changed now that we support IBS.

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Peter Zijlstra]

On Wed, 2012-07-25 at 23:16 -0600, David Ahern wrote:

> Peter's patch (see https://lkml.org/lkml/2012/7/9/298) changes kernel 
> side to require the use of exclude_guest if the precise modifier is 
> used, returning -EOPNOTSUPP if exclude_guest is not set. This patch goes 
> after the user experience: Today if a user specifies -e <event>:p all 
> other modifiers are reset - including exclude_guest. Going forward we 
> need :p to imply :pH if a user has not specified a GH modifer.
> 
> We could do nothing and handle the unsupported error and try setting the 
> exclude_guest option - like perf handles other new parameters. But 
> EOPNOTSUPP is not uniquely tied to this error -- e.g., it could be the 
> BTS is not supported (:pp). Also, we have no easy way to discriminate :p 
> from :pG or :pGH. It seems to me perf should not silently undo a user 
> request on the modifier, but inform the user the request is wrong. For 
> example if a user request -e cycles:pG it should not be silently turned 
> into :pH.
> 
> And then yesterday, Robert stated that none of the exclude_xxxx 
> modifiers can be set for the AMD if the precise modifier is used, so we 
> cannot blindly set exclude_guest if precise_ip is set.
> 
> So, seems to me perf need's one action for Intel processors and another 
> for AMD.

No, we just need to teach the IBS code about SVM enter/exit.

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Gleb Natapov]

On Thu, Jul 26, 2012 at 10:07:37AM +0200, Peter Zijlstra wrote:
> On Thu, 2012-07-26 at 08:50 +0300, Gleb Natapov wrote:
> > On Wed, Jul 25, 2012 at 10:35:46PM +0200, Peter Zijlstra wrote:
> > > On Tue, 2012-07-24 at 18:15 +0200, Robert Richter wrote:
> > > > David,
> > > > 
> > > > On 24.07.12 08:20:19, David Ahern wrote:
> > > > > On 7/23/12 12:13 PM, Arnaldo Carvalho de Melo wrote:
> > > > > > Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:
> > > > > >> PEBS cannot be used with guest mode. If user adds :p modifier set
> > > > > >> exclude_guest as well.
> > > > > >
> > > > > > Is this something Intel specific? Or can someone think of an arch where
> > > > > > this limitation wouldn't exist?
> > > > > 
> > > > > Good point. So far precise_ip is used by arch/x86 only. I don't have an 
> > > > > AMD based server so I don't know if there is a conflict between 
> > > > > virtualization and IBS. Added Robert for advice.
> > > > 
> > > > thanks for this hint.
> > > > 
> > > > On AMD cpus precise_ip maps to IBS, which does not support hardware
> > > > options as perfctrs do. Thus, following attr flags are not supported:
> > > > 
> > > >  exclude_user, exclude_kernel, exclude_host, exclude_guest
> > > > 
> > > > Counting in guest mode is possible with IBS, but not the exclusion of
> > > > a certain mode. If precise_ip counting is enabled on AMD we may not
> > > > set the exclude_guest flag.
> > > 
> > > IIRC we have SVM enter/exit hooks in kvm/perf already, we could use
> > > those to implement exclude_guest for IBS.
> > > 
> > > Now I've been trying to find the patches that introduced that, but I'm
> > > failing horridly. Gleb, got us a pointer here?
> 
> > The commit is 144d31e6, but it introduces hook that is used on VMX only.
> > SVM does not need it to implement guest/host only counters since it
> > has HW support for that in the PMU.
> 
> Right, seems that has changed now that we support IBS.

Copying Joerg who wrote guest/host exclude code for AMD.

--
			Gleb.

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Robert Richter]

On 26.07.12 10:07:37, Peter Zijlstra wrote:
> On Thu, 2012-07-26 at 08:50 +0300, Gleb Natapov wrote:
> > The commit is 144d31e6, but it introduces hook that is used on VMX only.
> > SVM does not need it to implement guest/host only counters since it
> > has HW support for that in the PMU.
> 
> Right, seems that has changed now that we support IBS.

Will look at this.

-Robert


-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[David Ahern]

Hi Peter:

On 7/25/12 11:50 PM, Gleb Natapov wrote:
> On Wed, Jul 25, 2012 at 10:35:46PM +0200, Peter Zijlstra wrote:
>> On Tue, 2012-07-24 at 18:15 +0200, Robert Richter wrote:
>>> thanks for this hint.
>>>
>>> On AMD cpus precise_ip maps to IBS, which does not support hardware
>>> options as perfctrs do. Thus, following attr flags are not supported:
>>>
>>>   exclude_user, exclude_kernel, exclude_host, exclude_guest
>>>
>>> Counting in guest mode is possible with IBS, but not the exclusion of
>>> a certain mode. If precise_ip counting is enabled on AMD we may not
>>> set the exclude_guest flag.
>>
>> IIRC we have SVM enter/exit hooks in kvm/perf already, we could use
>> those to implement exclude_guest for IBS.
>>
>> Now I've been trying to find the patches that introduced that, but I'm
>> failing horridly. Gleb, got us a pointer here?
> The commit is 144d31e6, but it introduces hook that is used on VMX only.
> SVM does not need it to implement guest/host only counters since it
> has HW support for that in the PMU.

Any updates?

David

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Robert Richter]

On 26.07.12 10:08:29, Peter Zijlstra wrote:
> On Wed, 2012-07-25 at 23:16 -0600, David Ahern wrote:
> 
> > Peter's patch (see https://lkml.org/lkml/2012/7/9/298) changes kernel 
> > side to require the use of exclude_guest if the precise modifier is 
> > used, returning -EOPNOTSUPP if exclude_guest is not set. This patch goes 
> > after the user experience: Today if a user specifies -e <event>:p all 
> > other modifiers are reset - including exclude_guest. Going forward we 
> > need :p to imply :pH if a user has not specified a GH modifer.
> > 
> > We could do nothing and handle the unsupported error and try setting the 
> > exclude_guest option - like perf handles other new parameters. But 
> > EOPNOTSUPP is not uniquely tied to this error -- e.g., it could be the 
> > BTS is not supported (:pp). Also, we have no easy way to discriminate :p 
> > from :pG or :pGH. It seems to me perf should not silently undo a user 
> > request on the modifier, but inform the user the request is wrong. For 
> > example if a user request -e cycles:pG it should not be silently turned 
> > into :pH.
> > 
> > And then yesterday, Robert stated that none of the exclude_xxxx 
> > modifiers can be set for the AMD if the precise modifier is used, so we 
> > cannot blindly set exclude_guest if precise_ip is set.
> > 
> > So, seems to me perf need's one action for Intel processors and another 
> > for AMD.
> 
> No, we just need to teach the IBS code about SVM enter/exit.

I aggree that this could be emulated in software by enabling/disabling
the event with a guest/host switch. And, even better, we add this for
every pmu in a generic way. E.g. northbridge counter and I guess also
Intel uncore events do not support G/H counting in hardware. Same to
other pmus that could be imaginable in the future like counters for
IOMMUs or other hardware devices.

But, as some pmus are not related to virtualization or other features
they simply do not need to support those attributes, or we want other
defaults, e.g. enable it system wide. Detecting features with syscall
error checking and then falling back to other defaults does not seem
the right approach to me, because it may require several syscalls to
check *combinations* of supported attributes, makes error logging and
detection more difficult due to noisy log messages and because there
is no strict attribute flag checking in current and older kernels.

I better would like to see a pmu feature flag in the same style as
with /proc/cpuinfo, e.g.:

 $ cat /sys/bus/event_source/devices/cpu/flags
 exclude_host exclude_guest

We also need stricter attribute flag checking, esp. of reseved flags
and for unsupported features in some pmus (I already work on some
patches for this). Userland then checks flags and sets up syscalls
according to the reported flags. The goal should be to avoid syscall
errors at all. Thus, we are able to improve dmesg logging in case of
errors, currently we do not see any message if a syscall fails.

And finally, if a feature could be emulated, we could provide this
emulation of an attr flag to all pmus.

Does this make sense?

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[David Ahern]

On 8/3/12 7:51 AM, Robert Richter wrote:
> On 26.07.12 10:08:29, Peter Zijlstra wrote:
>> On Wed, 2012-07-25 at 23:16 -0600, David Ahern wrote:
>>
>>> Peter's patch (see https://lkml.org/lkml/2012/7/9/298) changes kernel
>>> side to require the use of exclude_guest if the precise modifier is
>>> used, returning -EOPNOTSUPP if exclude_guest is not set. This patch goes
>>> after the user experience: Today if a user specifies -e <event>:p all
>>> other modifiers are reset - including exclude_guest. Going forward we
>>> need :p to imply :pH if a user has not specified a GH modifer.
>>>
>>> We could do nothing and handle the unsupported error and try setting the
>>> exclude_guest option - like perf handles other new parameters. But
>>> EOPNOTSUPP is not uniquely tied to this error -- e.g., it could be the
>>> BTS is not supported (:pp). Also, we have no easy way to discriminate :p
>>> from :pG or :pGH. It seems to me perf should not silently undo a user
>>> request on the modifier, but inform the user the request is wrong. For
>>> example if a user request -e cycles:pG it should not be silently turned
>>> into :pH.
>>>
>>> And then yesterday, Robert stated that none of the exclude_xxxx
>>> modifiers can be set for the AMD if the precise modifier is used, so we
>>> cannot blindly set exclude_guest if precise_ip is set.
>>>
>>> So, seems to me perf need's one action for Intel processors and another
>>> for AMD.
>>
>> No, we just need to teach the IBS code about SVM enter/exit.
>
> I aggree that this could be emulated in software by enabling/disabling
> the event with a guest/host switch. And, even better, we add this for
> every pmu in a generic way. E.g. northbridge counter and I guess also
> Intel uncore events do not support G/H counting in hardware. Same to
> other pmus that could be imaginable in the future like counters for
> IOMMUs or other hardware devices.
>
> But, as some pmus are not related to virtualization or other features
> they simply do not need to support those attributes, or we want other
> defaults, e.g. enable it system wide. Detecting features with syscall
> error checking and then falling back to other defaults does not seem
> the right approach to me, because it may require several syscalls to
> check *combinations* of supported attributes, makes error logging and
> detection more difficult due to noisy log messages and because there
> is no strict attribute flag checking in current and older kernels.
>
> I better would like to see a pmu feature flag in the same style as
> with /proc/cpuinfo, e.g.:
>
>   $ cat /sys/bus/event_source/devices/cpu/flags
>   exclude_host exclude_guest
>
> We also need stricter attribute flag checking, esp. of reseved flags
> and for unsupported features in some pmus (I already work on some
> patches for this). Userland then checks flags and sets up syscalls
> according to the reported flags. The goal should be to avoid syscall
> errors at all. Thus, we are able to improve dmesg logging in case of
> errors, currently we do not see any message if a syscall fails.
>
> And finally, if a feature could be emulated, we could provide this
> emulation of an attr flag to all pmus.
>
> Does this make sense?

any updates on how to handle exclude_guest on AMD?

Really need to get this finished. Kind of nasty to accidentally kill all 
running VMs with a perf modifier.

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[David Ahern]

In an attempt to jump start this thread...

On 8/3/12 7:51 AM, Robert Richter wrote:
> On 26.07.12 10:08:29, Peter Zijlstra wrote:
>> On Wed, 2012-07-25 at 23:16 -0600, David Ahern wrote:
>>
>>> Peter's patch (see https://lkml.org/lkml/2012/7/9/298) changes kernel
>>> side to require the use of exclude_guest if the precise modifier is
>>> used, returning -EOPNOTSUPP if exclude_guest is not set. This patch goes
>>> after the user experience: Today if a user specifies -e <event>:p all
>>> other modifiers are reset - including exclude_guest. Going forward we
>>> need :p to imply :pH if a user has not specified a GH modifer.
>>>
>>> We could do nothing and handle the unsupported error and try setting the
>>> exclude_guest option - like perf handles other new parameters. But
>>> EOPNOTSUPP is not uniquely tied to this error -- e.g., it could be the
>>> BTS is not supported (:pp). Also, we have no easy way to discriminate :p
>>> from :pG or :pGH. It seems to me perf should not silently undo a user
>>> request on the modifier, but inform the user the request is wrong. For
>>> example if a user request -e cycles:pG it should not be silently turned
>>> into :pH.
>>>
>>> And then yesterday, Robert stated that none of the exclude_xxxx
>>> modifiers can be set for the AMD if the precise modifier is used, so we
>>> cannot blindly set exclude_guest if precise_ip is set.
>>>
>>> So, seems to me perf need's one action for Intel processors and another
>>> for AMD.
>>
>> No, we just need to teach the IBS code about SVM enter/exit.
>
> I aggree that this could be emulated in software by enabling/disabling
> the event with a guest/host switch. And, even better, we add this for
> every pmu in a generic way. E.g. northbridge counter and I guess also
> Intel uncore events do not support G/H counting in hardware. Same to
> other pmus that could be imaginable in the future like counters for
> IOMMUs or other hardware devices.
>
> But, as some pmus are not related to virtualization or other features
> they simply do not need to support those attributes, or we want other
> defaults, e.g. enable it system wide. Detecting features with syscall
> error checking and then falling back to other defaults does not seem
> the right approach to me, because it may require several syscalls to
> check *combinations* of supported attributes, makes error logging and
> detection more difficult due to noisy log messages and because there
> is no strict attribute flag checking in current and older kernels.
>
> I better would like to see a pmu feature flag in the same style as
> with /proc/cpuinfo, e.g.:
>
>   $ cat /sys/bus/event_source/devices/cpu/flags
>   exclude_host exclude_guest
>
> We also need stricter attribute flag checking, esp. of reseved flags
> and for unsupported features in some pmus (I already work on some
> patches for this). Userland then checks flags and sets up syscalls
> according to the reported flags. The goal should be to avoid syscall
> errors at all. Thus, we are able to improve dmesg logging in case of
> errors, currently we do not see any message if a syscall fails.
>
> And finally, if a feature could be emulated, we could provide this
> emulation of an attr flag to all pmus.
>
> Does this make sense?

We need to require exclude_guest when using precise attribute with perf 
else all running VMs on Intel-based servers will crash. I do not have an 
AMD based server to even attempt the preferred solution. Best I can do 
is to attempt to keep this thread alive until someone with one can 
tackle the problem.

David

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Peter Zijlstra]

On Wed, 2012-09-05 at 09:43 -0600, David Ahern wrote:
> 
> We need to require exclude_guest when using precise attribute with perf 
> else all running VMs on Intel-based servers will crash. I do not have an 
> AMD based server to even attempt the preferred solution.

I've recently gotten a machine from AMD that would allow me to test this
stuff, so I'll put writing that somewhere near the top of the todo.

>  Best I can do 
> is to attempt to keep this thread alive until someone with one can 
> tackle the problem. 

Right you are.. keep doing this until its sorted.

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Robert Richter]

David,

On 06.09.12 00:07:52, Peter Zijlstra wrote:
> On Wed, 2012-09-05 at 09:43 -0600, David Ahern wrote:
> > We need to require exclude_guest when using precise attribute with perf 
> > else all running VMs on Intel-based servers will crash. I do not have an 
> > AMD based server to even attempt the preferred solution.

do you have a perf tool patch ready that enables exclude_guest per
default for precise events? I have ibs kernel code that returns EINVAL
in this case.  Current perf tool should then fall back by sending
another syscall with the exclude_guest modifier disabled. This should
work for ibs. Testing your patch wouln't be an effort for me.

To prevent Intel-based servers from crashing, pebs should be only
enabled if the exclude_guest modifier is enabled. I guess there is
already kernel code to prevent this, but didn't look at the sources.

To prevent probing the kernel with syscalls to detect pmu features, I
suggested some time ago to introduce

 /sys/bus/event_source/devices/*/flags

or so similar to /proc/cpuinfo:flags. Any opinions on that?

There is also the option to emulate exclude_guest for ibs in
software. So far I didn't have the time to look at this. We could
still add this in the future. Since we need to fix current mainline
anyway where such a patch wouldn't go in for v3.6, I will send my
current ibs kernel fixes I mentioned above next days. It would be
great to test this also with your patch.

Thanks,

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[David Ahern]

On 9/6/12 11:44 AM, Robert Richter wrote:
> David,
>
> On 06.09.12 00:07:52, Peter Zijlstra wrote:
>> On Wed, 2012-09-05 at 09:43 -0600, David Ahern wrote:
>>> We need to require exclude_guest when using precise attribute with perf
>>> else all running VMs on Intel-based servers will crash. I do not have an
>>> AMD based server to even attempt the preferred solution.
>
> do you have a perf tool patch ready that enables exclude_guest per
> default for precise events? I have ibs kernel code that returns EINVAL
> in this case.  Current perf tool should then fall back by sending
> another syscall with the exclude_guest modifier disabled. This should
> work for ibs. Testing your patch wouln't be an effort for me.

The beginning of this thread:
     https://lkml.org/lkml/2012/7/20/437

Takes the route of setting exclude_guest if precise is specified. This 
is simpler than adding yet another failure detect with fallback.


>
> To prevent Intel-based servers from crashing, pebs should be only
> enabled if the exclude_guest modifier is enabled. I guess there is
> already kernel code to prevent this, but didn't look at the sources.

Peter's patch takes care of this, but it can't be applied until 
userspace code is updated to avoid userspace breakage. And the userspace 
patch can't be applied until AMD side handles the exclude_guest setting.

>
> To prevent probing the kernel with syscalls to detect pmu features, I
> suggested some time ago to introduce
>
>   /sys/bus/event_source/devices/*/flags
>
> or so similar to /proc/cpuinfo:flags. Any opinions on that?

I think that is up to Peter, Ingo, Arnaldo.

David

>
> There is also the option to emulate exclude_guest for ibs in
> software. So far I didn't have the time to look at this. We could
> still add this in the future. Since we need to fix current mainline
> anyway where such a patch wouldn't go in for v3.6, I will send my
> current ibs kernel fixes I mentioned above next days. It would be
> great to test this also with your patch.
>
> Thanks,
>
> -Robert
>

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Robert Richter]

David,

just found your patch...

On 20.07.12 17:25:53, David Ahern wrote:
> PEBS cannot be used with guest mode. If user adds :p modifier set
> exclude_guest as well.

> @@ -653,6 +653,9 @@ int parse_events_modifier(struct list_head *list, char *str)
>  			eH = 0;
>  		} else if (*str == 'p') {
>  			precise++;
> +			/* use of precise requires exclude_guest */
> +			if (!exclude_GH)
> +				eG = 1;

We should only enable exclude_guest if no G/H modifiers are given on
the command line. If I correctly read the code this is indicated by
exclude_GH and correctly implemented.

To summarize how I think it should work:

 -e cycles:p ..... Enable exclude_guest bit. If the syscall fails,
                   fall back by disabling the guest bit and send
                   syscall again

 -e cycles:pGH ... Do not enable exclude_guest/_host. Do not fall back
                   on syscall errors.

Same for the case the p modifier is not set.

So we have the following:

 G           ... set exclude_host bit, clear exclude_guest
 H           ... set exclude_guest bit, clear exclude_host
 GH          ... clear both exclude_guest/_host
 <no G or H> ... default (set exclude_host bit, clear exclude_guest),
                 but fall back on syscall errors to clear both
                 exclude_guest/_host

The modifier parser should be correctly implemented and should work
with ibs. Will test this patch.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[David Ahern]

On 9/6/12 1:02 PM, Robert Richter wrote:
> David,
>
> just found your patch...
>
> On 20.07.12 17:25:53, David Ahern wrote:
>> PEBS cannot be used with guest mode. If user adds :p modifier set
>> exclude_guest as well.
>
>> @@ -653,6 +653,9 @@ int parse_events_modifier(struct list_head *list, char *str)
>>   			eH = 0;
>>   		} else if (*str == 'p') {
>>   			precise++;
>> +			/* use of precise requires exclude_guest */
>> +			if (!exclude_GH)
>> +				eG = 1;
>
> We should only enable exclude_guest if no G/H modifiers are given on
> the command line. If I correctly read the code this is indicated by
> exclude_GH and correctly implemented.

yes.

>
> To summarize how I think it should work:
>
>   -e cycles:p ..... Enable exclude_guest bit. If the syscall fails,
>                     fall back by disabling the guest bit and send
>                     syscall again

I think the fallback scenarios are getting out of hand -- too many and 
too many combinations. For this case (:p only) I proposed setting 
exclude_guest. It should not fail if the pebs/ibs is supported. If so, 
give the user a helpful message. e.g., https://lkml.org/lkml/2012/7/20/440


>
>   -e cycles:pGH ... Do not enable exclude_guest/_host. Do not fall back
>                     on syscall errors.

In this case give the user a message that the requested combination is 
invalid. I have a patch similar to the one above for this.

>
> Same for the case the p modifier is not set.
>
> So we have the following:
>
>   G           ... set exclude_host bit, clear exclude_guest
>   H           ... set exclude_guest bit, clear exclude_host
>   GH          ... clear both exclude_guest/_host
>   <no G or H> ... default (set exclude_host bit, clear exclude_guest),
>                   but fall back on syscall errors to clear both
>                   exclude_guest/_host

you lost me on the last one. If the user specifies no attributes on the 
events then exclude_host/exclude_guest is controlled by the 
perf_host/perf_guest variables - see event_attr_init().

>
> The modifier parser should be correctly implemented and should work
> with ibs. Will test this patch.

Thanks!

David

Re: [PATCH 08/11] perf tool: precise mode requires exclude_guest

[Robert Richter]

On 06.09.12 13:17:19, David Ahern wrote:
> On 9/6/12 1:02 PM, Robert Richter wrote:
> > just found your patch...

I tested the patch and it runs ok with my kernel patch that checks
syscall attributes for ibs. perf tools falls back and clears
exclude_guest/_host bits in attr. It also should work if the kernel
ignores those bits (without my patch).

Acked-by: Robert Richter <robert.richter@amd.com>

> > To summarize how I think it should work:
> >
> >   -e cycles:p ..... Enable exclude_guest bit. If the syscall fails,
> >                     fall back by disabling the guest bit and send
> >                     syscall again
> 
> I think the fallback scenarios are getting out of hand -- too many and 
> too many combinations. For this case (:p only) I proposed setting 
> exclude_guest. It should not fail if the pebs/ibs is supported. If so, 
> give the user a helpful message. e.g., https://lkml.org/lkml/2012/7/20/440
> 
> 
> >
> >   -e cycles:pGH ... Do not enable exclude_guest/_host. Do not fall back
> >                     on syscall errors.
> 
> In this case give the user a message that the requested combination is 
> invalid. I have a patch similar to the one above for this.

Yes, but cases where only guest or host-only mode is supported are
rare.

> 
> >
> > Same for the case the p modifier is not set.
> >
> > So we have the following:
> >
> >   G           ... set exclude_host bit, clear exclude_guest
> >   H           ... set exclude_guest bit, clear exclude_host
> >   GH          ... clear both exclude_guest/_host
> >   <no G or H> ... default (set exclude_host bit, clear exclude_guest),
> >                   but fall back on syscall errors to clear both
> >                   exclude_guest/_host
> 
> you lost me on the last one. If the user specifies no attributes on the 
> events then exclude_host/exclude_guest is controlled by the 
> perf_host/perf_guest variables - see event_attr_init().

As you said above, falling back by disabling flags becomes unhandy.
Current implementation falls back without (?) notice even if the user
explicitly demanded G or H. Falling back should only be the case if no
G/H modifiers are given. This is what I meant here.

> > The modifier parser should be correctly implemented and should work
> > with ibs. Will test this patch.
> 
> Thanks!

Sorry far being late a bit with all this.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH] perf, ibs: Check syscall attribute flags

[Robert Richter]

On 06.09.12 21:56:45, Robert Richter wrote:
> I tested the patch and it runs ok with my kernel patch that checks
> syscall attributes for ibs. perf tools falls back and clears
> exclude_guest/_host bits in attr.

This is the patch that checks syscall attributes for ibs. Should be
for tip:perf/urgent for ABI compatibility.

-Robert


>From 1d037614edef576da441936bd8c917d31f57b179 Mon Sep 17 00:00:00 2001
From: Robert Richter <robert.richter@amd.com>
Date: Wed, 25 Jul 2012 19:12:45 +0200
Subject: [PATCH] perf, ibs: Check syscall attribute flags

Current implementation simply ignores attribute flags. Thus, there is
no notification to userland of unsupported features. Check syscall's
attribute flags to let userland know if a feature is supported by the
kernel. This is also needed to distinguish between future kernels what
might support a feature.

Cc: <stable@vger.kernel.org> v3.5..
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/kernel/cpu/perf_event_amd_ibs.c |   11 +++++++++++
 include/linux/perf_event.h               |    2 ++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd_ibs.c b/arch/x86/kernel/cpu/perf_event_amd_ibs.c
index 7bfb5be..0456061 100644
--- a/arch/x86/kernel/cpu/perf_event_amd_ibs.c
+++ b/arch/x86/kernel/cpu/perf_event_amd_ibs.c
@@ -215,6 +215,14 @@ static int perf_ibs_init(struct perf_event *event)
 	struct perf_ibs *perf_ibs;
 	u64 max_cnt, config;
 	int ret;
+	struct perf_event_attr notsupp = {
+		.exclude_user	= 1,
+		.exclude_kernel	= 1,
+		.exclude_hv	= 1,
+		.exclude_idle	= 1,
+		.exclude_host	= 1,
+		.exclude_guest	= 1,
+	};
 
 	perf_ibs = get_ibs_pmu(event->attr.type);
 	if (perf_ibs) {
@@ -229,6 +237,9 @@ static int perf_ibs_init(struct perf_event *event)
 	if (event->pmu != &perf_ibs->pmu)
 		return -ENOENT;
 
+	if (perf_flags(&event->attr) & perf_flags(&notsupp))
+		return -EINVAL;
+
 	if (config & ~perf_ibs->config_mask)
 		return -EINVAL;
 
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 28f9cee..c36a04f 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -304,6 +304,8 @@ struct perf_event_attr {
 	__u32	__reserved_2;
 };
 
+#define perf_flags(attr)	(*(&(attr)->read_format + 1))
+
 /*
  * Ioctls that can be done on a perf event fd:
  */
-- 
1.7.8.6




-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH] perf, ibs: Check syscall attribute flags

[David Ahern]

On 9/7/12 10:41 AM, Robert Richter wrote:
> +	struct perf_event_attr notsupp = {
> +		.exclude_user	= 1,
> +		.exclude_kernel	= 1,
> +		.exclude_hv	= 1,
> +		.exclude_idle	= 1,
> +		.exclude_host	= 1,
> +		.exclude_guest	= 1,
> +	};
>
>   	perf_ibs = get_ibs_pmu(event->attr.type);
>   	if (perf_ibs) {
> @@ -229,6 +237,9 @@ static int perf_ibs_init(struct perf_event *event)
>   	if (event->pmu != &perf_ibs->pmu)
>   		return -ENOENT;
>
> +	if (perf_flags(&event->attr) & perf_flags(&notsupp))
> +		return -EINVAL;
> +

Am I reading this right - if exclude_guest is set then perf_ibs_init 
returns -EINVAL?

David

Re: [PATCH] perf, ibs: Check syscall attribute flags

[Peter Zijlstra]

On Fri, 2012-09-07 at 18:41 +0200, Robert Richter wrote:
> From 1d037614edef576da441936bd8c917d31f57b179 Mon Sep 17 00:00:00 2001
> From: Robert Richter <robert.richter@amd.com>
> Date: Wed, 25 Jul 2012 19:12:45 +0200
> Subject: [PATCH] perf, ibs: Check syscall attribute flags
> 
> Current implementation simply ignores attribute flags. Thus, there is
> no notification to userland of unsupported features. Check syscall's
> attribute flags to let userland know if a feature is supported by the
> kernel. This is also needed to distinguish between future kernels what
> might support a feature.
> 
> Cc: <stable@vger.kernel.org> v3.5..
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  arch/x86/kernel/cpu/perf_event_amd_ibs.c |   11 +++++++++++
>  include/linux/perf_event.h               |    2 ++
>  2 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_event_amd_ibs.c b/arch/x86/kernel/cpu/perf_event_amd_ibs.c
> index 7bfb5be..0456061 100644
> --- a/arch/x86/kernel/cpu/perf_event_amd_ibs.c
> +++ b/arch/x86/kernel/cpu/perf_event_amd_ibs.c
> @@ -215,6 +215,14 @@ static int perf_ibs_init(struct perf_event *event)
>         struct perf_ibs *perf_ibs;
>         u64 max_cnt, config;
>         int ret;
> +       struct perf_event_attr notsupp = {

static const ?

> +               .exclude_user   = 1,
> +               .exclude_kernel = 1,
> +               .exclude_hv     = 1,
> +               .exclude_idle   = 1,
> +               .exclude_host   = 1,
> +               .exclude_guest  = 1,

Ideally we'd grow support for those using SVM entry/exit hooks though.

> +       };
>  
>         perf_ibs = get_ibs_pmu(event->attr.type);
>         if (perf_ibs) {
> @@ -229,6 +237,9 @@ static int perf_ibs_init(struct perf_event *event)
>         if (event->pmu != &perf_ibs->pmu)
>                 return -ENOENT;
>  
> +       if (perf_flags(&event->attr) & perf_flags(&notsupp))
> +               return -EINVAL;
> +
>         if (config & ~perf_ibs->config_mask)
>                 return -EINVAL;
>  
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 28f9cee..c36a04f 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -304,6 +304,8 @@ struct perf_event_attr {
>         __u32   __reserved_2;
>  };
>  
> +#define perf_flags(attr)       (*(&(attr)->read_format + 1))


Another anonymous union shouldn't hurt..

---
 include/linux/perf_event.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index cc5e2cd..5df37a0 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -236,7 +236,9 @@ struct perf_event_attr {
 	__u64			sample_type;
 	__u64			read_format;
 
-	__u64			disabled       :  1, /* off by default        */
+	union {
+		__u64		flags;
+		__u64		disabled       :  1, /* off by default        */
 				inherit	       :  1, /* children inherit it   */
 				pinned	       :  1, /* must always be on PMU */
 				exclusive      :  1, /* only group on PMU     */
@@ -272,6 +274,7 @@ struct perf_event_attr {
 				exclude_callchain_user   : 1, /* exclude user callchains */
 
 				__reserved_1   : 41;
+	};
 
 	union {
 		__u32		wakeup_events;	  /* wakeup every n events */

Re: [PATCH] perf, ibs: Check syscall attribute flags

[Robert Richter]

On 07.09.12 10:50:35, David Ahern wrote:
> On 9/7/12 10:41 AM, Robert Richter wrote:
> > +	struct perf_event_attr notsupp = {
> > +		.exclude_user	= 1,
> > +		.exclude_kernel	= 1,
> > +		.exclude_hv	= 1,
> > +		.exclude_idle	= 1,
> > +		.exclude_host	= 1,
> > +		.exclude_guest	= 1,
> > +	};
> >
> >   	perf_ibs = get_ibs_pmu(event->attr.type);
> >   	if (perf_ibs) {
> > @@ -229,6 +237,9 @@ static int perf_ibs_init(struct perf_event *event)
> >   	if (event->pmu != &perf_ibs->pmu)
> >   		return -ENOENT;
> >
> > +	if (perf_flags(&event->attr) & perf_flags(&notsupp))
> > +		return -EINVAL;
> > +
> 
> Am I reading this right - if exclude_guest is set then perf_ibs_init 
> returns -EINVAL?

Yes, the hardware does not support this. I will look for a solution
which emulates this is software.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH] perf, ibs: Check syscall attribute flags

[David Ahern]

On 9/7/12 11:07 AM, Robert Richter wrote:
>> Am I reading this right - if exclude_guest is set then perf_ibs_init
>> returns -EINVAL?
>
> Yes, the hardware does not support this. I will look for a solution
> which emulates this is software.

ugh, that's exactly we need for Intel: if precise is set, exclude_guest 
must be set.

In perf I would like to set exclude_guest if precise is set -- what I 
mentioned yesterday for the patch https://lkml.org/lkml/2012/7/20/437. 
With my patch -e cycles:p on AMD will then fail EINVAL.

David

Re: [PATCH] perf, ibs: Check syscall attribute flags

[Robert Richter]

On 07.09.12 18:56:27, Peter Zijlstra wrote:

> > +               .exclude_user   = 1,
> > +               .exclude_kernel = 1,
> > +               .exclude_hv     = 1,
> > +               .exclude_idle   = 1,
> > +               .exclude_host   = 1,
> > +               .exclude_guest  = 1,
> 
> Ideally we'd grow support for those using SVM entry/exit hooks though.

Yes, I will look at this too. This patch is inteded for stable and
urgent.

> > +#define perf_flags(attr)       (*(&(attr)->read_format + 1))
> 
> 
> Another anonymous union shouldn't hurt..
> 
> ---
>  include/linux/perf_event.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index cc5e2cd..5df37a0 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -236,7 +236,9 @@ struct perf_event_attr {
>  	__u64			sample_type;
>  	__u64			read_format;
>  
> -	__u64			disabled       :  1, /* off by default        */
> +	union {
> +		__u64		flags;
> +		__u64		disabled       :  1, /* off by default        */
>  				inherit	       :  1, /* children inherit it   */
>  				pinned	       :  1, /* must always be on PMU */
>  				exclusive      :  1, /* only group on PMU     */
> @@ -272,6 +274,7 @@ struct perf_event_attr {
>  				exclude_callchain_user   : 1, /* exclude user callchains */
>  
>  				__reserved_1   : 41;
> +	};
>  
>  	union {
>  		__u32		wakeup_events;	  /* wakeup every n events */

I was thinking of this too. But this breaks existing code to compile
since static initialization of struct perf_event_attr fails, e.g.:

 builtin-test.c:469:3: error: unknown field ‘watermark’ specified in initializer

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH] perf, ibs: Check syscall attribute flags

[David Ahern]

On 9/7/12 11:11 AM, David Ahern wrote:
> On 9/7/12 11:07 AM, Robert Richter wrote:
>>> Am I reading this right - if exclude_guest is set then perf_ibs_init
>>> returns -EINVAL?
>>
>> Yes, the hardware does not support this. I will look for a solution
>> which emulates this is software.
>
> ugh, that's exactly we need for Intel: if precise is set, exclude_guest
> must be set.
>
> In perf I would like to set exclude_guest if precise is set -- what I
> mentioned yesterday for the patch https://lkml.org/lkml/2012/7/20/437.
> With my patch -e cycles:p on AMD will then fail EINVAL.

I see now... intel returns ENOTSUP if exclude_guest is not set, amd 
returns EINVAL if it is set.

For the AMD case the fallback_missing_features code kicks in for 
perf-top and perf-record; I just need to fix up the pr_debug for that case.

David

Re: [PATCH] perf, ibs: Check syscall attribute flags

[Robert Richter]

On 07.09.12 11:11:56, David Ahern wrote:
> On 9/7/12 11:07 AM, Robert Richter wrote:
> >> Am I reading this right - if exclude_guest is set then perf_ibs_init
> >> returns -EINVAL?
> >
> > Yes, the hardware does not support this. I will look for a solution
> > which emulates this is software.
> 
> ugh, that's exactly we need for Intel: if precise is set, exclude_guest 
> must be set.
> 
> In perf I would like to set exclude_guest if precise is set -- what I 
> mentioned yesterday for the patch https://lkml.org/lkml/2012/7/20/437. 
> With my patch -e cycles:p on AMD will then fail EINVAL.

You may set this bit per default. On EINVAL perf falls back and clears
the bit. I tested exactly this for ibs and it works.

So, on Intel, if the guest_exclude bit is not set, the kernel simply
throws an EINVAL too, but since perf sets it per default with the
precise modifier there shouldn't be any problem here.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH] perf, ibs: Check syscall attribute flags

[Peter Zijlstra]

On Fri, 2012-09-07 at 19:18 +0200, Robert Richter wrote:
> I was thinking of this too. But this breaks existing code to compile
> since static initialization of struct perf_event_attr fails, e.g.:
> 
>  builtin-test.c:469:3: error: unknown field ‘watermark’ specified in initializer
> 
> 
Oh bugger, I only compiled the kernel part :/

[PATCH -v2] perf, ibs: Check syscall attribute flags

[Robert Richter]

On 07.09.12 18:56:27, Peter Zijlstra wrote:
> > @@ -215,6 +215,14 @@ static int perf_ibs_init(struct perf_event *event)
> >         struct perf_ibs *perf_ibs;
> >         u64 max_cnt, config;
> >         int ret;
> > +       struct perf_event_attr notsupp = {
> 
> static const ?
> 
> > +               .exclude_user   = 1,
> > +               .exclude_kernel = 1,
> > +               .exclude_hv     = 1,
> > +               .exclude_idle   = 1,
> > +               .exclude_host   = 1,
> > +               .exclude_guest  = 1,

Version 2 below with static const used.

-Robert


>From faf80df002709ec95178cf8a6f3f7298200ac1a7 Mon Sep 17 00:00:00 2001
From: Robert Richter <robert.richter@amd.com>
Date: Wed, 25 Jul 2012 19:12:45 +0200
Subject: [PATCH] perf, ibs: Check syscall attribute flags

Current implementation simply ignores attribute flags. Thus, there is
no notification to userland of unsupported features. Check syscall's
attribute flags to let userland know if a feature is supported by the
kernel. This is also needed to distinguish between future kernels what
might support a feature.

Cc: <stable@vger.kernel.org> v3.5..
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/kernel/cpu/perf_event_amd_ibs.c |   12 ++++++++++++
 include/linux/perf_event.h               |    2 ++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd_ibs.c b/arch/x86/kernel/cpu/perf_event_amd_ibs.c
index 7bfb5be..eebd5ff 100644
--- a/arch/x86/kernel/cpu/perf_event_amd_ibs.c
+++ b/arch/x86/kernel/cpu/perf_event_amd_ibs.c
@@ -209,6 +209,15 @@ static int perf_ibs_precise_event(struct perf_event *event, u64 *config)
 	return -EOPNOTSUPP;
 }
 
+static const struct perf_event_attr ibs_notsupp = {
+	.exclude_user	= 1,
+	.exclude_kernel	= 1,
+	.exclude_hv	= 1,
+	.exclude_idle	= 1,
+	.exclude_host	= 1,
+	.exclude_guest	= 1,
+};
+
 static int perf_ibs_init(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
@@ -229,6 +238,9 @@ static int perf_ibs_init(struct perf_event *event)
 	if (event->pmu != &perf_ibs->pmu)
 		return -ENOENT;
 
+	if (perf_flags(&event->attr) & perf_flags(&ibs_notsupp))
+		return -EINVAL;
+
 	if (config & ~perf_ibs->config_mask)
 		return -EINVAL;
 
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 28f9cee..c36a04f 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -304,6 +304,8 @@ struct perf_event_attr {
 	__u32	__reserved_2;
 };
 
+#define perf_flags(attr)	(*(&(attr)->read_format + 1))
+
 /*
  * Ioctls that can be done on a perf event fd:
  */
-- 
1.7.8.6




-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH] perf, ibs: Check syscall attribute flags

[Robert Richter]

On 07.09.12 11:20:19, David Ahern wrote:
> I see now... intel returns ENOTSUP if exclude_guest is not set, amd 
> returns EINVAL if it is set.
> 
> For the AMD case the fallback_missing_features code kicks in for 
> perf-top and perf-record; I just need to fix up the pr_debug for that case.

It is EOPNOTSUP. I preferred returning EOPNOTSUP too. But then I saw
that fallback code is implemented to work mostly with EINVAL. It seems
to be the default answer to a syscall. ;)

There are several pieces of code in perf that implement fallback code,
it's hard to find it all and this already caused some bugs in the
past. I was looking at this to unify it, but it was too much effort.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH] perf, ibs: Check syscall attribute flags

[Arnaldo Carvalho de Melo]

Em Mon, Sep 10, 2012 at 03:07:00PM +0200, Robert Richter escreveu:
> On 07.09.12 11:20:19, David Ahern wrote:
> > I see now... intel returns ENOTSUP if exclude_guest is not set, amd 
> > returns EINVAL if it is set.

> > For the AMD case the fallback_missing_features code kicks in for 
> > perf-top and perf-record; I just need to fix up the pr_debug for that case.

> It is EOPNOTSUP. I preferred returning EOPNOTSUP too. But then I saw
> that fallback code is implemented to work mostly with EINVAL. It seems
> to be the default answer to a syscall. ;)

> There are several pieces of code in perf that implement fallback code,
> it's hard to find it all and this already caused some bugs in the
> past. I was looking at this to unify it, but it was too much effort.

Indeed these all need to be moved to perf_ev{sel,list}__open, its
something I meant to do long ago, will try again to consolidate that.

- Arnaldo

[tip:perf/urgent] perf/x86/ibs: Check syscall attribute flags

[tip-bot for Robert Richter]

Commit-ID:  bad9ac2d7f878a31cf1ae8c1ee3768077d222bcb
Gitweb:     http://git.kernel.org/tip/bad9ac2d7f878a31cf1ae8c1ee3768077d222bcb
Author:     Robert Richter <robert.richter@amd.com>
AuthorDate: Wed, 25 Jul 2012 19:12:45 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 13 Sep 2012 16:59:48 +0200

perf/x86/ibs: Check syscall attribute flags

Current implementation simply ignores attribute flags. Thus, there is
no notification to userland of unsupported features. Check syscall's
attribute flags to let userland know if a feature is supported by the
kernel. This is also needed to distinguish between future kernels what
might support a feature.

Cc: <stable@vger.kernel.org> v3.5..
Signed-off-by: Robert Richter <robert.richter@amd.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20120910093018.GO8285@erda.amd.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/perf_event_amd_ibs.c |   12 ++++++++++++
 include/linux/perf_event.h               |    2 ++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_amd_ibs.c b/arch/x86/kernel/cpu/perf_event_amd_ibs.c
index 7bfb5be..eebd5ff 100644
--- a/arch/x86/kernel/cpu/perf_event_amd_ibs.c
+++ b/arch/x86/kernel/cpu/perf_event_amd_ibs.c
@@ -209,6 +209,15 @@ static int perf_ibs_precise_event(struct perf_event *event, u64 *config)
 	return -EOPNOTSUPP;
 }
 
+static const struct perf_event_attr ibs_notsupp = {
+	.exclude_user	= 1,
+	.exclude_kernel	= 1,
+	.exclude_hv	= 1,
+	.exclude_idle	= 1,
+	.exclude_host	= 1,
+	.exclude_guest	= 1,
+};
+
 static int perf_ibs_init(struct perf_event *event)
 {
 	struct hw_perf_event *hwc = &event->hw;
@@ -229,6 +238,9 @@ static int perf_ibs_init(struct perf_event *event)
 	if (event->pmu != &perf_ibs->pmu)
 		return -ENOENT;
 
+	if (perf_flags(&event->attr) & perf_flags(&ibs_notsupp))
+		return -EINVAL;
+
 	if (config & ~perf_ibs->config_mask)
 		return -EINVAL;
 
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 33ed9d6..bdb4161 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -274,6 +274,8 @@ struct perf_event_attr {
 	__u64	branch_sample_type; /* enum branch_sample_type */
 };
 
+#define perf_flags(attr)	(*(&(attr)->read_format + 1))
+
 /*
  * Ioctls that can be done on a perf event fd:
  */