perf tools: Cleanup filesystem api

Following commit changed parse_events_add_pmu interface:
36adec85a86f perf tools: Change parse_events_add_pmu interface

but forgot to change one caller. Because of lessen compilation rules for the bison parser, the compiler did not warn on that.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/parse-events.y | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/perf/util/parse-events.y +1 -1

index 591905a02b92..9cd70819c795 100644
@@ -255,7 +255,7 @@ PE_PMU_EVENT_PRE '-' PE_PMU_EVENT_SUF sep_dc
 	list_add_tail(&term->list, head);
 
 	ALLOC_LIST(list);
-	ABORT_ON(parse_events_add_pmu(list, &data->idx, "cpu", head));
+	ABORT_ON(parse_events_add_pmu(data, list, "cpu", head));
 	parse_events__free_terms(head);
 	$$ = list;
 }

Signature · 1 line

2.4.3

Em Wed, Sep 02, 2015 at 09:56:31AM +0200, Jiri Olsa escreveu:

Quoted 5 lines

Following commit changed parse_events_add_pmu interface:
36adec85a86f perf tools: Change parse_events_add_pmu interface

but forgot to change one caller. Because of lessen compilation rules for the bison parser, the compiler did not warn on that.

Humm, wonder what can be done to improve this situation... No ideas?

Applying to perf/urgent,

Moving tracing_path interface into api/fs/tracing_path.c out of util.c. It seems generic enough to be used by others, and I couldn't think of better place.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/Build             |  1 +
 tools/lib/api/fs/tracing_path.c    | 83 ++++++++++++++++++++++++++++++++++++++
 tools/lib/api/fs/tracing_path.h    | 13 ++++++
 tools/perf/perf.c                  |  2 +-
 tools/perf/util/parse-events.c     |  2 +-
 tools/perf/util/trace-event-info.c |  2 +-
 tools/perf/util/trace-event.c      |  1 +
 tools/perf/util/util.c             | 70 --------------------------------
 tools/perf/util/util.h             |  6 ---
 9 files changed, 101 insertions(+), 79 deletions(-)
 create mode 100644 tools/lib/api/fs/tracing_path.c
 create mode 100644 tools/lib/api/fs/tracing_path.h

Quoted 6 lines

Moving tracing_path interface into api/fs/tracing_path.c out of util.c. It seems generic enough to be used by others, and I couldn't think of better place.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

Quoted 13 lines

---
 tools/lib/api/fs/Build             |  1 +
 tools/lib/api/fs/tracing_path.c    | 83 ++++++++++++++++++++++++++++++++++++++
 tools/lib/api/fs/tracing_path.h    | 13 ++++++
 tools/perf/perf.c                  |  2 +-
 tools/perf/util/parse-events.c     |  2 +-
 tools/perf/util/trace-event-info.c |  2 +-
 tools/perf/util/trace-event.c      |  1 +
 tools/perf/util/util.c             | 70 --------------------------------
 tools/perf/util/util.h             |  6 ---
 9 files changed, 101 insertions(+), 79 deletions(-)
 create mode 100644 tools/lib/api/fs/tracing_path.c
 create mode 100644 tools/lib/api/fs/tracing_path.h

Reviewed-by Matt Fleming <[email protected]>

--
Matt Fleming, Intel Open Source Technology Center

Moving debugfs__strerror_open out of api/fs/debugfs.c, because it's not debugfs specific. It'll be changed to consider tracefs mount as well in following patches.

Renaming it into tracing_path__strerror_open_tp to fit into the namespace. No functional change is intended.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/debugfs.c      | 52 ---------------------------------------
 tools/lib/api/fs/tracing_path.c | 54 +++++++++++++++++++++++++++++++++++++++++
 tools/lib/api/fs/tracing_path.h |  3 +++
 tools/perf/builtin-trace.c      |  5 ++--
 4 files changed, 60 insertions(+), 54 deletions(-)

tools/lib/api/fs/debugfs.c +0 -52

index eb7cf4d18f8a..c707cfb32782 100644
@@ -75,55 +75,3 @@ char *debugfs_mount(const char *mountpoint)
 out:
 	return debugfs_mountpoint;
 }
