2009-12-28 05:12:54

by Xiao Guangrong

[permalink] [raw]
Subject: [PATCH] perf_event: mount debugfs automatically

Mount debugfs automatically if it's not mounted, umount it
when programme exit

Signed-off-by: Xiao Guangrong <[email protected]>
---
tools/perf/perf.c | 10 +++++++++-
tools/perf/util/debugfs.c | 16 +++++++---------
tools/perf/util/debugfs.h | 2 +-
3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 873e55f..ee84fc1 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -388,7 +388,7 @@ static int run_argv(int *argcp, const char ***argv)
/* mini /proc/mounts parser: searching for "^blah /mount/point debugfs" */
static void get_debugfs_mntpt(void)
{
- const char *path = debugfs_find_mountpoint();
+ const char *path = debugfs_mount(NULL);

if (path)
strncpy(debugfs_mntpt, path, sizeof(debugfs_mntpt));
@@ -396,6 +396,11 @@ static void get_debugfs_mntpt(void)
debugfs_mntpt[0] = '\0';
}

+static void umount_debugfs(void)
+{
+ debugfs_umount();
+}
+
int main(int argc, const char **argv)
{
const char *cmd;
@@ -405,6 +410,9 @@ int main(int argc, const char **argv)
cmd = "perf-help";
/* get debugfs mount point from /proc/mounts */
get_debugfs_mntpt();
+
+ atexit(umount_debugfs);
+
/*
* "perf-xxxx" is the same as "perf xxxx", but we obviously:
*
diff --git a/tools/perf/util/debugfs.c b/tools/perf/util/debugfs.c
index 06b73ee..1f805fd 100644
--- a/tools/perf/util/debugfs.c
+++ b/tools/perf/util/debugfs.c
@@ -106,16 +106,14 @@ int debugfs_valid_entry(const char *path)
return 0;
}

-/* mount the debugfs somewhere */
+/* mount the debugfs somewhere if it's not mounted */

-int debugfs_mount(const char *mountpoint)
+char *debugfs_mount(const char *mountpoint)
{
- char mountcmd[128];
-
/* see if it's already mounted */
if (debugfs_find_mountpoint()) {
debugfs_premounted = 1;
- return 0;
+ return debugfs_mountpoint;
}

/* if not mounted and no argument */
@@ -127,13 +125,13 @@ int debugfs_mount(const char *mountpoint)
mountpoint = "/sys/kernel/debug";
}

+ if (mount(NULL, mountpoint, "debugfs", 0, NULL) < 0)
+ return NULL;
+
/* save the mountpoint */
strncpy(debugfs_mountpoint, mountpoint, sizeof(debugfs_mountpoint));

- /* mount it */
- snprintf(mountcmd, sizeof(mountcmd),
- "/bin/mount -t debugfs debugfs %s", mountpoint);
- return system(mountcmd);
+ return debugfs_mountpoint;
}

/* umount the debugfs */
diff --git a/tools/perf/util/debugfs.h b/tools/perf/util/debugfs.h
index 3cd14f9..83a0287 100644
--- a/tools/perf/util/debugfs.h
+++ b/tools/perf/util/debugfs.h
@@ -15,7 +15,7 @@
extern const char *debugfs_find_mountpoint(void);
extern int debugfs_valid_mountpoint(const char *debugfs);
extern int debugfs_valid_entry(const char *path);
-extern int debugfs_mount(const char *mountpoint);
+extern char *debugfs_mount(const char *mountpoint);
extern int debugfs_umount(void);
extern int debugfs_write(const char *entry, const char *value);
extern int debugfs_read(const char *entry, char *buffer, size_t size);
--
1.6.1.2


2009-12-28 07:45:05

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] perf_event: mount debugfs automatically


* Xiao Guangrong <[email protected]> wrote:

> Mount debugfs automatically if it's not mounted, umount it
> when programme exit
>
> Signed-off-by: Xiao Guangrong <[email protected]>
> ---
> tools/perf/perf.c | 10 +++++++++-
> tools/perf/util/debugfs.c | 16 +++++++---------
> tools/perf/util/debugfs.h | 2 +-
> 3 files changed, 17 insertions(+), 11 deletions(-)

