[PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[Dongsheng Yang]

Currently, if we use perf kvm --guestkallsyms --guestmodules report,
we can not get the perf information from perf data file. The all sample
are shown as unknown.

Reproducing steps:
	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.624 MB perf.data.guest (~27260 samples) ]
	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules report |grep %
	   100.00%  [guest/6471]  [unknown]         [g] 0xffffffff8164f330

This bug is caused when delivery sample. We use perf_session__find_machine_for_cpumode() to find
the machine, and it call perf_session__findnew_machine(). Then we will delivery the sample to a
new machine without kernel mapped.

This patch changes perf_session__findnew_machine() to perf_session__find_machine(), so the sample
will be deliveried to default guest machine if there is no machine specified for it.

Verify steps:
	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.651 MB perf.data.guest (~28437 samples) ]
	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules report |grep %
	    22.64%    :6471  [guest.kernel.kallsyms]  [g] update_rq_clock.part.70
	    19.99%    :6471  [guest.kernel.kallsyms]  [g] d_free
	    18.46%    :6471  [guest.kernel.kallsyms]  [g] bio_phys_segments
	    16.25%    :6471  [guest.kernel.kallsyms]  [g] dequeue_task
	    12.78%    :6471  [guest.kernel.kallsyms]  [g] __switch_to
	     7.91%    :6471  [guest.kernel.kallsyms]  [g] scheduler_tick
	     1.75%    :6471  [guest.kernel.kallsyms]  [g] native_apic_mem_write
	     0.21%    :6471  [guest.kernel.kallsyms]  [g] apic_timer_interrupt

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/util/session.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 8a7da6f..1770f2f 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -851,6 +851,7 @@ static struct machine *
 					       struct perf_sample *sample)
 {
 	const u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
+	struct machine *machine;
 
 	if (perf_guest &&
 	    ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
@@ -863,7 +864,11 @@ static struct machine *
 		else
 			pid = sample->pid;
 
-		return perf_session__findnew_machine(session, pid);
+		machine = perf_session__find_machine(session, pid);
+		if (!machine)
+			machine = perf_session__findnew_machine(session,
+						DEFAULT_GUEST_KERNEL_ID);
+		return machine;
 	}
 
 	return &session->machines.host;
-- 
1.8.2.1

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[Arnaldo Carvalho de Melo]

Em Fri, Dec 06, 2013 at 04:33:08PM -0500, Dongsheng Yang escreveu:
> Currently, if we use perf kvm --guestkallsyms --guestmodules report,
> we can not get the perf information from perf data file. The all sample
> are shown as unknown.
> 
> Reproducing steps:
> 	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules record -a sleep 1
> 	[ perf record: Woken up 1 times to write data ]
> 	[ perf record: Captured and wrote 0.624 MB perf.data.guest (~27260 samples) ]
> 	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules report |grep %
> 	   100.00%  [guest/6471]  [unknown]         [g] 0xffffffff8164f330
> 
> This bug is caused when delivery sample. We use perf_session__find_machine_for_cpumode() to find
> the machine, and it call perf_session__findnew_machine(). Then we will delivery the sample to a
> new machine without kernel mapped.
> 
> This patch changes perf_session__findnew_machine() to perf_session__find_machine(), so the sample
> will be deliveried to default guest machine if there is no machine specified for it.

Ok, but then we will use the provided guest modules, etc for _all_ VMs,
irrespective of its pid, which sounds risky, no?

You get correct results because the modules/kallsyms info you provided
matches the VM you're testing, i.e. the pid test is not needed.

I think we need to specify for which PID the --guestmodules and
--guestkallsyms is provided, so that it can refuse to use it for pids
where it is invalid.

If we do that then the current code will find it with
perf_session__find_machine_for_cpumode(), and when not finding it, it
will behave as expected and as it does today: refuse to resolve symbols
using invalid kallsyms/modules.

One way to achieve this automagically would be to have the build-id of
the guest kernel and modules be present on the PERF_RECORD_MMAP, so that
we wouldn't need to provide --guest{kallsyms,modules} as it would look
it up (kallsyms or vmlinux) in the ~/.debug or elsewhere, like in
/usr/lib/debug/lib/modules/3.11.4-101.fc18.x86_64/vmlinux (where
kernel-debuginfo packages install its contents.

Untill we have that, we need to at least document this limitation, i.e.
that great care must be taken to provide exactly the kallsyms/modules
pair to 'perf kvm', otherwise bogus results will be presented.

- Arnaldo
 
> Verify steps:
> 	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
> 	[ perf record: Woken up 1 times to write data ]
> 	[ perf record: Captured and wrote 0.651 MB perf.data.guest (~28437 samples) ]
> 	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules report |grep %
> 	    22.64%    :6471  [guest.kernel.kallsyms]  [g] update_rq_clock.part.70
> 	    19.99%    :6471  [guest.kernel.kallsyms]  [g] d_free
> 	    18.46%    :6471  [guest.kernel.kallsyms]  [g] bio_phys_segments
> 	    16.25%    :6471  [guest.kernel.kallsyms]  [g] dequeue_task
> 	    12.78%    :6471  [guest.kernel.kallsyms]  [g] __switch_to
> 	     7.91%    :6471  [guest.kernel.kallsyms]  [g] scheduler_tick
> 	     1.75%    :6471  [guest.kernel.kallsyms]  [g] native_apic_mem_write
> 	     0.21%    :6471  [guest.kernel.kallsyms]  [g] apic_timer_interrupt
> 
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>  tools/perf/util/session.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 8a7da6f..1770f2f 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -851,6 +851,7 @@ static struct machine *
>  					       struct perf_sample *sample)
>  {
>  	const u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
> +	struct machine *machine;
>  
>  	if (perf_guest &&
>  	    ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
> @@ -863,7 +864,11 @@ static struct machine *
>  		else
>  			pid = sample->pid;
>  
> -		return perf_session__findnew_machine(session, pid);
> +		machine = perf_session__find_machine(session, pid);
> +		if (!machine)
> +			machine = perf_session__findnew_machine(session,
> +						DEFAULT_GUEST_KERNEL_ID);
> +		return machine;
>  	}
>  
>  	return &session->machines.host;
> -- 
> 1.8.2.1

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[David Ahern]

On 12/6/13, 4:56 AM, Arnaldo Carvalho de Melo wrote:
> You get correct results because the modules/kallsyms info you provided
> matches the VM you're testing, i.e. the pid test is not needed.
>
> I think we need to specify for which PID the --guestmodules and
> --guestkallsyms is provided, so that it can refuse to use it for pids
> where it is invalid.

Those are arguments are shortcut defaults for guests. If the VMs are not 
running the same kernel use guestmount and pid specific directories. e.g.,

/tmp/guestmount/<pid>/proc/kallsyms

David

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[Arnaldo Carvalho de Melo]

Em Sun, Dec 08, 2013 at 10:39:36AM -0700, David Ahern escreveu:
> On 12/6/13, 4:56 AM, Arnaldo Carvalho de Melo wrote:
> >You get correct results because the modules/kallsyms info you provided
> >matches the VM you're testing, i.e. the pid test is not needed.
> >
> >I think we need to specify for which PID the --guestmodules and
> >--guestkallsyms is provided, so that it can refuse to use it for pids
> >where it is invalid.
> 
> Those are arguments are shortcut defaults for guests. If the VMs are
> not running the same kernel use guestmount and pid specific
> directories. e.g.,
> 
> /tmp/guestmount/<pid>/proc/kallsyms

Does this already works like that? /me tries to figure that out...

- Arnaldo

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[David Ahern]

On 12/9/13, 11:07 AM, Arnaldo Carvalho de Melo wrote:
> Em Sun, Dec 08, 2013 at 10:39:36AM -0700, David Ahern escreveu:
>> On 12/6/13, 4:56 AM, Arnaldo Carvalho de Melo wrote:
>>> You get correct results because the modules/kallsyms info you provided
>>> matches the VM you're testing, i.e. the pid test is not needed.
>>>
>>> I think we need to specify for which PID the --guestmodules and
>>> --guestkallsyms is provided, so that it can refuse to use it for pids
>>> where it is invalid.
>>
>> Those are arguments are shortcut defaults for guests. If the VMs are
>> not running the same kernel use guestmount and pid specific
>> directories. e.g.,
>>
>> /tmp/guestmount/<pid>/proc/kallsyms
>
> Does this already works like that? /me tries to figure that out...

yes.

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[Arnaldo Carvalho de Melo]

Em Mon, Dec 09, 2013 at 11:08:05AM -0700, David Ahern escreveu:
> On 12/9/13, 11:07 AM, Arnaldo Carvalho de Melo wrote:
> >Em Sun, Dec 08, 2013 at 10:39:36AM -0700, David Ahern escreveu:
> >>On 12/6/13, 4:56 AM, Arnaldo Carvalho de Melo wrote:
> >>>You get correct results because the modules/kallsyms info you provided
> >>>matches the VM you're testing, i.e. the pid test is not needed.
> >>>
> >>>I think we need to specify for which PID the --guestmodules and
> >>>--guestkallsyms is provided, so that it can refuse to use it for pids
> >>>where it is invalid.
> >>
> >>Those are arguments are shortcut defaults for guests. If the VMs are
> >>not running the same kernel use guestmount and pid specific
> >>directories. e.g.,
> >>
> >>/tmp/guestmount/<pid>/proc/kallsyms
> >
> >Does this already works like that? /me tries to figure that out...
> 
> yes.
> 

Yeah, I saw that:

        if ((pid != HOST_KERNEL_ID) &&
            (pid != DEFAULT_GUEST_KERNEL_ID) &&
            (symbol_conf.guestmount)) {
                sprintf(path, "%s/%d", symbol_conf.guestmount, pid);

So, in summary, ack/nack this patch?

- Arnaldo

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[David Ahern]

On 12/9/13, 11:49 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Dec 09, 2013 at 11:08:05AM -0700, David Ahern escreveu:
>> On 12/9/13, 11:07 AM, Arnaldo Carvalho de Melo wrote:
>>> Em Sun, Dec 08, 2013 at 10:39:36AM -0700, David Ahern escreveu:
>>>> On 12/6/13, 4:56 AM, Arnaldo Carvalho de Melo wrote:
>>>>> You get correct results because the modules/kallsyms info you provided
>>>>> matches the VM you're testing, i.e. the pid test is not needed.
>>>>>
>>>>> I think we need to specify for which PID the --guestmodules and
>>>>> --guestkallsyms is provided, so that it can refuse to use it for pids
>>>>> where it is invalid.
>>>>
>>>> Those are arguments are shortcut defaults for guests. If the VMs are
>>>> not running the same kernel use guestmount and pid specific
>>>> directories. e.g.,
>>>>
>>>> /tmp/guestmount/<pid>/proc/kallsyms
>>>
>>> Does this already works like that? /me tries to figure that out...
>>
>> yes.
>>
>
> Yeah, I saw that:
>
>          if ((pid != HOST_KERNEL_ID) &&
>              (pid != DEFAULT_GUEST_KERNEL_ID) &&
>              (symbol_conf.guestmount)) {
>                  sprintf(path, "%s/%d", symbol_conf.guestmount, pid);
>
> So, in summary, ack/nack this patch?
>

NACK. Not needed and breaks the very intent of those 2 options.

David

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[Dongsheng Yang]

> NACK. Not needed and breaks the very intent of those 2 options.
>
> David
> -- 

Okey, David, I saw your commit 207b57926 (perf kvm: Fix regression with 
guest machine creation) here. It is applied to fix a bug of SEGFAULT. I 
have to say that it changed the behavior of report and lead to current bug.

commit 207b5792696206663a38e525b9793644895bad3b
Author: David Ahern <dsahern@gmail.com>
Date:   Sun Jul 1 16:11:37 2012 -0600

     perf kvm: Fix regression with guest machine creation


diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index c3e399b..56142d0 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -926,7 +926,7 @@ static struct machine *
                 else
                         pid = event->ip.pid;

-               return perf_session__find_machine(session, pid);
+               return perf_session__findnew_machine(session, pid);
         }

         return perf_session__find_host_machine(session);

In original, it uses perf_session__find_machine(), it means we deliver 
symbol to machine which has the same pid, if no machine found, deliver 
it to *default* guest. But if we use perf_session__findnew_machine() 
here, if no machine was found, new machine with pid will be built and 
added. Then the default guest which with pid == 0 will never get a 
symbol, right?

And because the new machine initialized here has no kernel map created, 
the symbol delivered to it will be marked as "unknown".

So, my patch here is to revert commit 207b57926 and fix the SEGFAULT bug 
in another way.

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 8a7da6f..1770f2f 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -851,6 +851,7 @@ static struct machine *
                                                struct perf_sample *sample)
  {
         const u8 cpumode = event->header.misc & 
PERF_RECORD_MISC_CPUMODE_MASK;
+       struct machine *machine;

         if (perf_guest &&
             ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
@@ -863,7 +864,11 @@ static struct machine *
                 else
                         pid = sample->pid;

-               return perf_session__findnew_machine(session, pid);
+               machine = perf_session__find_machine(session, pid);
+               if (!machine)
+                       machine = perf_session__findnew_machine(session,
+ DEFAULT_GUEST_KERNEL_ID);
+               return machine;
         }

         return &session->machines.host;

If there is a machine for this event or sample, return this machine. If 
no, return *default* guest.

About the SEGFAULT bug you mentioned, it is solved by if (!machine).

Please correct me if I am wrong, thanx !!

- Yang

> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[David Ahern]

On 12/10/13, 11:54 AM, Dongsheng Yang wrote:
> Okey, David, I saw your commit 207b57926 (perf kvm: Fix regression with
> guest machine creation) here. It is applied to fix a bug of SEGFAULT. I
> have to say that it changed the behavior of report and lead to current bug.


Dongsheng, I appreciate your perseverance here. I am trying to survive 
this week due to a lot of demands on my time. Next week or the week 
after I will revisit your questions and comments on perf-kvm.

David

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[Dongsheng Yang]

On 12/10/2013 01:02 AM, David Ahern wrote:
> On 12/10/13, 11:54 AM, Dongsheng Yang wrote:
>> Okey, David, I saw your commit 207b57926 (perf kvm: Fix regression with
>> guest machine creation) here. It is applied to fix a bug of SEGFAULT. I
>> have to say that it changed the behavior of report and lead to 
>> current bug.
>
>
> Dongsheng, I appreciate your perseverance here. I am trying to survive 
> this week due to a lot of demands on my time. Next week or the week 
> after I will revisit your questions and comments on perf-kvm.

It is all right to me :).

Thanx !!

Yang
>
> David
> -- 
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

[PATCH V2] perf tools: Fix bug for perf kvm report without guestmount.

[Dongsheng Yang]

Currently, if we use perf kvm --guestkallsyms --guestmodules report,
we can not get the perf information from perf data file. The all sample
are shown as unknown.

Reproducing steps:
	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.624 MB perf.data.guest (~27260 samples) ]
	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules report |grep %
	   100.00%  [guest/6471]  [unknown]         [g] 0xffffffff8164f330

This bug was introduced by 207b57926 (perf kvm: Fix regression with guest machine creation).

commit 207b5792696206663a38e525b9793644895bad3b
Author: David Ahern <dsahern@gmail.com>
Date:   Sun Jul 1 16:11:37 2012 -0600

    perf kvm: Fix regression with guest machine creation

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index c3e399b..56142d0 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -926,7 +926,7 @@ static struct machine *
                else
                        pid = event->ip.pid;

-               return perf_session__find_machine(session, pid);
+               return perf_session__findnew_machine(session, pid);
        }

        return perf_session__find_host_machine(session);