-
-int debugfs__strerror_open(int err, char *buf, size_t size, const char *filename)
-{
-	char sbuf[128];
-
-	switch (err) {
-	case ENOENT:
-		if (debugfs_found) {
-			snprintf(buf, size,
-				 "Error:\tFile %s/%s not found.\n"
-				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
-				 debugfs_mountpoint, filename);
-			break;
-		}
-		snprintf(buf, size, "%s",
-			 "Error:\tUnable to find debugfs\n"
-			 "Hint:\tWas your kernel compiled with debugfs support?\n"
-			 "Hint:\tIs the debugfs filesystem mounted?\n"
-			 "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");
-		break;
-	case EACCES: {
-		const char *mountpoint = debugfs_mountpoint;
-
-		if (!access(debugfs_mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
-			const char *tracefs_mntpoint = tracefs_find_mountpoint();
-
-			if (tracefs_mntpoint)
-				mountpoint = tracefs_mntpoint;
-		}
-
-		snprintf(buf, size,
-			 "Error:\tNo permissions to read %s/%s\n"
-			 "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n",
-			 debugfs_mountpoint, filename, mountpoint);
-	}
-		break;
-	default:
-		snprintf(buf, size, "%s", strerror_r(err, sbuf, sizeof(sbuf)));
-		break;
-	}
-
-	return 0;
-}
-
-int debugfs__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
-{
-	char path[PATH_MAX];
-
-	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
-
-	return debugfs__strerror_open(err, buf, size, path);
-}

tools/lib/api/fs/tracing_path.c +54 -0

index 1fd6e1f99234..3b3e4f5fc50b 100644
@@ -5,6 +5,8 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <errno.h>
+#include <unistd.h>
 #include "debugfs.h"
 #include "tracefs.h"
 

@@ -81,3 +83,55 @@ void put_tracing_file(char *file)
 {
 	free(file);
 }
+
+static int strerror_open(int err, char *buf, size_t size, const char *filename)
+{
+	char sbuf[128];
+
+	switch (err) {
+	case ENOENT:
+		if (debugfs_configured()) {
+			snprintf(buf, size,
+				 "Error:\tFile %s/%s not found.\n"
+				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
+				 debugfs_mountpoint, filename);
+			break;
+		}
+		snprintf(buf, size, "%s",
+			 "Error:\tUnable to find debugfs\n"
+			 "Hint:\tWas your kernel compiled with debugfs support?\n"
+			 "Hint:\tIs the debugfs filesystem mounted?\n"
+			 "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");
+		break;
+	case EACCES: {
+		const char *mountpoint = debugfs_mountpoint;
+
+		if (!access(debugfs_mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
+			const char *tracefs_mntpoint = tracefs_find_mountpoint();
+
+			if (tracefs_mntpoint)
+				mountpoint = tracefs_mntpoint;
+		}
+
+		snprintf(buf, size,
+			 "Error:\tNo permissions to read %s/%s\n"
+			 "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n",
+			 debugfs_mountpoint, filename, mountpoint);
+	}
+		break;
+	default:
+		snprintf(buf, size, "%s", strerror_r(err, sbuf, sizeof(sbuf)));
+		break;
+	}
+
+	return 0;
+}
+
+int tracing_path__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name)
+{
+	char path[PATH_MAX];
+
+	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
+
+	return strerror_open(err, buf, size, path);
+}

tools/lib/api/fs/tracing_path.h +3 -0

index b132dc599fe5..3f233ac70b6f 100644
@@ -1,6 +1,8 @@
 #ifndef __API_FS_TRACING_PATH_H
 #define __API_FS_TRACING_PATH_H
 
+#include <linux/types.h>
+
 extern char tracing_path[];
 extern char tracing_events_path[];
 

@@ -10,4 +12,5 @@ const char *tracing_path_mount(void);
 char *get_tracing_file(const char *name);
 void put_tracing_file(char *file);
 
+int tracing_path__strerror_open_tp(int err, char *buf, size_t size, const char *sys, const char *name);
 #endif /* __API_FS_TRACING_PATH_H */

tools/perf/builtin-trace.c +3 -2

index 4e3abba03062..215653274102 100644
@@ -17,6 +17,7 @@
  */
 
 #include <traceevent/event-parse.h>
+#include <api/fs/tracing_path.h>
 #include "builtin.h"
 #include "util/color.h"
 #include "util/debug.h"

@@ -2686,11 +2687,11 @@ out_delete_evlist:
 	char errbuf[BUFSIZ];
 
 out_error_sched_stat_runtime:
-	debugfs__strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
+	tracing_path__strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
 	goto out_error;
 
 out_error_raw_syscalls:
-	debugfs__strerror_open_tp(errno, errbuf, sizeof(errbuf), "raw_syscalls", "sys_(enter|exit)");
+	tracing_path__strerror_open_tp(errno, errbuf, sizeof(errbuf), "raw_syscalls", "sys_(enter|exit)");
 	goto out_error;
 
 out_error_mmap:

Signature · 1 line

2.4.3

Moving debugfs__strerror_open out of api/fs/debugfs.c, because it's not debugfs specific. It'll be changed to consider tracefs mount as well in following patches.

Renaming it into tracing_path__strerror_open_tp to fit into the namespace. No functional change is intended.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/debugfs.c      | 52 ---------------------------------------
 tools/lib/api/fs/tracing_path.c | 54 +++++++++++++++++++++++++++++++++++++++++
 tools/lib/api/fs/tracing_path.h |  3 +++
 tools/perf/builtin-trace.c      |  5 ++--
 4 files changed, 60 insertions(+), 54 deletions(-)

Em Wed, Sep 02, 2015 at 09:56:35AM +0200, Jiri Olsa escreveu:
Humm, here I think that "_path" may look excessive... But tracing_path__ better than debugfs__, so its progress, applying, if we get a better name, we can change this later.

- Arnaldo

2015-09-02 9:12 GMT-04:00 Arnaldo Carvalho de Melo <[email protected]>:

Quoted 6 lines

Em Wed, Sep 02, 2015 at 09:56:35AM +0200, Jiri Olsa escreveu:
Humm, here I think that "_path" may look excessive... But tracing_path__ better than debugfs__, so its progress, applying, if we get a better name, we can change this later.

- Arnaldo

I agree that "tracing_path" seems a bit heavy. Why not using tracefs here like for the previous changes?

it's not fs per say, it's 'combination,choice' of debugfs and tracefs that provides 'path' for tracing ;-)

