[PATCH] perf tools: fix buffer allocation

[PATCH] perf tools: fix buffer allocation

[Eric Dumazet]

Hi Ingo

Here is a patch for perf.

BTW, use of openat() is a nuisance, since many machines have old glibc
(RHEL 4 Update 5 -> glibc-2.3.4-2.36 on my dev machine for example)

Thanks


[PATCH] perf tools: fix buffer allocation

"perf top" cores dump on my dev machine, if run from a directory where
vmlinux is present.

*** glibc detected *** malloc(): memory corruption: 0x085670d0 ***

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 tools/perf/util/module.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/module.c b/tools/perf/util/module.c
index 8f81622..0d8c85d 100644
--- a/tools/perf/util/module.c
+++ b/tools/perf/util/module.c
@@ -423,7 +423,7 @@ static int mod_dso__load_module_paths(struct mod_dso *self)
 	len += strlen(uts.release);
 	len += strlen("/modules.dep");
 
-	dpath = calloc(1, len);
+	dpath = calloc(1, len + 1);
 	if (dpath == NULL)
 		return err;
 

Re: [PATCH] perf tools: fix buffer allocation

[Ingo Molnar]

* Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Hi Ingo
> 
> Here is a patch for perf.

Applied, thanks Eric!

> BTW, use of openat() is a nuisance, since many machines have old glibc
> (RHEL 4 Update 5 -> glibc-2.3.4-2.36 on my dev machine for example)

We can certainly remove that reliance - wanna send a patch for it?

	Ingo

Re: [PATCH] perf tools: fix buffer allocation

[Eric Dumazet]

Ingo Molnar a écrit :
> * Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
>> Hi Ingo
>>
>> Here is a patch for perf.
> 
> Applied, thanks Eric!
> 
>> BTW, use of openat() is a nuisance, since many machines have old glibc
>> (RHEL 4 Update 5 -> glibc-2.3.4-2.36 on my dev machine for example)
> 
> We can certainly remove that reliance - wanna send a patch for it?

Yes I'll do it, thanks.

[PATCH] perf tools: Dont use openat()

[Eric Dumazet]

Ingo Molnar a écrit :
> * Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
>> Hi Ingo
>>
>> Here is a patch for perf.
> 
> Applied, thanks Eric!
> 
>> BTW, use of openat() is a nuisance, since many machines have old glibc
>> (RHEL 4 Update 5 -> glibc-2.3.4-2.36 on my dev machine for example)
> 
> We can certainly remove that reliance - wanna send a patch for it?

Here it is

Thanks

[PATCH] perf tools: Dont use openat()

openat() is still a young glibc facility, better to not use it
in a non performance critical program (perf list)

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 tools/perf/util/parse-events.c |   49 ++++++++++++-------------------
 1 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 13ab4b8..865208d 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -165,33 +165,31 @@ struct tracepoint_path *tracepoint_id_to_path(u64 config)
 	DIR *sys_dir, *evt_dir;
 	struct dirent *sys_next, *evt_next, sys_dirent, evt_dirent;
 	char id_buf[4];
-	int sys_dir_fd, fd;
+	int fd;
 	u64 id;
 	char evt_path[MAXPATHLEN];
+	char dir_path[MAXPATHLEN];
 
 	if (valid_debugfs_mount(debugfs_path))
 		return NULL;
 
 	sys_dir = opendir(debugfs_path);
 	if (!sys_dir)
