perf: precise mode and exclude_guest

PATCH Superseded David Ahern 3 participants 15 messages Sep 10, 2012 – Sep 12, 2012

Subsystems (2)

x86 perf events

lore

Share & tools

lore

David Ahern

author

2012-09-10 16:40 UTC #0 lore raw

cover perf: precise mode and exclude_guest

Hopefully thi wraps up the precise mode-exclude_guest dependency. I'm sure someone will let me know if I screwed up the attribution in the second patch.

David Ahern (3):
perf tool: precise mode requires exclude_guest
perf: require exclude_guest to use PEBS - kernel side enforcement perf tool: give user better message if precise is not supported

 arch/x86/kernel/cpu/perf_event.c |    6 ++++++
 tools/perf/builtin-record.c      |    5 +++++
 tools/perf/builtin-top.c         |    4 ++++
 tools/perf/util/parse-events.c   |    3 +++
 4 files changed, 18 insertions(+)

Signature · 1 line

1.7.10.1

David Ahern

author

2012-09-10 16:40 UTC #1 lore raw

PATCH 1/3 perf tool: precise mode requires exclude_guest

PEBS cannot be used with guest mode. See:
https://lkml.org/lkml/2012/7/9/264

If user adds :p modifier set exclude_guest as well.

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

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Robert Richter <[email protected]>

Cc Gleb Natapov <[email protected]>

Cc Avi Kivity <[email protected]>

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

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

index a031ee1..f2c02b2 100644
@@ -694,6 +694,9 @@ static int get_event_modifier(struct event_modifier *mod, 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

Peter Zijlstra

maintainer

2012-09-10 17:23 UTC #2 lore raw

Re: [PATCH 1/3] perf tool: precise mode requires exclude_guest

Quoted 2 lines · David Ahern

PEBS cannot be used with guest mode. See:
   https://lkml.org/lkml/2012/7/9/264

Expanding and not relying on external stuff is so much better.

So in particular you want something like:

"Intel PEBS in VT-x context uses the DS address as a guest linear address, even though its programmed by the host as a host linear address. This either results in guest memory corruption and or the hardware faulting and 'crashing' the virtual machine

Therefore we have to disable PEBS on VT-x enter and re-enable on VT-x exit, enforcing a strict exclude_guest.

AMB IBS does work but doesn't currently support exclude_* at all, setting an exclude_* bit will make it fail.

... explain how one can use IBS if we auto-fallback to !precise ..."

Hmm ?

Quoted 26 lines

If user adds :p modifier set exclude_guest as well.
Signed-off-by David Ahern <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Robert Richter <[email protected]>
Cc Gleb Natapov <[email protected]>
Cc Avi Kivity <[email protected]>
---
 tools/perf/util/parse-events.c |    3 +++
 1 file changed, 3 insertions(+)
tools/perf/util/parse-events.c +3 -0
index a031ee1..f2c02b2 100644
@@ -694,6 +694,9 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
 			eH = 0;
 		} else if (*str == 'p') {
 			precise++;
+			/* use of precise requires exclude_guest */
+			if (!exclude_GH)
+				eG = 1;
 		} else
 			break;
 

David Ahern

author

2012-09-10 16:41 UTC #3 lore raw

PATCH 2/3 perf: require exclude_guest to use PEBS - kernel side enforcement

In reply to David Ahern

From: Peter Zijlstra <[email protected]>

See https://lkml.org/lkml/2012/7/9/298

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

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Robert Richter <[email protected]>

Cc Gleb Natapov <[email protected]>

Cc Avi Kivity <[email protected]>

---
 arch/x86/kernel/cpu/perf_event.c |    6 ++++++
 1 file changed, 6 insertions(+)

arch/x86/kernel/cpu/perf_event.c +6 -0

index 915b876..4bc96c5 100644
@@ -338,6 +338,9 @@ int x86_setup_perfctr(struct perf_event *event)
 		/* BTS is currently only allowed for user-mode. */
 		if (!attr->exclude_kernel)
 			return -EOPNOTSUPP;
+
+		if (!attr->exclude_guest)
+			return -EOPNOTSUPP;
 	}
 
 	hwc->config |= config;

