Content-Type: text/plain;
	charset=gb2312
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH] perf symbols/KCORE: Rebuild rbtree when adjusting symbols for kcore
From: pi3orama <pi3orama@163.com>
In-Reply-To: <20151106140348.GD13236@kernel.org>
Date: Fri, 6 Nov 2015 22:33:40 +0800
Cc: "Wangnan (F)" <wangnan0@huawei.com>,
        Adrian Hunter <adrian.hunter@intel.com>, namhyung@kernel.org,
        lizefan@huawei.com, linux-kernel@vger.kernel.org, jolsa@kernel.org,
        masami.hiramatsu.pt@hitachi.com
Content-Transfer-Encoding: 8BIT
Message-Id: <6DA775DF-4969-405B-B074-B4D5DB8B9D53@163.com>
References: <1446803172-83107-1-git-send-email-wangnan0@huawei.com> <20151106131950.GA13236@kernel.org> <563CAC7F.4040302@huawei.com> <20151106140348.GD13236@kernel.org>
To: Arnaldo Carvalho de Melo <acme@kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3143
Lines: 78


?????ҵ? iPhone

> ?? 2015??11??6?գ?????10:03??Arnaldo Carvalho de Melo <acme@kernel.org> д????
> 
> Em Fri, Nov 06, 2015 at 09:34:55PM +0800, Wangnan (F) escreveu:
>> On 2015/11/6 21: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?
>> 
>> Not very sure. I'm not familar with this part of code. Actually
>> speaking I don't understand the relationship between what you said
>> and what I found...
> 
> What I said is that no code should, how did you state it? Here it is:
> 
> "In dso__split_kallsyms_for_kcore(), current code adjusts symbol's
> address"
> 
> It should not, not before adding it to the rbtree, and specially not
> _after, any adjustments should be done to 'struct map'.
> 
>> I spent a whole day to answer Masami's question that why
>> kernel_get_symbol_address_by_name success but __find_kernel_function()
>> fail in my platform, and described it in commit message.
> 
> Well, and that was a good exercise, I think, even one I wouldn't have
> done, being as busy as you.
> 
> Your fix was perfectly fine, there was no strict need to figure out when
> that would result in problem, at that point, if sym was NULL it should
> return -ENOENT and since 'ret' was being overwritten...
> 
>> This patch is the best one I can find.
> 
> And I thank you for that, the investigation + the patch uncovered a bug.
> We now need to find a fix, but not necessarily you need to do that tho.

And also thanks to our great testing team. They found this bug and push me to
solve it.

Thank you.

> 
>> It solves my problem but may be incorrect. Just want you and other
>> know my result. Please let me know if you and other want further
>> information. Now its pirority is low because patch 98d3b25 and
>> Masami's update are already enough for me.
> 
> Sure, lets move forward.
> 
>> I'll go back to BPF stuff. There are still much work to do :-)
> 
> Indeed, thank you for doing all this work!
> 
> - Arnaldo

--
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/