diff mbox

[RFC,2/2] perf/core: change errno for sampling event not supported in hardware

Message ID 1462786660-2900-3-git-send-email-vgupta@synopsys.com
State New
Headers show

Commit Message

Vineet Gupta May 9, 2016, 9:37 a.m. UTC
This allows userspace to identify this case specifically from the
catch all error msg it prints currently.

This is an ABI change

Before
-------
| # perf record ls
| Error:
| The sys_perf_event_open() syscall returned with 524 (Unknown error 524)
| for event (cycles:ppp).
| /bin/dmesg may provide additional information.
| No CONFIG_PERF_EVENTS=y kernel support configured?

Now
-------
| # perf record ls
| Error:
| PMU Hardware doesn't support sampling/overflow-interrupts.

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 kernel/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Vince Weaver May 9, 2016, 1:54 p.m. UTC | #1
On Mon, 9 May 2016, Vineet Gupta wrote:

> This allows userspace to identify this case specifically from the
> catch all error msg it prints currently.
> 
> This is an ABI change

An ABI change which will probably break things.

The original change from ENODEV to ENOTSUPP managed to break things 
although it took four kernel releases before anyone noticed.

The usage of ENOTSUPP was my fault, though I feel like at the time I was 
told that ENOTSUPP is for internal kernel usage and would be converted to 
EOPNOTSUPP when returning an error to userspace.  But now I 
can't find any sort of reference for that at all, except the fact that

	/usr/include/x86_64-linux-gnu/bits/errno.h

has
	/* Linux has no ENOTSUP error code.  */
	# define ENOTSUP EOPNOTSUPP

in it... but wait, that's ENOTSUP not ENOTSUPP.  Blargh.

Vince
Vineet Gupta May 9, 2016, 5:23 p.m. UTC | #2
On Monday 09 May 2016 07:24 PM, Vince Weaver wrote:
> On Mon, 9 May 2016, Vineet Gupta wrote:
> 
>> This allows userspace to identify this case specifically from the
>> catch all error msg it prints currently.
>>
>> This is an ABI change
> 
> An ABI change which will probably break things.


Right thats what I feared. But hold on, I don't think we need to change the ABI to
achieve what we want. Gosh why did I even take that path.

Currently the errno switch case in perf_evsel__open_strerror() in doesn't handle
ENOTSUPP. So how about we add that - augmented with the same sample_period !0
check to barf for lack of sampling support.

Do you see anything wrong with that ?

-Vineet

> 
> The original change from ENODEV to ENOTSUPP managed to break things 
> although it took four kernel releases before anyone noticed.
> 
> The usage of ENOTSUPP was my fault, though I feel like at the time I was 
> told that ENOTSUPP is for internal kernel usage and would be converted to 
> EOPNOTSUPP when returning an error to userspace.  But now I 
> can't find any sort of reference for that at all, except the fact that
> 
> 	/usr/include/x86_64-linux-gnu/bits/errno.h
> 
> has
> 	/* Linux has no ENOTSUP error code.  */
> 	# define ENOTSUP EOPNOTSUPP
> 
> in it... but wait, that's ENOTSUP not ENOTSUPP.  Blargh.
> 
> Vince
>
Vince Weaver May 11, 2016, 3:33 a.m. UTC | #3
On Mon, 9 May 2016, Vineet Gupta wrote:

> On Monday 09 May 2016 07:24 PM, Vince Weaver wrote:
> > On Mon, 9 May 2016, Vineet Gupta wrote:
> > 
> >> This allows userspace to identify this case specifically from the
> >> catch all error msg it prints currently.
> >>
> >> This is an ABI change
> > 
> > An ABI change which will probably break things.
> 
> 
> Right thats what I feared. But hold on, I don't think we need to change the ABI to
> achieve what we want. Gosh why did I even take that path.
> 
> Currently the errno switch case in perf_evsel__open_strerror() in doesn't handle
> ENOTSUPP. So how about we add that - augmented with the same sample_period !0
> check to barf for lack of sampling support.
> 
> Do you see anything wrong with that ?

no, but it would be nice if one of the actual maintainers would chime in 
with an opinion.

In any case if ENOTSUPP is being returned to userspace I should update the 
perf_event manpage to reflect that.

Vince
Peter Zijlstra May 11, 2016, 7:36 p.m. UTC | #4
On Mon, May 09, 2016 at 10:53:43PM +0530, Vineet Gupta wrote:

> Right thats what I feared. But hold on, I don't think we need to change the ABI to
> achieve what we want. Gosh why did I even take that path.
> 
> Currently the errno switch case in perf_evsel__open_strerror() in doesn't handle
> ENOTSUPP. So how about we add that - augmented with the same sample_period !0
> check to barf for lack of sampling support.
> 
> Do you see anything wrong with that ?

Should work I think.
Vineet Gupta May 12, 2016, 6:28 a.m. UTC | #5
On Thursday 12 May 2016 01:06 AM, Peter Zijlstra wrote:
> On Mon, May 09, 2016 at 10:53:43PM +0530, Vineet Gupta wrote:
> 
>> > Right thats what I feared. But hold on, I don't think we need to change the ABI to
>> > achieve what we want. Gosh why did I even take that path.
>> > 
>> > Currently the errno switch case in perf_evsel__open_strerror() in doesn't handle
>> > ENOTSUPP. So how about we add that - augmented with the same sample_period !0
>> > check to barf for lack of sampling support.
>> > 
>> > Do you see anything wrong with that ?
>
> Should work I think.

