lkml.org 
[lkml]   [2015]   [Nov]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] perf symbols/KCORE: Rebuild rbtree when adjusting symbols for kcore
From
Date


发自我的 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



\
 
 \ /
  Last update: 2015-11-06 15:41    [W:0.047 / U:0.276 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site