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

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

[Alexander Shishkin]

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).

[1] http://marc.info/?l=linux-kernel&m=141470811013082
[2] http://marc.info/?l=linux-kernel&m=141471816115946

Alexander Shishkin (4):
  perf: Introduce extended syscall error reporting
  perf: Add file name and line number to perf extended error reports
  perf: Annotate some of the error codes with perf_err()
  perf/x86: Annotate some of the error codes with perf_err()

 arch/x86/kernel/cpu/perf_event.c           |   8 ++-
 arch/x86/kernel/cpu/perf_event_intel_lbr.c |   2 +-
 include/linux/perf_event.h                 |  87 +++++++++++++++++++++++
 include/uapi/linux/perf_event.h            |   8 ++-
 kernel/events/core.c                       | 108 +++++++++++++++++++++++++----
 5 files changed, 195 insertions(+), 18 deletions(-)

-- 
2.1.4

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

[Alexander Shishkin]

It has been pointed several times out that perf syscall error reporting
leaves a lot to be desired [1].

This patch introduces a fairly simple extension that allows call sites
to annotate their error codes with arbitrary strings, which will then
be copied to userspace (if they asked for it) along with the module
name that produced the error message in JSON format. This way, we can
provide both human-readable and machine-parsable information to user and
leave room for extensions in the future, such as file name and line
number if kernel debugging is enabled.

Each error "site" is referred to by its index, which is folded into an
integer error value within the range of [-PERF_ERRNO, -MAX_ERRNO], where
PERF_ERRNO is chosen to be below any known error codes, but still leaving
enough room to enumerate error sites. This way, all the traditional macros
will still handle these as error codes and we'd only have to convert them
to their original values right before returning to userspace. This way we
also don't have to worry about keeping a separate pointer to the error
message inside a perf_event.

[1] http://marc.info/?l=linux-kernel&m=141470811013082

