perf: precise mode and exclude_guest

Hopefully this 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 (2):
perf tool: precise mode requires exclude_guest
perf tool: give user better message if precise is not supported

Peter Zijlstra (1):
perf: require exclude_guest to use PEBS - kernel side enforcement

v3: removed extra tab in patch 2
v2: updated commit messages for patches 1 and 2

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

1.7.10.1

Summary of events per Peter:

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

This patch handles userspace perf command, setting the exclude_guest attribute if precise mode is requested, but only if a user has not specified a request for guest or host only profiling (G or H attribute).

Kernel side AMD currently ignores all exclude_* bits, so there is no impact to existing IBS code paths. Robert Richter has a patch where IBS code will return EINVAL if an exclude_* bit is set. When this goes in it means use of :p on AMD with IBS will first fail with EINVAL (because exclude_guest will be set). Then the existing fallback code within perf will unset exclude_guest and try again. The second attempt will succeed if the CPU supports IBS profiling.

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

index 44afcf4..696cc7e 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;
 

1.7.10.1

Commit-ID:  1342798cc13e3b48d9b5738f0c8fa812ccea8101
Gitweb:     http://git.kernel.org/tip/1342798cc13e3b48d9b5738f0c8fa812ccea8101
Author:     David Ahern <[email protected]>
AuthorDate: Thu, 13 Sep 2012 14:59:13 -0600
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 16 Oct 2012 12:43:31 -0300

perf tool: Precise mode requires exclude_guest

Summary of events per Peter:

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

This patch handles userspace perf command, setting the exclude_guest attribute if precise mode is requested, but only if a user has not specified a request for guest or host only profiling (G or H attribute).

Kernel side AMD currently ignores all exclude_* bits, so there is no impact to existing IBS code paths. Robert Richter has a patch where IBS code will return EINVAL if an exclude_* bit is set. When this goes in it means use of :p on AMD with IBS will first fail with EINVAL (because exclude_guest will be set). Then the existing fallback code within perf will unset exclude_guest and try again. The second attempt will succeed if the CPU supports IBS profiling.

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

index aed38e4..75c7b0f 100644
@@ -690,6 +690,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;
 

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 may not be supported. Try removing 'p' modifier
  sleep: Terminated

v2: softened message to 'may not be' supported per Robert's suggestion

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

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

index 5550754..1d1c98c 100644
@@ -976,6 +976,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 == EOPNOTSUPP) && (attr->precise_ip)) {
+				ui__error("\'precise\' request may not be supported. "
+					  "Try removing 'p' modifier\n");
+				goto out_err;
 			}
 
 			ui__error("The sys_perf_event_open() syscall "

1.7.10.1

Well, either it's supported on this machine or it's not - why does the text have to be so unsure about it?

We use computers to increase determinism, not to insert extra uncertainty! ;-)

Thanks,

Ingo

Because EOPNOTSUPP is returned for more than just precise mode. We cannot say with certainty that the precise attribute caused that errno.

David

Well, then that is useful information we *lost*, and that situation needs to be improved on the ABI side: an expanded error code present in the event structure, copied back to user-space on errors, or so.

(Alternatively, a special event channel just to pass back expanded error conditions.)

Computers are supposed to make life easier for humans, by answering such "what did go wrong?" questions. Our losing of precise error conditions is a usability bug really - and in the perf project we are in a unique position to be able to improve both the kernel side code and make immediate use of it on the tooling side as well.

Thanks,

Ingo

Understood and there have been suggestions on how to definitely state what the kernel side did not like. I like Peter's last suggestion -- something along the lines of clearing attr on a failure except the offending setting.

David

Em Fri, Sep 14, 2012 at 05:43:42AM -0600, David Ahern escreveu:

I think ws need to use a new bit, attr.clear_opsup_on_error, that we would set, older kernels would return an error cause they don't support this new feature, the tool would then clear it and work as today, giving a vague message, cause that is all it can do.

We can't just clean the unsupported bits because then older tools would get completely confused when trying to use the current fallback mechanism, where, for instance, for sample_id_all, we just reset it in the tooling side, ask again the kernel and it works.

Older tools not setting the clear_supported_bits_on_error would get the current behaviour.

- Arnaldo

Quite so, for all the reasons you list. But you like the general idea? I wasn't sure I did, but it was the only thing I could come up with that would sort of do what we need it to.

