perf record regression?

perf record regression?

[Arun Sharma]

I pulled the perf/core branch in
git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux-2.6.git today
and found that:

perf record -ag -- sleep 1
perf report -D | grep dso:

was returning:

 ...... dso: <not found>
 ...... dso: <not found>
 ...... dso: /boot/vmlinux-2.6.38-rc5+

i.e perf report was not able to map IP addresses to the correct dso
for anything in user space.

Using git bisect, I narrowed it down to these two commits:

commit f60686f71f4afec7b2925b4682f33c70c0a7eb17
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Wed Mar 9 07:48:24 2011 -0300

    perf evsel: Assume rest of perf_header_attr functions

commit e62e48df064b431029658089ab1f8b529ff6da3d
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Tue Mar 8 09:05:57 2011 -0300

    perf session: Simplify evlist creation from perf.data header

perf built after these commits is able to symbolize properly for
perf.data collected using an older version of perf. So I believe, the
perf report side of things are ok. But perf record is not.

 -Arun

Re: perf record regression?

[Arnaldo Carvalho de Melo]

Em Wed, Mar 09, 2011 at 09:59:39PM -0800, Arun Sharma escreveu:
> I pulled the perf/core branch in
> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux-2.6.git today
> and found that:
> 
> perf record -ag -- sleep 1
> perf report -D | grep dso:
> 
> was returning:
> 
>  ...... dso: <not found>
>  ...... dso: <not found>
>  ...... dso: /boot/vmlinux-2.6.38-rc5+
> 
> i.e perf report was not able to map IP addresses to the correct dso
> for anything in user space.
> 
> Using git bisect, I narrowed it down to these two commits:
> 
> commit f60686f71f4afec7b2925b4682f33c70c0a7eb17
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date:   Wed Mar 9 07:48:24 2011 -0300
> 
>     perf evsel: Assume rest of perf_header_attr functions
> 
> commit e62e48df064b431029658089ab1f8b529ff6da3d
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date:   Tue Mar 8 09:05:57 2011 -0300
> 
>     perf session: Simplify evlist creation from perf.data header
> 
> perf built after these commits is able to symbolize properly for
> perf.data collected using an older version of perf. So I believe, the
> perf report side of things are ok. But perf record is not.

Thanks for bisecting, I noticed some problems yesterday and was trying
to bisect it, but run into another bug, will investigate now.

- Arnaldo

Re: perf record regression?

[Arnaldo Carvalho de Melo]

Em Thu, Mar 10, 2011 at 11:49:28AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Mar 09, 2011 at 09:59:39PM -0800, Arun Sharma escreveu:
> > I pulled the perf/core branch in
> > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux-2.6.git today
> > and found that:
> > 
> > perf record -ag -- sleep 1
> > perf report -D | grep dso:
> > 
> > was returning:
> > 
> >  ...... dso: <not found>
> >  ...... dso: <not found>
> >  ...... dso: /boot/vmlinux-2.6.38-rc5+
> > 
> > i.e perf report was not able to map IP addresses to the correct dso
> > for anything in user space.
> > perf built after these commits is able to symbolize properly for
> > perf.data collected using an older version of perf. So I believe, the
> > perf report side of things are ok. But perf record is not.
> 
> Thanks for bisecting, I noticed some problems yesterday and was trying
> to bisect it, but run into another bug, will investigate now.

Can you try with this one? Has the fix plus some simplifications.

The fix is to call perf_session__update_sample_type after we have the
perf_evsel created or read from the perf.data file. We need to do that
to cope with having or not sample_id_all, etc, that affects how we parse
the events.

This area still needs some work to support events with different
sample_types, the recent changes that introduced evsels and evlists are
a step in that direction.

- Arnaldo

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 81dbe27..40c6b10 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -39,7 +39,6 @@ enum write_mode_t {
 
 static u64			user_interval			= ULLONG_MAX;
 static u64			default_interval		=      0;
-static u64			sample_type;
 
 static unsigned int		page_size;
 static unsigned int		mmap_pages			=    128;
@@ -342,7 +341,7 @@ try_again:
 		}
 	}
 
