[PATCH] [RFC] perf: robustify proc and debugfs file recording

[PATCH] [RFC] perf: robustify proc and debugfs file recording

[Sonny Rao]

While attempting to create a timechart of boot up I found
perf didn't tolerate modules being loaded/unloaded.  This patch
fixes this by reading the file once and then writing the size
read at the correct point in the file.  It also simplifies the
code somewhat.

Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
---
 tools/perf/util/trace-event-info.c |  114 +++++++++---------------------------
 1 files changed, 27 insertions(+), 87 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 35729f4..5a7e562 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -183,106 +183,60 @@ int bigendian(void)
 	return *ptr == 0x01020304;
 }
 
-static unsigned long long copy_file_fd(int fd)
+/* 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];
-	int r;
-
-	do {
-		r = read(fd, buf, BUFSIZ);
-		if (r > 0) {
-			size += r;
-			write_or_die(buf, r);
-		}
-	} while (r > 0);
-
-	return size;
-}
-
-static unsigned long long copy_file(const char *file)
-{
-	unsigned long long size = 0;
-	int fd;
+	char buf[BUFSIZ], *sizep;
+	off_t hdr_pos = lseek(output_fd, 0, SEEK_CUR);
+	int r, fd;
 
 	fd = open(file, O_RDONLY);
 	if (fd < 0)
 		die("Can't read '%s'", file);
-	size = copy_file_fd(fd);
-	close(fd);
 
-	return size;
-}
-
-static unsigned long get_size_fd(int fd)
-{
-	unsigned long long size = 0;
-	char buf[BUFSIZ];
-	int r;
+	/* put in zeros for file size, then fill true size later */
+	write_or_die(&size, hdr_sz);
 
 	do {
 		r = read(fd, buf, BUFSIZ);
-		if (r > 0)
+		if (r > 0) {
 			size += r;
+			write_or_die(buf, r);
+		}
 	} while (r > 0);
-
-	lseek(fd, 0, SEEK_SET);
-
-	return size;
-}
-
-static unsigned long get_size(const char *file)
-{
-	unsigned long long size = 0;
-	int fd;
-
-	fd = open(file, O_RDONLY);
-	if (fd < 0)
-		die("Can't read '%s'", file);
-	size = get_size_fd(fd);
 	close(fd);
 
-	return size;
+	/* ugh, handle big-endian hdr_size == 4 */
+	sizep = (char*)&size;
+	if (bigendian())
+		sizep += sizeof(u64) - hdr_sz;
+
+	if (pwrite(output_fd, &size, hdr_sz, hdr_pos) < 0)
+		die("writing to %s", output_file);
 }
 
 static void read_header_files(void)
 {
 	unsigned long long size, check_size;
 	char *path;
-	int fd;
+	struct stat st;
 
 	path = get_tracing_file("events/header_page");
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
+	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
-	/* unfortunately, you can not stat debugfs files for size */
-	size = get_size_fd(fd);
-
 	write_or_die("header_page", 12);
-	write_or_die(&size, 8);
-	check_size = copy_file_fd(fd);
-	close(fd);
-
-	if (size != check_size)
-		die("wrong size for '%s' size=%lld read=%lld",
-		    path, size, check_size);
+	record_file(path, 8);
 	put_tracing_file(path);
 
 	path = get_tracing_file("events/header_event");
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
+	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
-	size = get_size_fd(fd);
-
 	write_or_die("header_event", 13);
-	write_or_die(&size, 8);
-	check_size = copy_file_fd(fd);
-	if (size != check_size)
-		die("wrong size for '%s'", path);
+	record_file(path, 8);
 	put_tracing_file(path);
-	close(fd);
 }
 
 static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
@@ -338,14 +292,8 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)
 		sprintf(format, "%s/%s/format", sys, dent->d_name);
 		ret = stat(format, &st);
 
-		if (ret >= 0) {
-			/* unfortunately, you can not stat debugfs files for size */
-			size = get_size(format);
-			write_or_die(&size, 8);
-			check_size = copy_file(format);
-			if (size != check_size)
-				die("error in size of file '%s'", format);
-		}
+		if (ret >= 0)
+			record_file(format, 8);
 
 		free(format);
 	}
@@ -438,12 +386,7 @@ static void read_proc_kallsyms(void)
 		write_or_die(&size, 4);
 		return;
 	}
-	size = get_size(path);
-	write_or_die(&size, 4);
-	check_size = copy_file(path);
-	if (size != check_size)
-		die("error in size of file '%s'", path);
-
+	record_file(path, 4);
 }
 
 static void read_ftrace_printk(void)
@@ -461,11 +404,8 @@ static void read_ftrace_printk(void)
 		write_or_die(&size, 4);
 		goto out;
 	}
-	size = get_size(path);
-	write_or_die(&size, 4);
-	check_size = copy_file(path);
-	if (size != check_size)
-		die("error in size of file '%s'", path);
+	record_file(path, 4);
+
 out:
 	put_tracing_file(path);
 }
-- 
1.7.3.1

Re: [PATCH] [RFC] perf: robustify proc and debugfs file recording

[Sonny Rao]

On Tue, Jul 12, 2011 at 2:15 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
> While attempting to create a timechart of boot up I found
> perf didn't tolerate modules being loaded/unloaded.  This patch
> fixes this by reading the file once and then writing the size
> read at the correct point in the file.  It also simplifies the
> code somewhat.
>
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> ---

Anton or Paul, I cc'ed you hoping you could give this a try on a
big-endian box.  There's some IMO less than pleasant code to
make sure it handles big-endian correctly, that I wasn't able to test.

Thanks
Sonny

Re: [PATCH] [RFC] perf: robustify proc and debugfs file recording

[Steven Rostedt]

On Tue, 2011-07-12 at 14:15 -0700, Sonny Rao wrote:
> While attempting to create a timechart of boot up I found
> perf didn't tolerate modules being loaded/unloaded.  This patch
> fixes this by reading the file once and then writing the size
> read at the correct point in the file.  It also simplifies the
> code somewhat.
> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>

>  static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
> @@ -338,14 +292,8 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)
>  		sprintf(format, "%s/%s/format", sys, dent->d_name);
>  		ret = stat(format, &st);
>  
> -		if (ret >= 0) {
> -			/* unfortunately, you can not stat debugfs files for size */
> -			size = get_size(format);
> -			write_or_die(&size, 8);
> -			check_size = copy_file(format);
> -			if (size != check_size)
> -				die("error in size of file '%s'", format);

You mean to tell me that you are hitting a race between the get_size()
and check_size()?  This is a very quick action, and this only happens at
start up. Are you starting up perf and loading modules at the same time?

-- Steve

> -		}
> +		if (ret >= 0)
> +			record_file(format, 8);
>  
>  		free(format);

Re: [PATCH] [RFC] perf: robustify proc and debugfs file recording

[Sonny Rao]

