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

Arnaldo Carvalho de Melo
2013-12-06T11:56:37Z

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