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

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

[Wang Nan]

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.

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++;
-- 
1.8.3.4

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

[Masami Hiramatsu]

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?

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

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

[Arnaldo Carvalho de Melo]

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.

- 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

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

[Masami Hiramatsu]

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.

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

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

[Arnaldo Carvalho de Melo]

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

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

[Masami Hiramatsu]

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

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

[Arnaldo Carvalho de Melo]

Em Tue, May 19, 2015 at 08:41:30AM +0900, Masami Hiramatsu escreveu:
> 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:
> >> 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.

Right, agreed with your comments, and that filter thing needs moving to
some other suitable place, like setting a filter at symbol system start,
etc.

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

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

[tip-bot for Wang Nan]

Commit-ID:  75e4a2a6af15e956993913314ced2582b350a647
Gitweb:     http://git.kernel.org/tip/75e4a2a6af15e956993913314ced2582b350a647
Author:     Wang Nan <wangnan0@huawei.com>
AuthorDate: Fri, 15 May 2015 12:14:44 +0000
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 18 May 2015 10:16:48 -0300

perf probe: Load map before glob matching

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 similar,
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.

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>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Zefan Li <lizefan@huawei.com>
Cc: pi3orama@163.com
Link: http://lkml.kernel.org/r/1431692084-46287-1-git-send-email-wangnan0@huawei.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.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++;