[PATCH] perf, tool, record: Fix the header generation for pipe

[PATCH] perf, tool, record: Fix the header generation for pipe

[Jiri Olsa]

The generation of the perf data file fails when using pipe
as the output file descriptor.

When record command generates the data header, several files are put
inside and the file size is stored ahead of the file contents itself.

The problem is that debugfs files cannot be stat-ed for size so we
need to read the whole file, count the size and update the file size
via seek&write (pwrite call).
This cannot be done for pipes, since it's not allowed to seek on it.

The attached patch changes current behaviour for pipes to get the
file size first and write correct data within the first pass.
For other than pipe files, the current behaviour stands.

This issue was caught when running the script command:

	# perf script  syscall-counts ls

since it connects record and report via pipe.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/trace-event-info.c |   81 +++++++++++++++++++++++++++++++-----
 1 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 3403f81..62ab9a2 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -59,6 +59,7 @@ unsigned int page_size;
 
 static const char *output_file = "trace.info";
 static int output_fd;
+static int output_is_pipe;
 
 struct event_list {
 	struct event_list *next;
@@ -183,20 +184,59 @@ int bigendian(void)
 	return *ptr == 0x01020304;
 }
 
+static off_t get_file_size(int fd)
+{
+	off_t size = 0;
+	char buf[BUFSIZ];
+	int r;
+
+	do {
+		r = read(fd, buf, BUFSIZ);
+		if (r > 0)
+			size += r;
+	} while (r > 0);
+
+	if (lseek(fd, 0, SEEK_SET))
+		die("seek failed");
+
+	return size;
+}
+
 /* unfortunately, you can not stat debugfs or proc files for size */
 static void record_file(const char *file, size_t hdr_sz)
 {
 	unsigned long long size = 0;
-	char buf[BUFSIZ], *sizep;
-	off_t hdr_pos = lseek(output_fd, 0, SEEK_CUR);
+	char buf[BUFSIZ], *sizep = (char *) &size;
+	off_t hdr_pos = -1;
 	int r, fd;
 
 	fd = open(file, O_RDONLY);
 	if (fd < 0)
 		die("Can't read '%s'", file);
 
-	/* put in zeros for file size, then fill true size later */
-	write_or_die(&size, hdr_sz);
+	/*
+	 * If the output_fd is pipe, we need to write the size
+	 * right away, because we cannot seek on pipe later.
+	 *
+	 * In case of regular/seekable file, we can go throught
+	 * only once and use pwrite to update the header value
+	 * afterwards.
+	 *
+	 * And finally the size does not need to be written at all
+	 * if we are in the calc mode - calc_data_size.
+	 */
+	if (!calc_data_size) {
+		if (output_is_pipe) {
+			size = get_file_size(fd);
+
+			/* ugh, handle big-endian hdr_size == 4 */
+			if (bigendian())
+				sizep += sizeof(u64) - hdr_sz;
+		} else
+			hdr_pos = lseek(output_fd, 0, SEEK_CUR);
+	}
+
+	write_or_die(sizep, hdr_sz);
 
 	do {
 		r = read(fd, buf, BUFSIZ);
@@ -205,15 +245,21 @@ static void record_file(const char *file, size_t hdr_sz)
 			write_or_die(buf, r);
 		}
 	} while (r > 0);
-	close(fd);
 
-	/* ugh, handle big-endian hdr_size == 4 */
-	sizep = (char*)&size;
-	if (bigendian())
-		sizep += sizeof(u64) - hdr_sz;
+	if (!output_is_pipe && !calc_data_size) {
+
+		if (hdr_pos == -1)
+			die("BUG: hdr_pos not initialized");
+
+		/* ugh, handle big-endian hdr_size == 4 */
+		if (bigendian())
+			sizep += sizeof(u64) - hdr_sz;
+
+		if (pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
+			die("writing to %s", output_file);
+	}
 
-	if (pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
-		die("writing to %s", output_file);
+	close(fd);
 }
 
 static void read_header_files(void)
@@ -439,6 +485,18 @@ bool have_tracepoints(struct list_head *pattrs)
 	return false;
 }
 
+static int is_fd_pipe(int fd)
+{
+	struct stat s;
+	int ret;
+
+	ret = fstat(fd, &s);
+	if (ret)
+		die("stat failed");
+
+	return ((s.st_mode & S_IFMT) == S_IFIFO);
+}
+
 int read_tracing_data(int fd, struct list_head *pattrs)
 {
 	char buf[BUFSIZ];
@@ -451,6 +509,7 @@ int read_tracing_data(int fd, struct list_head *pattrs)
 		return -1;
 
 	output_fd = fd;
+	output_is_pipe = is_fd_pipe(fd);
 
 	buf[0] = 23;
 	buf[1] = 8;
-- 
1.7.4

Re: [PATCH] perf, tool, record: Fix the header generation for pipe

[Eric Dumazet]

Le lundi 22 août 2011 à 16:23 +0200, Jiri Olsa a écrit :
> The generation of the perf data file fails when using pipe
> as the output file descriptor.
> 
> When record command generates the data header, several files are put
> inside and the file size is stored ahead of the file contents itself.
> 
> The problem is that debugfs files cannot be stat-ed for size so we
> need to read the whole file, count the size and update the file size
> via seek&write (pwrite call).
> This cannot be done for pipes, since it's not allowed to seek on it.
> 
> The attached patch changes current behaviour for pipes to get the
> file size first and write correct data within the first pass.
> For other than pipe files, the current behaviour stands.
> 
> This issue was caught when running the script command:
> 
> 	# perf script  syscall-counts ls
> 
> since it connects record and report via pipe.
> 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> ---
>  tools/perf/util/trace-event-info.c |   81 +++++++++++++++++++++++++++++++-----
>  1 files changed, 70 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
> index 3403f81..62ab9a2 100644
> --- a/tools/perf/util/trace-event-info.c
> +++ b/tools/perf/util/trace-event-info.c
> @@ -59,6 +59,7 @@ unsigned int page_size;
>  
>  static const char *output_file = "trace.info";
>  static int output_fd;
> +static int output_is_pipe;
>  
>  struct event_list {
>  	struct event_list *next;
> @@ -183,20 +184,59 @@ int bigendian(void)
>  	return *ptr == 0x01020304;
>  }
>  
> +static off_t get_file_size(int fd)
> +{
> +	off_t size = 0;
> +	char buf[BUFSIZ];
> +	int r;
> +
> +	do {
> +		r = read(fd, buf, BUFSIZ);
> +		if (r > 0)
> +			size += r;
> +	} while (r > 0);
> +
> +	if (lseek(fd, 0, SEEK_SET))
> +		die("seek failed");
> +
> +	return size;
> +}

This part makes no sense :

If fd is a pipe, you'll call die("seek failed")

If it's not a pipe, you can try lseek(fd, 0, SEEK_END)+lseek(fd, 0,
SEEK_SET) or fstat(fd, &st) instead of the read() loop.

Re: [PATCH] perf, tool, record: Fix the header generation for pipe

[Jiri Olsa]

On Mon, Aug 22, 2011 at 04:38:33PM +0200, Eric Dumazet wrote:
> Le lundi 22 août 2011 à 16:23 +0200, Jiri Olsa a écrit :
> > The generation of the perf data file fails when using pipe
> > as the output file descriptor.
> > 
> > When record command generates the data header, several files are put
> > inside and the file size is stored ahead of the file contents itself.
> > 
> > The problem is that debugfs files cannot be stat-ed for size so we
> > need to read the whole file, count the size and update the file size
> > via seek&write (pwrite call).
> > This cannot be done for pipes, since it's not allowed to seek on it.
> > 
> > The attached patch changes current behaviour for pipes to get the
> > file size first and write correct data within the first pass.
> > For other than pipe files, the current behaviour stands.
> > 
> > This issue was caught when running the script command:
> > 
> > 	# perf script  syscall-counts ls
> > 
> > since it connects record and report via pipe.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> > ---
> >  tools/perf/util/trace-event-info.c |   81 +++++++++++++++++++++++++++++++-----
> >  1 files changed, 70 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
> > index 3403f81..62ab9a2 100644
> > --- a/tools/perf/util/trace-event-info.c
> > +++ b/tools/perf/util/trace-event-info.c
> > @@ -59,6 +59,7 @@ unsigned int page_size;
> >  
> >  static const char *output_file = "trace.info";
> >  static int output_fd;
> > +static int output_is_pipe;
> >  
> >  struct event_list {
> >  	struct event_list *next;
> > @@ -183,20 +184,59 @@ int bigendian(void)
> >  	return *ptr == 0x01020304;
> >  }
> >  
> > +static off_t get_file_size(int fd)
> > +{
> > +	off_t size = 0;
> > +	char buf[BUFSIZ];
> > +	int r;
> > +
> > +	do {
> > +		r = read(fd, buf, BUFSIZ);
> > +		if (r > 0)
> > +			size += r;
> > +	} while (r > 0);
> > +
> > +	if (lseek(fd, 0, SEEK_SET))
> > +		die("seek failed");
> > +
> > +	return size;
> > +}
> 
> This part makes no sense :
> 
> If fd is a pipe, you'll call die("seek failed")
> 
> If it's not a pipe, you can try lseek(fd, 0, SEEK_END)+lseek(fd, 0,
> SEEK_SET) or fstat(fd, &st) instead of the read() loop.

- fd is always file.. the descriptor, which might be pipe, is in output_fd variable
  but, maybe the die call is not necessary.. this should not fail

- the record_file function is called only on debugfs or procfs files:
	events/header_page
	events/header_event
	events/**/format
	printk_formats
	/proc/kallsyms

so I think I need to read the whole file as in current code.

thanks,
jirka

Re: [PATCH] perf, tool, record: Fix the header generation for pipe

[Eric Dumazet]

Le lundi 22 août 2011 à 16:52 +0200, Jiri Olsa a écrit :

> - fd is always file.. the descriptor, which might be pipe, is in output_fd variable
>   but, maybe the die call is not necessary.. this should not fail
> 
> - the record_file function is called only on debugfs or procfs files:
> 	events/header_page
> 	events/header_event
> 	events/**/format
> 	printk_formats
> 	/proc/kallsyms
> 
> so I think I need to read the whole file as in current code.
> 

You read the file twice. Is it the right fix ?

Once to compute the length to be able to write the output header, once
to process the content.

Are you sure length cannot change between the two phases ?

/proc/kallsyms can definitely change when a module is loaded.

Usually, when input "file" is not seekable or procfs based (fstat()
returns st_size=0), we must load/cache it in memory (or using a
temporary file) to get a consistent view of it.

This is what is done by following commands :

cat /proc/kallsyms | less
less /proc/kallsyms

Re: [PATCH] perf, tool, record: Fix the header generation for pipe

[Jiri Olsa]

On Mon, Aug 22, 2011 at 05:51:06PM +0200, Eric Dumazet wrote:
> Le lundi 22 août 2011 à 16:52 +0200, Jiri Olsa a écrit :
> 
> > - fd is always file.. the descriptor, which might be pipe, is in output_fd variable
> >   but, maybe the die call is not necessary.. this should not fail
> > 
> > - the record_file function is called only on debugfs or procfs files:
> > 	events/header_page
> > 	events/header_event
> > 	events/**/format
> > 	printk_formats
> > 	/proc/kallsyms
> > 
> > so I think I need to read the whole file as in current code.
> > 
> 
> You read the file twice. Is it the right fix ?
> 
> Once to compute the length to be able to write the output header, once
> to process the content.
> 
> Are you sure length cannot change between the two phases ?
> 
> /proc/kallsyms can definitely change when a module is loaded.

Right, better to store it to a temp file as you suggest,
so we have consistent view.. I'll prepare new patch.

thanks,
jirka


> 
> Usually, when input "file" is not seekable or procfs based (fstat()
> returns st_size=0), we must load/cache it in memory (or using a
> temporary file) to get a consistent view of it.
> 
> This is what is done by following commands :
> 
> cat /proc/kallsyms | less
> less /proc/kallsyms
> 
> 
> 

Re: [PATCH] perf, tool, record: Fix the header generation for pipe

[Arnaldo Carvalho de Melo]

Em Mon, Aug 22, 2011 at 06:07:13PM +0200, Jiri Olsa escreveu:
> On Mon, Aug 22, 2011 at 05:51:06PM +0200, Eric Dumazet wrote:
> > Le lundi 22 août 2011 à 16:52 +0200, Jiri Olsa a écrit :
> > > - fd is always file.. the descriptor, which might be pipe, is in output_fd variable
> > >   but, maybe the die call is not necessary.. this should not fail

> > > - the record_file function is called only on debugfs or procfs files:
> > > 	events/header_page
> > > 	events/header_event
> > > 	events/**/format
> > > 	printk_formats
> > > 	/proc/kallsyms

> > > so I think I need to read the whole file as in current code.

> > You read the file twice. Is it the right fix ?

> > Once to compute the length to be able to write the output header, once
> > to process the content.

> > Are you sure length cannot change between the two phases ?

> > /proc/kallsyms can definitely change when a module is loaded.

> Right, better to store it to a temp file as you suggest, so we have
> consistent view.. I'll prepare new patch.

Hi Jiri, any news here?

I just stumbled in this bug while testing Neil's net_dropmonitor script
:-\

- Arnaldo

Re: [PATCH] perf, tool, record: Fix the header generation for pipe

[Jiri Olsa]

On Mon, Aug 29, 2011 at 10:20:37AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Aug 22, 2011 at 06:07:13PM +0200, Jiri Olsa escreveu:
> > On Mon, Aug 22, 2011 at 05:51:06PM +0200, Eric Dumazet wrote:
> > > Le lundi 22 août 2011 à 16:52 +0200, Jiri Olsa a écrit :
> > > > - fd is always file.. the descriptor, which might be pipe, is in output_fd variable
> > > >   but, maybe the die call is not necessary.. this should not fail
> 
> > > > - the record_file function is called only on debugfs or procfs files:
> > > > 	events/header_page
> > > > 	events/header_event
> > > > 	events/**/format
> > > > 	printk_formats
> > > > 	/proc/kallsyms
> 
> > > > so I think I need to read the whole file as in current code.
> 
> > > You read the file twice. Is it the right fix ?
> 
> > > Once to compute the length to be able to write the output header, once
> > > to process the content.
> 
> > > Are you sure length cannot change between the two phases ?
> 
> > > /proc/kallsyms can definitely change when a module is loaded.
> 
> > Right, better to store it to a temp file as you suggest, so we have
> > consistent view.. I'll prepare new patch.
> 
> Hi Jiri, any news here?
> 
> I just stumbled in this bug while testing Neil's net_dropmonitor script
> :-\

well, I was thinking about taking files snapshot, so we dont read them
twice and omit the reading for size.. but it'll be probably bigger
change, I'll try to send out something by the end of the week

