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

[David Ahern <dsahern@gmail.com>]

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