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;
>>>> }
>>>>
>>>> /*
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
