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

[Adrian Hunter <adrian.hunter@intel.com>]

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