On Tue, Jul 12, 2011 at 3:56 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 2011-07-12 at 14:15 -0700, Sonny Rao wrote:
>> While attempting to create a timechart of boot up I found
>> perf didn't tolerate modules being loaded/unloaded.  This patch
>> fixes this by reading the file once and then writing the size
>> read at the correct point in the file.  It also simplifies the
>> code somewhat.
>>
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>
>>  static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
>> @@ -338,14 +292,8 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)
>>               sprintf(format, "%s/%s/format", sys, dent->d_name);
>>               ret = stat(format, &st);
>>
>> -             if (ret >= 0) {
>> -                     /* unfortunately, you can not stat debugfs files for size */
>> -                     size = get_size(format);
>> -                     write_or_die(&size, 8);
>> -                     check_size = copy_file(format);
>> -                     if (size != check_size)
>> -                             die("error in size of file '%s'", format);
>
> You mean to tell me that you are hitting a race between the get_size()
> and check_size()?  This is a very quick action, and this only happens at
> start up. Are you starting up perf and loading modules at the same time?

I sure am... the thing we're trying to do here is analyze our boot time
and udev is asynchronously loading modules in the background while
perf is starting up.

We've put in stupid hacks like sleep for a while, but they've proven to be
unreliable so I figured I'd take a stab at fixing the issue.  I also think this
change will make the overhead a bit lower since we only read these files
once.

Sonny

Re: [PATCH] [RFC] perf: robustify proc and debugfs file recording

[Steven Rostedt]

On Tue, 2011-07-12 at 16:01 -0700, Sonny Rao wrote:

> I sure am... the thing we're trying to do here is analyze our boot time
> and udev is asynchronously loading modules in the background while
> perf is starting up.

OK, I'll take a deeper look at these patches tomorrow.

-- Steve

Re: [PATCH] [RFC] perf: robustify proc and debugfs file recording

[Michael Neuling]

Sonny,


> While attempting to create a timechart of boot up I found
> perf didn't tolerate modules being loaded/unloaded.  This patch
> fixes this by reading the file once and then writing the size
> read at the correct point in the file.  It also simplifies the
> code somewhat.

I'm getting a bunch of unused variables warnings when I compile this.
Care to clean them up?

    CC util/trace-event-info.o
util/trace-event-info.c: In function ‘read_header_files’:
util/trace-event-info.c:221:27: error: unused variable ‘check_size’ [-Werror=unused-variable]
util/trace-event-info.c:221:21: error: unused variable ‘size’ [-Werror=unused-variable]
util/trace-event-info.c: In function ‘copy_event_system’:
util/trace-event-info.c:255:27: error: unused variable ‘check_size’ [-Werror=unused-variable]
util/trace-event-info.c:255:21: error: unused variable ‘size’ [-Werror=unused-variable]
util/trace-event-info.c: In function ‘read_proc_kallsyms’:
util/trace-event-info.c:377:21: error: unused variable ‘check_size’ [-Werror=unused-variable]
util/trace-event-info.c: In function ‘read_ftrace_printk’:
util/trace-event-info.c:394:21: error: unused variable ‘check_size’ [-Werror=unused-variable]
cc1: all warnings being treated as errors

Mikey

Re: [PATCH] [RFC] perf: robustify proc and debugfs file recording

[Michael Neuling]

In message <29926.1310553581@neuling.org> you wrote:
> Sonny,
> 
> 
> > While attempting to create a timechart of boot up I found
> > perf didn't tolerate modules being loaded/unloaded.  This patch
> > fixes this by reading the file once and then writing the size
> > read at the correct point in the file.  It also simplifies the
> > code somewhat.
> 
> I'm getting a bunch of unused variables warnings when I compile this.
> Care to clean them up?
> 
>     CC util/trace-event-info.o
> util/trace-event-info.c: In function =E2=80=98read_header_files=E2=80=99:
> util/trace-event-info.c:221:27: error: unused variable =E2=80=98check_size=
> =E2=80=99 [-Werror=3Dunused-variable]
> util/trace-event-info.c:221:21: error: unused variable =E2=80=98size=E2=80=
> =99 [-Werror=3Dunused-variable]
> util/trace-event-info.c: In function =E2=80=98copy_event_system=E2=80=99:
> util/trace-event-info.c:255:27: error: unused variable =E2=80=98check_size=
> =E2=80=99 [-Werror=3Dunused-variable]
> util/trace-event-info.c:255:21: error: unused variable =E2=80=98size=E2=80=
> =99 [-Werror=3Dunused-variable]
> util/trace-event-info.c: In function =E2=80=98read_proc_kallsyms=E2=80=99:
> util/trace-event-info.c:377:21: error: unused variable =E2=80=98check_size=
> =E2=80=99 [-Werror=3Dunused-variable]
> util/trace-event-info.c: In function =E2=80=98read_ftrace_printk=E2=80=99:
> util/trace-event-info.c:394:21: error: unused variable =E2=80=98check_size=
> =E2=80=99 [-Werror=3Dunused-variable]
> cc1: all warnings being treated as errors

Actually, here's an updated patch to fix these..

FYI perf record/annotate/report works fine on my powerpc box here with
this.  I don't have modules handy so I've not tested that aspect.

Mikey



From: Sonny Rao <sonnyrao@chromium.org>

While attempting to create a timechart of boot up I found
perf didn't tolerate modules being loaded/unloaded.  This patch
fixes this by reading the file once and then writing the size
read at the correct point in the file.  It also simplifies the
code somewhat.

Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 tools/perf/util/trace-event-info.c |  120 ++++++++-----------------------------
 1 file changed, 29 insertions(+), 91 deletions(-)

Index: linux-ozlabs/tools/perf/util/trace-event-info.c
===================================================================
--- linux-ozlabs.orig/tools/perf/util/trace-event-info.c
+++ linux-ozlabs/tools/perf/util/trace-event-info.c
@@ -183,106 +183,59 @@ int bigendian(void)
 	return *ptr == 0x01020304;
 }
 
-static unsigned long long copy_file_fd(int fd)
+/* 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];
-	int r;
-
-	do {
-		r = read(fd, buf, BUFSIZ);
-		if (r > 0) {
-			size += r;
-			write_or_die(buf, r);
-		}
-	} while (r > 0);
-
-	return size;
-}
-
-static unsigned long long copy_file(const char *file)
-{
-	unsigned long long size = 0;
-	int fd;
+	char buf[BUFSIZ], *sizep;
+	off_t hdr_pos = lseek(output_fd, 0, SEEK_CUR);
+	int r, fd;
 
 	fd = open(file, O_RDONLY);
 	if (fd < 0)
 		die("Can't read '%s'", file);
-	size = copy_file_fd(fd);
-	close(fd);
 
-	return size;
-}
-
-static unsigned long get_size_fd(int fd)
-{
-	unsigned long long size = 0;
-	char buf[BUFSIZ];
-	int r;
+	/* put in zeros for file size, then fill true size later */
+	write_or_die(&size, hdr_sz);
 
 	do {
 		r = read(fd, buf, BUFSIZ);
-		if (r > 0)
+		if (r > 0) {
 			size += r;
+			write_or_die(buf, r);
+		}
 	} while (r > 0);
