Messages in this thread | | | Date | Fri, 31 Oct 2014 10:27:13 +0100 | From | Ingo Molnar <> | Subject | Re: [RFD] perf syscall error handling |
| |
* Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Oct 30, 2014 at 09:16:36PM -0400, Vince Weaver wrote: > > On Thu, 30 Oct 2014, Peter Zijlstra wrote: > > > > > So would something simple, like an offset into the struct > > > perf_event_attr pointing at the current field we're trying > > > to process make sense? Maybe with negative offsets to > > > indicate the syscall arguments? > > > > > > That would narrow down the 'WTF is wrong noaw' a lot I > > > think. But then, I've not actually done a lot of userspace > > > the last few years, so maybe I'm just dreaming things. > > > > well, as someone who spends a lot of time in userspace trying > > to help people who report probems like 'perf_event_open() > > returns EINVAL, what's wrong' I can say pretty much anything > > will be an improvement. > > Right, the situation is dire indeed :/ > > > What would really help is if we could somehow return the > > filename/line-number of whatever source code file that's > > setting errno. > > > > Even if perf_event_open() told me that hey, we're getting > > EOPNOTSUPP due to the precise_ip parameter (something that > > happened just yesterday) it's still a lot of grepping and > > poking around source files to find out what's going on. It > > would be much better if it just told me the issue was at > > kernel/events/core.c line 995 or so, but I'm not sure how you > > could pass that back to the user, and one could argue it > > wouldn't help much the average user without a kernel tree > > lying around. > > Would an additional bit mask help? With that we'd be able to > finger the exact flag that causes pain.
Well, I don't really like bitmasks nor __LINE__/__FILE__ obscurity, those are non-starters because they are user unfriendly.
What would work best is something like:
- user-space could request 'extended error code' passing from kernel to user-space via a (default off) feature bit in perf_attr, plus a user-space provided pointer to a string buffer, and a maximum length value.
- old user-space or user-space that does not want it would not be unaffected by the new 'extended error code' facility
- user-space gets back the regular errno (-EOPNOTSUPP or -ENOSYS or -EINVAL, etc.) and a string. Strings are really the most helpful information, because tools can just print that. They can also match on specific strings and programmatically react to them if they want to: we can promise to not arbitrarily change error strings once they are introduced. (but even if they change, user-space can still print them out.)
- in the kernel, instead of doing:
return -EOPNOTSUPP;
we'd do something like:
return perf_errno(-EOPNOTSUPP, "BTS not supported by this CPU architecture");
here the kernel would in the regular case just ignore the string, but if user-space requested the 'extended errno' facility, it would copy the (zero-delimited) error string to perf_attr->errno_str, taking errno_len into account.
If no extended string was written then user-space can detect this through the string not having been written to. (it can initialize it to a zero string.)
Note the various advantages of this approach:
- it's hard to get the facility wrong on the user-space side: in the simplest usage user-space simply prints out the error, which will be obvious to the user in most cases.
- it's hard to get it wrong on the kernel side: it's really similar to what we do today, plus a descriptive error string. Developers should take care to use descriptive and unique messages (but even duplicate messages will help in informing the user).
- it's infinitely extensible, does not involve magic numbers nor ever changing __LINE__ numbers and obscure source code file names.
- it's really _easy_ to add good error information on the kernel side: just add a perf_err() string passing method instead of a dumb return -EINVAL. Also, the information is in a single place, exactly where the problem occurs, so it will be easily maintainable going forward.
Thanks,
Ingo
| |