[PATCH] perf probe: Fix module probing with shortname

[PATCH] perf probe: Fix module probing with shortname

[Wang Nan]

After commit 3d39ac538629e4f00a6e1c38d46346f1b8e69505 ("perf machine:
No need to have two DSOs lists"), perf probe with module short name doesn't
work again. For example:

 # lsmod | grep e1000e
 e1000e                233472  0

 # cat /proc/modules | grep e1000e
 e1000e 233472 0 - Live 0xffffffffa0073000

 # cat /proc/kallsyms | grep '\<e1000e_up\>'
 ffffffffa0093860 t e1000e_up[e1000e]

 # perf probe -v -m e1000e --add e1000e_up
 probe-definition(0): e1000e_up
 symbol:e1000e_up file:(null) line:0 offset:0 return:0 lazy:(null)
 0 arguments
 Failed to find module e1000e.
 Could not open debuginfo. Try to use symbols.
 Looking at the vmlinux_path (7 entries long)
 Using /lib/modules/4.2.0-rc7+/build/vmlinux for symbols
 e1000e_up is out of .text, skip it.
   Error: Failed to add events. Reason: No such file or directory (Code: -2)

This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
iff dso is vmlinux.

This patch fix 'perf probe -m' with an ad-hoc way.

After this patch:

 # perf probe -v -m e1000e --add e1000e_up
 probe-definition(0): e1000e_up
 symbol:e1000e_up file:(null) line:0 offset:0 return:0 lazy:(null)
 0 arguments
 Open Debuginfo file: /lib/modules/4.2.0-rc7+/kernel/drivers/net/ethernet/intel/e1000e/e1000e.ko
 Try to find probe point from debuginfo.
 Matched function: e1000e_up
 Probe point found: e1000e_up+0
 Found 1 probe_trace_events.
 Opening /sys/kernel/debug/tracing//kprobe_events write=1
 Writing event: p:probe/e1000e_up e1000e:e1000e_up+0
 Added new event:
   probe:e1000e_up      (on e1000e_up in e1000e)

 You can now use it in all perf tools, such as:

 	perf record -e probe:e1000e_up -aR sleep 1

 # perf probe -l
 Failed to find debug information for address ffffffffa0093860
   probe:e1000e_up      (on e1000e_up in e1000e)

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---

I think there may be other places where dso->kernel is misused.
machine__process_kernel_mmap_event() may be one of them. If I understand
correctly, 'dso->kernel && is_kernel_module(dso->long_name)' should always
false theoretically. However, I don't have enough time to check whether that
code really cause problem.

---
 tools/perf/util/probe-event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 2b78e8f..c7d6d3d 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -270,7 +270,7 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
 
 	if (module) {
 		list_for_each_entry(dso, &host_machine->dsos.head, node) {
-			if (!dso->kernel)
+			if (dso->kernel)
 				continue;
 			if (strncmp(dso->short_name + 1, module,
 				    dso->short_name_len - 2) == 0)
-- 
1.8.3.4

Re: [PATCH] perf probe: Fix module probing with shortname

[Arnaldo Carvalho de Melo]

Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
> After commit 3d39ac538629e4f00a6e1c38d46346f1b8e69505 ("perf machine:
> No need to have two DSOs lists"), perf probe with module short name doesn't
> work again. For example:
> 
>  # lsmod | grep e1000e
>  e1000e                233472  0
> 
>  # cat /proc/modules | grep e1000e
>  e1000e 233472 0 - Live 0xffffffffa0073000
> 
>  # cat /proc/kallsyms | grep '\<e1000e_up\>'
>  ffffffffa0093860 t e1000e_up[e1000e]
> 
>  # perf probe -v -m e1000e --add e1000e_up
>  probe-definition(0): e1000e_up
>  symbol:e1000e_up file:(null) line:0 offset:0 return:0 lazy:(null)
>  0 arguments
>  Failed to find module e1000e.
>  Could not open debuginfo. Try to use symbols.
>  Looking at the vmlinux_path (7 entries long)
>  Using /lib/modules/4.2.0-rc7+/build/vmlinux for symbols
>  e1000e_up is out of .text, skip it.
>    Error: Failed to add events. Reason: No such file or directory (Code: -2)
> 
> This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
> that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
> iff dso is vmlinux.

Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
check that...

- Arnaldo

> 
> This patch fix 'perf probe -m' with an ad-hoc way.
> 
> After this patch:
> 
>  # perf probe -v -m e1000e --add e1000e_up
>  probe-definition(0): e1000e_up
>  symbol:e1000e_up file:(null) line:0 offset:0 return:0 lazy:(null)
>  0 arguments
>  Open Debuginfo file: /lib/modules/4.2.0-rc7+/kernel/drivers/net/ethernet/intel/e1000e/e1000e.ko
>  Try to find probe point from debuginfo.
>  Matched function: e1000e_up
>  Probe point found: e1000e_up+0
>  Found 1 probe_trace_events.
>  Opening /sys/kernel/debug/tracing//kprobe_events write=1
>  Writing event: p:probe/e1000e_up e1000e:e1000e_up+0
>  Added new event:
>    probe:e1000e_up      (on e1000e_up in e1000e)
> 
>  You can now use it in all perf tools, such as:
> 
>  	perf record -e probe:e1000e_up -aR sleep 1
> 
>  # perf probe -l
>  Failed to find debug information for address ffffffffa0093860
>    probe:e1000e_up      (on e1000e_up in e1000e)
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
> 
> I think there may be other places where dso->kernel is misused.
> machine__process_kernel_mmap_event() may be one of them. If I understand
> correctly, 'dso->kernel && is_kernel_module(dso->long_name)' should always
> false theoretically. However, I don't have enough time to check whether that
> code really cause problem.
> 
> ---
>  tools/perf/util/probe-event.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 2b78e8f..c7d6d3d 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -270,7 +270,7 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
>  
>  	if (module) {
>  		list_for_each_entry(dso, &host_machine->dsos.head, node) {
> -			if (!dso->kernel)
> +			if (dso->kernel)
>  				continue;
>  			if (strncmp(dso->short_name + 1, module,
>  				    dso->short_name_len - 2) == 0)
> -- 
> 1.8.3.4

Re: [PATCH] perf probe: Fix module probing with shortname

[Wangnan (F)]

On 2015/9/22 21:35, Arnaldo Carvalho de Melo wrote:
> Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
>> After commit 3d39ac538629e4f00a6e1c38d46346f1b8e69505 ("perf machine:
>> No need to have two DSOs lists"), perf probe with module short name doesn't
>> work again. For example:
>>
>>   # lsmod | grep e1000e
>>   e1000e                233472  0
>>
>>   # cat /proc/modules | grep e1000e
>>   e1000e 233472 0 - Live 0xffffffffa0073000
>>
>>   # cat /proc/kallsyms | grep '\<e1000e_up\>'
>>   ffffffffa0093860 t e1000e_up[e1000e]
>>
>>   # perf probe -v -m e1000e --add e1000e_up
>>   probe-definition(0): e1000e_up
>>   symbol:e1000e_up file:(null) line:0 offset:0 return:0 lazy:(null)
>>   0 arguments
>>   Failed to find module e1000e.
>>   Could not open debuginfo. Try to use symbols.
>>   Looking at the vmlinux_path (7 entries long)
>>   Using /lib/modules/4.2.0-rc7+/build/vmlinux for symbols
>>   e1000e_up is out of .text, skip it.
>>     Error: Failed to add events. Reason: No such file or directory (Code: -2)
>>
>> This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
>> that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
>> iff dso is vmlinux.
> Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
> check that...

I also noticed this problem when I working on commit
1f121b03d058dd07199d8924373d3c52a207f63b ("perf tools: Deal with kernel 
module names in '[]' correctly") ;)

It should be bug, but I think fixing it is costy. Here's an assumption 
that, if dso->kernel
is not zero, the dso should be vmlinux (not kernel module):

$ grep 'dso.>kernel)' ./tools/perf/ -r
./tools/perf/builtin-inject.c:    if (dso->kernel)
./tools/perf/util/symbol.c:    if (dso->kernel) {
./tools/perf/util/symbol-elf.c:        if (dso->kernel)
./tools/perf/util/symbol-elf.c:                if (remap_kernel && 
dso->kernel) {
./tools/perf/util/event.c:        if (pos->dso->kernel)
./tools/perf/util/probe-event.c:            if (dso->kernel)
./tools/perf/util/map.c: * map->dso->kernel) before calling 
__map__is_{kernel,kmodule}())
./tools/perf/util/map.c:    if (!map->dso || !map->dso->kernel) {
./tools/perf/builtin-top.c:    if (!map->dso->kernel)

So care must be taken.

Another solution seems simpler: we can redefine the meaning of enum 
dso_kernel_type like this:

# find  ./tools/perf/ -type f | xargs -n1 sed -i 
's/DSO_TYPE_USER/DSO_TYPE_NOT_VMLINUX/g'
# find  ./tools/perf/ -type f | xargs -n1 sed -i 
's/DSO_TYPE_KERNEL/DSO_TYPE_VMLINUX/g'
# find  ./tools/perf/ -type f | xargs -n1 sed -i 
's/DSO_TYPE_GUEST_KERNEL/DSO_TYPE_GUEST_VMLINUX/g'

By fixing the name of DSO_TYPE_USER, kernel module with 
DSO_TYPE_NOT_VMLINUX seems
not so buggy. (Please choose a better name...)

What's your opinion?

Thank you.

> - Arnaldo
>
>> This patch fix 'perf probe -m' with an ad-hoc way.
>>
>> After this patch:
>>
>>   # perf probe -v -m e1000e --add e1000e_up
>>   probe-definition(0): e1000e_up
>>   symbol:e1000e_up file:(null) line:0 offset:0 return:0 lazy:(null)
>>   0 arguments
>>   Open Debuginfo file: /lib/modules/4.2.0-rc7+/kernel/drivers/net/ethernet/intel/e1000e/e1000e.ko
>>   Try to find probe point from debuginfo.
>>   Matched function: e1000e_up
>>   Probe point found: e1000e_up+0
>>   Found 1 probe_trace_events.
>>   Opening /sys/kernel/debug/tracing//kprobe_events write=1
>>   Writing event: p:probe/e1000e_up e1000e:e1000e_up+0
>>   Added new event:
>>     probe:e1000e_up      (on e1000e_up in e1000e)
>>
>>   You can now use it in all perf tools, such as:
>>
>>   	perf record -e probe:e1000e_up -aR sleep 1
>>
>>   # perf probe -l
>>   Failed to find debug information for address ffffffffa0093860
>>     probe:e1000e_up      (on e1000e_up in e1000e)
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> ---
>>
>> I think there may be other places where dso->kernel is misused.
>> machine__process_kernel_mmap_event() may be one of them. If I understand
>> correctly, 'dso->kernel && is_kernel_module(dso->long_name)' should always
>> false theoretically. However, I don't have enough time to check whether that
>> code really cause problem.
>>
>> ---
>>   tools/perf/util/probe-event.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index 2b78e8f..c7d6d3d 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -270,7 +270,7 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
>>   
>>   	if (module) {
>>   		list_for_each_entry(dso, &host_machine->dsos.head, node) {
>> -			if (!dso->kernel)
>> +			if (dso->kernel)
>>   				continue;
>>   			if (strncmp(dso->short_name + 1, module,
>>   				    dso->short_name_len - 2) == 0)
>> -- 
>> 1.8.3.4

Re: [PATCH] perf probe: Fix module probing with shortname

[Arnaldo Carvalho de Melo]

Em Wed, Sep 23, 2015 at 09:14:44AM +0800, Wangnan (F) escreveu:
> 
> 
> On 2015/9/22 21:35, Arnaldo Carvalho de Melo wrote:
> >Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
> >>After commit 3d39ac538629e4f00a6e1c38d46346f1b8e69505 ("perf machine:
> >>No need to have two DSOs lists"), perf probe with module short name doesn't
> >>work again. For example:
> >>
> >>  # lsmod | grep e1000e
> >>  e1000e                233472  0
> >>
> >>  # cat /proc/modules | grep e1000e
> >>  e1000e 233472 0 - Live 0xffffffffa0073000
> >>
> >>  # cat /proc/kallsyms | grep '\<e1000e_up\>'
> >>  ffffffffa0093860 t e1000e_up[e1000e]
> >>
> >>  # perf probe -v -m e1000e --add e1000e_up
> >>  probe-definition(0): e1000e_up
> >>  symbol:e1000e_up file:(null) line:0 offset:0 return:0 lazy:(null)
> >>  0 arguments
> >>  Failed to find module e1000e.
> >>  Could not open debuginfo. Try to use symbols.
> >>  Looking at the vmlinux_path (7 entries long)
> >>  Using /lib/modules/4.2.0-rc7+/build/vmlinux for symbols
> >>  e1000e_up is out of .text, skip it.
> >>    Error: Failed to add events. Reason: No such file or directory (Code: -2)
> >>
> >>This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
> >>that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
> >>iff dso is vmlinux.
> >Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
> >check that...
> 
> I also noticed this problem when I working on commit
> 1f121b03d058dd07199d8924373d3c52a207f63b ("perf tools: Deal with
> kernel module names in '[]' correctly") ;)

Thanks for working on this, it is an area that needs cleaning up, too
many ways to say what a dso is, will study your findings and try to come
up with a patch proposal tomorrow.

- Arnaldo
 
> It should be bug, but I think fixing it is costy. Here's an
> assumption that, if dso->kernel
> is not zero, the dso should be vmlinux (not kernel module):
> 
> $ grep 'dso.>kernel)' ./tools/perf/ -r
> ./tools/perf/builtin-inject.c:    if (dso->kernel)
> ./tools/perf/util/symbol.c:    if (dso->kernel) {
> ./tools/perf/util/symbol-elf.c:        if (dso->kernel)
> ./tools/perf/util/symbol-elf.c:                if (remap_kernel &&
> dso->kernel) {
> ./tools/perf/util/event.c:        if (pos->dso->kernel)
> ./tools/perf/util/probe-event.c:            if (dso->kernel)
> ./tools/perf/util/map.c: * map->dso->kernel) before calling
> __map__is_{kernel,kmodule}())
> ./tools/perf/util/map.c:    if (!map->dso || !map->dso->kernel) {
> ./tools/perf/builtin-top.c:    if (!map->dso->kernel)
> 
> So care must be taken.
> 
> Another solution seems simpler: we can redefine the meaning of enum
> dso_kernel_type like this:
> 
> # find  ./tools/perf/ -type f | xargs -n1 sed -i
> 's/DSO_TYPE_USER/DSO_TYPE_NOT_VMLINUX/g'
> # find  ./tools/perf/ -type f | xargs -n1 sed -i
> 's/DSO_TYPE_KERNEL/DSO_TYPE_VMLINUX/g'
> # find  ./tools/perf/ -type f | xargs -n1 sed -i
> 's/DSO_TYPE_GUEST_KERNEL/DSO_TYPE_GUEST_VMLINUX/g'
> 
> By fixing the name of DSO_TYPE_USER, kernel module with
> DSO_TYPE_NOT_VMLINUX seems
> not so buggy. (Please choose a better name...)
> 
> What's your opinion?
> 
> Thank you.
> 
> >- Arnaldo
> >
> >>This patch fix 'perf probe -m' with an ad-hoc way.
> >>
> >>After this patch:
> >>
> >>  # perf probe -v -m e1000e --add e1000e_up
> >>  probe-definition(0): e1000e_up
> >>  symbol:e1000e_up file:(null) line:0 offset:0 return:0 lazy:(null)
> >>  0 arguments
> >>  Open Debuginfo file: /lib/modules/4.2.0-rc7+/kernel/drivers/net/ethernet/intel/e1000e/e1000e.ko
> >>  Try to find probe point from debuginfo.
> >>  Matched function: e1000e_up
> >>  Probe point found: e1000e_up+0
> >>  Found 1 probe_trace_events.
> >>  Opening /sys/kernel/debug/tracing//kprobe_events write=1
> >>  Writing event: p:probe/e1000e_up e1000e:e1000e_up+0
> >>  Added new event:
> >>    probe:e1000e_up      (on e1000e_up in e1000e)
> >>
> >>  You can now use it in all perf tools, such as:
> >>
> >>  	perf record -e probe:e1000e_up -aR sleep 1
> >>
> >>  # perf probe -l
> >>  Failed to find debug information for address ffffffffa0093860
> >>    probe:e1000e_up      (on e1000e_up in e1000e)
> >>
> >>Signed-off-by: Wang Nan <wangnan0@huawei.com>
> >>Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> >>Cc: Namhyung Kim <namhyung@kernel.org>
> >>Cc: Jiri Olsa <jolsa@redhat.com>
> >>Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >>---
> >>
> >>I think there may be other places where dso->kernel is misused.
> >>machine__process_kernel_mmap_event() may be one of them. If I understand
> >>correctly, 'dso->kernel && is_kernel_module(dso->long_name)' should always
> >>false theoretically. However, I don't have enough time to check whether that
> >>code really cause problem.
> >>
> >>---
> >>  tools/perf/util/probe-event.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> >>index 2b78e8f..c7d6d3d 100644
> >>--- a/tools/perf/util/probe-event.c
> >>+++ b/tools/perf/util/probe-event.c
> >>@@ -270,7 +270,7 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
> >>  	if (module) {
> >>  		list_for_each_entry(dso, &host_machine->dsos.head, node) {
> >>-			if (!dso->kernel)
> >>+			if (dso->kernel)
> >>  				continue;
> >>  			if (strncmp(dso->short_name + 1, module,
> >>  				    dso->short_name_len - 2) == 0)
> >>-- 
> >>1.8.3.4
> 

Re: [PATCH] perf probe: Fix module probing with shortname

[Arnaldo Carvalho de Melo]

Em Tue, Sep 22, 2015 at 11:49:02PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Sep 23, 2015 at 09:14:44AM +0800, Wangnan (F) escreveu:
> > On 2015/9/22 21:35, Arnaldo Carvalho de Melo wrote:
> > >Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
> > >>After commit 3d39ac538629e4f00a6e1c38d46346f1b8e69505 ("perf machine:
> > >>No need to have two DSOs lists"), perf probe with module short name doesn't
> > >>work again. For example:
> > >>
> > >>  # lsmod | grep e1000e
> > >>  e1000e                233472  0
> > >>
> > >>  # cat /proc/modules | grep e1000e
> > >>  e1000e 233472 0 - Live 0xffffffffa0073000
> > >>
> > >>  # cat /proc/kallsyms | grep '\<e1000e_up\>'
> > >>  ffffffffa0093860 t e1000e_up[e1000e]
> > >>
> > >>  # perf probe -v -m e1000e --add e1000e_up
> > >>  probe-definition(0): e1000e_up
> > >>  symbol:e1000e_up file:(null) line:0 offset:0 return:0 lazy:(null)
> > >>  0 arguments
> > >>  Failed to find module e1000e.
> > >>  Could not open debuginfo. Try to use symbols.
> > >>  Looking at the vmlinux_path (7 entries long)
> > >>  Using /lib/modules/4.2.0-rc7+/build/vmlinux for symbols
> > >>  e1000e_up is out of .text, skip it.
> > >>    Error: Failed to add events. Reason: No such file or directory (Code: -2)
> > >>
> > >>This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
> > >>that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
> > >>iff dso is vmlinux.
> > >Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
> > >check that...
> > 
> > I also noticed this problem when I working on commit
> > 1f121b03d058dd07199d8924373d3c52a207f63b ("perf tools: Deal with
> > kernel module names in '[]' correctly") ;)
> 
> Thanks for working on this, it is an area that needs cleaning up, too
> many ways to say what a dso is, will study your findings and try to come
> up with a patch proposal tomorrow.
> 
> - Arnaldo
>  
> > It should be bug, but I think fixing it is costy. Here's an
> > assumption that, if dso->kernel
> > is not zero, the dso should be vmlinux (not kernel module):
> > 
> > $ grep 'dso.>kernel)' ./tools/perf/ -r
> > ./tools/perf/builtin-inject.c:    if (dso->kernel)
> > ./tools/perf/util/symbol.c:    if (dso->kernel) {
> > ./tools/perf/util/symbol-elf.c:        if (dso->kernel)
> > ./tools/perf/util/symbol-elf.c:                if (remap_kernel &&
> > dso->kernel) {
> > ./tools/perf/util/event.c:        if (pos->dso->kernel)
> > ./tools/perf/util/probe-event.c:            if (dso->kernel)
> > ./tools/perf/util/map.c: * map->dso->kernel) before calling
> > __map__is_{kernel,kmodule}())
> > ./tools/perf/util/map.c:    if (!map->dso || !map->dso->kernel) {
> > ./tools/perf/builtin-top.c:    if (!map->dso->kernel)
> > 
> > So care must be taken.

So, yes, there are multiple uses for this dso->kernel thing, we need to
look at each one and go on clarifying it so that this gets corrected and
sane, but I think we need some helpers to clarify all this, namely:

	Adding DSO_TYPE_KMODULE and DSO_TYPE_GUEST_KMODULE, setting
dso->kernel with it when loading host and guest kernel modules and
adding:

	bool dso__is_host_kernel(struct dso *dso)
	bool dso__is_host_kmodule(struct dso *dso)

	bool dso__is_guest_kernel(struct dso *dso)
	bool dso__is_guest_kmodule(struct dso *dso)

	And then these:

	static inline bool dso__is_host_kernel_level(struct dso *dso)
	{
		return dso__is_host_kernel(dso) || dso__is_host_kmodule(module);
	}

	static inline bool dso__is_guest_kernel_level(struct dso *dso)
	{
		return dso__is_guest_kernel(dso) || dso__is_guest_kmodule(module);
	}

	static inline bool dso__is_kernel_level(struct dso *dso)
	{
		return dso__is_host_kernel_level(dso) || dso__is_guest_kernel_level(module);
	}

	static inline bool dso__is_kmodule(struct dso *dso)
	{
		return  dso__is_host_kmodule(dso) || dso__is_guest_kmodule(dso);
	}

	static inline bool dso__is_kernel(struct dso *dso)
	{
		return dso__is_host_kernel(dso) || dso__is_guest_kernel(module);
	}

Then, looking at where dso->kernel is used now, we use:

tools/perf/builtin-inject.c

  static int dso__inject_build_id(struct dso *dso, struct perf_tool *tool,
                                  struct machine *machine)
  {
          if (dso->kernel)
                misc = PERF_RECORD_MISC_KERNEL;

   Should use dso__is_host_kernel_level()

   And then there is a bug here, where we need to check for
dso__is_guest_kernel_level() as well and if that is true, set misc to
PERF_RECORD_MISC_GUEST_KERNEL, ditto for PERF_RECORD_MISC_GUEST_USER.

---------------------------------------------------------

  tools/perf/builtin-top.c:    if (!map->dso->kernel)

  static int symbol_filter(struct map *map, struct symbol *sym)
  {
          const char *name = sym->name;
                                               
          if (!map->dso->kernel)
                  return 0;

  Should use dso__is_kernel(), this is an old filter function dealing
only with vmlinux symbol names.

---------------------------------------------------------

  tools/perf/ui/browsers/hists.c

        if (asprintf(optstr, "Zoom %s %s DSO",
                     browser->hists->dso_filter ? "out of" : "into",
                     dso->kernel ? "the Kernel" : dso->short_name) < 0)

  Should use dso__is_host_kernel() and probably should support the guest
cases, with "Guest kernel" and a "Guest module " prefix for guest kernel
modules.

---------------------------------------------------------

int perf_event__synthesize_modules(struct perf_tool *tool,
                                   perf_event__handler_t process,
                                   struct machine *machine)

        for (pos = maps__first(maps); pos; pos = map__next(pos)) {
                size_t size;

                if (pos->dso->kernel)
                        continue;

This one is ok for non guest stuff, but should be clarified, and guests
should be supported, do that by using:

	if (!dso__is_kmodule(dso))
		continue;

---------------------------------------------------------

tools/perf/util/map.c

struct map *map__new2(u64 start, struct dso *dso, enum map_type type)
{
        struct map *map = calloc(1, (sizeof(*map) +
                                     (dso->kernel ? sizeof(struct kmap) : 0)));

Should use dso__is_kernel(dso)

---------------------------------------------------------
tools/perf/util/map.c

struct kmap *map__kmap(struct map *map)
{
        if (!map->dso || !map->dso->kernel) {
                pr_err("Internal error: map__kmap with a non-kernel map\n");

Same thing as above with map__new2()

---------------------------------------------------------

tools/perf/util/machine.c

machine__process_kernel_mmap_event()

                        if (!dso->kernel ||
                            is_kernel_module(dso->long_name,
                                             PERF_RECORD_MISC_CPUMODE_UNKNOWN))

	Should use:

	dso__is_kmodule()

	And remove that long comment :-)

---------------------------------------------------------
struct dso *machine__findnew_kernel(struct machine *machine, const char *name,
                                    const char *short_name, int dso_type)
{
        if (dso != NULL) {
                dso__set_short_name(dso, short_name, false);
                dso->kernel = dso_type;


This one is ok as it is, checked its callers.

---------------------------------------------------------

tools/perf/util/probe-event.c

static int kernel_get_module_dso(const char *module, struct dso **pdso)
{
        if (module) {
                list_for_each_entry(dso, &host_machine->dsos.head, node) {
                        if (!dso->kernel)
                                continue;


        Should use dso__is_kmodule(dso)

---------------------------------------------------------

Looking at the others after lunch, will try to add these step by step so
that we can bisect any thing we miss. But this has to be cleaned
out/clarified, got out of hand.

- Arnaldo
   
> > Another solution seems simpler: we can redefine the meaning of enum
> > dso_kernel_type like this:
> > 
> > # find  ./tools/perf/ -type f | xargs -n1 sed -i
> > 's/DSO_TYPE_USER/DSO_TYPE_NOT_VMLINUX/g'
> > # find  ./tools/perf/ -type f | xargs -n1 sed -i
> > 's/DSO_TYPE_KERNEL/DSO_TYPE_VMLINUX/g'
> > # find  ./tools/perf/ -type f | xargs -n1 sed -i
> > 's/DSO_TYPE_GUEST_KERNEL/DSO_TYPE_GUEST_VMLINUX/g'
> > 
> > By fixing the name of DSO_TYPE_USER, kernel module with
> > DSO_TYPE_NOT_VMLINUX seems
> > not so buggy. (Please choose a better name...)
> > 
> > What's your opinion?
> > 
> > Thank you.
> > 
> > >- Arnaldo
> > >
> > >>This patch fix 'perf probe -m' with an ad-hoc way.
> > >>
> > >>After this patch:
> > >>
> > >>  # perf probe -v -m e1000e --add e1000e_up
> > >>  probe-definition(0): e1000e_up
> > >>  symbol:e1000e_up file:(null) line:0 offset:0 return:0 lazy:(null)
> > >>  0 arguments
> > >>  Open Debuginfo file: /lib/modules/4.2.0-rc7+/kernel/drivers/net/ethernet/intel/e1000e/e1000e.ko
> > >>  Try to find probe point from debuginfo.
> > >>  Matched function: e1000e_up
> > >>  Probe point found: e1000e_up+0
> > >>  Found 1 probe_trace_events.
> > >>  Opening /sys/kernel/debug/tracing//kprobe_events write=1
> > >>  Writing event: p:probe/e1000e_up e1000e:e1000e_up+0
> > >>  Added new event:
> > >>    probe:e1000e_up      (on e1000e_up in e1000e)
> > >>
> > >>  You can now use it in all perf tools, such as:
> > >>
> > >>  	perf record -e probe:e1000e_up -aR sleep 1
> > >>
> > >>  # perf probe -l
> > >>  Failed to find debug information for address ffffffffa0093860
> > >>    probe:e1000e_up      (on e1000e_up in e1000e)
> > >>
> > >>Signed-off-by: Wang Nan <wangnan0@huawei.com>
> > >>Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > >>Cc: Namhyung Kim <namhyung@kernel.org>
> > >>Cc: Jiri Olsa <jolsa@redhat.com>
> > >>Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > >>---
> > >>
> > >>I think there may be other places where dso->kernel is misused.
> > >>machine__process_kernel_mmap_event() may be one of them. If I understand
> > >>correctly, 'dso->kernel && is_kernel_module(dso->long_name)' should always
> > >>false theoretically. However, I don't have enough time to check whether that
> > >>code really cause problem.
> > >>
> > >>---
> > >>  tools/perf/util/probe-event.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >>diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > >>index 2b78e8f..c7d6d3d 100644
> > >>--- a/tools/perf/util/probe-event.c
> > >>+++ b/tools/perf/util/probe-event.c
> > >>@@ -270,7 +270,7 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
> > >>  	if (module) {
> > >>  		list_for_each_entry(dso, &host_machine->dsos.head, node) {
> > >>-			if (!dso->kernel)
> > >>+			if (dso->kernel)
> > >>  				continue;
> > >>  			if (strncmp(dso->short_name + 1, module,
> > >>  				    dso->short_name_len - 2) == 0)
> > >>-- 
> > >>1.8.3.4
> > 

Re: [PATCH] perf probe: Fix module probing with shortname

[Arnaldo Carvalho de Melo]

Em Wed, Sep 23, 2015 at 01:03:19PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Sep 22, 2015 at 11:49:02PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Sep 23, 2015 at 09:14:44AM +0800, Wangnan (F) escreveu:
> > > On 2015/9/22 21:35, Arnaldo Carvalho de Melo wrote:
> > > >Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
> > > >>This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
> > > >>that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
> > > >>iff dso is vmlinux.
> > > >Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
> > > >check that...

> > > I also noticed this problem when I working on commit
> > > 1f121b03d058dd07199d8924373d3c52a207f63b ("perf tools: Deal with
> > > kernel module names in '[]' correctly") ;)

> > Thanks for working on this, it is an area that needs cleaning up, too
> > many ways to say what a dso is, will study your findings and try to come
> > up with a patch proposal tomorrow.
> > > So care must be taken.
> 
> So, yes, there are multiple uses for this dso->kernel thing, we need to
> look at each one and go on clarifying it so that this gets corrected and
> sane, but I think we need some helpers to clarify all this, namely:
> 
> 	Adding DSO_TYPE_KMODULE and DSO_TYPE_GUEST_KMODULE, setting
> dso->kernel with it when loading host and guest kernel modules and
> adding:
> 
> 	bool dso__is_host_kernel(struct dso *dso)
> 	bool dso__is_host_kmodule(struct dso *dso)
> 
> 	bool dso__is_guest_kernel(struct dso *dso)

So I tried this, ended up basically using __map__is_kernel(), in several
places, which I think is right, and I've stashed in my tmp.perf/core
branch so that you can take a look.

But then I realized that we already have a better way to achieve what is
needed in that function, see the patch below, fixed things for me:

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 2b78e8f19b45..7fb0533ab18c 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -269,12 +269,13 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
 	int ret = 0;
 
 	if (module) {
-		list_for_each_entry(dso, &host_machine->dsos.head, node) {
-			if (!dso->kernel)
-				continue;
-			if (strncmp(dso->short_name + 1, module,
-				    dso->short_name_len - 2) == 0)
-				goto found;
+		char module_name[128];
+
+		snprintf(module_name, sizeof(module_name), "[%s]", module);
+		map = map_groups__find_by_name(&host_machine->kmaps, MAP__FUNCTION, module_name);
+		if (map) {
+			dso = map->dso;
+			goto found;
 		}
 		pr_debug("Failed to find module %s.\n", module);
 		return -ENOENT;

Re: [PATCH] perf probe: Fix module probing with shortname

[Arnaldo Carvalho de Melo]

Em Wed, Sep 23, 2015 at 10:50:08PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Sep 23, 2015 at 01:03:19PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Sep 22, 2015 at 11:49:02PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Wed, Sep 23, 2015 at 09:14:44AM +0800, Wangnan (F) escreveu:
> > > > On 2015/9/22 21:35, Arnaldo Carvalho de Melo wrote:
> > > > >Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
> > > > >>This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
> > > > >>that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
> > > > >>iff dso is vmlinux.
> > > > >Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
> > > > >check that...
> 
> > > > I also noticed this problem when I working on commit
> > > > 1f121b03d058dd07199d8924373d3c52a207f63b ("perf tools: Deal with
> > > > kernel module names in '[]' correctly") ;)
> 
> > > Thanks for working on this, it is an area that needs cleaning up, too
> > > many ways to say what a dso is, will study your findings and try to come
> > > up with a patch proposal tomorrow.
> > > > So care must be taken.
> > 
> > So, yes, there are multiple uses for this dso->kernel thing, we need to
> > look at each one and go on clarifying it so that this gets corrected and
> > sane, but I think we need some helpers to clarify all this, namely:
> > 
> > 	Adding DSO_TYPE_KMODULE and DSO_TYPE_GUEST_KMODULE, setting
> > dso->kernel with it when loading host and guest kernel modules and
> > adding:
> > 
> > 	bool dso__is_host_kernel(struct dso *dso)
> > 	bool dso__is_host_kmodule(struct dso *dso)
> > 
> > 	bool dso__is_guest_kernel(struct dso *dso)
> 
> So I tried this, ended up basically using __map__is_kernel(), in several
> places, which I think is right, and I've stashed in my tmp.perf/core
> branch so that you can take a look.
> 
> But then I realized that we already have a better way to achieve what is
> needed in that function, see the patch below, fixed things for me:

Using it:


[root@zoo ~]# perf probe -v -m usbnet --add usbnet_start_xmit
probe-definition(0): usbnet_start_xmit
symbol:usbnet_start_xmit file:(null) line:0 offset:0 return:0 lazy:(null)
0 arguments
Open Debuginfo file: /lib/modules/4.3.0-rc1+/kernel/drivers/net/usb/usbnet.ko
Try to find probe point from debuginfo.
Matched function: usbnet_start_xmit
Probe point found: usbnet_start_xmit+0
Found 1 probe_trace_events.
Opening /sys/kernel/debug/tracing//kprobe_events write=1
Writing event: p:probe/usbnet_start_xmit usbnet:usbnet_start_xmit+0
Added new event:
  probe:usbnet_start_xmit (on usbnet_start_xmit in usbnet)

You can now use it in all perf tools, such as:

	perf record -e probe:usbnet_start_xmit -aR sleep 1

[root@zoo ~]#

ot@zoo ~]# perf probe -m kvm --add apic_has_pending_timer
Added new event:
  probe:apic_has_pending_timer (on apic_has_pending_timer in kvm)

You can now use it in all perf tools, such as:

	perf record -e probe:apic_has_pending_timer -aR sleep 1

[root@zoo ~]#

[root@zoo ~]# perf probe -m kvm -F ioapic*
ioapic_mmio_read
ioapic_mmio_write
ioapic_service
ioapic_set_irq
[root@zoo ~]# 

[root@zoo ~]# perf probe -m mac80211 -F 'i*_tx_*' | head -10
ieee80211_add_tx_ts
ieee80211_agg_tx_operational
ieee80211_clear_tx_pending
ieee80211_del_tx_ts
ieee80211_get_key_tx_seq
ieee80211_get_tx_power
ieee80211_get_tx_rates
ieee80211_init_tx_queue
ieee80211_mgd_conn_tx_status
ieee80211_mgmt_tx_cancel_wait
[root@zoo ~]# 

- Arnaldo

 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 2b78e8f19b45..7fb0533ab18c 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -269,12 +269,13 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
>  	int ret = 0;
>  
>  	if (module) {
> -		list_for_each_entry(dso, &host_machine->dsos.head, node) {
> -			if (!dso->kernel)
> -				continue;
> -			if (strncmp(dso->short_name + 1, module,
> -				    dso->short_name_len - 2) == 0)
> -				goto found;
> +		char module_name[128];
> +
> +		snprintf(module_name, sizeof(module_name), "[%s]", module);
> +		map = map_groups__find_by_name(&host_machine->kmaps, MAP__FUNCTION, module_name);
> +		if (map) {
> +			dso = map->dso;
> +			goto found;
>  		}
>  		pr_debug("Failed to find module %s.\n", module);
>  		return -ENOENT;

Re: [PATCH] perf probe: Fix module probing with shortname

[pi3orama]

发自我的 iPhone

> 在 2015年9月24日，上午10:05，Arnaldo Carvalho de Melo <acme@kernel.org> 写道：
> 
> Em Wed, Sep 23, 2015 at 10:50:08PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Wed, Sep 23, 2015 at 01:03:19PM -0300, Arnaldo Carvalho de Melo escreveu:
>>> Em Tue, Sep 22, 2015 at 11:49:02PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>> Em Wed, Sep 23, 2015 at 09:14:44AM +0800, Wangnan (F) escreveu:
>>>>>> On 2015/9/22 21:35, Arnaldo Carvalho de Melo wrote:
>>>>>> Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
>>>>>>> This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
>>>>>>> that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
>>>>>>> iff dso is vmlinux.
>>>>>> Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
>>>>>> check that...
>> 
>>>>> I also noticed this problem when I working on commit
>>>>> 1f121b03d058dd07199d8924373d3c52a207f63b ("perf tools: Deal with
>>>>> kernel module names in '[]' correctly") ;)
>> 
>>>> Thanks for working on this, it is an area that needs cleaning up, too
>>>> many ways to say what a dso is, will study your findings and try to come
>>>> up with a patch proposal tomorrow.
>>>>> So care must be taken.
>>> 
>>> So, yes, there are multiple uses for this dso->kernel thing, we need to
>>> look at each one and go on clarifying it so that this gets corrected and
>>> sane, but I think we need some helpers to clarify all this, namely:
>>> 
>>>    Adding DSO_TYPE_KMODULE and DSO_TYPE_GUEST_KMODULE, setting
>>> dso->kernel with it when loading host and guest kernel modules and
>>> adding:
>>> 
>>>    bool dso__is_host_kernel(struct dso *dso)
>>>    bool dso__is_host_kmodule(struct dso *dso)
>>> 
>>>    bool dso__is_guest_kernel(struct dso *dso)
>> 
>> So I tried this, ended up basically using __map__is_kernel(), in several
>> places, which I think is right, and I've stashed in my tmp.perf/core
>> branch so that you can take a look.
>> 
>> But then I realized that we already have a better way to achieve what is
>> needed in that function, see the patch below, fixed things for me:
> 

That's good, but the meaning of dso->kernel is still confusing.

However, since the problem is solved, do you still think it should be fixed?

Thank you.

> Using it:
> 
> 
> [root@zoo ~]# perf probe -v -m usbnet --add usbnet_start_xmit
> probe-definition(0): usbnet_start_xmit
> symbol:usbnet_start_xmit file:(null) line:0 offset:0 return:0 lazy:(null)
> 0 arguments
> Open Debuginfo file: /lib/modules/4.3.0-rc1+/kernel/drivers/net/usb/usbnet.ko
> Try to find probe point from debuginfo.
> Matched function: usbnet_start_xmit
> Probe point found: usbnet_start_xmit+0
> Found 1 probe_trace_events.
> Opening /sys/kernel/debug/tracing//kprobe_events write=1
> Writing event: p:probe/usbnet_start_xmit usbnet:usbnet_start_xmit+0
> Added new event:
>  probe:usbnet_start_xmit (on usbnet_start_xmit in usbnet)
> 
> You can now use it in all perf tools, such as:
> 
>    perf record -e probe:usbnet_start_xmit -aR sleep 1
> 
> [root@zoo ~]#
> 
> ot@zoo ~]# perf probe -m kvm --add apic_has_pending_timer
> Added new event:
>  probe:apic_has_pending_timer (on apic_has_pending_timer in kvm)
> 
> You can now use it in all perf tools, such as:
> 
>    perf record -e probe:apic_has_pending_timer -aR sleep 1
> 
> [root@zoo ~]#
> 
> [root@zoo ~]# perf probe -m kvm -F ioapic*
> ioapic_mmio_read
> ioapic_mmio_write
> ioapic_service
> ioapic_set_irq
> [root@zoo ~]# 
> 
> [root@zoo ~]# perf probe -m mac80211 -F 'i*_tx_*' | head -10
> ieee80211_add_tx_ts
> ieee80211_agg_tx_operational
> ieee80211_clear_tx_pending
> ieee80211_del_tx_ts
> ieee80211_get_key_tx_seq
> ieee80211_get_tx_power
> ieee80211_get_tx_rates
> ieee80211_init_tx_queue
> ieee80211_mgd_conn_tx_status
> ieee80211_mgmt_tx_cancel_wait
> [root@zoo ~]# 
> 
> - Arnaldo
> 
> 
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index 2b78e8f19b45..7fb0533ab18c 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -269,12 +269,13 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
>>    int ret = 0;
>> 
>>    if (module) {
>> -        list_for_each_entry(dso, &host_machine->dsos.head, node) {
>> -            if (!dso->kernel)
>> -                continue;
>> -            if (strncmp(dso->short_name + 1, module,
>> -                    dso->short_name_len - 2) == 0)
>> -                goto found;
>> +        char module_name[128];
>> +
>> +        snprintf(module_name, sizeof(module_name), "[%s]", module);
>> +        map = map_groups__find_by_name(&host_machine->kmaps, MAP__FUNCTION, module_name);
>> +        if (map) {
>> +            dso = map->dso;
>> +            goto found;
>>        }
>>        pr_debug("Failed to find module %s.\n", module);
>>        return -ENOENT;

RE: [PATCH] perf probe: Fix module probing with shortname

[平松雅巳 / HIRAMATU，MASAMI]

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


>-----Original Message-----
>From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
>Sent: Thursday, September 24, 2015 10:50 AM
>To: Wangnan (F)
>Cc: linux-kernel@vger.kernel.org; lizefan@huawei.com; pi3orama@163.com; Namhyung Kim; Jiri Olsa; 平松雅巳 / HIRAMATU，MASAMI
>Subject: Re: [PATCH] perf probe: Fix module probing with shortname
>
>Em Wed, Sep 23, 2015 at 01:03:19PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Tue, Sep 22, 2015 at 11:49:02PM -0300, Arnaldo Carvalho de Melo escreveu:
>> > Em Wed, Sep 23, 2015 at 09:14:44AM +0800, Wangnan (F) escreveu:
>> > > On 2015/9/22 21:35, Arnaldo Carvalho de Melo wrote:
>> > > >Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
>> > > >>This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
>> > > >>that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
>> > > >>iff dso is vmlinux.
>> > > >Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
>> > > >check that...
>
>> > > I also noticed this problem when I working on commit
>> > > 1f121b03d058dd07199d8924373d3c52a207f63b ("perf tools: Deal with
>> > > kernel module names in '[]' correctly") ;)
>
>> > Thanks for working on this, it is an area that needs cleaning up, too
>> > many ways to say what a dso is, will study your findings and try to come
>> > up with a patch proposal tomorrow.
>> > > So care must be taken.
>>
>> So, yes, there are multiple uses for this dso->kernel thing, we need to
>> look at each one and go on clarifying it so that this gets corrected and
>> sane, but I think we need some helpers to clarify all this, namely:
>>
>> 	Adding DSO_TYPE_KMODULE and DSO_TYPE_GUEST_KMODULE, setting
>> dso->kernel with it when loading host and guest kernel modules and
>> adding:
>>
>> 	bool dso__is_host_kernel(struct dso *dso)
>> 	bool dso__is_host_kmodule(struct dso *dso)
>>
>> 	bool dso__is_guest_kernel(struct dso *dso)
>
>So I tried this, ended up basically using __map__is_kernel(), in several
>places, which I think is right, and I've stashed in my tmp.perf/core
>branch so that you can take a look.
>
>But then I realized that we already have a better way to achieve what is
>needed in that function, see the patch below, fixed things for me:

Hmm, this is a bit hacky, but yes, it looks good to me.
However, I hope to have above solution, for future use.

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

Thanks!

>
>diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>index 2b78e8f19b45..7fb0533ab18c 100644
>--- a/tools/perf/util/probe-event.c
>+++ b/tools/perf/util/probe-event.c
>@@ -269,12 +269,13 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
> 	int ret = 0;
>
> 	if (module) {
>-		list_for_each_entry(dso, &host_machine->dsos.head, node) {
>-			if (!dso->kernel)
>-				continue;
>-			if (strncmp(dso->short_name + 1, module,
>-				    dso->short_name_len - 2) == 0)
>-				goto found;
>+		char module_name[128];
>+
>+		snprintf(module_name, sizeof(module_name), "[%s]", module);
>+		map = map_groups__find_by_name(&host_machine->kmaps, MAP__FUNCTION, module_name);
>+		if (map) {
>+			dso = map->dso;
>+			goto found;
> 		}
> 		pr_debug("Failed to find module %s.\n", module);
> 		return -ENOENT;

Re: [PATCH] perf probe: Fix module probing with shortname

['Arnaldo Carvalho de Melo']

Em Thu, Sep 24, 2015 at 10:10:31AM +0000, 平松雅巳 / HIRAMATU，MASAMI escreveu:
> 
> 
> -- 
> 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
> 
> 
> >-----Original Message-----
> >From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
> >Sent: Thursday, September 24, 2015 10:50 AM
> >To: Wangnan (F)
> >Cc: linux-kernel@vger.kernel.org; lizefan@huawei.com; pi3orama@163.com; Namhyung Kim; Jiri Olsa; 平松雅巳 / HIRAMATU，MASAMI
> >Subject: Re: [PATCH] perf probe: Fix module probing with shortname
> >
> >Em Wed, Sep 23, 2015 at 01:03:19PM -0300, Arnaldo Carvalho de Melo escreveu:
> >> Em Tue, Sep 22, 2015 at 11:49:02PM -0300, Arnaldo Carvalho de Melo escreveu:
> >> > Em Wed, Sep 23, 2015 at 09:14:44AM +0800, Wangnan (F) escreveu:
> >> > > On 2015/9/22 21:35, Arnaldo Carvalho de Melo wrote:
> >> > > >Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
> >> > > >>This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
> >> > > >>that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
> >> > > >>iff dso is vmlinux.
> >> > > >Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
> >> > > >check that...
> >
> >> > > I also noticed this problem when I working on commit
> >> > > 1f121b03d058dd07199d8924373d3c52a207f63b ("perf tools: Deal with
> >> > > kernel module names in '[]' correctly") ;)
> >
> >> > Thanks for working on this, it is an area that needs cleaning up, too
> >> > many ways to say what a dso is, will study your findings and try to come
> >> > up with a patch proposal tomorrow.
> >> > > So care must be taken.
> >>
> >> So, yes, there are multiple uses for this dso->kernel thing, we need to
> >> look at each one and go on clarifying it so that this gets corrected and
> >> sane, but I think we need some helpers to clarify all this, namely:
> >>
> >> 	Adding DSO_TYPE_KMODULE and DSO_TYPE_GUEST_KMODULE, setting
> >> dso->kernel with it when loading host and guest kernel modules and
> >> adding:
> >>
> >> 	bool dso__is_host_kernel(struct dso *dso)
> >> 	bool dso__is_host_kmodule(struct dso *dso)
> >>
> >> 	bool dso__is_guest_kernel(struct dso *dso)
> >
> >So I tried this, ended up basically using __map__is_kernel(), in several
> >places, which I think is right, and I've stashed in my tmp.perf/core
> >branch so that you can take a look.
> >
> >But then I realized that we already have a better way to achieve what is
> >needed in that function, see the patch below, fixed things for me:
> 
> Hmm, this is a bit hacky, but yes, it looks good to me.

You mean the [] dso->short_name part? But that was already being taken
into account by the previous, dso list search.

But yeah, probably we should remove those brackets, anything requiring
full disambiguation should use the dso->long_name anyway,  but I'll
leave this for later :)

> However, I hope to have above solution, for future use.

Right, my work now is to reduce the use of dso->kernel, using
__map__is_kernel(map), we'll see what is left.
 
> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thanks!
 
> Thanks!
> 
> >
> >diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> >index 2b78e8f19b45..7fb0533ab18c 100644
> >--- a/tools/perf/util/probe-event.c
> >+++ b/tools/perf/util/probe-event.c
> >@@ -269,12 +269,13 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
> > 	int ret = 0;
> >
> > 	if (module) {
> >-		list_for_each_entry(dso, &host_machine->dsos.head, node) {
> >-			if (!dso->kernel)
> >-				continue;
> >-			if (strncmp(dso->short_name + 1, module,
> >-				    dso->short_name_len - 2) == 0)
> >-				goto found;
> >+		char module_name[128];
> >+
> >+		snprintf(module_name, sizeof(module_name), "[%s]", module);
> >+		map = map_groups__find_by_name(&host_machine->kmaps, MAP__FUNCTION, module_name);
> >+		if (map) {
> >+			dso = map->dso;
> >+			goto found;
> > 		}
> > 		pr_debug("Failed to find module %s.\n", module);
> > 		return -ENOENT;

RE: [PATCH] perf probe: Fix module probing with shortname

[平松雅巳 / HIRAMATU，MASAMI]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4122 bytes --]

From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
>> >Sent: Thursday, September 24, 2015 10:50 AM
>> >To: Wangnan (F)
>> >Cc: linux-kernel@vger.kernel.org; lizefan@huawei.com; pi3orama@163.com; Namhyung Kim; Jiri Olsa; 平松雅巳 / HIRAMATU，MASAMI
>> >Subject: Re: [PATCH] perf probe: Fix module probing with shortname
>> >
>> >Em Wed, Sep 23, 2015 at 01:03:19PM -0300, Arnaldo Carvalho de Melo escreveu:
>> >> Em Tue, Sep 22, 2015 at 11:49:02PM -0300, Arnaldo Carvalho de Melo escreveu:
>> >> > Em Wed, Sep 23, 2015 at 09:14:44AM +0800, Wangnan (F) escreveu:
>> >> > > On 2015/9/22 21:35, Arnaldo Carvalho de Melo wrote:
>> >> > > >Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
>> >> > > >>This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
>> >> > > >>that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
>> >> > > >>iff dso is vmlinux.
>> >> > > >Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
>> >> > > >check that...
>> >
>> >> > > I also noticed this problem when I working on commit
>> >> > > 1f121b03d058dd07199d8924373d3c52a207f63b ("perf tools: Deal with
>> >> > > kernel module names in '[]' correctly") ;)
>> >
>> >> > Thanks for working on this, it is an area that needs cleaning up, too
>> >> > many ways to say what a dso is, will study your findings and try to come
>> >> > up with a patch proposal tomorrow.
>> >> > > So care must be taken.
>> >>
>> >> So, yes, there are multiple uses for this dso->kernel thing, we need to
>> >> look at each one and go on clarifying it so that this gets corrected and
>> >> sane, but I think we need some helpers to clarify all this, namely:
>> >>
>> >> 	Adding DSO_TYPE_KMODULE and DSO_TYPE_GUEST_KMODULE, setting
>> >> dso->kernel with it when loading host and guest kernel modules and
>> >> adding:
>> >>
>> >> 	bool dso__is_host_kernel(struct dso *dso)
>> >> 	bool dso__is_host_kmodule(struct dso *dso)
>> >>
>> >> 	bool dso__is_guest_kernel(struct dso *dso)
>> >
>> >So I tried this, ended up basically using __map__is_kernel(), in several
>> >places, which I think is right, and I've stashed in my tmp.perf/core
>> >branch so that you can take a look.
>> >
>> >But then I realized that we already have a better way to achieve what is
>> >needed in that function, see the patch below, fixed things for me:
>>
>> Hmm, this is a bit hacky, but yes, it looks good to me.
>
>You mean the [] dso->short_name part? But that was already being taken
>into account by the previous, dso list search.

Ah, OK. I didn't know that...

>But yeah, probably we should remove those brackets, anything requiring
>full disambiguation should use the dso->long_name anyway,  but I'll
>leave this for later :)

OK, thanks,

>
>> However, I hope to have above solution, for future use.
>
>Right, my work now is to reduce the use of dso->kernel, using
>__map__is_kernel(map), we'll see what is left.
>
>> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>
>Thanks!

Thanks!
>
>> Thanks!
>>
>> >
>> >diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> >index 2b78e8f19b45..7fb0533ab18c 100644
>> >--- a/tools/perf/util/probe-event.c
>> >+++ b/tools/perf/util/probe-event.c
>> >@@ -269,12 +269,13 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
>> > 	int ret = 0;
>> >
>> > 	if (module) {
>> >-		list_for_each_entry(dso, &host_machine->dsos.head, node) {
>> >-			if (!dso->kernel)
>> >-				continue;
>> >-			if (strncmp(dso->short_name + 1, module,
>> >-				    dso->short_name_len - 2) == 0)
>> >-				goto found;
>> >+		char module_name[128];
>> >+
>> >+		snprintf(module_name, sizeof(module_name), "[%s]", module);
>> >+		map = map_groups__find_by_name(&host_machine->kmaps, MAP__FUNCTION, module_name);
>> >+		if (map) {
>> >+			dso = map->dso;
>> >+			goto found;
>> > 		}
>> > 		pr_debug("Failed to find module %s.\n", module);
>> > 		return -ENOENT;
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] perf probe: Fix module probing with shortname

[平松雅巳 / HIRAMATU，MASAMI]

From: Arnaldo Carvalho de Melo [mailto:acme@kernel.org]
>Em Wed, Sep 23, 2015 at 01:03:19PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Tue, Sep 22, 2015 at 11:49:02PM -0300, Arnaldo Carvalho de Melo escreveu:
>> > Em Wed, Sep 23, 2015 at 09:14:44AM +0800, Wangnan (F) escreveu:
>> > > On 2015/9/22 21:35, Arnaldo Carvalho de Melo wrote:
>> > > >Em Tue, Sep 22, 2015 at 03:34:32AM +0000, Wang Nan escreveu:
>> > > >>This is caused by a misunderstood of dso->kernel in kernel_get_module_dso()
>> > > >>that, for kernel module, dso->kernel is DSO_TYPE_USER. dso->kernel is DSO_TYPE_KERNEL
>> > > >>iff dso is vmlinux.
>> > > >Kernel modules having DSO_TYPE_USER seems to be the bug, no? I'll try to
>> > > >check that...
>
>> > > I also noticed this problem when I working on commit
>> > > 1f121b03d058dd07199d8924373d3c52a207f63b ("perf tools: Deal with
>> > > kernel module names in '[]' correctly") ;)
>
>> > Thanks for working on this, it is an area that needs cleaning up, too
>> > many ways to say what a dso is, will study your findings and try to come
>> > up with a patch proposal tomorrow.
>> > > So care must be taken.
>>
>> So, yes, there are multiple uses for this dso->kernel thing, we need to
>> look at each one and go on clarifying it so that this gets corrected and
>> sane, but I think we need some helpers to clarify all this, namely:
>>
>> 	Adding DSO_TYPE_KMODULE and DSO_TYPE_GUEST_KMODULE, setting
>> dso->kernel with it when loading host and guest kernel modules and
>> adding:
>>
>> 	bool dso__is_host_kernel(struct dso *dso)
>> 	bool dso__is_host_kmodule(struct dso *dso)
>>
>> 	bool dso__is_guest_kernel(struct dso *dso)
>
>So I tried this, ended up basically using __map__is_kernel(), in several
>places, which I think is right, and I've stashed in my tmp.perf/core
>branch so that you can take a look.
>
>But then I realized that we already have a better way to achieve what is
>needed in that function, see the patch below, fixed things for me:


Hmm, this is a bit hacky, but yes, it looks good to me.
However, I hope to have above solution, for future use.

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

Thanks!

>

>
>diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>index 2b78e8f19b45..7fb0533ab18c 100644
>--- a/tools/perf/util/probe-event.c
>+++ b/tools/perf/util/probe-event.c
>@@ -269,12 +269,13 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
> 	int ret = 0;
>
> 	if (module) {
>-		list_for_each_entry(dso, &host_machine->dsos.head, node) {
>-			if (!dso->kernel)
>-				continue;
>-			if (strncmp(dso->short_name + 1, module,
>-				    dso->short_name_len - 2) == 0)
>-				goto found;
>+		char module_name[128];
>+
>+		snprintf(module_name, sizeof(module_name), "[%s]", module);
>+		map = map_groups__find_by_name(&host_machine->kmaps, MAP__FUNCTION, module_name);
>+		if (map) {
>+			dso = map->dso;
>+			goto found;
> 		}
> 		pr_debug("Failed to find module %s.\n", module);
> 		return -ENOENT;

[tip:perf/urgent] perf probe: Use existing routine to look for a kernel module by dso->short_name

[tip-bot for Arnaldo Carvalho de Melo]

Commit-ID:  266fa2b22294909ddf6e7d2f8acfe07adf9fd978
Gitweb:     http://git.kernel.org/tip/266fa2b22294909ddf6e7d2f8acfe07adf9fd978
Author:     Arnaldo Carvalho de Melo <acme@redhat.com>
AuthorDate: Thu, 24 Sep 2015 11:24:18 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 25 Sep 2015 10:41:31 -0300

perf probe: Use existing routine to look for a kernel module by dso->short_name

We have map_groups__find_by_name() to look at the list of modules that
are in place for a given machine, so use it instead of traversing the
machine dso list, which also includes DSOs for userspace.

When merging the user and kernel DSO lists a bug was introduced where
'perf probe' stopped being able to add probes to modules using its short
name:

  # perf probe -m usbnet --add usbnet_start_xmit
  usbnet_start_xmit is out of .text, skip it.
    Error: Failed to add events.
  #

With this fix it works again:

  # perf probe -m usbnet --add usbnet_start_xmit
  Added new event:
    probe:usbnet_start_xmit (on usbnet_start_xmit in usbnet)

  You can now use it in all perf tools, such as:

  	perf record -e probe:usbnet_start_xmit -aR sleep 1
  #

Reported-by: Wang Nan <wangnan0@huawei.com>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Fixes: 3d39ac538629 ("perf machine: No need to have two DSOs lists")
Link: http://lkml.kernel.org/r/20150924015008.GE1897@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/probe-event.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index eb5f18b..c6f9af7 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -270,12 +270,13 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
 	int ret = 0;
 
 	if (module) {
-		list_for_each_entry(dso, &host_machine->dsos.head, node) {
-			if (!dso->kernel)
-				continue;
-			if (strncmp(dso->short_name + 1, module,
-				    dso->short_name_len - 2) == 0)
-				goto found;
+		char module_name[128];
+
+		snprintf(module_name, sizeof(module_name), "[%s]", module);
+		map = map_groups__find_by_name(&host_machine->kmaps, MAP__FUNCTION, module_name);
+		if (map) {
+			dso = map->dso;
+			goto found;
 		}
 		pr_debug("Failed to find module %s.\n", module);
 		return -ENOENT;