if you need workaround, the initial patch should work

jirka

Re: [PATCH] perf, tool, record: Fix the header generation for pipe

[Arnaldo Carvalho de Melo]

Em Mon, Aug 29, 2011 at 03:41:47PM +0200, Jiri Olsa escreveu:
> On Mon, Aug 29, 2011 at 10:20:37AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Aug 22, 2011 at 06:07:13PM +0200, Jiri Olsa escreveu:
> > > On Mon, Aug 22, 2011 at 05:51:06PM +0200, Eric Dumazet wrote:
> > > > Are you sure length cannot change between the two phases ?
> > 
> > > > /proc/kallsyms can definitely change when a module is loaded.
> > 
> > > Right, better to store it to a temp file as you suggest, so we have
> > > consistent view.. I'll prepare new patch.
> > 
> > Hi Jiri, any news here?
> > 
> > I just stumbled in this bug while testing Neil's net_dropmonitor script
> > :-\
> 
> well, I was thinking about taking files snapshot, so we dont read them
> twice and omit the reading for size.. but it'll be probably bigger
> change, I'll try to send out something by the end of the week

Yeah, at least one thing there I plan to remove, i.e. no need to copy
kallsyms when we have build ids, just stash the build id and use it
later to get the right symtab to resolve symbols.
 
> if you need workaround, the initial patch should work

Ok, thanks.

- Arnaldo

[PATCH] perf tools: Fix tracing info recording

[Jiri Olsa]

On Mon, Aug 29, 2011 at 11:25:47AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > > 
> > > Hi Jiri, any news here?
> > > 
> > > I just stumbled in this bug while testing Neil's net_dropmonitor script
> > > :-\
> > 
> > well, I was thinking about taking files snapshot, so we dont read them
> > twice and omit the reading for size.. but it'll be probably bigger
> > change, I'll try to send out something by the end of the week

sry, I did not make it and then I was out for vacation..  sending now ;)

jirka


---
The tracing information is part of the perf data file. It contains
several files from within the tracing debugfs and procs directories.

Beside some static header files, for each tracing event the format
file is added. The /proc/kallsyms file is also added.

The tracing data are stored with preceeding size. This is causing some
dificulties for pipe output, since there's no way to tell debugfs/proc
file size before reading it. So, for pipe output, all the debugfs files
were read twice. Once to get the overall size and once to store the
content itself. This can cause problem in case any of these file
changed, within the storage time.

Fixing this behaviour by using temp file in case of pipe output. The
debugfs/proc files are being read only once, ensuring the integrity of
the tracing data.

Also changing the way the event files are searched by quering specified
event files directly, instead of walking the events directory.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/header.c           |   39 ++++-
 tools/perf/util/parse-events.h     |    1 +
 tools/perf/util/trace-event-info.c |  333 +++++++++++++++++++-----------------
 tools/perf/util/trace-event.h      |    7 +
 4 files changed, 215 insertions(+), 165 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index b6c1ad1..6a9fd5b 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -387,13 +387,21 @@ static int perf_header__adds_write(struct perf_header *header,
 
 	if (perf_header__has_feat(header, HEADER_TRACE_INFO)) {
 		struct perf_file_section *trace_sec;
+		struct tracing_data *tdata;
 
 		trace_sec = &feat_sec[idx++];
-
-		/* Write trace info */
 		trace_sec->offset = lseek(fd, 0, SEEK_CUR);
-		read_tracing_data(fd, &evlist->entries);
-		trace_sec->size = lseek(fd, 0, SEEK_CUR) - trace_sec->offset;
+
+		/*
+		 * We work over the real file, we can write data
+		 * directly, not temp file is needed.
+		 */
+		tdata = tracing_data_get(&evlist->entries, fd, false);
+		if (!tdata)
+			goto out_free;
+
+		trace_sec->size = tracing_data_size(tdata);
+		tracing_data_put(tdata);
 	}
 
 	if (perf_header__has_feat(header, HEADER_BUILD_ID)) {
@@ -1100,15 +1108,25 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
 				   struct perf_session *session __unused)
 {
 	union perf_event ev;
+	struct tracing_data *tdata;
 	ssize_t size = 0, aligned_size = 0, padding;
 	int err __used = 0;
 
+	/*
+	 * The fd descriptor is pipe, se we need to:
+	 * - write the tracing data to the temp file
+	 * - get/write the data size to pipe
+	 * - write the tracing data from the temp file
+	 *   to the pipe
+	 */
+	tdata = tracing_data_get(&evlist->entries, fd, true);
+	if (!tdata)
+		return -1;
+
 	memset(&ev, 0, sizeof(ev));
 
 	ev.tracing_data.header.type = PERF_RECORD_HEADER_TRACING_DATA;
-	size = read_tracing_data_size(fd, &evlist->entries);
-	if (size <= 0)
-		return size;
+	size = tracing_data_size(tdata);
 	aligned_size = ALIGN(size, sizeof(u64));
 	padding = aligned_size - size;
 	ev.tracing_data.header.size = sizeof(ev.tracing_data);
@@ -1116,7 +1134,12 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
 
 	process(&ev, NULL, session);
 
-	err = read_tracing_data(fd, &evlist->entries);
+	/*
+	 * The put function will copy all the tracing data
+	 * stored in temp file to the pipe.
+	 */
+	tracing_data_put(tdata);
+
 	write_padded(fd, NULL, 0, padding);
 
 	return aligned_size;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 2f8e375..1ea02ad 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -15,6 +15,7 @@ struct option;
 struct tracepoint_path {
 	char *system;
 	char *name;
+	bool done;
 	struct tracepoint_path *next;
 };
 
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 3403f81..05b2e30 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -196,7 +196,8 @@ static void record_file(const char *file, size_t hdr_sz)
 		die("Can't read '%s'", file);
 
 	/* put in zeros for file size, then fill true size later */
-	write_or_die(&size, hdr_sz);
+	if (hdr_sz)
+		write_or_die(&size, hdr_sz);
 
 	do {
 		r = read(fd, buf, BUFSIZ);
@@ -212,7 +213,7 @@ static void record_file(const char *file, size_t hdr_sz)
 	if (bigendian())
 		sizep += sizeof(u64) - hdr_sz;
 
-	if (pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
+	if (hdr_sz && pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
 		die("writing to %s", output_file);
 }
 
@@ -238,138 +239,6 @@ static void read_header_files(void)
 	put_tracing_file(path);
 }
 
-static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
-{
-	while (tps) {
-		if (!strcmp(sys, tps->name))
-			return true;
-		tps = tps->next;
-	}
-
-	return false;
-}
-
-static void copy_event_system(const char *sys, struct tracepoint_path *tps)
-{
-	struct dirent *dent;
-	struct stat st;
-	char *format;
-	DIR *dir;
-	int count = 0;
-	int ret;
-
-	dir = opendir(sys);
-	if (!dir)
-		die("can't read directory '%s'", sys);
-
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    !name_in_tp_list(dent->d_name, tps))
-			continue;
-		format = malloc_or_die(strlen(sys) + strlen(dent->d_name) + 10);
-		sprintf(format, "%s/%s/format", sys, dent->d_name);
-		ret = stat(format, &st);
-		free(format);
-		if (ret < 0)
-			continue;
-		count++;
-	}
-
-	write_or_die(&count, 4);
-
-	rewinddir(dir);
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    !name_in_tp_list(dent->d_name, tps))
-			continue;
-		format = malloc_or_die(strlen(sys) + strlen(dent->d_name) + 10);
-		sprintf(format, "%s/%s/format", sys, dent->d_name);
-		ret = stat(format, &st);
-
-		if (ret >= 0)
-			record_file(format, 8);
-
-		free(format);
-	}
-	closedir(dir);
-}
-
-static void read_ftrace_files(struct tracepoint_path *tps)
-{
-	char *path;
-
-	path = get_tracing_file("events/ftrace");
-
-	copy_event_system(path, tps);
-
-	put_tracing_file(path);
-}
-
-static bool system_in_tp_list(char *sys, struct tracepoint_path *tps)
-{
-	while (tps) {
-		if (!strcmp(sys, tps->system))
-			return true;
-		tps = tps->next;
-	}
-
-	return false;
-}
-
-static void read_event_files(struct tracepoint_path *tps)
-{
-	struct dirent *dent;
-	struct stat st;
-	char *path;
-	char *sys;
-	DIR *dir;
-	int count = 0;
-	int ret;
-
-	path = get_tracing_file("events");
-
-	dir = opendir(path);
-	if (!dir)
-		die("can't read directory '%s'", path);
-
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    strcmp(dent->d_name, "ftrace") == 0 ||
-		    !system_in_tp_list(dent->d_name, tps))
-			continue;
-		count++;
-	}
-
-	write_or_die(&count, 4);
-
-	rewinddir(dir);
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    strcmp(dent->d_name, "ftrace") == 0 ||
-		    !system_in_tp_list(dent->d_name, tps))
-			continue;
-		sys = malloc_or_die(strlen(path) + strlen(dent->d_name) + 2);
-		sprintf(sys, "%s/%s", path, dent->d_name);
-		ret = stat(sys, &st);
-		if (ret >= 0) {
-			write_or_die(dent->d_name, strlen(dent->d_name) + 1);
-			copy_event_system(sys, tps);
-		}
-		free(sys);
-	}
-
-	closedir(dir);
-	put_tracing_file(path);
-}
-
 static void read_proc_kallsyms(void)
 {
 	unsigned int size;
@@ -428,6 +297,19 @@ get_tracepoints_path(struct list_head *pattrs)
 	return nr_tracepoints > 0 ? path.next : NULL;
 }
 
+static void
+put_tracepoints_path(struct tracepoint_path *tps)
+{
+	while (tps) {
+		struct tracepoint_path *t = tps;
+
+		tps = tps->next;
+		free(t->name);
+		free(t->system);
+		free(t);
+	}
+}
+
 bool have_tracepoints(struct list_head *pattrs)
 {
 	struct perf_evsel *pos;
@@ -439,19 +321,114 @@ bool have_tracepoints(struct list_head *pattrs)
 	return false;
 }
 
-int read_tracing_data(int fd, struct list_head *pattrs)
+#define FILENAME_SIZE 50
+
+struct tracing_data {
+	ssize_t size;
+	bool temp;
+	char temp_file[FILENAME_SIZE];
+};
+
+ssize_t tracing_data_size(struct tracing_data *td)
 {
-	char buf[BUFSIZ];
-	struct tracepoint_path *tps = get_tracepoints_path(pattrs);
+	return td->size;
+}
 
-	/*
-	 * What? No tracepoints? No sense writing anything here, bail out.
-	 */
-	if (tps == NULL)
-		return -1;
+static char *get_format_file(char *sys, char *name)
+{
+	char *file, *path;
 
-	output_fd = fd;
+	path = get_tracing_file("events");
+	if (!path)
+		return NULL;
+
+	file = malloc_or_die(strlen(path) + strlen(sys) +
+			     strlen(name) + strlen("format") + 10);
+
+	sprintf(file, "%s/%s/%s/format", path, sys, name);
+	return file;
+}
+
+static void put_format_file(char *file)
+{
+	free(file);
+}
+
+/*
+ * Walk tracepoint event objects and store them.
+ * Only those matching the sys parameter are stored
+ * and marked as done.
+ */
+static void read_event_files_system(struct tracepoint_path *tps,
+				    const char *sys)
+{
+	off_t count_pos;
+	u32 count = 0;
+
+	/* specified events count under single system */
+	count_pos = lseek(output_fd, 0, SEEK_CUR);
+	write_or_die(&count, 4);
+
+	while (tps) {
+		if ((!tps->done) &&
+		    (!strcmp(sys, tps->system))) {
+			char *file;
+
+			file = get_format_file(tps->system, tps->name);
+			if (file) {
+				record_file(file, 8);
+				count++;
+			}
+
+			put_format_file(file);
+			tps->done = 1;
+		}
+
+		tps = tps->next;
+	}
+
+	if (pwrite(output_fd, &count, 4, count_pos) < 0)
+		die("pwrite");
+}
+
+/*
+ * We have all needed tracepoint event files stored in
+ * the tracepoint_path objects.
+ *
+ * - first we store ftrace system events
+ * - then we walk all '!done' event objects
+ *   and process them
+ */
+static void read_event_files(struct tracepoint_path *tps)
+{
+	off_t count_pos;
+	u32 count = 0;
+
+	read_event_files_system(tps, "ftrace");
+
+	/* systems count */
+	count_pos = lseek(output_fd, 0, SEEK_CUR);
+	write_or_die(&count, 4);
+
+	while (tps) {
+		if (!tps->done) {
+			write_or_die(tps->system, strlen(tps->system) + 1);
+			read_event_files_system(tps, tps->system);
+			count++;
+		}
+
+		tps = tps->next;
+	}
 
+	if (pwrite(output_fd, &count, 4, count_pos) < 0)
+		die("write");
+}
+
+static void tracing_data_header(void)
+{
+	char buf[50];
+
+	/* just guessing this is someone's birthday.. ;) */
 	buf[0] = 23;
 	buf[1] = 8;
 	buf[2] = 68;
@@ -476,28 +453,70 @@ int read_tracing_data(int fd, struct list_head *pattrs)
 	/* save page_size */
 	page_size = sysconf(_SC_PAGESIZE);
 	write_or_die(&page_size, 4);
+}
+
+struct tracing_data *tracing_data_get(struct list_head *pattrs,
+				      int fd, bool temp)
+{
+	struct tracepoint_path *tps;
+	struct tracing_data *tdata;
 
+	output_fd = fd;
+
+	tps = get_tracepoints_path(pattrs);
+	if (!tps)
+		return NULL;
+
+	tdata = malloc_or_die(sizeof(*tdata));
+	tdata->temp = temp;
+	tdata->size = 0;
+
+	if (temp) {
+		int temp_fd;
+
+		snprintf(tdata->temp_file, FILENAME_SIZE, "/tmp/perf-XXXXXX");
+		if (!mkstemp(tdata->temp_file))
+			die("Can't make temp file");
+
+		temp_fd = open(tdata->temp_file, O_RDWR);
+		if (temp_fd < 0)
+			die("Can't read '%s'", tdata->temp_file);
+
+		/*
+		 * Set the temp file the default output, so all the
+		 * tracing data are stored into it.
+		 */
+		output_fd = temp_fd;
+	} else
+		tdata->size = -lseek(output_fd, 0, SEEK_CUR);
+
+	tracing_data_header();
 	read_header_files();
-	read_ftrace_files(tps);
 	read_event_files(tps);
 	read_proc_kallsyms();
 	read_ftrace_printk();
 
-	return 0;
-}
+	tdata->size += lseek(output_fd, 0, SEEK_CUR);
 