@@ -380,6 +383,9 @@ int x86_pmu_hw_config(struct perf_event *event)
 	if (event->attr.precise_ip) {
 		int precise = 0;
 
+		if (!event->attr.exclude_guest)
+				return -EOPNOTSUPP;
+
 		/* Support for constant skid */
 		if (x86_pmu.pebs_active && !x86_pmu.pebs_broken) {
 			precise++;

Signature · 1 line

1.7.10.1

Peter Zijlstra

maintainer

2012-09-10 17:24 UTC #4 lore raw

Re: [PATCH 2/3] perf: require exclude_guest to use PEBS - kernel side enforcement

Quoted 3 lines · David Ahern

From: Peter Zijlstra <[email protected]>
See https://lkml.org/lkml/2012/7/9/298

Expanding that a little would be so much better.. take some of the reply to 1/3 on why we have to enforce a strict exclude_guest.

David Ahern

author

2012-09-10 16:40 UTC #5 lore raw

PATCH 3/3 perf tool: give user better message if precise is not supported

In reply to David Ahern

Platforms (e.g., VM's) without support for precise mode get a confusing error message. e.g.,

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

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

which is not clear that precise mode might be the root problem. With this patch:

$ perf record -e cycles:p -fo /tmp/perf.data -- sleep 1
  Error:
  'precise' request not supported. Try removing 'p' modifier
  sleep: Terminated

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

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Robert Richter <[email protected]>

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

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

index 7b8b891..ccf529b 100644
@@ -294,6 +294,11 @@ try_again:
 					  perf_evsel__name(pos));
 				rc = -err;
 				goto out;
+			} else if ((err == ENOTSUP) && (attr->precise_ip)) {
+				ui__error("\'precise\' request not supported. "
+					  "Try removing 'p' modifier\n");
+				rc = -err;
+				goto out;
 			}
 
 			printf("\n");

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

index 0513aaa..0d3653b 100644
@@ -975,6 +975,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("\'precise\' request not supported. "
+					  "Try removing 'p' modifier\n");
+				goto out_err;
 			}
 
 			ui__error("The sys_perf_event_open() syscall "

Signature · 1 line

1.7.10.1

Robert Richter

2012-09-11 09:21 UTC #6 lore raw

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

tools/perf/builtin-record.c try_again: · line 297

Quoted 7 lines · David Ahern

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

@@ -294,6 +294,11 @@ try_again:
 					  perf_evsel__name(pos));
 				rc = -err;
 				goto out;
+			} else if ((err == ENOTSUP) && (attr->precise_ip)) {

It is EOPNOTSUPP, did you test this?

tools/perf/builtin-record.c try_again: · line 299

Quoted 2 lines

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

+				ui__error("\'precise\' request not supported. "
+					  "Try removing 'p' modifier\n");

I would better print "... request may not be supported.", since you don't know for sure if this is the real cause.

Quoted 10 lines

tools/perf/builtin-record.c +2 -0
+				rc = -err;
+				goto out;
 			}
 
 			printf("\n");
tools/perf/builtin-top.c +0 -0
index 0513aaa..0d3653b 100644

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

Quoted 6 lines

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

@@ -975,6 +975,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("\'precise\' request not supported. "
+					  "Try removing 'p' modifier\n");

Same here.

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

Quoted 1 line

tools/perf/builtin-top.c +1 -0
+				goto out_err;

To avoid adding more duplicate code, maybe we should start to unify the code by implementing this in a shared helper function.

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

David Ahern

author

2012-09-11 13:22 UTC #7 lore raw

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

Quoted 10 lines · Robert Richter

tools/perf/builtin-record.c try_again: · line 297
Quoted 7 lines · David Ahern
tools/perf/builtin-record.c +1 -0
@@ -294,6 +294,11 @@ try_again:
  					  perf_evsel__name(pos));
  				rc = -err;
  				goto out;
+			} else if ((err == ENOTSUP) && (attr->precise_ip)) {
It is EOPNOTSUPP, did you test this?

I do not post patches without testing them. This particular patch was verified in a Virtual Machine (no PEBS) and using :pG modifier.

'egrep -r ENOTSUP tools/perf' shows hits in 3 other files, so I am not the only one using the shortcut. I'll change it in the follow up with better commit messages to make it consistent with patch 2.

Quoted 6 lines

tools/perf/builtin-record.c try_again: · line 299
Quoted 3 lines
tools/perf/builtin-record.c +2 -0
+				ui__error("\'precise\' request not supported. "
+					  "Try removing 'p' modifier\n");
I would better print "... request may not be supported.", since you don't know for sure if this is the real cause.

Sure I'll add the 'may not be'.

Quoted 20 lines

Quoted 11 lines

tools/perf/builtin-record.c +2 -0
+				rc = -err;
+				goto out;
  			}
 
  			printf("\n");