-
-	lseek(fd, 0, SEEK_SET);
-
-	return size;
-}
-
-static unsigned long get_size(const char *file)
-{
-	unsigned long long size = 0;
-	int fd;
-
-	fd = open(file, O_RDONLY);
-	if (fd < 0)
-		die("Can't read '%s'", file);
-	size = get_size_fd(fd);
 	close(fd);
 
-	return size;
+	/* ugh, handle big-endian hdr_size == 4 */
+	sizep = (char*)&size;
+	if (bigendian())
+		sizep += sizeof(u64) - hdr_sz;
+
+	if (pwrite(output_fd, &size, hdr_sz, hdr_pos) < 0)
+		die("writing to %s", output_file);
 }
 
 static void read_header_files(void)
 {
-	unsigned long long size, check_size;
 	char *path;
-	int fd;
+	struct stat st;
 
 	path = get_tracing_file("events/header_page");
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
+	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
-	/* unfortunately, you can not stat debugfs files for size */
-	size = get_size_fd(fd);
-
 	write_or_die("header_page", 12);
-	write_or_die(&size, 8);
-	check_size = copy_file_fd(fd);
-	close(fd);
-
-	if (size != check_size)
-		die("wrong size for '%s' size=%lld read=%lld",
-		    path, size, check_size);
+	record_file(path, 8);
 	put_tracing_file(path);
 
 	path = get_tracing_file("events/header_event");
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
+	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
-	size = get_size_fd(fd);
-
 	write_or_die("header_event", 13);
-	write_or_die(&size, 8);
-	check_size = copy_file_fd(fd);
-	if (size != check_size)
-		die("wrong size for '%s'", path);
+	record_file(path, 8);
 	put_tracing_file(path);
-	close(fd);
 }
 
 static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