-ssize_t read_tracing_data_size(int fd, struct list_head *pattrs)
-{
-	ssize_t size;
-	int err = 0;
+	/*
+	 * All tracing data are stored by now, we can restore
+	 * the default output file in case we used temp file.
+	 */
+	if (temp) {
+		close(output_fd);
+		output_fd = fd;
+	}
 
-	calc_data_size = 1;
-	err = read_tracing_data(fd, pattrs);
-	size = calc_data_size - 1;
-	calc_data_size = 0;
+	put_tracepoints_path(tps);
+	return tdata;
+}
 
-	if (err < 0)
-		return err;
+void tracing_data_put(struct tracing_data *tdata)
+{
+	if (tdata->temp) {
+		record_file(tdata->temp_file, 0);
+		unlink(tdata->temp_file);
+	}
 
-	return size;
+	free(tdata);
 }
diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
index f674dda..2a23893 100644
--- a/tools/perf/util/trace-event.h
+++ b/tools/perf/util/trace-event.h
@@ -265,6 +265,13 @@ unsigned long long eval_flag(const char *flag);
 int read_tracing_data(int fd, struct list_head *pattrs);
 ssize_t read_tracing_data_size(int fd, struct list_head *pattrs);
 
+struct tracing_data;
+struct tracing_data *tracing_data_get(struct list_head *pattrs,
+				      int fd, bool temp);
+void tracing_data_put(struct tracing_data *tdata);
+ssize_t tracing_data_size(struct tracing_data *td);
+
+
 /* taken from kernel/trace/trace.h */
 enum trace_flag_type {
 	TRACE_FLAG_IRQS_OFF		= 0x01,
-- 
1.7.4

Re: [PATCH] perf tools: Fix tracing info recording

[Neil Horman]

On Wed, Sep 14, 2011 at 03:58:40PM +0200, Jiri Olsa wrote:
> On Mon, Aug 29, 2011 at 11:25:47AM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > > > 
> > > > Hi Jiri, any news here?
> > > > 
> > > > I just stumbled in this bug while testing Neil's net_dropmonitor script
> > > > :-\
> > > 
> > > well, I was thinking about taking files snapshot, so we dont read them
> > > twice and omit the reading for size.. but it'll be probably bigger
> > > change, I'll try to send out something by the end of the week
> 
> sry, I did not make it and then I was out for vacation..  sending now ;)
> 
> jirka
> 
Thanks Jiri!

Re: [PATCH] perf tools: Fix tracing info recording

[Frederic Weisbecker]

On Wed, Sep 14, 2011 at 03:58:40PM +0200, Jiri Olsa wrote:
> The tracing information is part of the perf data file. It contains
> several files from within the tracing debugfs and procs directories.
> 
> Beside some static header files, for each tracing event the format
> file is added. The /proc/kallsyms file is also added.
> 
> The tracing data are stored with preceeding size. This is causing some
> dificulties for pipe output, since there's no way to tell debugfs/proc
> file size before reading it. So, for pipe output, all the debugfs files
> were read twice. Once to get the overall size and once to store the
> content itself. This can cause problem in case any of these file
> changed, within the storage time.
> 
> Fixing this behaviour by using temp file in case of pipe output. The
> debugfs/proc files are being read only once, ensuring the integrity of
> the tracing data.
> 
> Also changing the way the event files are searched by quering specified
> event files directly, instead of walking the events directory.
> 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>

Looks good overall, but I have some comments about details:

> ---
>  tools/perf/util/header.c           |   39 ++++-
>  tools/perf/util/parse-events.h     |    1 +
>  tools/perf/util/trace-event-info.c |  333 +++++++++++++++++++-----------------
>  tools/perf/util/trace-event.h      |    7 +
>  4 files changed, 215 insertions(+), 165 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index b6c1ad1..6a9fd5b 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -387,13 +387,21 @@ static int perf_header__adds_write(struct perf_header *header,
>  
>  	if (perf_header__has_feat(header, HEADER_TRACE_INFO)) {
>  		struct perf_file_section *trace_sec;
> +		struct tracing_data *tdata;
>  
>  		trace_sec = &feat_sec[idx++];
> -
> -		/* Write trace info */
>  		trace_sec->offset = lseek(fd, 0, SEEK_CUR);
> -		read_tracing_data(fd, &evlist->entries);
> -		trace_sec->size = lseek(fd, 0, SEEK_CUR) - trace_sec->offset;
> +
> +		/*
> +		 * We work over the real file, we can write data
> +		 * directly, not temp file is needed.

s/not/no

May be also briefly explain why we can write directly here. The fact
we have reserved space to write the size in the headers already.

> +		 */
> +		tdata = tracing_data_get(&evlist->entries, fd, false);
> +		if (!tdata)
> +			goto out_free;
> +
> +		trace_sec->size = tracing_data_size(tdata);
> +		tracing_data_put(tdata);
>  	}
>  
>  	if (perf_header__has_feat(header, HEADER_BUILD_ID)) {
> @@ -1100,15 +1108,25 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
>  				   struct perf_session *session __unused)
>  {
>  	union perf_event ev;
> +	struct tracing_data *tdata;
>  	ssize_t size = 0, aligned_size = 0, padding;
>  	int err __used = 0;
>  
> +	/*
> +	 * The fd descriptor is pipe, se we need to:
> +	 * - write the tracing data to the temp file
> +	 * - get/write the data size to pipe
> +	 * - write the tracing data from the temp file
> +	 *   to the pipe
> +	 */

That also needs a brief explanation on why we need to do that.


> +	tdata = tracing_data_get(&evlist->entries, fd, true);
> +	if (!tdata)
> +		return -1;
> +
>  	memset(&ev, 0, sizeof(ev));
>  
>  	ev.tracing_data.header.type = PERF_RECORD_HEADER_TRACING_DATA;
> -	size = read_tracing_data_size(fd, &evlist->entries);
> -	if (size <= 0)
> -		return size;
> +	size = tracing_data_size(tdata);
>  	aligned_size = ALIGN(size, sizeof(u64));
>  	padding = aligned_size - size;
>  	ev.tracing_data.header.size = sizeof(ev.tracing_data);
<snip>
> -int read_tracing_data(int fd, struct list_head *pattrs)
> +#define FILENAME_SIZE 50
> +
> +struct tracing_data {
> +	ssize_t size;
> +	bool temp;
> +	char temp_file[FILENAME_SIZE];
> +};
> +
> +ssize_t tracing_data_size(struct tracing_data *td)
>  {
> -	char buf[BUFSIZ];
> -	struct tracepoint_path *tps = get_tracepoints_path(pattrs);
> +	return td->size;
> +}

May be inline it. Or rather directly access td->size from calling code.

>  
> -	/*
> -	 * What? No tracepoints? No sense writing anything here, bail out.
> -	 */
> -	if (tps == NULL)
> -		return -1;
> +static char *get_format_file(char *sys, char *name)
> +{
> +	char *file, *path;
>  
> -	output_fd = fd;
> +	path = get_tracing_file("events");
> +	if (!path)
> +		return NULL;
> +
> +	file = malloc_or_die(strlen(path) + strlen(sys) +
> +			     strlen(name) + strlen("format") + 10);

Why "10" ?

> +
> +	sprintf(file, "%s/%s/%s/format", path, sys, name);
> +	return file;
> +}
> +
> +static void put_format_file(char *file)
> +{
> +	free(file);
> +}
> +
> +/*
> + * Walk tracepoint event objects and store them.
> + * Only those matching the sys parameter are stored
> + * and marked as done.
> + */
> +static void read_event_files_system(struct tracepoint_path *tps,
> +				    const char *sys)
> +{
> +	off_t count_pos;
> +	u32 count = 0;
> +
> +	/* specified events count under single system */
> +	count_pos = lseek(output_fd, 0, SEEK_CUR);
> +	write_or_die(&count, 4);
> +
> +	while (tps) {
> +		if ((!tps->done) &&
> +		    (!strcmp(sys, tps->system))) {
> +			char *file;
> +
> +			file = get_format_file(tps->system, tps->name);
> +			if (file) {
> +				record_file(file, 8);
> +				count++;
> +			}
> +
> +			put_format_file(file);
> +			tps->done = 1;
> +		}
> +
> +		tps = tps->next;
> +	}
> +
> +	if (pwrite(output_fd, &count, 4, count_pos) < 0)
> +		die("pwrite");
> +}
> +
> +/*
> + * We have all needed tracepoint event files stored in
> + * the tracepoint_path objects.
> + *
> + * - first we store ftrace system events
> + * - then we walk all '!done' event objects
> + *   and process them
> + */
> +static void read_event_files(struct tracepoint_path *tps)
> +{
> +	off_t count_pos;
> +	u32 count = 0;
> +
> +	read_event_files_system(tps, "ftrace");
> +
> +	/* systems count */
> +	count_pos = lseek(output_fd, 0, SEEK_CUR);
> +	write_or_die(&count, 4);
> +
> +	while (tps) {
> +		if (!tps->done) {
> +			write_or_die(tps->system, strlen(tps->system) + 1);
> +			read_event_files_system(tps, tps->system);
> +			count++;
> +		}
> +
> +		tps = tps->next;
> +	}
>  
> +	if (pwrite(output_fd, &count, 4, count_pos) < 0)
> +		die("write");
> +}

There seem to be a significant reorganization of the code in that file
that seem to overlap the initial scope of this patch. Should
it go to a seperate single purpose patch?

Thanks.

Re: [PATCH] perf tools: Fix tracing info recording

[Jiri Olsa]

On Wed, Sep 21, 2011 at 05:30:24PM +0200, Frederic Weisbecker wrote:
> On Wed, Sep 14, 2011 at 03:58:40PM +0200, Jiri Olsa wrote:
> > The tracing information is part of the perf data file. It contains
> > several files from within the tracing debugfs and procs directories.
> > 
> > Beside some static header files, for each tracing event the format
> > file is added. The /proc/kallsyms file is also added.
> > 
> > The tracing data are stored with preceeding size. This is causing some
> > dificulties for pipe output, since there's no way to tell debugfs/proc
> > file size before reading it. So, for pipe output, all the debugfs files
> > were read twice. Once to get the overall size and once to store the
> > content itself. This can cause problem in case any of these file
> > changed, within the storage time.
> > 
> > Fixing this behaviour by using temp file in case of pipe output. The
> > debugfs/proc files are being read only once, ensuring the integrity of
> > the tracing data.
> > 
> > Also changing the way the event files are searched by quering specified
> > event files directly, instead of walking the events directory.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> 
> Looks good overall, but I have some comments about details:

hi,
thanks for comments,
I'll make the changes and send out new version soon

jirka

[PATCHv2 1/2] perf tools: Fix tracing info recording

[Jiri Olsa]

hi,
I modified the patch based on your comments and separated
it into 2 parts:

- 1/2 perf tools: Collect tracing event data files directly
- 2/2 perf tools: Fix tracing info recording

thanks for comments,
jirka
---
 tools/perf/util/header.c           |   50 +++++-
 tools/perf/util/parse-events.h     |    1 +
 tools/perf/util/trace-event-info.c |  309 +++++++++++++++++++-----------------
 tools/perf/util/trace-event.h      |   11 +-
 4 files changed, 212 insertions(+), 159 deletions(-)

[PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Jiri Olsa]

Changing the way the event files are searched by quering specified
event files directly, instead of walking the events directory.

Hopefully this way is more straightforward and faster.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/parse-events.h     |    1 +
 tools/perf/util/trace-event-info.c |  231 +++++++++++++++--------------------
 2 files changed, 100 insertions(+), 132 deletions(-)

diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 2f8e375..1ea02ad 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -15,6 +15,7 @@ struct option;
 struct tracepoint_path {
 	char *system;
 	char *name;
+	bool done;
 	struct tracepoint_path *next;
 };
 
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 3403f81..cf8f89e 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -238,136 +238,98 @@ static void read_header_files(void)
 	put_tracing_file(path);
 }
 
-static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
+static char *get_format_file(char *sys, char *name)
 {
-	while (tps) {
-		if (!strcmp(sys, tps->name))
-			return true;
-		tps = tps->next;
-	}
-
-	return false;
-}
-
-static void copy_event_system(const char *sys, struct tracepoint_path *tps)
-{
-	struct dirent *dent;
-	struct stat st;
-	char *format;
-	DIR *dir;
-	int count = 0;
-	int ret;
-
-	dir = opendir(sys);
-	if (!dir)
-		die("can't read directory '%s'", sys);
-
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    !name_in_tp_list(dent->d_name, tps))
-			continue;
-		format = malloc_or_die(strlen(sys) + strlen(dent->d_name) + 10);
-		sprintf(format, "%s/%s/format", sys, dent->d_name);
-		ret = stat(format, &st);
-		free(format);
-		if (ret < 0)
-			continue;
-		count++;
-	}
-
-	write_or_die(&count, 4);
+	char *file, *path;
 
-	rewinddir(dir);
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    !name_in_tp_list(dent->d_name, tps))
-			continue;
-		format = malloc_or_die(strlen(sys) + strlen(dent->d_name) + 10);
-		sprintf(format, "%s/%s/format", sys, dent->d_name);
-		ret = stat(format, &st);
+	path = get_tracing_file("events");
+	if (!path)
+		return NULL;
 
-		if (ret >= 0)
-			record_file(format, 8);
+	/*
+	 * The '+ 5' is for '/' we add to the patch, plus NULL byte,
+	 * and one safety byte ;)
+	 */
+	file = malloc_or_die(strlen(path) + strlen(sys) +
+			     strlen(name) + strlen("format") + 5);
 
-		free(format);
-	}
-	closedir(dir);
+	sprintf(file, "%s/%s/%s/format", path, sys, name);
+	return file;
 }
 
-static void read_ftrace_files(struct tracepoint_path *tps)
+static void put_format_file(char *file)
 {
-	char *path;
-
-	path = get_tracing_file("events/ftrace");
-
-	copy_event_system(path, tps);
-
-	put_tracing_file(path);
+	free(file);
 }
 
-static bool system_in_tp_list(char *sys, struct tracepoint_path *tps)
+/*
+ * Walk tracepoint event objects and store them.
+ * Only those matching the sys parameter are stored
+ * and marked as done.
+ */
+static void read_event_files_system(struct tracepoint_path *tps,
+				    const char *sys)
 {
+	off_t count_pos;
+	u32 count = 0;
+
+	/* Place for number of events under single system. */
+	count_pos = lseek(output_fd, 0, SEEK_CUR);
+	write_or_die(&count, 4);
+
 	while (tps) {
-		if (!strcmp(sys, tps->system))
-			return true;
+		if ((!tps->done) &&
+		    (!strcmp(sys, tps->system))) {
+			char *file;
+
+			file = get_format_file(tps->system, tps->name);
+			if (file) {
+				record_file(file, 8);
+				count++;
+			}
+
+			put_format_file(file);
+			tps->done = 1;
+		}
+
 		tps = tps->next;
 	}
 
-	return false;
+	if (pwrite(output_fd, &count, 4, count_pos) < 0)
+		die("pwrite");
 }
 
