Message-ID: <87r3mqgf8w.fsf@ashishki-desk.ger.corp.intel.com>
Date:	Wed, 26 Aug 2015 19:56:47 +0300
From:	Alexander Shishkin <alexander.shishkin@...ux.intel.com>
To:	Ingo Molnar <mingo@...nel.org>,
	Johannes Berg <johannes@...solutions.net>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>, adrian.hunter@...el.com,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	Vince Weaver <vince@...ter.net>,
	Arnaldo Carvalho de Melo <acme@...radead.org>,
	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	"H. Peter Anvin" <hpa@...or.com>,
	Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v2 0/6] perf: Introduce extended syscall error reporting

Ingo Molnar <mingo@...nel.org> writes:

> * Ingo Molnar <mingo@...nel.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).

# 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@...ux.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@...ux.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists