[PATCH 0/3] perf: builtin-record: Be more helpful when running up against mlock limits.

[PATCH 0/3] perf: builtin-record: Be more helpful when running up against mlock limits.

[Nelson Elhage]

With the current code, it's relatively easy to run up against the
perf_event_mlock_kb limit if you try to attach to a multithreaded
target with 'perf record -p', because perf will attempt to mmap 512k
per target thread, and the default mlock limit is (512k + 1 page),
total.

Current, this results in a completely inscrutable
  Fatal: failed to mmap with 22 (Invalid argument)

We should perhaps automatically detect a better choice of mmap_pages,
but in the meanwhile, it's easy to improve the error to give users
some guidance about what's going on, so that hopefully I can be the
last person to have to source-dive to figure out what's going on.

[PATCH 1/3] perf: __perf_evlist__mmap: Fix errno value on failed map.

[Nelson Elhage]

On failure, perf_evlist__mmap_per_{cpu,thread} will try to munmap()
every map that doesn't have a NULL base. This will fail with EINVAL if
one of them has base == MAP_FAILED, clobbering errno, so that
perf_evlist__map will return EINVAL on any failure regardless of the
root cause.

Fix this by resetting failed maps to a NULL base.

Signed-off-by: Nelson Elhage <nelhage@nelhage.com>
---
 tools/perf/util/evlist.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index fbb4b4a..271c849 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -298,8 +298,10 @@ 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)
+	if (evlist->mmap[idx].base == MAP_FAILED) {
+		evlist->mmap[idx].base = NULL;
 		return -1;
+	}
 
 	perf_evlist__add_pollfd(evlist, fd);
 	return 0;
-- 
1.7.4.41.g50b8f

Re: [PATCH 1/3] perf: __perf_evlist__mmap: Fix errno value on failed map.

[Namhyung Kim]

Hello, Nelson

Nelson Elhage <nelhage@nelhage.com> writes:
> On failure, perf_evlist__mmap_per_{cpu,thread} will try to munmap()
> every map that doesn't have a NULL base. This will fail with EINVAL if
> one of them has base == MAP_FAILED, clobbering errno, so that
> perf_evlist__map will return EINVAL on any failure regardless of the
> root cause.
>
> Fix this by resetting failed maps to a NULL base.
>
> Signed-off-by: Nelson Elhage <nelhage@nelhage.com>
> ---
>  tools/perf/util/evlist.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index fbb4b4a..271c849 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -298,8 +298,10 @@ 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)
> +	if (evlist->mmap[idx].base == MAP_FAILED) {
> +		evlist->mmap[idx].base = NULL;
>  		return -1;
> +	}
>  
>  	perf_evlist__add_pollfd(evlist, fd);
>  	return 0;

Thanks for fixing this. I posted a basically same patch for this [1]
last week, but it seems my patch doesn't get included yet (right?),
and your patch looks bit simpler. So Arnaldo, I'm totally fine if you
decide to take this instead of mine.

Thanks.
Namhyung Kim


[1] https://lkml.org/lkml/2011/12/12/275

Re: [PATCH 1/3] perf: __perf_evlist__mmap: Fix errno value on failed map.

[Arnaldo Carvalho de Melo]

Em Tue, Dec 20, 2011 at 12:33:30AM +0900, Namhyung Kim escreveu:
> Nelson Elhage <nelhage@nelhage.com> writes:
> > On failure, perf_evlist__mmap_per_{cpu,thread} will try to munmap()
> > every map that doesn't have a NULL base. This will fail with EINVAL if
> > one of them has base == MAP_FAILED, clobbering errno, so that
> > perf_evlist__map will return EINVAL on any failure regardless of the
> > root cause.
> >
> > Fix this by resetting failed maps to a NULL base.
> >
> > Signed-off-by: Nelson Elhage <nelhage@nelhage.com>

> Thanks for fixing this. I posted a basically same patch for this [1]
> last week, but it seems my patch doesn't get included yet (right?),
> and your patch looks bit simpler. So Arnaldo, I'm totally fine if you
> decide to take this instead of mine.

Ok, and thank you both for the fixes, I was out on a business trip so a
bit backlogged right now, will get to your patches as soon as possible,

Thanks!

- Arnaldo
 
> Thanks.
> Namhyung Kim
> 
> 
> [1] https://lkml.org/lkml/2011/12/12/275

Re: [PATCH 1/3] perf: __perf_evlist__mmap: Fix errno value on failed map.

[Arnaldo Carvalho de Melo]