I'm not sure that's a good idea. What happens if two perf tools are running in
parallel:

perf report #1 start
perf report #2 start
perf report #2 exit [umount debugfs]
perf report #2 tries to open /debug file: kaboom
perf report #2 exit

But your idea is sound if we only do the first half: we should mount it under
/sys/kernel/debug/ if it's not mounted already [and that directory is
available], and leave it mounted there.

Furthermore please not that we must not mount it under any other path -
mounting is always a dangerous operation because it changes the VFS namespace.
For example some system might have some local files under /debug for whatever
reason, we must not over-mount it and potentially destroy data because we'd
confuse an app that writes into /debug. /sys/kernel/debug/ is a fair game to
mount into OTOH.

(and even then we should just leave it mounted and never umount it.)

Thanks,

Ingo

2009-12-28 08:04:25

by Xiao Guangrong

[permalink] [raw]
Subject: Re: [PATCH] perf_event: mount debugfs automatically



Ingo Molnar wrote:
> * Xiao Guangrong <[email protected]> wrote:
>
>> Mount debugfs automatically if it's not mounted, umount it
>> when programme exit
>>
>> Signed-off-by: Xiao Guangrong <[email protected]>
>> ---
>> tools/perf/perf.c | 10 +++++++++-
>> tools/perf/util/debugfs.c | 16 +++++++---------
>> tools/perf/util/debugfs.h | 2 +-
>> 3 files changed, 17 insertions(+), 11 deletions(-)
>
> I'm not sure that's a good idea. What happens if two perf tools are running in
> parallel:
>
> perf report #1 start
> perf report #2 start
> perf report #2 exit [umount debugfs]
> perf report #2 tries to open /debug file: kaboom
> perf report #2 exit
>

Sorry, i not notice it, it can't work under this case.

> But your idea is sound if we only do the first half: we should mount it under
> /sys/kernel/debug/ if it's not mounted already [and that directory is
> available], and leave it mounted there.
>

Um, i'll rewrite it and just mount it under /sys/kernel/debug

Thanks,
Xiao

2009-12-28 08:06:32

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] perf_event: mount debugfs automatically


* Xiao Guangrong <[email protected]> wrote:

> > But your idea is sound if we only do the first half: we should mount it
> > under /sys/kernel/debug/ if it's not mounted already [and that directory
> > is available], and leave it mounted there.
> >
>
> Um, i'll rewrite it and just mount it under /sys/kernel/debug

Yeah, i think that will work better. Thanks!

Ingo

2009-12-28 08:49:08

by Xiao Guangrong

[permalink] [raw]
Subject: [PATCH v2 1/3] perf tools: mount debugfs automatically

Mount debugfs filesystem under '/sys/kernel/debug' if it's not mounted

Signed-off-by: Xiao Guangrong <[email protected]>
---
tools/perf/perf.c | 2 +-
tools/perf/util/debugfs.c | 16 +++++++---------
tools/perf/util/debugfs.h | 2 +-
3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 873e55f..fc89005 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -388,7 +388,7 @@ static int run_argv(int *argcp, const char ***argv)
/* mini /proc/mounts parser: searching for "^blah /mount/point debugfs" */
static void get_debugfs_mntpt(void)
{
- const char *path = debugfs_find_mountpoint();
+ const char *path = debugfs_mount(NULL);

if (path)
strncpy(debugfs_mntpt, path, sizeof(debugfs_mntpt));
diff --git a/tools/perf/util/debugfs.c b/tools/perf/util/debugfs.c
index 06b73ee..1f805fd 100644
--- a/tools/perf/util/debugfs.c
+++ b/tools/perf/util/debugfs.c
@@ -106,16 +106,14 @@ int debugfs_valid_entry(const char *path)
return 0;
}

-/* mount the debugfs somewhere */
+/* mount the debugfs somewhere if it's not mounted */