+/*
+ * We have all needed tracepoint event files stored in
+ * the tracepoint_path objects.
+ *
+ * - first we store ftrace system events
+ * - then we walk all '!done' event objects
+ *   and process them
+ */
 static void read_event_files(struct tracepoint_path *tps)
 {
-	struct dirent *dent;
-	struct stat st;
-	char *path;
-	char *sys;
-	DIR *dir;
-	int count = 0;
-	int ret;
-
-	path = get_tracing_file("events");
+	off_t count_pos;
+	u32 count = 0;
 
-	dir = opendir(path);
-	if (!dir)
-		die("can't read directory '%s'", path);
-
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    strcmp(dent->d_name, "ftrace") == 0 ||
-		    !system_in_tp_list(dent->d_name, tps))
-			continue;
-		count++;
-	}
+	read_event_files_system(tps, "ftrace");
 
+	/* systems count */
+	count_pos = lseek(output_fd, 0, SEEK_CUR);
 	write_or_die(&count, 4);
 
-	rewinddir(dir);
-	while ((dent = readdir(dir))) {
-		if (dent->d_type != DT_DIR ||
-		    strcmp(dent->d_name, ".") == 0 ||
-		    strcmp(dent->d_name, "..") == 0 ||
-		    strcmp(dent->d_name, "ftrace") == 0 ||
-		    !system_in_tp_list(dent->d_name, tps))
-			continue;
-		sys = malloc_or_die(strlen(path) + strlen(dent->d_name) + 2);
-		sprintf(sys, "%s/%s", path, dent->d_name);
-		ret = stat(sys, &st);
-		if (ret >= 0) {
-			write_or_die(dent->d_name, strlen(dent->d_name) + 1);
-			copy_event_system(sys, tps);
+	while (tps) {
+		if (!tps->done) {
+			write_or_die(tps->system, strlen(tps->system) + 1);
+			read_event_files_system(tps, tps->system);
+			count++;
 		}
-		free(sys);
+
+		tps = tps->next;
 	}
 
-	closedir(dir);
-	put_tracing_file(path);
+	if (pwrite(output_fd, &count, 4, count_pos) < 0)
+		die("write");
 }
 
 static void read_proc_kallsyms(void)
@@ -387,6 +349,37 @@ static void read_proc_kallsyms(void)
 	record_file(path, 4);
 }
 
+static void tracing_data_header(void)
+{
+	char buf[50];
+
+	/* just guessing this is someone's birthday.. ;) */
+	buf[0] = 23;
+	buf[1] = 8;
+	buf[2] = 68;
+	memcpy(buf + 3, "tracing", 7);
+
+	write_or_die(buf, 10);
+
+	write_or_die(VERSION, strlen(VERSION) + 1);
+
+	/* save endian */
+	if (bigendian())
+		buf[0] = 1;
+	else
+		buf[0] = 0;
+
+	write_or_die(buf, 1);
+
+	/* save size of long */
+	buf[0] = sizeof(long);
+	write_or_die(buf, 1);
+
+	/* save page_size */
+	page_size = sysconf(_SC_PAGESIZE);
+	write_or_die(&page_size, 4);
+}
+
 static void read_ftrace_printk(void)
 {
 	unsigned int size;
@@ -441,7 +434,6 @@ bool have_tracepoints(struct list_head *pattrs)
 
 int read_tracing_data(int fd, struct list_head *pattrs)
 {
-	char buf[BUFSIZ];
 	struct tracepoint_path *tps = get_tracepoints_path(pattrs);
 
 	/*
@@ -452,33 +444,8 @@ int read_tracing_data(int fd, struct list_head *pattrs)
 
 	output_fd = fd;
 
-	buf[0] = 23;
-	buf[1] = 8;
-	buf[2] = 68;
-	memcpy(buf + 3, "tracing", 7);
-
-	write_or_die(buf, 10);
-
-	write_or_die(VERSION, strlen(VERSION) + 1);
-
-	/* save endian */
-	if (bigendian())
-		buf[0] = 1;
-	else
-		buf[0] = 0;
-
-	write_or_die(buf, 1);
-
-	/* save size of long */
-	buf[0] = sizeof(long);
-	write_or_die(buf, 1);
-
-	/* save page_size */
-	page_size = sysconf(_SC_PAGESIZE);
-	write_or_die(&page_size, 4);
-
+	tracing_data_header();
 	read_header_files();
-	read_ftrace_files(tps);
 	read_event_files(tps);
 	read_proc_kallsyms();
 	read_ftrace_printk();
-- 
1.7.4

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Steven Rostedt]

On Mon, 2011-09-26 at 11:11 +0200, Jiri Olsa wrote:
> Changing the way the event files are searched by quering specified
> event files directly, instead of walking the events directory.
> 
> Hopefully this way is more straightforward and faster.

Have you looked at my code I posted earlier that uses the libparsevents?

It uses globs such that you could do -e sched:sched* and it will enable
all sched events.

-- Steve



> +	 * and one safety byte ;)
> +	 */
> +	file = malloc_or_die(strlen(path) + strlen(sys) +
> +			     strlen(name) + strlen("format") + 5);
>  
> -		free(format);
> -	}
> -	closedir(dir);
> +	sprintf(file, "%s/%s/%s/format", path, sys, name);
> +	return file;
>  }
>  
> -static void read_ftrace_files(struct tracepoint_path *tps)
> +static void put_format_file(char *file)
>  {
> -	char *path;
> -
> -	path = get_tracing_file("events/ftrace");
> -
> -	copy_event_system(path, tps);
> -
> -	put_tracing_file(path);
> +	free(file);
>  }
>  
> -static bool system_in_tp_list(char *sys, struct tracepoint_path *tps)
> +/*
> + * Walk tracepoint event objects and store them.
> + * Only those matching the sys parameter are stored
> + * and marked as done.
> + */
> +static void read_event_files_system(struct tracepoint_path *tps,
> +				    const char *sys)
>  {
> +	off_t count_pos;
> +	u32 count = 0;
> +
> +	/* Place for number of events under single system. */
> +	count_pos = lseek(output_fd, 0, SEEK_CUR);
> +	write_or_die(&count, 4);
> +
>  	while (tps) {
> -		if (!strcmp(sys, tps->system))
> -			return true;
> +		if ((!tps->done) &&
> +		    (!strcmp(sys, tps->system))) {
> +			char *file;
> +
> +			file = get_format_file(tps->system, tps->name);
> +			if (file) {
> +				record_file(file, 8);
> +				count++;
> +			}
> +
> +			put_format_file(file);
> +			tps->done = 1;
> +		}
> +
>  		tps = tps->next;

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Jiri Olsa]

On Mon, Sep 26, 2011 at 09:36:31AM -0400, Steven Rostedt wrote:
> On Mon, 2011-09-26 at 11:11 +0200, Jiri Olsa wrote:
> > Changing the way the event files are searched by quering specified
> > event files directly, instead of walking the events directory.
> > 
> > Hopefully this way is more straightforward and faster.
> 
> Have you looked at my code I posted earlier that uses the libparsevents?
> 
> It uses globs such that you could do -e sched:sched* and it will enable
> all sched events.

ops, haven't seen those changes yet..
I think I can go only with 2/2 patch, if the 1/2 collides with your changes

I'll check, thanks
jirka

> 
> -- Steve
> 
> 
> 
> > +	 * and one safety byte ;)
> > +	 */
> > +	file = malloc_or_die(strlen(path) + strlen(sys) +
> > +			     strlen(name) + strlen("format") + 5);
> >  
> > -		free(format);
> > -	}
> > -	closedir(dir);
> > +	sprintf(file, "%s/%s/%s/format", path, sys, name);
> > +	return file;
> >  }
> >  
> > -static void read_ftrace_files(struct tracepoint_path *tps)
> > +static void put_format_file(char *file)
> >  {
> > -	char *path;
> > -
> > -	path = get_tracing_file("events/ftrace");
> > -
> > -	copy_event_system(path, tps);
> > -
> > -	put_tracing_file(path);
> > +	free(file);
> >  }
> >  
> > -static bool system_in_tp_list(char *sys, struct tracepoint_path *tps)
> > +/*
> > + * Walk tracepoint event objects and store them.
> > + * Only those matching the sys parameter are stored
> > + * and marked as done.
> > + */
> > +static void read_event_files_system(struct tracepoint_path *tps,
> > +				    const char *sys)
> >  {
> > +	off_t count_pos;
> > +	u32 count = 0;
> > +
> > +	/* Place for number of events under single system. */
> > +	count_pos = lseek(output_fd, 0, SEEK_CUR);
> > +	write_or_die(&count, 4);
> > +
> >  	while (tps) {
> > -		if (!strcmp(sys, tps->system))
> > -			return true;
> > +		if ((!tps->done) &&
> > +		    (!strcmp(sys, tps->system))) {
> > +			char *file;
> > +
> > +			file = get_format_file(tps->system, tps->name);
> > +			if (file) {
> > +				record_file(file, 8);
> > +				count++;
> > +			}
> > +
> > +			put_format_file(file);
> > +			tps->done = 1;
> > +		}
> > +
> >  		tps = tps->next;
> 
> 

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Frederic Weisbecker]

On Mon, Sep 26, 2011 at 04:56:06PM +0200, Jiri Olsa wrote:
> On Mon, Sep 26, 2011 at 09:36:31AM -0400, Steven Rostedt wrote:
> > On Mon, 2011-09-26 at 11:11 +0200, Jiri Olsa wrote:
> > > Changing the way the event files are searched by quering specified
> > > event files directly, instead of walking the events directory.
> > > 
> > > Hopefully this way is more straightforward and faster.
> > 
> > Have you looked at my code I posted earlier that uses the libparsevents?
> > 
> > It uses globs such that you could do -e sched:sched* and it will enable
> > all sched events.
> 
> ops, haven't seen those changes yet..
> I think I can go only with 2/2 patch, if the 1/2 collides with your changes

But it seems Steve's patches are not completely uncontroversial because
of some crazy disagreements on where the libparsevent.so should lay (tools generic
or tied to perf).

So until we get that situation solved, we should continue to move forward.

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Steven Rostedt]

On Wed, 2011-09-28 at 15:55 +0200, Frederic Weisbecker wrote:
> On Mon, Sep 26, 2011 at 04:56:06PM +0200, Jiri Olsa wrote:
> > On Mon, Sep 26, 2011 at 09:36:31AM -0400, Steven Rostedt wrote:
> > > On Mon, 2011-09-26 at 11:11 +0200, Jiri Olsa wrote:
> > > > Changing the way the event files are searched by quering specified
> > > > event files directly, instead of walking the events directory.
> > > > 
> > > > Hopefully this way is more straightforward and faster.
> > > 
> > > Have you looked at my code I posted earlier that uses the libparsevents?
> > > 
> > > It uses globs such that you could do -e sched:sched* and it will enable
> > > all sched events.
> > 
> > ops, haven't seen those changes yet..
> > I think I can go only with 2/2 patch, if the 1/2 collides with your changes
> 
> But it seems Steve's patches are not completely uncontroversial because
> of some crazy disagreements on where the libparsevent.so should lay (tools generic
> or tied to perf).

Which to me seems to be a silly road block, in which I never got a clear
answer for.

> 
> So until we get that situation solved, we should continue to move forward.

Sure, but it just forks the code even more, and it's almost to the point
where maintaining a fork will just be easier, not to mention quicker to
get out to the distros.

-- Steve

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Frederic Weisbecker]

On Wed, Sep 28, 2011 at 10:03:08AM -0400, Steven Rostedt wrote:
> On Wed, 2011-09-28 at 15:55 +0200, Frederic Weisbecker wrote:
> > On Mon, Sep 26, 2011 at 04:56:06PM +0200, Jiri Olsa wrote:
> > > On Mon, Sep 26, 2011 at 09:36:31AM -0400, Steven Rostedt wrote:
> > > > On Mon, 2011-09-26 at 11:11 +0200, Jiri Olsa wrote:
> > > > > Changing the way the event files are searched by quering specified
> > > > > event files directly, instead of walking the events directory.
> > > > > 
> > > > > Hopefully this way is more straightforward and faster.
> > > > 
> > > > Have you looked at my code I posted earlier that uses the libparsevents?
> > > > 
> > > > It uses globs such that you could do -e sched:sched* and it will enable
> > > > all sched events.
> > > 
> > > ops, haven't seen those changes yet..
> > > I think I can go only with 2/2 patch, if the 1/2 collides with your changes
> > 
> > But it seems Steve's patches are not completely uncontroversial because
> > of some crazy disagreements on where the libparsevent.so should lay (tools generic
> > or tied to perf).
> 
> Which to me seems to be a silly road block, in which I never got a clear
> answer for.

Yeah we need to sort that out with Ingo.

> 
> > 
> > So until we get that situation solved, we should continue to move forward.
> 
> Sure, but it just forks the code even more, and it's almost to the point
> where maintaining a fork will just be easier, not to mention quicker to
> get out to the distros.

Right. May be Jiri can have a look at what trace-cmd does there.

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Steven Rostedt]

On Wed, 2011-09-28 at 16:17 +0200, Frederic Weisbecker wrote:
> > > 
> > > But it seems Steve's patches are not completely uncontroversial because
> > > of some crazy disagreements on where the libparsevent.so should lay (tools generic
> > > or tied to perf).
> > 
> > Which to me seems to be a silly road block, in which I never got a clear
> > answer for.
> 
> Yeah we need to sort that out with Ingo.

And include Arnaldo, as he was the one that suggested to do what I did
that Ingo NACKd. And I consider Arnaldo the maintainer of the userspace
side of perf, so I will not step on his toes in what I do.

-- Steve

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> > > But it seems Steve's patches are not completely uncontroversial 
> > > because of some crazy disagreements on where the 
> > > libparsevent.so should lay (tools generic or tied to perf).
> > 
> > Which to me seems to be a silly road block, in which I never got 
> > a clear answer for.
> 
> Yeah we need to sort that out with Ingo.

Basically, since it's not at all clear to me where these things (and 
APIs) will go, I'd be much more comfortable with this starting out as 
tools/perf/lib/ - we can still split it out later on. Merging it in 
will be a lot harder.

Thanks,

	Ingo

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Steven Rostedt]

Good to see your email is back :)


On Wed, 2011-09-28 at 18:56 +0200, Ingo Molnar wrote:
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > > > But it seems Steve's patches are not completely uncontroversial 
> > > > because of some crazy disagreements on where the 
> > > > libparsevent.so should lay (tools generic or tied to perf).
> > > 
> > > Which to me seems to be a silly road block, in which I never got 
> > > a clear answer for.
> > 
> > Yeah we need to sort that out with Ingo.
> 
> Basically, since it's not at all clear to me where these things (and 
> APIs) will go, I'd be much more comfortable with this starting out as 
> tools/perf/lib/ - we can still split it out later on. Merging it in 
> will be a lot harder.

