Messages in this thread | | | Subject | Re: [PATCH] perf symbols/KCORE: Rebuild rbtree when adjusting symbols for kcore | From | Adrian Hunter <> | Date | Tue, 10 Nov 2015 15:41:15 +0200 |
| |
On 09/11/15 16:56, Arnaldo Carvalho de Melo wrote: > Em Mon, Nov 09, 2015 at 10:26:13AM +0200, Adrian Hunter escreveu: >> On 06/11/15 20:51, Arnaldo Carvalho de Melo wrote: >>> Em Fri, Nov 06, 2015 at 03:59:29PM +0200, Adrian Hunter escreveu: >>>> The problem is when the order in memory (in kallsyms) is different >>>> to the order on the dso (kcore). > >>> What order? Can you ellaborate a bit more? > >> 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 > > So you're saying that some symbols get adjusted, by say X bytes, while > some other symbols are adjusted by a different, Y value, or are _all_ > the symbols adjusted by the same value, i.e. one that could be adjusted > in 'struct map' instead?
Yes X != Y. The maps are correct - just the symbols need adjusting.
> >> 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. > > Sure, the minimal fix should be not to change the key (sym->start/end) > after you add it to an rbtree that uses that key. > >>> I thought more about keeping >>> whatever address is in the symtab from where we read the symbols, and >>> then create one map per kernel module all pointing to the same DSO, that >>> would be the one loaded from kallsyms. >>> >>> Any adjustments would be fone in the map, not the DSO. >>> >>> I.e. we wouldn't be splitting anything, just creating struct map >>> instances pointing to the same DSO. >>> >>> - Arnaldo >>> >>>> I think to make it more general it needs to insert to a new tree. >>>> e.g. >>> >>> >>>> 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; >>>> } >>>> >>>> /* >>> >
| |