[PATCH] perf: do not flush maps on COMM for perf report

[PATCH] perf: do not flush maps on COMM for perf report

[Luigi Semenzato]

This fixes a long-standing bug caused by the lack of separate
COMM and EXEC record types, which makes "perf report" lose
track of symbols when a process renames itself.

With this fix (suggested by Stephane Eranian), a COMM (rename)
no longer flushes the maps, which is the correct behavior.
An EXEC also no longer flushes the maps, but this doesn't
matter because as new mappings are created (for the executable
and the libraries) the old mappings are automatically removed.
This is not by accident: the functionality is necessary because
DLLs can be explicitly loaded at any time with dlopen(),
possibly on top of existing text, so "perf report" handles
correctly the clobbering of new mappings on top of old ones.

An alternative patch (which I proposed earlier) would be to
introduce a separate PERF_RECORD_EXEC type, but it is a much
larger change (about 300 lines) and is not necessary.

Signed-off-by: Luigi Semenzato <semenzato@chromium.org>
---
 tools/perf/util/thread.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index fb4b7ea..8b3e593 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -39,7 +39,6 @@ int thread__set_comm(struct thread *self, const char *comm)
 	err = self->comm == NULL ? -ENOMEM : 0;
 	if (!err) {
 		self->comm_set = true;
-		map_groups__flush(&self->mg);
 	}
 	return err;
 }
-- 
1.7.7.3

Re: [PATCH] perf: do not flush maps on COMM for perf report

[Ingo Molnar]

* Luigi Semenzato <semenzato@chromium.org> wrote:

> This fixes a long-standing bug caused by the lack of separate
> COMM and EXEC record types, which makes "perf report" lose
> track of symbols when a process renames itself.
> 
> With this fix (suggested by Stephane Eranian), a COMM (rename)
> no longer flushes the maps, which is the correct behavior.
> An EXEC also no longer flushes the maps, but this doesn't
> matter because as new mappings are created (for the executable
> and the libraries) the old mappings are automatically removed.
> This is not by accident: the functionality is necessary because
> DLLs can be explicitly loaded at any time with dlopen(),
> possibly on top of existing text, so "perf report" handles
> correctly the clobbering of new mappings on top of old ones.
> 
> An alternative patch (which I proposed earlier) would be to
> introduce a separate PERF_RECORD_EXEC type, but it is a much
> larger change (about 300 lines) and is not necessary.

It would be nice to add that too - we already have FORK/EXIT, 
this seems like a natural extension.

Thanks,

	Ingo

Re: [PATCH] perf: do not flush maps on COMM for perf report

[Luigi Semenzato]

On Wed, Aug 22, 2012 at 12:28 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Luigi Semenzato <semenzato@chromium.org> wrote:
>
>> This fixes a long-standing bug caused by the lack of separate
>> COMM and EXEC record types, which makes "perf report" lose
>> track of symbols when a process renames itself.
>>
>> With this fix (suggested by Stephane Eranian), a COMM (rename)
>> no longer flushes the maps, which is the correct behavior.
>> An EXEC also no longer flushes the maps, but this doesn't
>> matter because as new mappings are created (for the executable
>> and the libraries) the old mappings are automatically removed.
>> This is not by accident: the functionality is necessary because
>> DLLs can be explicitly loaded at any time with dlopen(),
>> possibly on top of existing text, so "perf report" handles
>> correctly the clobbering of new mappings on top of old ones.
>>
>> An alternative patch (which I proposed earlier) would be to
>> introduce a separate PERF_RECORD_EXEC type, but it is a much
>> larger change (about 300 lines) and is not necessary.
>
> It would be nice to add that too - we already have FORK/EXIT,
> this seems like a natural extension.

Yes.  Adding PERF_RECORD_EXEC is/would be the right long-term
solution.  But there are two issues.

1. One nice aspect of perf is that perf.data files and "perf report"
are compatible across a large number of versions.  Adding
PERF_RECORD_EXEC breaks compatibility in a somewhat unpleasant manner.
 New perf.data files won't work with old versions of perf and *might*
fail poorly (segv) although this situation is difficult to analyze.

2. Adding a new record type is messy.  It replicates a lot of
boilerplate code, much of it in the kernel, and affects many parts of
the system.  It adds to size, complexity, and likelihood of new bugs.

I would prioritize the "would be nice" category as follows.

1. Improve the handling of unknown record types for future better
backward compatibility.  (Small change.)

2. Refactor/cleanup code to improve readability and robustness.  (Big
change, but can be broken into many smaller pieces.)

3. Add PERF_RECORD_EXEC.

If there is consensus, I might be able to give a shot to 1 and 2
(courtesy of Google).

Re: [PATCH] perf: do not flush maps on COMM for perf report

[Arnaldo Carvalho de Melo]