-		goto cleanup;
-	sys_dir_fd = dirfd(sys_dir);
+		return NULL;
 
 	for_each_subsystem(sys_dir, sys_dirent, sys_next) {
-		int dfd = openat(sys_dir_fd, sys_dirent.d_name,
-				 O_RDONLY|O_DIRECTORY), evt_dir_fd;
-		if (dfd == -1)
-			continue;
-		evt_dir = fdopendir(dfd);
-		if (!evt_dir) {
-			close(dfd);
+
+		snprintf(dir_path, MAXPATHLEN, "%s/%s", debugfs_path,
+			 sys_dirent.d_name);
+		evt_dir = opendir(dir_path);
+		if (!evt_dir)
 			continue;
-		}
-		evt_dir_fd = dirfd(evt_dir);
+
 		for_each_event(sys_dirent, evt_dir, evt_dirent, evt_next) {
-			snprintf(evt_path, MAXPATHLEN, "%s/id",
+			
+			snprintf(evt_path, MAXPATHLEN, "%s/%s/id", dir_path,
 				 evt_dirent.d_name);
-			fd = openat(evt_dir_fd, evt_path, O_RDONLY);
+			fd = open(evt_path, O_RDONLY);
 			if (fd < 0)
 				continue;
 			if (read(fd, id_buf, sizeof(id_buf)) < 0) {
@@ -225,7 +223,6 @@ struct tracepoint_path *tracepoint_id_to_path(u64 config)
 		closedir(evt_dir);
 	}
 
-cleanup:
 	closedir(sys_dir);
 	return NULL;
 }
@@ -761,28 +758,24 @@ static void print_tracepoint_events(void)
 {
 	DIR *sys_dir, *evt_dir;
 	struct dirent *sys_next, *evt_next, sys_dirent, evt_dirent;
-	int sys_dir_fd;
 	char evt_path[MAXPATHLEN];
+	char dir_path[MAXPATHLEN];
 
 	if (valid_debugfs_mount(debugfs_path))
 		return;
 
 	sys_dir = opendir(debugfs_path);
 	if (!sys_dir)
-		goto cleanup;
-	sys_dir_fd = dirfd(sys_dir);
+		return;
 
 	for_each_subsystem(sys_dir, sys_dirent, sys_next) {
-		int dfd = openat(sys_dir_fd, sys_dirent.d_name,
-				 O_RDONLY|O_DIRECTORY), evt_dir_fd;
-		if (dfd == -1)
-			continue;
-		evt_dir = fdopendir(dfd);
-		if (!evt_dir) {
-			close(dfd);
+
+		snprintf(dir_path, MAXPATHLEN, "%s/%s", debugfs_path,
+			 sys_dirent.d_name);
+		evt_dir = opendir(dir_path);
+		if (!evt_dir)
 			continue;
-		}
-		evt_dir_fd = dirfd(evt_dir);
+
 		for_each_event(sys_dirent, evt_dir, evt_dirent, evt_next) {
 			snprintf(evt_path, MAXPATHLEN, "%s:%s",
 				 sys_dirent.d_name, evt_dirent.d_name);
@@ -791,8 +784,6 @@ static void print_tracepoint_events(void)
 		}
 		closedir(evt_dir);
 	}
-
-cleanup:
 	closedir(sys_dir);
 }
 

Re: [PATCH] perf tools: Dont use openat()

[Ingo Molnar]

* Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Ingo Molnar a ?crit :
> > * Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > 
> >> Hi Ingo
> >>
> >> Here is a patch for perf.
> > 
> > Applied, thanks Eric!
> > 
> >> BTW, use of openat() is a nuisance, since many machines have old glibc
> >> (RHEL 4 Update 5 -> glibc-2.3.4-2.36 on my dev machine for example)
> > 
> > We can certainly remove that reliance - wanna send a patch for it?
> 
> Here it is
> 
> Thanks
> 
> [PATCH] perf tools: Dont use openat()
> 
> openat() is still a young glibc facility, better to not use it
> in a non performance critical program (perf list)
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  tools/perf/util/parse-events.c |   49 ++++++++++++-------------------
>  1 files changed, 20 insertions(+), 29 deletions(-)

Applied, thanks Eric! I think it's also significant enough to be pushed 
to 2.6.32.

	Ingo

[tip:perf/urgent] perf tools: Dont use openat()

[tip-bot for Eric Dumazet]

Commit-ID:  725b13685c61168f71825b3fb67d96d2d7aa3b0f
Gitweb:     http://git.kernel.org/tip/725b13685c61168f71825b3fb67d96d2d7aa3b0f
Author:     Eric Dumazet <eric.dumazet@gmail.com>
AuthorDate: Thu, 24 Sep 2009 15:39:09 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 24 Sep 2009 21:20:08 +0200

perf tools: Dont use openat()

openat() is still a young glibc facility, better to not use it in a
non performance critical program (perf list)

Many machines have older glibc (RHEL 4 Update 5 -> glibc-2.3.4-2.36
on my dev machine for example).

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Ulrich Drepper <drepper@redhat.com>
LKML-Reference: <4ABB767D.6080004@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 tools/perf/util/parse-events.c |   49 ++++++++++++++++-----------------------
 1 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 13ab4b8..87c424d 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -165,33 +165,31 @@ struct tracepoint_path *tracepoint_id_to_path(u64 config)
 	DIR *sys_dir, *evt_dir;
 	struct dirent *sys_next, *evt_next, sys_dirent, evt_dirent;
 	char id_buf[4];
-	int sys_dir_fd, fd;
+	int fd;
 	u64 id;
 	char evt_path[MAXPATHLEN];
+	char dir_path[MAXPATHLEN];
 
 	if (valid_debugfs_mount(debugfs_path))
 		return NULL;
 
 	sys_dir = opendir(debugfs_path);
 	if (!sys_dir)
-		goto cleanup;
-	sys_dir_fd = dirfd(sys_dir);
+		return NULL;
 
 	for_each_subsystem(sys_dir, sys_dirent, sys_next) {
-		int dfd = openat(sys_dir_fd, sys_dirent.d_name,
-				 O_RDONLY|O_DIRECTORY), evt_dir_fd;
-		if (dfd == -1)
-			continue;
-		evt_dir = fdopendir(dfd);
-		if (!evt_dir) {
-			close(dfd);
+
+		snprintf(dir_path, MAXPATHLEN, "%s/%s", debugfs_path,
+			 sys_dirent.d_name);
+		evt_dir = opendir(dir_path);
+		if (!evt_dir)
 			continue;
-		}
-		evt_dir_fd = dirfd(evt_dir);
+
 		for_each_event(sys_dirent, evt_dir, evt_dirent, evt_next) {
-			snprintf(evt_path, MAXPATHLEN, "%s/id",
+
+			snprintf(evt_path, MAXPATHLEN, "%s/%s/id", dir_path,
 				 evt_dirent.d_name);
-			fd = openat(evt_dir_fd, evt_path, O_RDONLY);
+			fd = open(evt_path, O_RDONLY);
 			if (fd < 0)
 				continue;
 			if (read(fd, id_buf, sizeof(id_buf)) < 0) {
@@ -225,7 +223,6 @@ struct tracepoint_path *tracepoint_id_to_path(u64 config)
 		closedir(evt_dir);
 	}
 
-cleanup:
 	closedir(sys_dir);
 	return NULL;
 }
@@ -761,28 +758,24 @@ static void print_tracepoint_events(void)
 {
 	DIR *sys_dir, *evt_dir;
 	struct dirent *sys_next, *evt_next, sys_dirent, evt_dirent;
-	int sys_dir_fd;
 	char evt_path[MAXPATHLEN];
+	char dir_path[MAXPATHLEN];
 
 	if (valid_debugfs_mount(debugfs_path))
 		return;
 
 	sys_dir = opendir(debugfs_path);
 	if (!sys_dir)
-		goto cleanup;
-	sys_dir_fd = dirfd(sys_dir);
+		return;
 
 	for_each_subsystem(sys_dir, sys_dirent, sys_next) {
-		int dfd = openat(sys_dir_fd, sys_dirent.d_name,
-				 O_RDONLY|O_DIRECTORY), evt_dir_fd;
-		if (dfd == -1)
-			continue;
-		evt_dir = fdopendir(dfd);
-		if (!evt_dir) {
-			close(dfd);
+
+		snprintf(dir_path, MAXPATHLEN, "%s/%s", debugfs_path,
+			 sys_dirent.d_name);
+		evt_dir = opendir(dir_path);
+		if (!evt_dir)
 			continue;
-		}
-		evt_dir_fd = dirfd(evt_dir);
+
 		for_each_event(sys_dirent, evt_dir, evt_dirent, evt_next) {
 			snprintf(evt_path, MAXPATHLEN, "%s:%s",
 				 sys_dirent.d_name, evt_dirent.d_name);
@@ -791,8 +784,6 @@ static void print_tracepoint_events(void)
 		}
 		closedir(evt_dir);
 	}
-
-cleanup:
 	closedir(sys_dir);
 }
 

Re: [PATCH] perf tools: Dont use openat()

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Eric Dumazet wrote:
>> We can certainly remove that reliance - wanna send a patch for it?

Come on, the silliness has to stop.  The kernel must be recent and to
use it adequately the C library also must be recent.  And "recent" is
not even correct anymore: the functions are available for more then two
years.  Removing the use of the modern interfaces makes everything
slower and might even re-introduce race conditions my patch fixed.

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkq733cACgkQ2ijCOnn/RHQHzQCgsmre/xVEVghMVRWDCIevMPwc
+ecAmwaSOSLuSySZXRzSY/S64jf7K0Bd
=y6MN
-----END PGP SIGNATURE-----

Re: [PATCH] perf tools: Dont use openat()

[Eric Dumazet]

Ulrich Drepper a écrit :
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Eric Dumazet wrote:
>>> We can certainly remove that reliance - wanna send a patch for it?
> 
> Come on, the silliness has to stop.  The kernel must be recent and to
> use it adequately the C library also must be recent.  And "recent" is
> not even correct anymore: the functions are available for more then two
> years.  Removing the use of the modern interfaces makes everything
> slower and might even re-introduce race conditions my patch fixed.
> 

First time I ear that C library *must* be recent. This was never
mentionned in Documentation/Changes, "Minimal Requirements"

"perf list" can be 100x slower, and even racy, I dont mind. At all.

$ time perf list >/dev/null 2>/dev/null

real    0m0.001s
user    0m0.000s
sys     0m0.001s

With openat(), I cannot use "perf" on machines I can only change kernel,
since changing glibc is too risky for legacy apps. I could use static and
private glibc, but last time I tried this I lost few hours and failed.

If modern interfaces means : "Upgrade glibc or die, silly you..."
I prefer to be silly, and keep increasing linux usability, even
if I dont have the chance to have a modern lab with up2date distros.

BTW, oprofile compiles perfectly on RHEL4 and 'old' glibc.

Thanks

Re: [PATCH] perf tools: Dont use openat()

[Ingo Molnar]

* Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Ulrich Drepper a ??crit :
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > Eric Dumazet wrote:
> >>> We can certainly remove that reliance - wanna send a patch for it?
> > 
> > Come on, the silliness has to stop.  The kernel must be recent and to
> > use it adequately the C library also must be recent.  And "recent" is
> > not even correct anymore: the functions are available for more then two
> > years.  Removing the use of the modern interfaces makes everything
> > slower and might even re-introduce race conditions my patch fixed.
> > 
> 
> First time I ear that C library *must* be recent. This was never
> mentionned in Documentation/Changes, "Minimal Requirements"
> 
> "perf list" can be 100x slower, and even racy, I dont mind. At all.
> 
> $ time perf list >/dev/null 2>/dev/null
> 
> real    0m0.001s
> user    0m0.000s
> sys     0m0.001s

Yep, doesnt look problematic from a performance POV.

> With openat(), I cannot use "perf" on machines I can only change 
> kernel, since changing glibc is too risky for legacy apps. I could use 
> static and private glibc, but last time I tried this I lost few hours 
> and failed.

Yeah, we generally dont want to force an upgrade of other components if 
possible. The fact that you ran into it in a pre -rc1 kernel means that 
there's a thousand others out there who will run into similar problems.

	Ingo

[tip:perf/urgent] perf tools: Fix buffer allocation

[tip-bot for Eric Dumazet]

Commit-ID:  a255a9981a8566a1efabec983b7811e937e662d2
Gitweb:     http://git.kernel.org/tip/a255a9981a8566a1efabec983b7811e937e662d2
Author:     Eric Dumazet <eric.dumazet@gmail.com>
AuthorDate: Thu, 24 Sep 2009 15:05:59 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 24 Sep 2009 15:11:23 +0200

perf tools: Fix buffer allocation

"perf top" cores dump on my dev machine, if run from a directory
where vmlinux is present:

  *** glibc detected *** malloc(): memory corruption: 0x085670d0 ***

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: <stable@kernel.org>
LKML-Reference: <4ABB6EB7.7000002@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 tools/perf/util/module.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/module.c b/tools/perf/util/module.c
index 8f81622..0d8c85d 100644
--- a/tools/perf/util/module.c
+++ b/tools/perf/util/module.c
@@ -423,7 +423,7 @@ static int mod_dso__load_module_paths(struct mod_dso *self)
 	len += strlen(uts.release);
 	len += strlen("/modules.dep");
 
-	dpath = calloc(1, len);
+	dpath = calloc(1, len + 1);
 	if (dpath == NULL)
 		return err;