NYet-Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 include/linux/perf_event.h      | 76 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/perf_event.h |  8 +++-
 kernel/events/core.c            | 81 ++++++++++++++++++++++++++++++++++++++---
 3 files changed, 159 insertions(+), 6 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 2027809433..9e9af962f6 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -56,6 +56,82 @@ struct perf_guest_info_callbacks {
 #include <linux/cgroup.h>
 #include <asm/local.h>
 
+#ifndef PERF_MODNAME
+#define PERF_MODNAME "perf"
+#endif
+
+/*
+ * Extended error reporting: annotate an error code with a string
+ * and a module name to help users diagnase problems with their
+ * attributes and whatnot.
+ */
+struct perf_err_site {
+	const char		*message;
+	const char		*owner;
+	const int		code;
+};
+
+#ifdef CONFIG_PERF_EVENTS
+
+/*
+ * Place all occurrences of struct perf_err_site into a special section,
+ * so that we can find out their offsets, which we'll use to refer back
+ * to the error sites.
+ */
+extern const struct perf_err_site __start___perf_err[], __stop___perf_err[];
+
+#define __perf_err(__e, __c, __m) ({				\
+	static struct perf_err_site				\
+	__attribute__ ((unused,__section__("__perf_err")))	\
+	__err_site = {						\
+		.message	= (__m),			\
+		.owner		= PERF_MODNAME,			\
+		.code		= __builtin_constant_p((__c)) ?	\
+		(__c) : 0,					\
+	};							\
+	(__e) = &__err_site;					\
+})
+
+/*
+ * Use part of the [-1, -MAX_ERRNO] errno range for perf's extended error
+ * reporting. Anything within [-PERF_ERRNO, -MAX_ERRNO] is an index of a
+ * perf_err_site structure within __perf_err section. 3.5k should be enough
+ * for everybody, but let's add a boot-time warning just in case it overflows
+ * one day.
+ */
+#define PERF_ERRNO 512
+
+static inline int perf_errno(const struct perf_err_site *site)
+{
+	unsigned long err = site - __start___perf_err;
+
+	trace_printk("[%ld] %s:%d, %d\n", err, site->file, site->line, site->code);
+	return -(int)err - PERF_ERRNO;
+}
+
+static inline const struct perf_err_site *perf_errno_to_site(int err)
+{
+	return __start___perf_err - err - PERF_ERRNO;
+}
+
+#ifdef MODULE
+/*
+ * Module support is a tad trickier, but far from rocket surgery. Let's
+ * bypass it for now.
+ */
+#define perf_err(__c, __m) (__c)
+#else
+#define perf_err(__c, __m) ({					\
+	struct perf_err_site *s;				\
+	__perf_err(s, (__c), (__m));				\
+	perf_errno(s);						\
+})
+#endif
+
+#define PERF_ERR_PTR(__e, __m)	(ERR_PTR(perf_err(__e, __m)))
+
+#endif
+
 struct perf_callchain_entry {
 	__u64				nr;
 	__u64				ip[PERF_MAX_STACK_DEPTH];
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index d97f84c080..d1ae1a079c 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -264,6 +264,7 @@ enum perf_event_read_format {
 					/* add: sample_stack_user */
 #define PERF_ATTR_SIZE_VER4	104	/* add: sample_regs_intr */
 #define PERF_ATTR_SIZE_VER5	112	/* add: aux_watermark */
+#define PERF_ATTR_SIZE_VER6	120	/* add: perf_err */
 
 /*
  * Hardware event_id to monitor via a performance monitoring event:
@@ -374,7 +375,12 @@ struct perf_event_attr {
 	 * Wakeup watermark for AUX area
 	 */
 	__u32	aux_watermark;
-	__u32	__reserved_2;	/* align to __u64 */
+
+	/*
+	 * Extended error reporting buffer
+	 */
+	__u32	perf_err_size;
+	__u64	perf_err;
 };
 
 #define perf_flags(attr)	(*(&(attr)->read_format + 1))
diff --git a/kernel/events/core.c b/kernel/events/core.c
index d3dae3419b..85bcf3a5f9 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -49,6 +49,74 @@
 
 #include <asm/irq_regs.h>
 
+static bool extended_reporting_enabled(struct perf_event_attr *attr)
+{
+	if (attr->size >= PERF_ATTR_SIZE_VER6 &&
+	    attr->perf_err_size > 0)
+		return true;
+
+	return false;
+}
+
+/*
+ * Provide a JSON formatted error report to the user if they asked for it.
+ */
+static void perf_error_report_site(struct perf_event_attr *attr,
+				   const struct perf_err_site *site)
+{
+	void *buffer;
+
+	if (!site || !extended_reporting_enabled(attr))
+		return;
+
+	/* in case of nested perf_err()s, which you shouldn't really do */
+	while (site->code <= -PERF_ERRNO)
+		site = perf_errno_to_site(site->code);
+
+	buffer = kasprintf(GFP_KERNEL,
+			   "{\n"
+			   "\t\"code\": %d,\n"
+			   "\t\"module\": \"%s\",\n"
+			   "\t\"message\": \"%s\"\n"
+			   "}\n",
+			   site->code, site->owner, site->message
+			   );
+	if (!buffer)
+		return;
+
+	if (copy_to_user((void __user *)attr->perf_err, buffer,
+			 attr->perf_err_size)) {
+		/* if we failed to copy once, don't bother later */
+		attr->perf_err_size = 0;
+	}
+
+	kfree(buffer);
+}
+
+/*
+ * Synchronous version of perf_err(), for the paths where we return immediately
+ * back to userspace.
+ */
+#define perf_err_sync(__attr, __c, __m) ({		\
+	struct perf_err_site *__site;			\
+	__perf_err(__site, (__c), (__m));		\
+	perf_error_report_site(__attr, __site);		\
+	(__c);						\
+})
+
+static int perf_error_report(struct perf_event_attr *attr, int err)
+{
+	const struct perf_err_site *site;
+
+	if (err > -PERF_ERRNO)
+		return err;
+
+	site = perf_errno_to_site(err);
+	perf_error_report_site(attr, site);
+
+	return site->code;
+}
+
 static struct workqueue_struct *perf_wq;
 
 typedef int (*remote_function_f)(void *);
