perf tool: assorted cleanups and bug fixes

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 <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
 tools/perf/util/map.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

tools/perf/util/map.c +16 -1

index a1f4e36..8668569 100644
@@ -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)

Signature · 1 line

1.7.10.1

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

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 <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jiri Olsa <[email protected]>

Signed-off-by David Ahern <[email protected]>

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 <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
 tools/perf/util/map.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

tools/perf/util/map.c +1 -0

index 16d783d..cc33486 100644
@@ -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)) {

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

Signed-off-by David Ahern <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Gleb Natapov <[email protected]>

Link https://lkml.org/lkml/2012/7/9/264

---
 tools/perf/util/parse-events.c |    3 +++
 1 file changed, 3 insertions(+)

tools/perf/util/parse-events.c +3 -0

index 1aa721d..f34629b 100644
@@ -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;
 

Signature · 1 line

1.7.10.1

Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:

Quoted 2 lines

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?

Quoted 31 lines

Signed-off-by David Ahern <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Gleb Natapov <[email protected]>

Link https://lkml.org/lkml/2012/7/9/264

---
  tools/perf/util/parse-events.c |    3 +++
  1 file changed, 3 insertions(+)

tools/perf/util/parse-events.c +3 -1

index 1aa721d..f34629b 100644
@@ -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

Quoted 6 lines · Arnaldo Carvalho de Melo

Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:

Quoted 2 lines

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,

Quoted 11 lines · David Ahern

Quoted 6 lines · Arnaldo Carvalho de Melo

Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:

Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 3 lines

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?

Em Tue, Jul 24, 2012 at 11:28:48AM -0600, David Ahern escreveu:

Quoted 12 lines · Robert Richter

Quoted 8 lines

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.

Quoted 1 line

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

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.

Quoted 16 lines · David Ahern

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.

Quoted 6 lines

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.

Quoted 18 lines · Peter Zijlstra

Quoted 16 lines · David Ahern

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.

Quoted 8 lines

Quoted 6 lines

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?

Quoted 18 lines · Peter Zijlstra

Quoted 16 lines · David Ahern

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.

Quoted 8 lines

Quoted 6 lines

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.

Quoted 3 lines · David Ahern

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.

David,

Quoted 4 lines · Peter Zijlstra

Quoted 3 lines · David Ahern

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

David,

Quoted 11 lines · David Ahern

Quoted 6 lines · Arnaldo Carvalho de Melo

Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:

Quoted 2 lines

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.

Quoted 17 lines · Robert Richter

David,

Quoted 11 lines · David Ahern

Quoted 6 lines · Arnaldo Carvalho de Melo

Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:

Quoted 2 lines

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.

Quoted 8 lines

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?

Quoted 30 lines · Peter Zijlstra

Quoted 17 lines · Robert Richter

David,

Quoted 11 lines · David Ahern

Quoted 6 lines · Arnaldo Carvalho de Melo

Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:

Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 31 lines · Gleb Natapov

Quoted 30 lines · Peter Zijlstra

Quoted 17 lines · Robert Richter

David,

Quoted 11 lines · David Ahern

Quoted 6 lines · Arnaldo Carvalho de Melo

Em Fri, Jul 20, 2012 at 05:25:53PM -0600, David Ahern escreveu:

Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 3 lines · Gleb Natapov

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.

Quoted 12 lines · Peter Zijlstra

Quoted 10 lines · Robert Richter

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.

Quoted 5 lines

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.

David,

just found your patch...

Quoted 2 lines · David Ahern

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

tools/perf/util/parse-events.c int parse_events_modifier(struct list_head *list, char *str) · line 658

Quoted 7 lines

tools/perf/util/parse-events.c +3 -0

@@ -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.

Quoted 1 line · Robert Richter

just found your patch...

Quoted 5 lines

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

Quoted 1 line

Quoted 2 lines

-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.

Quoted 4 lines

Same for the case the p modifier is not set.

Quoted 8 lines

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().

Quoted 2 lines

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

Thanks!

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.

From 1d037614edef576da441936bd8c917d31f57b179 Mon Sep 17 00:00:00 2001

arch/x86/kernel/cpu/perf_event_amd_ibs.c +8 -0

+	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) {

Quoted 15 lines · Robert Richter

arch/x86/kernel/cpu/perf_event_amd_ibs.c +8 -0

+	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;
 

arch/x86/kernel/cpu/perf_event_amd_ibs.c static int perf_ibs_init(struct perf_event *event) · line 242

Quoted 3 lines

arch/x86/kernel/cpu/perf_event_amd_ibs.c +3 -0

+	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?

Quoted 2 lines

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.

Quoted 5 lines · Robert Richter

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 5 lines · Robert Richter

Quoted 2 lines

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.

From 1d037614edef576da441936bd8c917d31f57b179 Mon Sep 17 00:00:00 2001
From: Robert Richter <[email protected]>
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 <[email protected]> v3.5..

Signed-off-by Robert Richter <[email protected]>

---
 arch/x86/kernel/cpu/perf_event_amd_ibs.c |   11 +++++++++++
 include/linux/perf_event.h               |    2 ++
 2 files changed, 13 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/perf_event_amd_ibs.c +0 -0

index 7bfb5be..0456061 100644

arch/x86/kernel/cpu/perf_event_amd_ibs.c +4 -0

+       };
 
        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;
 

