Skip to contentSkip to conversation

perf: robustify proc and debugfs file recording

RFC PATCH Superseded Sonny Rao 5 participants 23 messages Jul 12, 2011 – Jul 21, 2011 perf events

lore
Share & tools
lore

Sonny Rao

author
#0 lore raw

RFC PATCH 1/1 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 <[email protected]>
---
 tools/perf/util/trace-event-info.c |  114 +++++++++---------------------------
 1 files changed, 27 insertions(+), 87 deletions(-)
tools/perf/util/trace-event-info.c +27 -87
index 35729f4..5a7e562 100644
@@ -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);
 }
Signature · 1 line
1.7.3.1
author
#1 lore raw
Quoted 8 lines · 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 <[email protected]>

---

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

#2 lore raw
In reply to Sonny Rao
Quoted 10 lines · 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 <[email protected]>

static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)

tools/perf/util/trace-event-info.c static void copy_event_system(const char *sys, struct tracepoint_path *tps) · line 347

Quoted 10 lines
tools/perf/util/trace-event-info.c +0 -7
@@ -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

Quoted 5 lines
tools/perf/util/trace-event-info.c +2 -1
-		}
+		if (ret >= 0)
+			record_file(format, 8);
 
 		free(format);
author
#3 lore raw
Quoted 14 lines · Steven Rostedt
Quoted 13 lines · 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 <[email protected]>

 static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)

tools/perf/util/trace-event-info.c +0 -0
@@ -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);
Quoted 11 lines

tools/perf/util/trace-event-info.c static void copy_event_system(const char *sys, struct tracepoint_path *tps) · line 347

Quoted 7 lines
tools/perf/util/trace-event-info.c +0 -7
-             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

#4 lore raw
Quoted 3 lines · Sonny Rao

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

#5 lore raw
In reply to Sonny Rao

Sonny,

Quoted 5 lines

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

#6 lore raw
Quoted 22 lines · In message <[email protected]> you wrote:

Sonny,

Quoted 5 lines

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=

Quoted 8 lines

=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 <[email protected]>

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 <[email protected]>
Signed-off-by Michael Neuling <[email protected]>
---
 tools/perf/util/trace-event-info.c |  120 ++++++++-----------------------------
 1 file changed, 29 insertions(+), 91 deletions(-)
tools/perf/util/trace-event-info.c +29 -91
Index: 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);
 }
author
#7 lore raw
Quoted 32 lines · Michael Neuling
Quoted 22 lines · In message <[email protected]> you wrote:

Sonny,

Quoted 5 lines

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=

Quoted 8 lines

=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

Quoted 6 lines

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

#8 lore raw
In reply to Michael Neuling
Quoted 4 lines · Michael Neuling

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...

Quoted 8 lines
tools/perf/util/trace-event-info.c +0 -8
-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);

tools/perf/util/trace-event-info.c int bigendian(void) · line 215

Quoted 10 lines
tools/perf/util/trace-event-info.c +6 -2
-	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

Quoted 2 lines
tools/perf/util/trace-event-info.c +1 -0
+		die("writing to %s", output_file);
 }
author
#9 lore raw
Quoted 21 lines · Steven Rostedt
Quoted 4 lines · Michael Neuling

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...

Quoted 11 lines
tools/perf/util/trace-event-info.c +0 -9
-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);

Quoted 9 lines
Quoted 7 lines

-     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

author
#10 lore raw
Quoted 32 lines · Sonny Rao
Quoted 21 lines · Steven Rostedt
Quoted 4 lines · Michael Neuling

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...

Quoted 11 lines
tools/perf/util/trace-event-info.c +0 -9
-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);

Quoted 9 lines
Quoted 7 lines

-     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/

Quoted 3 lines

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

#11 lore raw
Quoted 11 lines
Quoted 7 lines
Quoted 3 lines
Quoted 1 line

+ 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

author
#12 lore raw

RFC PATCH v2 1/1 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 <[email protected]>
Signed-off-by Michael Neuling <[email protected]>
---
 tools/perf/util/trace-event-info.c |  122 +++++++++---------------------------
 1 files changed, 31 insertions(+), 91 deletions(-)
tools/perf/util/trace-event-info.c +31 -91
index 35729f4..44a0250 100644
@@ -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);
 }
Signature · 1 line
1.7.3.1
#13 lore raw
Quoted 6 lines · 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.

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

Quoted 2 lines
Signed-off-by Sonny Rao <[email protected]>
Signed-off-by Michael Neuling <[email protected]>
#14 lore raw

RFC PATCH v3 1/1 perf: robustify proc and debugfs file recording

Quoted 8 lines · In message <[email protected]> you wrote:
Quoted 6 lines · 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.

Quoted 8 lines

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 <[email protected]>

[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 <[email protected]>
Signed-off-by Michael Neuling <[email protected]>
---
 tools/perf/util/trace-event-info.c |  120 ++++++++-----------------------------
 1 file changed, 29 insertions(+), 91 deletions(-)
tools/perf/util/trace-event-info.c +29 -91
Index: linux-ozlabs/tools/perf/util/trace-event-info.c
===================================================================
@@ -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);
 }
#15 lore raw
Quoted 18 lines · Michael Neuling
Quoted 8 lines · In message <[email protected]> you wrote:
Quoted 6 lines · 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.

Quoted 8 lines

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

Quoted 2 lines

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

Quoted 221 lines

Mikey

From: Sonny Rao <[email protected]>

[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 <[email protected]>
Signed-off-by Michael Neuling <[email protected]>
---
 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
===================================================================

tools/perf/util/trace-event-info.c +29 -91
@@ -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);
 }
#16 lore raw
Quoted 17 lines · Steven Rostedt

---
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

(quoted diff) +0 -0
@@ -183,106 +183,59 @@
        return *ptr =3D=3D 0x01020304;
 }

=20

tools/perf/util/trace-event-info.c

Quoted 5 lines
tools/perf/util/trace-event-info.c +2 -1
-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..

#17 lore raw
Quoted 5 lines · Peter Zijlstra

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

#18 lore raw
In reply to Steven Rostedt
Quoted 19 lines · In message <[email protected]> you wrote:
Quoted 18 lines · Michael Neuling
Quoted 16 lines · In message <[email protected]> you wrote:
Quoted 6 lines · 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.

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

Quoted 5 lines
Quoted 2 lines

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

#19 lore raw
Quoted 8 lines · Michael Neuling
Quoted 2 lines

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

author
#20 lore raw
Quoted 11 lines · Steven Rostedt
Quoted 8 lines · Michael Neuling
Quoted 2 lines

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.

Quoted 7 lines

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

author
#21 lore raw

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

In reply to Michael Neuling
Commit-ID:  259032bfe379281bf7cba512b7705bdb4ce41db5
Gitweb:     http://git.kernel.org/tip/259032bfe379281bf7cba512b7705bdb4ce41db5
Author:     Sonny Rao <[email protected]>
AuthorDate: Thu, 14 Jul 2011 13:34:43 +1000
Committer:  Steven Rostedt <[email protected]>
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 <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Signed-off-by Sonny Rao <[email protected]>
Signed-off-by Michael Neuling <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Steven Rostedt <[email protected]>
---
 tools/perf/util/trace-event-info.c |  120 +++++++++---------------------------
 1 files changed, 29 insertions(+), 91 deletions(-)
tools/perf/util/trace-event-info.c +29 -91
index 35729f4..3403f81 100644
@@ -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);
 }
#22 lore raw
In reply to Sonny Rao

Hello,

Il -10/01/-28163 20:59, Sonny Rao ha scritto:

Quoted 2 lines

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