In original code, it uses perf_session__find_machine(), it means we deliver symbol to machine
which has the same pid, if no machine found, deliver it to *default* guest. But if we use
perf_session__findnew_machine() here, if no machine was found, new machine with pid will be built
and added. Then the default guest which with pid == 0 will never get a symbol.

And because the new machine initialized here has no kernel map created, the symbol delivered to
it will be marked as "unknown".

This patch here is to revert commit 207b57926 and fix the SEGFAULT bug in another way.

Verification steps:
	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.651 MB perf.data.guest (~28437 samples) ]
	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules report |grep %
	    22.64%    :6471  [guest.kernel.kallsyms]  [g] update_rq_clock.part.70
	    19.99%    :6471  [guest.kernel.kallsyms]  [g] d_free
	    18.46%    :6471  [guest.kernel.kallsyms]  [g] bio_phys_segments
	    16.25%    :6471  [guest.kernel.kallsyms]  [g] dequeue_task
	    12.78%    :6471  [guest.kernel.kallsyms]  [g] __switch_to
	     7.91%    :6471  [guest.kernel.kallsyms]  [g] scheduler_tick
	     1.75%    :6471  [guest.kernel.kallsyms]  [g] native_apic_mem_write
	     0.21%    :6471  [guest.kernel.kallsyms]  [g] apic_timer_interrupt

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
    Changelog since V1:
        * More commit message.

 tools/perf/util/session.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 989b2e3..a12dfdd 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -830,6 +830,7 @@ static struct machine *
 					       struct perf_sample *sample)
 {
 	const u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
+	struct machine *machine;
 
 	if (perf_guest &&
 	    ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
@@ -842,7 +843,11 @@ static struct machine *
 		else
 			pid = sample->pid;
 
-		return perf_session__findnew_machine(session, pid);
+		machine = perf_session__find_machine(session, pid);
+		if (!machine)
+			machine = perf_session__findnew_machine(session,
+						DEFAULT_GUEST_KERNEL_ID);
+		return machine;
 	}
 
 	return &session->machines.host;
-- 
1.8.2.1

Re: [PATCH V2] perf tools: Fix bug for perf kvm report without guestmount.

[Dongsheng Yang]

David,
     Could you please help to review this patch when you have a time ?

     Thanx in advance

- Yang

On 12/16/2013 11:26 AM, Dongsheng Yang wrote:
> Currently, if we use perf kvm --guestkallsyms --guestmodules report,
> we can not get the perf information from perf data file. The all sample
> are shown as unknown.
>
> Reproducing steps:
> 	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules record -a sleep 1
> 	[ perf record: Woken up 1 times to write data ]
> 	[ perf record: Captured and wrote 0.624 MB perf.data.guest (~27260 samples) ]
> 	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules report |grep %
> 	   100.00%  [guest/6471]  [unknown]         [g] 0xffffffff8164f330
>
> This bug was introduced by 207b57926 (perf kvm: Fix regression with guest machine creation).
>
> commit 207b5792696206663a38e525b9793644895bad3b
> Author: David Ahern <dsahern@gmail.com>
> Date:   Sun Jul 1 16:11:37 2012 -0600
>
>      perf kvm: Fix regression with guest machine creation
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index c3e399b..56142d0 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -926,7 +926,7 @@ static struct machine *
>                  else
>                          pid = event->ip.pid;
>
> -               return perf_session__find_machine(session, pid);
> +               return perf_session__findnew_machine(session, pid);
>          }
>
>          return perf_session__find_host_machine(session);
> In original code, it uses perf_session__find_machine(), it means we deliver symbol to machine
> which has the same pid, if no machine found, deliver it to *default* guest. But if we use
> perf_session__findnew_machine() here, if no machine was found, new machine with pid will be built
> and added. Then the default guest which with pid == 0 will never get a symbol.
>
> And because the new machine initialized here has no kernel map created, the symbol delivered to
> it will be marked as "unknown".
>
> This patch here is to revert commit 207b57926 and fix the SEGFAULT bug in another way.
>
> Verification steps:
> 	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
> 	[ perf record: Woken up 1 times to write data ]
> 	[ perf record: Captured and wrote 0.651 MB perf.data.guest (~28437 samples) ]
> 	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules report |grep %
> 	    22.64%    :6471  [guest.kernel.kallsyms]  [g] update_rq_clock.part.70
> 	    19.99%    :6471  [guest.kernel.kallsyms]  [g] d_free
> 	    18.46%    :6471  [guest.kernel.kallsyms]  [g] bio_phys_segments
> 	    16.25%    :6471  [guest.kernel.kallsyms]  [g] dequeue_task
> 	    12.78%    :6471  [guest.kernel.kallsyms]  [g] __switch_to
> 	     7.91%    :6471  [guest.kernel.kallsyms]  [g] scheduler_tick
> 	     1.75%    :6471  [guest.kernel.kallsyms]  [g] native_apic_mem_write
> 	     0.21%    :6471  [guest.kernel.kallsyms]  [g] apic_timer_interrupt
>
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>      Changelog since V1:
>          * More commit message.
>
>   tools/perf/util/session.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 989b2e3..a12dfdd 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -830,6 +830,7 @@ static struct machine *
>   					       struct perf_sample *sample)
>   {
>   	const u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
> +	struct machine *machine;
>   
>   	if (perf_guest &&
>   	    ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
> @@ -842,7 +843,11 @@ static struct machine *
>   		else
>   			pid = sample->pid;
>   
> -		return perf_session__findnew_machine(session, pid);
> +		machine = perf_session__find_machine(session, pid);
> +		if (!machine)
> +			machine = perf_session__findnew_machine(session,
> +						DEFAULT_GUEST_KERNEL_ID);
> +		return machine;
>   	}
>   
>   	return &session->machines.host;

Re: [PATCH V2] perf tools: Fix bug for perf kvm report without guestmount.

[David Ahern]

On 12/16/13, 9:26 AM, Dongsheng Yang wrote:
> Currently, if we use perf kvm --guestkallsyms --guestmodules report,
> we can not get the perf information from perf data file. The all sample
> are shown as unknown.
>
> Reproducing steps:
> 	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules record -a sleep 1
> 	[ perf record: Woken up 1 times to write data ]
> 	[ perf record: Captured and wrote 0.624 MB perf.data.guest (~27260 samples) ]
> 	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules report |grep %
> 	   100.00%  [guest/6471]  [unknown]         [g] 0xffffffff8164f330
>
> This bug was introduced by 207b57926 (perf kvm: Fix regression with guest machine creation).
>
> commit 207b5792696206663a38e525b9793644895bad3b
> Author: David Ahern <dsahern@gmail.com>
> Date:   Sun Jul 1 16:11:37 2012 -0600
>
>      perf kvm: Fix regression with guest machine creation
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index c3e399b..56142d0 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -926,7 +926,7 @@ static struct machine *
>                  else
>                          pid = event->ip.pid;
>
> -               return perf_session__find_machine(session, pid);
> +               return perf_session__findnew_machine(session, pid);
>          }
>
>          return perf_session__find_host_machine(session);