Moving debugfs__strerror_open out of api/fs/debugfs.c, because it's not debugfs specific. It'll be changed to consider tracefs mount as well in following patches.

Renaming it into tracing_path__strerror_open_tp to fit into the namespace. No functional change is intended.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/debugfs.c      | 52 ---------------------------------------
 tools/lib/api/fs/tracing_path.c | 54 +++++++++++++++++++++++++++++++++++++++++
 tools/lib/api/fs/tracing_path.h |  3 +++
 tools/perf/builtin-trace.c      |  5 ++--
 4 files changed, 60 insertions(+), 54 deletions(-)

Making tracing_path__strerror_open_tp message generic by mentioning

both debugfs/tracefs words in error message plus the tracing_path instead of debugfs_mountpoint.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/tracing_path.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

tools/lib/api/fs/tracing_path.c +0 -0

index 3b3e4f5fc50b..b0ee3b3acef0 100644

tools/lib/api/fs/tracing_path.c +8 -8

 			break;
 		}
 		snprintf(buf, size, "%s",
-			 "Error:\tUnable to find debugfs\n"
-			 "Hint:\tWas your kernel compiled with debugfs support?\n"
-			 "Hint:\tIs the debugfs filesystem mounted?\n"
+			 "Error:\tUnable to find debugfs/tracefs\n"
+			 "Hint:\tWas your kernel compiled with debugfs/tracefs support?\n"
+			 "Hint:\tIs the debugfs/tracefs filesystem mounted?\n"
 			 "Hint:\tTry 'sudo mount -t debugfs nodev /sys/kernel/debug'");
 		break;
 	case EACCES: {
-		const char *mountpoint = debugfs_mountpoint;
+		const char *mountpoint = debugfs_find_mountpoint();
 
-		if (!access(debugfs_mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
+		if (!access(mountpoint, R_OK) && strncmp(filename, "tracing/", 8) == 0) {
 			const char *tracefs_mntpoint = tracefs_find_mountpoint();
 
 			if (tracefs_mntpoint)
-				mountpoint = tracefs_mntpoint;
+				mountpoint = tracefs_find_mountpoint();
 		}
 
 		snprintf(buf, size,
 			 "Error:\tNo permissions to read %s/%s\n"
 			 "Hint:\tTry 'sudo mount -o remount,mode=755 %s'\n",
-			 debugfs_mountpoint, filename, mountpoint);
+			 tracing_events_path, filename, mountpoint);
 	}
 		break;
 	default:

@@ -131,7 +131,7 @@ int tracing_path__strerror_open_tp(int err, char *buf, size_t size, const char *
 {
 	char path[PATH_MAX];
 
-	snprintf(path, PATH_MAX, "tracing/events/%s/%s", sys, name ?: "*");
+	snprintf(path, PATH_MAX, "%s/%s", sys, name ?: "*");
 
 	return strerror_open(err, buf, size, path);
 }

Signature · 1 line

2.4.3

Em Wed, Sep 02, 2015 at 09:56:36AM +0200, Jiri Olsa escreveu:

Quoted 1 line

Making tracing_path__strerror_open_tp message generic by mentioning

What means "making message generic"? What is the current behaviour you think is problematic. what is the new behaviour ad why do you think it is better?

The test for ENOENT became confusing, i.e. since this was a test for "tracefs", if debugfs_configured() returned true, i.e. debugfs _was_ found in the system, then, the message makes sense, even if probably could be made better, i.e. isn't true that if CONFIG_DEBUGFS is configured and furthermore, debugfs_configure() returns true, then it should be something like CONFIG_TRACEFS that needs enabling?

I applied all patches before this one, BTW.

- Arnaldo

Quoted 15 lines

both debugfs/tracefs words in error message plus the tracing_path instead of debugfs_mountpoint.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/tracing_path.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

tools/lib/api/fs/tracing_path.c +0 -0

index 3b3e4f5fc50b..b0ee3b3acef0 100644
@@ -90,33 +90,33 @@ static int strerror_open(int err, char *buf, size_t size, const char *filename)
 

tools/lib/api/fs/tracing_path.c static int strerror_open(int err, char *buf, size_t size, const char *filename) · line 97

Quoted 9 lines

tools/lib/api/fs/tracing_path.c +2 -2

 	switch (err) {
 	case ENOENT:
-		if (debugfs_configured()) {
+		if (debugfs_configured() || tracefs_configured()) {
 			snprintf(buf, size,
 				 "Error:\tFile %s/%s not found.\n"
 				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
-				 debugfs_mountpoint, filename);
+				 tracing_events_path, filename);

Humm

Quoted 33 lines

Em Wed, Sep 02, 2015 at 09:56:36AM +0200, Jiri Olsa escreveu:

Quoted 1 line

Making tracing_path__strerror_open_tp message generic by mentioning

What means "making message generic"? What is the current behaviour you think is problematic. what is the new behaviour ad why do you think it is better?

The test for ENOENT became confusing, i.e. since this was a test for "tracefs", if debugfs_configured() returned true, i.e. debugfs _was_ found in the system, then, the message makes sense, even if probably could be made better, i.e. isn't true that if CONFIG_DEBUGFS is configured and furthermore, debugfs_configure() returns true, then it should be something like CONFIG_TRACEFS that needs enabling?

I applied all patches before this one, BTW.

- Arnaldo

Quoted 15 lines

both debugfs/tracefs words in error message plus the tracing_path instead of debugfs_mountpoint.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/tracing_path.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

tools/lib/api/fs/tracing_path.c +0 -0

index 3b3e4f5fc50b..b0ee3b3acef0 100644
@@ -90,33 +90,33 @@ static int strerror_open(int err, char *buf, size_t size, const char *filename)
 

Quoted 11 lines

tools/lib/api/fs/tracing_path.c static int strerror_open(int err, char *buf, size_t size, const char *filename) · line 97

Quoted 9 lines

tools/lib/api/fs/tracing_path.c +2 -2

 	switch (err) {
 	case ENOENT:
-		if (debugfs_configured()) {
+		if (debugfs_configured() || tracefs_configured()) {
 			snprintf(buf, size,
 				 "Error:\tFile %s/%s not found.\n"
 				 "Hint:\tPerhaps this kernel misses some CONFIG_ setting to enable this feature?.\n",
-				 debugfs_mountpoint, filename);
+				 tracing_events_path, filename);

Humm

we will get here if we can't find the tracepoint, but one of debugfs or tracefs is configured, which means you probably want some tracepoint which wasn't compiled in your kernel

before it did not take into account we could have tracefs configured thats what other changes in here are about, to consider tracefs mount

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs) interface that tries to mount the filesystem in case no mount is found.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/fs.c | 44 +++++++++++++++++++++++++++++++++++++++-----
 tools/lib/api/fs/fs.h | 15 +++++++++++----
 2 files changed, 50 insertions(+), 9 deletions(-)

tools/lib/api/fs/fs.c +39 -5

index ef16d2a83a27..bc93baf33fff 100644
@@ -9,6 +9,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <unistd.h>
+#include <sys/mount.h>
 
 #include "debugfs.h"
 #include "fs.h"

@@ -215,16 +216,49 @@ static const char *fs__mountpoint(int idx)
        return fs__get_mountpoint(fs);
 }
 
-#define FS__MOUNTPOINT(name, idx)      \
+static const char *mount_overload(struct fs *fs)
+{
+       size_t name_len = strlen(fs->name);
+       /* "PERF_" + name + "_ENVIRONMENT" + '\0' */
+       char upper_name[5 + name_len + 12 + 1];
+
+       snprintf(upper_name, name_len, "PERF_%s_ENVIRONMENT", fs->name);
+       mem_toupper(upper_name, name_len);
+
+       return getenv(upper_name) ?: *fs->mounts;
+}
+
+static const char *fs__mount(int idx)
+{
+       struct fs *fs = &fs__entries[idx];
+       const char *mountpoint;
+
+       if (fs__mountpoint(idx))
+               return (const char *)fs->path;
+
+       mountpoint = mount_overload(fs);
+
+       if (mount(NULL, mountpoint, fs->name, 0, NULL) < 0)
+               return NULL;
+
+       return fs__check_mounts(fs) ? fs->path : NULL;
+}
+
+#define FS(name, idx)                  \
 const char *name##__mountpoint(void)   \
 {                                      \
        return fs__mountpoint(idx);     \
+}                                      \
+                                       \
+const char *name##__mount(void)                \
+{                                      \
+       return fs__mount(idx);          \
 }
 
-FS__MOUNTPOINT(sysfs,   FS__SYSFS);
-FS__MOUNTPOINT(procfs,  FS__PROCFS);
-FS__MOUNTPOINT(debugfs, FS__DEBUGFS);
-FS__MOUNTPOINT(tracefs, FS__TRACEFS);
+FS(sysfs,   FS__SYSFS);
+FS(procfs,  FS__PROCFS);
+FS(debugfs, FS__DEBUGFS);
+FS(tracefs, FS__TRACEFS);
 
 int filename__read_int(const char *filename, int *value)
 {

tools/lib/api/fs/fs.h +11 -5

index 9013227ae0d1..a9627ea5e6ae 100644
@@ -9,10 +9,17 @@
 #define PATH_MAX 4096
 #endif
 
-const char *sysfs__mountpoint(void);
-const char *procfs__mountpoint(void);
-const char *debugfs__mountpoint(void);
-const char *tracefs__mountpoint(void);
+#define FS(name)                               \
+       const char *name##__mountpoint(void);   \
+       const char *name##__mount(void);
+
+FS(sysfs)
+FS(procfs)
+FS(debugfs)
+FS(tracefs)
+
+#undef FS
+
 
 int filename__read_int(const char *filename, int *value);
 int sysctl__read_int(const char *sysctl, int *value);
--

2.4.3

2015-09-02 3:56 GMT-04:00 Jiri Olsa <[email protected]>:

Quoted 9 lines

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs) interface that tries to mount the filesystem in case no mount is found.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/fs.c | 44 +++++++++++++++++++++++++++++++++++++++-----
 tools/lib/api/fs/fs.h | 15 +++++++++++----
 2 files changed, 50 insertions(+), 9 deletions(-)

Why automatic mounting the **fs filesystems?
Isn't it better to let the user decide when and where to mount it if s/he forgot to do it before? Forcing a mount is also potentially forcing to umount then mount when mounting has been forgotten before.

2015-09-02 3:56 GMT-04:00 Jiri Olsa <[email protected]>:

Quoted 9 lines

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs) interface that tries to mount the filesystem in case no mount is found.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/fs.c | 44 +++++++++++++++++++++++++++++++++++++++-----
 tools/lib/api/fs/fs.h | 15 +++++++++++----
 2 files changed, 50 insertions(+), 9 deletions(-)

Why automatic mounting the **fs filesystems?
Isn't it better to let the user decide when and where to mount it if s/he forgot to do it before? Forcing a mount is also potentially forcing to umount then mount when mounting has been forgotten before.

Quoted 98 lines

tools/lib/api/fs/fs.c +39 -5

index ef16d2a83a27..bc93baf33fff 100644
@@ -9,6 +9,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <unistd.h>
+#include <sys/mount.h>
 
 #include "debugfs.h"
 #include "fs.h"

@@ -215,16 +216,49 @@ static const char *fs__mountpoint(int idx)
        return fs__get_mountpoint(fs);
 }
 
-#define FS__MOUNTPOINT(name, idx)      \
+static const char *mount_overload(struct fs *fs)
+{
+       size_t name_len = strlen(fs->name);
+       /* "PERF_" + name + "_ENVIRONMENT" + '\0' */
+       char upper_name[5 + name_len + 12 + 1];
+
+       snprintf(upper_name, name_len, "PERF_%s_ENVIRONMENT", fs->name);
+       mem_toupper(upper_name, name_len);
+
+       return getenv(upper_name) ?: *fs->mounts;
+}
+
+static const char *fs__mount(int idx)
+{
+       struct fs *fs = &fs__entries[idx];
+       const char *mountpoint;
+
+       if (fs__mountpoint(idx))
+               return (const char *)fs->path;
+
+       mountpoint = mount_overload(fs);
+
+       if (mount(NULL, mountpoint, fs->name, 0, NULL) < 0)
+               return NULL;
+
+       return fs__check_mounts(fs) ? fs->path : NULL;
+}
+
+#define FS(name, idx)                  \
 const char *name##__mountpoint(void)   \
 {                                      \
        return fs__mountpoint(idx);     \
+}                                      \
+                                       \
+const char *name##__mount(void)                \
+{                                      \
+       return fs__mount(idx);          \
 }
 
-FS__MOUNTPOINT(sysfs,   FS__SYSFS);
-FS__MOUNTPOINT(procfs,  FS__PROCFS);
-FS__MOUNTPOINT(debugfs, FS__DEBUGFS);
-FS__MOUNTPOINT(tracefs, FS__TRACEFS);
+FS(sysfs,   FS__SYSFS);
+FS(procfs,  FS__PROCFS);
+FS(debugfs, FS__DEBUGFS);
+FS(tracefs, FS__TRACEFS);
 
 int filename__read_int(const char *filename, int *value)
 {

tools/lib/api/fs/fs.h +11 -5

index 9013227ae0d1..a9627ea5e6ae 100644
@@ -9,10 +9,17 @@
 #define PATH_MAX 4096
 #endif
 
-const char *sysfs__mountpoint(void);
-const char *procfs__mountpoint(void);
-const char *debugfs__mountpoint(void);
-const char *tracefs__mountpoint(void);
+#define FS(name)                               \
+       const char *name##__mountpoint(void);   \
+       const char *name##__mount(void);
+
+FS(sysfs)
+FS(procfs)
+FS(debugfs)
+FS(tracefs)
+
+#undef FS
+
 
 int filename__read_int(const char *filename, int *value);
 int sysctl__read_int(const char *sysctl, int *value);
--

2.4.3

Quoted 11 lines · Raphaël Beamonte

2015-09-02 3:56 GMT-04:00 Jiri Olsa <[email protected]>:

Quoted 9 lines

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs) interface that tries to mount the filesystem in case no mount is found.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/fs.c | 44 +++++++++++++++++++++++++++++++++++++++-----
 tools/lib/api/fs/fs.h | 15 +++++++++++----
 2 files changed, 50 insertions(+), 9 deletions(-)

Quoted 4 lines

Why automatic mounting the **fs filesystems?
Isn't it better to let the user decide when and where to mount it if s/he forgot to do it before? Forcing a mount is also potentially forcing to umount then mount when mounting has been forgotten before.

well, I guess at some point we decided this was a good idea to make perf more user friendly

AFAICS we do this only for debugfs and tracefs.. procfs and sysfs are mounted most of the time anyway

we could track and unmount on exit, but I'd say let's have someone complaining about it first ;-)

Quoted 2 lines

AFAICS we do this only for debugfs and tracefs.. procfs and sysfs are mounted most of the time anyway

Note, if sysfs isn't already mounted, than nothing will get mounted by perf. As sysfs is a requirement to get to /sys/kernel/trace and debug.

I will also add that the reason the /sys/kernel/debug directory is created when DEBUGFS is enabled, was specifically so that tools had a place to automatically mount it when needed. That was the entire purpose of that directory.

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs) interface that tries to mount the filesystem in case no mount is found.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/fs.c | 44 +++++++++++++++++++++++++++++++++++++++-----
 tools/lib/api/fs/fs.h | 15 +++++++++++----
 2 files changed, 50 insertions(+), 9 deletions(-)

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs) interface that returns bool state of the filesystem mount:
true - mounted, false - not mounted