@@ -3890,7 +3958,7 @@ perf_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	ret = perf_read_hw(event, buf, count);
 	perf_event_ctx_unlock(event, ctx);
 
-	return ret;
+	return perf_error_report(&event->attr, ret);
 }
 
 static unsigned int perf_poll(struct file *file, poll_table *wait)
@@ -4103,7 +4171,7 @@ static long perf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	ret = _perf_ioctl(event, cmd, arg);
 	perf_event_ctx_unlock(event, ctx);
 
-	return ret;
+	return perf_error_report(&event->attr, ret);
 }
 
 #ifdef CONFIG_COMPAT
@@ -4703,7 +4771,7 @@ aux_unlock:
 	if (event->pmu->event_mapped)
 		event->pmu->event_mapped(event);
 
-	return ret;
+	return perf_error_report(&event->attr, ret);
 }
 
 static int perf_fasync(int fd, struct file *filp, int on)
@@ -4717,7 +4785,7 @@ static int perf_fasync(int fd, struct file *filp, int on)
 	mutex_unlock(&inode->i_mutex);
 
 	if (retval < 0)
-		return retval;
+		return perf_error_report(&event->attr, retval);
 
 	return 0;
 }
@@ -8208,7 +8276,7 @@ err_group_fd:
 	fdput(group);
 err_fd:
 	put_unused_fd(event_fd);
-	return err;
+	return perf_error_report(&attr, err);
 }
 
 /**
@@ -8986,6 +9054,9 @@ void __init perf_event_init(void)
 	 */
 	BUILD_BUG_ON((offsetof(struct perf_event_mmap_page, data_head))
 		     != 1024);