Em Tue, Dec 20, 2011 at 12:33:30AM +0900, Namhyung Kim escreveu:
> >  	evlist->mmap[idx].base = mmap(NULL, evlist->mmap_len, prot,
> >  				      MAP_SHARED, fd, 0);
> > -	if (evlist->mmap[idx].base == MAP_FAILED)
> > +	if (evlist->mmap[idx].base == MAP_FAILED) {
> > +		evlist->mmap[idx].base = NULL;
> >  		return -1;
> > +	}
> >  
> >  	perf_evlist__add_pollfd(evlist, fd);
> >  	return 0;
> 
> Thanks for fixing this. I posted a basically same patch for this [1]
> last week, but it seems my patch doesn't get included yet (right?),
> and your patch looks bit simpler. So Arnaldo, I'm totally fine if you
> decide to take this instead of mine.

Yeah, Nelson's seems more simple, so I'm applying his and adding your
Acked-by,

Thanks,
 
> Thanks.
> Namhyung Kim
> 
> 
> [1] https://lkml.org/lkml/2011/12/12/275

[tip:perf/core] perf evlist: Fix errno value reporting on failed mmap

[tip-bot for Nelson Elhage]

Commit-ID:  301b195db179241da8be25f345f3c4e64960f1d5
Gitweb:     http://git.kernel.org/tip/301b195db179241da8be25f345f3c4e64960f1d5
Author:     Nelson Elhage <nelhage@nelhage.com>
AuthorDate: Mon, 19 Dec 2011 08:39:30 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 20 Dec 2011 13:31:15 -0200

perf evlist: Fix errno value reporting on failed mmap

On failure, perf_evlist__mmap_per_{cpu,thread} will try to munmap()
every map that doesn't have a NULL base. This will fail with EINVAL if
one of them has base == MAP_FAILED, clobbering errno, so that
perf_evlist__map will return EINVAL on any failure regardless of the
root cause.

Fix this by resetting failed maps to a NULL base.

Acked-by: Namhyung Kim <namhyung@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1324301972-22740-2-git-send-email-nelhage@nelhage.com
Signed-off-by: Nelson Elhage <nelhage@nelhage.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 8b19e7a..963d63d 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -447,8 +447,10 @@ 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)
+	if (evlist->mmap[idx].base == MAP_FAILED) {
+		evlist->mmap[idx].base = NULL;
 		return -1;
+	}
 
 	perf_evlist__add_pollfd(evlist, fd);
 	return 0;

[PATCH 2/3] perf: builtin-record: Provide advice if mmap'ing fails with EPERM.

[Nelson Elhage]

This failure is most likely due to running up against the
kernel.perf_event_mlock_kb sysctl, so we can tell the user what to do
to fix the issue.

Signed-off-by: Nelson Elhage <nelhage@nelhage.com>
---
 tools/perf/builtin-record.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 6ab58cc..45b23c1 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -358,8 +358,15 @@ try_again:
 		exit(-1);
 	}
 
-	if (perf_evlist__mmap(evlist, mmap_pages, false) < 0)
+	if (perf_evlist__mmap(evlist, mmap_pages, false) < 0) {
+		if (errno == EPERM)
+			die("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", mmap_pages);
 		die("failed to mmap with %d (%s)\n", errno, strerror(errno));
+	}
 
 	if (file_new)
 		session->evlist = evlist;
-- 
1.7.4.41.g50b8f

[tip:perf/core] perf: builtin-record: Provide advice if mmap' ing fails with EPERM.

[tip-bot for Nelson Elhage]

Commit-ID:  18e6093904abfd51671ff5846c2fdaba9ebbf21b
Gitweb:     http://git.kernel.org/tip/18e6093904abfd51671ff5846c2fdaba9ebbf21b
Author:     Nelson Elhage <nelhage@nelhage.com>
AuthorDate: Mon, 19 Dec 2011 08:39:31 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 23 Dec 2011 16:44:34 -0200

perf: builtin-record: Provide advice if mmap'ing fails with EPERM.

This failure is most likely due to running up against the
kernel.perf_event_mlock_kb sysctl, so we can tell the user what to do to
fix the issue.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1324301972-22740-3-git-send-email-nelhage@nelhage.com
Signed-off-by: Nelson Elhage <nelhage@nelhage.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index f8fd14f..56bb447 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -272,8 +272,15 @@ try_again:
 		exit(-1);
 	}
 