It will not try to mount the filesystem.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/fs.c | 23 ++++++++++++++---------
 tools/lib/api/fs/fs.h |  5 ++++-
 2 files changed, 18 insertions(+), 10 deletions(-)

tools/lib/api/fs/fs.c +14 -9

index bc93baf33fff..8afe08a99bc6 100644
@@ -244,15 +244,20 @@ static const char *fs__mount(int idx)
 	return fs__check_mounts(fs) ? fs->path : NULL;
 }
 
-#define FS(name, idx)			\
-const char *name##__mountpoint(void)	\
-{					\
-	return fs__mountpoint(idx);	\
-}					\
-					\
-const char *name##__mount(void)		\
-{					\
-	return fs__mount(idx);		\
+#define FS(name, idx)				\
+const char *name##__mountpoint(void)		\
+{						\
+	return fs__mountpoint(idx);		\
+}						\
+						\
+const char *name##__mount(void)			\
+{						\
+	return fs__mount(idx);			\
+}						\
+						\
+bool name##__configured(void)			\
+{						\
+	return name##__mountpoint() != NULL;	\
 }
 
 FS(sysfs,   FS__SYSFS);

tools/lib/api/fs/fs.h +4 -1

index a9627ea5e6ae..f654bcb99d1e 100644
@@ -1,6 +1,8 @@
 #ifndef __API_FS__
 #define __API_FS__
 
+#include <stdbool.h>
+
 /*
  * On most systems <limits.h> would have given us this, but  not on some systems
  * (e.g. GNU/Hurd).

@@ -11,7 +13,8 @@
 
 #define FS(name)				\
 	const char *name##__mountpoint(void);	\
-	const char *name##__mount(void);
+	const char *name##__mount(void);	\
+	bool name##__configured(void);		\
 
 FS(sysfs)
 FS(procfs)

Signature · 1 line

2.4.3

Em Wed, Sep 02, 2015 at 09:56:42AM +0200, Jiri Olsa escreveu:

Quoted 3 lines

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs) interface that returns bool state of the filesystem mount:
true - mounted, false - not mounted

Well, why not take the opportunity and call it... name__mounted()? "__configured()" is too vague...

Also there was some cut'n'pasting mistake here... if you agree I can fix it all up and make this change, well, I'm doing it on my local tree, holler if you have any objection :)

- Arnaldo

Quoted 1 line

It will not try to mount the filesystem.

No FSTYPE__configured() (to-be FSTYPE__mounted()) doesn't mount anything, that is what FSTYPE__mount() does :-)

- Arnaldo

Quoted 66 lines

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/api/fs/fs.c | 23 ++++++++++++++---------
 tools/lib/api/fs/fs.h |  5 ++++-
 2 files changed, 18 insertions(+), 10 deletions(-)

tools/lib/api/fs/fs.c +14 -9

index bc93baf33fff..8afe08a99bc6 100644
@@ -244,15 +244,20 @@ static const char *fs__mount(int idx)
 	return fs__check_mounts(fs) ? fs->path : NULL;
 }
 
-#define FS(name, idx)			\
-const char *name##__mountpoint(void)	\
-{					\
-	return fs__mountpoint(idx);	\
-}					\
-					\
-const char *name##__mount(void)		\
-{					\
-	return fs__mount(idx);		\
+#define FS(name, idx)				\
+const char *name##__mountpoint(void)		\
+{						\
+	return fs__mountpoint(idx);		\
+}						\
+						\
+const char *name##__mount(void)			\
+{						\
+	return fs__mount(idx);			\
+}						\
+						\
+bool name##__configured(void)			\
+{						\
+	return name##__mountpoint() != NULL;	\
 }
 
 FS(sysfs,   FS__SYSFS);

tools/lib/api/fs/fs.h +4 -1

index a9627ea5e6ae..f654bcb99d1e 100644
@@ -1,6 +1,8 @@
 #ifndef __API_FS__
 #define __API_FS__
 
+#include <stdbool.h>
+
 /*
  * On most systems <limits.h> would have given us this, but  not on some systems
  * (e.g. GNU/Hurd).

@@ -11,7 +13,8 @@
 
 #define FS(name)				\
 	const char *name##__mountpoint(void);	\
-	const char *name##__mount(void);
+	const char *name##__mount(void);	\
+	bool name##__configured(void);		\
 
 FS(sysfs)
 FS(procfs)

Signature · 1 line

2.4.3

Em Wed, Sep 02, 2015 at 09:56:42AM +0200, Jiri Olsa escreveu:

Quoted 3 lines

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs) interface that returns bool state of the filesystem mount:
true - mounted, false - not mounted

Well, why not take the opportunity and call it... name__mounted()? "__configured()" is too vague...

Also there was some cut'n'pasting mistake here... if you agree I can fix it all up and make this change, well, I'm doing it on my local tree, holler if you have any objection :)

- Arnaldo

Quoted 1 line

It will not try to mount the filesystem.

No FSTYPE__configured() (to-be FSTYPE__mounted()) doesn't mount anything, that is what FSTYPE__mount() does :-)

Quoted 2 lines

Em Wed, Sep 02, 2015 at 09:56:42AM +0200, Jiri Olsa escreveu:

Quoted 1 line

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs)

Quoted 5 lines

Quoted 2 lines

interface that returns bool state of the filesystem mount:
true - mounted, false - not mounted

Well, why not take the opportunity and call it... name__mounted()? "__configured()" is too vague...

I mirrored the debugfs/tracefs object interface
we can have another patch changing that

Quoted 9 lines

Also there was some cut'n'pasting mistake here... if you agree I can fix it all up and make this change, well, I'm doing it on my local tree, holler if you have any objection :)

Quoted 1 line

It will not try to mount the filesystem.

No FSTYPE__configured() (to-be FSTYPE__mounted()) doesn't mount anything, that is what FSTYPE__mount() does :-)

I stated it just to be clear ;-)

Em Wed, Sep 02, 2015 at 03:46:25PM +0200, Jiri Olsa escreveu:

Quoted 3 lines · Arnaldo Carvalho de Melo

Quoted 2 lines

Em Wed, Sep 02, 2015 at 09:56:42AM +0200, Jiri Olsa escreveu:

Quoted 1 line

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs)

The cut'n'paste error I mentioned was in the above line, BTW.

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

interface that returns bool state of the filesystem mount:
true - mounted, false - not mounted

Quoted 5 lines

Quoted 2 lines

Well, why not take the opportunity and call it... name__mounted()? "__configured()" is too vague...

I mirrored the debugfs/tracefs object interface
we can have another patch changing that

Ok, agreed, will leave it as configured() and then we can change it.

Quoted 11 lines

Quoted 9 lines

Also there was some cut'n'pasting mistake here... if you agree I can fix it all up and make this change, well, I'm doing it on my local tree, holler if you have any objection :)

Quoted 1 line

It will not try to mount the filesystem.

No FSTYPE__configured() (to-be FSTYPE__mounted()) doesn't mount anything, that is what FSTYPE__mount() does :-)

I stated it just to be clear ;-)

Sure, somehow the cut'n'paste error on the first line flipped a bit in my brain, sorry ;-\

- arnaldo

Em Wed, Sep 02, 2015 at 11:17:59AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 13 lines

Em Wed, Sep 02, 2015 at 03:46:25PM +0200, Jiri Olsa escreveu:

Quoted 3 lines · Arnaldo Carvalho de Melo

Quoted 2 lines

Em Wed, Sep 02, 2015 at 09:56:42AM +0200, Jiri Olsa escreveu:

Quoted 1 line

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs)

The cut'n'paste error I mentioned was in the above line, BTW.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

interface that returns bool state of the filesystem mount:
true - mounted, false - not mounted

Well, why not take the opportunity and call it... name__mounted()? "__configured()" is too vague...

Quoted 4 lines

Quoted 2 lines

I mirrored the debugfs/tracefs object interface
we can have another patch changing that

Ok, agreed, will leave it as configured() and then we can change it.

What I have is at my tmp.perf/core branch, out for lunch.

Quoted 14 lines

Em Wed, Sep 02, 2015 at 11:17:59AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 13 lines

Em Wed, Sep 02, 2015 at 03:46:25PM +0200, Jiri Olsa escreveu:

Quoted 3 lines · Arnaldo Carvalho de Melo

Quoted 2 lines

Em Wed, Sep 02, 2015 at 09:56:42AM +0200, Jiri Olsa escreveu:

Quoted 1 line

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs)

The cut'n'paste error I mentioned was in the above line, BTW.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

interface that returns bool state of the filesystem mount:
true - mounted, false - not mounted

Well, why not take the opportunity and call it... name__mounted()? "__configured()" is too vague...

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

I mirrored the debugfs/tracefs object interface
we can have another patch changing that

Ok, agreed, will leave it as configured() and then we can change it.

What I have is at my tmp.perf/core branch, out for lunch.

I can see all of it in your's perf/core apart from:

Applying: tools lib api: Make tracing_path_strerror_open message generic
Applying: tools lib api: Replace debugfs/tracefs objects interface with fs.c
Applying: tools lib api: Remove debugfs, tracefs and findfs objects
Applying: perf tools: Switch to tracing_path interface on appropriate places

should I repost or do you plan to take it? I didn't get any comments on those

Em Fri, Sep 04, 2015 at 09:08:45AM +0200, Jiri Olsa escreveu:

Quoted 29 lines · Arnaldo Carvalho de Melo

Quoted 14 lines

Em Wed, Sep 02, 2015 at 11:17:59AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 13 lines

Em Wed, Sep 02, 2015 at 03:46:25PM +0200, Jiri Olsa escreveu:

Quoted 3 lines · Arnaldo Carvalho de Melo

Quoted 2 lines

Em Wed, Sep 02, 2015 at 09:56:42AM +0200, Jiri Olsa escreveu:

Quoted 1 line

Adding name__mount (where name is in sysfs,procfs,debugfs,tracefs)

The cut'n'paste error I mentioned was in the above line, BTW.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

interface that returns bool state of the filesystem mount:
true - mounted, false - not mounted

Well, why not take the opportunity and call it... name__mounted()? "__configured()" is too vague...

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

I mirrored the debugfs/tracefs object interface
we can have another patch changing that

Ok, agreed, will leave it as configured() and then we can change it.

What I have is at my tmp.perf/core branch, out for lunch.

I can see all of it in your's perf/core apart from:

Applying: tools lib api: Make tracing_path_strerror_open message generic
Applying: tools lib api: Replace debugfs/tracefs objects interface with fs.c
Applying: tools lib api: Remove debugfs, tracefs and findfs objects
Applying: perf tools: Switch to tracing_path interface on appropriate places

Quoted 1 line

should I repost or do you plan to take it? I didn't get any comments on those

I'll get back to those, its just that the number of patches in my perf/core branch was getting too high, better flush it a bit, will continue.

If you can just make sure that a rebase on top of my perf/core goes well, that helps.