-int debugfs_mount(const char *mountpoint)
+char *debugfs_mount(const char *mountpoint)
{
- char mountcmd[128];
-
/* see if it's already mounted */
if (debugfs_find_mountpoint()) {
debugfs_premounted = 1;
- return 0;
+ return debugfs_mountpoint;
}

/* if not mounted and no argument */
@@ -127,13 +125,13 @@ int debugfs_mount(const char *mountpoint)
mountpoint = "/sys/kernel/debug";
}

+ if (mount(NULL, mountpoint, "debugfs", 0, NULL) < 0)
+ return NULL;
+
/* save the mountpoint */
strncpy(debugfs_mountpoint, mountpoint, sizeof(debugfs_mountpoint));

- /* mount it */
- snprintf(mountcmd, sizeof(mountcmd),
- "/bin/mount -t debugfs debugfs %s", mountpoint);
- return system(mountcmd);
+ return debugfs_mountpoint;
}

/* umount the debugfs */
diff --git a/tools/perf/util/debugfs.h b/tools/perf/util/debugfs.h
index 3cd14f9..83a0287 100644
--- a/tools/perf/util/debugfs.h
+++ b/tools/perf/util/debugfs.h
@@ -15,7 +15,7 @@
extern const char *debugfs_find_mountpoint(void);
extern int debugfs_valid_mountpoint(const char *debugfs);
extern int debugfs_valid_entry(const char *path);
-extern int debugfs_mount(const char *mountpoint);
+extern char *debugfs_mount(const char *mountpoint);
extern int debugfs_umount(void);
extern int debugfs_write(const char *entry, const char *value);
extern int debugfs_read(const char *entry, char *buffer, size_t size);
--
1.6.1.2

2009-12-28 08:50:23

by Xiao Guangrong

[permalink] [raw]
Subject: [PATCH 2/3] perf trace: cleanup find_debugfs()

Remove redundant code for 'perf trace'

Signed-off-by: Xiao Guangrong <[email protected]>
---
tools/perf/util/debugfs.c | 1 +
tools/perf/util/trace-event-info.c | 29 +++++------------------------
2 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/tools/perf/util/debugfs.c b/tools/perf/util/debugfs.c
index 1f805fd..a88fefc 100644
--- a/tools/perf/util/debugfs.c
+++ b/tools/perf/util/debugfs.c
@@ -130,6 +130,7 @@ char *debugfs_mount(const char *mountpoint)

/* save the mountpoint */
strncpy(debugfs_mountpoint, mountpoint, sizeof(debugfs_mountpoint));
+ debugfs_found = 1;

return debugfs_mountpoint;
}
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index cace355..5dd5c81 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -37,6 +37,7 @@

#include "../perf.h"
#include "trace-event.h"
+#include "debugfs.h"

#define VERSION "0.5"

@@ -101,32 +102,12 @@ void *malloc_or_die(unsigned int size)

static const char *find_debugfs(void)
{
- static char debugfs[MAX_PATH+1];
- static int debugfs_found;
- char type[100];
- FILE *fp;
-
- if (debugfs_found)
- return debugfs;
-
- if ((fp = fopen("/proc/mounts","r")) == NULL)
- die("Can't open /proc/mounts for read");
-
- while (fscanf(fp, "%*s %"
- STR(MAX_PATH)
- "s %99s %*s %*d %*d\n",
- debugfs, type) == 2) {
- if (strcmp(type, "debugfs") == 0)
- break;
- }
- fclose(fp);
-
- if (strcmp(type, "debugfs") != 0)
- die("debugfs not mounted, please mount");
+ const char *path = debugfs_mount(NULL);

- debugfs_found = 1;
+ if (!path)
+ die("Your kernel not support debugfs filesystem");

- return debugfs;
+ return path;
}

