Re: [PATCH] perf symbols/KCORE: Rebuild rbtree when adjusting symbols for kcore

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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?

> 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;
> >>  }
> >>  
> >>  /*
> >