tools/perf/builtin-top.c +0 -0
index 0513aaa..0d3653b 100644

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

Quoted 6 lines

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

@@ -975,6 +975,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("\'precise\' request not supported. "
+					  "Try removing 'p' modifier\n");

Same here.

Quoted 4 lines

tools/perf/builtin-top.c try_again: · line 981
Quoted 1 line
tools/perf/builtin-top.c +1 -0
+				goto out_err;
To avoid adding more duplicate code, maybe we should start to unify the code by implementing this in a shared helper function.

Doing that requires additional modifications to not break perl and python scripts. Adding it in both commands here is consistent with all other open counter failures. Consolidation of those loops into a common base is known to do item.

David

Robert Richter

2012-09-11 14:02 UTC #8 lore raw

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

Quoted 11 lines · David Ahern

Quoted 10 lines · Robert Richter
tools/perf/builtin-record.c try_again: · line 297
Quoted 7 lines · David Ahern
tools/perf/builtin-record.c +1 -0
@@ -294,6 +294,11 @@ try_again:
  					  perf_evsel__name(pos));
  				rc = -err;
  				goto out;
+			} else if ((err == ENOTSUP) && (attr->precise_ip)) {
It is EOPNOTSUPP, did you test this?

Ok, wrong question. Better would have been: Did you run it on a non-pebs Intel machine of an non-ibs AMD machine?

Quoted 6 lines

I do not post patches without testing them. This particular patch was verified in a Virtual Machine (no PEBS) and using :pG modifier.
'egrep -r ENOTSUP tools/perf' shows hits in 3 other files, so I am not the only one using the shortcut. I'll change it in the follow up with better commit messages to make it consistent with patch 2.

For VM this might be valid. Don't know where ENOTSUP comes from. It is neither in kernel/events/ nor arch/x86/kernel/cpu/perf.

If you run this bare-metal on older machines which do not support pebs or ibs, the syscall returns EOPNOTSUPP. You can trigger the same behaviour on newer systems with:

 # perf record -e cycles:ppp -c 2097120 -R -a sleep 1

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

It should work in this case too.

Quoted 7 lines

Quoted 2 lines
To avoid adding more duplicate code, maybe we should start to unify the code by implementing this in a shared helper function.
Doing that requires additional modifications to not break perl and python scripts. Adding it in both commands here is consistent with all other open counter failures. Consolidation of those loops into a common base is known to do item.

I am fine with that too.

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

David Ahern

author

2012-09-11 14:33 UTC #9 lore raw

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

Quoted 3 lines · Robert Richter

Ok, wrong question. Better would have been: Did you run it on a non-pebs Intel machine of an non-ibs AMD machine?

Intel: yes. VM for example. All the servers I have now are Nehalem or better - ie., with a PEBS.

AMD: no. I do not have any AMD-based servers.

Quoted 10 lines

Quoted 7 lines
I do not post patches without testing them. This particular patch was verified in a Virtual Machine (no PEBS) and using :pG modifier.
'egrep -r ENOTSUP tools/perf' shows hits in 3 other files, so I am not the only one using the shortcut. I'll change it in the follow up with better commit messages to make it consistent with patch 2.
For VM this might be valid. Don't know where ENOTSUP comes from. It is neither in kernel/events/ nor arch/x86/kernel/cpu/perf.

My guess would be /usr/include/bits/errno.h:

/* Linux has no ENOTSUP error code. */

# define ENOTSUP EOPNOTSUPP

Quoted 11 lines

If you run this bare-metal on older machines which do not support pebs or ibs, the syscall returns EOPNOTSUPP. You can trigger the same behaviour on newer systems with:
  # perf record -e cycles:ppp -c 2097120 -R -a sleep 1
Error: sys_perf_event_open() syscall returned with 95 (Operation not supported). /bin/dmesg may provide additional information.
...
It should work in this case too.

The commit message was a copy and paste from the failure of both :p in a VM (PEBS is not supported in a VM). I also ran the bare metal case with :pG which per the second patch in this series generates the not supported message.

David

Robert Richter

2012-09-11 15:12 UTC #10 lore raw

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

Quoted 4 lines · David Ahern

My guess would be /usr/include/bits/errno.h:
/* Linux has no ENOTSUP error code. */
# define ENOTSUP EOPNOTSUPP

Ok, so ENOTSUP is actually the same as EOPNOTSUPP. Since the syscall returns a EOPNOTSUPP, I prefer this when checking perf_event_open() return codes. ENOTSUP is not used in the kernel. Was there a reason for choosing ENOTSUP?

Quoted 9 lines

Quoted 9 lines
If you run this bare-metal on older machines which do not support pebs or ibs, the syscall returns EOPNOTSUPP. You can trigger the same behaviour on newer systems with:
  # perf record -e cycles:ppp -c 2097120 -R -a sleep 1
Error: sys_perf_event_open() syscall returned with 95 (Operation not supported). /bin/dmesg may provide additional information.
...

Quoted 6 lines

Quoted 1 line
It should work in this case too.
The commit message was a copy and paste from the failure of both :p in a VM (PEBS is not supported in a VM). I also ran the bare metal case with :pG which per the second patch in this series generates the not supported message.

Since the error codes are the same, your code should work also on bare-metal. Can you test on a host using :ppp? This should trigger the same error message as in a vm.

Thanks,

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

David Ahern

author

2012-09-12 14:59 UTC #11 lore raw

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

Quoted 10 lines · Robert Richter

Quoted 4 lines · David Ahern
My guess would be /usr/include/bits/errno.h:
/* Linux has no ENOTSUP error code. */
# define ENOTSUP EOPNOTSUPP
Ok, so ENOTSUP is actually the same as EOPNOTSUPP. Since the syscall returns a EOPNOTSUPP, I prefer this when checking perf_event_open() return codes. ENOTSUP is not used in the kernel. Was there a reason for choosing ENOTSUP?

poor memory? laziness? a habit I was not aware I had acquired with using ENOTSUP? I mentioned in a prior response I would change it to EOPNOTSUPP to be consistent with the 2nd patch -- what the kernel is returning.

Quoted 12 lines

Quoted 12 lines
Quoted 11 lines
If you run this bare-metal on older machines which do not support pebs or ibs, the syscall returns EOPNOTSUPP. You can trigger the same behaviour on newer systems with:
   # perf record -e cycles:ppp -c 2097120 -R -a sleep 1
Error: sys_perf_event_open() syscall returned with 95 (Operation not supported). /bin/dmesg may provide additional information.
...
It should work in this case too.

Quoted 8 lines

Quoted 4 lines
The commit message was a copy and paste from the failure of both :p in a VM (PEBS is not supported in a VM). I also ran the bare metal case with :pG which per the second patch in this series generates the not supported message.
Since the error codes are the same, your code should work also on bare-metal. Can you test on a host using :ppp? This should trigger the same error message as in a vm.

As expected:

$ perf record -e cycles:ppp -a
Error:
'precise' request not supported. Try removing 'p' modifier

Resending patchset in a few minutes.

David

Peter Zijlstra

maintainer

2012-09-10 16:57 UTC #12 lore raw

In reply to David Ahern

Quoted 3 lines · David Ahern

Hopefully thi wraps up the precise mode-exclude_guest dependency. I'm sure someone will let me know if I screwed up the attribution in the second patch.

I'll wait with applying until we have the IBS stuff sorted, other than that, thanks for collecting this..

David Ahern

author

2012-09-10 17:01 UTC #13 lore raw

Quoted 9 lines · Peter Zijlstra

Quoted 3 lines · David Ahern
Hopefully thi wraps up the precise mode-exclude_guest dependency. I'm sure someone will let me know if I screwed up the attribution in the second patch.
I'll wait with applying until we have the IBS stuff sorted, other than that, thanks for collecting this..

Robert recently confirmed that this series should not have an impact on AMD as it currently stands -- exclude attributes are ignored. And then his patch to require they be unset works fine with the first patch in this set because of the existing EINVAL fallback handling in perf-record and perf-top.

Did you have something else in mind that needs to be done?

David

Peter Zijlstra

maintainer

2012-09-10 17:13 UTC #14 lore raw

Quoted 11 lines · David Ahern

Quoted 9 lines · Peter Zijlstra
Quoted 3 lines · David Ahern
Hopefully thi wraps up the precise mode-exclude_guest dependency. I'm sure someone will let me know if I screwed up the attribution in the second patch.
I'll wait with applying until we have the IBS stuff sorted, other than that, thanks for collecting this..

Quoted 7 lines

Robert recently confirmed that this series should not have an impact on AMD as it currently stands -- exclude attributes are ignored. And then his patch to require they be unset works fine with the first patch in this set because of the existing EINVAL fallback handling in perf-record and perf-top.
Did you have something else in mind that needs to be done?

Ah, no, just hadn't integrated all this various data yet. There seems to be a lag between reading stuff and actually knowing it :/

OK, let me go through all this once again.