public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] perf, report: use properly build_id kernel binaries
@ 2011-06-01 19:43 Jiri Olsa
  2011-06-17 12:38 ` Jiri Olsa
  2011-08-14 15:36 ` [tip:perf/core] perf report: Use " tip-bot for Jiri Olsa
  0 siblings, 2 replies; 4+ messages in thread
From: Jiri Olsa @ 2011-06-01 19:43 UTC (permalink / raw)
  To: acme; +Cc: fweisbec, yanmin_zhang, avi, mingo, Jiri Olsa, linux-kernel

resending with correct lkml address, sry
jirka

---
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 <jolsa@redhat.com>
---
 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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] perf, report: use properly build_id kernel binaries
  2011-06-01 19:43 [PATCH] perf, report: use properly build_id kernel binaries Jiri Olsa
@ 2011-06-17 12:38 ` Jiri Olsa
  2011-07-17 18:34   ` Frederic Weisbecker
  2011-08-14 15:36 ` [tip:perf/core] perf report: Use " tip-bot for Jiri Olsa
  1 sibling, 1 reply; 4+ messages in thread
From: Jiri Olsa @ 2011-06-17 12:38 UTC (permalink / raw)
  To: acme; +Cc: fweisbec, yanmin_zhang, avi, mingo, linux-kernel

hi, any feedback?

thanks,
jirka

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 <jolsa@redhat.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] perf, report: use properly build_id kernel binaries
  2011-06-17 12:38 ` Jiri Olsa
@ 2011-07-17 18:34   ` Frederic Weisbecker
  0 siblings, 0 replies; 4+ messages in thread
From: Frederic Weisbecker @ 2011-07-17 18:34 UTC (permalink / raw)
  To: Jiri Olsa; +Cc: acme, yanmin_zhang, avi, mingo, linux-kernel

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 <jolsa@redhat.com>
> > ---
> >  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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [tip:perf/core] perf report: Use properly build_id kernel binaries
  2011-06-01 19:43 [PATCH] perf, report: use properly build_id kernel binaries Jiri Olsa
  2011-06-17 12:38 ` Jiri Olsa
@ 2011-08-14 15:36 ` tip-bot for Jiri Olsa
  1 sibling, 0 replies; 4+ messages in thread
From: tip-bot for Jiri Olsa @ 2011-08-14 15:36 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: acme, linux-kernel, hpa, mingo, yanmin_zhang, jolsa, fweisbec,
	tglx, avi, mingo

Commit-ID:  f57b05ed532ccf3b3e22878a5678ca10de50ad29
Gitweb:     http://git.kernel.org/tip/f57b05ed532ccf3b3e22878a5678ca10de50ad29
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Wed, 1 Jun 2011 21:43:46 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 11 Aug 2011 08:58:03 -0300

perf report: Use properly build_id kernel binaries

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.

Cc: Avi Kivity <avi@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Yanmin Zhang <yanmin_zhang@linux.intel.com>
Link: http://lkml.kernel.org/r/20110601194346.GB1934@jolsa.brq.redhat.com
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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 d4f3101..b6c1ad1 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 a8b5371..e142c21 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2181,27 +2181,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;
@@ -2219,24 +2214,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;
 }
 
@@ -2340,7 +2347,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);

^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2011-08-14 15:37 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-06-01 19:43 [PATCH] perf, report: use properly build_id kernel binaries Jiri Olsa
2011-06-17 12:38 ` Jiri Olsa
2011-07-17 18:34   ` Frederic Weisbecker
2011-08-14 15:36 ` [tip:perf/core] perf report: Use " tip-bot for Jiri Olsa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox