[PATCH 0/4] perf tools: Pedantic fixes for mmap_pages

[PATCH 0/4] perf tools: Pedantic fixes for mmap_pages

[Adrian Hunter]

Hi

Here are some pedantic fixes for mmap_pages.


Adrian Hunter (4):
      perf tools: Remove unnecessary parentheses
      perf tools: Fix max mmap_pages
      perf tools: Fix mmap pages rounding to power of 2
      perf tools: Fix display of incorrect mmap pages

 tools/perf/builtin-record.c |  2 +-
 tools/perf/util/evlist.c    | 10 ++++++----
 tools/perf/util/util.h      | 12 ++++++++++++
 3 files changed, 19 insertions(+), 5 deletions(-)


Regards
Adrian

[PATCH 1/4] perf tools: Remove unnecessary parentheses

[Adrian Hunter]

Remove unnecessary parentheses.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/evlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 7bb6ee1..4d0945c 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -732,7 +732,7 @@ static long parse_pages_arg(const char *str, unsigned long min,
 			return -EINVAL;
 	}
 
-	if ((pages == 0) && (min == 0)) {
+	if (pages == 0 && min == 0) {
 		/* leave number of pages at 0 */
 	} else if (pages < (1UL << 31) && !is_power_of_2(pages)) {
 		/* round pages up to next power of 2 */
-- 
1.7.11.7

[PATCH 2/4] perf tools: Fix max mmap_pages

[Adrian Hunter]

'SIZE_MAX / page_size' is an upper limit for
the maximum number of mmap pages, not a lower
limit.  Change the condition accordingly.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/evlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 4d0945c..98ec96b 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -754,7 +754,7 @@ int perf_evlist__parse_mmap_pages(const struct option *opt, const char *str,
 	unsigned long max = UINT_MAX;
 	long pages;
 
-	if (max < SIZE_MAX / page_size)
+	if (max > SIZE_MAX / page_size)
 		max = SIZE_MAX / page_size;
 
 	pages = parse_pages_arg(str, 1, max);
-- 
1.7.11.7

[PATCH 3/4] perf tools: Fix mmap pages rounding to power of 2

[Adrian Hunter]

'next_pow2()' only works for 'unsigned int' but the
argument is 'unsigned long'.  Checking for values
less than (1 << 31) ensures that 'next_pow2()' is
not passed a value out of range but lets anything
else go through unvalidated.

As a result mmap_pages of zero is used e.g.

  perf record -v -m2147483649 uname
  mmap size 0B
  failed to mmap with 22 (Invalid argument)

Fixed:

  perf record -m2147483649 uname
  rounding mmap pages size to 17592186044416 bytes (4294967296 pages)
  Invalid argument for --mmap_pages/-m

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/evlist.c |  6 ++++--
 tools/perf/util/util.h   | 12 ++++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 98ec96b..af25055 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -734,9 +734,11 @@ static long parse_pages_arg(const char *str, unsigned long min,
 
 	if (pages == 0 && min == 0) {
 		/* leave number of pages at 0 */
-	} else if (pages < (1UL << 31) && !is_power_of_2(pages)) {
+	} else if (!is_power_of_2(pages)) {
 		/* round pages up to next power of 2 */
-		pages = next_pow2(pages);
+		pages = next_pow2_l(pages);
+		if (!pages)
+			return -EINVAL;
 		pr_info("rounding mmap pages size to %lu bytes (%lu pages)\n",
 			pages * page_size, pages);
 	}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index adb39f2..659abf3 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -73,6 +73,7 @@
 #include <sys/ttydefaults.h>
 #include <lk/debugfs.h>
 #include <termios.h>
+#include <linux/bitops.h>
 
 extern const char *graph_line;
 extern const char *graph_dotted_line;
@@ -281,6 +282,17 @@ static inline unsigned next_pow2(unsigned x)
 	return 1ULL << (32 - __builtin_clz(x - 1));
 }
 
+static inline unsigned long next_pow2_l(unsigned long x)
+{
+#if BITS_PER_LONG == 64
+	if (x <= (1UL << 31))
+		return next_pow2(x);
+	return (unsigned long)next_pow2(x >> 32) << 32;
+#else
+	return next_pow2(x);
+#endif
+}
+
 size_t hex_width(u64 v);
 int hex2u64(const char *ptr, u64 *val);
 
-- 
1.7.11.7

[PATCH 4/4] perf tools: Fix display of incorrect mmap pages

[Adrian Hunter]

'mmap_pages' is 'unsigned int' not 'int' e.g.

  perf record -m2147483648 uname
  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: -2147483648)

Fixed:

  perf record -m2147483648 uname
  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: 2147483648)

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/builtin-record.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index d93e2ee..c1c1200 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -224,7 +224,7 @@ try_again:
 			       "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);
+			       "(current value: %u)\n", opts->mmap_pages);
 			rc = -errno;
 		} else {
 			pr_err("failed to mmap with %d (%s)\n", errno, strerror(errno));
-- 
1.7.11.7

Re: [PATCH 2/4] perf tools: Fix max mmap_pages

[Jiri Olsa]

On Mon, Dec 09, 2013 at 03:18:38PM +0200, Adrian Hunter wrote:
> 'SIZE_MAX / page_size' is an upper limit for
> the maximum number of mmap pages, not a lower
> limit.  Change the condition accordingly.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/util/evlist.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 4d0945c..98ec96b 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -754,7 +754,7 @@ int perf_evlist__parse_mmap_pages(const struct option *opt, const char *str,
>  	unsigned long max = UINT_MAX;
>  	long pages;
>  
> -	if (max < SIZE_MAX / page_size)
> +	if (max > SIZE_MAX / page_size)
>  		max = SIZE_MAX / page_size;

I agree with the change.. but it looks like the check
is superfluos, maybe we want to define max as:

   unsigned long max = SIZE_MAX / page_size

?

jirka
>  
>  	pages = parse_pages_arg(str, 1, max);
> -- 
> 1.7.11.7
> 

Re: [PATCH 0/4] perf tools: Pedantic fixes for mmap_pages

[Jiri Olsa]

On Mon, Dec 09, 2013 at 03:18:36PM +0200, Adrian Hunter wrote:
> Hi
> 
> Here are some pedantic fixes for mmap_pages.
> 
> 
> Adrian Hunter (4):
>       perf tools: Remove unnecessary parentheses
>       perf tools: Fix max mmap_pages
>       perf tools: Fix mmap pages rounding to power of 2
>       perf tools: Fix display of incorrect mmap pages
> 
>  tools/perf/builtin-record.c |  2 +-
>  tools/perf/util/evlist.c    | 10 ++++++----
>  tools/perf/util/util.h      | 12 ++++++++++++
>  3 files changed, 19 insertions(+), 5 deletions(-)

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

Re: [PATCH 2/4] perf tools: Fix max mmap_pages

[Adrian Hunter]

On 09/12/13 15:35, Jiri Olsa wrote:
> On Mon, Dec 09, 2013 at 03:18:38PM +0200, Adrian Hunter wrote:
>> 'SIZE_MAX / page_size' is an upper limit for
>> the maximum number of mmap pages, not a lower
>> limit.  Change the condition accordingly.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  tools/perf/util/evlist.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index 4d0945c..98ec96b 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -754,7 +754,7 @@ int perf_evlist__parse_mmap_pages(const struct option *opt, const char *str,
>>  	unsigned long max = UINT_MAX;
>>  	long pages;
>>  
>> -	if (max < SIZE_MAX / page_size)
>> +	if (max > SIZE_MAX / page_size)
>>  		max = SIZE_MAX / page_size;
> 
> I agree with the change.. but it looks like the check
> is superfluos, maybe we want to define max as:
> 
>    unsigned long max = SIZE_MAX / page_size

I think it is OK because mmap_pages (in struct perf_record_opts) is an
unsigned int i.e.

	max = MIN(UINT_MAX, SIZE_MAX / 4096)

c.f.
			32-bit		64-bit
UINT_MAX		2^32 - 1	2^32 - 1
SIZE_MAX		2^32 - 1	2^64 - 1
SIZE_MAX / 4096		2^20 - 1	2^52 - 1

So on 32-bit the max is SIZE_MAX / 4096 but on
64-bit the max is UINT_MAX

> 
> ?
> 
> jirka
>>  
>>  	pages = parse_pages_arg(str, 1, max);
>> -- 
>> 1.7.11.7
>>
> 
> 

[tip:perf/core] perf record: Fix display of incorrect mmap pages

[tip-bot for Adrian Hunter]

Commit-ID:  53653d70a0784a997748cc9e315ddf19d310e812
Gitweb:     http://git.kernel.org/tip/53653d70a0784a997748cc9e315ddf19d310e812
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Mon, 9 Dec 2013 15:18:40 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 9 Dec 2013 15:17:23 -0300

perf record: Fix display of incorrect mmap pages

'mmap_pages' is 'unsigned int' not 'int' e.g.

  perf record -m2147483648 uname
  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: -2147483648)

Fixed:

  perf record -m2147483648 uname
  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: 2147483648)

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1386595120-22978-5-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index d93e2ee..c1c1200 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -224,7 +224,7 @@ try_again:
 			       "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);
+			       "(current value: %u)\n", opts->mmap_pages);
 			rc = -errno;
 		} else {
 			pr_err("failed to mmap with %d (%s)\n", errno, strerror(errno));

[tip:perf/core] perf evlist: Remove unnecessary parentheses

[tip-bot for Adrian Hunter]

Commit-ID:  2bcab6c146fde0d0286b132068111f98b6217460
Gitweb:     http://git.kernel.org/tip/2bcab6c146fde0d0286b132068111f98b6217460
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Mon, 9 Dec 2013 15:18:37 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 9 Dec 2013 15:17:31 -0300

perf evlist: Remove unnecessary parentheses

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1386595120-22978-2-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 7bb6ee1..4d0945c 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -732,7 +732,7 @@ static long parse_pages_arg(const char *str, unsigned long min,
 			return -EINVAL;
 	}
 
-	if ((pages == 0) && (min == 0)) {
+	if (pages == 0 && min == 0) {
 		/* leave number of pages at 0 */
 	} else if (pages < (1UL << 31) && !is_power_of_2(pages)) {
 		/* round pages up to next power of 2 */

[tip:perf/core] perf evlist: Fix max mmap_pages

[tip-bot for Adrian Hunter]

Commit-ID:  f5ae9c424e4c80c44e9ab7e5ff4a6b79490c23be
Gitweb:     http://git.kernel.org/tip/f5ae9c424e4c80c44e9ab7e5ff4a6b79490c23be
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Mon, 9 Dec 2013 15:18:38 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 9 Dec 2013 15:17:38 -0300

perf evlist: Fix max mmap_pages

'SIZE_MAX / page_size' is an upper limit for the maximum number of mmap
pages, not a lower limit.  Change the condition accordingly.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1386595120-22978-3-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 4d0945c..98ec96b 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -754,7 +754,7 @@ int perf_evlist__parse_mmap_pages(const struct option *opt, const char *str,
 	unsigned long max = UINT_MAX;
 	long pages;
 
-	if (max < SIZE_MAX / page_size)
+	if (max > SIZE_MAX / page_size)
 		max = SIZE_MAX / page_size;
 
 	pages = parse_pages_arg(str, 1, max);

[tip:perf/core] perf evlist: Fix mmap pages rounding to power of 2

[tip-bot for Adrian Hunter]

Commit-ID:  1dbfa9387b397f2c4b8c65411b3e3fdf9284d2b1
Gitweb:     http://git.kernel.org/tip/1dbfa9387b397f2c4b8c65411b3e3fdf9284d2b1
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Mon, 9 Dec 2013 15:18:39 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 9 Dec 2013 15:18:05 -0300

perf evlist: Fix mmap pages rounding to power of 2

'next_pow2()' only works for 'unsigned int' but the argument is
'unsigned long'.  Checking for values less than (1 << 31) ensures that
'next_pow2()' is not passed a value out of range but lets anything else
go through unvalidated.

As a result mmap_pages of zero is used e.g.

  perf record -v -m2147483649 uname
  mmap size 0B
  failed to mmap with 22 (Invalid argument)

Fixed:

  perf record -m2147483649 uname
  rounding mmap pages size to 17592186044416 bytes (4294967296 pages)
  Invalid argument for --mmap_pages/-m

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1386595120-22978-4-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c |  6 ++++--
 tools/perf/util/util.h   | 12 ++++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 98ec96b..af25055 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -734,9 +734,11 @@ static long parse_pages_arg(const char *str, unsigned long min,
 
 	if (pages == 0 && min == 0) {
 		/* leave number of pages at 0 */
-	} else if (pages < (1UL << 31) && !is_power_of_2(pages)) {
+	} else if (!is_power_of_2(pages)) {
 		/* round pages up to next power of 2 */
-		pages = next_pow2(pages);
+		pages = next_pow2_l(pages);
+		if (!pages)
+			return -EINVAL;
 		pr_info("rounding mmap pages size to %lu bytes (%lu pages)\n",
 			pages * page_size, pages);
 	}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index adb39f2..659abf3 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -73,6 +73,7 @@
 #include <sys/ttydefaults.h>
 #include <lk/debugfs.h>
 #include <termios.h>
+#include <linux/bitops.h>
 
 extern const char *graph_line;
 extern const char *graph_dotted_line;
@@ -281,6 +282,17 @@ static inline unsigned next_pow2(unsigned x)
 	return 1ULL << (32 - __builtin_clz(x - 1));
 }
 
+static inline unsigned long next_pow2_l(unsigned long x)
+{
+#if BITS_PER_LONG == 64
+	if (x <= (1UL << 31))
+		return next_pow2(x);
+	return (unsigned long)next_pow2(x >> 32) << 32;
+#else
+	return next_pow2(x);
+#endif
+}
+
 size_t hex_width(u64 v);
 int hex2u64(const char *ptr, u64 *val);