+
+	/* Too many error sites; see the comment at PERF_ERRNO definition */
+	WARN_ON(__stop___perf_err - __start___perf_err > MAX_ERRNO - PERF_ERRNO);
 }
 
 ssize_t perf_event_sysfs_show(struct device *dev, struct device_attribute *attr,
-- 
2.1.4

[PATCH RFC v1 2/4] perf: Add file name and line number to perf extended error reports

[Alexander Shishkin]

If kernel debugging is enabled, include additional information to extended
syscall error reports: file name and line number, to make error hunting even
easier.

And in really desperate times, this allows for such things (a variation on
Hugh Dickins' trick [1]):

	#undef EINVAL
	#define EINVAL perf_err(-22, "")

However, CONFIG_DEBUG_KERNEL might not be the right option for this as I
notice that, for example, debian enables it in its kernels.

[1] http://marc.info/?l=linux-kernel&m=141215166009542

NYet-Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 include/linux/perf_event.h | 11 +++++++++++
 kernel/events/core.c       |  7 +++++++
 2 files changed, 18 insertions(+)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 9e9af962f6..d84a2914f5 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -68,11 +68,21 @@ struct perf_guest_info_callbacks {
 struct perf_err_site {
 	const char		*message;
 	const char		*owner;
+#ifdef CONFIG_DEBUG_KERNEL
+	const char		*file;
+	const int		line;
+#endif
 	const int		code;
 };
 
 #ifdef CONFIG_PERF_EVENTS
 
+#ifdef CONFIG_DEBUG_KERNEL
+#define DEBUG_PERF_ERR	.file = __FILE__, .line = __LINE__,
+#else
+#define DEBUG_PERF_ERR
+#endif
+
 /*
  * Place all occurrences of struct perf_err_site into a special section,
  * so that we can find out their offsets, which we'll use to refer back
@@ -88,6 +98,7 @@ extern const struct perf_err_site __start___perf_err[], __stop___perf_err[];
 		.owner		= PERF_MODNAME,			\
 		.code		= __builtin_constant_p((__c)) ?	\
 		(__c) : 0,					\
+		DEBUG_PERF_ERR					\
 	};							\
 	(__e) = &__err_site;					\
 })
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 85bcf3a5f9..4ff09fdc67 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -75,10 +75,17 @@ static void perf_error_report_site(struct perf_event_attr *attr,
 
 	buffer = kasprintf(GFP_KERNEL,
 			   "{\n"
+#ifdef CONFIG_DEBUG_KERNEL
+			   "\t\"file\": \"%s\",\n"
+			   "\t\"line\": %d,\n"
+#endif
 			   "\t\"code\": %d,\n"
 			   "\t\"module\": \"%s\",\n"
 			   "\t\"message\": \"%s\"\n"
 			   "}\n",
+#ifdef CONFIG_DEBUG_KERNEL
+			   site->file, site->line,
+#endif
 			   site->code, site->owner, site->message
 			   );
 	if (!buffer)
-- 
2.1.4

[PATCH RFC v1 3/4] perf: Annotate some of the error codes with perf_err()

[Alexander Shishkin]

This patch annotates a few semi-random error paths in perf core to
illustrate the extended error reporting facility. Most of them can
be triggered from perf tools.

NYet-Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 kernel/events/core.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 4ff09fdc67..e1d9fe6f23 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3367,10 +3367,10 @@ find_lively_task_by_vpid(pid_t vpid)
 	rcu_read_unlock();
 
 	if (!task)
-		return ERR_PTR(-ESRCH);
+		return PERF_ERR_PTR(-ESRCH, "task not found");
 
 	/* Reuse ptrace permission checks for now. */
-	err = -EACCES;
+	err = perf_err(-EACCES, "insufficient permissions for tracing this task");
 	if (!ptrace_may_access(task, PTRACE_MODE_READ))
 		goto errout;
 
@@ -3398,7 +3398,8 @@ find_get_context(struct pmu *pmu, struct task_struct *task,
 	if (!task) {
 		/* Must be root to operate on a CPU event: */
 		if (perf_paranoid_cpu() && !capable(CAP_SYS_ADMIN))
-			return ERR_PTR(-EACCES);
+			return PERF_ERR_PTR(-EACCES,
+					    "must be root to operate on a CPU event");
 
 		/*
 		 * We could be clever and allow to attach a event to an
@@ -3406,7 +3407,7 @@ find_get_context(struct pmu *pmu, struct task_struct *task,
 		 * that's for later.
 		 */
 		if (!cpu_online(cpu))
-			return ERR_PTR(-ENODEV);
+			return PERF_ERR_PTR(-ENODEV, "cpu is offline");
 
 		cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
 		ctx = &cpuctx->ctx;
@@ -7985,15 +7986,16 @@ SYSCALL_DEFINE5(perf_event_open,
 
 	if (!attr.exclude_kernel) {
 		if (perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN))
-			return -EACCES;
+			return perf_err_sync(&attr, -EACCES,
+					     "kernel tracing forbidden for the unprivileged");
 	}
 
 	if (attr.freq) {
 		if (attr.sample_freq > sysctl_perf_event_sample_rate)
-			return -EINVAL;
+			return perf_err_sync(&attr, -EINVAL, "sample_freq too high");
 	} else {
 		if (attr.sample_period & (1ULL << 63))
-			return -EINVAL;
+			return perf_err_sync(&attr, -EINVAL, "sample_period too high");
 	}
 
 	/*
@@ -8003,14 +8005,14 @@ SYSCALL_DEFINE5(perf_event_open,
 	 * cgroup.
 	 */
 	if ((flags & PERF_FLAG_PID_CGROUP) && (pid == -1 || cpu == -1))
-		return -EINVAL;
+		return perf_err_sync(&attr, -EINVAL, "pid and cpu need to be set in cgroup mode");
 
 	if (flags & PERF_FLAG_FD_CLOEXEC)
 		f_flags |= O_CLOEXEC;
 
 	event_fd = get_unused_fd_flags(f_flags);
 	if (event_fd < 0)
-		return event_fd;
+		return perf_err_sync(&attr, event_fd, "can't obtain a file descriptor");
 
 	if (group_fd != -1) {
 		err = perf_fget_light(group_fd, &group);
-- 
2.1.4

[PATCH RFC v1 4/4] perf/x86: Annotate some of the error codes with perf_err()

[Alexander Shishkin]

This patch annotates a few x86-specific error paths with perf's extended
error reporting facility.

NYet-Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 arch/x86/kernel/cpu/perf_event.c           | 8 ++++++--
 arch/x86/kernel/cpu/perf_event_intel_lbr.c | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 3658de4790..2b22e57100 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 PERF_MODNAME	"perf/x86"
+
 #include <linux/perf_event.h>
 #include <linux/capability.h>
 #include <linux/notifier.h>
@@ -426,11 +428,13 @@ 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,
+					"BTS sampling not allowed for kernel space");
 
 		/* disallow bts if conflicting events are present */
 		if (x86_add_exclusive(x86_lbr_exclusive_lbr))
-			return -EBUSY;
+			return perf_err(-EBUSY,
+					"LBR conflicts with active events");
 
 		event->destroy = hw_perf_lbr_event_destroy;
 	}
diff --git a/arch/x86/kernel/cpu/perf_event_intel_lbr.c b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
index 452a7bd2de..b004fb310f 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
@@ -577,7 +577,7 @@ int intel_pmu_setup_lbr_filter(struct perf_event *event)
 	 * no LBR on this PMU
 	 */
 	if (!x86_pmu.lbr_nr)
-		return -EOPNOTSUPP;
+		return perf_err(-EOPNOTSUPP, "LBR is not supported by this cpu");
 
 	/*
 	 * setup SW LBR filter
-- 
2.1.4

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

[Alexander Shishkin]

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

> +static inline int perf_errno(const struct perf_err_site *site)
> +{
> +	unsigned long err = site - __start___perf_err;
> +
> +	trace_printk("[%ld] %s:%d, %d\n", err, site->file, site->line,
> site->code);

I should mention that this trace_printk() was unintentional.

Regards,
--
Alex

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

[Peter Zijlstra]

On Fri, Jul 24, 2015 at 02:45:56PM +0300, Alexander Shishkin wrote:
> +/*
> + * Use part of the [-1, -MAX_ERRNO] errno range for perf's extended error
> + * reporting. Anything within [-PERF_ERRNO, -MAX_ERRNO] is an index of a
> + * perf_err_site structure within __perf_err section. 3.5k should be enough
> + * for everybody, but let's add a boot-time warning just in case it overflows
> + * one day.
> + */
> +#define PERF_ERRNO 512

linux/errno.h has 512-529 in use, so this range overlaps with 'normal'
errno values.

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

[Peter Zijlstra]

On Fri, Jul 24, 2015 at 02:45:56PM +0300, Alexander Shishkin wrote:
> +#define __perf_err(__e, __c, __m) ({				\
> +	static struct perf_err_site				\
> +	__attribute__ ((unused,__section__("__perf_err")))	\
> +	__err_site = {						\
> +		.message	= (__m),			\
> +		.owner		= PERF_MODNAME,			\
> +		.code		= __builtin_constant_p((__c)) ?	\
> +		(__c) : 0,					\
> +	};							\
> +	(__e) = &__err_site;					\
> +})

Why is __e an argument at all? Why not simply return &__err_site ?

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

[Peter Zijlstra]

On Fri, Jul 24, 2015 at 02:45:56PM +0300, Alexander Shishkin wrote:
> +static void perf_error_report_site(struct perf_event_attr *attr,
> +				   const struct perf_err_site *site)
> +{
> +	void *buffer;
> +
> +	if (!site || !extended_reporting_enabled(attr))
> +		return;
> +
> +	/* in case of nested perf_err()s, which you shouldn't really do */
> +	while (site->code <= -PERF_ERRNO)
> +		site = perf_errno_to_site(site->code);
> +
> +	buffer = kasprintf(GFP_KERNEL,
> +			   "{\n"
> +			   "\t\"code\": %d,\n"
> +			   "\t\"module\": \"%s\",\n"
> +			   "\t\"message\": \"%s\"\n"
> +			   "}\n",
> +			   site->code, site->owner, site->message
> +			   );
> +	if (!buffer)
> +		return;
> +
> +	if (copy_to_user((void __user *)attr->perf_err, buffer,
> +			 attr->perf_err_size)) {

Should that not be min(attr->perf_err_size, strlen(buffer)) ?

Also, should we not '\0' the last char in attr->perf_err in case buffer
is longer.

> +		/* if we failed to copy once, don't bother later */
> +		attr->perf_err_size = 0;
> +	}

So we want update the user's perf_err_size with the actual size we
copied in?

> +
> +	kfree(buffer);
> +}

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

[Peter Zijlstra]

On Fri, Jul 24, 2015 at 02:45:56PM +0300, Alexander Shishkin wrote:

> @@ -3890,7 +3958,7 @@ perf_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>  	ret = perf_read_hw(event, buf, count);
>  	perf_event_ctx_unlock(event, ctx);
>  
> -	return ret;
> +	return perf_error_report(&event->attr, ret);
>  }
>  
>  static unsigned int perf_poll(struct file *file, poll_table *wait)
> @@ -4103,7 +4171,7 @@ static long perf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	ret = _perf_ioctl(event, cmd, arg);
>  	perf_event_ctx_unlock(event, ctx);
>  
> -	return ret;
> +	return perf_error_report(&event->attr, ret);
>  }
>  
>  #ifdef CONFIG_COMPAT