-	if (perf_evlist__mmap(evlist, opts->mmap_pages, false) < 0)
+	if (perf_evlist__mmap(evlist, opts->mmap_pages, false) < 0) {
+		if (errno == EPERM)
+			die("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);
 		die("failed to mmap with %d (%s)\n", errno, strerror(errno));
+	}
 
 	if (rec->file_new)
 		session->evlist = evlist;

[PATCH 3/3] perf: builtin-record: Document and check that mmap_pages must be a power of two.

[Nelson Elhage]

Now that we automatically point users at it, let's provide them some
guidance so that they hopefully don't just get mysterious EINVAL's
from the kernel.

Signed-off-by: Nelson Elhage <nelhage@nelhage.com>
---
 tools/perf/Documentation/perf-record.txt |    2 +-
 tools/perf/builtin-record.c              |    3 +++
 tools/perf/util/util.h                   |   11 +++++++++++
 3 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 5a520f8..2937f7e 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -89,7 +89,7 @@ OPTIONS
 
 -m::
 --mmap-pages=::
-	Number of mmap data pages.
+	Number of mmap data pages. Must be a power of two.
 
 -g::
 --call-graph::
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 45b23c1..275c78d 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -560,6 +560,9 @@ static int __cmd_record(int argc, const char **argv)
 	if (mmap_pages == UINT_MAX)
 		mmap_pages = (512 * 1024) / page_size;
 
+	if (!is_power_of_2(mmap_pages))
+		die("--mmap_pages/-m value must be a power of two.");
+
 	if (forks) {
 		child_pid = fork();
 		if (child_pid < 0) {
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 0128906..37be34d 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -245,4 +245,15 @@ int readn(int fd, void *buf, size_t size);
 #define _STR(x) #x
 #define STR(x) _STR(x)
 
+/*
+ *  Determine whether some value is a power of two, where zero is
+ * *not* considered a power of two.
+ */
+
+static inline __attribute__((const))
+bool is_power_of_2(unsigned long n)
+{
+	return (n != 0 && ((n & (n - 1)) == 0));
+}
+
 #endif
-- 
1.7.4.41.g50b8f

[tip:perf/core] perf: builtin-record: Document and check that mmap_pages must be a power of two.

[tip-bot for Nelson Elhage]

Commit-ID:  41d0d933494ce10eb77758a1168b08e317c42e8e
Gitweb:     http://git.kernel.org/tip/41d0d933494ce10eb77758a1168b08e317c42e8e
Author:     Nelson Elhage <nelhage@nelhage.com>
AuthorDate: Mon, 19 Dec 2011 08:39:32 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 23 Dec 2011 16:53:58 -0200

perf: builtin-record: Document and check that mmap_pages must be a power of two.

Now that we automatically point users at it, let's provide them some
guidance so that they hopefully don't just get mysterious EINVAL's
from the kernel.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1324301972-22740-4-git-send-email-nelhage@nelhage.com
Signed-off-by: Nelson Elhage <nelhage@nelhage.com>
[ committer note: Made it work after 50a682c ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Documentation/perf-record.txt |    2 +-
 tools/perf/builtin-record.c              |    3 +++
 tools/perf/util/evlist.c                 |    2 ++
 tools/perf/util/util.h                   |   11 +++++++++++
 4 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index 5a520f8..2937f7e 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -89,7 +89,7 @@ OPTIONS
 
 -m::
 --mmap-pages=::
-	Number of mmap data pages.
+	Number of mmap data pages. Must be a power of two.
 
 -g::
 --call-graph::
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 56bb447..e873ae2 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -279,6 +279,9 @@ try_again:
 			    "/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);
+		else if (!is_power_of_2(opts->mmap_pages))
+			die("--mmap_pages/-m value must be a power of two.");
+
 		die("failed to mmap with %d (%s)\n", errno, strerror(errno));
 	}
 
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 963d63d..fa18370 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -563,6 +563,8 @@ int perf_evlist__mmap(struct perf_evlist *evlist, unsigned int pages,
         /* 512 kiB: default amount of unprivileged mlocked memory */
         if (pages == UINT_MAX)
                 pages = (512 * 1024) / page_size;
+	else if (!is_power_of_2(pages))
+		return -EINVAL;
 
 	mask = pages * page_size - 1;
 
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 0128906..37be34d 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -245,4 +245,15 @@ int readn(int fd, void *buf, size_t size);
 #define _STR(x) #x
 #define STR(x) _STR(x)
 
+/*
+ *  Determine whether some value is a power of two, where zero is
+ * *not* considered a power of two.
+ */
+
+static inline __attribute__((const))
+bool is_power_of_2(unsigned long n)
+{
+	return (n != 0 && ((n & (n - 1)) == 0));
+}
+
 #endif