Re: [PATCH RFC v1 0/4] perf: Introduce extended syscall error reporting

[Ingo Molnar <mingo@kernel.org>]

* Alexander Shishkin <alexander.shishkin@linux.intel.com> wrote:

> Ingo Molnar <mingo@kernel.org> writes:
> 
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> >
> >> On Fri, Jul 24, 2015 at 02:45:55PM +0300, Alexander Shishkin wrote:
> >> > Hi Peter and Ingo and everybody,
> >> > 
> >> > Here's my second stab at improving perf's error reporting by attaching
> >> > arbitrary strings to the integer error codes. This is largely based
> >> > off of the previous email thread [1].
> >> > 
> >> > This time around, I employed a linker trick to convert the structures
> >> > containing extended error information into integers, which are then
> >> > made to look just like normal error codes so that IS_ERR_VALUE() and
> >> > friends would still work correctly on them. So no extra pointers in
> >> > the struct perf_event or anywhere else; the extended error codes are
> >> > passed around like normal error codes. They only need to be converted
> >> > in syscalls' topmost return statements. This is done in 1/4.
> >> > 
> >> > Then, 2/4 illustrates how error sites can be extended to include more
> >> > information such as file names and line numbers [1].
> >> > 
> >> > The other two patches add perf_err() annotation to a few semi-randomly
> >> > picked places in perf core (3/4) and x86 bits (4/4).
> >> 
> >> Looks generally ok to me. Thanks for doing this.
> >
> > I like this too.
> >
> > Alexander, mind sending a finalized, signed off version?
> 
> Sure, I have everything ready, except for what about 2/4 (using 
> CONFIG_DEBUG_KERNEL to extend output with file name and line number)? Should I 
> leave it out or can we pick a more specific kconfig option or add a new one?

I'd make it unconditional. We'll see whether we compress the debug info some more 
if the number of callsites increases dramatically. Tooling can decide whether it 
wants to display it by default, or print it only if some verbosity option is 
activated.

Also, mind structuring it in a bit more generic way that makes it possible for 
other kernel subsystems to use this facility too? I.e. add a new header for it 
instead of embedding it in perf, etc. Nothing complex, just some common-sense 
generalization for a useful looking facility.

For example the scheduler could start using it in struct sched_attr and feed back 
the 15+ of failure causes in sys_sched_setattr() / __sched_setscheduler() in a bit 
more usable fashion.

Thanks,

	Ingo