[PATCH 0/3] perf: precise mode and exclude_guest

[PATCH 0/3] perf: precise mode and exclude_guest

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

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

-- 
1.7.10.1

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

[David Ahern]

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 <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Robert Richter <robert.richter@amd.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Avi Kivity <avi@redhat.com>
---
 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 a031ee1..f2c02b2 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -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

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

[David Ahern]

From: Peter Zijlstra <peterz@infradead.org>

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

Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Robert Richter <robert.richter@amd.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Avi Kivity <avi@redhat.com>
---
 arch/x86/kernel/cpu/perf_event.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 915b876..4bc96c5 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -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

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

[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 <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Robert Richter <robert.richter@amd.com>
---
 tools/perf/builtin-record.c |    5 +++++
 tools/perf/builtin-top.c    |    4 ++++
 2 files changed, 9 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 7b8b891..ccf529b 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -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");
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 0513aaa..0d3653b 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -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 "
-- 
1.7.10.1

Re: [PATCH 0/3] perf: precise mode and exclude_guest

[Peter Zijlstra]

On Mon, 2012-09-10 at 10:40 -0600, David Ahern wrote:
> 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..

Re: [PATCH 0/3] perf: precise mode and exclude_guest

[David Ahern]

On 9/10/12 10:57 AM, Peter Zijlstra wrote:
> On Mon, 2012-09-10 at 10:40 -0600, David Ahern wrote:
>> 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

Re: [PATCH 0/3] perf: precise mode and exclude_guest

[Peter Zijlstra]

On Mon, 2012-09-10 at 11:01 -0600, David Ahern wrote:
> On 9/10/12 10:57 AM, Peter Zijlstra wrote:
> > On Mon, 2012-09-10 at 10:40 -0600, David Ahern wrote:
> >> 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?

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.

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

[Peter Zijlstra]

On Mon, 2012-09-10 at 10:40 -0600, David Ahern wrote:
> 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 ?

> If user adds :p modifier set exclude_guest as well.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Robert Richter <robert.richter@amd.com>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Avi Kivity <avi@redhat.com>
> ---
>  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 a031ee1..f2c02b2 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -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;
>  

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

[Peter Zijlstra]

On Mon, 2012-09-10 at 10:40 -0600, David Ahern wrote:
> From: Peter Zijlstra <peterz@infradead.org>
> 
> 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.

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

[Robert Richter]

On 10.09.12 10:40:16, David Ahern wrote:
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -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?

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

> +				rc = -err;
> +				goto out;
>  			}
>  
>  			printf("\n");
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 0513aaa..0d3653b 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -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.

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

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

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

[David Ahern]

On 9/11/12 3:20 AM, Robert Richter wrote:
> On 10.09.12 10:40:16, David Ahern wrote:
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -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.

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

>
>> +				rc = -err;
>> +				goto out;
>>   			}
>>
>>   			printf("\n");
>> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
>> index 0513aaa..0d3653b 100644
>> --- a/tools/perf/builtin-top.c
>> +++ b/tools/perf/builtin-top.c
>> @@ -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.
>
>> +				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

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

[Robert Richter]

On 11.09.12 07:22:32, David Ahern wrote:
> On 9/11/12 3:20 AM, Robert Richter wrote:
> > On 10.09.12 10:40:16, David Ahern wrote:
> >> --- a/tools/perf/builtin-record.c
> >> +++ b/tools/perf/builtin-record.c
> >> @@ -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?

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

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

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

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

[David Ahern]

On 9/11/12 8:01 AM, Robert Richter wrote:
>
> 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.

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


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

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

[Robert Richter]

On 11.09.12 08:32:55, David Ahern wrote:
> 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?

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

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


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

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

[David Ahern]

On 9/11/12 9:11 AM, Robert Richter wrote:
> On 11.09.12 08:32:55, David Ahern wrote:
>> 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.

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