* [PATCH 1/3] perf session: Free the ref_reloc_sym memory at the right place
2010-08-03 2:09 [GIT PULL 0/3] perf/core fixes and improvements Arnaldo Carvalho de Melo
@ 2010-08-03 2:09 ` Arnaldo Carvalho de Melo
2010-08-03 2:09 ` [PATCH 2/3] perf session: Invalidate last_match when removing threads from rb_tree Arnaldo Carvalho de Melo
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2010-08-03 2:09 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, Arnaldo Carvalho de Melo, Frederic Weisbecker,
Mike Galbraith, Peter Zijlstra, Stephane Eranian
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Which is at perf_session__destroy_kernel_maps, counterpart to the
perf_session__create_kernel_maps where the kmap structure is located, just
after the vmlinux_maps.
Make it also check if the kernel maps were actually created, which may not
be the case if, for instance, perf_session__new can't complete due to
permission problems in, for instance, a 'perf report' case, when a
segfault will take place, that is how this was noticed.
The problem was introduced in d65a458, thus post .35.
This also adds code to release guest machines as them are also created
in perf_session__create_kernel_maps, so should be deleted on this newly
introduced counterpart, perf_session__destroy_kernel_maps.
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
LKML-Reference: <new-submission>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/map.c | 18 +++++++++++-------
tools/perf/util/map.h | 7 +++++++
tools/perf/util/session.c | 7 +++++++
tools/perf/util/symbol.c | 43 +++++++++++++++++++++++++++++++++++++++++++
tools/perf/util/symbol.h | 2 ++
5 files changed, 70 insertions(+), 7 deletions(-)
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 15d6a6d..801e696 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -506,6 +506,11 @@ void maps__insert(struct rb_root *maps, struct map *map)
rb_insert_color(&map->rb_node, maps);
}
+void maps__remove(struct rb_root *self, struct map *map)
+{
+ rb_erase(&map->rb_node, self);
+}
+
struct map *maps__find(struct rb_root *maps, u64 ip)
{
struct rb_node **p = &maps->rb_node;
@@ -551,13 +556,6 @@ static void dsos__delete(struct list_head *self)
void machine__exit(struct machine *self)
{
- struct kmap *kmap = map__kmap(self->vmlinux_maps[MAP__FUNCTION]);
-
- if (kmap->ref_reloc_sym) {
- free((char *)kmap->ref_reloc_sym->name);
- free(kmap->ref_reloc_sym);
- }
-
map_groups__exit(&self->kmaps);
dsos__delete(&self->user_dsos);
dsos__delete(&self->kernel_dsos);
@@ -565,6 +563,12 @@ void machine__exit(struct machine *self)
self->root_dir = NULL;
}
+void machine__delete(struct machine *self)
+{
+ machine__exit(self);
+ free(self);
+}
+
struct machine *machines__add(struct rb_root *self, pid_t pid,
const char *root_dir)
{
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 0e0984e..5b51bbd 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -125,6 +125,7 @@ void map__reloc_vmlinux(struct map *self);
size_t __map_groups__fprintf_maps(struct map_groups *self,
enum map_type type, int verbose, FILE *fp);
void maps__insert(struct rb_root *maps, struct map *map);
+void maps__remove(struct rb_root *self, struct map *map);
struct map *maps__find(struct rb_root *maps, u64 addr);
void map_groups__init(struct map_groups *self);
void map_groups__exit(struct map_groups *self);
@@ -144,6 +145,7 @@ struct machine *machines__findnew(struct rb_root *self, pid_t pid);
char *machine__mmap_name(struct machine *self, char *bf, size_t size);
int machine__init(struct machine *self, const char *root_dir, pid_t pid);
void machine__exit(struct machine *self);
+void machine__delete(struct machine *self);
/*
* Default guest kernel is defined by parameter --guestkallsyms
@@ -165,6 +167,11 @@ static inline void map_groups__insert(struct map_groups *self, struct map *map)
map->groups = self;
}
+static inline void map_groups__remove(struct map_groups *self, struct map *map)
+{
+ maps__remove(&self->maps[map->type], map);
+}
+
static inline struct map *map_groups__find(struct map_groups *self,
enum map_type type, u64 addr)
{
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 04a3b3d..5d2fd52 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -79,6 +79,12 @@ int perf_session__create_kernel_maps(struct perf_session *self)
return ret;
}
+static void perf_session__destroy_kernel_maps(struct perf_session *self)
+{
+ machine__destroy_kernel_maps(&self->host_machine);
+ machines__destroy_guest_kernel_maps(&self->machines);
+}
+
struct perf_session *perf_session__new(const char *filename, int mode, bool force, bool repipe)
{
size_t len = filename ? strlen(filename) + 1 : 0;
@@ -150,6 +156,7 @@ static void perf_session__delete_threads(struct perf_session *self)
void perf_session__delete(struct perf_session *self)
{
perf_header__exit(&self->header);
+ perf_session__destroy_kernel_maps(self);
perf_session__delete_dead_threads(self);
perf_session__delete_threads(self);
machine__exit(&self->host_machine);
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 3b8c005..6f0dd90 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2107,6 +2107,36 @@ int __machine__create_kernel_maps(struct machine *self, struct dso *kernel)
return 0;
}
+void machine__destroy_kernel_maps(struct machine *self)
+{
+ enum map_type type;
+
+ for (type = 0; type < MAP__NR_TYPES; ++type) {
+ struct kmap *kmap;
+
+ if (self->vmlinux_maps[type] == NULL)
+ continue;
+
+ kmap = map__kmap(self->vmlinux_maps[type]);
+ map_groups__remove(&self->kmaps, self->vmlinux_maps[type]);
+ if (kmap->ref_reloc_sym) {
+ /*
+ * ref_reloc_sym is shared among all maps, so free just
+ * on one of them.
+ */
+ if (type == MAP__FUNCTION) {
+ free((char *)kmap->ref_reloc_sym->name);
+ kmap->ref_reloc_sym->name = NULL;
+ free(kmap->ref_reloc_sym);
+ }
+ kmap->ref_reloc_sym = NULL;
+ }
+
+ map__delete(self->vmlinux_maps[type]);
+ self->vmlinux_maps[type] = NULL;
+ }
+}
+
int machine__create_kernel_maps(struct machine *self)
{
struct dso *kernel = machine__create_kernel(self);
@@ -2351,6 +2381,19 @@ failure:
return ret;
}
+void machines__destroy_guest_kernel_maps(struct rb_root *self)
+{
+ struct rb_node *next = rb_first(self);
+
+ while (next) {
+ struct machine *pos = rb_entry(next, struct machine, rb_node);
+
+ next = rb_next(&pos->rb_node);
+ rb_erase(&pos->rb_node, self);
+ machine__delete(pos);
+ }
+}
+
int machine__load_kallsyms(struct machine *self, const char *filename,
enum map_type type, symbol_filter_t filter)
{
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 33d53ce..906be20 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -212,11 +212,13 @@ int kallsyms__parse(const char *filename, void *arg,
int (*process_symbol)(void *arg, const char *name,
char type, u64 start));
+void machine__destroy_kernel_maps(struct machine *self);
int __machine__create_kernel_maps(struct machine *self, struct dso *kernel);
int machine__create_kernel_maps(struct machine *self);
int machines__create_kernel_maps(struct rb_root *self, pid_t pid);
int machines__create_guest_kernel_maps(struct rb_root *self);
+void machines__destroy_guest_kernel_maps(struct rb_root *self);
int symbol__init(void);
void symbol__exit(void);
--
1.6.2.5
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 2/3] perf session: Invalidate last_match when removing threads from rb_tree
2010-08-03 2:09 [GIT PULL 0/3] perf/core fixes and improvements Arnaldo Carvalho de Melo
2010-08-03 2:09 ` [PATCH 1/3] perf session: Free the ref_reloc_sym memory at the right place Arnaldo Carvalho de Melo
@ 2010-08-03 2:09 ` Arnaldo Carvalho de Melo
2010-08-03 2:09 ` [PATCH 3/3] perf tools: Don't keep unreferenced maps when unmaps are detected Arnaldo Carvalho de Melo
2010-08-03 5:44 ` [GIT PULL 0/3] perf/core fixes and improvements Ingo Molnar
3 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2010-08-03 2:09 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, Arnaldo Carvalho de Melo, Frederic Weisbecker,
Mike Galbraith, Peter Zijlstra, Stephane Eranian
From: Arnaldo Carvalho de Melo <acme@redhat.com>
If we receive two PERF_RECORD_EXIT for the same thread, we can end up
reusing session->last_match and trying to remove the thread twice from
the rb_tree, causing a segfault, so invalidade last_match in
perf_session__remove_thread.
Receiving two PERF_RECORD_EXIT for the same thread is a bug, but its a
harmless one if we make the tool more robust, like this patch does.
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
LKML-Reference: <new-submission>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/session.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 5d2fd52..fa9d652 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -166,6 +166,7 @@ void perf_session__delete(struct perf_session *self)
void perf_session__remove_thread(struct perf_session *self, struct thread *th)
{
+ self->last_match = NULL;
rb_erase(&th->rb_node, &self->threads);
/*
* We may have references to this thread, for instance in some hist_entry
--
1.6.2.5
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 3/3] perf tools: Don't keep unreferenced maps when unmaps are detected
2010-08-03 2:09 [GIT PULL 0/3] perf/core fixes and improvements Arnaldo Carvalho de Melo
2010-08-03 2:09 ` [PATCH 1/3] perf session: Free the ref_reloc_sym memory at the right place Arnaldo Carvalho de Melo
2010-08-03 2:09 ` [PATCH 2/3] perf session: Invalidate last_match when removing threads from rb_tree Arnaldo Carvalho de Melo
@ 2010-08-03 2:09 ` Arnaldo Carvalho de Melo
2010-08-03 5:44 ` [GIT PULL 0/3] perf/core fixes and improvements Ingo Molnar
3 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2010-08-03 2:09 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, Arnaldo Carvalho de Melo, Frederic Weisbecker,
Mike Galbraith, Peter Zijlstra, Stephane Eranian
From: Arnaldo Carvalho de Melo <acme@redhat.com>
For a file with:
[root@emilia linux-2.6-tip]# perf report -D -fi allmodconfig-j32.perf.data | grep events:
TOTAL events: 36933
MMAP events: 9056
LOST events: 0
COMM events: 1702
EXIT events: 1887
THROTTLE events: 8
UNTHROTTLE events: 8
FORK events: 1894
READ events: 0
SAMPLE events: 22378
ATTR events: 0
EVENT_TYPE events: 0
TRACING_DATA events: 0
BUILD_ID events: 0
[root@emilia linux-2.6-tip]#
Testing with valgrind and making perf_session__delete() a nop, so that
we can notice how many maps were actually deleted due to not having any
samples on it:
==== HEAP SUMMARY:
Before:
==10339== in use at exit: 8,909,997 bytes in 68,690 blocks
==10339== total heap usage: 78,696 allocs, 10,007 frees, 11,925,853 bytes allocated
After:
==10506== in use at exit: 8,902,605 bytes in 68,606 blocks
==10506== total heap usage: 78,696 allocs, 10,091 frees, 11,925,853 bytes allocated
I.e. just 84 detected unmaps with no hits out of 9056 for this workload,
not much, but in some other long running workload this may save more
bytes.
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
LKML-Reference: <new-submission>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/util/hist.c | 2 ++
tools/perf/util/map.c | 31 +++++++++++++++++++++----------
tools/perf/util/map.h | 3 ++-
3 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index a6cea28..e7263d4 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -93,6 +93,8 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template)
if (self != NULL) {
*self = *template;
self->nr_events = 1;
+ if (self->ms.map)
+ self->ms.map->referenced = true;
if (symbol_conf.use_callchain)
callchain_init(self->callchain);
}
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 801e696..3a7eb6e 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -29,6 +29,7 @@ void map__init(struct map *self, enum map_type type,
self->unmap_ip = map__unmap_ip;
RB_CLEAR_NODE(&self->rb_node);
self->groups = NULL;
+ self->referenced = false;
}
struct map *map__new(struct list_head *dsos__list, u64 start, u64 len,
@@ -387,6 +388,7 @@ int map_groups__fixup_overlappings(struct map_groups *self, struct map *map,
{
struct rb_root *root = &self->maps[map->type];
struct rb_node *next = rb_first(root);
+ int err = 0;
while (next) {
struct map *pos = rb_entry(next, struct map, rb_node);
@@ -403,20 +405,16 @@ int map_groups__fixup_overlappings(struct map_groups *self, struct map *map,
rb_erase(&pos->rb_node, root);
/*
- * We may have references to this map, for instance in some
- * hist_entry instances, so just move them to a separate
- * list.
- */
- list_add_tail(&pos->node, &self->removed_maps[map->type]);
- /*
* Now check if we need to create new maps for areas not
* overlapped by the new map:
*/
if (map->start > pos->start) {
struct map *before = map__clone(pos);
- if (before == NULL)
- return -ENOMEM;
+ if (before == NULL) {
+ err = -ENOMEM;
+ goto move_map;
+ }
before->end = map->start - 1;
map_groups__insert(self, before);
@@ -427,14 +425,27 @@ int map_groups__fixup_overlappings(struct map_groups *self, struct map *map,
if (map->end < pos->end) {
struct map *after = map__clone(pos);
- if (after == NULL)
- return -ENOMEM;
+ if (after == NULL) {
+ err = -ENOMEM;
+ goto move_map;
+ }
after->start = map->end + 1;
map_groups__insert(self, after);
if (verbose >= 2)
map__fprintf(after, fp);
}
+move_map:
+ /*
+ * If we have references, just move them to a separate list.
+ */
+ if (pos->referenced)
+ list_add_tail(&pos->node, &self->removed_maps[map->type]);
+ else
+ map__delete(pos);
+
+ if (err)
+ return err;
}
return 0;
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 5b51bbd..7857579 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -29,7 +29,8 @@ struct map {
};
u64 start;
u64 end;
- enum map_type type;
+ u8 /* enum map_type */ type;
+ bool referenced;
u32 priv;
u64 pgoff;
--
1.6.2.5
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [GIT PULL 0/3] perf/core fixes and improvements
2010-08-03 2:09 [GIT PULL 0/3] perf/core fixes and improvements Arnaldo Carvalho de Melo
` (2 preceding siblings ...)
2010-08-03 2:09 ` [PATCH 3/3] perf tools: Don't keep unreferenced maps when unmaps are detected Arnaldo Carvalho de Melo
@ 2010-08-03 5:44 ` Ingo Molnar
3 siblings, 0 replies; 5+ messages in thread
From: Ingo Molnar @ 2010-08-03 5:44 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: linux-kernel, Frederic Weisbecker, Mike Galbraith,
Peter Zijlstra, Stephane Eranian
* Arnaldo Carvalho de Melo <acme@infradead.org> wrote:
> Hi Ingo,
>
> Please pull from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux-2.6 perf/core
>
> Regards,
>
> - Arnaldo
>
> Arnaldo Carvalho de Melo (3):
> perf session: Free the ref_reloc_sym memory at the right place
> perf session: Invalidate last_match when removing threads from rb_tree
> perf tools: Don't keep unreferenced maps when unmaps are detected
>
> tools/perf/util/hist.c | 2 +
> tools/perf/util/map.c | 49 +++++++++++++++++++++++++++++---------------
> tools/perf/util/map.h | 10 ++++++++-
> tools/perf/util/session.c | 8 +++++++
> tools/perf/util/symbol.c | 43 +++++++++++++++++++++++++++++++++++++++
> tools/perf/util/symbol.h | 2 +
> 6 files changed, 96 insertions(+), 18 deletions(-)
Pulled, thanks Arnaldo!
Ingo
^ permalink raw reply [flat|nested] 5+ messages in thread