/*
--
1.6.1.2

2009-12-28 08:51:31

by Xiao Guangrong

[permalink] [raw]
Subject: [PATCH 3/3] perf trace: fix forgetting close file/dir

Fix forgetting close file/dir

Signed-off-by: Xiao Guangrong <[email protected]>
---
tools/perf/util/trace-event-info.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 5dd5c81..2c84fb6 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -252,6 +252,8 @@ static void read_header_files(void)
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);
@@ -270,6 +272,7 @@ static void read_header_files(void)
if (size != check_size)
die("wrong size for '%s'", path);
put_tracing_file(path);
+ close(fd);
}

static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
@@ -334,6 +337,7 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)

free(format);
}
+ closedir(dir);
}

static void read_ftrace_files(struct tracepoint_path *tps)
@@ -411,6 +415,7 @@ static void read_event_files(struct tracepoint_path *tps)
free(sys);
}

+ closedir(dir);
put_tracing_file(path);
}

--
1.6.1.2

2009-12-28 10:12:46

by Xiao Guangrong

[permalink] [raw]
Subject: [tip:perf/core] perf tools: Mount debugfs automatically

Commit-ID: 29c52aa2300173dd45df04dae1f5acc81a2c93b1
Gitweb: http://git.kernel.org/tip/29c52aa2300173dd45df04dae1f5acc81a2c93b1
Author: Xiao Guangrong <[email protected]>
AuthorDate: Mon, 28 Dec 2009 16:47:12 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Mon, 28 Dec 2009 10:36:36 +0100

perf tools: Mount debugfs automatically

Mount debugfs filesystem under '/sys/kernel/debug', if it's not
mounted.

Signed-off-by: Xiao Guangrong <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Clark Williams <[email protected]>
Cc: John Kacur <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
tools/perf/perf.c | 2 +-
tools/perf/util/debugfs.c | 16 +++++++---------
tools/perf/util/debugfs.h | 2 +-
3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 873e55f..fc89005 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -388,7 +388,7 @@ static int run_argv(int *argcp, const char ***argv)
/* mini /proc/mounts parser: searching for "^blah /mount/point debugfs" */
static void get_debugfs_mntpt(void)
{
- const char *path = debugfs_find_mountpoint();
+ const char *path = debugfs_mount(NULL);

if (path)
strncpy(debugfs_mntpt, path, sizeof(debugfs_mntpt));
diff --git a/tools/perf/util/debugfs.c b/tools/perf/util/debugfs.c
index 06b73ee..1f805fd 100644
--- a/tools/perf/util/debugfs.c
+++ b/tools/perf/util/debugfs.c
@@ -106,16 +106,14 @@ int debugfs_valid_entry(const char *path)
return 0;
}

-/* mount the debugfs somewhere */
+/* mount the debugfs somewhere if it's not mounted */

-int debugfs_mount(const char *mountpoint)
+char *debugfs_mount(const char *mountpoint)
{
- char mountcmd[128];
-
/* see if it's already mounted */
if (debugfs_find_mountpoint()) {
debugfs_premounted = 1;
- return 0;
+ return debugfs_mountpoint;
}

/* if not mounted and no argument */
@@ -127,13 +125,13 @@ int debugfs_mount(const char *mountpoint)
mountpoint = "/sys/kernel/debug";
}

+ if (mount(NULL, mountpoint, "debugfs", 0, NULL) < 0)
+ return NULL;
+
/* save the mountpoint */
strncpy(debugfs_mountpoint, mountpoint, sizeof(debugfs_mountpoint));

- /* mount it */
- snprintf(mountcmd, sizeof(mountcmd),
- "/bin/mount -t debugfs debugfs %s", mountpoint);
- return system(mountcmd);
+ return debugfs_mountpoint;
}

/* umount the debugfs */
diff --git a/tools/perf/util/debugfs.h b/tools/perf/util/debugfs.h
index 3cd14f9..83a0287 100644
--- a/tools/perf/util/debugfs.h
+++ b/tools/perf/util/debugfs.h
@@ -15,7 +15,7 @@
extern const char *debugfs_find_mountpoint(void);
extern int debugfs_valid_mountpoint(const char *debugfs);
extern int debugfs_valid_entry(const char *path);
-extern int debugfs_mount(const char *mountpoint);
+extern char *debugfs_mount(const char *mountpoint);
extern int debugfs_umount(void);
extern int debugfs_write(const char *entry, const char *value);
extern int debugfs_read(const char *entry, char *buffer, size_t size);

