[GIT PULL 0/3] perf fixes (inject, report, record)

[GIT PULL 0/3] perf fixes (inject, report, record)

[Arnaldo Carvalho de Melo]

Hi Ingo,

	Please pull from:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux-2.6 perf

Regards,

- Arnaldo

Anton Blanchard (1):
  perf: Fix performance issue with perf report

Arnaldo Carvalho de Melo (1):
  perf inject: Add missing bits

Tom Zanussi (1):
  perf: record TRACE_INFO only if using tracepoints and SAMPLE_RAW

 tools/perf/Documentation/perf-buildid-cache.txt |    4 +-
 tools/perf/Documentation/perf-inject.txt        |   35 +++++++++++++++++++++++
 tools/perf/builtin-inject.c                     |    2 +-
 tools/perf/builtin-record.c                     |   35 +++++++++++++---------
 tools/perf/command-list.txt                     |    1 +
 tools/perf/util/event.c                         |    1 +
 tools/perf/util/header.c                        |    9 +++++-
 tools/perf/util/parse-events.h                  |    1 +
 tools/perf/util/trace-event-info.c              |    5 +++
 9 files changed, 75 insertions(+), 18 deletions(-)
 create mode 100644 tools/perf/Documentation/perf-inject.txt

[PATCH 1/3] perf: record TRACE_INFO only if using tracepoints and SAMPLE_RAW

[Arnaldo Carvalho de Melo]

From: Tom Zanussi <tzanussi@gmail.com>

The current perf code implicitly assumes SAMPLE_RAW means tracepoints
are being used, but doesn't check for that.  It happily records the
TRACE_INFO even if SAMPLE_RAW is used without tracepoints, but when the
perf data is read it won't go any further when it finds TRACE_INFO but
no tracepoints, and displays misleading errors.

This adds a check for both in perf-record, and won't record TRACE_INFO
unless both are true.  This at least allows perf report -D to dump raw
events, and avoids triggering a misleading error condition in perf
trace.  It doesn't actually enable the non-tracepoint raw events to be
displayed in perf trace, since perf trace currently only deals with
tracepoint events.

Cc: Frédéric Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <1272865861.7932.16.camel@tropicana>
Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c        |   35 +++++++++++++++++++++--------------
 tools/perf/util/header.c           |    1 -
 tools/perf/util/parse-events.h     |    1 +
 tools/perf/util/trace-event-info.c |    5 +++++
 4 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ac989e9..0ff67d1 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -560,11 +560,12 @@ static int __cmd_record(int argc, const char **argv)
 			return err;
 	}
 