I would actually argue that merging is much easier than splitting
something up. If A depends on B, merging B back into A is trivial. But
if B is a part of A, breaking it out of A is a much more difficult task,
as the boundaries of A and B are not so easy to find and the coupling of
the two is much tighter.

I find trying to break things out of perf is very hard to do as the
dependencies are throughout all of perf, and no one seems to agree on
how to do so.

-- Steve

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Good to see your email is back :)
> 
> 
> On Wed, 2011-09-28 at 18:56 +0200, Ingo Molnar wrote:
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > > > But it seems Steve's patches are not completely uncontroversial 
> > > > > because of some crazy disagreements on where the 
> > > > > libparsevent.so should lay (tools generic or tied to perf).
> > > > 
> > > > Which to me seems to be a silly road block, in which I never got 
> > > > a clear answer for.
> > > 
> > > Yeah we need to sort that out with Ingo.
> > 
> > Basically, since it's not at all clear to me where these things (and 
> > APIs) will go, I'd be much more comfortable with this starting out as 
> > tools/perf/lib/ - we can still split it out later on. Merging it in 
> > will be a lot harder.
> 
> I would actually argue that merging is much easier than splitting 
> something up. [...]

Technically it's somewhat easier - socially, not.

> [...] If A depends on B, merging B back into A is trivial. But if B 
> is a part of A, breaking it out of A is a much more difficult task, 
> as the boundaries of A and B are not so easy to find and the 
> coupling of the two is much tighter.

That's a technical problem. In reality we can split up and merge 
projects of very significant size just fine. I've done split-ups and 
factorings-out from millions of lines of code impact. tools/perf is 
still well within such size boundaries.

> I find trying to break things out of perf is very hard to do as the 
> dependencies are throughout all of perf, and no one seems to agree 
> on how to do so.

Then it would be absolute madness to make that non-agreement external 
and hard-code a separate social structure for it!

Libraries are for *boring* infrastructure stuff *everyone agrees on*. 

They are absolutely lousy arbitrators of technical disagreement and 
if there's a serious disagreement they can easily become an absolute 
nightmare socially and a technical distraction. How difficult is this 
to understand?

Please work out disagreements with Arnaldo and do librarization 
within perf if you are interested in that angle. If those internal 
interfaces become visibly boring and are consistently used by 
everything in a way that every main contributor agrees on then we can 
perhaps librarize it. Not the other way around.

Thanks,

	Ingo

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Steven Rostedt]

On Mon, 2011-10-10 at 07:22 +0200, Ingo Molnar wrote:
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
>  
> > I would actually argue that merging is much easier than splitting 
> > something up. [...]
> 
> Technically it's somewhat easier - socially, not.

I'm trying to keep this on a technical level.

> 
> > [...] If A depends on B, merging B back into A is trivial. But if B 
> > is a part of A, breaking it out of A is a much more difficult task, 
> > as the boundaries of A and B are not so easy to find and the 
> > coupling of the two is much tighter.
> 
> That's a technical problem. In reality we can split up and merge 
> projects of very significant size just fine. I've done split-ups and 
> factorings-out from millions of lines of code impact. tools/perf is 
> still well within such size boundaries.

I find splitting perf up is not so easy. Perhaps if you do it so well
then you could help.

> 
> > I find trying to break things out of perf is very hard to do as the 
> > dependencies are throughout all of perf, and no one seems to agree 
> > on how to do so.
> 
> Then it would be absolute madness to make that non-agreement external 
> and hard-code a separate social structure for it!

I seem to agree with Arnaldo, Boris and Frederic, but you do not seem to
agree. My arguments with Arnaldo started with "but Ingo wants it this
way", his reply was, "What do you want". Honestly, I agreed with
Arnaldo.

> 
> Libraries are for *boring* infrastructure stuff *everyone agrees on*. 


But what I have proposed is something that we all agreed on (besides
you) that it *is* boring infrastructure stuff. Hell, the code never was
developed for perf in the first place. Frederic pulled it in from
trace-cmd. The update code currently lives in trace-cmd as a separate
library, that was designed to work with any other project (including
perf). You want me to rip it apart to make it a perf only lib?

Currently perf has an old outdated version, because it never was
incorporated into perf as a library. If it was, then it could have
easily benefited by the updates. A lot of people want perf to have the
functionality that trace-cmd currently has. But because we are fighting
over where the library will be, perf is still suffering.


> 
> They are absolutely lousy arbitrators of technical disagreement and 
> if there's a serious disagreement they can easily become an absolute 
> nightmare socially and a technical distraction. How difficult is this 
> to understand?

Must be very difficult to understand, because I do not understand why a
library that can work with multiple tools (perf, powertop, latencytrace,
trace-cmd, kernelshark) needs to be in tools/perf/lib instead of
tools/lib?


> 
> Please work out disagreements with Arnaldo and do librarization 
> within perf if you are interested in that angle. If those internal 
> interfaces become visibly boring and are consistently used by 
> everything in a way that every main contributor agrees on then we can 
> perhaps librarize it. Not the other way around.

For parsing of events, which is what I proposed, it seems to be boring
technical stuff that has been well established by the limitations of the
ABI forced on the debugfs system. It's already over a year old and used
by several developers. One nice feature to come with this is the ability
to add plugins to parse the trace events without needing to read all the
format files. Things like kvm events will suddenly work.

The one disagreement that we are still sorting out is just the name of
the library. I first said libperf (which would make sense to have in
tools/perf/lib) but then it wasn't doing perf specific work. It was just
a way to parse kernel trace points. Nobody seemed to like libparsevent.
I think one of the names that makes sense is libtrace, or libktrace as
it has to do with tracepoints in the kernel. Maybe just libktracepoint?
But that's quite an ugly name.


-- Steve

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Frederic Weisbecker]

On Mon, Oct 10, 2011 at 08:27:34AM -0400, Steven Rostedt wrote:
> > Please work out disagreements with Arnaldo and do librarization 
> > within perf if you are interested in that angle. If those internal 
> > interfaces become visibly boring and are consistently used by 
> > everything in a way that every main contributor agrees on then we can 
> > perhaps librarize it. Not the other way around.
> 
> For parsing of events, which is what I proposed, it seems to be boring
> technical stuff that has been well established by the limitations of the
> ABI forced on the debugfs system. It's already over a year old and used
> by several developers. One nice feature to come with this is the ability
> to add plugins to parse the trace events without needing to read all the
> format files. Things like kvm events will suddenly work.
> 
> The one disagreement that we are still sorting out is just the name of
> the library. I first said libperf (which would make sense to have in
> tools/perf/lib) but then it wasn't doing perf specific work. It was just
> a way to parse kernel trace points. Nobody seemed to like libparsevent.
> I think one of the names that makes sense is libtrace, or libktrace as
> it has to do with tracepoints in the kernel. Maybe just libktracepoint?
> But that's quite an ugly name.

libtracevent ?

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[David Ahern]

