Date: Fri, 6 Nov 2015 15:51:04 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Adrian Hunter <adrian.hunter@intel.com>
Cc: Wang Nan <wangnan0@huawei.com>, namhyung@kernel.org, lizefan@huawei.com,
        pi3orama@163.com, linux-kernel@vger.kernel.org, jolsa@kernel.org,
        masami.hiramatsu.pt@hitachi.com
Subject: Re: [PATCH] perf symbols/KCORE: Rebuild rbtree when adjusting
 symbols for kcore
Message-ID: <20151106185104.GF13236@kernel.org>
References: <1446803172-83107-1-git-send-email-wangnan0@huawei.com>
 <20151106131950.GA13236@kernel.org>
 <563CB241.2090701@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <563CB241.2090701@intel.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3923
Lines: 114

Em Fri, Nov 06, 2015 at 03:59:29PM +0200, Adrian Hunter escreveu:
> On 06/11/15 15:19, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Nov 06, 2015 at 09:46:12AM +0000, Wang Nan escreveu:
> >> In dso__split_kallsyms_for_kcore(), current code adjusts symbol's
> >> address but only reinsert it into rbtree if the symbol belongs to
> >> another map. However, the expression for adjusting symbol (pos->start -=
> >> curr_map->start - curr_map->pgoff) can change the relative order between
> >> two symbols (even if the affected symbols are in different maps, in
> >> kcore case they are possible to share one same dso), which damages the
> >> rbtree.
> > 
> > Right, some code does change the symbol values it gets from whatever
> > symtab (kallsyms, ELF, JIT maps, etc) when it should instead use the per
> > map data structure (struct map) and its ->{map,unmap}_ip, ->pgoff,
> > ->reloc, members for that :-\
> > 
> > I.e. 'struct dso' should be just what comes from the symtab, while
> > 'struct map' should be about where that DSO is in memory.
> > 
> > With that in mind, do you still think your fix is the correct one?
> > 
> > Adrian?
> 
> 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? 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/