[RFC] perf: perf record sets inherit by default

[RFC] perf: perf record sets inherit by default

[Stephane Eranian]

Hi,


I am confused by the inheritance cmd line option of perf record:

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

    -e, --event <event>   event selector. use 'perf list' to list
available events
        --filter <filter>
                          event filter
    -p, --pid <n>         record events on existing process id
    -t, --tid <n>         record events on existing thread id
    -r, --realtime <n>    collect data with this RT SCHED_FIFO priority
    -R, --raw-samples     collect raw sample records from all opened counters
    -a, --all-cpus        system-wide collection from all CPUs
    -A, --append          append to the output file to do incremental profiling
    -C, --profile_cpu <n>
                          CPU to profile on
    -f, --force           overwrite existing data file (deprecated)
    -c, --count           event period to sample
    -o, --output <file>   output file name
    -i, --inherit         child tasks inherit counters

This leads to believe that by default inheritance in children is off.

However, builtin-record.c says:

static bool                     inherit                         =   true;

If that's the case, what's the point of the -i option?

Another side effect of inheritance is that in per-thread mode,
perf creates as many "sessions" as you have CPUs. So
on a 16-way processor, sampling on cycles, perf creates
16 events and 16 x 2-page sampling buffers. That's a lot of
resources consumed if I am just interested in monitoring
a single-threaded workload.

Am I missing something here?

Re: [RFC] perf: perf record sets inherit by default

[Peter Zijlstra]

On Tue, 2010-05-11 at 16:04 +0200, Stephane Eranian wrote:
> Hi,
> 
> 
> I am confused by the inheritance cmd line option of perf record:
> 
> $ perf record -h
>  usage: perf record [<options>] [<command>]
>     or: perf record [<options>] -- <command> [<options>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list
> available events
>         --filter <filter>
>                           event filter
>     -p, --pid <n>         record events on existing process id
>     -t, --tid <n>         record events on existing thread id
>     -r, --realtime <n>    collect data with this RT SCHED_FIFO priority
>     -R, --raw-samples     collect raw sample records from all opened counters
>     -a, --all-cpus        system-wide collection from all CPUs
>     -A, --append          append to the output file to do incremental profiling
>     -C, --profile_cpu <n>
>                           CPU to profile on
>     -f, --force           overwrite existing data file (deprecated)
>     -c, --count           event period to sample
>     -o, --output <file>   output file name
>     -i, --inherit         child tasks inherit counters
> 
> This leads to believe that by default inheritance in children is off.
> 
> However, builtin-record.c says:
> 
> static bool                     inherit                         =   true;
> 
> If that's the case, what's the point of the -i option?

Right, I think we should invert that, does --no-inherit work?

> Another side effect of inheritance is that in per-thread mode,
> perf creates as many "sessions" as you have CPUs. So
> on a 16-way processor, sampling on cycles, perf creates
> 16 events and 16 x 2-page sampling buffers. That's a lot of
> resources consumed if I am just interested in monitoring
> a single-threaded workload.

Right, but I think the default of inherit is right, and once you do that
you basically have to do the per-task-per-cpu thing, otherwise your
fancy 16-way will start spending most of its time in cacheline bounces.

That said, !inherit wouldn't need that, so a patch doing that would be
nice.

Re: [RFC] perf: perf record sets inherit by default

[Stephane Eranian]

On Tue, May 11, 2010 at 4:48 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, 2010-05-11 at 16:04 +0200, Stephane Eranian wrote:
>> Hi,
>>
>>
>> I am confused by the inheritance cmd line option of perf record:
>>
>> $ perf record -h
>>  usage: perf record [<options>] [<command>]
>>     or: perf record [<options>] -- <command> [<options>]
>>
>>     -e, --event <event>   event selector. use 'perf list' to list
>> available events
>>         --filter <filter>
>>                           event filter
>>     -p, --pid <n>         record events on existing process id
>>     -t, --tid <n>         record events on existing thread id
>>     -r, --realtime <n>    collect data with this RT SCHED_FIFO priority
>>     -R, --raw-samples     collect raw sample records from all opened counters
>>     -a, --all-cpus        system-wide collection from all CPUs
>>     -A, --append          append to the output file to do incremental profiling
>>     -C, --profile_cpu <n>
>>                           CPU to profile on
>>     -f, --force           overwrite existing data file (deprecated)
>>     -c, --count           event period to sample
>>     -o, --output <file>   output file name
>>     -i, --inherit         child tasks inherit counters
>>
>> This leads to believe that by default inheritance in children is off.
>>
>> However, builtin-record.c says:
>>
>> static bool                     inherit                         =   true;
>>
>> If that's the case, what's the point of the -i option?
>
> Right, I think we should invert that, does --no-inherit work?
>
>> Another side effect of inheritance is that in per-thread mode,
>> perf creates as many "sessions" as you have CPUs. So
>> on a 16-way processor, sampling on cycles, perf creates
>> 16 events and 16 x 2-page sampling buffers. That's a lot of
>> resources consumed if I am just interested in monitoring
>> a single-threaded workload.
>
> Right, but I think the default of inherit is right, and once you do that
> you basically have to do the per-task-per-cpu thing, otherwise your
> fancy 16-way will start spending most of its time in cacheline bounces.
>
In that case, don't you think you should also ensure that the buffer is
allocated on the NUMA node of the designated per-thread-per-cpu?
I don't think it is the case today.

Re: [RFC] perf: perf record sets inherit by default

[Peter Zijlstra]

On Mon, 2010-05-17 at 16:25 +0200, Stephane Eranian wrote:
> > Right, but I think the default of inherit is right, and once you do that
> > you basically have to do the per-task-per-cpu thing, otherwise your
> > fancy 16-way will start spending most of its time in cacheline bounces.
> >
> In that case, don't you think you should also ensure that the buffer is
> allocated on the NUMA node of the designated per-thread-per-cpu?
> I don't think it is the case today.

Yeah, something like the below ought to do I guess..

Almost-Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 kernel/perf_event.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 9dbe8cd..85e2d32 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -2288,6 +2288,19 @@ perf_mmap_to_page(struct perf_mmap_data *data, unsigned long pgoff)
 	return virt_to_page(data->data_pages[pgoff - 1]);
 }
 
