From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756163Ab1GQSei (ORCPT ); Sun, 17 Jul 2011 14:34:38 -0400 Received: from mail-ww0-f44.google.com ([74.125.82.44]:60528 "EHLO mail-ww0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754577Ab1GQSeg (ORCPT ); Sun, 17 Jul 2011 14:34:36 -0400 Date: Sun, 17 Jul 2011 20:34:32 +0200 From: Frederic Weisbecker To: Jiri Olsa Cc: acme@redhat.com, yanmin_zhang@linux.intel.com, avi@redhat.com, mingo@elte.hu, linux-kernel@vger.kernel.org Subject: Re: [PATCH] perf, report: use properly build_id kernel binaries Message-ID: <20110717183429.GE29355@somewhere> References: <20110601194346.GB1934@jolsa.brq.redhat.com> <20110617123856.GB2699@jolsa.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20110617123856.GB2699@jolsa.brq.redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jun 17, 2011 at 02:38:56PM +0200, Jiri Olsa wrote: > hi, any feedback? > > thanks, > jirka I prefer to let that for Arnaldo when he's back. I don't know well the build-id part. Thanks. > > On Wed, Jun 01, 2011 at 09:43:46PM +0200, Jiri Olsa wrote: > > If we bring the recorded perf data together with kernel > > binary from another machine using: > > > > on server A: > > perf archive > > > > on server B: > > tar xjvf perf.data.tar.bz2 -C ~/.debug > > > > the build_id kernel dso is not properly recognized during > > the "perf report" command on server B. > > > > The reason is, that build_id dsos are added during the session initialization, > > while the kernel maps are created during the sample event processing. > > > > The machine__create_kernel_maps functions ends up creating new dso object > > for kernel, but it does not check if we already have one added by build_id > > processing. > > > > > > Also the build_id reading ABI quirk added in commit: > > > > - commit b25114817a73bbd2b84ce9dba02ee1ef8989a947 > > perf build-id: Add quirk to deal with perf.data file format breakage > > > > populates the "struct build_id_event::pid" with 0, which > > is later interpreted as DEFAULT_GUEST_KERNEL_ID. > > > > This is not always correct, so it's better to guess the pid > > value based on the "struct build_id_event::header::misc" value. > > > > > > - Tested with data generated on x86 kernel version v2.6.34 > > and reported back on x86_64 current kernel. > > - Not tested for guest kernel case. > > > > > > Note the problem stays for PERF_RECORD_MMAP events recorded by perf that > > does not use proper pid (HOST_KERNEL_ID/DEFAULT_GUEST_KERNEL_ID). They are > > misinterpreted within the current perf code. Probably there's not much we > > can do about that. > > > > > > Signed-off-by: Jiri Olsa > > --- > > tools/perf/util/header.c | 11 ++++++++- > > tools/perf/util/symbol.c | 57 +++++++++++++++++++++++++-------------------- > > tools/perf/util/symbol.h | 1 - > > 3 files changed, 42 insertions(+), 27 deletions(-) > > > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c > > index afb0849..a0d90b1 100644 > > --- a/tools/perf/util/header.c > > +++ b/tools/perf/util/header.c > > @@ -726,7 +726,16 @@ static int perf_header__read_build_ids_abi_quirk(struct perf_header *header, > > return -1; > > > > bev.header = old_bev.header; > > - bev.pid = 0; > > + > > + /* > > + * As the pid is the missing value, we need to fill > > + * it properly. The header.misc value give us nice hint. > > + */ > > + bev.pid = HOST_KERNEL_ID; > > + if (bev.header.misc == PERF_RECORD_MISC_GUEST_USER || > > + bev.header.misc == PERF_RECORD_MISC_GUEST_KERNEL) > > + bev.pid = DEFAULT_GUEST_KERNEL_ID; > > + > > memcpy(bev.build_id, old_bev.build_id, sizeof(bev.build_id)); > > __event_process_build_id(&bev, filename, session); > > > > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c > > index eec1963..c307121 100644 > > --- a/tools/perf/util/symbol.c > > +++ b/tools/perf/util/symbol.c > > @@ -2170,27 +2170,22 @@ size_t machines__fprintf_dsos_buildid(struct rb_root *machines, > > return ret; > > } > > > > -struct dso *dso__new_kernel(const char *name) > > +static struct dso* > > +dso__kernel_findnew(struct machine *machine, const char *name, > > + const char *short_name, int dso_type) > > { > > - struct dso *dso = dso__new(name ?: "[kernel.kallsyms]"); > > - > > - if (dso != NULL) { > > - dso__set_short_name(dso, "[kernel]"); > > - dso->kernel = DSO_TYPE_KERNEL; > > - } > > - > > - return dso; > > -} > > + /* > > + * The kernel dso could be created by build_id processing. > > + */ > > + struct dso *dso = __dsos__findnew(&machine->kernel_dsos, name); > > > > -static struct dso *dso__new_guest_kernel(struct machine *machine, > > - const char *name) > > -{ > > - char bf[PATH_MAX]; > > - struct dso *dso = dso__new(name ?: machine__mmap_name(machine, bf, > > - sizeof(bf))); > > + /* > > + * We need to run this in all cases, since during the build_id > > + * processing we had no idea this was the kernel dso. > > + */ > > if (dso != NULL) { > > - dso__set_short_name(dso, "[guest.kernel]"); > > - dso->kernel = DSO_TYPE_GUEST_KERNEL; > > + dso__set_short_name(dso, short_name); > > + dso->kernel = dso_type; > > } > > > > return dso; > > @@ -2208,24 +2203,36 @@ void dso__read_running_kernel_build_id(struct dso *dso, struct machine *machine) > > dso->has_build_id = true; > > } > > > > -static struct dso *machine__create_kernel(struct machine *machine) > > +static struct dso *machine__get_kernel(struct machine *machine) > > { > > const char *vmlinux_name = NULL; > > struct dso *kernel; > > > > if (machine__is_host(machine)) { > > vmlinux_name = symbol_conf.vmlinux_name; > > - kernel = dso__new_kernel(vmlinux_name); > > + if (!vmlinux_name) > > + vmlinux_name = "[kernel.kallsyms]"; > > + > > + kernel = dso__kernel_findnew(machine, vmlinux_name, > > + "[kernel]", > > + DSO_TYPE_KERNEL); > > } else { > > + char bf[PATH_MAX]; > > + > > if (machine__is_default_guest(machine)) > > vmlinux_name = symbol_conf.default_guest_vmlinux_name; > > - kernel = dso__new_guest_kernel(machine, vmlinux_name); > > + if (!vmlinux_name) > > + vmlinux_name = machine__mmap_name(machine, bf, > > + sizeof(bf)); > > + > > + kernel = dso__kernel_findnew(machine, vmlinux_name, > > + "[guest.kernel]", > > + DSO_TYPE_GUEST_KERNEL); > > } > > > > - if (kernel != NULL) { > > + if (kernel != NULL && (!kernel->has_build_id)) > > dso__read_running_kernel_build_id(kernel, machine); > > - dsos__add(&machine->kernel_dsos, kernel); > > - } > > + > > return kernel; > > } > > > > @@ -2329,7 +2336,7 @@ void machine__destroy_kernel_maps(struct machine *machine) > > > > int machine__create_kernel_maps(struct machine *machine) > > { > > - struct dso *kernel = machine__create_kernel(machine); > > + struct dso *kernel = machine__get_kernel(machine); > > > > if (kernel == NULL || > > __machine__create_kernel_maps(machine, kernel) < 0) > > diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h > > index 325ee36..4f377d9 100644 > > --- a/tools/perf/util/symbol.h > > +++ b/tools/perf/util/symbol.h > > @@ -155,7 +155,6 @@ struct dso { > > }; > > > > struct dso *dso__new(const char *name); > > -struct dso *dso__new_kernel(const char *name); > > void dso__delete(struct dso *dso); > > > > int dso__name_len(const struct dso *dso); > > -- > > 1.7.1