Oooh, shiny.. So not only does it work for SYSCALL time fails, it keeps
the pointer about and reports all subsequent fails in there too.

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

[Peter Zijlstra]

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.

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

[Alexander Shishkin]

Peter Zijlstra <peterz@infradead.org> writes:

> On Fri, Jul 24, 2015 at 02:45:56PM +0300, Alexander Shishkin wrote:
>> +static void perf_error_report_site(struct perf_event_attr *attr,
>> +				   const struct perf_err_site *site)
>> +{
>> +	void *buffer;
>> +
>> +	if (!site || !extended_reporting_enabled(attr))
>> +		return;
>> +
>> +	/* in case of nested perf_err()s, which you shouldn't really do */
>> +	while (site->code <= -PERF_ERRNO)
>> +		site = perf_errno_to_site(site->code);
>> +
>> +	buffer = kasprintf(GFP_KERNEL,
>> +			   "{\n"
>> +			   "\t\"code\": %d,\n"
>> +			   "\t\"module\": \"%s\",\n"
>> +			   "\t\"message\": \"%s\"\n"
>> +			   "}\n",
>> +			   site->code, site->owner, site->message
>> +			   );
>> +	if (!buffer)
>> +		return;
>> +
>> +	if (copy_to_user((void __user *)attr->perf_err, buffer,
>> +			 attr->perf_err_size)) {
>
> Should that not be min(attr->perf_err_size, strlen(buffer)) ?

Indeed.

> Also, should we not '\0' the last char in attr->perf_err in case buffer
> is longer.

I'm guessing we should, because otherwise the user is likely to get all
confused. They can tell, however, if the buffer they allocated was too
small by looking for (and not finding) the closing bracket.

>
>> +		/* if we failed to copy once, don't bother later */
>> +		attr->perf_err_size = 0;
>> +	}
>
> So we want update the user's perf_err_size with the actual size we
> copied in?

We *could* do that for the perf_event_open() case where we have the
user's copy of the attribute and can mess around with it, but as you
noticed we're also using this error reporting in the other syscalls that
involve perf events and with those we won't have the user's attribute
any more.

Regards,
--
Alex

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

[Ingo Molnar]

* 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?

Thanks,

	Ingo

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

[Alexander Shishkin]

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?

Regards,
--
Alex

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

[Ingo Molnar]

* 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

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

[Alexander Shishkin]

Ingo Molnar <mingo@kernel.org> writes:

> * 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.

Done.

> 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.

Yes, now I have both a perf specific and a more generic implementation
that perf then makes use of. Now let's arrive at a common denominator in
the other thread, I guess, and then we can go ahead with either version.

> 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.

Yes, what I posted yesterday should be useful for this case as well.

Regards,
--
Alex