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 <[email protected]>
---
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;
* Eric Dumazet <[email protected]> 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
Ingo Molnar a ?crit :
> * Eric Dumazet <[email protected]> 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.
Commit-ID: a255a9981a8566a1efabec983b7811e937e662d2
Gitweb: http://git.kernel.org/tip/a255a9981a8566a1efabec983b7811e937e662d2
Author: Eric Dumazet <[email protected]>
AuthorDate: Thu, 24 Sep 2009 15:05:59 +0200
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Cc: <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
Ingo Molnar a ?crit :
> * Eric Dumazet <[email protected]> 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 <[email protected]>
---
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);
}
* Eric Dumazet <[email protected]> wrote:
> Ingo Molnar a ?crit :
> > * Eric Dumazet <[email protected]> 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 <[email protected]>
> ---
> 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
Commit-ID: 725b13685c61168f71825b3fb67d96d2d7aa3b0f
Gitweb: http://git.kernel.org/tip/725b13685c61168f71825b3fb67d96d2d7aa3b0f
Author: Eric Dumazet <[email protected]>
AuthorDate: Thu, 24 Sep 2009 15:39:09 +0200
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ulrich Drepper <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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);
}
-----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-----
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
* Eric Dumazet <[email protected]> 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