@@ -298,7 +251,6 @@ static bool name_in_tp_list(char *sys, s
 
 static void copy_event_system(const char *sys, struct tracepoint_path *tps)
 {
-	unsigned long long size, check_size;
 	struct dirent *dent;
 	struct stat st;
 	char *format;
@@ -338,14 +290,8 @@ static void copy_event_system(const char
 		sprintf(format, "%s/%s/format", sys, dent->d_name);
 		ret = stat(format, &st);
 
-		if (ret >= 0) {
-			/* unfortunately, you can not stat debugfs files for size */
-			size = get_size(format);
-			write_or_die(&size, 8);
-			check_size = copy_file(format);
-			if (size != check_size)
-				die("error in size of file '%s'", format);
-		}
+		if (ret >= 0)
+			record_file(format, 8);
 
 		free(format);
 	}
@@ -426,7 +372,7 @@ static void read_event_files(struct trac
 
 static void read_proc_kallsyms(void)
 {
-	unsigned int size, check_size;
+	unsigned int size;
 	const char *path = "/proc/kallsyms";
 	struct stat st;
 	int ret;
@@ -438,17 +384,12 @@ static void read_proc_kallsyms(void)
 		write_or_die(&size, 4);
 		return;
 	}
-	size = get_size(path);
-	write_or_die(&size, 4);
-	check_size = copy_file(path);
-	if (size != check_size)
-		die("error in size of file '%s'", path);
-
+	record_file(path, 4);
 }
 
 static void read_ftrace_printk(void)
 {
-	unsigned int size, check_size;
+	unsigned int size;
 	char *path;
 	struct stat st;
 	int ret;
@@ -461,11 +402,8 @@ static void read_ftrace_printk(void)
 		write_or_die(&size, 4);
 		goto out;
 	}
-	size = get_size(path);
-	write_or_die(&size, 4);
-	check_size = copy_file(path);
-	if (size != check_size)
-		die("error in size of file '%s'", path);
+	record_file(path, 4);
+
 out:
 	put_tracing_file(path);
 }

Re: [PATCH] [RFC] perf: robustify proc and debugfs file recording

[Sonny Rao]

On Wed, Jul 13, 2011 at 3:52 AM, Michael Neuling <mikey@neuling.org> wrote:
> In message <29926.1310553581@neuling.org> you wrote:
>> Sonny,
>>
>>
>> > While attempting to create a timechart of boot up I found
>> > perf didn't tolerate modules being loaded/unloaded.  This patch
>> > fixes this by reading the file once and then writing the size
>> > read at the correct point in the file.  It also simplifies the
>> > code somewhat.
>>
>> I'm getting a bunch of unused variables warnings when I compile this.
>> Care to clean them up?
>>
>>     CC util/trace-event-info.o
>> util/trace-event-info.c: In function =E2=80=98read_header_files=E2=80=99:
>> util/trace-event-info.c:221:27: error: unused variable =E2=80=98check_size=
>> =E2=80=99 [-Werror=3Dunused-variable]
>> util/trace-event-info.c:221:21: error: unused variable =E2=80=98size=E2=80=
>> =99 [-Werror=3Dunused-variable]
>> util/trace-event-info.c: In function =E2=80=98copy_event_system=E2=80=99:
>> util/trace-event-info.c:255:27: error: unused variable =E2=80=98check_size=
>> =E2=80=99 [-Werror=3Dunused-variable]
>> util/trace-event-info.c:255:21: error: unused variable =E2=80=98size=E2=80=
>> =99 [-Werror=3Dunused-variable]
>> util/trace-event-info.c: In function =E2=80=98read_proc_kallsyms=E2=80=99:
>> util/trace-event-info.c:377:21: error: unused variable =E2=80=98check_size=
>> =E2=80=99 [-Werror=3Dunused-variable]
>> util/trace-event-info.c: In function =E2=80=98read_ftrace_printk=E2=80=99:
>> util/trace-event-info.c:394:21: error: unused variable =E2=80=98check_size=
>> =E2=80=99 [-Werror=3Dunused-variable]
>> cc1: all warnings being treated as errors
>
> Actually, here's an updated patch to fix these..
>
> FYI perf record/annotate/report works fine on my powerpc box here with
> this.  I don't have modules handy so I've not tested that aspect.
>
> Mikey

Thanks for the testing and the cleanup!  You don't really need to test
the modules thing
I just wanted to make sure it worked period on big-endian.

Sonny

Re: [PATCH] [RFC] perf: robustify proc and debugfs file recording

[Steven Rostedt]

On Wed, 2011-07-13 at 20:52 +1000, Michael Neuling wrote:

> Actually, here's an updated patch to fix these..
> 
> FYI perf record/annotate/report works fine on my powerpc box here with
> this.  I don't have modules handy so I've not tested that aspect.

Sure it worked for you...


> -static unsigned long get_size(const char *file)
> -{
> -	unsigned long long size = 0;
> -	int fd;
> -
> -	fd = open(file, O_RDONLY);
> -	if (fd < 0)
> -		die("Can't read '%s'", file);
> -	size = get_size_fd(fd);
>  	close(fd);
>  
> -	return size;
> +	/* ugh, handle big-endian hdr_size == 4 */
> +	sizep = (char*)&size;
> +	if (bigendian())
> +		sizep += sizeof(u64) - hdr_sz;
> +
> +	if (pwrite(output_fd, &size, hdr_sz, hdr_pos) < 0)

s/&size/sizep/

-- Steve

> +		die("writing to %s", output_file);
>  }

Re: [PATCH] [RFC] perf: robustify proc and debugfs file recording

[Sonny Rao]

On Wed, Jul 13, 2011 at 1:38 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 2011-07-13 at 20:52 +1000, Michael Neuling wrote:
>
>> Actually, here's an updated patch to fix these..
>>
>> FYI perf record/annotate/report works fine on my powerpc box here with
>> this.  I don't have modules handy so I've not tested that aspect.
>
> Sure it worked for you...
>
>
>> -static unsigned long get_size(const char *file)
>> -{
>> -     unsigned long long size = 0;
>> -     int fd;
>> -
>> -     fd = open(file, O_RDONLY);
>> -     if (fd < 0)
>> -             die("Can't read '%s'", file);
>> -     size = get_size_fd(fd);
>>       close(fd);
>>
>> -     return size;
>> +     /* ugh, handle big-endian hdr_size == 4 */
>> +     sizep = (char*)&size;
>> +     if (bigendian())
>> +             sizep += sizeof(u64) - hdr_sz;
>> +
>> +     if (pwrite(output_fd, &size, hdr_sz, hdr_pos) < 0)
>
> s/&size/sizep/

Argh, I'm really messing this up... so yeah this shouldn't have worked
on big-endian without that fix..
Mikey, are you sure it worked?

Sonny

Re: [PATCH] [RFC] perf: robustify proc and debugfs file recording

[Sonny Rao]

On Wed, Jul 13, 2011 at 1:49 PM, Sonny Rao <sonnyrao@chromium.org> wrote:
> On Wed, Jul 13, 2011 at 1:38 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Wed, 2011-07-13 at 20:52 +1000, Michael Neuling wrote:
>>
>>> Actually, here's an updated patch to fix these..
>>>
>>> FYI perf record/annotate/report works fine on my powerpc box here with
>>> this.  I don't have modules handy so I've not tested that aspect.
>>
>> Sure it worked for you...
>>
>>
>>> -static unsigned long get_size(const char *file)
>>> -{
>>> -     unsigned long long size = 0;
>>> -     int fd;
>>> -
>>> -     fd = open(file, O_RDONLY);
>>> -     if (fd < 0)
>>> -             die("Can't read '%s'", file);
>>> -     size = get_size_fd(fd);
>>>       close(fd);
>>>
>>> -     return size;
>>> +     /* ugh, handle big-endian hdr_size == 4 */
>>> +     sizep = (char*)&size;
>>> +     if (bigendian())
>>> +             sizep += sizeof(u64) - hdr_sz;
>>> +
>>> +     if (pwrite(output_fd, &size, hdr_sz, hdr_pos) < 0)
>>
>> s/&size/sizep/
>
> Argh, I'm really messing this up... so yeah this shouldn't have worked
> on big-endian without that fix..
> Mikey, are you sure it worked?

Actually, I think you need to use tracepoints or this code won't be invoked.
So, please try with some tracepoints, thanks.

Sonny

Re: [PATCH] [RFC] perf: robustify proc and debugfs file recording

[Michael Neuling]

> >>> +  if (pwrite(output_fd, &size, hdr_sz, hdr_pos) < 0)
> >>
> >> s/&size/sizep/
> >
> > Argh, I'm really messing this up... so yeah this shouldn't have worked
> > on big-endian without that fix..
> > Mikey, are you sure it worked?
> 
> Actually, I think you need to use tracepoints or this code won't be invoked=
> .
> So, please try with some tracepoints, thanks.

Yeah, with tracepoints it's broken.  

With Steven fix though it's all good on my powerpc box.

Mikey

[PATCH] [RFCv2] perf: robustify proc and debugfs file recording

[Sonny Rao]

While attempting to create a timechart of boot up I found
perf didn't tolerate modules being loaded/unloaded.  This patch
fixes this by reading the file once and then writing the size
read at the correct point in the file.  It also simplifies the
code somewhat.

Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 tools/perf/util/trace-event-info.c |  122 +++++++++---------------------------
 1 files changed, 31 insertions(+), 91 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 35729f4..44a0250 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -183,106 +183,60 @@ int bigendian(void)
 	return *ptr == 0x01020304;
 }
 
-static unsigned long long copy_file_fd(int fd)
+/* 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];
-	int r;
-
-	do {
-		r = read(fd, buf, BUFSIZ);
-		if (r > 0) {
-			size += r;
-			write_or_die(buf, r);
-		}
-	} while (r > 0);
-
-	return size;
-}
-
-static unsigned long long copy_file(const char *file)
-{
-	unsigned long long size = 0;
-	int fd;
+	char buf[BUFSIZ], *sizep;
+	off_t hdr_pos = lseek(output_fd, 0, SEEK_CUR);
+	int r, fd;
 
 	fd = open(file, O_RDONLY);
 	if (fd < 0)
 		die("Can't read '%s'", file);
-	size = copy_file_fd(fd);
-	close(fd);
 
-	return size;
-}
-
-static unsigned long get_size_fd(int fd)
-{
-	unsigned long long size = 0;
-	char buf[BUFSIZ];
-	int r;
+	/* put in zeros for file size, then fill true size later */
+	write_or_die(&size, hdr_sz);
 
 	do {
 		r = read(fd, buf, BUFSIZ);
-		if (r > 0)
+		if (r > 0) {
 			size += r;
+			write_or_die(buf, r);
+		}
 	} while (r > 0);
-
-	lseek(fd, 0, SEEK_SET);
-
-	return size;
-}
-
-static unsigned long get_size(const char *file)
-{
-	unsigned long long size = 0;
-	int fd;
-
-	fd = open(file, O_RDONLY);
-	if (fd < 0)
-		die("Can't read '%s'", file);
-	size = get_size_fd(fd);
 	close(fd);
 
-	return size;
+	/* ugh, handle big-endian hdr_size == 4 */
+	sizep = (char*)&size;
+	if (bigendian())
+		sizep += sizeof(u64) - hdr_sz;
+
+	if (pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
+		die("writing to %s", output_file);
 }
 
 static void read_header_files(void)
 {
-	unsigned long long size, check_size;
+	unsigned long long size;
 	char *path;
-	int fd;
+	struct stat st;
 
 	path = get_tracing_file("events/header_page");
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
+	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
-	/* unfortunately, you can not stat debugfs files for size */
-	size = get_size_fd(fd);
-
 	write_or_die("header_page", 12);
-	write_or_die(&size, 8);
-	check_size = copy_file_fd(fd);
-	close(fd);
-
-	if (size != check_size)
-		die("wrong size for '%s' size=%lld read=%lld",
-		    path, size, check_size);
+	record_file(path, 8);
 	put_tracing_file(path);
 
 	path = get_tracing_file("events/header_event");
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
+	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
-	size = get_size_fd(fd);
-
 	write_or_die("header_event", 13);
-	write_or_die(&size, 8);
-	check_size = copy_file_fd(fd);
-	if (size != check_size)
-		die("wrong size for '%s'", path);
+	record_file(path, 8);
 	put_tracing_file(path);
-	close(fd);
 }
 
 static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
@@ -298,7 +252,7 @@ static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
 
 static void copy_event_system(const char *sys, struct tracepoint_path *tps)
 {
-	unsigned long long size, check_size;
+	unsigned long long size;
 	struct dirent *dent;
 	struct stat st;
 	char *format;
@@ -338,14 +292,8 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)
 		sprintf(format, "%s/%s/format", sys, dent->d_name);
 		ret = stat(format, &st);
 
-		if (ret >= 0) {
-			/* unfortunately, you can not stat debugfs files for size */
-			size = get_size(format);
-			write_or_die(&size, 8);
-			check_size = copy_file(format);
-			if (size != check_size)
-				die("error in size of file '%s'", format);
-		}
+		if (ret >= 0)
+			record_file(format, 8);
 
 		free(format);
 	}
@@ -426,7 +374,7 @@ static void read_event_files(struct tracepoint_path *tps)
 
 static void read_proc_kallsyms(void)
 {
-	unsigned int size, check_size;
+	unsigned int size;
 	const char *path = "/proc/kallsyms";
 	struct stat st;
 	int ret;
@@ -438,17 +386,12 @@ static void read_proc_kallsyms(void)
 		write_or_die(&size, 4);
 		return;
 	}
-	size = get_size(path);
-	write_or_die(&size, 4);
-	check_size = copy_file(path);
-	if (size != check_size)
-		die("error in size of file '%s'", path);
-
+	record_file(path, 4);
 }
 
 static void read_ftrace_printk(void)
 {
-	unsigned int size, check_size;
+	unsigned int size;
 	char *path;
 	struct stat st;
 	int ret;
@@ -461,11 +404,8 @@ static void read_ftrace_printk(void)
 		write_or_die(&size, 4);
 		goto out;
 	}
