Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Wed, Aug 26, 2015 at 07:56:47PM +0300, Alexander Shishkin escreveu:
> Ingo Molnar <mingo@kernel.org> writes:
> 
> > * Ingo Molnar <mingo@kernel.org> wrote:
> >
> >> ... but back then I didn't feel like complicating an error recovery ABI for the 
> >> needs of the 1%, robust error handling is all about simplicity: if it's not 
> >> simple, tools won't use it.
> >
> > And note that it needs to be 'simple' in two places for usage to grow naturally: 
> >
> >   - the usage site in the kernel
> >   - the tooling side that recovers the information.
> >
> > That's why I think that such a form:
> >
> > 	return err_str(-EINVAL, "x86/perf: CPU does not support precise sampling");
> >
> > is obviously simple on the kernel side as it returns -EINVAL, and is very simple 
> > on the tooling side as well, if we are allowed to extend prctl().
> 
> So I hacked stuff a bit [1] to accomodate some of the above
> ideas. The below diff shows how these ideas integrate with perf. The
> rest is in my github tree.
> 
>  - this exterr implementation allows its users to add arbitrary
>  information to the  call site structures and also pretty print them on
>  the way out; in the example, perf stores perf_event_attr field name
>  that is the source of trouble; it's a string rather than offsetof(),
>  because half of our event attribute is a bit field;
>  - the "way out" doesn't have to be syscall return path (although in the
>  example it is);
>  - userspace can fetch the extended error reports via prctl() like you
>  suggested above;
>  - error codes are still passed around in the [-EXT_ERRNO..-MAX_ERRNO]
>  range until they are passed to userspace (which is where ext_err_code()
>  converts them back to traditional errno.h values).

Hey, can we see the builtin-record.c patch please?

- Arnaldo
 
> # perf record -e branches -c1 ls
> kernel says (0/95): {
>         "file": "/home/ash/work/linux/arch/x86/kernel/cpu/perf_event.c",
>         "line": 432,
>         "code": -95,
>         "module": "perf/x86",
>         "message": "BTS sampling not allowed for kernel space"
> ,       "attr_field": "exclude_kernel"
> }
> 
> Error:
> No hardware sampling interrupt available.
> No APIC? If so then you can boot the kernel with the "lapic" boot parameter to force-enable it.
> # 
> 
> [1] https://github.com/virtuoso/linux-perf/commits/exterr
> 
> >From 1338fe417223cc1d8bc7588d95c6a913993d966f Mon Sep 17 00:00:00 2001
> From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Date: Wed, 26 Aug 2015 18:36:14 +0300
> Subject: [PATCH] perf: Use extended syscall error reporting somewhat
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/perf_event.c |  5 ++++-
>  include/linux/perf_event.h       | 14 ++++++++++++++
>  kernel/events/core.c             | 17 ++++++++++++++++-
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index f56cf074d0..5e8f2edb2c 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -12,6 +12,8 @@
>   *  For licencing details see kernel-base/COPYING
>   */
>  
> +#define EXTERR_MODNAME	"perf/x86"
> +
>  #include <linux/perf_event.h>
>  #include <linux/capability.h>
>  #include <linux/notifier.h>
> @@ -426,7 +428,8 @@ int x86_setup_perfctr(struct perf_event *event)
>  
>  		/* BTS is currently only allowed for user-mode. */
>  		if (!attr->exclude_kernel)
> -			return -EOPNOTSUPP;
> +			return perf_err(-EOPNOTSUPP, exclude_kernel,
> +					"BTS sampling not allowed for kernel space");
>  
>  		/* disallow bts if conflicting events are present */
>  		if (x86_add_exclusive(x86_lbr_exclusive_lbr))
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 2027809433..eb63074012 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -16,6 +16,20 @@
>  
>  #include <uapi/linux/perf_event.h>
>  
> +#include <linux/exterr.h>
> +
> +struct perf_ext_err_site {
> +	struct ext_err_site	site;
> +	const char		*attr_field;
> +};
> +
> +#define perf_err(__c, __a, __m)						\
> +	({ /* make sure it's a real field before stringifying it */	\
> +		struct perf_event_attr __x; (void)__x.__a;		\
> +		ext_err(perf, __c, __m,					\
> +			.attr_field = __stringify(__a));		\
> +	})
> +
>  /*
>   * Kernel-internal data types and definitions:
>   */
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index ae16867670..5523c623c4 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -9,6 +9,8 @@
>   * For licensing details see kernel-base/COPYING
>   */
>  
> +#define EXTERR_MODNAME	"perf"
> +
>  #include <linux/fs.h>
>  #include <linux/mm.h>
>  #include <linux/cpu.h>
> @@ -44,11 +46,24 @@
>  #include <linux/compat.h>
>  #include <linux/bpf.h>
>  #include <linux/filter.h>
> +#include <linux/exterr.h>
>  
>  #include "internal.h"
>  
>  #include <asm/irq_regs.h>
>  
> +static char *perf_exterr_format(void *site)
> +{
> +	struct perf_ext_err_site *psite = site;
> +	char *output;
> +
> +	output = kasprintf(GFP_KERNEL, ",\t\"attr_field\": \"%s\"\n",
> +			   psite->attr_field);
> +	return output;
> +}
> +
> +DECLARE_EXTERR_DOMAIN(perf, perf_exterr_format);
> +
>  static struct workqueue_struct *perf_wq;
>  
>  typedef int (*remote_function_f)(void *);
> @@ -8352,7 +8367,7 @@ err_group_fd:
>  	fdput(group);
>  err_fd:
>  	put_unused_fd(event_fd);
> -	return err;
> +	return ext_err_errno(err);
>  }
>  
>  /**
> -- 
> 2.5.0