[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 <yangd...@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

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

[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

[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

[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

[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

[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 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.

[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.

[Dongsheng Yang]

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.

[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 :(

[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.

[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

[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

[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

[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 <dsa...@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

[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

[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

[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 <dsa...@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 <yangd...@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

[Dongsheng Yang]

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

Thanx in advance

- Yang

[Dongsheng Yang]

On 12/20/2013 12:31 AM, David Ahern wrote:

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

> And for this change you can add an
> Acked-by: David Ahern <dsa...@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

[David Ahern]

On 12/16/13, 9:26 AM, Dongsheng Yang wrote:

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

And for this change you can add an
Acked-by: David Ahern <dsa...@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

[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 <yangd...@cn.fujitsu.com>

Acked-by: David Ahern <dsa...@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

[David Ahern]

Arnaldo:

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

[tip-bot for Dongsheng Yang]

Commit-ID: ad85ace07a05062ef6b59c35a5e80b6eaee1eee6
Gitweb: http://git.kernel.org/tip/ad85ace07a05062ef6b59c35a5e80b6eaee1eee6
Author: Dongsheng Yang <yangd...@cn.fujitsu.com>
AuthorDate: Fri, 20 Dec 2013 13:41:47 -0500
Committer: Arnaldo Carvalho de Melo <ac...@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

Cc: sta...@vger.kernel.org # 3.3+
Cc: David Ahern <dsa...@gmail.com>
Link: http://lkml.kernel.org/r/1387564907-3045-1-git-...@cn.fujitsu.com
Signed-off-by: Arnaldo Carvalho de Melo <ac...@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;
--