-	size = get_size(path);
-	write_or_die(&size, 4);
-	check_size = copy_file(path);
-	if (size != check_size)
-		die("error in size of file '%s'", path);
+	record_file(path, 4);
+
 out:
 	put_tracing_file(path);
 }
-- 
1.7.3.1

Re: [PATCH] [RFCv2] perf: robustify proc and debugfs file recording

[Steven Rostedt]

On Wed, 2011-07-13 at 17:40 -0700, Sonny Rao wrote:
> While attempting to create a timechart of boot up I found
> perf didn't tolerate modules being loaded/unloaded.  This patch
> fixes this by reading the file once and then writing the size
> read at the correct point in the file.  It also simplifies the
> code somewhat.
> 

I get this:

cc1: warnings being treated as errors
util/trace-event-info.c: In function ‘read_header_files’:
util/trace-event-info.c:221: error: unused variable ‘size’
util/trace-event-info.c: In function ‘copy_event_system’:
util/trace-event-info.c:255: error: unused variable ‘size’
make: *** [/tmp/build/util/trace-event-info.o] Error 1

-- Steve


> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> Signed-off-by: Michael Neuling <mikey@neuling.org>

[PATCH] [RFCv3] perf: robustify proc and debugfs file recording

[Michael Neuling]

In message <1310612277.27864.5.camel@gandalf.stny.rr.com> you wrote:
> On Wed, 2011-07-13 at 17:40 -0700, Sonny Rao wrote:
> > While attempting to create a timechart of boot up I found
> > perf didn't tolerate modules being loaded/unloaded.  This patch
> > fixes this by reading the file once and then writing the size
> > read at the correct point in the file.  It also simplifies the
> > code somewhat.
> > 
> 
> I get this:
> 
> cc1: warnings being treated as errors
> util/trace-event-info.c: In function ‘read_header_files’:
> util/trace-event-info.c:221: error: unused variable ‘size’
> util/trace-event-info.c: In function ‘copy_event_system’:
> util/trace-event-info.c:255: error: unused variable ‘size’
> make: *** [/tmp/build/util/trace-event-info.o] Error 1

Looks like sonny didn't pick up my changes correctly.  Here's the
working I tested with the sizep and warnings cleanups.

Mikey



From: Sonny Rao <sonnyrao@chromium.org>

[RFCv3] perf: robustify proc and debugfs file recording

While attempting to create a timechart of boot up I found perf didn't
tolerate modules being loaded/unloaded.  This patch fixes this by
reading the file once and then writing the size read at the correct
point in the file.  It also simplifies the code somewhat.

Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 tools/perf/util/trace-event-info.c |  120 ++++++++-----------------------------
 1 file changed, 29 insertions(+), 91 deletions(-)

Index: linux-ozlabs/tools/perf/util/trace-event-info.c
===================================================================
--- linux-ozlabs.orig/tools/perf/util/trace-event-info.c	2011-07-13 20:42:24.442945973 +1000
+++ linux-ozlabs/tools/perf/util/trace-event-info.c	2011-07-14 10:14:19.072946058 +1000
@@ -183,106 +183,59 @@
 	return *ptr == 0x01020304;
 }
 
-static unsigned long long copy_file_fd(int fd)
+/* 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];
-	int r;
-
-	do {
-		r = read(fd, buf, BUFSIZ);
-		if (r > 0) {
-			size += r;
-			write_or_die(buf, r);
-		}
-	} while (r > 0);
-
-	return size;
-}
-
-static unsigned long long copy_file(const char *file)
-{
-	unsigned long long size = 0;
-	int fd;
+	char buf[BUFSIZ], *sizep;
+	off_t hdr_pos = lseek(output_fd, 0, SEEK_CUR);
+	int r, fd;
 
 	fd = open(file, O_RDONLY);
 	if (fd < 0)
 		die("Can't read '%s'", file);
-	size = copy_file_fd(fd);
-	close(fd);
 
-	return size;
-}
-
-static unsigned long get_size_fd(int fd)
-{
-	unsigned long long size = 0;
-	char buf[BUFSIZ];
-	int r;
+	/* put in zeros for file size, then fill true size later */
+	write_or_die(&size, hdr_sz);
 
 	do {
 		r = read(fd, buf, BUFSIZ);
-		if (r > 0)
+		if (r > 0) {
 			size += r;
+			write_or_die(buf, r);
+		}
 	} while (r > 0);
-
-	lseek(fd, 0, SEEK_SET);
-
-	return size;
-}
-
-static unsigned long get_size(const char *file)
-{
-	unsigned long long size = 0;
-	int fd;
-
-	fd = open(file, O_RDONLY);
-	if (fd < 0)
-		die("Can't read '%s'", file);
-	size = get_size_fd(fd);
 	close(fd);
 
-	return size;
+	/* ugh, handle big-endian hdr_size == 4 */
+	sizep = (char*)&size;
+	if (bigendian())
+		sizep += sizeof(u64) - hdr_sz;
+
+	if (pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
+		die("writing to %s", output_file);
 }
 
 static void read_header_files(void)
 {
-	unsigned long long size, check_size;
 	char *path;
-	int fd;
+	struct stat st;
 
 	path = get_tracing_file("events/header_page");
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
+	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
-	/* unfortunately, you can not stat debugfs files for size */
-	size = get_size_fd(fd);
-
 	write_or_die("header_page", 12);
-	write_or_die(&size, 8);
-	check_size = copy_file_fd(fd);
-	close(fd);
-
-	if (size != check_size)
-		die("wrong size for '%s' size=%lld read=%lld",
-		    path, size, check_size);
+	record_file(path, 8);
 	put_tracing_file(path);
 
 	path = get_tracing_file("events/header_event");
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
+	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
-	size = get_size_fd(fd);
-
 	write_or_die("header_event", 13);
-	write_or_die(&size, 8);
-	check_size = copy_file_fd(fd);
-	if (size != check_size)
-		die("wrong size for '%s'", path);
+	record_file(path, 8);
 	put_tracing_file(path);
-	close(fd);
 }
 
 static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
@@ -298,7 +251,6 @@
 
 static void copy_event_system(const char *sys, struct tracepoint_path *tps)
 {
-	unsigned long long size, check_size;
 	struct dirent *dent;
 	struct stat st;
 	char *format;
@@ -338,14 +290,8 @@
 		sprintf(format, "%s/%s/format", sys, dent->d_name);
 		ret = stat(format, &st);
 
-		if (ret >= 0) {
-			/* unfortunately, you can not stat debugfs files for size */
-			size = get_size(format);
-			write_or_die(&size, 8);
-			check_size = copy_file(format);
-			if (size != check_size)
-				die("error in size of file '%s'", format);
-		}
+		if (ret >= 0)
+			record_file(format, 8);
 
 		free(format);
 	}
@@ -426,7 +372,7 @@
 
 static void read_proc_kallsyms(void)
 {
-	unsigned int size, check_size;
+	unsigned int size;
 	const char *path = "/proc/kallsyms";
 	struct stat st;
 	int ret;
@@ -438,17 +384,12 @@
 		write_or_die(&size, 4);
 		return;
 	}
-	size = get_size(path);
-	write_or_die(&size, 4);
-	check_size = copy_file(path);
-	if (size != check_size)
-		die("error in size of file '%s'", path);
-
+	record_file(path, 4);
 }
 
 static void read_ftrace_printk(void)
 {
-	unsigned int size, check_size;
+	unsigned int size;
 	char *path;
 	struct stat st;
 	int ret;
@@ -461,11 +402,8 @@
 		write_or_die(&size, 4);
 		goto out;
 	}
