Message-Id: <6DA775DF-4969-405B-B074-B4D5DB8B9D53@163.com>
Date:	Fri, 6 Nov 2015 22:33:40 +0800
From:	pi3orama <pi3orama@....com>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:	"Wangnan (F)" <wangnan0@...wei.com>,
	Adrian Hunter <adrian.hunter@...el.com>, namhyung@...nel.org,
	lizefan@...wei.com, linux-kernel@...r.kernel.org, jolsa@...nel.org,
	masami.hiramatsu.pt@...achi.com
Subject: Re: [PATCH] perf symbols/KCORE: Rebuild rbtree when adjusting symbols for kcore



发自我的 iPhone

> 在 2015年11月6日，下午10:03，Arnaldo Carvalho de Melo <acme@...nel.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists