| Message ID | 1462786660-2900-3-git-send-email-vgupta@synopsys.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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.
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)
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?
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 :-)
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? ;-)
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
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.
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 --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; } }
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(-)