+static void *perf_mmap_alloc_page(int cpu)
+{
+	struct page *page;
+	int node;
+
+	node = (cpu == -1) ? cpu : cpu_to_node(cpu);
+	page = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 0);
+	if (!page)
+		return NULL;
+
+	return page_address(page);
+}
+
 static struct perf_mmap_data *
 perf_mmap_data_alloc(struct perf_event *event, int nr_pages)
 {
@@ -2304,12 +2317,12 @@ perf_mmap_data_alloc(struct perf_event *event, int nr_pages)
 	if (!data)
 		goto fail;
 
-	data->user_page = (void *)get_zeroed_page(GFP_KERNEL);
+	data->user_page = perf_mmap_alloc_page(event->cpu);
 	if (!data->user_page)
 		goto fail_user_page;
 
 	for (i = 0; i < nr_pages; i++) {
-		data->data_pages[i] = (void *)get_zeroed_page(GFP_KERNEL);
+		data->data_pages[i] = perf_mmap_alloc_page(event->cpu);
 		if (!data->data_pages[i])
 			goto fail_data_pages;
 	}

[tip:perf/core] perf: Optimize buffer placement by allocating buffers NUMA aware

[tip-bot for Peter Zijlstra]

Commit-ID:  a19d35c11fd559dd7dfd5a2078df7c9af74a5d88
Gitweb:     http://git.kernel.org/tip/a19d35c11fd559dd7dfd5a2078df7c9af74a5d88
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Mon, 17 May 2010 18:48:00 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 18 May 2010 18:35:47 +0200

perf: Optimize buffer placement by allocating buffers NUMA aware

Ensure cpu bound buffers live on the right NUMA node.

Suggested-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <1274114880.5605.5236.camel@twins>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/perf_event.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 8d61d29..6ae6218 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -2320,6 +2320,19 @@ perf_mmap_to_page(struct perf_mmap_data *data, unsigned long pgoff)
 	return virt_to_page(data->data_pages[pgoff - 1]);
 }
 