Adding the diff to the commit message confuses git-am. Resubmit the 
patch with just the commit id that introduced the change.

> In original code, it uses perf_session__find_machine(), it means we deliver symbol to machine
> which has the same pid, if no machine found, deliver it to *default* guest. But if we use
> perf_session__findnew_machine() here, if no machine was found, new machine with pid will be built
> and added. Then the default guest which with pid == 0 will never get a symbol.
>
> And because the new machine initialized here has no kernel map created, the symbol delivered to
> it will be marked as "unknown".
>
> This patch here is to revert commit 207b57926 and fix the SEGFAULT bug in another way.
>
> Verification steps:
> 	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
> 	[ perf record: Woken up 1 times to write data ]
> 	[ perf record: Captured and wrote 0.651 MB perf.data.guest (~28437 samples) ]
> 	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules report |grep %
> 	    22.64%    :6471  [guest.kernel.kallsyms]  [g] update_rq_clock.part.70
> 	    19.99%    :6471  [guest.kernel.kallsyms]  [g] d_free
> 	    18.46%    :6471  [guest.kernel.kallsyms]  [g] bio_phys_segments
> 	    16.25%    :6471  [guest.kernel.kallsyms]  [g] dequeue_task
> 	    12.78%    :6471  [guest.kernel.kallsyms]  [g] __switch_to
> 	     7.91%    :6471  [guest.kernel.kallsyms]  [g] scheduler_tick
> 	     1.75%    :6471  [guest.kernel.kallsyms]  [g] native_apic_mem_write
> 	     0.21%    :6471  [guest.kernel.kallsyms]  [g] apic_timer_interrupt
>
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>      Changelog since V1:
>          * More commit message.
>
>   tools/perf/util/session.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 989b2e3..a12dfdd 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -830,6 +830,7 @@ static struct machine *
>   					       struct perf_sample *sample)
>   {
>   	const u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
> +	struct machine *machine;
>
>   	if (perf_guest &&
>   	    ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
> @@ -842,7 +843,11 @@ static struct machine *
>   		else
>   			pid = sample->pid;
>
> -		return perf_session__findnew_machine(session, pid);
> +		machine = perf_session__find_machine(session, pid);
> +		if (!machine)
> +			machine = perf_session__findnew_machine(session,
> +						DEFAULT_GUEST_KERNEL_ID);
> +		return machine;
>   	}
>
>   	return &session->machines.host;
>

And for this change you can add an
Acked-by: David Ahern <dsahern@gmail.com>

So this patch fixes the behavior for defaulting to a guest machine with 
the --guestkallsyms / --guestmodules options. And for the curious, last 
properly working version of perf-kvm for --guestkallsyms + 
--guestmodules options is v3.2. Thanks for digging into it the regression.

With this patch the only degradation for these options appears to be the 
comm change. I'd like to see it stay [guest/<pid>], but was not able to 
find a quick solution for that.

David

Re: [PATCH V2] perf tools: Fix bug for perf kvm report without guestmount.

[Dongsheng Yang]

On 12/20/2013 12:31 AM, David Ahern wrote:
> On 12/16/13, 9:26 AM, Dongsheng Yang wrote:
>> Currently, if we use perf kvm --guestkallsyms --guestmodules report,
>> we can not get the perf information from perf data file. The all sample
>> are shown as unknown.
>>
>> Reproducing steps:
>>     # perf kvm --guestkallsyms /tmp/kallsyms --guestmodules 
>> /tmp/modules record -a sleep 1
>>     [ perf record: Woken up 1 times to write data ]
>>     [ perf record: Captured and wrote 0.624 MB perf.data.guest 
>> (~27260 samples) ]
>>     # perf kvm --guestkallsyms /tmp/kallsyms --guestmodules 
>> /tmp/modules report |grep %
>>        100.00%  [guest/6471]  [unknown]         [g] 0xffffffff8164f330
>>
>> This bug was introduced by 207b57926 (perf kvm: Fix regression with 
>> guest machine creation).
>>
>> commit 207b5792696206663a38e525b9793644895bad3b
>> Author: David Ahern <dsahern@gmail.com>
>> Date:   Sun Jul 1 16:11:37 2012 -0600
>>
>>      perf kvm: Fix regression with guest machine creation
>>
>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>> index c3e399b..56142d0 100644
>> --- a/tools/perf/util/session.c
>> +++ b/tools/perf/util/session.c
>> @@ -926,7 +926,7 @@ static struct machine *
>>                  else
>>                          pid = event->ip.pid;
>>
>> -               return perf_session__find_machine(session, pid);
>> +               return perf_session__findnew_machine(session, pid);
>>          }
>>
>>          return perf_session__find_host_machine(session);
>
> Adding the diff to the commit message confuses git-am. Resubmit the 
> patch with just the commit id that introduced the change.
>
>> In original code, it uses perf_session__find_machine(), it means we 
>> deliver symbol to machine
>> which has the same pid, if no machine found, deliver it to *default* 
>> guest. But if we use
>> perf_session__findnew_machine() here, if no machine was found, new 
>> machine with pid will be built
>> and added. Then the default guest which with pid == 0 will never get 
>> a symbol.
>>
>> And because the new machine initialized here has no kernel map 
>> created, the symbol delivered to
>> it will be marked as "unknown".
>>
>> This patch here is to revert commit 207b57926 and fix the SEGFAULT 
>> bug in another way.
>>
>> Verification steps:
>>     # ./perf kvm --guestkallsyms /home/kallsyms --guestmodules 
>> /home/modules record -a sleep 1
>>     [ perf record: Woken up 1 times to write data ]
>>     [ perf record: Captured and wrote 0.651 MB perf.data.guest 
>> (~28437 samples) ]
>>     # ./perf kvm --guestkallsyms /home/kallsyms --guestmodules 
>> /home/modules report |grep %
>>         22.64%    :6471  [guest.kernel.kallsyms]  [g] 
>> update_rq_clock.part.70
>>         19.99%    :6471  [guest.kernel.kallsyms]  [g] d_free
>>         18.46%    :6471  [guest.kernel.kallsyms]  [g] bio_phys_segments
>>         16.25%    :6471  [guest.kernel.kallsyms]  [g] dequeue_task
>>         12.78%    :6471  [guest.kernel.kallsyms]  [g] __switch_to
>>          7.91%    :6471  [guest.kernel.kallsyms]  [g] scheduler_tick
>>          1.75%    :6471  [guest.kernel.kallsyms]  [g] 
>> native_apic_mem_write
>>          0.21%    :6471  [guest.kernel.kallsyms]  [g] 
>> apic_timer_interrupt
>>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>      Changelog since V1:
>>          * More commit message.
>>
>>   tools/perf/util/session.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>> index 989b2e3..a12dfdd 100644
>> --- a/tools/perf/util/session.c
>> +++ b/tools/perf/util/session.c
>> @@ -830,6 +830,7 @@ static struct machine *
>>                              struct perf_sample *sample)
>>   {
>>       const u8 cpumode = event->header.misc & 
>> PERF_RECORD_MISC_CPUMODE_MASK;
>> +    struct machine *machine;
>>
>>       if (perf_guest &&
>>           ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
>> @@ -842,7 +843,11 @@ static struct machine *
>>           else
>>               pid = sample->pid;
>>
>> -        return perf_session__findnew_machine(session, pid);
>> +        machine = perf_session__find_machine(session, pid);
>> +        if (!machine)
>> +            machine = perf_session__findnew_machine(session,
>> +                        DEFAULT_GUEST_KERNEL_ID);
>> +        return machine;
>>       }
>>
>>       return &session->machines.host;
>>
>
> And for this change you can add an
> Acked-by: David Ahern <dsahern@gmail.com>
>
> So this patch fixes the behavior for defaulting to a guest machine 
> with the --guestkallsyms / --guestmodules options. And for the 
> curious, last properly working version of perf-kvm for --guestkallsyms 
> + --guestmodules options is v3.2. Thanks for digging into it the 
> regression.
>
> With this patch the only degradation for these options appears to be 
> the comm change. I'd like to see it stay [guest/<pid>], but was not 
> able to find a quick solution for that.
>

Thank you, David. I will send a v3 soon.

- Yang
> David
>
>
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

[PATCH V3] perf tools: Fix bug for perf kvm report without guestmount.

[Dongsheng Yang]

Currently, if we use perf kvm --guestkallsyms --guestmodules report,
we can not get the perf information from perf data file. The all sample
are shown as unknown.

Reproducing steps:
	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.624 MB perf.data.guest (~27260 samples) ]
	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules report |grep %
	   100.00%  [guest/6471]  [unknown]         [g] 0xffffffff8164f330