-	if (raw_samples) {
+	if (raw_samples && have_tracepoints(attrs, nr_counters)) {
 		perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
 	} else {
 		for (i = 0; i < nr_counters; i++) {
-			if (attrs[i].sample_type & PERF_SAMPLE_RAW) {
+			if (attrs[i].sample_type & PERF_SAMPLE_RAW &&
+				attrs[i].type == PERF_TYPE_TRACEPOINT) {
 				perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
 				break;
 			}
@@ -662,19 +663,25 @@ static int __cmd_record(int argc, const char **argv)
 			return err;
 		}
 
-		err = event__synthesize_tracing_data(output, attrs,
-						     nr_counters,
-						     process_synthesized_event,
-						     session);
-		/*
-		 * FIXME err <= 0 here actually means that there were no tracepoints
-		 * so its not really an error, just that we don't need to synthesize
-		 * anything.
-		 * We really have to return this more properly and also propagate
-		 * errors that now are calling die()
-		 */
-		if (err > 0)
+		if (have_tracepoints(attrs, nr_counters)) {
+			/*
+			 * FIXME err <= 0 here actually means that
+			 * there were no tracepoints so its not really
+			 * an error, just that we don't need to
+			 * synthesize anything.  We really have to
+			 * return this more properly and also
+			 * propagate errors that now are calling die()
+			 */
+			err = event__synthesize_tracing_data(output, attrs,
+							     nr_counters,
+							     process_synthesized_event,
+							     session);
+			if (err <= 0) {
+				pr_err("Couldn't record tracing data.\n");
+				return err;
+			}
 			advance_output(err);
+		}
 	}
 
 	machine = perf_session__find_host_machine(session);
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 79da0e5..2b9f898 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -436,7 +436,6 @@ static int perf_header__adds_write(struct perf_header *self, int fd)
 		trace_sec->size = lseek(fd, 0, SEEK_CUR) - trace_sec->offset;
 	}
 
-
 	if (perf_header__has_feat(self, HEADER_BUILD_ID)) {
 		struct perf_file_section *buildid_sec;
 
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index b8c1f64..fc4ab3f 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -13,6 +13,7 @@ struct tracepoint_path {
 };
 
 extern struct tracepoint_path *tracepoint_id_to_path(u64 config);
+extern bool have_tracepoints(struct perf_event_attr *pattrs, int nb_events);
 
 extern int			nr_counters;
 
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 30cd9b5..0a1fb9d 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -487,6 +487,11 @@ get_tracepoints_path(struct perf_event_attr *pattrs, int nb_events)
 	return nr_tracepoints > 0 ? path.next : NULL;
 }
 
+bool have_tracepoints(struct perf_event_attr *pattrs, int nb_events)
+{
+	return get_tracepoints_path(pattrs, nb_events) ? true : false;
+}
+
 int read_tracing_data(int fd, struct perf_event_attr *pattrs, int nb_events)
 {
 	char buf[BUFSIZ];
-- 
1.6.2.5

Re: [PATCH 1/3] perf: record TRACE_INFO only if using tracepoints and SAMPLE_RAW

[Frederic Weisbecker]

On Tue, May 04, 2010 at 11:00:05AM -0300, Arnaldo Carvalho de Melo wrote:
> From: Tom Zanussi <tzanussi@gmail.com>
> 
> The current perf code implicitly assumes SAMPLE_RAW means tracepoints
> are being used, but doesn't check for that.  It happily records the
> TRACE_INFO even if SAMPLE_RAW is used without tracepoints, but when the
> perf data is read it won't go any further when it finds TRACE_INFO but
> no tracepoints, and displays misleading errors.
> 
> This adds a check for both in perf-record, and won't record TRACE_INFO
> unless both are true.  This at least allows perf report -D to dump raw
> events, and avoids triggering a misleading error condition in perf
> trace.  It doesn't actually enable the non-tracepoint raw events to be
> displayed in perf trace, since perf trace currently only deals with
> tracepoint events.
> 
> Cc: Frédéric Weisbecker <fweisbec@gmail.com>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> LKML-Reference: <1272865861.7932.16.camel@tropicana>
> Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/builtin-record.c        |   35 +++++++++++++++++++++--------------
>  tools/perf/util/header.c           |    1 -
>  tools/perf/util/parse-events.h     |    1 +
>  tools/perf/util/trace-event-info.c |    5 +++++
>  4 files changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index ac989e9..0ff67d1 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -560,11 +560,12 @@ static int __cmd_record(int argc, const char **argv)
>  			return err;
>  	}
>  
> -	if (raw_samples) {
> +	if (raw_samples && have_tracepoints(attrs, nr_counters)) {
>  		perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
>  	} else {



Using get_tracepoints_path() is a bit costly just to check if we use
tracepoints as it allocates and fill the paths.



>  		for (i = 0; i < nr_counters; i++) {
> -			if (attrs[i].sample_type & PERF_SAMPLE_RAW) {
> +			if (attrs[i].sample_type & PERF_SAMPLE_RAW &&
> +				attrs[i].type == PERF_TYPE_TRACEPOINT) {
>  				perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
>  				break;
>  			}



In fact now we always have PERF_SAMPLE_RAW for tracepoints.



> @@ -662,19 +663,25 @@ static int __cmd_record(int argc, const char **argv)
>  			return err;
>  		}
>  
> -		err = event__synthesize_tracing_data(output, attrs,
> -						     nr_counters,
> -						     process_synthesized_event,
> -						     session);
> -		/*
> -		 * FIXME err <= 0 here actually means that there were no tracepoints
> -		 * so its not really an error, just that we don't need to synthesize
> -		 * anything.
> -		 * We really have to return this more properly and also propagate
> -		 * errors that now are calling die()
> -		 */
> -		if (err > 0)
> +		if (have_tracepoints(attrs, nr_counters)) {
> +			/*
> +			 * FIXME err <= 0 here actually means that
> +			 * there were no tracepoints so its not really
> +			 * an error, just that we don't need to
> +			 * synthesize anything.  We really have to
> +			 * return this more properly and also
> +			 * propagate errors that now are calling die()
> +			 */
> +			err = event__synthesize_tracing_data(output, attrs,
> +							     nr_counters,
> +							     process_synthesized_event,
> +							     session);
> +			if (err <= 0) {
> +				pr_err("Couldn't record tracing data.\n");
> +				return err;
> +			}
>  			advance_output(err);
> +		}
>  	}



Now get_tracepoints_path() may be called three times. You are leaking some
memory by doing that as the paths are reallocated without beeing released.




>  
>  	machine = perf_session__find_host_machine(session);
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 79da0e5..2b9f898 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -436,7 +436,6 @@ static int perf_header__adds_write(struct perf_header *self, int fd)
>  		trace_sec->size = lseek(fd, 0, SEEK_CUR) - trace_sec->offset;
>  	}
>  
> -
>  	if (perf_header__has_feat(self, HEADER_BUILD_ID)) {
>  		struct perf_file_section *buildid_sec;
>  
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index b8c1f64..fc4ab3f 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -13,6 +13,7 @@ struct tracepoint_path {
>  };
>  
>  extern struct tracepoint_path *tracepoint_id_to_path(u64 config);
> +extern bool have_tracepoints(struct perf_event_attr *pattrs, int nb_events);
>  
>  extern int			nr_counters;
>  
> diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
> index 30cd9b5..0a1fb9d 100644
> --- a/tools/perf/util/trace-event-info.c
> +++ b/tools/perf/util/trace-event-info.c
> @@ -487,6 +487,11 @@ get_tracepoints_path(struct perf_event_attr *pattrs, int nb_events)
>  	return nr_tracepoints > 0 ? path.next : NULL;
>  }
>  
> +bool have_tracepoints(struct perf_event_attr *pattrs, int nb_events)
> +{
> +	return get_tracepoints_path(pattrs, nb_events) ? true : false;
> +}
> +
>  int read_tracing_data(int fd, struct perf_event_attr *pattrs, int nb_events)
>  {
>  	char buf[BUFSIZ];
> -- 
> 1.6.2.5
> 

Re: [PATCH 1/3] perf: record TRACE_INFO only if using tracepoints and SAMPLE_RAW

[Arnaldo Carvalho de Melo]

Em Tue, May 04, 2010 at 07:06:45PM +0200, Frederic Weisbecker escreveu:
> On Tue, May 04, 2010 at 11:00:05AM -0300, Arnaldo Carvalho de Melo wrote:
> > From: Tom Zanussi <tzanussi@gmail.com>
> > 
> > The current perf code implicitly assumes SAMPLE_RAW means tracepoints
> > are being used, but doesn't check for that.  It happily records the
> > TRACE_INFO even if SAMPLE_RAW is used without tracepoints, but when the
> > perf data is read it won't go any further when it finds TRACE_INFO but
> > no tracepoints, and displays misleading errors.
> > 
> > This adds a check for both in perf-record, and won't record TRACE_INFO
> > unless both are true.  This at least allows perf report -D to dump raw
> > events, and avoids triggering a misleading error condition in perf
> > trace.  It doesn't actually enable the non-tracepoint raw events to be
> > displayed in perf trace, since perf trace currently only deals with
> > tracepoint events.
> > 
> > Cc: Frédéric Weisbecker <fweisbec@gmail.com>
> > Cc: Mike Galbraith <efault@gmx.de>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > LKML-Reference: <1272865861.7932.16.camel@tropicana>
> > Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> >  tools/perf/builtin-record.c        |   35 +++++++++++++++++++++--------------
> >  tools/perf/util/header.c           |    1 -
> >  tools/perf/util/parse-events.h     |    1 +
> >  tools/perf/util/trace-event-info.c |    5 +++++
> >  4 files changed, 27 insertions(+), 15 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index ac989e9..0ff67d1 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -560,11 +560,12 @@ static int __cmd_record(int argc, const char **argv)
> >  			return err;
> >  	}
> >  
> > -	if (raw_samples) {
> > +	if (raw_samples && have_tracepoints(attrs, nr_counters)) {
> >  		perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
> >  	} else {
> 
> 
> 
> Using get_tracepoints_path() is a bit costly just to check if we use
> tracepoints as it allocates and fill the paths.

Can you please send a fix?

- Arnaldo

Re: [PATCH 1/3] perf: record TRACE_INFO only if using tracepoints and SAMPLE_RAW

[Tom Zanussi]

On Tue, 2010-05-04 at 18:18 -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, May 04, 2010 at 07:06:45PM +0200, Frederic Weisbecker escreveu:
> > On Tue, May 04, 2010 at 11:00:05AM -0300, Arnaldo Carvalho de Melo wrote:
> > > From: Tom Zanussi <tzanussi@gmail.com>
> > > 
> > > The current perf code implicitly assumes SAMPLE_RAW means tracepoints
> > > are being used, but doesn't check for that.  It happily records the
> > > TRACE_INFO even if SAMPLE_RAW is used without tracepoints, but when the
> > > perf data is read it won't go any further when it finds TRACE_INFO but
> > > no tracepoints, and displays misleading errors.
> > > 
> > > This adds a check for both in perf-record, and won't record TRACE_INFO
> > > unless both are true.  This at least allows perf report -D to dump raw
> > > events, and avoids triggering a misleading error condition in perf
> > > trace.  It doesn't actually enable the non-tracepoint raw events to be
> > > displayed in perf trace, since perf trace currently only deals with
> > > tracepoint events.
> > > 
> > > Cc: Frédéric Weisbecker <fweisbec@gmail.com>
> > > Cc: Mike Galbraith <efault@gmx.de>
> > > Cc: Paul Mackerras <paulus@samba.org>
> > > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > LKML-Reference: <1272865861.7932.16.camel@tropicana>
> > > Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
> > > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > ---
> > >  tools/perf/builtin-record.c        |   35 +++++++++++++++++++++--------------
> > >  tools/perf/util/header.c           |    1 -
> > >  tools/perf/util/parse-events.h     |    1 +
> > >  tools/perf/util/trace-event-info.c |    5 +++++
> > >  4 files changed, 27 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > > index ac989e9..0ff67d1 100644
> > > --- a/tools/perf/builtin-record.c
> > > +++ b/tools/perf/builtin-record.c
> > > @@ -560,11 +560,12 @@ static int __cmd_record(int argc, const char **argv)
> > >  			return err;
> > >  	}
> > >  
> > > -	if (raw_samples) {
> > > +	if (raw_samples && have_tracepoints(attrs, nr_counters)) {
> > >  		perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
> > >  	} else {
> > 
> > 
> > 
> > Using get_tracepoints_path() is a bit costly just to check if we use
> > tracepoints as it allocates and fill the paths.
> 
> Can you please send a fix?
> 

Yeah, there's a lot of room for improvement here - thanks for pointing
it out, Frederic.  The patch below should make it better...

Tom

From: Tom Zanussi <tzanussi@gmail.com>
Date: Tue, 4 May 2010 22:20:16 -0500
Subject: [PATCH] perf/record: simplify TRACE_INFO tracepoint check

Fix a couple of inefficiencies and redundancies related to
have_tracepoints() and its use when checking whether to write
TRACE_INFO.

First, there's no need to use get_tracepoints_path() in
have_tracepoints() - we really just want the part that checks whether
any attributes correspondo to tracepoints.

Second, we really don't care about raw_samples per se - tracepoints
are always raw_samples.  In any case, the have_tracepoints() check
should be sufficient to decide whether or not to write TRACE_INFO.

Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
---
 tools/perf/builtin-record.c        |   11 +----------
 tools/perf/util/trace-event-info.c |    8 +++++++-
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 0ff67d1..d3981ac 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -560,17 +560,8 @@ static int __cmd_record(int argc, const char **argv)
 			return err;
 	}
 
-	if (raw_samples && have_tracepoints(attrs, nr_counters)) {
+	if (have_tracepoints(attrs, nr_counters))
 		perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
-	} else {
-		for (i = 0; i < nr_counters; i++) {
-			if (attrs[i].sample_type & PERF_SAMPLE_RAW &&
-				attrs[i].type == PERF_TYPE_TRACEPOINT) {
-				perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
-				break;
-			}
-		}
-	}
 
 	atexit(atexit_header);
 
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 0a1fb9d..b157260 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -489,7 +489,13 @@ get_tracepoints_path(struct perf_event_attr *pattrs, int nb_events)
 
 bool have_tracepoints(struct perf_event_attr *pattrs, int nb_events)
 {
-	return get_tracepoints_path(pattrs, nb_events) ? true : false;
+	int i;
+
+	for (i = 0; i < nb_events; i++)
+		if (pattrs[i].type == PERF_TYPE_TRACEPOINT)
+			return true;
+
+	return false;
 }
 
 int read_tracing_data(int fd, struct perf_event_attr *pattrs, int nb_events)
-- 
1.6.4.GIT

Re: [PATCH 1/3] perf: record TRACE_INFO only if using tracepoints and SAMPLE_RAW

[Frederic Weisbecker]

On Tue, May 04, 2010 at 10:39:30PM -0500, Tom Zanussi wrote:
> On Tue, 2010-05-04 at 18:18 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, May 04, 2010 at 07:06:45PM +0200, Frederic Weisbecker escreveu:
> > > On Tue, May 04, 2010 at 11:00:05AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > From: Tom Zanussi <tzanussi@gmail.com>
> > > > 
> > > > The current perf code implicitly assumes SAMPLE_RAW means tracepoints
> > > > are being used, but doesn't check for that.  It happily records the
> > > > TRACE_INFO even if SAMPLE_RAW is used without tracepoints, but when the
> > > > perf data is read it won't go any further when it finds TRACE_INFO but
> > > > no tracepoints, and displays misleading errors.
> > > > 
> > > > This adds a check for both in perf-record, and won't record TRACE_INFO
> > > > unless both are true.  This at least allows perf report -D to dump raw
> > > > events, and avoids triggering a misleading error condition in perf
> > > > trace.  It doesn't actually enable the non-tracepoint raw events to be
> > > > displayed in perf trace, since perf trace currently only deals with
> > > > tracepoint events.
> > > > 
> > > > Cc: Frédéric Weisbecker <fweisbec@gmail.com>
> > > > Cc: Mike Galbraith <efault@gmx.de>
> > > > Cc: Paul Mackerras <paulus@samba.org>
> > > > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > > LKML-Reference: <1272865861.7932.16.camel@tropicana>
> > > > Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
> > > > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > > ---
> > > >  tools/perf/builtin-record.c        |   35 +++++++++++++++++++++--------------
> > > >  tools/perf/util/header.c           |    1 -
> > > >  tools/perf/util/parse-events.h     |    1 +
> > > >  tools/perf/util/trace-event-info.c |    5 +++++
> > > >  4 files changed, 27 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > > > index ac989e9..0ff67d1 100644
> > > > --- a/tools/perf/builtin-record.c
> > > > +++ b/tools/perf/builtin-record.c
> > > > @@ -560,11 +560,12 @@ static int __cmd_record(int argc, const char **argv)
> > > >  			return err;
> > > >  	}
> > > >  
> > > > -	if (raw_samples) {
> > > > +	if (raw_samples && have_tracepoints(attrs, nr_counters)) {
> > > >  		perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
> > > >  	} else {
> > > 
> > > 
> > > 
> > > Using get_tracepoints_path() is a bit costly just to check if we use
> > > tracepoints as it allocates and fill the paths.
> > 
> > Can you please send a fix?
> > 
> 
> Yeah, there's a lot of room for improvement here - thanks for pointing
> it out, Frederic.  The patch below should make it better...
> 
> Tom
> 
> From: Tom Zanussi <tzanussi@gmail.com>
> Date: Tue, 4 May 2010 22:20:16 -0500
> Subject: [PATCH] perf/record: simplify TRACE_INFO tracepoint check
> 
> Fix a couple of inefficiencies and redundancies related to
> have_tracepoints() and its use when checking whether to write
> TRACE_INFO.
> 
> First, there's no need to use get_tracepoints_path() in
> have_tracepoints() - we really just want the part that checks whether
> any attributes correspondo to tracepoints.
> 
> Second, we really don't care about raw_samples per se - tracepoints
> are always raw_samples.  In any case, the have_tracepoints() check
> should be sufficient to decide whether or not to write TRACE_INFO.
> 
> Signed-off-by: Tom Zanussi <tzanussi@gmail.com>



Thanks a lot!

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>



> ---
>  tools/perf/builtin-record.c        |   11 +----------
>  tools/perf/util/trace-event-info.c |    8 +++++++-
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 0ff67d1..d3981ac 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -560,17 +560,8 @@ static int __cmd_record(int argc, const char **argv)
>  			return err;
>  	}
>  
> -	if (raw_samples && have_tracepoints(attrs, nr_counters)) {
> +	if (have_tracepoints(attrs, nr_counters))
>  		perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
> -	} else {
> -		for (i = 0; i < nr_counters; i++) {
> -			if (attrs[i].sample_type & PERF_SAMPLE_RAW &&
> -				attrs[i].type == PERF_TYPE_TRACEPOINT) {
> -				perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
> -				break;
> -			}
> -		}
> -	}
>  
>  	atexit(atexit_header);
>  
> diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
> index 0a1fb9d..b157260 100644
> --- a/tools/perf/util/trace-event-info.c
> +++ b/tools/perf/util/trace-event-info.c
> @@ -489,7 +489,13 @@ get_tracepoints_path(struct perf_event_attr *pattrs, int nb_events)
>  
>  bool have_tracepoints(struct perf_event_attr *pattrs, int nb_events)
>  {
> -	return get_tracepoints_path(pattrs, nb_events) ? true : false;
> +	int i;
> +
> +	for (i = 0; i < nb_events; i++)
> +		if (pattrs[i].type == PERF_TYPE_TRACEPOINT)
> +			return true;
> +
> +	return false;
>  }
>  
>  int read_tracing_data(int fd, struct perf_event_attr *pattrs, int nb_events)
> -- 
> 1.6.4.GIT
> 
> 
> 
> 
> 

[tip:perf/core] perf/record: simplify TRACE_INFO tracepoint check

[tip-bot for Tom Zanussi]

Commit-ID:  db620b1c2fb172346dc54eb62bba9b4a117d173b
Gitweb:     http://git.kernel.org/tip/db620b1c2fb172346dc54eb62bba9b4a117d173b
Author:     Tom Zanussi <tzanussi@gmail.com>
AuthorDate: Tue, 4 May 2010 22:20:16 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 5 May 2010 11:12:53 -0300

perf/record: simplify TRACE_INFO tracepoint check

Fix a couple of inefficiencies and redundancies related to
have_tracepoints() and its use when checking whether to write
TRACE_INFO.

First, there's no need to use get_tracepoints_path() in
have_tracepoints() - we really just want the part that checks whether
any attributes correspondo to tracepoints.

Second, we really don't care about raw_samples per se - tracepoints
are always raw_samples.  In any case, the have_tracepoints() check
should be sufficient to decide whether or not to write TRACE_INFO.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>,
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <1273030770.6383.6.camel@tropicana>
Signed-off-by: Tom Zanussi <tzanussi@gmail.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c        |   11 +----------
 tools/perf/util/trace-event-info.c |    8 +++++++-
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 0ff67d1..d3981ac 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -560,17 +560,8 @@ static int __cmd_record(int argc, const char **argv)
 			return err;
 	}
 
-	if (raw_samples && have_tracepoints(attrs, nr_counters)) {
+	if (have_tracepoints(attrs, nr_counters))
 		perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
-	} else {
-		for (i = 0; i < nr_counters; i++) {
-			if (attrs[i].sample_type & PERF_SAMPLE_RAW &&
-				attrs[i].type == PERF_TYPE_TRACEPOINT) {
-				perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
-				break;
-			}
-		}
-	}
 
 	atexit(atexit_header);
 
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 0a1fb9d..b157260 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -489,7 +489,13 @@ get_tracepoints_path(struct perf_event_attr *pattrs, int nb_events)
 
 bool have_tracepoints(struct perf_event_attr *pattrs, int nb_events)
 {
-	return get_tracepoints_path(pattrs, nb_events) ? true : false;
+	int i;
+
+	for (i = 0; i < nb_events; i++)
+		if (pattrs[i].type == PERF_TYPE_TRACEPOINT)
+			return true;
+
+	return false;
 }
 
 int read_tracing_data(int fd, struct perf_event_attr *pattrs, int nb_events)

[PATCH 2/3] perf inject: Add missing bits

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

New commands need to have Documentation and be added to command-list.txt
so that they can appear when 'perf' is called withouth any subcommand:

[root@doppio linux-2.6-tip]# perf

 usage: perf [--version] [--help] COMMAND [ARGS]

 The most commonly used perf commands are:
   annotate        Read perf.data (created by perf record) and display annotated code
   archive         Create archive with object files with build-ids found in perf.data file
   bench           General framework for benchmark suites
   buildid-cache   Manage build-id cache.
   buildid-list    List the buildids in a perf.data file
   diff            Read two perf.data files and display the differential profile
   inject          Filter to augment the events stream with additional information
   kmem            Tool to trace/measure kernel memory(slab) properties
   kvm             Tool to trace/measure kvm guest os
   list            List all symbolic event types
   lock            Analyze lock events
   probe           Define new dynamic tracepoints
   record          Run a command and record its profile into perf.data
   report          Read perf.data (created by perf record) and display the profile
   sched           Tool to trace/measure scheduler properties (latencies)
   stat            Run a command and gather performance counter statistics
   test            Runs sanity tests.
   timechart       Tool to visualize total system behavior during a workload
   top             System profiling tool.
   trace           Read perf.data (created by perf record) and display trace output

 See 'perf help COMMAND' for more information on a specific command.

[root@doppio linux-2.6-tip]#

The new 'perf inject' command hadn't so it wasn't appearing on that list.

Also fix the long option, that should have no spaces in it, rename the faulty one
to be '--build-ids', instead of '--inject build-ids'.

Reported-by: Ingo Molnar <mingo@elte.hu>
Cc: Frédéric Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Tom Zanussi <tzanussi@gmail.com>
LKML-Reference: <new-submission>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Documentation/perf-buildid-cache.txt |    4 +-
 tools/perf/Documentation/perf-inject.txt        |   35 +++++++++++++++++++++++
 tools/perf/builtin-inject.c                     |    2 +-
 tools/perf/command-list.txt                     |    1 +
 4 files changed, 39 insertions(+), 3 deletions(-)
 create mode 100644 tools/perf/Documentation/perf-inject.txt

diff --git a/tools/perf/Documentation/perf-buildid-cache.txt b/tools/perf/Documentation/perf-buildid-cache.txt
index 88bc3b5..5d1a950 100644
--- a/tools/perf/Documentation/perf-buildid-cache.txt
+++ b/tools/perf/Documentation/perf-buildid-cache.txt
@@ -8,7 +8,7 @@ perf-buildid-cache - Manage build-id cache.
 SYNOPSIS
 --------
 [verse]
-'perf buildid-list <options>'
+'perf buildid-cache <options>'
 
 DESCRIPTION
 -----------
@@ -30,4 +30,4 @@ OPTIONS
 
 SEE ALSO
 --------
-linkperf:perf-record[1], linkperf:perf-report[1]
+linkperf:perf-record[1], linkperf:perf-report[1], linkperf:perf-buildid-list[1]
diff --git a/tools/perf/Documentation/perf-inject.txt b/tools/perf/Documentation/perf-inject.txt
new file mode 100644
index 0000000..025630d
--- /dev/null
+++ b/tools/perf/Documentation/perf-inject.txt
@@ -0,0 +1,35 @@
+perf-inject(1)
+==============
+
+NAME
+----
+perf-inject - Filter to augment the events stream with additional information
+
+SYNOPSIS
+--------
+[verse]
+'perf inject <options>'
+
+DESCRIPTION
+-----------
+perf-inject reads a perf-record event stream and repipes it to stdout.  At any
+point the processing code can inject other events into the event stream - in
+this case build-ids (-b option) are read and injected as needed into the event
+stream.
+
+Build-ids are just the first user of perf-inject - potentially anything that
+needs userspace processing to augment the events stream with additional
+information could make use of this facility.
+
+OPTIONS
+-------
+-b::
+--build-ids=::
+        Inject build-ids into the output stream
+-v::
+--verbose::
+	Be more verbose.
+
+SEE ALSO
+--------
+linkperf:perf-record[1], linkperf:perf-report[1], linkperf:perf-archive[1]
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 59e981a..8e3e47b 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -204,7 +204,7 @@ static const char * const report_usage[] = {
 };
 
 static const struct option options[] = {
-	OPT_BOOLEAN('b', "inject build-ids", &inject_build_ids,
+	OPT_BOOLEAN('b', "build-ids", &inject_build_ids,
 		    "Inject build-ids into the output stream"),
 	OPT_INCR('v', "verbose", &verbose,
 		 "be more verbose (show build ids, etc)"),
diff --git a/tools/perf/command-list.txt b/tools/perf/command-list.txt
index 80a1a44..949d77f 100644
--- a/tools/perf/command-list.txt
+++ b/tools/perf/command-list.txt
@@ -8,6 +8,7 @@ perf-bench			mainporcelain common
 perf-buildid-cache		mainporcelain common
 perf-buildid-list		mainporcelain common
 perf-diff			mainporcelain common
+perf-inject			mainporcelain common
 perf-list			mainporcelain common
 perf-sched			mainporcelain common
 perf-record			mainporcelain common
-- 
1.6.2.5

Re: [PATCH 2/3] perf inject: Add missing bits

[Tom Zanussi]

On Tue, 2010-05-04 at 11:00 -0300, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> New commands need to have Documentation and be added to command-list.txt
> so that they can appear when 'perf' is called withouth any subcommand:
> 

Thanks for adding this!

Tom

[PATCH 3/3] perf: Fix performance issue with perf report

[Arnaldo Carvalho de Melo]

From: Anton Blanchard <anton@samba.org>

On a large machine we spend a lot of time in perf_header__find_attr when
running perf report.

If we are parsing a file without PERF_SAMPLE_ID then for each sample we call
perf_header__find_attr and loop through all counter IDs, never finding a match.
As the machine gets larger there are more per cpu counters and we spend an
awful lot of time in there.

The patch below initialises each sample id to -1ULL and checks for this in
perf_header__find_attr. We may need to do something more intelligent eventually
(eg a hash lookup from counter id to attr) but this at least fixes the most
common usage of perf report.

Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Eric B Munson <ebmunson@us.ibm.com>
Acked-by: Eric B Munson <ebmunson@us.ibm.com>
LKML-Reference: <20100504111915.GB14636@kryten>
Signed-off-by: Anton Blanchard <anton@samba.org>
--
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/event.c  |    1 +
 tools/perf/util/header.c |    8 ++++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 1757b0f..2477270 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -713,6 +713,7 @@ int event__parse_sample(event_t *event, u64 type, struct sample_data *data)
 		array++;
 	}
 
+	data->id = -1ULL;
 	if (type & PERF_SAMPLE_ID) {
 		data->id = *array;
 		array++;
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 2b9f898..8847bec 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -922,6 +922,14 @@ perf_header__find_attr(u64 id, struct perf_header *header)
 {
 	int i;
 
+	/*
+	 * We set id to -1 if the data file doesn't contain sample
+	 * ids. Check for this and avoid walking through the entire
+	 * list of ids which may be large.
+	 */
+	if (id == -1ULL)
+		return NULL;
+
 	for (i = 0; i < header->attrs; i++) {
 		struct perf_header_attr *attr = header->attr[i];
 		int j;
-- 
1.6.2.5

Re: [GIT PULL 0/3] perf fixes (inject, report, record)

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@infradead.org> wrote:

> Hi Ingo,
> 
> 	Please pull from:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux-2.6 perf
> 
> Regards,
> 
> - Arnaldo
> 
> Anton Blanchard (1):
>   perf: Fix performance issue with perf report
> 
> Arnaldo Carvalho de Melo (1):
>   perf inject: Add missing bits
> 
> Tom Zanussi (1):
>   perf: record TRACE_INFO only if using tracepoints and SAMPLE_RAW
> 
>  tools/perf/Documentation/perf-buildid-cache.txt |    4 +-
>  tools/perf/Documentation/perf-inject.txt        |   35 +++++++++++++++++++++++
>  tools/perf/builtin-inject.c                     |    2 +-
>  tools/perf/builtin-record.c                     |   35 +++++++++++++---------
>  tools/perf/command-list.txt                     |    1 +
>  tools/perf/util/event.c                         |    1 +
>  tools/perf/util/header.c                        |    9 +++++-
>  tools/perf/util/parse-events.h                  |    1 +
>  tools/perf/util/trace-event-info.c              |    5 +++
>  9 files changed, 75 insertions(+), 18 deletions(-)
>  create mode 100644 tools/perf/Documentation/perf-inject.txt

Pulled, thanks Arnaldo!

	Ingo