+static void *perf_mmap_alloc_page(int cpu)
+{
+	struct page *page;
+	int node;
+
+	node = (cpu == -1) ? cpu : cpu_to_node(cpu);
+	page = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 0);
+	if (!page)
+		return NULL;
+
+	return page_address(page);
+}
+
 static struct perf_mmap_data *
 perf_mmap_data_alloc(struct perf_event *event, int nr_pages)
 {
@@ -2336,12 +2349,12 @@ perf_mmap_data_alloc(struct perf_event *event, int nr_pages)
 	if (!data)
 		goto fail;
 
-	data->user_page = (void *)get_zeroed_page(GFP_KERNEL);
+	data->user_page = perf_mmap_alloc_page(event->cpu);
 	if (!data->user_page)
 		goto fail_user_page;
 
 	for (i = 0; i < nr_pages; i++) {
-		data->data_pages[i] = (void *)get_zeroed_page(GFP_KERNEL);
+		data->data_pages[i] = perf_mmap_alloc_page(event->cpu);
 		if (!data->data_pages[i])
 			goto fail_data_pages;
 	}

Re: [RFC] perf: perf record sets inherit by default

[Arnaldo Carvalho de Melo]

Em Tue, May 11, 2010 at 04:04:17PM +0200, Stephane Eranian escreveu:
> I am confused by the inheritance cmd line option of perf record:

>  usage: perf record [<options>] [<command>]
>     or: perf record [<options>] -- <command> [<options>]
>     -p, --pid <n>         record events on existing process id
>     -t, --tid <n>         record events on existing thread id
>     -a, --all-cpus        system-wide collection from all CPUs
>     -C, --profile_cpu <n> CPU to profile on
>     -i, --inherit         child tasks inherit counters
 
> This leads to believe that by default inheritance in children is off.
 
> However, builtin-record.c says:
> static bool                     inherit    =   true;
 
> If that's the case, what's the point of the -i option?

Humm, since for -C and -a using -i doesn't make sense, I guess it should
be off by default and only be auto-activated if we don't specify any
option, i.e. when using it like:

perf record ./hackbench

What do you think?
 
> Another side effect of inheritance is that in per-thread mode, perf
> creates as many "sessions" as you have CPUs. So on a 16-way processor,
> sampling on cycles, perf creates 16 events and 16 x 2-page sampling
> buffers. That's a lot of resources consumed if I am just interested in
> monitoring a single-threaded workload.
 
> Am I missing something here?

I don't think so, but maybe I'm missing too :-)

- Arnaldo

Re: [RFC] perf: perf record sets inherit by default

[Peter Zijlstra]

On Tue, 2010-05-11 at 12:00 -0300, Arnaldo Carvalho de Melo wrote:
> 
> Humm, since for -C and -a using -i doesn't make sense, I guess it should
> be off by default and only be auto-activated if we don't specify any
> option, i.e. when using it like:
> 
> perf record ./hackbench
> 
> What do you think? 

-ENOPARSE

-a/-C usage creates per-cpu counters and will thus ignore any and all
perf_event_attr::inherit state.

Your above suggestion would still have inherit enabled by default, and
would thus not change anything.

The thing is that perf-record defaults to inherited per-task-per-cpu
counters, which, I think, is a reasonable default, just sub-optimal for
single threaded/!forking subjects.

So what would make sense is for -i to mean --no-inherit, and for !
inherit create a per-task counter instead of a per-task-per-cpu counter.

Re: [RFC] perf: perf record sets inherit by default

[Stephane Eranian]

On Tue, May 11, 2010 at 5:13 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, 2010-05-11 at 12:00 -0300, Arnaldo Carvalho de Melo wrote:
>>
>> Humm, since for -C and -a using -i doesn't make sense, I guess it should
>> be off by default and only be auto-activated if we don't specify any
>> option, i.e. when using it like:
>>
>> perf record ./hackbench
>>
>> What do you think?
>
> -ENOPARSE
>
> -a/-C usage creates per-cpu counters and will thus ignore any and all
> perf_event_attr::inherit state.
>
> Your above suggestion would still have inherit enabled by default, and
> would thus not change anything.
>
> The thing is that perf-record defaults to inherited per-task-per-cpu
> counters, which, I think, is a reasonable default, just sub-optimal for
> single threaded/!forking subjects.
>
> So what would make sense is for -i to mean --no-inherit, and for !
> inherit create a per-task counter instead of a per-task-per-cpu counter.
>

If inherit is the preferred default, then yes, the solution is to revert
the meaning of -i. You'd have to be consistent across all commands
using -i for inherit such as perf stat.

Re: [RFC] perf: perf record sets inherit by default

[Arnaldo Carvalho de Melo]

