[RFC 0/6] perf tools: Add perf_evlist errno

[RFC 0/6] perf tools: Add perf_evlist errno

[Jiri Olsa]

hi,
Andi reported wrong error message for :S modifier
on kernel without event ID ioctl support.

The reason was that the ioctl failed, but the error was
printed like the mmap would:

    $ perf.old record -e '{cycles,cache-misses}:S' ls
    failed to mmap with 25 (Inappropriate ioctl for device)
    ls: Terminated

I experimentally added sort of 'libc errno' interface for
perf_evlist to be able to get proper error message, like:

    $ perf record -e '{cycles,cache-misses}:S' ls
    Cannot read event group on this kernel.
    Please consider kernel update (v3.12+).
    ls: Terminated

I'm not sure about this approach. Maybe it'd be better be more
global..? So before throwing this out, sending it as RFC ;-)

thanks for ideas,
jirka

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Andi Kleen <andi@firstfloor.org>
---
Jiri Olsa (6):
      perf tools: Add perf_evlist error string interface
      perf tools: Add PERF_EVLIST__ERRNO_MMAP internal error
      perf tools: Add PERF_EVLIST__ERRNO_OPEN internal error
      perf tools: Add PERF_EVLIST__ERRNO_IOCTL_ID_GROUP internal error
      perf tools: Add PERF_EVLIST__ERRNO_NEWTP internal error
      perf tools: Use perf_evlist__strerror in kvm/record/top/trace commands

 tools/perf/builtin-kvm.c    |   2 +-
 tools/perf/builtin-record.c |  13 +------
 tools/perf/builtin-top.c    |   3 +-
 tools/perf/builtin-trace.c  |  34 ++++++++---------
 tools/perf/util/evlist.c    | 205 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------
 tools/perf/util/evlist.h    |  23 +++++++++++-
 6 files changed, 181 insertions(+), 99 deletions(-)

[PATCH 1/6] perf tools: Add perf_evlist error string interface

[Jiri Olsa]

Adding perf_evlist__strerror function that returns
static string of the last error occured during
'struct perf_evlist' handling:

  char *perf_evlist__strerror(struct perf_evlist *evlist);

After each 'struct perf_evlist' operation that supports
internal error handling, this function returns proper
error string for output.

The point is to have internal error handling so we could
print our propper error message, eg. when 'perf_evlist__mmap'
fails, we have no idea if it was for the mmap failure or ioctl
(event ID retrieval).

NOTE We can add thread safe '_r' variant in the
future, currently it's not needed.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Andi Kleen <andi@firstfloor.org>
---
 tools/perf/util/evlist.c | 34 ++++++++++++++++++++++++++++++++++
 tools/perf/util/evlist.h | 13 +++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 76fa764..dfb72d4 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -28,6 +28,11 @@
 #define FD(e, x, y) (*(int *)xyarray__entry(e->fd, x, y))
 #define SID(e, x, y) xyarray__entry(e->sample_id, x, y)
 
+#define SET_ERR(_err) ({				\
+	evlist->err      = PERF_EVLIST__ERRNO_##_err;	\
+	evlist->err_libc = errno;			\
+})
+
 void perf_evlist__init(struct perf_evlist *evlist, struct cpu_map *cpus,
 		       struct thread_map *threads)
 {
@@ -600,6 +605,34 @@ static int perf_evlist__alloc_mmap(struct perf_evlist *evlist)
 	return evlist->mmap != NULL ? 0 : -ENOMEM;
 }
 