-	size = get_size(path);
-	write_or_die(&size, 4);
-	check_size = copy_file(path);
-	if (size != check_size)
-		die("error in size of file '%s'", path);
+	record_file(path, 4);
+
 out:
 	put_tracing_file(path);
 }

Re: [PATCH] [RFCv3] perf: robustify proc and debugfs file recording

[Steven Rostedt]

On Thu, 2011-07-14 at 13:34 +1000, Michael Neuling wrote:
> In message <1310612277.27864.5.camel@gandalf.stny.rr.com> you wrote:
> > On Wed, 2011-07-13 at 17:40 -0700, Sonny Rao wrote:
> > > While attempting to create a timechart of boot up I found
> > > perf didn't tolerate modules being loaded/unloaded.  This patch
> > > fixes this by reading the file once and then writing the size
> > > read at the correct point in the file.  It also simplifies the
> > > code somewhat.
> > > 
> > 
> > I get this:
> > 
> > cc1: warnings being treated as errors
> > util/trace-event-info.c: In function ‘read_header_files’:
> > util/trace-event-info.c:221: error: unused variable ‘size’
> > util/trace-event-info.c: In function ‘copy_event_system’:
> > util/trace-event-info.c:255: error: unused variable ‘size’
> > make: *** [/tmp/build/util/trace-event-info.o] Error 1
> 
> Looks like sonny didn't pick up my changes correctly.  Here's the
> working I tested with the sizep and warnings cleanups.

Hmm, your patch has some serious issues caused by your mail client. When
saving (or looking at the raw message) I get this:

---
 tools/perf/util/trace-event-info.c |  120 ++++++++------------------------=
-----
 1 file changed, 29 insertions(+), 91 deletions(-)

Index: linux-ozlabs/tools/perf/util/trace-event-info.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- linux-ozlabs.orig/tools/perf/util/trace-event-info.c        2011-07-13 20:42:2=
4.442945973 +1000
+++ linux-ozlabs/tools/perf/util/trace-event-info.c     2011-07-14 10:14:19.072=
946058 +1000
@@ -183,106 +183,59 @@
        return *ptr =3D=3D 0x01020304;
 }
=20
-static unsigned long long copy_file_fd(int fd)
+/* 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 =3D 0;
---

-- Steve

> 
> Mikey
> 
> 
> 
> From: Sonny Rao <sonnyrao@chromium.org>
> 
> [RFCv3] perf: robustify proc and debugfs file recording
> 
> While attempting to create a timechart of boot up I found perf didn't
> tolerate modules being loaded/unloaded.  This patch fixes this by
> reading the file once and then writing the size read at the correct
> point in the file.  It also simplifies the code somewhat.
> 
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> ---
>  tools/perf/util/trace-event-info.c |  120 ++++++++-----------------------------
>  1 file changed, 29 insertions(+), 91 deletions(-)
> 
> Index: linux-ozlabs/tools/perf/util/trace-event-info.c
> ===================================================================
> --- linux-ozlabs.orig/tools/perf/util/trace-event-info.c	2011-07-13 20:42:24.442945973 +1000
> +++ linux-ozlabs/tools/perf/util/trace-event-info.c	2011-07-14 10:14:19.072946058 +1000
> @@ -183,106 +183,59 @@
>  	return *ptr == 0x01020304;
>  }
>  
> -static unsigned long long copy_file_fd(int fd)
> +/* 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];
> -	int r;
> -
> -	do {
> -		r = read(fd, buf, BUFSIZ);
> -		if (r > 0) {
> -			size += r;
> -			write_or_die(buf, r);
> -		}
> -	} while (r > 0);
> -
> -	return size;
> -}
> -
> -static unsigned long long copy_file(const char *file)
> -{
> -	unsigned long long size = 0;
> -	int fd;
> +	char buf[BUFSIZ], *sizep;
> +	off_t hdr_pos = lseek(output_fd, 0, SEEK_CUR);
> +	int r, fd;
>  
>  	fd = open(file, O_RDONLY);
>  	if (fd < 0)
>  		die("Can't read '%s'", file);
> -	size = copy_file_fd(fd);
> -	close(fd);
>  
> -	return size;
> -}
> -
> -static unsigned long get_size_fd(int fd)
> -{
> -	unsigned long long size = 0;
> -	char buf[BUFSIZ];
> -	int r;
> +	/* put in zeros for file size, then fill true size later */
> +	write_or_die(&size, hdr_sz);
>  
>  	do {
>  		r = read(fd, buf, BUFSIZ);
> -		if (r > 0)
> +		if (r > 0) {
>  			size += r;
> +			write_or_die(buf, r);
> +		}
>  	} while (r > 0);
> -
> -	lseek(fd, 0, SEEK_SET);
> -
> -	return size;
> -}
> -
> -static unsigned long get_size(const char *file)
> -{
> -	unsigned long long size = 0;
> -	int fd;
> -
> -	fd = open(file, O_RDONLY);
> -	if (fd < 0)
> -		die("Can't read '%s'", file);
> -	size = get_size_fd(fd);
>  	close(fd);
>  
> -	return size;
> +	/* ugh, handle big-endian hdr_size == 4 */
> +	sizep = (char*)&size;
> +	if (bigendian())
> +		sizep += sizeof(u64) - hdr_sz;
> +
> +	if (pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
> +		die("writing to %s", output_file);
>  }
>  
>  static void read_header_files(void)
>  {
> -	unsigned long long size, check_size;
>  	char *path;
> -	int fd;
> +	struct stat st;
>  
>  	path = get_tracing_file("events/header_page");
> -	fd = open(path, O_RDONLY);
> -	if (fd < 0)
> +	if (stat(path, &st) < 0)
>  		die("can't read '%s'", path);
>  
> -	/* unfortunately, you can not stat debugfs files for size */
> -	size = get_size_fd(fd);
> -
>  	write_or_die("header_page", 12);
> -	write_or_die(&size, 8);
> -	check_size = copy_file_fd(fd);
> -	close(fd);
> -
> -	if (size != check_size)
> -		die("wrong size for '%s' size=%lld read=%lld",
> -		    path, size, check_size);
> +	record_file(path, 8);
>  	put_tracing_file(path);
>  
>  	path = get_tracing_file("events/header_event");
> -	fd = open(path, O_RDONLY);
> -	if (fd < 0)
> +	if (stat(path, &st) < 0)
>  		die("can't read '%s'", path);
>  
> -	size = get_size_fd(fd);
> -
>  	write_or_die("header_event", 13);
> -	write_or_die(&size, 8);
> -	check_size = copy_file_fd(fd);
> -	if (size != check_size)
> -		die("wrong size for '%s'", path);
> +	record_file(path, 8);
>  	put_tracing_file(path);
> -	close(fd);
>  }
>  
>  static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
> @@ -298,7 +251,6 @@
>  
>  static void copy_event_system(const char *sys, struct tracepoint_path *tps)
>  {
> -	unsigned long long size, check_size;
>  	struct dirent *dent;
>  	struct stat st;
>  	char *format;
> @@ -338,14 +290,8 @@
>  		sprintf(format, "%s/%s/format", sys, dent->d_name);
>  		ret = stat(format, &st);
>  
> -		if (ret >= 0) {
> -			/* unfortunately, you can not stat debugfs files for size */
> -			size = get_size(format);
> -			write_or_die(&size, 8);
> -			check_size = copy_file(format);
> -			if (size != check_size)
> -				die("error in size of file '%s'", format);
> -		}
> +		if (ret >= 0)
> +			record_file(format, 8);
>  
>  		free(format);
>  	}
> @@ -426,7 +372,7 @@
>  
>  static void read_proc_kallsyms(void)
>  {
> -	unsigned int size, check_size;
> +	unsigned int size;
>  	const char *path = "/proc/kallsyms";
>  	struct stat st;
>  	int ret;
> @@ -438,17 +384,12 @@
>  		write_or_die(&size, 4);
>  		return;
>  	}
> -	size = get_size(path);
> -	write_or_die(&size, 4);
> -	check_size = copy_file(path);
> -	if (size != check_size)
> -		die("error in size of file '%s'", path);
> -
> +	record_file(path, 4);
>  }
>  
>  static void read_ftrace_printk(void)
>  {
> -	unsigned int size, check_size;
> +	unsigned int size;
>  	char *path;
>  	struct stat st;
>  	int ret;
> @@ -461,11 +402,8 @@
>  		write_or_die(&size, 4);
>  		goto out;
>  	}
> -	size = get_size(path);
> -	write_or_die(&size, 4);
> -	check_size = copy_file(path);
> -	if (size != check_size)
> -		die("error in size of file '%s'", path);
> +	record_file(path, 4);
> +
>  out:
>  	put_tracing_file(path);
>  }