This bug was introduced by 207b57926 (perf kvm: Fix regression with guest machine creation).
In original code, it uses perf_session__find_machine(), it means we deliver symbol to machine
which has the same pid, if no machine found, deliver it to *default* guest. But if we use
perf_session__findnew_machine() here, if no machine was found, new machine with pid will be built
and added. Then the default guest which with pid == 0 will never get a symbol.

And because the new machine initialized here has no kernel map created, the symbol delivered to
it will be marked as "unknown".

This patch here is to revert commit 207b57926 and fix the SEGFAULT bug in another way.

Verification steps:
	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.651 MB perf.data.guest (~28437 samples) ]
	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules report |grep %
	    22.64%    :6471  [guest.kernel.kallsyms]  [g] update_rq_clock.part.70
	    19.99%    :6471  [guest.kernel.kallsyms]  [g] d_free
	    18.46%    :6471  [guest.kernel.kallsyms]  [g] bio_phys_segments
	    16.25%    :6471  [guest.kernel.kallsyms]  [g] dequeue_task
	    12.78%    :6471  [guest.kernel.kallsyms]  [g] __switch_to
	     7.91%    :6471  [guest.kernel.kallsyms]  [g] scheduler_tick
	     1.75%    :6471  [guest.kernel.kallsyms]  [g] native_apic_mem_write
	     0.21%    :6471  [guest.kernel.kallsyms]  [g] apic_timer_interrupt

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Acked-by: David Ahern <dsahern@gmail.com>
---
 tools/perf/util/session.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 989b2e3..a12dfdd 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -830,6 +830,7 @@ static struct machine *
 					       struct perf_sample *sample)
 {
 	const u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
+	struct machine *machine;
 
 	if (perf_guest &&
 	    ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
@@ -842,7 +843,11 @@ static struct machine *
 		else
 			pid = sample->pid;
 
-		return perf_session__findnew_machine(session, pid);
+		machine = perf_session__find_machine(session, pid);
+		if (!machine)
+			machine = perf_session__findnew_machine(session,
+						DEFAULT_GUEST_KERNEL_ID);
+		return machine;
 	}
 
 	return &session->machines.host;
-- 
1.8.2.1

Re: [PATCH V3] perf tools: Fix bug for perf kvm report without guestmount.

[David Ahern]

Arnaldo:

This should go into all stable releases from v3.3 up.


On 12/20/13, 11:41 AM, Dongsheng Yang wrote:
> Currently, if we use perf kvm --guestkallsyms --guestmodules report,
> we can not get the perf information from perf data file. The all sample
> are shown as unknown.
>
> Reproducing steps:
> 	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules record -a sleep 1
> 	[ perf record: Woken up 1 times to write data ]
> 	[ perf record: Captured and wrote 0.624 MB perf.data.guest (~27260 samples) ]
> 	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules report |grep %
> 	   100.00%  [guest/6471]  [unknown]         [g] 0xffffffff8164f330
>
> This bug was introduced by 207b57926 (perf kvm: Fix regression with guest machine creation).
> In original code, it uses perf_session__find_machine(), it means we deliver symbol to machine
> which has the same pid, if no machine found, deliver it to *default* guest. But if we use
> perf_session__findnew_machine() here, if no machine was found, new machine with pid will be built
> and added. Then the default guest which with pid == 0 will never get a symbol.
>
> And because the new machine initialized here has no kernel map created, the symbol delivered to
> it will be marked as "unknown".
>
> This patch here is to revert commit 207b57926 and fix the SEGFAULT bug in another way.
>
> Verification steps:
> 	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
> 	[ perf record: Woken up 1 times to write data ]
> 	[ perf record: Captured and wrote 0.651 MB perf.data.guest (~28437 samples) ]
> 	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules report |grep %
> 	    22.64%    :6471  [guest.kernel.kallsyms]  [g] update_rq_clock.part.70
> 	    19.99%    :6471  [guest.kernel.kallsyms]  [g] d_free
> 	    18.46%    :6471  [guest.kernel.kallsyms]  [g] bio_phys_segments
> 	    16.25%    :6471  [guest.kernel.kallsyms]  [g] dequeue_task
> 	    12.78%    :6471  [guest.kernel.kallsyms]  [g] __switch_to
> 	     7.91%    :6471  [guest.kernel.kallsyms]  [g] scheduler_tick
> 	     1.75%    :6471  [guest.kernel.kallsyms]  [g] native_apic_mem_write
> 	     0.21%    :6471  [guest.kernel.kallsyms]  [g] apic_timer_interrupt
>
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> Acked-by: David Ahern <dsahern@gmail.com>
> ---
>   tools/perf/util/session.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 989b2e3..a12dfdd 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -830,6 +830,7 @@ static struct machine *
>   					       struct perf_sample *sample)
>   {
>   	const u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
> +	struct machine *machine;
>
>   	if (perf_guest &&
>   	    ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
> @@ -842,7 +843,11 @@ static struct machine *
>   		else
>   			pid = sample->pid;
>
> -		return perf_session__findnew_machine(session, pid);
> +		machine = perf_session__find_machine(session, pid);
> +		if (!machine)
> +			machine = perf_session__findnew_machine(session,
> +						DEFAULT_GUEST_KERNEL_ID);
> +		return machine;
>   	}
>
>   	return &session->machines.host;
>

[tip:perf/core] perf kvm: Fix kvm report without guestmount.

[tip-bot for Dongsheng Yang]

Commit-ID:  ad85ace07a05062ef6b59c35a5e80b6eaee1eee6
Gitweb:     http://git.kernel.org/tip/ad85ace07a05062ef6b59c35a5e80b6eaee1eee6
Author:     Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
AuthorDate: Fri, 20 Dec 2013 13:41:47 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 23 Dec 2013 16:49:48 -0300

perf kvm: Fix kvm report without guestmount.

Currently, if we use perf kvm --guestkallsyms --guestmodules report, we
can not get the perf information from perf data file. All sample are
shown as unknown.

Reproducing steps:
	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.624 MB perf.data.guest (~27260 samples) ]
	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules report |grep %
	   100.00%  [guest/6471]  [unknown]         [g] 0xffffffff8164f330