Em Tue, May 11, 2010 at 05:13:20PM +0200, Peter Zijlstra escreveu:
> On Tue, 2010-05-11 at 12:00 -0300, Arnaldo Carvalho de Melo wrote:
> > Humm, since for -C and -a using -i doesn't make sense, I guess it should
> > be off by default and only be auto-activated if we don't specify any
> > option, i.e. when using it like:

> > perf record ./hackbench
 
> -ENOPARSE
 
> -a/-C usage creates per-cpu counters and will thus ignore any and all
> perf_event_attr::inherit state.

What I tried to say was that if one does:

perf record -t 1234

then inherit would be off, or if:

perf record -p 5678

it would also be off.

but when just pass some program to run, like in:

perf record make -j allmodconfig

we would then assume that the user is interested in everything that the
program perf is starting does, i.e. the user is interested in the whole
workload started from perf, thus we would auto-enable -i.

> Your above suggestion would still have inherit enabled by default, and
> would thus not change anything.

Nope, see above.
 
- Arnaldo

Re: [RFC] perf: perf record sets inherit by default

[Peter Zijlstra]

On Tue, 2010-05-11 at 12:50 -0300, Arnaldo Carvalho de Melo wrote:

> Nope, see above. 

Ah, -p/-t might make sense to default to inherit off indeed.

Re: [RFC] perf: perf record sets inherit by default

[Stephane Eranian]

On Tue, May 11, 2010 at 5:52 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, 2010-05-11 at 12:50 -0300, Arnaldo Carvalho de Melo wrote:
>
>> Nope, see above.
>
> Ah, -p/-t might make sense to default to inherit off indeed.
>
>
I would rather think that -p would inherit but not -t.
If I attach to a multi-threaded app with -p, I'd like all threads monitored.
But if I use -t to point to a specific thread within a process, then most
likely I care only about that thread.




-- 
Stephane Eranian  | EMEA Software Engineering
Google France | 38 avenue de l'Opéra | 75002 Paris
Tel : +33 (0) 1 42 68 53 00
This email may be confidential or privileged. If you received this
communication by mistake, please
don't forward it to anyone else, please erase all copies and
attachments, and please let me know that
it went to the wrong person. Thanks

Re: [RFC] perf: perf record sets inherit by default

[Arnaldo Carvalho de Melo]

Em Tue, May 11, 2010 at 05:55:44PM +0200, Stephane Eranian escreveu:
> On Tue, May 11, 2010 at 5:52 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Tue, 2010-05-11 at 12:50 -0300, Arnaldo Carvalho de Melo wrote:
> >
> >> Nope, see above.
> >
> > Ah, -p/-t might make sense to default to inherit off indeed.
> >
> >
> I would rather think that -p would inherit but not -t.
> If I attach to a multi-threaded app with -p, I'd like all threads monitored.
> But if I use -t to point to a specific thread within a process, then most
> likely I care only about that thread.

Ok, so fine tuning of the inherit flag is needed, i.e. we need both a
--inherit and a --no-inherit, well, good thing is that I think that the
option parsing stuff we, erm, inherited from git does just that, lemme
check how it works in this case more precisely...

- Arnaldo

Re: [RFC] perf: perf record sets inherit by default

[Arnaldo Carvalho de Melo]

Em Tue, May 11, 2010 at 05:52:11PM +0200, Peter Zijlstra escreveu:
> On Tue, 2010-05-11 at 12:50 -0300, Arnaldo Carvalho de Melo wrote:
> 
> > Nope, see above. 
> 
> Ah, -p/-t might make sense to default to inherit off indeed.

That is what Stephane wanted in the first place, right?

- Arnaldo

Re: [RFC] perf: perf record sets inherit by default

[Stephane Eranian]

On Tue, May 11, 2010 at 5:56 PM, Arnaldo Carvalho de Melo
<acme@infradead.org> wrote:
> Em Tue, May 11, 2010 at 05:52:11PM +0200, Peter Zijlstra escreveu:
>> On Tue, 2010-05-11 at 12:50 -0300, Arnaldo Carvalho de Melo wrote:
>>
>> > Nope, see above.
>>
>> Ah, -p/-t might make sense to default to inherit off indeed.
>
> That is what Stephane wanted in the first place, right?
>
I was asking about the general behavior of the tool and
the contradictory meaning of the -i options.

perf record foo       -> default=inherit on, use -i to disable
perf record -t 1234  -> default=inherit off
perf record -p 1234  -> default=on, use -i to disable

What about that?