Re: [PATCH] [RFCv3] perf: robustify proc and debugfs file recording

[Peter Zijlstra]

On Thu, 2011-07-14 at 08:45 -0400, Steven Rostedt wrote:
> ---
>  tools/perf/util/trace-event-info.c |  120 ++++++++------------------------=
> -----
>  1 file changed, 29 insertions(+), 91 deletions(-)
> 
> Index: linux-ozlabs/tools/perf/util/trace-event-info.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- linux-ozlabs.orig/tools/perf/util/trace-event-info.c        2011-07-13 20:42:2=
> 4.442945973 +1000
> +++ linux-ozlabs/tools/perf/util/trace-event-info.c     2011-07-14 10:14:19.072=
> 946058 +1000
> @@ -183,106 +183,59 @@
>         return *ptr =3D=3D 0x01020304;
>  }
> =20
> -static unsigned long long copy_file_fd(int fd)
> +/* 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 =3D 0;

Yeah, its all the rage, we're supposed to write full RFC compliant email
parsers these days :(

/Content-Transfer-Encoding:.*quoted-printable.*/ {
        decode = 1;
}

// {
	tmp = $0

	if (!decode) {
		print tmp
		next
	}

	if (concat) {
                tmp = last tmp
                concat = 0;
        }

        if (tmp ~ /=$/) {
                concat = 1;
                gsub("=$", "", tmp);
        }

        offset = 0;
        while (match(tmp, /=[[:xdigit:]][[:xdigit:]]/, a)) {
                if (a[0] < offset)
                        break;
                hex = substr(a[0], 2)
                char = sprintf("%c", strtonum("0x"hex))
                gsub(a[0], char, tmp)
                offset = a[0];
        }

        if (concat) {
                last = tmp
                next
        }

        print tmp
}


Should decode that crap I think..

Re: [PATCH] [RFCv3] perf: robustify proc and debugfs file recording

[Steven Rostedt]

On Thu, 2011-07-14 at 14:55 +0200, Peter Zijlstra wrote:

> Yeah, its all the rage, we're supposed to write full RFC compliant email
> parsers these days :(
> 

> Should decode that crap I think..

Thanks Peter! This seems to work.

-- Steve

Re: [PATCH] [RFCv3] perf: robustify proc and debugfs file recording

[Michael Neuling]

In message <1310647558.27864.19.camel@gandalf.stny.rr.com> you wrote:
> On Thu, 2011-07-14 at 13:34 +1000, Michael Neuling wrote:
> > In message <1310612277.27864.5.camel@gandalf.stny.rr.com> you wrote:
> > > On Wed, 2011-07-13 at 17:40 -0700, Sonny Rao wrote:
> > > > While attempting to create a timechart of boot up I found
> > > > perf didn't tolerate modules being loaded/unloaded.  This patch
> > > > fixes this by reading the file once and then writing the size
> > > > read at the correct point in the file.  It also simplifies the
> > > > code somewhat.
> > > > 
> > > 
> > > I get this:
> > > 
> > > cc1: warnings being treated as errors
> > > util/trace-event-info.c: In function ‘read_header_files’:
> > > util/trace-event-info.c:221: error: unused variable ‘size’
> > > util/trace-event-info.c: In function ‘copy_event_system’:
> > > util/trace-event-info.c:255: error: unused variable ‘size’
> > > make: *** [/tmp/build/util/trace-event-info.o] Error 1
> > 
> > Looks like sonny didn't pick up my changes correctly.  Here's the
> > working I tested with the sizep and warnings cleanups.
> 
> Hmm, your patch has some serious issues caused by your mail client. When
> saving (or looking at the raw message) I get this:

Crap, sorry.  

This is the same mail client I've used for sending patches for donkeys
years so something much have under the covers with a distro upgrade or
some such.  

Mikey

Re: [PATCH] [RFCv3] perf: robustify proc and debugfs file recording

[Steven Rostedt]

On Fri, 2011-07-15 at 07:38 +1000, Michael Neuling wrote:

> > Hmm, your patch has some serious issues caused by your mail client. When
> > saving (or looking at the raw message) I get this:
> 
> Crap, sorry.  
> 
> This is the same mail client I've used for sending patches for donkeys
> years so something much have under the covers with a distro upgrade or
> some such.  

Even after fixing it, I had really strange issues with using my script
to pull it in with git am. I kept getting this strange error about the
patch header missing or something. Don't have it anymore as I just
finally did it by manually git am (error), apply patch by hand, git add,
git am resolve, which seemed to do the trick.

-- Steve

Re: [PATCH] [RFCv3] perf: robustify proc and debugfs file recording

[Sonny Rao]

On Thu, Jul 14, 2011 at 2:54 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 2011-07-15 at 07:38 +1000, Michael Neuling wrote:
>
>> > Hmm, your patch has some serious issues caused by your mail client. When
>> > saving (or looking at the raw message) I get this:
>>
>> Crap, sorry.
>>
>> This is the same mail client I've used for sending patches for donkeys
>> years so something much have under the covers with a distro upgrade or
>> some such.
>
> Even after fixing it, I had really strange issues with using my script
> to pull it in with git am. I kept getting this strange error about the
> patch header missing or something. Don't have it anymore as I just
> finally did it by manually git am (error), apply patch by hand, git add,
> git am resolve, which seemed to do the trick.
>
> -- Steve

Apologies for all the problems... I had tested it and it built but obviously
I screwed up somewhere.  Thanks for looking at it

Sonny

[tip:perf/core] perf: Robustify proc and debugfs file recording

[tip-bot for Sonny Rao]

Commit-ID:  259032bfe379281bf7cba512b7705bdb4ce41db5
Gitweb:     http://git.kernel.org/tip/259032bfe379281bf7cba512b7705bdb4ce41db5
Author:     Sonny Rao <sonnyrao@chromium.org>
AuthorDate: Thu, 14 Jul 2011 13:34:43 +1000
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Thu, 14 Jul 2011 15:53:01 -0400

perf: Robustify proc and debugfs file recording

While attempting to create a timechart of boot up I found perf didn't
tolerate modules being loaded/unloaded.  This patch fixes this by
reading the file once and then writing the size read at the correct
point in the file.  It also simplifies the code somewhat.

Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
Signed-off-by: Michael Neuling <mikey@neuling.org>
Link: http://lkml.kernel.org/r/10011.1310614483@neuling.org
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 tools/perf/util/trace-event-info.c |  120 +++++++++---------------------------
 1 files changed, 29 insertions(+), 91 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 35729f4..3403f81 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -183,106 +183,59 @@ int bigendian(void)
 	return *ptr == 0x01020304;
 }
 
