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

[Dongsheng Yang <yangds.fnst@cn.fujitsu.com>]

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