[PATCH] perf-probe: make "perf-probe -L <function>" display the absolute path and absolute line number

[PATCH] perf-probe: make "perf-probe -L <function>" display the absolute path and absolute line number

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

It should be more usefull to get the full location of the function
(absolute line number + full path) instead of repeating the name of
the function and the start line number given by the command line.

So we had before:

   $ perf probe -L schedule | head -n3
   <schedule:0>
         0  asmlinkage void __sched schedule(void)
         1  {

and now we get:

   $ perf probe -L schedule | head -n3
   </usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:3813>
         0  asmlinkage void __sched schedule(void)
         1  {

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 tools/perf/util/probe-event.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index fcbb543..bd09b87 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -382,11 +382,7 @@ int show_line_range(struct line_range *lr, const char *module)
 
 	setup_pager();
 
-	if (lr->function)
-		fprintf(stdout, "<%s:%d>\n", lr->function,
-			lr->start - lr->offset);
-	else
-		fprintf(stdout, "<%s:%d>\n", lr->path, lr->start);
+	fprintf(stdout, "<%s:%d>\n", lr->path, lr->start);
 
 	fp = fopen(lr->path, "r");
 	if (fp == NULL) {
-- 
1.7.3.2

Re: [PATCH] perf-probe: make "perf-probe -L <function>" display the absolute path and absolute line number

[Masami Hiramatsu]

(2011/01/13 19:20), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <fbuihuu@gmail.com>
> 
> It should be more usefull to get the full location of the function
> (absolute line number + full path) instead of repeating the name of
> the function and the start line number given by the command line.
> 
> So we had before:
> 
>    $ perf probe -L schedule | head -n3
>    <schedule:0>
>          0  asmlinkage void __sched schedule(void)
>          1  {
> 
> and now we get:
> 
>    $ perf probe -L schedule | head -n3
>    </usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:3813>
>          0  asmlinkage void __sched schedule(void)
>          1  {

Indeed, it could be useful for users to see where the function is...

However, I think that should be optional, because the output lines
have the relative line numbers from the function, and those numbers
are important for users who want to probe a specific line by using
function relative line numbers. e.g. "schedule:10"

And with that option, I'd suggest to show absolute line numbers on each line.

  $ perf probe -L schedule:0-1 --by-source
  </usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:3813>
     3813  asmlinkage void __sched schedule(void)
     3814  {

Or, just show source file as an additional information.

  $ perf probe -L schedule:0-1
  <schedule@/usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:0>
        0  asmlinkage void __sched schedule(void)
        1  {

I just would like to keep the consistency of the output/input format.
In above cases, users can define a new event just by replacing the last
number with the line number which they see.

Thank you,

> 
> Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
> ---
>  tools/perf/util/probe-event.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index fcbb543..bd09b87 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -382,11 +382,7 @@ int show_line_range(struct line_range *lr, const char *module)
>  
>  	setup_pager();
>  
> -	if (lr->function)
> -		fprintf(stdout, "<%s:%d>\n", lr->function,
> -			lr->start - lr->offset);
> -	else
> -		fprintf(stdout, "<%s:%d>\n", lr->path, lr->start);
> +	fprintf(stdout, "<%s:%d>\n", lr->path, lr->start);
>  
>  	fp = fopen(lr->path, "r");
>  	if (fp == NULL) {


-- 
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH] perf-probe: make "perf-probe -L <function>" display the absolute path and absolute line number

[Franck Bui-Huu]

Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:

> (2011/01/13 19:20), Franck Bui-Huu wrote:
>> From: Franck Bui-Huu <fbuihuu@gmail.com>
>> 
>> It should be more usefull to get the full location of the function
>
>> (absolute line number + full path) instead of repeating the name of
>> the function and the start line number given by the command line.
>> 
>> So we had before:
>> 
>>    $ perf probe -L schedule | head -n3
>>    <schedule:0>
>>          0  asmlinkage void __sched schedule(void)
>>          1  {
>> 
>> and now we get:
>> 
>>    $ perf probe -L schedule | head -n3
>>    </usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:3813>
>>          0  asmlinkage void __sched schedule(void)
>>          1  {
>
> Indeed, it could be useful for users to see where the function is...
>
> However, I think that should be optional, because the output lines
> have the relative line numbers from the function, and those numbers
> are important for users who want to probe a specific line by using
> function relative line numbers. e.g. "schedule:10"
>
> And with that option, I'd suggest to show absolute line numbers on each line.
>
>   $ perf probe -L schedule:0-1 --by-source
>   </usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:3813>
>      3813  asmlinkage void __sched schedule(void)
>      3814  {
>
> Or, just show source file as an additional information.
>
>   $ perf probe -L schedule:0-1
>   <schedule@/usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:0>
>         0  asmlinkage void __sched schedule(void)
>         1  {
>
> I just would like to keep the consistency of the output/input format.

Well, for consistency, I thought that the additional information (given
inside angle brackets) should always be the same: a full path and an
absolute line number which clearly identify which source file perf-probe
is listing.

-- 
		Franck

Re: [PATCH] perf-probe: make "perf-probe -L <function>" display the absolute path and absolute line number

[Masami Hiramatsu]

(2011/01/14 4:42), Franck Bui-Huu wrote:
 Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:
> 
>> (2011/01/13 19:20), Franck Bui-Huu wrote:
>>> From: Franck Bui-Huu <fbuihuu@gmail.com>
>>>
>>> It should be more usefull to get the full location of the function
>>
>>> (absolute line number + full path) instead of repeating the name of
>>> the function and the start line number given by the command line.
>>>
>>> So we had before:
>>>
>>>    $ perf probe -L schedule | head -n3
>>>    <schedule:0>
>>>          0  asmlinkage void __sched schedule(void)
>>>          1  {
>>>
>>> and now we get:
>>>
>>>    $ perf probe -L schedule | head -n3
>>>    </usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:3813>
>>>          0  asmlinkage void __sched schedule(void)
>>>          1  {
>>
>> Indeed, it could be useful for users to see where the function is...
>>
>> However, I think that should be optional, because the output lines
>> have the relative line numbers from the function, and those numbers
>> are important for users who want to probe a specific line by using
>> function relative line numbers. e.g. "schedule:10"
>>
>> And with that option, I'd suggest to show absolute line numbers on each line.
>>
>>   $ perf probe -L schedule:0-1 --by-source
>>   </usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:3813>
>>      3813  asmlinkage void __sched schedule(void)
>>      3814  {
>>
>> Or, just show source file as an additional information.
>>
>>   $ perf probe -L schedule:0-1
>>   <schedule@/usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:0>
>>         0  asmlinkage void __sched schedule(void)
>>         1  {
>>
>> I just would like to keep the consistency of the output/input format.
> 
> Well, for consistency, I thought that the additional information (given
> inside angle brackets) should always be the same: a full path and an
> absolute line number which clearly identify which source file perf-probe
> is listing.

No, that is NOT an additional information. That indicates from where those
lines are started, and also gives you a hint how you can specify the
actual probe point. For example,

$ perf probe -L schedule:10
<schedule:10>
     10         rq = cpu_rq(cpu);
     11         rcu_note_context_switch(cpu);
     12         prev = rq->curr;

this indicates the lines started from 10th line of schedule(), and
if you want to put a new event on "prev = rq->curr;" line, you
just need to say "perf probe schedule:12"

$ perf probe -L kernel/sched.c:4077
</home/mhiramat/ksrc/linux-2.6-tip/kernel/sched.c:4077>
   4077         rq = cpu_rq(cpu);
   4078         rcu_note_context_switch(cpu);
   4079         prev = rq->curr;

And this also gives you a hint to say (just copy & paste)
"perf probe /home/mhiramat/ksrc/linux-2.6-tip/kernel/sched.c:4079"

Since perf probe also accepts FUNC@SRC:RLN, my suggestion
also keeps that rule.
---
  $ perf probe -L schedule:0-1
  <schedule@/usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:0>
        0  asmlinkage void __sched schedule(void)
        1  {
---

Thank you,

-- 
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com>

Re: [PATCH] perf-probe: make "perf-probe -L <function>" display the absolute path and absolute line number

[Franck Bui-Huu]

Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:

> (2011/01/14 4:42), Franck Bui-Huu wrote:

[...]

>> Well, for consistency, I thought that the additional information
>> (given inside angle brackets) should always be the same: a full path
>> and an absolute line number which clearly identify which source file
>> perf-probe is listing.
>
> No, that is NOT an additional information. That indicates from where
> those lines are started

But this indication is currently not enough, it's too ambiguous to be
usefull:

        <schedule:10>

there might be several 'schedule()' functions inside the kernel, also we
don't know which kernel tree is used.

BTW, if there're several 'do_something' functions defined inside the
kernel, what is the behaviour of:

  $ perf probe do_something

Does it set a probe to all 'do_something' functions ? I don't think it's
documented.

> , and also gives you a hint how you can specify the actual probe
> point. For example,
>
> $ perf probe -L schedule:10
> <schedule:10>
>      10         rq = cpu_rq(cpu);
>      11         rcu_note_context_switch(cpu);
>      12         prev = rq->curr;
>
> this indicates the lines started from 10th line of schedule(), and

but I'm not sure it's really usefull since you've asked for it when
invoking perf-probe so it seems to be redundant.

> if you want to put a new event on "prev = rq->curr;" line, you just
> need to say "perf probe schedule:12"

yes and this patch doesn't change that, does it ?

> $ perf probe -L kernel/sched.c:4077
> </home/mhiramat/ksrc/linux-2.6-tip/kernel/sched.c:4077>
>    4077         rq = cpu_rq(cpu);
>    4078         rcu_note_context_switch(cpu);
>    4079         prev = rq->curr;
>
> And this also gives you a hint to say (just copy & paste)

No copy & paste in my case, I just reuse the shell history. This is the
reason why I didn't understand your suggestion.

So to get back to my initial suggestion and with this patch applied:

   $ perf probe -L schedule:10-15
   </usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:3823>
        10         rq = cpu_rq(cpu);
        11         rcu_note_context_switch(cpu);
        12         prev = rq->curr;
        13         switch_count = &prev->nivcsw;
            
        15         release_kernel_lock(prev);

I know exactly which bit of code I'm looking at, and to put a probe I
can still simply do (without any copy and paste):

    $ perf probe schedule:12

I still use the function name of the probe and the relative line number
showed by perf-probe.

For probe specified by filename:

    $ perf probe -L kernel/sched.c:3823-3826
    </usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:3823>
       3823         rq = cpu_rq(cpu);
       3824         rcu_note_context_switch(cpu);
       3825         prev = rq->curr;
       3826         switch_count = &prev->nivcsw;

now to set a probe, I would do:

    $ perf probe kernel/sched.c:3825

I still use the linenum showed by perf-probe, and the additional
information is always consistent and still usefull since it shows which
tree perf-probe is using.

But if you think it should be used to hint for a probe point syntax,
(you'll probably use copy & paste since it uses absolute path name),
then this patch is wrong.

Thanks
-- 
		Franck

Re: [PATCH] perf-probe: make "perf-probe -L <function>" display the absolute path and absolute line number

[Masami Hiramatsu]

(2011/01/14 18:03), Franck Bui-Huu wrote:
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:
> 
>> (2011/01/14 4:42), Franck Bui-Huu wrote:
> 
> [...]
> 
>>> Well, for consistency, I thought that the additional information
>>> (given inside angle brackets) should always be the same: a full path
>>> and an absolute line number which clearly identify which source file
>>> perf-probe is listing.
>>
>> No, that is NOT an additional information. That indicates from where
>> those lines are started
> 
> But this indication is currently not enough, it's too ambiguous to be
> usefull:
> 
>         <schedule:10>
> 
> there might be several 'schedule()' functions inside the kernel, also we
> don't know which kernel tree is used.

Right, so I didn't decline your idea itself.
I've just commented on the output format of the path information.


> BTW, if there're several 'do_something' functions defined inside the
> kernel, what is the behaviour of:
> 
>   $ perf probe do_something
> 
> Does it set a probe to all 'do_something' functions ? I don't think it's
> documented.

If do_something is an inline function, it tries to find all
functions. If it (the first one) is a normal function, it
just stopped after finding the first one.

If someone wants to put a probe on the specific function,
he can add some additional path information, like

 $ perf probe do_something@path/to/source.c


>> , and also gives you a hint how you can specify the actual probe
>> point. For example,
>>
>> $ perf probe -L schedule:10
>> <schedule:10>
>>      10         rq = cpu_rq(cpu);
>>      11         rcu_note_context_switch(cpu);
>>      12         prev = rq->curr;
>>
>> this indicates the lines started from 10th line of schedule(), and
> 
> but I'm not sure it's really usefull since you've asked for it when
> invoking perf-probe so it seems to be redundant.

If you list the lines of enough long function, the output will be
passed to pager and it flushes out the command line.

In that case, user will see only below output.

   </usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:3823>
        10         rq = cpu_rq(cpu);
        11         rcu_note_context_switch(cpu);
        12         prev = rq->curr;
        13         switch_count = &prev->nivcsw;

        15         release_kernel_lock(prev);

Hmm, how can he find the probe point from this? :)
Yeah, I know user must know what he has given. However, I'd like to
show users what actually he has given.

Thanks,

-- 
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH] perf-probe: make "perf-probe -L <function>" display the absolute path and absolute line number

[Masami Hiramatsu]

Hi Franck,

(2011/01/14 18:03), Franck Bui-Huu wrote:
> But if you think it should be used to hint for a probe point syntax,
> (you'll probably use copy & paste since it uses absolute path name),

Not only for copy&paste, but also for avoiding confusion.
Please imagine that user see this output.

   </usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:3823>
        10         rq = cpu_rq(cpu);
        11         rcu_note_context_switch(cpu);

"Why the definition line number and starting line number are different?"
"How I can put a probe?  sched.c:3823 ... + 10??"
"Or, it's just buggy! report it!"

I don't want this.

> then this patch is wrong.

Yeah, but just a _bit_. Basically, I agree with your idea of showing
actual path of the function, because, indeed, perf-probe just shows
the first one even if there are many same-name functions.

So, here, I had suggested an enhancement idea;

  $ perf probe -L schedule:0-1
  <schedule@/usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:0>
        0  asmlinkage void __sched schedule(void)
        1  {

This is acceptable, because it shows which function you see and
it also gives you how you can specify a probe point on a function
line.

Thank you,

-- 
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH] perf-probe: make "perf-probe -L <function>" display the absolute path and absolute line number

[Franck Bui-Huu]

Hi Masami,

Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> writes:

> (2011/01/14 18:03), Franck Bui-Huu wrote:
>> But if you think it should be used to hint for a probe point syntax,
>> (you'll probably use copy & paste since it uses absolute path name),
>
> Not only for copy&paste, but also for avoiding confusion.
> Please imagine that user see this output.
>
>    </usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:3823>
>         10         rq = cpu_rq(cpu);
>         11         rcu_note_context_switch(cpu);
>
> "Why the definition line number and starting line number are different?"
> "How I can put a probe?  sched.c:3823 ... + 10??"

No because if you see relative line number, then the user had used the
following command:

   $ perf probe -L schedule

so I would assume that his next add command would still use a function
name:

   $ perf probe schedule:10

> 
> "Or, it's just buggy! report it!"
>
> I don't want this.
>
>> then this patch is wrong.
>
> Yeah, but just a _bit_. Basically, I agree with your idea of showing
> actual path of the function, because, indeed, perf-probe just shows
> the first one even if there are many same-name functions.
>
> So, here, I had suggested an enhancement idea;
>
>   $ perf probe -L schedule:0-1
>   <schedule@/usr/src/debug/kernel-2.6.35.fc14/linux-2.6.35.x86_64/kernel/sched.c:0>
>         0  asmlinkage void __sched schedule(void)
>         1  {
>
> This is acceptable, because it shows which function you see and
> it also gives you how you can specify a probe point on a function
> line.

I can understand your point, but I'm currently not seeing it as a hint
for a probe point syntax.

But you're a far more experienced user than I am so you're probably
right.

Thanks
-- 
		Franck