Em Wed, Aug 22, 2012 at 09:09:10AM -0700, Luigi Semenzato escreveu:
> On Wed, Aug 22, 2012 at 12:28 AM, Ingo Molnar <mingo@kernel.org> wrote:
> > * Luigi Semenzato <semenzato@chromium.org> wrote:
> >> An alternative patch (which I proposed earlier) would be to
> >> introduce a separate PERF_RECORD_EXEC type, but it is a much
> >> larger change (about 300 lines) and is not necessary.

> > It would be nice to add that too - we already have FORK/EXIT,
> > this seems like a natural extension.

> Yes.  Adding PERF_RECORD_EXEC is/would be the right long-term
> solution.  But there are two issues.

> 1. One nice aspect of perf is that perf.data files and "perf report"
> are compatible across a large number of versions.  Adding
> PERF_RECORD_EXEC breaks compatibility in a somewhat unpleasant manner.
>  New perf.data files won't work with old versions of perf and *might*
> fail poorly (segv) although this situation is difficult to analyze.

> 2. Adding a new record type is messy.  It replicates a lot of
> boilerplate code, much of it in the kernel, and affects many parts of
> the system.  It adds to size, complexity, and likelihood of new bugs.

> I would prioritize the "would be nice" category as follows.

> 1. Improve the handling of unknown record types for future better
> backward compatibility.  (Small change.)

> 2. Refactor/cleanup code to improve readability and robustness.  (Big
> change, but can be broken into many smaller pieces.)

> 3. Add PERF_RECORD_EXEC.

> If there is consensus, I might be able to give a shot to 1 and 2
> (courtesy of Google).

I think this is just common sense, i.e. I'd love to take patches that
improve the robustness of the tools any day.

Refactoring code to make it cleaner and possibly faster and/or smaller,
ditto.

Adding the EXEC event, ditto. And I agree that while adding it we want
to do 1/2 as pre-requisite.

- Arnaldo

Re: [PATCH] perf: do not flush maps on COMM for perf report

[David Ahern]

On 8/22/12 10:29 AM, Arnaldo Carvalho de Melo wrote:
> Adding the EXEC event, ditto. And I agree that while adding it we want
> to do 1/2 as pre-requisite.

maps should not be flushed on a COMM event, so that was a mistake. Given 
that what new information does an EXEC event provide? Same process id. A 
COMM event is generated on an exec, so the name change happens. Mappings 
are dropped prior to that - and there is no unmap event. That seems to 
be a missing piece. Maps are added which is handled by MMAP events. 
After that why is an exec event relevant?

David

Re: [PATCH] perf: do not flush maps on COMM for perf report

[Arnaldo Carvalho de Melo]

Em Wed, Aug 22, 2012 at 10:56:13AM -0600, David Ahern escreveu:
> On 8/22/12 10:29 AM, Arnaldo Carvalho de Melo wrote:
> >Adding the EXEC event, ditto. And I agree that while adding it we want
> >to do 1/2 as pre-requisite.
> 
> maps should not be flushed on a COMM event, so that was a mistake.
> Given that what new information does an EXEC event provide? Same
> process id. A COMM event is generated on an exec, so the name change
> happens. Mappings are dropped prior to that - and there is no unmap
> event. That seems to be a missing piece. Maps are added which is
> handled by MMAP events. After that why is an exec event relevant?

Please read the original discussion about it:

https://lkml.org/lkml/2012/2/13/545

- Arnaldo

Re: [PATCH] perf: do not flush maps on COMM for perf report

[Luigi Semenzato]

There is also this (incomplete, untested) patch, which shows what it
takes to add the new record type.

https://lkml.org/lkml/2012/3/2/345

On Wed, Aug 22, 2012 at 11:16 AM, Arnaldo Carvalho de Melo
<acme@ghostprotocols.net> wrote:
> Em Wed, Aug 22, 2012 at 10:56:13AM -0600, David Ahern escreveu:
>> On 8/22/12 10:29 AM, Arnaldo Carvalho de Melo wrote:
>> >Adding the EXEC event, ditto. And I agree that while adding it we want
>> >to do 1/2 as pre-requisite.
>>
>> maps should not be flushed on a COMM event, so that was a mistake.
>> Given that what new information does an EXEC event provide? Same
>> process id. A COMM event is generated on an exec, so the name change
>> happens. Mappings are dropped prior to that - and there is no unmap
>> event. That seems to be a missing piece. Maps are added which is
>> handled by MMAP events. After that why is an exec event relevant?
>
> Please read the original discussion about it:
>
> https://lkml.org/lkml/2012/2/13/545
>
> - Arnaldo

Re: [PATCH] perf: do not flush maps on COMM for perf report

[David Ahern]

