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@xxxxxxxxx>
> > 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@xxxxxxxxxxxxxx>
> > ---
> >      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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/