Re: [PATCH tip/perf/core] perf probe: Load map before glob matching

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

On 2015/05/18 22:17, Arnaldo Carvalho de Melo wrote:
> Em Sat, May 16, 2015 at 09:10:59PM +0900, Masami Hiramatsu escreveu:
>> On 2015/05/16 5:07, Arnaldo Carvalho de Melo wrote:
>>> Em Sat, May 16, 2015 at 12:15:45AM +0900, Masami Hiramatsu escreveu:
>>>> On 2015/05/15 21:14, Wang Nan wrote:
>>>>> Commit 4c859351226c920b227fec040a3b447f0d482af3 ("perf probe: Support
>>>>> glob wildcards for function name") introduces a problem:
>>>>>
>>>>>   # /root/perf probe kmem_cache_free
>>>>>  Failed to find symbol kmem_cache_free in kernel
>>>>>    Error: Failed to add events.
>>>>>
>>>>> The reason is the replacement of map__for_each_symbol_by_name() (by
>>>>> map__for_each_symbol()). Although their names are similay,
>>>>> map__for_each_symbol doesn't call map__load() and dso__sort_by_name()
>>>>> before searching. The missing of map__load() causes this problem
>>>>> because it search symbol before load dso map.
>>>>
>>>> Oops, yes. But I think we'd better fix map__for_each_symbol() to call
>>>> map__load() if the map is not loaded, doesn't it?
>>>
>>> Well, that is something for a follow up patch, I'm applying his patch as
>>> it fixes a problem and is trivially correct.
>>
>> OK, then I'm OK for this patch.
> 
> Ok, tranforming the above statement into an Acked-by, ok?

Yes, here is my ack :)

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>


And also, I've found that including map__load() in map__for_each_symbol()
is not good idea. Also I think map__for_each_symbol_by_name() should
not call map__load(), since the "filter" argument will be ignored if
the map is already loaded. This means that the iterated elements can
be changed.
So, obviously the map should be loaded before calling map__for_each_XXX.

Thank you,

>  
>> Thank you,
>>
>>>
>>> - Arnaldo
>>>  
>>>> Thank you,
>>>>
>>>>>
>>>>> This patch ensures map__load() is called before using
>>>>> map__for_each_symbol().
>>>>>
>>>>> After this patch:
>>>>>
>>>>>  # /root/perf probe kmem_cache_free
>>>>>   Added new event:
>>>>>     probe:kmem_cache_free (on kmem_cache_free%return)
>>>>>
>>>>> You can now use it in all perf tools, such as:
>>>>>
>>>>>         perf record -e probe:kmem_cache_free -aR sleep 1
>>>>>
>>>>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>>>>> ---
>>>>>  tools/perf/util/probe-event.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>>>>> index 2399dc4..1faa1e6 100644
>>>>> --- a/tools/perf/util/probe-event.c
>>>>> +++ b/tools/perf/util/probe-event.c
>>>>> @@ -2499,6 +2499,9 @@ static int find_probe_functions(struct map *map, char *name)
>>>>>  	struct symbol *sym;
>>>>>  	struct rb_node *tmp;
>>>>>  
>>>>> +	if (map__load(map, NULL) < 0)
>>>>> +		return 0;
>>>>> +
>>>>>  	map__for_each_symbol(map, sym, tmp) {
>>>>>  		if (strglobmatch(sym->name, name))
>>>>>  			found++;
>>>>>
>>>>
>> -- 
>> Masami HIRAMATSU
>> Linux Technology Research Center, System Productivity Research Dept.
>> Center for Technology Innovation - Systems Engineering
>> Hitachi, Ltd., Research & Development Group
>> E-mail: masami.hiramatsu.pt@hitachi.com
> 


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com