The fact that you destroy the user input is awkward, I don't think there's another syscall that behaves in this fashion.

Destroying/clearing stuff looks really hacky.

Why not use a single error status field, set via a long list of enum error constants, a 'perf errnos'?

The only real problem with the kernel's syscall error code is that it's not wide enough for historic reasons, so we cannot just create our own errnos. But we can create our errors in the attr just fine and make them finegrained enough so that tooling can figure out what happened exactly when it gets a syscall error.

Yes, that's old-fashioned technology, but it works. With time we could put some structure into the list of error IDs, to make it easily extensible yet grouped in some fashion, etc.

Thanks,

Ingo

And we could start this small: just covering the problem at hand or so, with proper tooling side support - and then extend it as needed. I.e. have something in place that we could ask people adding new kernel side error paths to use - and with time old code could be enriched with better error codes as well.

perf could in fact internally standardize on a 32-bit wide error code, and translate it to -EINVAL at the syscall return level. That way we could keep all the simple error paths as they are today, with a very little syscall level logic (triggered in error cases only, so no overhead in the regular case).

I.e.:

return -EPERF_CPU_PRECISE_EV_NOTSUPP;

would result in that error code being available to new, improved tooling that knows about the new attr field. Old tooling would still work as well, it would only see the -EINVAL of the syscall.

Or something like this - simple and using existing patterns and practices. Anything else will likely fail due to non-use.

Thanks,

Ingo

I just don't like having to enumerate all possible fails, I'm too lazy. Can't we be smarter about that? Could we do a {reason}x{bit-offset} like thing?

Where we limit reason to a few simple things like:

invalid
out-of-range
not-supported

and have the bit-offset indicate the field we're having the particular problem with?

Then all we need is a smart way to generate and map the bit-offsets without too much manual labour.

Putting the 'where' into a separate field would do that, and thus we could generate and report such structured errors as well
- but nevertheless there will always be special/individual errors as well that won't fit into such a scheme, for which we should include a 'boring' errno range as well ...

I.e. a {where},{what} s32 pair of fields - if 'where' is zero then 'what' is the enumerated errno value I suggested, if it's nonzero then it's the 'where' indication you suggested.

Thanks,

Ingo

Commit-ID:  2305c82fb35dd2c8c9533303bb1693f1636c66e4
Gitweb:     http://git.kernel.org/tip/2305c82fb35dd2c8c9533303bb1693f1636c66e4
Author:     David Ahern <[email protected]>
AuthorDate: Thu, 13 Sep 2012 14:59:15 -0600
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 24 Oct 2012 14:20:11 -0200

perf tools: Give user better message if precise is not supported

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 may not be supported. Try removing 'p' modifier
  sleep: Terminated

v2: softened message to 'may not be' supported per Robert's suggestion

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

index 73b5d7f..53c9892 100644
@@ -317,6 +317,11 @@ try_again:
 					  perf_evsel__name(pos));
 				rc = -err;
 				goto out;
+			} else if ((err == EOPNOTSUPP) && (attr->precise_ip)) {
+				ui__error("\'precise\' request may not be supported. "
+					  "Try removing 'p' modifier\n");
+				rc = -err;
+				goto out;
 			}
 
 			printf("\n");

index fb9da71..f2ecd49 100644
@@ -977,6 +977,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 == EOPNOTSUPP) && (attr->precise_ip)) {
+				ui__error("\'precise\' request may not be supported. "
+					  "Try removing 'p' modifier\n");
+				goto out_err;
 			}
 
 			ui__error("The sys_perf_event_open() syscall "

From: Peter Zijlstra <[email protected]>

Per Peter:
"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.

This patch enforces exclude_guest kernel side.

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

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

1.7.10.1

Commit-ID:  20b279ddb38ca42f8863cec07b4d45ec24589f13
Gitweb:     http://git.kernel.org/tip/20b279ddb38ca42f8863cec07b4d45ec24589f13
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Thu, 13 Sep 2012 14:59:14 -0600
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 16 Oct 2012 12:43:58 -0300

perf: Require exclude_guest to use PEBS - kernel side enforcement

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.

This patch enforces exclude_guest kernel side.

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

index 915b876..3373f84 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++;

Ping? Is this version Peter approved?

David

Any chance this get committed? At the least userspace code? It has been 3 months since I first reported the problem and what appears to be an acceptable solution has been lingering for a month.

David