+static void __perf_evlist__strerror(struct perf_evlist *evlist,
+				    char *buf, size_t size)
+{
+	int err_libc = evlist->err_libc;
+	int err      = evlist->err;
+
+	switch (err) {
+	case PERF_EVLIST__ERRNO_SUCCESS:
+		break;
+	default:
+		scnprintf(buf, size, "Unknown error\n");
+		return;
+	}
+
+	if (!err_libc)
+		scnprintf(buf, size, "Success.");
+	else
+		scnprintf(buf, size, "Failed with %d (%s)\n",
+			  err_libc, strerror(err_libc));
+}
+
+char *perf_evlist__strerror(struct perf_evlist *evlist)
+{
+	static char str[BUFSIZ];
+	__perf_evlist__strerror(evlist, str, BUFSIZ);
+	return str;
+}
+
 static int __perf_evlist__mmap(struct perf_evlist *evlist,
 			       int idx, int prot, int mask, int fd)
 {
@@ -607,6 +640,7 @@ static int __perf_evlist__mmap(struct perf_evlist *evlist,
 	evlist->mmap[idx].mask = mask;
 	evlist->mmap[idx].base = mmap(NULL, evlist->mmap_len, prot,
 				      MAP_SHARED, fd, 0);
+
 	if (evlist->mmap[idx].base == MAP_FAILED) {
 		pr_debug2("failed to mmap perf event ring buffer, error %d\n",
 			  errno);
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 649d6ea..9832fbd 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -17,6 +17,10 @@ struct perf_record_opts;
 #define PERF_EVLIST__HLIST_BITS 8
 #define PERF_EVLIST__HLIST_SIZE (1 << PERF_EVLIST__HLIST_BITS)
 
+enum {
+	PERF_EVLIST__ERRNO_SUCCESS		= 0,
+};
+
 struct perf_mmap {
 	void		 *base;
 	int		 mask;
@@ -45,6 +49,14 @@ struct perf_evlist {
 	struct thread_map *threads;
 	struct cpu_map	  *cpus;
 	struct perf_evsel *selected;
+
+	/*
+	 * Internal error handling:
+	 *   err      - internal error value of last operation (
+	 *   err_libc - adjacent libc errno value
+	 */
+	int		 err;
+	int		 err_libc;
 };
 
 struct perf_evsel_str_handler {
@@ -172,6 +184,7 @@ size_t perf_evlist__fprintf(struct perf_evlist *evlist, FILE *fp);
 
 int perf_evlist__strerror_tp(struct perf_evlist *evlist, int err, char *buf, size_t size);
 int perf_evlist__strerror_open(struct perf_evlist *evlist, int err, char *buf, size_t size);
+char *perf_evlist__strerror(struct perf_evlist *evlist);
 
 static inline unsigned int perf_mmap__read_head(struct perf_mmap *mm)
 {
-- 
1.8.3.1

[PATCH 2/6] perf tools: Add PERF_EVLIST__ERRNO_MMAP internal error

[Jiri Olsa]

Adding internal error (PERF_EVLIST__ERRNO_MMAP) for case
when 'mmap' syscall fails.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Andi Kleen <andi@firstfloor.org>
---
 tools/perf/util/evlist.c | 28 ++++++++++++++++++++++++++++
 tools/perf/util/evlist.h |  1 +
 2 files changed, 29 insertions(+)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index dfb72d4..d156f0a 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -605,6 +605,28 @@ static int perf_evlist__alloc_mmap(struct perf_evlist *evlist)
 	return evlist->mmap != NULL ? 0 : -ENOMEM;
 }
 
+static void strerror_mmap(struct perf_evlist *evlist,
+			  char *buf, size_t size)
+{
+	int err_libc = evlist->err_libc;
+
+	switch (evlist->err_libc) {
+	case EPERM:
+		scnprintf(buf, size,
+			"Permission error mapping pages.\n"
+			"Consider increasing "
+			"/proc/sys/kernel/perf_event_mlock_kb,\n"
+			"or try again with a smaller value of -m/--mmap_pages.\n"
+			"(current size: %dB)\n", evlist->mmap_len);
+		break;
+	default:
+		scnprintf(buf, size,
+			"Failed to mmap events with error %d (%s)\n",
+			err_libc, strerror(err_libc));
+		break;
+	}
+}
+
 static void __perf_evlist__strerror(struct perf_evlist *evlist,
 				    char *buf, size_t size)
 {
@@ -614,6 +636,9 @@ static void __perf_evlist__strerror(struct perf_evlist *evlist,
 	switch (err) {
 	case PERF_EVLIST__ERRNO_SUCCESS:
 		break;
+	case PERF_EVLIST__ERRNO_MMAP:
+		strerror_mmap(evlist, buf, size);
+		return;
 	default:
 		scnprintf(buf, size, "Unknown error\n");
 		return;
@@ -642,6 +667,7 @@ static int __perf_evlist__mmap(struct perf_evlist *evlist,
 				      MAP_SHARED, fd, 0);
 
 	if (evlist->mmap[idx].base == MAP_FAILED) {
+		SET_ERR(MMAP);
 		pr_debug2("failed to mmap perf event ring buffer, error %d\n",
 			  errno);
 		evlist->mmap[idx].base = NULL;
@@ -821,6 +847,8 @@ int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages,
 	const struct thread_map *threads = evlist->threads;
 	int prot = PROT_READ | (overwrite ? 0 : PROT_WRITE), mask;
 
+	SET_ERR(SUCCESS);
+
 	if (evlist->mmap == NULL && perf_evlist__alloc_mmap(evlist) < 0)
 		return -ENOMEM;
 
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 9832fbd..02df99a 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -19,6 +19,7 @@ struct perf_record_opts;
 
 enum {
 	PERF_EVLIST__ERRNO_SUCCESS		= 0,
+	PERF_EVLIST__ERRNO_MMAP			= 1,
 };
 
 struct perf_mmap {
-- 
1.8.3.1

[PATCH 3/6] perf tools: Add PERF_EVLIST__ERRNO_OPEN internal error

[Jiri Olsa]

Adding internal error (PERF_EVLIST__ERRNO_OPEN) for
cases when perf_evlist__open fails.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Andi Kleen <andi@firstfloor.org>
---
 tools/perf/builtin-trace.c | 11 ++++---
 tools/perf/util/evlist.c   | 75 ++++++++++++++++++++++++----------------------
 tools/perf/util/evlist.h   |  2 +-
 3 files changed, 45 insertions(+), 43 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 9f2a242..cba33e8 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1987,17 +1987,16 @@ out_delete_evlist:
 out:
 	trace->live = false;
 	return err;
+
+out_error_open:
+	fprintf(trace->output, "%s\n", perf_evlist__strerror(evlist));
+	goto out_delete_evlist;
+
 {
 	char errbuf[BUFSIZ];
 
 out_error_tp:
 	perf_evlist__strerror_tp(evlist, errno, errbuf, sizeof(errbuf));
-	goto out_error;
-
-out_error_open:
-	perf_evlist__strerror_open(evlist, errno, errbuf, sizeof(errbuf));
-
-out_error:
 	fprintf(trace->output, "%s\n", errbuf);
 	goto out_delete_evlist;
 }
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index d156f0a..4af0cca 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -627,6 +627,40 @@ static void strerror_mmap(struct perf_evlist *evlist,
 	}
 }
 
+static void strerror_open(struct perf_evlist *evlist,
+			  char *buf, size_t size)
+{
+	int printed, value, err_libc = evlist->err_libc;
+	char sbuf[128], *emsg = strerror_r(err_libc, sbuf, sizeof(sbuf));
+
+	switch (err_libc) {
+	case EACCES:
+	case EPERM:
+		printed = scnprintf(buf, size,
+				    "Error:\t%s.\n"
+				    "Hint:\tCheck /proc/sys/kernel/perf_event_paranoid setting.", emsg);
+
+		if (filename__read_int("/proc/sys/kernel/perf_event_paranoid", &value))
+			break;
+
+		printed += scnprintf(buf + printed, size - printed, "\nHint:\t");
+
+		if (value >= 2) {
+			printed += scnprintf(buf + printed, size - printed,
+					     "For your workloads it needs to be <= 1\nHint:\t");
+		}
+		printed += scnprintf(buf + printed, size - printed,
+				     "For system wide tracing it needs to be set to -1");
+
+		printed += scnprintf(buf + printed, size - printed,
+				    ".\nHint:\tThe current value is %d.", value);
+		break;
+	default:
+		scnprintf(buf, size, "%s", emsg);
+		break;
+	}
+}
+
 static void __perf_evlist__strerror(struct perf_evlist *evlist,
 				    char *buf, size_t size)
 {
@@ -639,6 +673,9 @@ static void __perf_evlist__strerror(struct perf_evlist *evlist,
 	case PERF_EVLIST__ERRNO_MMAP:
 		strerror_mmap(evlist, buf, size);
 		return;
+	case PERF_EVLIST__ERRNO_OPEN:
+		strerror_open(evlist, buf, size);
+		return;
 	default:
 		scnprintf(buf, size, "Unknown error\n");
 		return;
@@ -1089,8 +1126,10 @@ int perf_evlist__open(struct perf_evlist *evlist)
 			goto out_err;
 	}
 
+	SET_ERR(SUCCESS);
 	return 0;
 out_err:
+	SET_ERR(OPEN);
 	perf_evlist__close(evlist);
 	errno = -err;
 	return err;
@@ -1243,39 +1282,3 @@ int perf_evlist__strerror_tp(struct perf_evlist *evlist __maybe_unused,
 
 	return 0;
 }
-
-int perf_evlist__strerror_open(struct perf_evlist *evlist __maybe_unused,
-			       int err, char *buf, size_t size)
-{
-	int printed, value;
-	char sbuf[128], *emsg = strerror_r(err, sbuf, sizeof(sbuf));
-
-	switch (err) {
-	case EACCES:
-	case EPERM:
-		printed = scnprintf(buf, size,
-				    "Error:\t%s.\n"
-				    "Hint:\tCheck /proc/sys/kernel/perf_event_paranoid setting.", emsg);
-
-		if (filename__read_int("/proc/sys/kernel/perf_event_paranoid", &value))
-			break;
-
-		printed += scnprintf(buf + printed, size - printed, "\nHint:\t");
-
-		if (value >= 2) {
-			printed += scnprintf(buf + printed, size - printed,
-					     "For your workloads it needs to be <= 1\nHint:\t");
-		}
-		printed += scnprintf(buf + printed, size - printed,
-				     "For system wide tracing it needs to be set to -1");
-
-		printed += scnprintf(buf + printed, size - printed,
-				    ".\nHint:\tThe current value is %d.", value);
-		break;
-	default:
-		scnprintf(buf, size, "%s", emsg);
-		break;
-	}
-
-	return 0;
-}
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 02df99a..48ac4c5 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -20,6 +20,7 @@ struct perf_record_opts;
 enum {
 	PERF_EVLIST__ERRNO_SUCCESS		= 0,
 	PERF_EVLIST__ERRNO_MMAP			= 1,
+	PERF_EVLIST__ERRNO_OPEN			= 2,
 };
 
 struct perf_mmap {
@@ -184,7 +185,6 @@ static inline struct perf_evsel *perf_evlist__last(struct perf_evlist *evlist)
 size_t perf_evlist__fprintf(struct perf_evlist *evlist, FILE *fp);
 
 int perf_evlist__strerror_tp(struct perf_evlist *evlist, int err, char *buf, size_t size);
-int perf_evlist__strerror_open(struct perf_evlist *evlist, int err, char *buf, size_t size);
 char *perf_evlist__strerror(struct perf_evlist *evlist);
 
 static inline unsigned int perf_mmap__read_head(struct perf_mmap *mm)
-- 
1.8.3.1

[PATCH 4/6] perf tools: Add PERF_EVLIST__ERRNO_IOCTL_ID_GROUP internal error

[Jiri Olsa]

Adding internal error (PERF_EVLIST__ERRNO_IOCTL_ID_GROUP) for
cases when ioctl syscall fails during perf_evlist__mmap call.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Andi Kleen <andi@firstfloor.org>
---
 tools/perf/util/evlist.c | 9 ++++++++-
 tools/perf/util/evlist.h | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 4af0cca..f9c5abd 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -399,8 +399,10 @@ static int perf_evlist__id_add_fd(struct perf_evlist *evlist,
 	 * This way does not work with group format read, so bail
 	 * out in that case.
 	 */
-	if (perf_evlist__read_format(evlist) & PERF_FORMAT_GROUP)
+	if (perf_evlist__read_format(evlist) & PERF_FORMAT_GROUP) {
+		SET_ERR(IOCTL_ID_GROUP);
 		return -1;
+	}
 
 	if (!(evsel->attr.read_format & PERF_FORMAT_ID) ||
 	    read(fd, &read_data, sizeof(read_data)) == -1)
@@ -676,6 +678,11 @@ static void __perf_evlist__strerror(struct perf_evlist *evlist,
 	case PERF_EVLIST__ERRNO_OPEN:
 		strerror_open(evlist, buf, size);
 		return;
+	case PERF_EVLIST__ERRNO_IOCTL_ID_GROUP:
+		scnprintf(buf, size,
+			"Cannot read event group on this kernel.\n"
+			"Please consider kernel update (v3.12+).\n");
+		return;
 	default:
 		scnprintf(buf, size, "Unknown error\n");
 		return;
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 48ac4c5..e5ce8c7 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -21,6 +21,7 @@ enum {
 	PERF_EVLIST__ERRNO_SUCCESS		= 0,
 	PERF_EVLIST__ERRNO_MMAP			= 1,
 	PERF_EVLIST__ERRNO_OPEN			= 2,
+	PERF_EVLIST__ERRNO_IOCTL_ID_GROUP	= 3,
 };
 
 struct perf_mmap {
-- 
1.8.3.1

[PATCH 5/6] perf tools: Add PERF_EVLIST__ERRNO_NEWTP internal error

[Jiri Olsa]

Adding internal error (PERF_EVLIST__ERRNO_NEWTP) for case
when perf_evlist__add_newtp call fails.

Moving perf_evlist__strerror_tp function to the error
string handling framework.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Andi Kleen <andi@firstfloor.org>
---
 tools/perf/builtin-trace.c | 31 ++++++++++-----------
 tools/perf/util/evlist.c   | 67 ++++++++++++++++++++++++----------------------
 tools/perf/util/evlist.h   |  6 +++++
 3 files changed, 55 insertions(+), 49 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index cba33e8..f2886c7 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -199,23 +199,29 @@ static int perf_evlist__add_syscall_newtp(struct perf_evlist *evlist,
 	int ret = -1;
 	struct perf_evsel *sys_enter, *sys_exit;
 
+#define GOTO_ERR(label) ({			\
+	PERF_EVLIST__SET_ERR(evlist, OPEN);	\
+	goto label;				\
+})
+
 	sys_enter = perf_evsel__syscall_newtp("sys_enter", sys_enter_handler);
 	if (sys_enter == NULL)
-		goto out;
+		GOTO_ERR(out);
 
 	if (perf_evsel__init_sc_tp_ptr_field(sys_enter, args))
-		goto out_delete_sys_enter;
+		GOTO_ERR(out_delete_sys_enter);
 
 	sys_exit = perf_evsel__syscall_newtp("sys_exit", sys_exit_handler);
 	if (sys_exit == NULL)
-		goto out_delete_sys_enter;
+		GOTO_ERR(out_delete_sys_enter);
 
 	if (perf_evsel__init_sc_tp_uint_field(sys_exit, ret))
-		goto out_delete_sys_exit;
+		GOTO_ERR(out_delete_sys_exit);
 
 	perf_evlist__add(evlist, sys_enter);
 	perf_evlist__add(evlist, sys_exit);
 
+	PERF_EVLIST__SET_ERR(evlist, SUCCESS);
 	ret = 0;
 out:
 	return ret;
@@ -1851,14 +1857,14 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
 	}
 
 	if (perf_evlist__add_syscall_newtp(evlist, trace__sys_enter, trace__sys_exit))
-		goto out_error_tp;
+		goto out_error;
 
 	perf_evlist__add_vfs_getname(evlist);
 
 	if (trace->sched &&
 		perf_evlist__add_newtp(evlist, "sched", "sched_stat_runtime",
 				trace__sched_stat_runtime))
-		goto out_error_tp;
+		goto out_error;
 
 	err = perf_evlist__create_maps(evlist, &trace->opts.target);
 	if (err < 0) {
@@ -1888,7 +1894,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
 
 	err = perf_evlist__open(evlist);
 	if (err < 0)
-		goto out_error_open;
+		goto out_error;
 
 	err = perf_evlist__mmap(evlist, trace->opts.mmap_pages, false);
 	if (err < 0) {
@@ -1988,18 +1994,9 @@ out:
 	trace->live = false;
 	return err;
 
-out_error_open:
+out_error:
 	fprintf(trace->output, "%s\n", perf_evlist__strerror(evlist));
 	goto out_delete_evlist;
-
-{
-	char errbuf[BUFSIZ];
-
-out_error_tp:
-	perf_evlist__strerror_tp(evlist, errno, errbuf, sizeof(errbuf));
-	fprintf(trace->output, "%s\n", errbuf);
-	goto out_delete_evlist;
-}
 }
 
 static int trace__replay(struct trace *trace)
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index f9c5abd..edb933c 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -28,10 +28,7 @@
 #define FD(e, x, y) (*(int *)xyarray__entry(e->fd, x, y))
 #define SID(e, x, y) xyarray__entry(e->sample_id, x, y)
 
-#define SET_ERR(_err) ({				\
-	evlist->err      = PERF_EVLIST__ERRNO_##_err;	\
-	evlist->err_libc = errno;			\
-})
+#define SET_ERR(_err) PERF_EVLIST__SET_ERR(evlist, _err)
 
 void perf_evlist__init(struct perf_evlist *evlist, struct cpu_map *cpus,
 		       struct thread_map *threads)
@@ -258,9 +255,12 @@ int perf_evlist__add_newtp(struct perf_evlist *evlist,
 {
 	struct perf_evsel *evsel = perf_evsel__newtp(sys, name);
 
-	if (evsel == NULL)
+	if (evsel == NULL) {
+		SET_ERR(NEWTP);
 		return -1;
+	}
 
+	SET_ERR(SUCCESS);
 	evsel->handler = handler;
 	perf_evlist__add(evlist, evsel);
 	return 0;
@@ -663,6 +663,33 @@ static void strerror_open(struct perf_evlist *evlist,
 	}
 }
 
+static void strerror_newtp(struct perf_evlist *evlist,
+			char *buf, size_t size)
+{
+	int err_libc = evlist->err_libc;
+	char sbuf[128];
+
+	switch (err_libc) {
+	case ENOENT:
+		scnprintf(buf, size, "%s",
+			  "Error:\tUnable to find debugfs\n"
+			  "Hint:\tWas your kernel was compiled with debugfs support?\n"
+			  "Hint:\tIs the debugfs filesystem mounted?\n"
+			  "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");
+		break;
+	case EACCES:
+		scnprintf(buf, size,
+			  "Error:\tNo permissions to read %s/tracing/events/raw_syscalls\n"
+			  "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n",
+			  debugfs_mountpoint, debugfs_mountpoint);
+		break;
+	default:
+		scnprintf(buf, size, "%s",
+			  strerror_r(err_libc, sbuf, sizeof(sbuf)));
+		break;
+	}
+}
+
 static void __perf_evlist__strerror(struct perf_evlist *evlist,
 				    char *buf, size_t size)
 {
@@ -683,6 +710,9 @@ static void __perf_evlist__strerror(struct perf_evlist *evlist,
 			"Cannot read event group on this kernel.\n"
 			"Please consider kernel update (v3.12+).\n");
 		return;
+	case PERF_EVLIST__ERRNO_NEWTP:
+		strerror_newtp(evlist, buf, size);
+		return;
 	default:
 		scnprintf(buf, size, "Unknown error\n");
 		return;
@@ -1262,30 +1292,3 @@ size_t perf_evlist__fprintf(struct perf_evlist *evlist, FILE *fp)
 
 	return printed + fprintf(fp, "\n");
 }
-
-int perf_evlist__strerror_tp(struct perf_evlist *evlist __maybe_unused,
-			     int err, char *buf, size_t size)
-{
-	char sbuf[128];
-
-	switch (err) {
-	case ENOENT:
-		scnprintf(buf, size, "%s",
-			  "Error:\tUnable to find debugfs\n"
-			  "Hint:\tWas your kernel was compiled with debugfs support?\n"
-			  "Hint:\tIs the debugfs filesystem mounted?\n"
-			  "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");
-		break;
-	case EACCES:
-		scnprintf(buf, size,
-			  "Error:\tNo permissions to read %s/tracing/events/raw_syscalls\n"
-			  "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n",
-			  debugfs_mountpoint, debugfs_mountpoint);
-		break;
-	default:
-		scnprintf(buf, size, "%s", strerror_r(err, sbuf, sizeof(sbuf)));
-		break;
-	}
-
-	return 0;
-}
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index e5ce8c7..46036e3 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -17,11 +17,17 @@ struct perf_record_opts;
 #define PERF_EVLIST__HLIST_BITS 8
 #define PERF_EVLIST__HLIST_SIZE (1 << PERF_EVLIST__HLIST_BITS)
 
+#define PERF_EVLIST__SET_ERR(evlist, _err) ({		\
+	evlist->err      = PERF_EVLIST__ERRNO_##_err;	\
+	evlist->err_libc = errno;			\
+})
+
 enum {
 	PERF_EVLIST__ERRNO_SUCCESS		= 0,
 	PERF_EVLIST__ERRNO_MMAP			= 1,
 	PERF_EVLIST__ERRNO_OPEN			= 2,
 	PERF_EVLIST__ERRNO_IOCTL_ID_GROUP	= 3,
+	PERF_EVLIST__ERRNO_NEWTP		= 4,
 };
 
 struct perf_mmap {
-- 
1.8.3.1

[PATCH 6/6] perf tools: Use perf_evlist__strerror in kvm/record/top/trace commands

[Jiri Olsa]

Using perf_evlist__strerror in kvm/record/top/trace commands
to handle perf_evlist__mmap failures.

Examples with kernel not supporting event ID ioctl:

  * record session before:

    $ perf.old record -e '{cycles,cache-misses}:S' ls
    failed to mmap with 25 (Inappropriate ioctl for device)
    ls: Terminated

  * record session now:

    $ perf record -e '{cycles,cache-misses}:S' ls
    Cannot read event group on this kernel.
    Please consider kernel update (v3.12+).
    ls: Terminated

Examples with session allocating ring buffer with the
size over the allowed threshold.

  * record session before:

    $ perf record -m 10M ls
    rounding mmap pages size to 16777216 bytes (4096 pages)
    Permission error mapping pages.
    Consider increasing /proc/sys/kernel/perf_event_mlock_kb,
    or try again with a smaller value of -m/--mmap_pages.
    (current size: 16781312B)
    ls: Terminated

  * record session now:

    $ perf record -m 10M ls
    rounding mmap pages size to 16777216 bytes (4096 pages)
    Permission error mapping pages.
    Consider increasing /proc/sys/kernel/perf_event_mlock_kb,
    or try again with a smaller value of -m/--mmap_pages.
    (current value: 4096)
    ls: Terminated

  * top session, before:

    $ perf top -m 100M --stdio
    rounding mmap pages size to 134217728 bytes (32768 pages)
    Error:
    Failed to mmap with 1 (Operation not permitted)

  * top session, now:

    $ perf top -m 100M --stdio
    rounding mmap pages size to 134217728 bytes (32768 pages)
    Error:
    Permission error mapping pages.
    Consider increasing /proc/sys/kernel/perf_event_mlock_kb,
    or try again with a smaller value of -m/--mmap_pages.
    (current size: 134221824B)

  * trace session, before:

    $ perf trace -m 100M ./ex
    rounding mmap pages size to 16777216 bytes (4096 pages)
    Permission error mapping pages.
    Consider increasing /proc/sys/kernel/perf_event_mlock_kb,
    or try again with a smaller value of -m/--mmap_pages.
    (current value: 4096)
    ls: Terminated

  * trace session, now:

    $ perf trace -m 100M ./ex
    rounding mmap pages size to 134217728 bytes (32768 pages)
    Permission error mapping pages.
    Consider increasing /proc/sys/kernel/perf_event_mlock_kb,
    or try again with a smaller value of -m/--mmap_pages.
    (current size: 134221824B)

  * kvm stat live session, before:

    $ perf kvm stat live -m 10000M
    rounding mmap pages size to 17179869184 bytes (4194304 pages)
    Error:
    Failed to mmap the events: Cannot allocate memory

  * kvm stat live session, now:

    $ perf kvm stat live -m 10000M
    rounding mmap pages size to 17179869184 bytes (4194304 pages)
    Error:
    Failed to mmap events with error 12 (Cannot allocate memory)

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Andi Kleen <andi@firstfloor.org>
---
 tools/perf/builtin-kvm.c    |  2 +-
 tools/perf/builtin-record.c | 13 ++-----------
 tools/perf/builtin-top.c    |  3 +--
 tools/perf/builtin-trace.c  |  2 +-
 4 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index f8bf5f2..131781f 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1210,7 +1210,7 @@ static int kvm_live_open_events(struct perf_kvm_stat *kvm)
 	}
 
 	if (perf_evlist__mmap(evlist, kvm->opts.mmap_pages, false) < 0) {
-		ui__error("Failed to mmap the events: %s\n", strerror(errno));
+		ui__error("%s", perf_evlist__strerror(evlist));
 		perf_evlist__close(evlist);
 		goto out;
 	}
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index d93e2ee..f697362 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -219,17 +219,8 @@ try_again:
 	}
 
 	if (perf_evlist__mmap(evlist, opts->mmap_pages, false) < 0) {
-		if (errno == EPERM) {
-			pr_err("Permission error mapping pages.\n"
-			       "Consider increasing "
-			       "/proc/sys/kernel/perf_event_mlock_kb,\n"
-			       "or try again with a smaller value of -m/--mmap_pages.\n"
-			       "(current value: %d)\n", opts->mmap_pages);
-			rc = -errno;
-		} else {
-			pr_err("failed to mmap with %d (%s)\n", errno, strerror(errno));
-			rc = -errno;
-		}
+		pr_err("%s", perf_evlist__strerror(evlist));
+		rc = -errno;
 		goto out;
 	}
 
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 03d37a7..3825495 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -879,8 +879,7 @@ try_again:
 	}
 
 	if (perf_evlist__mmap(evlist, opts->mmap_pages, false) < 0) {
-		ui__error("Failed to mmap with %d (%s)\n",
-			    errno, strerror(errno));
+		ui__error("%s", perf_evlist__strerror(evlist));
 		goto out_err;
 	}
 
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index f2886c7..2fed70b 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1898,7 +1898,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
 
 	err = perf_evlist__mmap(evlist, trace->opts.mmap_pages, false);
 	if (err < 0) {
-		fprintf(trace->output, "Couldn't mmap the events: %s\n", strerror(errno));
+		fprintf(trace->output, "%s", perf_evlist__strerror(evlist));
 		goto out_close_evlist;
 	}
 
-- 
1.8.3.1

Re: [RFC 0/6] perf tools: Add perf_evlist errno

[Namhyung Kim]

Hi Jiri,

On Fri, 29 Nov 2013 12:45:04 +0100, Jiri Olsa wrote:
> hi,
> Andi reported wrong error message for :S modifier
> on kernel without event ID ioctl support.
>
> The reason was that the ioctl failed, but the error was
> printed like the mmap would:
>
>     $ perf.old record -e '{cycles,cache-misses}:S' ls
>     failed to mmap with 25 (Inappropriate ioctl for device)
>     ls: Terminated

I see same confusing error message..

>
> I experimentally added sort of 'libc errno' interface for
> perf_evlist to be able to get proper error message, like:
>
>     $ perf record -e '{cycles,cache-misses}:S' ls
>     Cannot read event group on this kernel.
>     Please consider kernel update (v3.12+).
>     ls: Terminated
>
> I'm not sure about this approach. Maybe it'd be better be more
> global..? So before throwing this out, sending it as RFC ;-)

I like it. :)  We still need to improve this user-visible error handling.

But what you mean by 'more global'?  I think the evlist APIs are pretty
global alreay.  And we have same error handling in perf_target code too.

But I think it'd be better making it thread-safe even though it's not
needed for now.  The code is growing really fast.. ;-)

Thanks,
Namhyung

Re: [RFC 0/6] perf tools: Add perf_evlist errno

[Jiri Olsa]

On Mon, Dec 02, 2013 at 05:19:18PM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Fri, 29 Nov 2013 12:45:04 +0100, Jiri Olsa wrote:
> > hi,
> > Andi reported wrong error message for :S modifier
> > on kernel without event ID ioctl support.
> >
> > The reason was that the ioctl failed, but the error was
> > printed like the mmap would:
> >
> >     $ perf.old record -e '{cycles,cache-misses}:S' ls
> >     failed to mmap with 25 (Inappropriate ioctl for device)
> >     ls: Terminated
> 
> I see same confusing error message..
> 
> >
> > I experimentally added sort of 'libc errno' interface for
> > perf_evlist to be able to get proper error message, like:
> >
> >     $ perf record -e '{cycles,cache-misses}:S' ls
> >     Cannot read event group on this kernel.
> >     Please consider kernel update (v3.12+).
> >     ls: Terminated
> >
> > I'm not sure about this approach. Maybe it'd be better be more
> > global..? So before throwing this out, sending it as RFC ;-)
> 
> I like it. :)  We still need to improve this user-visible error handling.
> 
> But what you mean by 'more global'?  I think the evlist APIs are pretty
> global alreay.  And we have same error handling in perf_target code too.

aah, missed the target object has that already.. I meant
to keep the same way of error handling globaly

> 
> But I think it'd be better making it thread-safe even though it's not
> needed for now.  The code is growing really fast.. ;-)

ok, will repost 

thanks,
jirka