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

From: Arnaldo Carvalho de Melo
Date: Mon Nov 09 2015 - 09:57:00 EST


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