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 <[email protected]>
---
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
* Luigi Semenzato <[email protected]> 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
On Wed, Aug 22, 2012 at 12:28 AM, Ingo Molnar <[email protected]> wrote:
>
> * Luigi Semenzato <[email protected]> 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).
Em Wed, Aug 22, 2012 at 09:09:10AM -0700, Luigi Semenzato escreveu:
> On Wed, Aug 22, 2012 at 12:28 AM, Ingo Molnar <[email protected]> wrote:
> > * Luigi Semenzato <[email protected]> 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
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
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
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
<[email protected]> 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
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
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 <[email protected]>
> ---
> 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 <[email protected]>
Commit-ID: 9fdbf671ba7e8adb2cbb93d716232ebcab55f6bd
Gitweb: http://git.kernel.org/tip/9fdbf671ba7e8adb2cbb93d716232ebcab55f6bd
Author: Luigi Semenzato <[email protected]>
AuthorDate: Tue, 21 Aug 2012 14:52:20 -0700
Committer: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Tested-by: Stephane Eranian <[email protected]>
Acked-by: Stephane Eranian <[email protected]>
Cc: "Eric W. Biederman" <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Lucas De Marchi <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Olof Johansson <[email protected]>
Cc: Paul Gortmaker <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Robert Richter <[email protected]>
Cc: Sonny Rao <[email protected]>
Cc: Stephane Eranian <[email protected]>
Cc: Stephen Wilson <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Vasiliy Kulikov <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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;
}