Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752836AbbKKUof (ORCPT ); Wed, 11 Nov 2015 15:44:35 -0500 Received: from mail.kernel.org ([198.145.29.136]:54627 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752187AbbKKUod (ORCPT ); Wed, 11 Nov 2015 15:44:33 -0500 Date: Wed, 11 Nov 2015 17:44:27 -0300 From: Arnaldo Carvalho de Melo To: "Wangnan (F)" Cc: Adrian Hunter , namhyung@kernel.org, lizefan@huawei.com, pi3orama@163.com, linux-kernel@vger.kernel.org, jolsa@kernel.org, masami.hiramatsu.pt@hitachi.com Subject: Re: [PATCH] perf symbols/KCORE: Rebuild rbtree when adjusting symbols for kcore Message-ID: <20151111204427.GA12579@kernel.org> References: <1446803172-83107-1-git-send-email-wangnan0@huawei.com> <20151106131950.GA13236@kernel.org> <563CB241.2090701@intel.com> <5642E80B.4050101@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5642E80B.4050101@huawei.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7121 Lines: 207 Em Wed, Nov 11, 2015 at 03:02:35PM +0800, Wangnan (F) escreveu: > > > On 2015/11/6 21:59, Adrian Hunter wrote: > >On 06/11/15 15:19, Arnaldo Carvalho de Melo wrote: > >>Em Fri, Nov 06, 2015 at 09:46:12AM +0000, Wang Nan escreveu: > >>>In dso__split_kallsyms_for_kcore(), current code adjusts symbol's > >>>address but only reinsert it into rbtree if the symbol belongs to > >>>another map. However, the expression for adjusting symbol (pos->start -= > >>>curr_map->start - curr_map->pgoff) can change the relative order between > >>>two symbols (even if the affected symbols are in different maps, in > >>>kcore case they are possible to share one same dso), which damages the > >>>rbtree. > >>Right, some code does change the symbol values it gets from whatever > >>symtab (kallsyms, ELF, JIT maps, etc) when it should instead use the per > >>map data structure (struct map) and its ->{map,unmap}_ip, ->pgoff, > >>->reloc, members for that :-\ > >> > >>I.e. 'struct dso' should be just what comes from the symtab, while > >>'struct map' should be about where that DSO is in memory. > >> > >>With that in mind, do you still think your fix is the correct one? > >> > >>Adrian? > >The problem is when the order in memory (in kallsyms) is different > >to the order on the dso (kcore). > > > >I think to make it more general it needs to insert to a new tree. > >e.g. > > > > I have tested this patch and it works for me. > > Thank you. Adrian, I took your explanation as the commit log, adding your S-o-B, so far not provided, is that ok with you, can I have your S-o-B? >From 500fe7dbd2c6cebc3638196352439490e1e3a8a4 Mon Sep 17 00:00:00 2001 From: Adrian Hunter Date: Fri, 6 Nov 2015 15:59:29 +0200 Subject: [PATCH 1/1] perf symbols: Rebuild rbtree when adjusting symbols for kcore Normally symbols are read from the DSO and adjusted, if need be, so that the symbol start matches the file offset in the DSO file (we want the file offset because that is what we know from MMAP events). That is done by dso__load_sym() which inserts the symbols *after* adjusting them. In the case of kcore, the symbols have been read from kallsyms and the symbol start is the memory address. The symbols have to be adjusted to match the kcore file offsets. dso__split_kallsyms_for_kcore() does that, but now the adjustment is being done *after* the symbols have been inserted. It appears dso__split_kallsyms_for_kcore() was assuming that changing the symbol start would not change the order in the rbtree - which is, of course, not guaranteed. Signed-off-by: Adrian Hunter Tested-by: Wang Nan Cc: Jiri Olsa Cc: Masami Hiramatsu Cc: Namhyung Kim Cc: Zefan Li Cc: pi3orama@163.com Link: http://lkml.kernel.org/r/563CB241.2090701@intel.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/symbol.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c index b4cc7662677e..09343a880c0b 100644 --- a/tools/perf/util/symbol.c +++ b/tools/perf/util/symbol.c @@ -654,19 +654,24 @@ static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map, struct map_groups *kmaps = map__kmaps(map); struct map *curr_map; struct symbol *pos; - int count = 0, moved = 0; + int count = 0; + struct rb_root old_root = dso->symbols[map->type]; struct rb_root *root = &dso->symbols[map->type]; struct rb_node *next = rb_first(root); if (!kmaps) return -1; + *root = RB_ROOT; + while (next) { char *module; pos = rb_entry(next, struct symbol, rb_node); next = rb_next(&pos->rb_node); + rb_erase_init(&pos->rb_node, &old_root); + module = strchr(pos->name, '\t'); if (module) *module = '\0'; @@ -674,28 +679,21 @@ static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map, curr_map = map_groups__find(kmaps, map->type, pos->start); if (!curr_map || (filter && filter(curr_map, pos))) { - rb_erase_init(&pos->rb_node, root); symbol__delete(pos); - } else { - pos->start -= curr_map->start - curr_map->pgoff; - if (pos->end) - pos->end -= curr_map->start - curr_map->pgoff; - if (curr_map->dso != map->dso) { - rb_erase_init(&pos->rb_node, root); - symbols__insert( - &curr_map->dso->symbols[curr_map->type], - pos); - ++moved; - } else { - ++count; - } + continue; } + + pos->start -= curr_map->start - curr_map->pgoff; + if (pos->end) + pos->end -= curr_map->start - curr_map->pgoff; + symbols__insert(&curr_map->dso->symbols[curr_map->type], pos); + ++count; } /* Symbols have been adjusted */ dso->adjust_symbols = 1; - return count + moved; + return count; } /* -- 2.1.0 > >diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c > >index b4cc7662677e..09343a880c0b 100644 > >--- a/tools/perf/util/symbol.c > >+++ b/tools/perf/util/symbol.c > >@@ -654,19 +654,24 @@ static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map, > > struct map_groups *kmaps = map__kmaps(map); > > struct map *curr_map; > > struct symbol *pos; > >- int count = 0, moved = 0; > >+ int count = 0; > >+ struct rb_root old_root = dso->symbols[map->type]; > > struct rb_root *root = &dso->symbols[map->type]; > > struct rb_node *next = rb_first(root); > > if (!kmaps) > > return -1; > >+ *root = RB_ROOT; > >+ > > while (next) { > > char *module; > > pos = rb_entry(next, struct symbol, rb_node); > > next = rb_next(&pos->rb_node); > >+ rb_erase_init(&pos->rb_node, &old_root); > >+ > > module = strchr(pos->name, '\t'); > > if (module) > > *module = '\0'; > >@@ -674,28 +679,21 @@ static int dso__split_kallsyms_for_kcore(struct dso *dso, struct map *map, > > curr_map = map_groups__find(kmaps, map->type, pos->start); > > if (!curr_map || (filter && filter(curr_map, pos))) { > >- rb_erase_init(&pos->rb_node, root); > > symbol__delete(pos); > >- } else { > >- pos->start -= curr_map->start - curr_map->pgoff; > >- if (pos->end) > >- pos->end -= curr_map->start - curr_map->pgoff; > >- if (curr_map->dso != map->dso) { > >- rb_erase_init(&pos->rb_node, root); > >- symbols__insert( > >- &curr_map->dso->symbols[curr_map->type], > >- pos); > >- ++moved; > >- } else { > >- ++count; > >- } > >+ continue; > > } > >+ > >+ pos->start -= curr_map->start - curr_map->pgoff; > >+ if (pos->end) > >+ pos->end -= curr_map->start - curr_map->pgoff; > >+ symbols__insert(&curr_map->dso->symbols[curr_map->type], pos); > >+ ++count; > > } > > /* Symbols have been adjusted */ > > dso->adjust_symbols = 1; > >- return count + moved; > >+ return count; > > } > > /* > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/