This bug was introduced by 207b57926 (perf kvm: Fix regression with guest machine creation).
In original code, it uses perf_session__find_machine(), it means we deliver symbol to machine
which has the same pid, if no machine found, deliver it to *default* guest. But if we use
perf_session__findnew_machine() here, if no machine was found, new machine with pid will be built
and added. Then the default guest which with pid == 0 will never get a symbol.

And because the new machine initialized here has no kernel map created, the symbol delivered to
it will be marked as "unknown".

This patch here is to revert commit 207b57926 and fix the SEGFAULT bug in another way.

Verification steps:
	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.651 MB perf.data.guest (~28437 samples) ]
	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules report |grep %
	    22.64%    :6471  [guest.kernel.kallsyms]  [g] update_rq_clock.part.70
	    19.99%    :6471  [guest.kernel.kallsyms]  [g] d_free
	    18.46%    :6471  [guest.kernel.kallsyms]  [g] bio_phys_segments
	    16.25%    :6471  [guest.kernel.kallsyms]  [g] dequeue_task
	    12.78%    :6471  [guest.kernel.kallsyms]  [g] __switch_to
	     7.91%    :6471  [guest.kernel.kallsyms]  [g] scheduler_tick
	     1.75%    :6471  [guest.kernel.kallsyms]  [g] native_apic_mem_write
	     0.21%    :6471  [guest.kernel.kallsyms]  [g] apic_timer_interrupt

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Acked-by: David Ahern <dsahern@gmail.com>
Cc: stable@vger.kernel.org # 3.3+
Cc: David Ahern <dsahern@gmail.com>
Link: http://lkml.kernel.org/r/1387564907-3045-1-git-send-email-yangds.fnst@cn.fujitsu.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/session.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index cbacaab..d3a857be 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -830,6 +830,7 @@ static struct machine *
 					       struct perf_sample *sample)
 {
 	const u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
+	struct machine *machine;
 
 	if (perf_guest &&
 	    ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
@@ -842,7 +843,11 @@ static struct machine *
 		else
 			pid = sample->pid;
 
-		return perf_session__findnew_machine(session, pid);
+		machine = perf_session__find_machine(session, pid);
+		if (!machine)
+			machine = perf_session__findnew_machine(session,
+						DEFAULT_GUEST_KERNEL_ID);
+		return machine;
 	}
 
 	return &session->machines.host;

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[Dongsheng Yang]

Hi Arnaldo and David.....

On 12/06/2013 06:56 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Dec 06, 2013 at 04:33:08PM -0500, Dongsheng Yang escreveu:
>> Currently, if we use perf kvm --guestkallsyms --guestmodules report,
>> we can not get the perf information from perf data file. The all sample
>> are shown as unknown.
>>
>> Reproducing steps:
>> 	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules record -a sleep 1
>> 	[ perf record: Woken up 1 times to write data ]
>> 	[ perf record: Captured and wrote 0.624 MB perf.data.guest (~27260 samples) ]
>> 	# perf kvm --guestkallsyms /tmp/kallsyms --guestmodules /tmp/modules report |grep %
>> 	   100.00%  [guest/6471]  [unknown]         [g] 0xffffffff8164f330
>>
>> This bug is caused when delivery sample. We use perf_session__find_machine_for_cpumode() to find
>> the machine, and it call perf_session__findnew_machine(). Then we will delivery the sample to a
>> new machine without kernel mapped.
>>
>> This patch changes perf_session__findnew_machine() to perf_session__find_machine(), so the sample
>> will be deliveried to default guest machine if there is no machine specified for it.
> Ok, but then we will use the provided guest modules, etc for _all_ VMs,
> irrespective of its pid, which sounds risky, no?

Yes, it is risky.

As my guess, the intention of default guest was to take care of the symbols
which are out of existing guest machines. It means when we have no guest 
specified for a
symbol, it will be delivered to default guest, right? Otherwise, I can 
not understand what
the default guest is used for.

But, yes, this design is really really risky. And I think it is time to 
do some change for it.
How about introduce an option named --guestpid? Then we can make the 
usage of perf kvm
more clear:
     * perf kvm --guestkallsyms --guestmodules --guestpid 
[top|record|report]
         This usage is for only one guest and will not resolve the 
symbols from other guests.
     * perf kvm --guestmount [top|record|report]
         This usage is for one or more guest. But you have to use sshfs 
to mount the guestfs on host.

This way, 1st usage is limited for one guest (actually, it is for only 
one guest currently and insidiously),
user will get a clear result as the wish. Then the 1st usage is a 
shortcut of usage 2 as David said.

Comments?


- Yang
>
> You get correct results because the modules/kallsyms info you provided
> matches the VM you're testing, i.e. the pid test is not needed.
>
> I think we need to specify for which PID the --guestmodules and
> --guestkallsyms is provided, so that it can refuse to use it for pids
> where it is invalid.
>
> If we do that then the current code will find it with
> perf_session__find_machine_for_cpumode(), and when not finding it, it
> will behave as expected and as it does today: refuse to resolve symbols
> using invalid kallsyms/modules.
>
> One way to achieve this automagically would be to have the build-id of
> the guest kernel and modules be present on the PERF_RECORD_MMAP, so that
> we wouldn't need to provide --guest{kallsyms,modules} as it would look
> it up (kallsyms or vmlinux) in the ~/.debug or elsewhere, like in
> /usr/lib/debug/lib/modules/3.11.4-101.fc18.x86_64/vmlinux (where
> kernel-debuginfo packages install its contents.
>
> Untill we have that, we need to at least document this limitation, i.e.
> that great care must be taken to provide exactly the kallsyms/modules
> pair to 'perf kvm', otherwise bogus results will be presented.
>
> - Arnaldo
>   
>> Verify steps:
>> 	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
>> 	[ perf record: Woken up 1 times to write data ]
>> 	[ perf record: Captured and wrote 0.651 MB perf.data.guest (~28437 samples) ]
>> 	# ./perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules report |grep %
>> 	    22.64%    :6471  [guest.kernel.kallsyms]  [g] update_rq_clock.part.70
>> 	    19.99%    :6471  [guest.kernel.kallsyms]  [g] d_free
>> 	    18.46%    :6471  [guest.kernel.kallsyms]  [g] bio_phys_segments
>> 	    16.25%    :6471  [guest.kernel.kallsyms]  [g] dequeue_task
>> 	    12.78%    :6471  [guest.kernel.kallsyms]  [g] __switch_to
>> 	     7.91%    :6471  [guest.kernel.kallsyms]  [g] scheduler_tick
>> 	     1.75%    :6471  [guest.kernel.kallsyms]  [g] native_apic_mem_write
>> 	     0.21%    :6471  [guest.kernel.kallsyms]  [g] apic_timer_interrupt
>>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>   tools/perf/util/session.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>> index 8a7da6f..1770f2f 100644
>> --- a/tools/perf/util/session.c
>> +++ b/tools/perf/util/session.c
>> @@ -851,6 +851,7 @@ static struct machine *
>>   					       struct perf_sample *sample)
>>   {
>>   	const u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
>> +	struct machine *machine;
>>   
>>   	if (perf_guest &&
>>   	    ((cpumode == PERF_RECORD_MISC_GUEST_KERNEL) ||
>> @@ -863,7 +864,11 @@ static struct machine *
>>   		else
>>   			pid = sample->pid;
>>   
>> -		return perf_session__findnew_machine(session, pid);
>> +		machine = perf_session__find_machine(session, pid);
>> +		if (!machine)
>> +			machine = perf_session__findnew_machine(session,
>> +						DEFAULT_GUEST_KERNEL_ID);
>> +		return machine;
>>   	}
>>   
>>   	return &session->machines.host;
>> -- 
>> 1.8.2.1

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[David Ahern]

On 12/9/13, 8:20 AM, Dongsheng Yang wrote:
> How about introduce an option named --guestpid? Then we can make the
> usage of perf kvm
> more clear:
>      * perf kvm --guestkallsyms --guestmodules --guestpid
> [top|record|report]
>          This usage is for only one guest and will not resolve the
> symbols from other guests.

If there is only 1 guest then there should not be a problem right? You 
give perf a single guest kallsyms as the "default" and it works. 
--guestpid adds no value in that case.

David

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[Dongsheng Yang]

On 12/08/2013 10:42 PM, David Ahern wrote:
> On 12/9/13, 8:20 AM, Dongsheng Yang wrote:
>> How about introduce an option named --guestpid? Then we can make the
>> usage of perf kvm
>> more clear:
>>      * perf kvm --guestkallsyms --guestmodules --guestpid
>> [top|record|report]
>>          This usage is for only one guest and will not resolve the
>> symbols from other guests.
>
> If there is only 1 guest then there should not be a problem right? You 
> give perf a single guest kallsyms as the "default" and it works. 
> --guestpid adds no value in that case.

Yes, if there is only one guest is running, "default" guest is "the" 
guest. Then with my patch in this thread applied, it works well.

But consider this scenario, there are two guests are running, but we 
need to record-report one of them.

--guestmount can achieve this request, but as a shortcut of guestmount, 
--guest{kallysms, modules} dose not
support it well, right? So, I think we can discard the default guest, 
and use guestpid in record-report.
>
> David
>
>
>

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[David Ahern]

On 12/9/13, 10:12 AM, Dongsheng Yang wrote:
> On 12/08/2013 10:42 PM, David Ahern wrote:
>> On 12/9/13, 8:20 AM, Dongsheng Yang wrote:
>>> How about introduce an option named --guestpid? Then we can make the
>>> usage of perf kvm
>>> more clear:
>>>      * perf kvm --guestkallsyms --guestmodules --guestpid
>>> [top|record|report]
>>>          This usage is for only one guest and will not resolve the
>>> symbols from other guests.
>>
>> If there is only 1 guest then there should not be a problem right? You
>> give perf a single guest kallsyms as the "default" and it works.
>> --guestpid adds no value in that case.
>
> Yes, if there is only one guest is running, "default" guest is "the"
> guest. Then with my patch in this thread applied, it works well.
>
> But consider this scenario, there are two guests are running, but we
> need to record-report one of them.
>
> --guestmount can achieve this request, but as a shortcut of guestmount,
> --guest{kallysms, modules} dose not
> support it well, right? So, I think we can discard the default guest,
> and use guestpid in record-report.

No.

Use cases:
1. one guest
--guestkallsyms and --guestmodules apply to default guest; user should 
supply files that apply to the one guest. Supplying any other kallsyms 
is just nonsense. *NO* other arguments are needed.

2. more than 1 VM, *ALL* VMs running the same kernel
--guestkallsyms and --guestmodules apply to default guest; user should 
supply files that apply to all of guests. No other arguments are needed.

3. more than 1 VM, VMs running different kernels. 1+ VMs running the 
same kernel
--guestmount allows user to supply files that apply to all of guests 
based on pid. --guestkallsyms/guestmodules is used for any guest not 
showing up in guestmount.

David

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[Dongsheng Yang]

On 12/08/2013 11:32 PM, David Ahern wrote:
> On 12/9/13, 10:12 AM, Dongsheng Yang wrote:
>> On 12/08/2013 10:42 PM, David Ahern wrote:
>>> On 12/9/13, 8:20 AM, Dongsheng Yang wrote:
>>>> How about introduce an option named --guestpid? Then we can make the
>>>> usage of perf kvm
>>>> more clear:
>>>>      * perf kvm --guestkallsyms --guestmodules --guestpid
>>>> [top|record|report]
>>>>          This usage is for only one guest and will not resolve the
>>>> symbols from other guests.
>>>
>>> If there is only 1 guest then there should not be a problem right? You
>>> give perf a single guest kallsyms as the "default" and it works.
>>> --guestpid adds no value in that case.
>>
>> Yes, if there is only one guest is running, "default" guest is "the"
>> guest. Then with my patch in this thread applied, it works well.
>>
>> But consider this scenario, there are two guests are running, but we
>> need to record-report one of them.
>>
>> --guestmount can achieve this request, but as a shortcut of guestmount,
>> --guest{kallysms, modules} dose not
>> support it well, right? So, I think we can discard the default guest,
>> and use guestpid in record-report.
>
> No.
>
> Use cases:
> 1. one guest
> --guestkallsyms and --guestmodules apply to default guest; user should 
> supply files that apply to the one guest. Supplying any other kallsyms 
> is just nonsense. *NO* other arguments are needed.
>
> 2. more than 1 VM, *ALL* VMs running the same kernel
> --guestkallsyms and --guestmodules apply to default guest; user should 
> supply files that apply to all of guests. No other arguments are needed.
>
> 3. more than 1 VM, VMs running different kernels. 1+ VMs running the 
> same kernel
> --guestmount allows user to supply files that apply to all of guests 
> based on pid. --guestkallsyms/guestmodules is used for any guest not 
> showing up in guestmount.
>

When more than 1 VM, the cases you provided is all about record-report 
the symbols from __all__ guests. How about I want to record-report one 
of them?
Example:
     There are 2 guests are running different kernels, I want to 
record-report VM1.

Currently, we can use --guestmount to get the symbols of VM1, but we can 
not remove the symbols of VM2 from report. It means the percentage of 
each symbol is not  the symbol only in this VM1.

What I want with introducing guestpid is to record-report the symbols 
only from VM1, and ignore VM2.

> David
>
>

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[Dongsheng Yang]

On 12/09/2013 01:06 PM, Dongsheng Yang wrote:
> On 12/08/2013 11:32 PM, David Ahern wrote:
>> On 12/9/13, 10:12 AM, Dongsheng Yang wrote:
>>> On 12/08/2013 10:42 PM, David Ahern wrote:
>>>> On 12/9/13, 8:20 AM, Dongsheng Yang wrote:
>>>>> How about introduce an option named --guestpid? Then we can make the
>>>>> usage of perf kvm
>>>>> more clear:
>>>>>      * perf kvm --guestkallsyms --guestmodules --guestpid
>>>>> [top|record|report]
>>>>>          This usage is for only one guest and will not resolve the
>>>>> symbols from other guests.
>>>>
>>>> If there is only 1 guest then there should not be a problem right? You
>>>> give perf a single guest kallsyms as the "default" and it works.
>>>> --guestpid adds no value in that case.
>>>
>>> Yes, if there is only one guest is running, "default" guest is "the"
>>> guest. Then with my patch in this thread applied, it works well.
>>>
>>> But consider this scenario, there are two guests are running, but we
>>> need to record-report one of them.
>>>
>>> --guestmount can achieve this request, but as a shortcut of guestmount,
>>> --guest{kallysms, modules} dose not
>>> support it well, right? So, I think we can discard the default guest,
>>> and use guestpid in record-report.
>>
>> No.
>>
>> Use cases:
>> 1. one guest
>> --guestkallsyms and --guestmodules apply to default guest; user 
>> should supply files that apply to the one guest. Supplying any other 
>> kallsyms is just nonsense. *NO* other arguments are needed.
>>
>> 2. more than 1 VM, *ALL* VMs running the same kernel
>> --guestkallsyms and --guestmodules apply to default guest; user 
>> should supply files that apply to all of guests. No other arguments 
>> are needed.
>>
>> 3. more than 1 VM, VMs running different kernels. 1+ VMs running the 
>> same kernel
>> --guestmount allows user to supply files that apply to all of guests 
>> based on pid. --guestkallsyms/guestmodules is used for any guest not 
>> showing up in guestmount.
>>
>
> When more than 1 VM, the cases you provided is all about record-report 
> the symbols from __all__ guests. How about I want to record-report one 
> of them?
> Example:
>     There are 2 guests are running different kernels, I want to 
> record-report VM1.
>
> Currently, we can use --guestmount to get the symbols of VM1, but we 
> can not remove the symbols of VM2 from report. It means the percentage 
> of each symbol is not  the symbol only in this VM1.
>
> What I want with introducing guestpid is to record-report the symbols 
> only from VM1, and ignore VM2.
>

Besides, as we provide two usages of perf-kvm in manpage, guestmount-way 
and guest{kallsyms,modules}-way, but the guest{kallsyms, modules} is not 
used when
there is only one guest is running, the guestmount is used in all 
situations, it seems not reasonable. I think we can provide a guestpid 
to make the guestkallsyms-way can be used in any situation.

This way, user can understand and choose the usage easily.

1. one or more guests you want to record-report, --guestmount-way. It 
provides the full functionality of perf kvm.

2. only one guest you want to record-report, --guestmount is also 
available, and there is another usage for this most used case as a 
shortcut , --guest{kallsyms, modules, pid}.

Then the relationship between the two kinds of usage is more clear, 
right? And in this way, we can make the result of perf-kvm more 
foreseeable to user.

>> David
>>
>>
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] perf tools: Fix bug for perf kvm report without guestmount.

[Dongsheng Yang]

>> When more than 1 VM, the cases you provided is all about 
>> record-report the symbols from __all__ guests. How about I want to 
>> record-report one of them?
>> Example:
>>     There are 2 guests are running different kernels, I want to 
>> record-report VM1.
>>
>> Currently, we can use --guestmount to get the symbols of VM1, but we 
>> can not remove the symbols of VM2 from report. It means the 
>> percentage of each symbol is not  the symbol only in this VM1.
>>
>> What I want with introducing guestpid is to record-report the symbols 
>> only from VM1, and ignore VM2.
>>
>
> Besides, as we provide two usages of perf-kvm in manpage, 
> guestmount-way and guest{kallsyms,modules}-way, but the 
> guest{kallsyms, modules} is not used 
s/not/only. Sorry for the typo :(
> when
> there is only one guest is running, the guestmount is used in all 
> situations, it seems not reasonable. I think we can provide a guestpid 
> to make the guestkallsyms-way can be used in any situation.
>
> This way, user can understand and choose the usage easily.
>
> 1. one or more guests you want to record-report, --guestmount-way. It 
> provides the full functionality of perf kvm.
>
> 2. only one guest you want to record-report, --guestmount is also 
> available, and there is another usage for this most used case as a 
> shortcut , --guest{kallsyms, modules, pid}.
>
> Then the relationship between the two kinds of usage is more clear, 
> right? And in this way, we can make the result of perf-kvm more 
> foreseeable to user.
>
>>> David
>>>
>>>
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe 
>> linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>