2009-12-28 10:11:10

by Xiao Guangrong

[permalink] [raw]
Subject: [tip:perf/core] perf trace: Clean up find_debugfs()

Commit-ID: 61be3e59ba7a6dbd39f92fd1f107285a0caeb008
Gitweb: http://git.kernel.org/tip/61be3e59ba7a6dbd39f92fd1f107285a0caeb008
Author: Xiao Guangrong <[email protected]>
AuthorDate: Mon, 28 Dec 2009 16:48:30 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Mon, 28 Dec 2009 10:36:36 +0100

perf trace: Clean up find_debugfs()

Remove redundant code for 'perf trace'

Signed-off-by: Xiao Guangrong <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Clark Williams <[email protected]>
Cc: John Kacur <[email protected]>
LKML-Reference: <[email protected]>
[ v2: resolved conflicts with recent changes ]
Signed-off-by: Ingo Molnar <[email protected]>
---
tools/perf/util/debugfs.c | 1 +
tools/perf/util/trace-event-info.c | 29 +++++------------------------
2 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/tools/perf/util/debugfs.c b/tools/perf/util/debugfs.c
index 1f805fd..a88fefc 100644
--- a/tools/perf/util/debugfs.c
+++ b/tools/perf/util/debugfs.c
@@ -130,6 +130,7 @@ char *debugfs_mount(const char *mountpoint)

/* save the mountpoint */
strncpy(debugfs_mountpoint, mountpoint, sizeof(debugfs_mountpoint));
+ debugfs_found = 1;

return debugfs_mountpoint;
}
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index dfef238..535176d 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -38,6 +38,7 @@

#include "../perf.h"
#include "trace-event.h"
+#include "debugfs.h"

#define VERSION "0.5"

@@ -102,32 +103,12 @@ void *malloc_or_die(unsigned int size)

static const char *find_debugfs(void)
{
- static char debugfs[MAX_PATH+1];
- static int debugfs_found;
- FILE *fp;
- struct mntent *m;
-
- if (debugfs_found)
- return debugfs;
-
- fp = setmntent("/proc/mounts", "r");
- if (!fp)
- die("Can't open /proc/mounts for read");
-
- while ((m = getmntent(fp)) != NULL) {
- if (strcmp(m->mnt_type, "debugfs") == 0) {
- strcpy(debugfs, m->mnt_dir);
- debugfs_found = 1;
- break;
- }
- }
-
- endmntent(fp);
+ const char *path = debugfs_mount(NULL);

- if (!debugfs_found)
- die("debugfs not mounted, please mount");
+ if (!path)
+ die("Your kernel not support debugfs filesystem");

- return debugfs;
+ return path;
}

/*

2009-12-28 10:12:31

by Xiao Guangrong

[permalink] [raw]
Subject: [tip:perf/core] perf trace: Fix forgotten close of file/dir

Commit-ID: 9967411e5b324a908e344d6ce66b77bd5d372c3e
Gitweb: http://git.kernel.org/tip/9967411e5b324a908e344d6ce66b77bd5d372c3e
Author: Xiao Guangrong <[email protected]>
AuthorDate: Mon, 28 Dec 2009 16:49:38 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Mon, 28 Dec 2009 10:36:36 +0100

perf trace: Fix forgotten close of file/dir

Signed-off-by: Xiao Guangrong <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Clark Williams <[email protected]>
Cc: John Kacur <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
tools/perf/util/trace-event-info.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index 535176d..407fd65 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -253,6 +253,8 @@ static void read_header_files(void)
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);
@@ -271,6 +273,7 @@ static void read_header_files(void)
if (size != check_size)
die("wrong size for '%s'", path);
put_tracing_file(path);
+ close(fd);
}

static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
@@ -337,6 +340,7 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)

free(format);
}
+ closedir(dir);
}

static void read_ftrace_files(struct tracepoint_path *tps)
@@ -407,6 +411,7 @@ static void read_event_files(struct tracepoint_path *tps)
free(sys);
}

+ closedir(dir);
put_tracing_file(path);
}