include/linux/perf_event.h +0 -0

index 28f9cee..c36a04f 100644

Review

Quoted 6 lines

(quoted diff) +6 -0

+               .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.

Quoted 1 line

+#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(-)

include/linux/perf_event.h +3 -1

index cc5e2cd..5df37a0 100644
@@ -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     */

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

Review static int perf_ibs_init(struct perf_event *event) · line 218

Quoted 5 lines

(quoted diff) +1 -0

@@ -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 ?

Quoted 6 lines

(quoted diff) +6 -0

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

From faf80df002709ec95178cf8a6f3f7298200ac1a7 Mon Sep 17 00:00:00 2001

tools/perf/builtin-top.c +0 -0

index e3cab5f..0ce665c 100644

Em Fri, Jul 20, 2012 at 05:25:54PM -0600, David Ahern escreveu:

Quoted 5 lines

tools/perf/builtin-top.c +0 -0

index e3cab5f..0ce665c 100644

tools/perf/builtin-top.c try_again: · line 949

Quoted 9 lines

tools/perf/builtin-top.c +4 -2

@@ -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) {

Quoted 9 lines

Em Fri, Jul 20, 2012 at 05:25:54PM -0600, David Ahern escreveu:

tools/perf/builtin-top.c try_again: · line 948

Quoted 3 lines

tools/perf/builtin-top.c +3 -0

+			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.

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 <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
 tools/perf/builtin-record.c |    4 ++++
 tools/perf/builtin-top.c    |    4 ++++
 2 files changed, 8 insertions(+)

tools/perf/builtin-record.c +4 -0

index f5a6452..911e300 100644
@@ -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");

tools/perf/builtin-top.c +4 -0

index 0ce665c..1706dc9 100644
@@ -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 "

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

'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 <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
 tools/perf/builtin-top.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

tools/perf/builtin-top.c +2 -2

index 1706dc9..6285374 100644
@@ -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];

Em Fri, Jul 20, 2012 at 05:25:56PM -0600, David Ahern escreveu:

Quoted 33 lines

'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 <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
  tools/perf/builtin-top.c |   14 ++++++++++++--
  1 file changed, 12 insertions(+), 2 deletions(-)

tools/perf/builtin-top.c +6 -2

index 1706dc9..6285374 100644
@@ -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);
+

tools/perf/builtin-top.c static void perf_event__process_sample(struct perf_tool *tool, · line 720

Quoted 6 lines

tools/perf/builtin-top.c +6 -0

+		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?

Quoted 7 lines

Em Fri, Jul 20, 2012 at 05:25:56PM -0600, David Ahern escreveu:

Quoted 5 lines

tools/perf/builtin-top.c +4 -0

+		static struct strlist *seen;
+		char pidstr[8];
 
+		if (!seen)
+			seen = strlist__new(true, NULL);

Quoted 7 lines

tools/perf/builtin-top.c static void perf_event__process_sample(struct perf_tool *tool, · line 719

Quoted 5 lines

tools/perf/builtin-top.c +5 -0

+		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.