On 8/22/12 12:16 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 22, 2012 at 10:56:13AM -0600, David Ahern escreveu:
>> On 8/22/12 10:29 AM, Arnaldo Carvalho de Melo wrote:
>>> Adding the EXEC event, ditto. And I agree that while adding it we want
>>> to do 1/2 as pre-requisite.
>>
>> maps should not be flushed on a COMM event, so that was a mistake.
>> Given that what new information does an EXEC event provide? Same
>> process id. A COMM event is generated on an exec, so the name change
>> happens. Mappings are dropped prior to that - and there is no unmap
>> event. That seems to be a missing piece. Maps are added which is
>> handled by MMAP events. After that why is an exec event relevant?
>
> Please read the original discussion about it:
>
> https://lkml.org/lkml/2012/2/13/545

I do recall that discussion (and re-read just now). If maps are not 
flushed on a rename (and they should not be), then I still do not 
understand what information is conveyed by an exec event? It's not a new 
process or new map and the name change is already handled by a comm event.

David

Re: [PATCH] perf: do not flush maps on COMM for perf report

[David Ahern]

On 8/21/12 3:52 PM, Luigi Semenzato wrote:
> This fixes a long-standing bug caused by the lack of separate
> COMM and EXEC record types, which makes "perf report" lose
> track of symbols when a process renames itself.
>
> With this fix (suggested by Stephane Eranian), a COMM (rename)
> no longer flushes the maps, which is the correct behavior.
> An EXEC also no longer flushes the maps, but this doesn't
> matter because as new mappings are created (for the executable
> and the libraries) the old mappings are automatically removed.
> This is not by accident: the functionality is necessary because
> DLLs can be explicitly loaded at any time with dlopen(),
> possibly on top of existing text, so "perf report" handles
> correctly the clobbering of new mappings on top of old ones.
>
> An alternative patch (which I proposed earlier) would be to
> introduce a separate PERF_RECORD_EXEC type, but it is a much
> larger change (about 300 lines) and is not necessary.
>
> Signed-off-by: Luigi Semenzato <semenzato@chromium.org>
> ---
>   tools/perf/util/thread.c |    1 -
>   1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index fb4b7ea..8b3e593 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -39,7 +39,6 @@ int thread__set_comm(struct thread *self, const char *comm)
>   	err = self->comm == NULL ? -ENOMEM : 0;
>   	if (!err) {
>   		self->comm_set = true;
> -		map_groups__flush(&self->mg);
>   	}
>   	return err;
>   }
>

Acked-by: David Ahern <dsahern@gmail.com>

[tip:perf/urgent] perf tools: do not flush maps on COMM for perf report

[tip-bot for Luigi Semenzato]

Commit-ID:  9fdbf671ba7e8adb2cbb93d716232ebcab55f6bd
Gitweb:     http://git.kernel.org/tip/9fdbf671ba7e8adb2cbb93d716232ebcab55f6bd
Author:     Luigi Semenzato <semenzato@chromium.org>
AuthorDate: Tue, 21 Aug 2012 14:52:20 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 22 Oct 2012 13:55:53 -0200

perf tools: do not flush maps on COMM for perf report

This fixes a long-standing bug caused by the lack of separate COMM and EXEC
record types, which makes "perf report" lose track of symbols when a process
renames itself.

With this fix (suggested by Stephane Eranian), a COMM (rename) no longer
flushes the maps, which is the correct behavior.  An EXEC also no longer
flushes the maps, but this doesn't matter because as new mappings are created
(for the executable and the libraries) the old mappings are automatically
removed.  This is not by accident: the functionality is necessary because DLLs
can be explicitly loaded at any time with dlopen(), possibly on top of existing
text, so "perf report" handles correctly the clobbering of new mappings on top
of old ones.

An alternative patch (which I proposed earlier) would be to introduce a
separate PERF_RECORD_EXEC type, but it is a much larger change (about 300
lines) and is not necessary.

Signed-off-by: Luigi Semenzato <semenzato@chromium.org>
Tested-by: Stephane Eranian <eranian@google.com>
Acked-by: Stephane Eranian <eranian@google.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Lucas De Marchi <lucas.demarchi@profusion.mobi>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Olof Johansson <olofj@chromium.org>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Robert Richter <robert.richter@amd.com>
Cc: Sonny Rao <sonnyrao@chromium.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Stephen Wilson <wilsons@start.ca>
Cc: Tejun Heo <tj@kernel.org>
Cc: Vasiliy Kulikov <segoon@openwall.com>
Link: http://lkml.kernel.org/r/1345585940-6497-1-git-send-email-semenzato@chromium.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/thread.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index fb4b7ea..8b3e593 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -39,7 +39,6 @@ int thread__set_comm(struct thread *self, const char *comm)
 	err = self->comm == NULL ? -ENOMEM : 0;
 	if (!err) {
 		self->comm_set = true;
-		map_groups__flush(&self->mg);
 	}
 	return err;
 }