Messages in this thread | | | Date | Tue, 10 Jul 2012 20:16:15 -0600 | From | David Ahern <> | Subject | Re: [PATCH 4/4] perf kvm: fix bug resolving guest kernel syms |
| |
On 7/10/12 6:54 PM, Namhyung Kim wrote: > Hi, David > > On Tue, 10 Jul 2012 15:48:14 -0600, David Ahern wrote: >> Guest kernel symbols are not resolved despite passing the information >> needed to resolve them. e.g., >> >> perf kvm --guest --guestmount=/tmp/guest-mount record -a -- sleep 1 >> perf kvm --guest --guestmount=/tmp/guest-mount report --stdio >> >> 36.55% [guest/11399] [unknown] [g] 0xffffffff81600bc8 >> 33.19% [guest/10474] [unknown] [g] 0x00000000c0116e00 >> 30.26% [guest/11094] [unknown] [g] 0xffffffff8100a288 >> >> 43.69% [guest/10474] [unknown] [g] 0x00000000c0103d90 >> 37.38% [guest/11399] [unknown] [g] 0xffffffff81600bc8 >> 12.24% [guest/11094] [unknown] [g] 0xffffffff810aa91d >> 6.69% [guest/11094] [unknown] [u] 0x00007fa784d721c3 >> >> which is just pathetic. >> >> After a maddening 2 days sifting through perf minutia I found it -- >> id_hdr_size is not initialized for guest machines. This shows up on the >> report side as random garbage for the cpu and timestamp, e.g., >> >> 29816 7310572949125804849 0x1ac0 [0x50]: PERF_RECORD_MMAP ... >> >> that messes up the sample sorting such that synthesized guest maps are >> processed last. >> >> With this patch you get a much more helpful report: >> >> 12.11% [guest/11399] [guest.kernel.kallsyms.11399] [g] irqtime_account_process_tick >> 10.58% [guest/11399] [guest.kernel.kallsyms.11399] [g] run_timer_softirq >> 6.95% [guest/11094] [guest.kernel.kallsyms.11094] [g] printk_needs_cpu >> 6.50% [guest/11094] [guest.kernel.kallsyms.11094] [g] do_timer >> 6.45% [guest/11399] [guest.kernel.kallsyms.11399] [g] idle_balance >> 4.90% [guest/11094] [guest.kernel.kallsyms.11094] [g] native_read_tsc >> ... >> >> Signed-off-by: David Ahern <dsahern@gmail.com> >> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> >> Cc: Ingo Molnar <mingo@kernel.org> >> Cc: Jiri Olsa <jolsa@redhat.com> >> Cc: Namhyung Kim <namhyung@gmail.com> >> Cc: Frederic Weisbecker <fweisbec@gmail.com> >> Cc: Peter Zijlstra <peterz@infradead.org> >> --- >> tools/perf/util/map.c | 24 ++++++++++++++++++++++++ >> tools/perf/util/map.h | 1 + >> tools/perf/util/session.c | 1 + >> 3 files changed, 26 insertions(+) >> >> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c >> index 641377e..da3411b 100644 >> --- a/tools/perf/util/map.c >> +++ b/tools/perf/util/map.c >> @@ -730,3 +730,27 @@ char *machine__mmap_name(struct machine *self, char *bf, size_t size) >> >> return bf; >> } >> + >> +void machines__set_id_hdr_size(struct rb_root *self, u16 id_hdr_size) >> +{ >> + struct rb_node *p; >> + struct machine *machine; >> + >> + p = self->rb_node; >> + while (p != NULL) { >> + machine = rb_entry(p, struct machine, rb_node); >> + machine->id_hdr_size = id_hdr_size; >> + p = rb_next(p); >> + } >> + > > Looks like white-space damaged. :(
bleah, something on my dev server converted tabs to spaces. Will fix.
> > The loop itself looks fine, or you might use this form: > > for (p = rb_first(self); p; p = rb_next(p))
Doesn't work - I still see machines not updated. > > > Is there a something like rb_for_each_entry() ?
Didn't see one.
> > >> + p = self->rb_node; >> + if (p) >> + p = rb_prev(p); >> + while (p != NULL) { >> + machine = rb_entry(p, struct machine, rb_node); >> + machine->id_hdr_size = id_hdr_size; >> + p = rb_prev(p); >> + } >> + > > I don't see why this second loop is necessary?
To get the left half of the tree I believe. First loop walks the root and all of the leaves to the right; this loop walks all of the leaves to the left.
David
| |