Tried that and doesn't even compile. Reconfirms what Vince said, ENOTSUPP is not
exposed to userspace (being in include/linux and not include/uapi/linux)
Peter Zijlstra May 12, 2016, 6:42 a.m. UTC | #6
On Thu, May 12, 2016 at 11:58:43AM +0530, Vineet Gupta wrote:
> On Thursday 12 May 2016 01:06 AM, Peter Zijlstra wrote:
> > On Mon, May 09, 2016 at 10:53:43PM +0530, Vineet Gupta wrote:
> > 
> >> > Right thats what I feared. But hold on, I don't think we need to change the ABI to
> >> > achieve what we want. Gosh why did I even take that path.
> >> > 
> >> > Currently the errno switch case in perf_evsel__open_strerror() in doesn't handle
> >> > ENOTSUPP. So how about we add that - augmented with the same sample_period !0
> >> > check to barf for lack of sampling support.
> >> > 
> >> > Do you see anything wrong with that ?
> >
> > Should work I think.
> 
> Tried that and doesn't even compile. Reconfirms what Vince said, ENOTSUPP is not
> exposed to userspace (being in include/linux and not include/uapi/linux)

Durr, so what does userspace see?
Vineet Gupta May 12, 2016, 6:54 a.m. UTC | #7
On Thursday 12 May 2016 12:12 PM, Peter Zijlstra wrote:
>> Tried that and doesn't even compile. Reconfirms what Vince said, ENOTSUPP is not
>> > exposed to userspace (being in include/linux and not include/uapi/linux)
> Durr, so what does userspace see?

It sees the "value" of ENOTSUPP, i.e. 524 but there is no symbolic reference to it :-)
Peter Zijlstra May 12, 2016, 7:25 a.m. UTC | #8
On Thu, May 12, 2016 at 12:24:25PM +0530, Vineet Gupta wrote:
> On Thursday 12 May 2016 12:12 PM, Peter Zijlstra wrote:
> >> Tried that and doesn't even compile. Reconfirms what Vince said, ENOTSUPP is not
> >> > exposed to userspace (being in include/linux and not include/uapi/linux)
> > Durr, so what does userspace see?
> 
> It sees the "value" of ENOTSUPP, i.e. 524 but there is no symbolic reference to it :-)

Ah.. which might be a hint that nobody is actually explicitly testing
for this and we might just get away with changing the ABI.

Vince, what say you; shall we try and get away with it? ;-)
Vince Weaver May 12, 2016, 11:04 p.m. UTC | #9
On Thu, 12 May 2016, Peter Zijlstra wrote:

> Ah.. which might be a hint that nobody is actually explicitly testing
> for this and we might just get away with changing the ABI.
> 
> Vince, what say you; shall we try and get away with it? ;-)

It's probably worth trying.

The only other time anyone else noticed was in this thread
	https://lkml.org/lkml/2015/6/11/136
and I think the conclusion at the end of that was also that it's probably 
worth changing.

Vince
Vineet Gupta May 13, 2016, 8:36 a.m. UTC | #10
On Friday 13 May 2016 04:34 AM, Vince Weaver wrote:
> On Thu, 12 May 2016, Peter Zijlstra wrote:
> 
>> Ah.. which might be a hint that nobody is actually explicitly testing
>> for this and we might just get away with changing the ABI.
>>
>> Vince, what say you; shall we try and get away with it? ;-)
> 
> It's probably worth trying.
> 
> The only other time anyone else noticed was in this thread
> 	https://lkml.org/lkml/2015/6/11/136
> and I think the conclusion at the end of that was also that it's probably 
> worth changing.

I presume that the RFC patches I posted are fine or do we want any changes.
Vineet Gupta May 30, 2016, 11:31 a.m. UTC | #11
Hi,

This is a repost of RFC [1], rebased on 4.7-rc1, to print pretty PMU lacking
overflow interrupt support if user tries perf sampling.

At the time of RFC review, both PeterZ and Vince seemed convinced to go ahead
with this patch despite the ABI change [2].

Thx,
-Vineet


[1] http://lists.infradead.org/pipermail/linux-snps-arc/2016-May/001014.html
[2] http://lists.infradead.org/pipermail/linux-snps-arc/2016-May/001032.html

Vineet Gupta (2):
  tools/perf: Handle EOPNOTSUPP for sampling events
  perf/core: change errno for sampling event not supported in hardware

 kernel/events/core.c    | 2 +-
 tools/perf/util/evsel.c | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)
diff mbox

Patch

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 4e2ebf6f2f1f..41c5c7122987 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8435,7 +8435,7 @@  SYSCALL_DEFINE5(perf_event_open,
 
 	if (is_sampling_event(event)) {
 		if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
-			err = -ENOTSUPP;
+			err = -EOPNOTSUPP;
 			goto err_alloc;
 		}
 	}