[PATCH] perf tool: Round mmap pages to power 2

[PATCH] perf tool: Round mmap pages to power 2

[David Ahern]

Currently perf requires the -m / --mmap_pages option to be a power of 2.
To be more user friendly perf should automatically round this up to the
next power of 2.

Currently:
  $ perf record -m 3 -a -- sleep 1
  --mmap_pages/-m value must be a power of two.sleep: Terminated

With patch:
  $ perf record -m 3 -a -- sleep 1
  rounding mmap pages size to 16384 (4 pages)
  ...

Signed-off-by: David Ahern <dsahern@gmail.com>
Suggested-by: Ingo Molnar <mingo@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/evlist.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index b939221efd8d..9ec3a5a45f22 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -722,11 +722,6 @@ int perf_evlist__parse_mmap_pages(const struct option *opt, const char *str,
 	if (val != (unsigned long) -1) {
 		/* we got file size value */
 		pages = PERF_ALIGN(val, page_size) / page_size;
-		if (pages < (1UL << 31) && !is_power_of_2(pages)) {
-			pages = next_pow2(pages);
-			pr_info("rounding mmap pages size to %lu (%lu pages)\n",
-				pages * page_size, pages);
-		}
 	} else {
 		/* we got pages count value */
 		char *eptr;
@@ -737,6 +732,12 @@ int perf_evlist__parse_mmap_pages(const struct option *opt, const char *str,
 		}
 	}
 
+	if (pages < (1UL << 31) && !is_power_of_2(pages)) {
+		pages = next_pow2(pages);
+		pr_info("rounding mmap pages size to %lu (%lu pages)\n",
+			pages * page_size, pages);
+	}
+
 	if (pages > UINT_MAX || pages > SIZE_MAX / page_size) {
 		pr_err("--mmap_pages/-m value too big\n");
 		return -1;
-- 
1.8.3.4 (Apple Git-47)

Re: [PATCH] perf tool: Round mmap pages to power 2

[Adrian Hunter]

On 08/11/13 06:36, David Ahern wrote:
> Currently perf requires the -m / --mmap_pages option to be a power of 2.
> To be more user friendly perf should automatically round this up to the
> next power of 2.
>
> Currently:
>   $ perf record -m 3 -a -- sleep 1
>   --mmap_pages/-m value must be a power of two.sleep: Terminated
>
> With patch:
>   $ perf record -m 3 -a -- sleep 1
>   rounding mmap pages size to 16384 (4 pages)
>   ...

This prevents:

    --out-pages=0

from working e.g.

    tools/perf/perf record -vv --out-pages=0 uname
    rounding mmap pages size to 4096 (1 pages)

Although without this patch:

    tools/perf/perf record -vv --out-pages=0 uname
    --mmap_pages/-m value must be a power of two.
     usage: perf record [<options>] [<command>]
        or: perf record [<options>] -- <command> [<options>]

            --out-pages <pages>
                          Number of pages or size with units to use for
output (default 64M)

Also there is:

    tools/perf/perf record -vv --no-out-pages uname
    Segmentation fault (core dumped)



> Signed-off-by: David Ahern <dsahern@gmail.com>
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> ---
>  tools/perf/util/evlist.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index b939221efd8d..9ec3a5a45f22 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -722,11 +722,6 @@ int perf_evlist__parse_mmap_pages(const struct option *opt, const char *str,
>  	if (val != (unsigned long) -1) {
>  		/* we got file size value */
>  		pages = PERF_ALIGN(val, page_size) / page_size;
> -		if (pages < (1UL << 31) && !is_power_of_2(pages)) {
> -			pages = next_pow2(pages);
> -			pr_info("rounding mmap pages size to %lu (%lu pages)\n",
> -				pages * page_size, pages);
> -		}
>  	} else {
>  		/* we got pages count value */
>  		char *eptr;
> @@ -737,6 +732,12 @@ int perf_evlist__parse_mmap_pages(const struct option *opt, const char *str,
>  		}
>  	}
>  
> +	if (pages < (1UL << 31) && !is_power_of_2(pages)) {
> +		pages = next_pow2(pages);
> +		pr_info("rounding mmap pages size to %lu (%lu pages)\n",
> +			pages * page_size, pages);
> +	}
> +
>  	if (pages > UINT_MAX || pages > SIZE_MAX / page_size) {
>  		pr_err("--mmap_pages/-m value too big\n");
>  		return -1;

Re: [PATCH] perf tool: Round mmap pages to power 2

[David Ahern]

[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]

On 11/8/13, 2:11 AM, Adrian Hunter wrote:
> This prevents:
>
>      --out-pages=0
>
> from working e.g.
>
>      tools/perf/perf record -vv --out-pages=0 uname
>      rounding mmap pages size to 4096 (1 pages)
>
> Although without this patch:
>
>      tools/perf/perf record -vv --out-pages=0 uname
>      --mmap_pages/-m value must be a power of two.
>       usage: perf record [<options>] [<command>]
>          or: perf record [<options>] -- <command> [<options>]
>
>              --out-pages <pages>
>                            Number of pages or size with units to use for
> output (default 64M)
>
> Also there is:
>
>      tools/perf/perf record -vv --no-out-pages uname
>      Segmentation fault (core dumped)

This is problem with perf_evlist__parse_mmap_pages(); same thing happens 
with --no-map-pages.

With the attached both round a 0 up to 1 page:

[daahern@nxos-vdc-dev3 perf]$ perf record --out-pages 0 uname
rounding mmap pages size to 4096 (1 pages)
Linux
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.008 MB perf.data (~339 samples) ]

[daahern@nxos-vdc-dev3 perf]$ perf record --mmap-pages 0 uname
rounding mmap pages size to 4096 (1 pages)
Linux
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.008 MB perf.data (~339 samples) ]

[daahern@nxos-vdc-dev3 perf]$ perf record --no-mmap-pages uname

  usage: perf record [<options>] [<command>]
     or: perf record [<options>] -- <command> [<options>]

     -m, --mmap-pages <pages>
                           number of mmap data pages
David


[-- Attachment #2: 0001-perf-record-Fix-segfault-with-no-mmap-pages.patch --]
[-- Type: text/plain, Size: 1183 bytes --]

>From fc7c5a6b2b47a2e7a04613b4e82e478a0dcabf42 Mon Sep 17 00:00:00 2001
From: David Ahern <dsahern@gmail.com>
Date: Fri, 8 Nov 2013 07:37:43 -0700
Subject: [PATCH] perf record: Fix segfault with --no-mmap-pages

Adrian reported a segfault when using --no-out-pages
$ tools/perf/perf record -vv --no-out-pages uname
Segmentation fault (core dumped)

The same occurs with --no-mmap-pages. Fix by checking that str is non-NULL
before parsing it.

Reported-by: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/evlist.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 9ec3a5a45f22..1f103616d906 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -718,6 +718,9 @@ int perf_evlist__parse_mmap_pages(const struct option *opt, const char *str,
 		{ .tag  = 0 },
 	};
 
+	if (str == NULL)
+		return -1;
+
 	val = parse_tag_value(str, tags);
 	if (val != (unsigned long) -1) {
 		/* we got file size value */
-- 
1.8.3.4 (Apple Git-47)

Re: [PATCH] perf tool: Round mmap pages to power 2

[Adrian Hunter]

On 08/11/13 16:41, David Ahern wrote:
> On 11/8/13, 2:11 AM, Adrian Hunter wrote:
>> This prevents:
>>
>>      --out-pages=0
>>
>> from working e.g.
>>
>>      tools/perf/perf record -vv --out-pages=0 uname
>>      rounding mmap pages size to 4096 (1 pages)
>>
>> Although without this patch:
>>
>>      tools/perf/perf record -vv --out-pages=0 uname
>>      --mmap_pages/-m value must be a power of two.
>>       usage: perf record [<options>] [<command>]
>>          or: perf record [<options>] -- <command> [<options>]
>>
>>              --out-pages <pages>
>>                            Number of pages or size with units to use for
>> output (default 64M)
>>
>> Also there is:
>>
>>      tools/perf/perf record -vv --no-out-pages uname
>>      Segmentation fault (core dumped)
>
> This is problem with perf_evlist__parse_mmap_pages(); same thing happens
> with --no-map-pages.
>
> With the attached both round a 0 up to 1 page:
>
> [daahern@nxos-vdc-dev3 perf]$ perf record --out-pages 0 uname
> rounding mmap pages size to 4096 (1 pages)


Your code looks like out_pages = 0 selects write() instead of mmap().
So it you can't set out_pages = 0, how do you select write()?


> Linux
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.008 MB perf.data (~339 samples) ]
>
> [daahern@nxos-vdc-dev3 perf]$ perf record --mmap-pages 0 uname
> rounding mmap pages size to 4096 (1 pages)
> Linux
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.008 MB perf.data (~339 samples) ]
>
> [daahern@nxos-vdc-dev3 perf]$ perf record --no-mmap-pages uname
>
>  usage: perf record [<options>] [<command>]
>     or: perf record [<options>] -- <command> [<options>]
>
>     -m, --mmap-pages <pages>
>                           number of mmap data pages
> David
>

Re: [PATCH] perf tool: Round mmap pages to power 2

[Ingo Molnar]

* David Ahern <dsahern@gmail.com> wrote:

> Currently perf requires the -m / --mmap_pages option to be a power of 2.
> To be more user friendly perf should automatically round this up to the
> next power of 2.
> 
> Currently:
>   $ perf record -m 3 -a -- sleep 1
>   --mmap_pages/-m value must be a power of two.sleep: Terminated
> 
> With patch:
>   $ perf record -m 3 -a -- sleep 1
>   rounding mmap pages size to 16384 (4 pages)

Please add 'bytes'.

I'd also suggest generally prefixing tooling messages with some sort of 
'subsystem' prefix, so that in the great and rich network of perf tooling 
subsystems the user knows roughly where the message comes from.

Here it should probably be something like:

    INFO: ring-buffer: Rounding mmap pages size to 16384 bytes (4 pages)

?

While if the message was related to evlists for example and was a hard 
error, it would have this pattern:

    ERROR: event-list: ...

while if it's a warning, it would say:

    WARNING: event-list: ...

I.e. we could match how the kernel handled printk()d message types, 
priorities and subsystems.

Thanks,

	Ingo

Re: [PATCH] perf tool: Round mmap pages to power 2

[Arnaldo Carvalho de Melo]

Em Mon, Nov 11, 2013 at 12:27:54PM +0100, Ingo Molnar escreveu:
> * David Ahern <dsahern@gmail.com> wrote:
> 
> > Currently perf requires the -m / --mmap_pages option to be a power of 2.
> > To be more user friendly perf should automatically round this up to the
> > next power of 2.
> > 
> > Currently:
> >   $ perf record -m 3 -a -- sleep 1
> >   --mmap_pages/-m value must be a power of two.sleep: Terminated
> > 
> > With patch:
> >   $ perf record -m 3 -a -- sleep 1
> >   rounding mmap pages size to 16384 (4 pages)
> 
> Please add 'bytes'.

Ok, waiting for v2 with this and Adrian's concerns addressed.

- Arnaldo