On 09/26/2011 03:11 AM, Jiri Olsa wrote:
> Changing the way the event files are searched by quering specified
> event files directly, instead of walking the events directory.
> 
> Hopefully this way is more straightforward and faster.
> 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> ---
>  tools/perf/util/parse-events.h     |    1 +
>  tools/perf/util/trace-event-info.c |  231 +++++++++++++++--------------------
>  2 files changed, 100 insertions(+), 132 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 2f8e375..1ea02ad 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -15,6 +15,7 @@ struct option;
>  struct tracepoint_path {
>  	char *system;
>  	char *name;
> +	bool done;
>  	struct tracepoint_path *next;
>  };
>  
> diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
> index 3403f81..cf8f89e 100644
> --- a/tools/perf/util/trace-event-info.c
> +++ b/tools/perf/util/trace-event-info.c
> @@ -238,136 +238,98 @@ static void read_header_files(void)
>  	put_tracing_file(path);
>  }
>  
> -static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
> +static char *get_format_file(char *sys, char *name)
>  {
> -	while (tps) {
> -		if (!strcmp(sys, tps->name))
> -			return true;
> -		tps = tps->next;
> -	}
> -
> -	return false;
> -}
> -
> -static void copy_event_system(const char *sys, struct tracepoint_path *tps)
> -{
> -	struct dirent *dent;
> -	struct stat st;
> -	char *format;
> -	DIR *dir;
> -	int count = 0;
> -	int ret;
> -
> -	dir = opendir(sys);
> -	if (!dir)
> -		die("can't read directory '%s'", sys);
> -
> -	while ((dent = readdir(dir))) {
> -		if (dent->d_type != DT_DIR ||
> -		    strcmp(dent->d_name, ".") == 0 ||
> -		    strcmp(dent->d_name, "..") == 0 ||
> -		    !name_in_tp_list(dent->d_name, tps))
> -			continue;
> -		format = malloc_or_die(strlen(sys) + strlen(dent->d_name) + 10);
> -		sprintf(format, "%s/%s/format", sys, dent->d_name);
> -		ret = stat(format, &st);
> -		free(format);
> -		if (ret < 0)
> -			continue;
> -		count++;
> -	}
> -
> -	write_or_die(&count, 4);
> +	char *file, *path;
>  
> -	rewinddir(dir);
> -	while ((dent = readdir(dir))) {
> -		if (dent->d_type != DT_DIR ||
> -		    strcmp(dent->d_name, ".") == 0 ||
> -		    strcmp(dent->d_name, "..") == 0 ||
> -		    !name_in_tp_list(dent->d_name, tps))
> -			continue;
> -		format = malloc_or_die(strlen(sys) + strlen(dent->d_name) + 10);
> -		sprintf(format, "%s/%s/format", sys, dent->d_name);
> -		ret = stat(format, &st);
> +	path = get_tracing_file("events");
> +	if (!path)
> +		return NULL;
>  
> -		if (ret >= 0)
> -			record_file(format, 8);
> +	/*
> +	 * The '+ 5' is for '/' we add to the patch, plus NULL byte,
> +	 * and one safety byte ;)
> +	 */
> +	file = malloc_or_die(strlen(path) + strlen(sys) +
> +			     strlen(name) + strlen("format") + 5);

Why not go the simple route and just use PATH_MAX?

David


>  
> -		free(format);
> -	}
> -	closedir(dir);
> +	sprintf(file, "%s/%s/%s/format", path, sys, name);
> +	return file;
>  }
>  
> -static void read_ftrace_files(struct tracepoint_path *tps)
> +static void put_format_file(char *file)
>  {
> -	char *path;
> -
> -	path = get_tracing_file("events/ftrace");
> -
> -	copy_event_system(path, tps);
> -
> -	put_tracing_file(path);
> +	free(file);
>  }
>  
> -static bool system_in_tp_list(char *sys, struct tracepoint_path *tps)
> +/*
> + * Walk tracepoint event objects and store them.
> + * Only those matching the sys parameter are stored
> + * and marked as done.
> + */
> +static void read_event_files_system(struct tracepoint_path *tps,
> +				    const char *sys)
>  {
> +	off_t count_pos;
> +	u32 count = 0;
> +
> +	/* Place for number of events under single system. */
> +	count_pos = lseek(output_fd, 0, SEEK_CUR);
> +	write_or_die(&count, 4);
> +
>  	while (tps) {
> -		if (!strcmp(sys, tps->system))
> -			return true;
> +		if ((!tps->done) &&
> +		    (!strcmp(sys, tps->system))) {
> +			char *file;
> +
> +			file = get_format_file(tps->system, tps->name);
> +			if (file) {
> +				record_file(file, 8);
> +				count++;
> +			}
> +
> +			put_format_file(file);
> +			tps->done = 1;
> +		}
> +
>  		tps = tps->next;
>  	}
>  
> -	return false;
> +	if (pwrite(output_fd, &count, 4, count_pos) < 0)
> +		die("pwrite");
>  }
>  
> +/*
> + * We have all needed tracepoint event files stored in
> + * the tracepoint_path objects.
> + *
> + * - first we store ftrace system events
> + * - then we walk all '!done' event objects
> + *   and process them
> + */
>  static void read_event_files(struct tracepoint_path *tps)
>  {
> -	struct dirent *dent;
> -	struct stat st;
> -	char *path;
> -	char *sys;
> -	DIR *dir;
> -	int count = 0;
> -	int ret;
> -
> -	path = get_tracing_file("events");
> +	off_t count_pos;
> +	u32 count = 0;
>  
> -	dir = opendir(path);
> -	if (!dir)
> -		die("can't read directory '%s'", path);
> -
> -	while ((dent = readdir(dir))) {
> -		if (dent->d_type != DT_DIR ||
> -		    strcmp(dent->d_name, ".") == 0 ||
> -		    strcmp(dent->d_name, "..") == 0 ||
> -		    strcmp(dent->d_name, "ftrace") == 0 ||
> -		    !system_in_tp_list(dent->d_name, tps))
> -			continue;
> -		count++;
> -	}
> +	read_event_files_system(tps, "ftrace");
>  
> +	/* systems count */
> +	count_pos = lseek(output_fd, 0, SEEK_CUR);
>  	write_or_die(&count, 4);
>  
> -	rewinddir(dir);
> -	while ((dent = readdir(dir))) {
> -		if (dent->d_type != DT_DIR ||
> -		    strcmp(dent->d_name, ".") == 0 ||
> -		    strcmp(dent->d_name, "..") == 0 ||
> -		    strcmp(dent->d_name, "ftrace") == 0 ||
> -		    !system_in_tp_list(dent->d_name, tps))
> -			continue;
> -		sys = malloc_or_die(strlen(path) + strlen(dent->d_name) + 2);
> -		sprintf(sys, "%s/%s", path, dent->d_name);
> -		ret = stat(sys, &st);
> -		if (ret >= 0) {
> -			write_or_die(dent->d_name, strlen(dent->d_name) + 1);
> -			copy_event_system(sys, tps);
> +	while (tps) {
> +		if (!tps->done) {
> +			write_or_die(tps->system, strlen(tps->system) + 1);
> +			read_event_files_system(tps, tps->system);
> +			count++;
>  		}
> -		free(sys);
> +
> +		tps = tps->next;
>  	}
>  
> -	closedir(dir);
> -	put_tracing_file(path);
> +	if (pwrite(output_fd, &count, 4, count_pos) < 0)
> +		die("write");
>  }
>  
>  static void read_proc_kallsyms(void)
> @@ -387,6 +349,37 @@ static void read_proc_kallsyms(void)
>  	record_file(path, 4);
>  }
>  
> +static void tracing_data_header(void)
> +{
> +	char buf[50];
> +
> +	/* just guessing this is someone's birthday.. ;) */
> +	buf[0] = 23;
> +	buf[1] = 8;
> +	buf[2] = 68;
> +	memcpy(buf + 3, "tracing", 7);
> +
> +	write_or_die(buf, 10);
> +
> +	write_or_die(VERSION, strlen(VERSION) + 1);
> +
> +	/* save endian */
> +	if (bigendian())
> +		buf[0] = 1;
> +	else
> +		buf[0] = 0;
> +
> +	write_or_die(buf, 1);
> +
> +	/* save size of long */
> +	buf[0] = sizeof(long);
> +	write_or_die(buf, 1);
> +
> +	/* save page_size */
> +	page_size = sysconf(_SC_PAGESIZE);
> +	write_or_die(&page_size, 4);
> +}
> +
>  static void read_ftrace_printk(void)
>  {
>  	unsigned int size;
> @@ -441,7 +434,6 @@ bool have_tracepoints(struct list_head *pattrs)
>  
>  int read_tracing_data(int fd, struct list_head *pattrs)
>  {
> -	char buf[BUFSIZ];
>  	struct tracepoint_path *tps = get_tracepoints_path(pattrs);
>  
>  	/*
> @@ -452,33 +444,8 @@ int read_tracing_data(int fd, struct list_head *pattrs)
>  
>  	output_fd = fd;
>  
> -	buf[0] = 23;
> -	buf[1] = 8;
> -	buf[2] = 68;
> -	memcpy(buf + 3, "tracing", 7);
> -
> -	write_or_die(buf, 10);
> -
> -	write_or_die(VERSION, strlen(VERSION) + 1);
> -
> -	/* save endian */
> -	if (bigendian())
> -		buf[0] = 1;
> -	else
> -		buf[0] = 0;
> -
> -	write_or_die(buf, 1);
> -
> -	/* save size of long */
> -	buf[0] = sizeof(long);
> -	write_or_die(buf, 1);
> -
> -	/* save page_size */
> -	page_size = sysconf(_SC_PAGESIZE);
> -	write_or_die(&page_size, 4);
> -
> +	tracing_data_header();
>  	read_header_files();
> -	read_ftrace_files(tps);
>  	read_event_files(tps);
>  	read_proc_kallsyms();
>  	read_ftrace_printk();

Re: [PATCHv2 1/2] perf tools: Collect tracing event data files directly

[Jiri Olsa]

On Mon, Sep 26, 2011 at 07:43:02AM -0600, David Ahern wrote:
> 
> 

SNIP

> > +	/*
> > +	 * The '+ 5' is for '/' we add to the patch, plus NULL byte,
> > +	 * and one safety byte ;)
> > +	 */
> > +	file = malloc_or_die(strlen(path) + strlen(sys) +
> > +			     strlen(name) + strlen("format") + 5);
> 
> Why not go the simple route and just use PATH_MAX?
> 
> David

ok, seems more simple.. ;)

thanks,
jirka

[PATCHv2 2/2] perf tools: Fix tracing info recording

[Jiri Olsa]

Fixing the way the tracing information is stored within record command.
The current implementation is causing inconsistency issues for pipe
output (described below), also following commands fail currently:

	perf script syscall-counts ls
	perf record -e syscalls:sys_exit_read ls | ./perf report -i -

The tracing information is part of the perf data file. It contains
several files from within the tracing debugfs and procs directories.

Beside some static header files, for each tracing event the format
file is added. The /proc/kallsyms file is also added.

The tracing data are stored with preceeding size. This is causing
dificulties for pipe output, since there's no way to tell debugfs/proc
file size before reading it. So, for pipe output, all the debugfs files
were read twice. Once to get the overall size and once to store the
content itself. This can cause problem in case any of these file
changed, within the storage time.

To fix this behaviour and ensure the integrity of the tracing data, we:
    - read debugfs/proc file into the temp file
    - get temp file size and dump it to the pipe
    - dump the temp file contents to the pipe

This change also fix current code, which did not distinguish regular
file and pipe and failed on seek call over pipe.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/header.c           |   50 +++++++++++++++++---
 tools/perf/util/trace-event-info.c |   90 ++++++++++++++++++++++++++---------
 tools/perf/util/trace-event.h      |   11 ++++-
 3 files changed, 118 insertions(+), 33 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index b6c1ad1..477c73b 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -382,18 +382,33 @@ static int perf_header__adds_write(struct perf_header *header,
 
 	sec_size = sizeof(*feat_sec) * nr_sections;
 
+	/*
+	 * We reserve header space for each section, and
+	 * update/store it later once we know the section size.
+	 */
 	sec_start = header->data_offset + header->data_size;
 	lseek(fd, sec_start + sec_size, SEEK_SET);
 
 	if (perf_header__has_feat(header, HEADER_TRACE_INFO)) {
 		struct perf_file_section *trace_sec;
+		struct tracing_data *tdata;
 
 		trace_sec = &feat_sec[idx++];
-
-		/* Write trace info */
 		trace_sec->offset = lseek(fd, 0, SEEK_CUR);
-		read_tracing_data(fd, &evlist->entries);
-		trace_sec->size = lseek(fd, 0, SEEK_CUR) - trace_sec->offset;
+
+		/*
+		 * We work over the real file, so we can write data
+		 * directly, no temp file is needed.
+		 */
+		tdata = tracing_data_get(&evlist->entries, fd, false);
+		if (!tdata)
+			goto out_free;
+
+		/*
+		 * Update the section header information.
+		 */
+		trace_sec->size = tdata->size;
+		tracing_data_put(tdata);
 	}
 
 	if (perf_header__has_feat(header, HEADER_BUILD_ID)) {
@@ -1100,15 +1115,29 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
 				   struct perf_session *session __unused)
 {
 	union perf_event ev;
+	struct tracing_data *tdata;
 	ssize_t size = 0, aligned_size = 0, padding;
 	int err __used = 0;
 
+	/*
+	 * We are going to store the size of the data followed
+	 * by the data contents. Since the fd descriptor is a pipe,
+	 * we cannot seek back to store the size of the data once
+	 * we know it. Instead we:
+	 *
+	 * - write the tracing data to the temp file
+	 * - get/write the data size to pipe
+	 * - write the tracing data from the temp file
+	 *   to the pipe
+	 */
+	tdata = tracing_data_get(&evlist->entries, fd, true);
+	if (!tdata)
+		return -1;
+
 	memset(&ev, 0, sizeof(ev));
 
 	ev.tracing_data.header.type = PERF_RECORD_HEADER_TRACING_DATA;
-	size = read_tracing_data_size(fd, &evlist->entries);
-	if (size <= 0)
-		return size;
+	size = tdata->size;
 	aligned_size = ALIGN(size, sizeof(u64));
 	padding = aligned_size - size;
 	ev.tracing_data.header.size = sizeof(ev.tracing_data);
@@ -1116,7 +1145,12 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
 
 	process(&ev, NULL, session);
 
-	err = read_tracing_data(fd, &evlist->entries);
+	/*
+	 * The put function will copy all the tracing data
+	 * stored in temp file to the pipe.
+	 */
+	tracing_data_put(tdata);
+
 	write_padded(fd, NULL, 0, padding);
 
 	return aligned_size;
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index cf8f89e..2e73e09 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -196,7 +196,8 @@ static void record_file(const char *file, size_t hdr_sz)
 		die("Can't read '%s'", file);
 
 	/* put in zeros for file size, then fill true size later */
-	write_or_die(&size, hdr_sz);
+	if (hdr_sz)
+		write_or_die(&size, hdr_sz);
 
 	do {
 		r = read(fd, buf, BUFSIZ);
@@ -212,7 +213,7 @@ static void record_file(const char *file, size_t hdr_sz)
 	if (bigendian())
 		sizep += sizeof(u64) - hdr_sz;
 
-	if (pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
+	if (hdr_sz && pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
 		die("writing to %s", output_file);
 }
 
@@ -421,6 +422,19 @@ get_tracepoints_path(struct list_head *pattrs)
 	return nr_tracepoints > 0 ? path.next : NULL;
 }
 
+static void
+put_tracepoints_path(struct tracepoint_path *tps)
+{
+	while (tps) {
+		struct tracepoint_path *t = tps;
+
+		tps = tps->next;
+		free(t->name);
+		free(t->system);
+		free(t);
+	}
+}
+
 bool have_tracepoints(struct list_head *pattrs)
 {
 	struct perf_evsel *pos;
@@ -432,39 +446,69 @@ bool have_tracepoints(struct list_head *pattrs)
 	return false;
 }
 
-int read_tracing_data(int fd, struct list_head *pattrs)
+struct tracing_data *tracing_data_get(struct list_head *pattrs,
+				      int fd, bool temp)
 {
-	struct tracepoint_path *tps = get_tracepoints_path(pattrs);
-
-	/*
-	 * What? No tracepoints? No sense writing anything here, bail out.
-	 */
-	if (tps == NULL)
-		return -1;
+	struct tracepoint_path *tps;
+	struct tracing_data *tdata;
 
 	output_fd = fd;
 
+	tps = get_tracepoints_path(pattrs);
+	if (!tps)
+		return NULL;
+
+	tdata = malloc_or_die(sizeof(*tdata));
+	tdata->temp = temp;
+	tdata->size = 0;
+
+	if (temp) {
+		int temp_fd;
+
+		snprintf(tdata->temp_file, sizeof(tdata->temp_file),
+			 "/tmp/perf-XXXXXX");
+		if (!mkstemp(tdata->temp_file))
+			die("Can't make temp file");
+
+		temp_fd = open(tdata->temp_file, O_RDWR);
+		if (temp_fd < 0)
+			die("Can't read '%s'", tdata->temp_file);
+
+		/*
+		 * Set the temp file the default output, so all the
+		 * tracing data are stored into it.
+		 */
+		output_fd = temp_fd;
+	} else
+		tdata->size = -lseek(output_fd, 0, SEEK_CUR);
+
 	tracing_data_header();
 	read_header_files();
 	read_event_files(tps);
 	read_proc_kallsyms();
 	read_ftrace_printk();
 
-	return 0;
-}
+	tdata->size += lseek(output_fd, 0, SEEK_CUR);
 
-ssize_t read_tracing_data_size(int fd, struct list_head *pattrs)
-{
-	ssize_t size;
-	int err = 0;
+	/*
+	 * All tracing data are stored by now, we can restore
+	 * the default output file in case we used temp file.
+	 */
+	if (temp) {
+		close(output_fd);
+		output_fd = fd;
+	}
 
-	calc_data_size = 1;
-	err = read_tracing_data(fd, pattrs);
-	size = calc_data_size - 1;
-	calc_data_size = 0;
+	put_tracepoints_path(tps);
+	return tdata;
+}
 
-	if (err < 0)
-		return err;
+void tracing_data_put(struct tracing_data *tdata)
+{
+	if (tdata->temp) {
+		record_file(tdata->temp_file, 0);
+		unlink(tdata->temp_file);
+	}
 
-	return size;
+	free(tdata);
 }
diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
index f674dda..92274b9 100644
--- a/tools/perf/util/trace-event.h
+++ b/tools/perf/util/trace-event.h
@@ -262,8 +262,15 @@ raw_field_value(struct event *event, const char *name, void *data);
 void *raw_field_ptr(struct event *event, const char *name, void *data);
 unsigned long long eval_flag(const char *flag);
 
-int read_tracing_data(int fd, struct list_head *pattrs);
-ssize_t read_tracing_data_size(int fd, struct list_head *pattrs);
+struct tracing_data {
+	ssize_t size;
+	bool temp;
+	char temp_file[50];
+};
+
+struct tracing_data *tracing_data_get(struct list_head *pattrs,
+				      int fd, bool temp);
+void tracing_data_put(struct tracing_data *tdata);
 
 /* taken from kernel/trace/trace.h */
 enum trace_flag_type {
-- 
1.7.4

[PATCHv3 0/2] perf tools: Fix tracing info recording

[Jiri Olsa]

hi,
attached patches:
- 1/2 perf tools: Fix raw sample reading
- 2/2 perf tools: Fix tracing info recording

v3 changes:
 - I cut off the 1/2 patch from v2, and replaced it with another
   fix which I found out is needed during the testing.. ;)

 - The 2/2 patch is same as in v2, besides changing to the
   current tip tree.

thanks for comments,
jirka
---
 tools/perf/util/evsel.c            |    7 ++-
 tools/perf/util/header.c           |   50 +++++++++++++++---
 tools/perf/util/trace-event-info.c |  102 ++++++++++++++++++++++++++---------
 tools/perf/util/trace-event.h      |   11 +++-
 4 files changed, 132 insertions(+), 38 deletions(-)

[PATCHv3 1/2] perf tools: Fix raw sample reading

[Jiri Olsa]

Wrong pointer is being passed for raw data sanity checking,
when parsing sample event.

This ends up with invalid event and perf record being stuck in
__perf_session__process_events function during processing
build IDs (process_buildids function).

Following command hangs up in my setup:
	./perf record -e raw_syscalls:sys_enter ls

The fix is to use proper pointer to the raw data instead
of the 'u' union.

CC: David Ahern <dsahern@gmail.com>
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/evsel.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index c5748c5..e389815 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -449,6 +449,8 @@ int perf_event__parse_sample(const union perf_event *event, u64 type,
 	}
 
 	if (type & PERF_SAMPLE_RAW) {
+		const u64 *pdata;
+
 		u.val64 = *array;
 		if (WARN_ONCE(swapped,
 			      "Endianness of raw data not corrected!\n")) {
@@ -462,11 +464,12 @@ int perf_event__parse_sample(const union perf_event *event, u64 type,
 			return -EFAULT;
 
 		data->raw_size = u.val32[0];
+		pdata = (void *) array + sizeof(u32);
 
-		if (sample_overlap(event, &u.val32[1], data->raw_size))
+		if (sample_overlap(event, pdata, data->raw_size))
 			return -EFAULT;
 
-		data->raw_data = &u.val32[1];
+		data->raw_data = (void *) pdata;
 	}
 
 	return 0;
-- 
1.7.4

Re: [PATCHv3 1/2] perf tools: Fix raw sample reading

[David Ahern]

On 09/29/2011 09:05 AM, Jiri Olsa wrote:
> Wrong pointer is being passed for raw data sanity checking,
> when parsing sample event.
> 
> This ends up with invalid event and perf record being stuck in
> __perf_session__process_events function during processing
> build IDs (process_buildids function).
> 
> Following command hangs up in my setup:
> 	./perf record -e raw_syscalls:sys_enter ls
> 
> The fix is to use proper pointer to the raw data instead
> of the 'u' union.
> 
> CC: David Ahern <dsahern@gmail.com>
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> ---
>  tools/perf/util/evsel.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index c5748c5..e389815 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -449,6 +449,8 @@ int perf_event__parse_sample(const union perf_event *event, u64 type,
>  	}
>  
>  	if (type & PERF_SAMPLE_RAW) {
> +		const u64 *pdata;
> +
>  		u.val64 = *array;
>  		if (WARN_ONCE(swapped,
>  			      "Endianness of raw data not corrected!\n")) {
> @@ -462,11 +464,12 @@ int perf_event__parse_sample(const union perf_event *event, u64 type,
>  			return -EFAULT;
>  
>  		data->raw_size = u.val32[0];
> +		pdata = (void *) array + sizeof(u32);
>  
> -		if (sample_overlap(event, &u.val32[1], data->raw_size))
> +		if (sample_overlap(event, pdata, data->raw_size))
>  			return -EFAULT;
>  
> -		data->raw_data = &u.val32[1];
> +		data->raw_data = (void *) pdata;
>  	}
>  
>  	return 0;

Oops. Thanks for fixing.

Reviewed-by: David Ahern <dsahern@gmail.com>

[PATCHv3 2/2] perf tools: Fix tracing info recording

[Jiri Olsa]

Fixing the way the tracing information is stored within record command.
The current implementation is causing issues for pipe output.

Following commands fail currently:
	perf script syscall-counts ls
	perf record -e syscalls:sys_exit_read ls | ./perf report -i -

The tracing information is part of the perf data file. It contains
several files from within the tracing debugfs and procs directories.

Beside some static header files, for each tracing event the format
file is added. The /proc/kallsyms file is also added.

The tracing data are stored with preceeding size. This is causing some
dificulties for pipe output, since there's no way to tell debugfs/proc
file size before reading it. So, for pipe output, all the debugfs files
were read twice. Once to get the overall size and once to store the
content itself. This can cause problem in case any of these file
changed, within the storage time.

To fix this behaviour and ensure the integrity of the tracing data, we:
    - read debugfs/proc file into the temp file
    - get temp file size and dump it to the pipe
    - dump the temp file contents to the pipe

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/header.c           |   50 +++++++++++++++---
 tools/perf/util/trace-event-info.c |  102 ++++++++++++++++++++++++++---------
 tools/perf/util/trace-event.h      |   11 +++-
 3 files changed, 127 insertions(+), 36 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index b6c1ad1..477c73b 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -382,18 +382,33 @@ static int perf_header__adds_write(struct perf_header *header,
 
 	sec_size = sizeof(*feat_sec) * nr_sections;
 
+	/*
+	 * We reserve header space for each section, and
+	 * update/store it later once we know the section size.
+	 */
 	sec_start = header->data_offset + header->data_size;
 	lseek(fd, sec_start + sec_size, SEEK_SET);
 
 	if (perf_header__has_feat(header, HEADER_TRACE_INFO)) {
 		struct perf_file_section *trace_sec;
+		struct tracing_data *tdata;
 
 		trace_sec = &feat_sec[idx++];
-
-		/* Write trace info */
 		trace_sec->offset = lseek(fd, 0, SEEK_CUR);
-		read_tracing_data(fd, &evlist->entries);
-		trace_sec->size = lseek(fd, 0, SEEK_CUR) - trace_sec->offset;
+
+		/*
+		 * We work over the real file, so we can write data
+		 * directly, no temp file is needed.
+		 */
+		tdata = tracing_data_get(&evlist->entries, fd, false);
+		if (!tdata)
+			goto out_free;
+
+		/*
+		 * Update the section header information.
+		 */
+		trace_sec->size = tdata->size;
+		tracing_data_put(tdata);
 	}
 
 	if (perf_header__has_feat(header, HEADER_BUILD_ID)) {
@@ -1100,15 +1115,29 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
 				   struct perf_session *session __unused)
 {
 	union perf_event ev;
+	struct tracing_data *tdata;
 	ssize_t size = 0, aligned_size = 0, padding;
 	int err __used = 0;
 
+	/*
+	 * We are going to store the size of the data followed
+	 * by the data contents. Since the fd descriptor is a pipe,
+	 * we cannot seek back to store the size of the data once
+	 * we know it. Instead we:
+	 *
+	 * - write the tracing data to the temp file
+	 * - get/write the data size to pipe
+	 * - write the tracing data from the temp file
+	 *   to the pipe
+	 */
+	tdata = tracing_data_get(&evlist->entries, fd, true);
+	if (!tdata)
+		return -1;
+
 	memset(&ev, 0, sizeof(ev));
 
 	ev.tracing_data.header.type = PERF_RECORD_HEADER_TRACING_DATA;
-	size = read_tracing_data_size(fd, &evlist->entries);
-	if (size <= 0)
-		return size;
+	size = tdata->size;
 	aligned_size = ALIGN(size, sizeof(u64));
 	padding = aligned_size - size;
 	ev.tracing_data.header.size = sizeof(ev.tracing_data);
@@ -1116,7 +1145,12 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
 
 	process(&ev, NULL, session);
 
-	err = read_tracing_data(fd, &evlist->entries);
+	/*
+	 * The put function will copy all the tracing data
+	 * stored in temp file to the pipe.
+	 */
+	tracing_data_put(tdata);
+
 	write_padded(fd, NULL, 0, padding);
 
 	return aligned_size;
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 3403f81..c9aa4c9 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -196,7 +196,8 @@ static void record_file(const char *file, size_t hdr_sz)
 		die("Can't read '%s'", file);
 
 	/* put in zeros for file size, then fill true size later */
-	write_or_die(&size, hdr_sz);
+	if (hdr_sz)
+		write_or_die(&size, hdr_sz);
 
 	do {
 		r = read(fd, buf, BUFSIZ);
@@ -212,7 +213,7 @@ static void record_file(const char *file, size_t hdr_sz)
 	if (bigendian())
 		sizep += sizeof(u64) - hdr_sz;
 
-	if (pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
+	if (hdr_sz && pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
 		die("writing to %s", output_file);
 }
 
@@ -428,6 +429,19 @@ get_tracepoints_path(struct list_head *pattrs)
 	return nr_tracepoints > 0 ? path.next : NULL;
 }
 
+static void
+put_tracepoints_path(struct tracepoint_path *tps)
+{
+	while (tps) {
+		struct tracepoint_path *t = tps;
+
+		tps = tps->next;
+		free(t->name);
+		free(t->system);
+		free(t);
+	}
+}
+
 bool have_tracepoints(struct list_head *pattrs)
 {
 	struct perf_evsel *pos;
@@ -439,19 +453,11 @@ bool have_tracepoints(struct list_head *pattrs)
 	return false;
 }
 
-int read_tracing_data(int fd, struct list_head *pattrs)
+static void tracing_data_header(void)
 {
-	char buf[BUFSIZ];
-	struct tracepoint_path *tps = get_tracepoints_path(pattrs);
-
-	/*
-	 * What? No tracepoints? No sense writing anything here, bail out.
-	 */
-	if (tps == NULL)
-		return -1;
-
-	output_fd = fd;
+	char buf[50];
 
+	/* just guessing this is someone's birthday.. ;) */
 	buf[0] = 23;
 	buf[1] = 8;
 	buf[2] = 68;
@@ -476,28 +482,72 @@ int read_tracing_data(int fd, struct list_head *pattrs)
 	/* save page_size */
 	page_size = sysconf(_SC_PAGESIZE);
 	write_or_die(&page_size, 4);
+}
+
+struct tracing_data *tracing_data_get(struct list_head *pattrs,
+				      int fd, bool temp)
+{
+	struct tracepoint_path *tps;
+	struct tracing_data *tdata;
+
+	output_fd = fd;
+
+	tps = get_tracepoints_path(pattrs);
+	if (!tps)
+		return NULL;
+
+	tdata = malloc_or_die(sizeof(*tdata));
+	tdata->temp = temp;
+	tdata->size = 0;
+
+	if (temp) {
+		int temp_fd;
+
+		snprintf(tdata->temp_file, sizeof(tdata->temp_file),
+			 "/tmp/perf-XXXXXX");
+		if (!mkstemp(tdata->temp_file))
+			die("Can't make temp file");
+
+		temp_fd = open(tdata->temp_file, O_RDWR);
+		if (temp_fd < 0)
+			die("Can't read '%s'", tdata->temp_file);
+
+		/*
+		 * Set the temp file the default output, so all the
+		 * tracing data are stored into it.
+		 */
+		output_fd = temp_fd;
+	} else
+		tdata->size = -lseek(output_fd, 0, SEEK_CUR);
 
+	tracing_data_header();
 	read_header_files();
 	read_ftrace_files(tps);
 	read_event_files(tps);
 	read_proc_kallsyms();
 	read_ftrace_printk();
 
-	return 0;
-}
+	tdata->size += lseek(output_fd, 0, SEEK_CUR);
 
-ssize_t read_tracing_data_size(int fd, struct list_head *pattrs)
-{
-	ssize_t size;
-	int err = 0;
+	/*
+	 * All tracing data are stored by now, we can restore
+	 * the default output file in case we used temp file.
+	 */
+	if (temp) {
+		close(output_fd);
+		output_fd = fd;
+	}
 
-	calc_data_size = 1;
-	err = read_tracing_data(fd, pattrs);
-	size = calc_data_size - 1;
-	calc_data_size = 0;
+	put_tracepoints_path(tps);
+	return tdata;
+}
 
-	if (err < 0)
-		return err;
+void tracing_data_put(struct tracing_data *tdata)
+{
+	if (tdata->temp) {
+		record_file(tdata->temp_file, 0);
+		unlink(tdata->temp_file);
+	}
 
-	return size;
+	free(tdata);
 }
diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
index f674dda..92274b9 100644
--- a/tools/perf/util/trace-event.h
+++ b/tools/perf/util/trace-event.h
@@ -262,8 +262,15 @@ raw_field_value(struct event *event, const char *name, void *data);
 void *raw_field_ptr(struct event *event, const char *name, void *data);
 unsigned long long eval_flag(const char *flag);
 
-int read_tracing_data(int fd, struct list_head *pattrs);
-ssize_t read_tracing_data_size(int fd, struct list_head *pattrs);
+struct tracing_data {
+	ssize_t size;
+	bool temp;
+	char temp_file[50];
+};
+
+struct tracing_data *tracing_data_get(struct list_head *pattrs,
+				      int fd, bool temp);
+void tracing_data_put(struct tracing_data *tdata);
 
 /* taken from kernel/trace/trace.h */
 enum trace_flag_type {
-- 
1.7.4

Re: [PATCHv3 2/2] perf tools: Fix tracing info recording

[Jiri Olsa]

hi,
any feedback on this?

I cut off changes colliding with Steven's change,
now it just fixies the pipe/file issue..

thanks,
jirka


On Thu, Sep 29, 2011 at 05:05:09PM +0200, Jiri Olsa wrote:
> Fixing the way the tracing information is stored within record command.
> The current implementation is causing issues for pipe output.
> 
> Following commands fail currently:
> 	perf script syscall-counts ls
> 	perf record -e syscalls:sys_exit_read ls | ./perf report -i -
> 
> The tracing information is part of the perf data file. It contains
> several files from within the tracing debugfs and procs directories.
> 
> Beside some static header files, for each tracing event the format
> file is added. The /proc/kallsyms file is also added.
> 
> The tracing data are stored with preceeding size. This is causing some
> dificulties for pipe output, since there's no way to tell debugfs/proc
> file size before reading it. So, for pipe output, all the debugfs files
> were read twice. Once to get the overall size and once to store the
> content itself. This can cause problem in case any of these file
> changed, within the storage time.
> 
> To fix this behaviour and ensure the integrity of the tracing data, we:
>     - read debugfs/proc file into the temp file
>     - get temp file size and dump it to the pipe
>     - dump the temp file contents to the pipe
> 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> ---
>  tools/perf/util/header.c           |   50 +++++++++++++++---
>  tools/perf/util/trace-event-info.c |  102 ++++++++++++++++++++++++++---------
>  tools/perf/util/trace-event.h      |   11 +++-
>  3 files changed, 127 insertions(+), 36 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index b6c1ad1..477c73b 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -382,18 +382,33 @@ static int perf_header__adds_write(struct perf_header *header,
>  
>  	sec_size = sizeof(*feat_sec) * nr_sections;
>  
> +	/*
> +	 * We reserve header space for each section, and
> +	 * update/store it later once we know the section size.
> +	 */
>  	sec_start = header->data_offset + header->data_size;
>  	lseek(fd, sec_start + sec_size, SEEK_SET);
>  
>  	if (perf_header__has_feat(header, HEADER_TRACE_INFO)) {
>  		struct perf_file_section *trace_sec;
> +		struct tracing_data *tdata;
>  
>  		trace_sec = &feat_sec[idx++];
> -
> -		/* Write trace info */
>  		trace_sec->offset = lseek(fd, 0, SEEK_CUR);
> -		read_tracing_data(fd, &evlist->entries);
> -		trace_sec->size = lseek(fd, 0, SEEK_CUR) - trace_sec->offset;
> +
> +		/*
> +		 * We work over the real file, so we can write data
> +		 * directly, no temp file is needed.
> +		 */
> +		tdata = tracing_data_get(&evlist->entries, fd, false);
> +		if (!tdata)
> +			goto out_free;
> +
> +		/*
> +		 * Update the section header information.
> +		 */
> +		trace_sec->size = tdata->size;
> +		tracing_data_put(tdata);
>  	}
>  
>  	if (perf_header__has_feat(header, HEADER_BUILD_ID)) {
> @@ -1100,15 +1115,29 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
>  				   struct perf_session *session __unused)
>  {
>  	union perf_event ev;
> +	struct tracing_data *tdata;
>  	ssize_t size = 0, aligned_size = 0, padding;
>  	int err __used = 0;
>  
> +	/*
> +	 * We are going to store the size of the data followed
> +	 * by the data contents. Since the fd descriptor is a pipe,
> +	 * we cannot seek back to store the size of the data once
> +	 * we know it. Instead we:
> +	 *
> +	 * - write the tracing data to the temp file
> +	 * - get/write the data size to pipe
> +	 * - write the tracing data from the temp file
> +	 *   to the pipe
> +	 */
> +	tdata = tracing_data_get(&evlist->entries, fd, true);
> +	if (!tdata)
> +		return -1;
> +
>  	memset(&ev, 0, sizeof(ev));
>  
>  	ev.tracing_data.header.type = PERF_RECORD_HEADER_TRACING_DATA;
> -	size = read_tracing_data_size(fd, &evlist->entries);
> -	if (size <= 0)
> -		return size;
> +	size = tdata->size;
>  	aligned_size = ALIGN(size, sizeof(u64));
>  	padding = aligned_size - size;
>  	ev.tracing_data.header.size = sizeof(ev.tracing_data);
> @@ -1116,7 +1145,12 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
>  
>  	process(&ev, NULL, session);
>  
> -	err = read_tracing_data(fd, &evlist->entries);
> +	/*
> +	 * The put function will copy all the tracing data
> +	 * stored in temp file to the pipe.
> +	 */
> +	tracing_data_put(tdata);
> +
>  	write_padded(fd, NULL, 0, padding);
>  
>  	return aligned_size;
> diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
> index 3403f81..c9aa4c9 100644
> --- a/tools/perf/util/trace-event-info.c
> +++ b/tools/perf/util/trace-event-info.c
> @@ -196,7 +196,8 @@ static void record_file(const char *file, size_t hdr_sz)
>  		die("Can't read '%s'", file);
>  
>  	/* put in zeros for file size, then fill true size later */
> -	write_or_die(&size, hdr_sz);
> +	if (hdr_sz)
> +		write_or_die(&size, hdr_sz);
>  
>  	do {
>  		r = read(fd, buf, BUFSIZ);
> @@ -212,7 +213,7 @@ static void record_file(const char *file, size_t hdr_sz)
>  	if (bigendian())
>  		sizep += sizeof(u64) - hdr_sz;
>  
> -	if (pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
> +	if (hdr_sz && pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
>  		die("writing to %s", output_file);
>  }
>  
> @@ -428,6 +429,19 @@ get_tracepoints_path(struct list_head *pattrs)
>  	return nr_tracepoints > 0 ? path.next : NULL;
>  }
>  
> +static void
> +put_tracepoints_path(struct tracepoint_path *tps)
> +{
> +	while (tps) {
> +		struct tracepoint_path *t = tps;
> +
> +		tps = tps->next;
> +		free(t->name);
> +		free(t->system);
> +		free(t);
> +	}
> +}
> +
>  bool have_tracepoints(struct list_head *pattrs)
>  {
>  	struct perf_evsel *pos;
> @@ -439,19 +453,11 @@ bool have_tracepoints(struct list_head *pattrs)
>  	return false;
>  }
>  
> -int read_tracing_data(int fd, struct list_head *pattrs)
> +static void tracing_data_header(void)
>  {
> -	char buf[BUFSIZ];
> -	struct tracepoint_path *tps = get_tracepoints_path(pattrs);
> -
> -	/*
> -	 * What? No tracepoints? No sense writing anything here, bail out.
> -	 */
> -	if (tps == NULL)
> -		return -1;
> -
> -	output_fd = fd;
> +	char buf[50];
>  
> +	/* just guessing this is someone's birthday.. ;) */
>  	buf[0] = 23;
>  	buf[1] = 8;
>  	buf[2] = 68;
> @@ -476,28 +482,72 @@ int read_tracing_data(int fd, struct list_head *pattrs)
>  	/* save page_size */
>  	page_size = sysconf(_SC_PAGESIZE);
>  	write_or_die(&page_size, 4);
> +}
> +
> +struct tracing_data *tracing_data_get(struct list_head *pattrs,
> +				      int fd, bool temp)
> +{
> +	struct tracepoint_path *tps;
> +	struct tracing_data *tdata;
> +
> +	output_fd = fd;
> +
> +	tps = get_tracepoints_path(pattrs);
> +	if (!tps)
> +		return NULL;
> +
> +	tdata = malloc_or_die(sizeof(*tdata));
> +	tdata->temp = temp;
> +	tdata->size = 0;
> +
> +	if (temp) {
> +		int temp_fd;
> +
> +		snprintf(tdata->temp_file, sizeof(tdata->temp_file),
> +			 "/tmp/perf-XXXXXX");
> +		if (!mkstemp(tdata->temp_file))
> +			die("Can't make temp file");
> +
> +		temp_fd = open(tdata->temp_file, O_RDWR);
> +		if (temp_fd < 0)
> +			die("Can't read '%s'", tdata->temp_file);
> +
> +		/*
> +		 * Set the temp file the default output, so all the
> +		 * tracing data are stored into it.
> +		 */
> +		output_fd = temp_fd;
> +	} else
> +		tdata->size = -lseek(output_fd, 0, SEEK_CUR);
>  
> +	tracing_data_header();
>  	read_header_files();
>  	read_ftrace_files(tps);
>  	read_event_files(tps);
>  	read_proc_kallsyms();
>  	read_ftrace_printk();
>  
> -	return 0;
> -}
> +	tdata->size += lseek(output_fd, 0, SEEK_CUR);
>  
> -ssize_t read_tracing_data_size(int fd, struct list_head *pattrs)
> -{
> -	ssize_t size;
> -	int err = 0;
> +	/*
> +	 * All tracing data are stored by now, we can restore
> +	 * the default output file in case we used temp file.
> +	 */
> +	if (temp) {
> +		close(output_fd);
> +		output_fd = fd;
> +	}
>  
> -	calc_data_size = 1;
> -	err = read_tracing_data(fd, pattrs);
> -	size = calc_data_size - 1;
> -	calc_data_size = 0;
> +	put_tracepoints_path(tps);
> +	return tdata;
> +}
>  
> -	if (err < 0)
> -		return err;
> +void tracing_data_put(struct tracing_data *tdata)
> +{
> +	if (tdata->temp) {
> +		record_file(tdata->temp_file, 0);
> +		unlink(tdata->temp_file);
> +	}
>  
> -	return size;
> +	free(tdata);
>  }
> diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
> index f674dda..92274b9 100644
> --- a/tools/perf/util/trace-event.h
> +++ b/tools/perf/util/trace-event.h
> @@ -262,8 +262,15 @@ raw_field_value(struct event *event, const char *name, void *data);
>  void *raw_field_ptr(struct event *event, const char *name, void *data);
>  unsigned long long eval_flag(const char *flag);
>  
> -int read_tracing_data(int fd, struct list_head *pattrs);
> -ssize_t read_tracing_data_size(int fd, struct list_head *pattrs);
> +struct tracing_data {
> +	ssize_t size;
> +	bool temp;
> +	char temp_file[50];
> +};
> +
> +struct tracing_data *tracing_data_get(struct list_head *pattrs,
> +				      int fd, bool temp);
> +void tracing_data_put(struct tracing_data *tdata);
>  
>  /* taken from kernel/trace/trace.h */
>  enum trace_flag_type {
> -- 
> 1.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCHv4] perf tools: Fix tracing info recording

[Jiri Olsa]

hi,
sending version updated to the latest tip tree code.

wbr,
jirka

---
Fixing the way the tracing information is stored within record command.
The current implementation is causing issues for pipe output.

Following commands fail currently:
	perf script syscall-counts ls
	perf record -e syscalls:sys_exit_read ls | ./perf report -i -

The tracing information is part of the perf data file. It contains
several files from within the tracing debugfs and procs directories.

Beside some static header files, for each tracing event the format
file is added. The /proc/kallsyms file is also added.

The tracing data are stored with preceeding size. This is causing some
dificulties for pipe output, since there's no way to tell debugfs/proc
file size before reading it. So, for pipe output, all the debugfs files
were read twice. Once to get the overall size and once to store the
content itself. This can cause problem in case any of these file
changed, within the storage time.

To fix this behaviour and ensure the integrity of the tracing data, we:
    - read debugfs/proc file into the temp file
    - get temp file size and dump it to the pipe
    - dump the temp file contents to the pipe

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/header.c           |   27 +++++++-
 tools/perf/util/trace-event-info.c |  112 ++++++++++++++++++++++++++++--------
 tools/perf/util/trace-event.h      |   13 ++++-
 3 files changed, 123 insertions(+), 29 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 6a9c041..76c0b2c 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2218,15 +2218,29 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
 				   struct perf_session *session __unused)
 {
 	union perf_event ev;
+	struct tracing_data *tdata;
 	ssize_t size = 0, aligned_size = 0, padding;
 	int err __used = 0;
 
+	/*
+	 * We are going to store the size of the data followed
+	 * by the data contents. Since the fd descriptor is a pipe,
+	 * we cannot seek back to store the size of the data once
+	 * we know it. Instead we:
+	 *
+	 * - write the tracing data to the temp file
+	 * - get/write the data size to pipe
+	 * - write the tracing data from the temp file
+	 *   to the pipe
+	 */
+	tdata = tracing_data_get(&evlist->entries, fd, true);
+	if (!tdata)
+		return -1;
+
 	memset(&ev, 0, sizeof(ev));
 
 	ev.tracing_data.header.type = PERF_RECORD_HEADER_TRACING_DATA;
-	size = read_tracing_data_size(fd, &evlist->entries);
-	if (size <= 0)
-		return size;
+	size = tdata->size;
 	aligned_size = ALIGN(size, sizeof(u64));
 	padding = aligned_size - size;
 	ev.tracing_data.header.size = sizeof(ev.tracing_data);
@@ -2234,7 +2248,12 @@ int perf_event__synthesize_tracing_data(int fd, struct perf_evlist *evlist,
 
 	process(&ev, NULL, session);
 
-	err = read_tracing_data(fd, &evlist->entries);
+	/*
+	 * The put function will copy all the tracing data
+	 * stored in temp file to the pipe.
+	 */
+	tracing_data_put(tdata);
+
 	write_padded(fd, NULL, 0, padding);
 
 	return aligned_size;
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 3403f81..2d530cf 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -196,7 +196,8 @@ static void record_file(const char *file, size_t hdr_sz)
 		die("Can't read '%s'", file);
 
 	/* put in zeros for file size, then fill true size later */
-	write_or_die(&size, hdr_sz);
+	if (hdr_sz)
+		write_or_die(&size, hdr_sz);
 
 	do {
 		r = read(fd, buf, BUFSIZ);
@@ -212,7 +213,7 @@ static void record_file(const char *file, size_t hdr_sz)
 	if (bigendian())
 		sizep += sizeof(u64) - hdr_sz;
 
-	if (pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
+	if (hdr_sz && pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
 		die("writing to %s", output_file);
 }
 
@@ -428,6 +429,19 @@ get_tracepoints_path(struct list_head *pattrs)
 	return nr_tracepoints > 0 ? path.next : NULL;
 }
 
+static void
+put_tracepoints_path(struct tracepoint_path *tps)
+{
+	while (tps) {
+		struct tracepoint_path *t = tps;
+
+		tps = tps->next;
+		free(t->name);
+		free(t->system);
+		free(t);
+	}
+}
+
 bool have_tracepoints(struct list_head *pattrs)
 {
 	struct perf_evsel *pos;
@@ -439,19 +453,11 @@ bool have_tracepoints(struct list_head *pattrs)
 	return false;
 }
 
-int read_tracing_data(int fd, struct list_head *pattrs)
+static void tracing_data_header(void)
 {
-	char buf[BUFSIZ];
-	struct tracepoint_path *tps = get_tracepoints_path(pattrs);
-
-	/*
-	 * What? No tracepoints? No sense writing anything here, bail out.
-	 */
-	if (tps == NULL)
-		return -1;
-
-	output_fd = fd;
+	char buf[20];
 
+	/* just guessing this is someone's birthday.. ;) */
 	buf[0] = 23;
 	buf[1] = 8;
 	buf[2] = 68;
@@ -476,28 +482,86 @@ int read_tracing_data(int fd, struct list_head *pattrs)
 	/* save page_size */
 	page_size = sysconf(_SC_PAGESIZE);
 	write_or_die(&page_size, 4);
+}
+
+struct tracing_data *tracing_data_get(struct list_head *pattrs,
+				      int fd, bool temp)
+{
+	struct tracepoint_path *tps;
+	struct tracing_data *tdata;
+
+	output_fd = fd;
+
+	tps = get_tracepoints_path(pattrs);
+	if (!tps)
+		return NULL;
 
+	tdata = malloc_or_die(sizeof(*tdata));
+	tdata->temp = temp;
+	tdata->size = 0;
+
+	if (temp) {
+		int temp_fd;
+
+		snprintf(tdata->temp_file, sizeof(tdata->temp_file),
+			 "/tmp/perf-XXXXXX");
+		if (!mkstemp(tdata->temp_file))
+			die("Can't make temp file");
+
+		temp_fd = open(tdata->temp_file, O_RDWR);
+		if (temp_fd < 0)
+			die("Can't read '%s'", tdata->temp_file);
+
+		/*
+		 * Set the temp file the default output, so all the
+		 * tracing data are stored into it.
+		 */
+		output_fd = temp_fd;
+	}
+
+	tracing_data_header();
 	read_header_files();
 	read_ftrace_files(tps);
 	read_event_files(tps);
 	read_proc_kallsyms();
 	read_ftrace_printk();
 
-	return 0;
+	/*
+	 * All tracing data are stored by now, we can restore
+	 * the default output file in case we used temp file.
+	 */
+	if (temp) {
+		tdata->size = lseek(output_fd, 0, SEEK_CUR);
+		close(output_fd);
+		output_fd = fd;
+	}
+
+	put_tracepoints_path(tps);
+	return tdata;
 }
 
-ssize_t read_tracing_data_size(int fd, struct list_head *pattrs)
+void tracing_data_put(struct tracing_data *tdata)
 {
-	ssize_t size;
-	int err = 0;
+	if (tdata->temp) {
+		record_file(tdata->temp_file, 0);
+		unlink(tdata->temp_file);
+	}
 
-	calc_data_size = 1;
-	err = read_tracing_data(fd, pattrs);
-	size = calc_data_size - 1;
-	calc_data_size = 0;
+	free(tdata);
+}
 
-	if (err < 0)
-		return err;
+int read_tracing_data(int fd, struct list_head *pattrs)
+{
+	struct tracing_data *tdata;
 
-	return size;
+	/*
+	 * We work over the real file, so we can write data
+	 * directly, no temp file is needed.
+	 */
+	tdata = tracing_data_get(pattrs, fd, false);
+	if (!tdata)
+		return -ENOMEM;
+
+	tracing_data_put(tdata);
+	return 0;
 }
diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
index f674dda..a841008 100644
--- a/tools/perf/util/trace-event.h
+++ b/tools/perf/util/trace-event.h
@@ -263,7 +263,18 @@ void *raw_field_ptr(struct event *event, const char *name, void *data);
 unsigned long long eval_flag(const char *flag);
 
 int read_tracing_data(int fd, struct list_head *pattrs);
-ssize_t read_tracing_data_size(int fd, struct list_head *pattrs);
+
+struct tracing_data {
+	/* size is only valid if temp is 'true' */
+	ssize_t size;
+	bool temp;
+	char temp_file[50];
+};
+
+struct tracing_data *tracing_data_get(struct list_head *pattrs,
+				      int fd, bool temp);
+void tracing_data_put(struct tracing_data *tdata);
+
 
 /* taken from kernel/trace/trace.h */
 enum trace_flag_type {
-- 
1.7.4

Re: [PATCHv4] perf tools: Fix tracing info recording

[Arnaldo Carvalho de Melo]

Em Thu, Oct 20, 2011 at 03:59:43PM +0200, Jiri Olsa escreveu:
> hi,
> sending version updated to the latest tip tree code.

Thanks for refreshing it, applied. I'll push it together with some other
changes after Ingo pulls what I have in perf/core.

- Arnaldo