-static unsigned long long copy_file_fd(int fd)
+/* 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];
-	int r;
-
-	do {
-		r = read(fd, buf, BUFSIZ);
-		if (r > 0) {
-			size += r;
-			write_or_die(buf, r);
-		}
-	} while (r > 0);
-
-	return size;
-}
-
-static unsigned long long copy_file(const char *file)
-{
-	unsigned long long size = 0;
-	int fd;
+	char buf[BUFSIZ], *sizep;
+	off_t hdr_pos = lseek(output_fd, 0, SEEK_CUR);
+	int r, fd;
 
 	fd = open(file, O_RDONLY);
 	if (fd < 0)
 		die("Can't read '%s'", file);
-	size = copy_file_fd(fd);
-	close(fd);
 
-	return size;
-}
-
-static unsigned long get_size_fd(int fd)
-{
-	unsigned long long size = 0;
-	char buf[BUFSIZ];
-	int r;
+	/* put in zeros for file size, then fill true size later */
+	write_or_die(&size, hdr_sz);
 
 	do {
 		r = read(fd, buf, BUFSIZ);
-		if (r > 0)
+		if (r > 0) {
 			size += r;
+			write_or_die(buf, r);
+		}
 	} while (r > 0);
-
-	lseek(fd, 0, SEEK_SET);
-
-	return size;
-}
-
-static unsigned long get_size(const char *file)
-{
-	unsigned long long size = 0;
-	int fd;
-
-	fd = open(file, O_RDONLY);
-	if (fd < 0)
-		die("Can't read '%s'", file);
-	size = get_size_fd(fd);
 	close(fd);
 
-	return size;
+	/* ugh, handle big-endian hdr_size == 4 */
+	sizep = (char*)&size;
+	if (bigendian())
+		sizep += sizeof(u64) - hdr_sz;
+
+	if (pwrite(output_fd, sizep, hdr_sz, hdr_pos) < 0)
+		die("writing to %s", output_file);
 }
 
 static void read_header_files(void)
 {
-	unsigned long long size, check_size;
 	char *path;
-	int fd;
+	struct stat st;
 
 	path = get_tracing_file("events/header_page");
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
+	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
-	/* unfortunately, you can not stat debugfs files for size */
-	size = get_size_fd(fd);
-
 	write_or_die("header_page", 12);
-	write_or_die(&size, 8);
-	check_size = copy_file_fd(fd);
-	close(fd);
-
-	if (size != check_size)
-		die("wrong size for '%s' size=%lld read=%lld",
-		    path, size, check_size);
+	record_file(path, 8);
 	put_tracing_file(path);
 
 	path = get_tracing_file("events/header_event");
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
+	if (stat(path, &st) < 0)
 		die("can't read '%s'", path);
 
-	size = get_size_fd(fd);
-
 	write_or_die("header_event", 13);
-	write_or_die(&size, 8);
-	check_size = copy_file_fd(fd);
-	if (size != check_size)
-		die("wrong size for '%s'", path);
+	record_file(path, 8);
 	put_tracing_file(path);
-	close(fd);
 }
 
 static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
@@ -298,7 +251,6 @@ static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
 
 static void copy_event_system(const char *sys, struct tracepoint_path *tps)
 {
-	unsigned long long size, check_size;
 	struct dirent *dent;
 	struct stat st;
 	char *format;
@@ -338,14 +290,8 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)
 		sprintf(format, "%s/%s/format", sys, dent->d_name);
 		ret = stat(format, &st);
 
-		if (ret >= 0) {
-			/* unfortunately, you can not stat debugfs files for size */
-			size = get_size(format);
-			write_or_die(&size, 8);
-			check_size = copy_file(format);
-			if (size != check_size)
-				die("error in size of file '%s'", format);
-		}
+		if (ret >= 0)
+			record_file(format, 8);
 
 		free(format);
 	}
@@ -426,7 +372,7 @@ static void read_event_files(struct tracepoint_path *tps)
 
 static void read_proc_kallsyms(void)
 {
-	unsigned int size, check_size;
+	unsigned int size;
 	const char *path = "/proc/kallsyms";
 	struct stat st;
 	int ret;
@@ -438,17 +384,12 @@ static void read_proc_kallsyms(void)
 		write_or_die(&size, 4);
 		return;
 	}
-	size = get_size(path);
-	write_or_die(&size, 4);
-	check_size = copy_file(path);
-	if (size != check_size)
-		die("error in size of file '%s'", path);
-
+	record_file(path, 4);
 }
 
 static void read_ftrace_printk(void)
 {
-	unsigned int size, check_size;
+	unsigned int size;
 	char *path;
 	struct stat st;
 	int ret;
@@ -461,11 +402,8 @@ static void read_ftrace_printk(void)
 		write_or_die(&size, 4);
 		goto out;
 	}
-	size = get_size(path);
-	write_or_die(&size, 4);
-	check_size = copy_file(path);
-	if (size != check_size)
-		die("error in size of file '%s'", path);
+	record_file(path, 4);
+
 out:
 	put_tracing_file(path);
 }

Re: [RFC] perf: robustify proc and debugfs file recording

[Riccardo Magliocchetti]

Hello,

Il -10/01/-28163 20:59, Sonny Rao ha scritto:
> While attempting to create a timechart of boot up I found
> perf didn't tolerate modules being loaded/unloaded.  This patch

Is this tool available somewhere?

Please CC me since i'm not subscribed.

thanks,
Riccardo Magliocchetti