-	sample_type = pos->attr.sample_type;
+	perf_session__update_sample_type(session);
 }
 
 static int process_buildids(void)
@@ -563,8 +562,6 @@ static int __cmd_record(int argc, const char **argv)
 
 	open_counters(evsel_list);
 
-	perf_session__set_sample_type(session, sample_type);
-
 	/*
 	 * perf_session__delete(session) will be called at atexit_header()
 	 */
@@ -583,8 +580,6 @@ static int __cmd_record(int argc, const char **argv)
 
 	post_processing_offset = lseek(output, 0, SEEK_CUR);
 
-	perf_session__set_sample_id_all(session, sample_id_all_avail);
-
 	if (pipe_output) {
 		err = perf_session__synthesize_attrs(session,
 						     process_synthesized_event);
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 417f757..847a9e0 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -883,7 +883,6 @@ try_again:
 static int __cmd_top(void)
 {
 	pthread_t thread;
-	struct perf_evsel *first;
 	int ret __used;
 	/*
 	 * FIXME: perf_session__new should allow passing a O_MMAP, so that all this
@@ -900,8 +899,7 @@ static int __cmd_top(void)
 		perf_event__synthesize_threads(perf_event__process, session);
 
 	start_counters(top.evlist);
-	first = list_entry(top.evlist->entries.next, struct perf_evsel, node);
-	perf_session__set_sample_type(session, first->attr.sample_type);
+	perf_session__update_sample_type(session);
 
 	/* Wait for a minimal set of events before starting the snapshot */
 	poll(top.evlist->pollfd, top.evlist->nr_fds, 100);
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index f642615..f26639f 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -94,17 +94,6 @@ out:
        session->id_hdr_size = size;
 }
 
-void perf_session__set_sample_id_all(struct perf_session *session, bool value)
-{
-	session->sample_id_all = value;
-	perf_session__id_header_size(session);
-}
-
-void perf_session__set_sample_type(struct perf_session *session, u64 type)
-{
-	session->sample_type = type;
-}
-
 void perf_session__update_sample_type(struct perf_session *self)
 {
 	self->sample_type = perf_evlist__sample_type(self->evlist);
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index 05dd7bc..b5b148b 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -112,8 +112,6 @@ void mem_bswap_64(void *src, int byte_size);
 int perf_session__create_kernel_maps(struct perf_session *self);
 
 void perf_session__update_sample_type(struct perf_session *self);
-void perf_session__set_sample_id_all(struct perf_session *session, bool value);
-void perf_session__set_sample_type(struct perf_session *session, u64 type);
 void perf_session__remove_thread(struct perf_session *self, struct thread *th);
 
 static inline

Re: perf record regression?

[Arnaldo Carvalho de Melo]

Em Thu, Mar 10, 2011 at 12:28:07PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Mar 10, 2011 at 11:49:28AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Mar 09, 2011 at 09:59:39PM -0800, Arun Sharma escreveu:
> > > I pulled the perf/core branch in
> > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux-2.6.git today
> > > and found that:
> > > 
> > > perf record -ag -- sleep 1
> > > perf report -D | grep dso:
> > > 
> > > was returning:
> > > 
> > >  ...... dso: <not found>
> > >  ...... dso: <not found>
> > >  ...... dso: /boot/vmlinux-2.6.38-rc5+
> > > 
> > > i.e perf report was not able to map IP addresses to the correct dso
> > > for anything in user space.
> > > perf built after these commits is able to symbolize properly for
> > > perf.data collected using an older version of perf. So I believe, the
> > > perf report side of things are ok. But perf record is not.
> > 
> > Thanks for bisecting, I noticed some problems yesterday and was trying
> > to bisect it, but run into another bug, will investigate now.
> 
> Can you try with this one? Has the fix plus some simplifications.

Ah, just noticed that Ingo didn't merge this branch due to some other
isues with 'perf script' patches from David Ahern, so I'll just fold
this before resubmitting to him, so that we remove this problem from our
bisectable upstream :-)

- Arnaldo