[PATCH 0/3] perf tools: Add perf_event_max_sample_rate freq check

[PATCH 0/3] perf tools: Add perf_event_max_sample_rate freq check

[Jiri Olsa]

hi,
adding the check for maximum allowed frequency rate
defined in perf_event_max_sample_rate.

Plus procfs mountpoint reading code. Reachable here:
  git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  perf/cc

jirka   

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Joe Mario <jmario@redhat.com>
Cc: Don Zickus <dzickus@redhat.com>
---
Jiri Olsa (3):
      perf tools: Factor sysfs code into generic fs object
      perf tools: Add procfs support into fs object
      perf tools: Check maximum frequency rate for record/top

 tools/perf/Makefile.perf              |   4 ++--
 tools/perf/builtin-record.c           |   3 +++
 tools/perf/builtin-top.c              |   3 +++
 tools/perf/tests/parse-events.c       |   2 +-
 tools/perf/util/cpumap.c              |   2 +-
 tools/perf/util/evlist.h              |   1 +
 tools/perf/util/fs.c                  | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/fs.h                  |   7 ++++++
 tools/perf/util/include/linux/magic.h |   4 ++++
 tools/perf/util/pmu.c                 |   2 +-
 tools/perf/util/python-ext-sources    |   2 +-
 tools/perf/util/record.c              |  35 ++++++++++++++++++++++++++++
 tools/perf/util/sysfs.c               |  60 -----------------------------------------------
 tools/perf/util/sysfs.h               |   6 -----
 14 files changed, 178 insertions(+), 72 deletions(-)
 create mode 100644 tools/perf/util/fs.c
 create mode 100644 tools/perf/util/fs.h
 delete mode 100644 tools/perf/util/sysfs.c
 delete mode 100644 tools/perf/util/sysfs.h

[PATCH 1/3] perf tools: Factor sysfs code into generic fs object

[Jiri Olsa]

Moving sysfs code into generic fs object and preparing
it to carry procfs support.

This should be merged with tools/lib/lk/debugfs.c at
some point in the future.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Makefile.perf           |   4 +-
 tools/perf/tests/parse-events.c    |   2 +-
 tools/perf/util/cpumap.c           |   2 +-
 tools/perf/util/fs.c               | 107 +++++++++++++++++++++++++++++++++++++
 tools/perf/util/fs.h               |   6 +++
 tools/perf/util/pmu.c              |   2 +-
 tools/perf/util/python-ext-sources |   2 +-
 tools/perf/util/sysfs.c            |  60 ---------------------
 tools/perf/util/sysfs.h            |   6 ---
 9 files changed, 119 insertions(+), 72 deletions(-)
 create mode 100644 tools/perf/util/fs.c
 create mode 100644 tools/perf/util/fs.h
 delete mode 100644 tools/perf/util/sysfs.c
 delete mode 100644 tools/perf/util/sysfs.h

diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index bc7cfa1..f5d3251 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -242,7 +242,7 @@ LIB_H += util/cache.h
 LIB_H += util/callchain.h
 LIB_H += util/build-id.h
 LIB_H += util/debug.h
-LIB_H += util/sysfs.h
+LIB_H += util/fs.h
 LIB_H += util/pmu.h
 LIB_H += util/event.h
 LIB_H += util/evsel.h
@@ -303,7 +303,7 @@ LIB_OBJS += $(OUTPUT)util/annotate.o
 LIB_OBJS += $(OUTPUT)util/build-id.o
 LIB_OBJS += $(OUTPUT)util/config.o
 LIB_OBJS += $(OUTPUT)util/ctype.o
-LIB_OBJS += $(OUTPUT)util/sysfs.o
+LIB_OBJS += $(OUTPUT)util/fs.o
 LIB_OBJS += $(OUTPUT)util/pmu.o
 LIB_OBJS += $(OUTPUT)util/environment.o
 LIB_OBJS += $(OUTPUT)util/event.o
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 48114d1..f47bf45 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -2,7 +2,7 @@
 #include "parse-events.h"
 #include "evsel.h"
 #include "evlist.h"
-#include "sysfs.h"
+#include "fs.h"
 #include <lk/debugfs.h>
 #include "tests.h"
 #include <linux/hw_breakpoint.h>
diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index beb8cf9..4af5a23 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -1,5 +1,5 @@
 #include "util.h"
-#include "sysfs.h"
+#include "fs.h"
 #include "../perf.h"
 #include "cpumap.h"
 #include <assert.h>
diff --git a/tools/perf/util/fs.c b/tools/perf/util/fs.c
new file mode 100644
index 0000000..346669a
--- /dev/null
+++ b/tools/perf/util/fs.c
@@ -0,0 +1,107 @@
+
+/* TODO merge/factor into tools/lib/lk/debugfs.c */
+
+#include "util.h"
+#include "util/fs.h"
+
+static const char * const sysfs_known_mountpoints[] = {
+	"/sys",
+	0,
+};
+
+struct perf_fs {
+	const char		*name;
+	const char * const	*mounts;
+	char			 path[PATH_MAX + 1];
+	bool			 found;
+	long			 magic;
+};
+
+enum {
+	FS_SYSFS = 0,
+};
+
+static struct perf_fs fss[] = {
+	[FS_SYSFS] = {
+		.name	= "sysfs",
+		.mounts	= sysfs_known_mountpoints,
+		.magic	= SYSFS_MAGIC,
+	},
+};
+
+static bool read_mounts(struct perf_fs *fs)
+{
+	bool found = false;
+	char type[100];
+	FILE *fp;
+
+	fp = fopen("/proc/mounts", "r");
+	if (fp == NULL)
+		return NULL;
+
+	while (!found &&
+	       fscanf(fp, "%*s %" STR(PATH_MAX) "s %99s %*s %*d %*d\n",
+		      fs->path, type) == 2) {
+
+		if (strcmp(type, fs->name) == 0)
+			found = true;
+	}
+
+	fclose(fp);
+	return fs->found = found;
+}
+
+static int valid_mount(const char *fs, long magic)
+{
+	struct statfs st_fs;
+
+	if (statfs(fs, &st_fs) < 0)
+		return -ENOENT;
+	else if (st_fs.f_type != magic)
+		return -ENOENT;
+
+	return 0;
+}
+
+static bool check_mounts(struct perf_fs *fs)
+{
+	const char * const *ptr;
+
+	ptr = fs->mounts;
+	while (*ptr) {
+		if (valid_mount(*ptr, fs->magic) == 0) {
+			fs->found = true;
+			strcpy(fs->path, *ptr);
+			return true;
+		}
+		ptr++;
+	}
+
+	return false;
+}
+
+static const char *get_mountpoint(struct perf_fs *fs)
+{
+	if (check_mounts(fs))
+		return fs->path;
+
+	return read_mounts(fs) ? fs->path : NULL;
+}
+
+static const char *find_mountpoint(int idx)
+{
+	struct perf_fs *fs = &fss[idx];
+
+	if (fs->found)
+		return (const char *) fs->path;
+
+	return get_mountpoint(fs);
+}
+
+#define FIND_MOUNTPOINT(name, idx)		\
+const char *name##_find_mountpoint(void)	\
+{						\
+	return find_mountpoint(idx);		\
+}
+
+FIND_MOUNTPOINT(sysfs, FS_SYSFS);
diff --git a/tools/perf/util/fs.h b/tools/perf/util/fs.h
new file mode 100644
index 0000000..082edbd
--- /dev/null
+++ b/tools/perf/util/fs.h
@@ -0,0 +1,6 @@
+#ifndef __PERF_FS
+#define __PERF_FS
+
+const char *sysfs_find_mountpoint(void);
+
+#endif /* __PERF_FS */
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 64362fe..45b42df 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -4,7 +4,7 @@
 #include <unistd.h>
 #include <stdio.h>
 #include <dirent.h>
-#include "sysfs.h"
+#include "fs.h"
 #include "util.h"
 #include "pmu.h"
 #include "parse-events.h"
diff --git a/tools/perf/util/python-ext-sources b/tools/perf/util/python-ext-sources
index f75ae1b..239036f 100644
--- a/tools/perf/util/python-ext-sources
+++ b/tools/perf/util/python-ext-sources
@@ -17,5 +17,5 @@ util/xyarray.c
 util/cgroup.c
 util/rblist.c
 util/strlist.c
-util/sysfs.c
+util/fs.c
 ../../lib/rbtree.c
diff --git a/tools/perf/util/sysfs.c b/tools/perf/util/sysfs.c
deleted file mode 100644
index f71e9ea..0000000
--- a/tools/perf/util/sysfs.c
+++ /dev/null
@@ -1,60 +0,0 @@
-
-#include "util.h"
-#include "sysfs.h"
-
-static const char * const sysfs_known_mountpoints[] = {
-	"/sys",
-	0,
-};
-
-static int sysfs_found;
-char sysfs_mountpoint[PATH_MAX + 1];
-
-static int sysfs_valid_mountpoint(const char *sysfs)
-{
-	struct statfs st_fs;
-
-	if (statfs(sysfs, &st_fs) < 0)
-		return -ENOENT;
-	else if (st_fs.f_type != (long) SYSFS_MAGIC)
-		return -ENOENT;
-
-	return 0;
-}
-
-const char *sysfs_find_mountpoint(void)
-{
-	const char * const *ptr;
-	char type[100];
-	FILE *fp;
-
-	if (sysfs_found)
-		return (const char *) sysfs_mountpoint;
-
-	ptr = sysfs_known_mountpoints;
-	while (*ptr) {
-		if (sysfs_valid_mountpoint(*ptr) == 0) {
-			sysfs_found = 1;
-			strcpy(sysfs_mountpoint, *ptr);
-			return sysfs_mountpoint;
-		}
-		ptr++;
-	}
-
-	/* give up and parse /proc/mounts */
-	fp = fopen("/proc/mounts", "r");
-	if (fp == NULL)
-		return NULL;
-
-	while (!sysfs_found &&
-	       fscanf(fp, "%*s %" STR(PATH_MAX) "s %99s %*s %*d %*d\n",
-		      sysfs_mountpoint, type) == 2) {
-
-		if (strcmp(type, "sysfs") == 0)
-			sysfs_found = 1;
-	}
-
-	fclose(fp);
-
-	return sysfs_found ? sysfs_mountpoint : NULL;
-}
diff --git a/tools/perf/util/sysfs.h b/tools/perf/util/sysfs.h
deleted file mode 100644
index a813b72..0000000
--- a/tools/perf/util/sysfs.h
+++ /dev/null
@@ -1,6 +0,0 @@
-#ifndef __SYSFS_H__
-#define __SYSFS_H__
-
-const char *sysfs_find_mountpoint(void);
-
-#endif /* __DEBUGFS_H__ */
-- 
1.7.11.7

[PATCH 2/3] perf tools: Add procfs support into fs object

[Jiri Olsa]

Adding procfs support into fs object.

The interface function:
  const char *procfs_find_mountpoint(void);

provides valid mountpoint path for procfs.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Joe Mario <jmario@redhat.com>
Cc: Don Zickus <dzickus@redhat.com>
---
 tools/perf/util/fs.c                  | 16 ++++++++++++++--
 tools/perf/util/fs.h                  |  1 +
 tools/perf/util/include/linux/magic.h |  4 ++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/fs.c b/tools/perf/util/fs.c
index 346669a..37b18b7 100644
--- a/tools/perf/util/fs.c
+++ b/tools/perf/util/fs.c
@@ -9,6 +9,11 @@ static const char * const sysfs_known_mountpoints[] = {
 	0,
 };
 
+static const char * const procfs_known_mountpoints[] = {
+	"/proc",
+	0,
+};
+
 struct perf_fs {
 	const char		*name;
 	const char * const	*mounts;
@@ -18,7 +23,8 @@ struct perf_fs {
 };
 
 enum {
-	FS_SYSFS = 0,
+	FS_SYSFS  = 0,
+	FS_PROCFS = 1,
 };
 
 static struct perf_fs fss[] = {
@@ -27,6 +33,11 @@ static struct perf_fs fss[] = {
 		.mounts	= sysfs_known_mountpoints,
 		.magic	= SYSFS_MAGIC,
 	},
+	[FS_PROCFS] = {
+		.name	= "proc",
+		.mounts	= procfs_known_mountpoints,
+		.magic	= PROC_SUPER_MAGIC,
+	},
 };
 
 static bool read_mounts(struct perf_fs *fs)
@@ -104,4 +115,5 @@ const char *name##_find_mountpoint(void)	\
 	return find_mountpoint(idx);		\
 }
 
-FIND_MOUNTPOINT(sysfs, FS_SYSFS);
+FIND_MOUNTPOINT(sysfs,  FS_SYSFS);
+FIND_MOUNTPOINT(procfs, FS_PROCFS);
diff --git a/tools/perf/util/fs.h b/tools/perf/util/fs.h
index 082edbd..bc6556f 100644
--- a/tools/perf/util/fs.h
+++ b/tools/perf/util/fs.h
@@ -2,5 +2,6 @@
 #define __PERF_FS
 
 const char *sysfs_find_mountpoint(void);
+const char *procfs_find_mountpoint(void);
 
 #endif /* __PERF_FS */
diff --git a/tools/perf/util/include/linux/magic.h b/tools/perf/util/include/linux/magic.h
index 58b64ed..07d63cf 100644
--- a/tools/perf/util/include/linux/magic.h
+++ b/tools/perf/util/include/linux/magic.h
@@ -9,4 +9,8 @@
 #define SYSFS_MAGIC            0x62656572
 #endif
 
+#ifndef PROC_SUPER_MAGIC
+#define PROC_SUPER_MAGIC       0x9fa0
+#endif
+
 #endif
-- 
1.7.11.7

[PATCH 3/3] perf tools: Check maximum frequency rate for record/top

[Jiri Olsa]

Adding the check for maximum allowed frequency rate
defined in following file:
  /proc/sys/kernel/perf_event_max_sample_rate

When we cross the maximum value we fail and display
detailed error message with advise.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c |  3 +++
 tools/perf/builtin-top.c    |  3 +++
 tools/perf/util/evlist.h    |  1 +
 tools/perf/util/record.c    | 35 +++++++++++++++++++++++++++++++++++
 4 files changed, 42 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ab8d15e..f300034 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -201,6 +201,9 @@ static int perf_record__open(struct perf_record *rec)
 	struct perf_record_opts *opts = &rec->opts;
 	int rc = 0;
 
+	if (perf_opts__check(opts))
+		return -1;
+
 	perf_evlist__config(evlist, opts);
 
 	list_for_each_entry(pos, &evlist->entries, node) {
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 04f5bf2..9c17af8 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -879,6 +879,9 @@ static int perf_top__start_counters(struct perf_top *top)
 	struct perf_evlist *evlist = top->evlist;
 	struct perf_record_opts *opts = &top->record_opts;
 
+	if (perf_opts__check(opts))
+		return -1;
+
 	perf_evlist__config(evlist, opts);
 
 	list_for_each_entry(counter, &evlist->entries, node) {
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 6e8acc9..3acadd9 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -99,6 +99,7 @@ void perf_evlist__set_id_pos(struct perf_evlist *evlist);
 bool perf_can_sample_identifier(void);
 void perf_evlist__config(struct perf_evlist *evlist,
 			 struct perf_record_opts *opts);
+int perf_opts__check(struct perf_record_opts *opts);
 
 int perf_evlist__prepare_workload(struct perf_evlist *evlist,
 				  struct perf_target *target,
diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
index 18d73aa..d881209 100644
--- a/tools/perf/util/record.c
+++ b/tools/perf/util/record.c
@@ -2,6 +2,8 @@
 #include "evsel.h"
 #include "cpumap.h"
 #include "parse-events.h"
+#include "fs.h"
+#include "util.h"
 
 typedef void (*setup_probe_fn_t)(struct perf_evsel *evsel);
 
@@ -106,3 +108,36 @@ void perf_evlist__config(struct perf_evlist *evlist,
 
 	perf_evlist__set_id_pos(evlist);
 }
+
+static int get_max_rate(unsigned int *rate)
+{
+	const char *procfs;
+	char path[PATH_MAX];
+
+	procfs = procfs_find_mountpoint();
+	if (!procfs)
+		return -1;
+
+	snprintf(path, PATH_MAX,
+		 "%s/sys/kernel/perf_event_max_sample_rate", procfs);
+
+	return filename__read_int(path, (int *) rate);
+}
+
+int perf_opts__check(struct perf_record_opts *opts)
+{
+	unsigned int max_rate;
+
+	if (get_max_rate(&max_rate))
+		return 0;
+
+	if (max_rate < opts->freq) {
+		pr_err("Maximum rate (%u) for frequency reached.\n"
+		   "Please use -F freq option with lower value or consider\n"
+		   "tweaking /proc/sys/kernel/perf_event_max_sample_rate.\n",
+		   max_rate);
+		return -1;
+	}
+
+	return 0;
+}
-- 
1.7.11.7

Re: [PATCH 3/3] perf tools: Check maximum frequency rate for record/top

[David Ahern]

On 11/4/13, 4:06 AM, Jiri Olsa wrote:
> Adding the check for maximum allowed frequency rate
> defined in following file:
>    /proc/sys/kernel/perf_event_max_sample_rate
>
> When we cross the maximum value we fail and display
> detailed error message with advise.

perf commands should automatically adjust the default frequency too. If 
the user did not specify a frequency then this message is going to be 
confusing.

Though the algorithm that keeps lowering the rate should be tweaked as well.

David

Re: [PATCH 3/3] perf tools: Check maximum frequency rate for record/top

[Jiri Olsa]

On Mon, Nov 04, 2013 at 08:17:24AM -0700, David Ahern wrote:
> On 11/4/13, 4:06 AM, Jiri Olsa wrote:
> >Adding the check for maximum allowed frequency rate
> >defined in following file:
> >   /proc/sys/kernel/perf_event_max_sample_rate
> >
> >When we cross the maximum value we fail and display
> >detailed error message with advise.
> 
> perf commands should automatically adjust the default frequency too.
> If the user did not specify a frequency then this message is going
> to be confusing.

ok, I'll add that check and probably some warning
if we are force to go below the default rate.

> 
> Though the algorithm that keeps lowering the rate should be tweaked as well.

yea.. I was thinking putting the max rate back up after
some sane period

or/plus having 'echo 0 > /proc/sys/kernel/perf_event_max_sample_rate' to
disable it completely

jirka

Re: [PATCH 3/3] perf tools: Check maximum frequency rate for record/top

[Arnaldo Carvalho de Melo]

Em Mon, Nov 04, 2013 at 04:32:16PM +0100, Jiri Olsa escreveu:
> On Mon, Nov 04, 2013 at 08:17:24AM -0700, David Ahern wrote:
> > On 11/4/13, 4:06 AM, Jiri Olsa wrote:
> > >Adding the check for maximum allowed frequency rate
> > >defined in following file:
> > >   /proc/sys/kernel/perf_event_max_sample_rate
> > >
> > >When we cross the maximum value we fail and display
> > >detailed error message with advise.
> > 
> > perf commands should automatically adjust the default frequency too.
> > If the user did not specify a frequency then this message is going
> > to be confusing.
> 
> ok, I'll add that check and probably some warning
> if we are force to go below the default rate.

I think we should just make it clear the frequency used and pick the
best according to the hardware.

And we are not doing gthat at all, at least in the TUI, need to change
that.

Only in the stdio, with that long list of commented lines, that should
as well be hidden, too verbose.

And here we also see the frequency used:

[root@zoo linux]# perf evlist -v
cycles: sample_freq=4000, size: 96, sample_type: IP|TID|TIME|PERIOD,
disabled: 1, inherit: 1, mmap: 1, comm: 1, freq: 1, enable_on_exec: 1,
sample_id_all: 1, exclude_guest: 1
[root@zoo linux]#
 
 > 
> > Though the algorithm that keeps lowering the rate should be tweaked as well.
> 
> yea.. I was thinking putting the max rate back up after
> some sane period
> 
> or/plus having 'echo 0 > /proc/sys/kernel/perf_event_max_sample_rate' to
> disable it completely
> 
> jirka

Re: [PATCH 0/3] perf tools: Add perf_event_max_sample_rate freq check

[Jiri Olsa]

On Mon, Nov 04, 2013 at 12:06:10PM +0100, Jiri Olsa wrote:
> hi,
> adding the check for maximum allowed frequency rate
> defined in perf_event_max_sample_rate.
> 
> Plus procfs mountpoint reading code. Reachable here:
>   git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>   perf/cc

oops, ^^^ should be 'perf/max_rate'

jirka

Re: [PATCH 1/3] perf tools: Factor sysfs code into generic fs object

[Arnaldo Carvalho de Melo]

Em Mon, Nov 04, 2013 at 12:06:11PM +0100, Jiri Olsa escreveu:
> Moving sysfs code into generic fs object and preparing
> it to carry procfs support.

Cool, but these are not 'perf' specific at all, so, please see below
some requests/suggestions:
 
> 
> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> index bc7cfa1..f5d3251 100644
> --- a/tools/perf/Makefile.perf
> +++ b/tools/perf/Makefile.perf
> @@ -242,7 +242,7 @@ LIB_H += util/cache.h
>  LIB_H += util/callchain.h
>  LIB_H += util/build-id.h
>  LIB_H += util/debug.h
> -LIB_H += util/sysfs.h
> +LIB_H += util/fs.h

Perfect naming, 'fs'

> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index beb8cf9..4af5a23 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -1,5 +1,5 @@
>  #include "util.h"
> -#include "sysfs.h"
> +#include "fs.h"
>  #include "../perf.h"
>  #include "cpumap.h"
>  #include <assert.h>
> diff --git a/tools/perf/util/fs.c b/tools/perf/util/fs.c
> new file mode 100644
> index 0000000..346669a
> --- /dev/null
> +++ b/tools/perf/util/fs.c
> @@ -0,0 +1,107 @@
> +
> +/* TODO merge/factor into tools/lib/lk/debugfs.c */
> +
> +#include "util.h"
> +#include "util/fs.h"
> +
> +static const char * const sysfs_known_mountpoints[] = {
> +	"/sys",
> +	0,
> +};
> +
> +struct perf_fs {

Ditch the 'perf', make it plain 'struct fs'.

> +	const char		*name;
> +	const char * const	*mounts;
> +	char			 path[PATH_MAX + 1];
> +	bool			 found;
> +	long			 magic;
> +};
> +
> +enum {
> +	FS_SYSFS = 0,

FS__SYSFS

> +};
> +
> +static struct perf_fs fss[] = {

Funny name, perhaps fs__entries instead? :-)

And here we have it static, at some point we could introduce a
'fs__register', that would be the counterpart of 'register_filesystem'
in the kernel sources.

> +	[FS_SYSFS] = {
> +		.name	= "sysfs",
> +		.mounts	= sysfs_known_mountpoints,
> +		.magic	= SYSFS_MAGIC,
> +	},
> +};
> +
> +static bool read_mounts(struct perf_fs *fs)
> +{
> +	bool found = false;
> +	char type[100];
> +	FILE *fp;
> +
> +	fp = fopen("/proc/mounts", "r");
> +	if (fp == NULL)
> +		return NULL;
> +
> +	while (!found &&
> +	       fscanf(fp, "%*s %" STR(PATH_MAX) "s %99s %*s %*d %*d\n",
> +		      fs->path, type) == 2) {
> +
> +		if (strcmp(type, fs->name) == 0)
> +			found = true;
> +	}
> +
> +	fclose(fp);
> +	return fs->found = found;
> +}

This is not a per instance method, I to traverse /proc/mounts once,
checking all entries in 'fs__entries' marking the ones that are present,
i.e. fs__entries would be a list/tree of 'struct fs'.

> +static int valid_mount(const char *fs, long magic)
> +{
> +	struct statfs st_fs;
> +
> +	if (statfs(fs, &st_fs) < 0)
> +		return -ENOENT;
> +	else if (st_fs.f_type != magic)
> +		return -ENOENT;
> +
> +	return 0;
> +}

Now this is getting me confused, so we will have to traverse
/proc/mounts looking  for that .name to be what we expect in entries and
afterwards we do a second step, checking if the magic number is the one
expected? Can't we do both verifications in just one place?

I know you haven't written this code, is just generalizing, but I got
confused so had to comment on it :-\

Perhaps since you're just making it useful for more filesystems just
please address the 'perf_fs' naming suggestions and we can deal with
these other issues later?

- Arnaldo

> +static bool check_mounts(struct perf_fs *fs)
> +{
> +	const char * const *ptr;
> +
> +	ptr = fs->mounts;
> +	while (*ptr) {
> +		if (valid_mount(*ptr, fs->magic) == 0) {
> +			fs->found = true;
> +			strcpy(fs->path, *ptr);
> +			return true;
> +		}
> +		ptr++;
> +	}
> +
> +	return false;
> +}
> +
> +static const char *get_mountpoint(struct perf_fs *fs)
> +{
> +	if (check_mounts(fs))
> +		return fs->path;
> +
> +	return read_mounts(fs) ? fs->path : NULL;
> +}
> +
> +static const char *find_mountpoint(int idx)
> +{
> +	struct perf_fs *fs = &fss[idx];
> +
> +	if (fs->found)
> +		return (const char *) fs->path;
> +
> +	return get_mountpoint(fs);
> +}
> +
> +#define FIND_MOUNTPOINT(name, idx)		\
> +const char *name##_find_mountpoint(void)	\
> +{						\
> +	return find_mountpoint(idx);		\
> +}
> +
> +FIND_MOUNTPOINT(sysfs, FS_SYSFS);
> diff --git a/tools/perf/util/fs.h b/tools/perf/util/fs.h
> new file mode 100644
> index 0000000..082edbd
> --- /dev/null
> +++ b/tools/perf/util/fs.h
> @@ -0,0 +1,6 @@
> +#ifndef __PERF_FS
> +#define __PERF_FS
> +
> +const char *sysfs_find_mountpoint(void);
> +
> +#endif /* __PERF_FS */
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 64362fe..45b42df 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -4,7 +4,7 @@
>  #include <unistd.h>
>  #include <stdio.h>
>  #include <dirent.h>
> -#include "sysfs.h"
> +#include "fs.h"
>  #include "util.h"
>  #include "pmu.h"
>  #include "parse-events.h"
> diff --git a/tools/perf/util/python-ext-sources b/tools/perf/util/python-ext-sources
> index f75ae1b..239036f 100644
> --- a/tools/perf/util/python-ext-sources
> +++ b/tools/perf/util/python-ext-sources
> @@ -17,5 +17,5 @@ util/xyarray.c
>  util/cgroup.c
>  util/rblist.c
>  util/strlist.c
> -util/sysfs.c
> +util/fs.c
>  ../../lib/rbtree.c
> diff --git a/tools/perf/util/sysfs.c b/tools/perf/util/sysfs.c
> deleted file mode 100644
> index f71e9ea..0000000
> --- a/tools/perf/util/sysfs.c
> +++ /dev/null
> @@ -1,60 +0,0 @@
> -
> -#include "util.h"
> -#include "sysfs.h"
> -
> -static const char * const sysfs_known_mountpoints[] = {
> -	"/sys",
> -	0,
> -};
> -
> -static int sysfs_found;
> -char sysfs_mountpoint[PATH_MAX + 1];
> -
> -static int sysfs_valid_mountpoint(const char *sysfs)
> -{
> -	struct statfs st_fs;
> -
> -	if (statfs(sysfs, &st_fs) < 0)
> -		return -ENOENT;
> -	else if (st_fs.f_type != (long) SYSFS_MAGIC)
> -		return -ENOENT;
> -
> -	return 0;
> -}
> -
> -const char *sysfs_find_mountpoint(void)
> -{
> -	const char * const *ptr;
> -	char type[100];
> -	FILE *fp;
> -
> -	if (sysfs_found)
> -		return (const char *) sysfs_mountpoint;
> -
> -	ptr = sysfs_known_mountpoints;
> -	while (*ptr) {
> -		if (sysfs_valid_mountpoint(*ptr) == 0) {
> -			sysfs_found = 1;
> -			strcpy(sysfs_mountpoint, *ptr);
> -			return sysfs_mountpoint;
> -		}
> -		ptr++;
> -	}
> -
> -	/* give up and parse /proc/mounts */
> -	fp = fopen("/proc/mounts", "r");
> -	if (fp == NULL)
> -		return NULL;
> -
> -	while (!sysfs_found &&
> -	       fscanf(fp, "%*s %" STR(PATH_MAX) "s %99s %*s %*d %*d\n",
> -		      sysfs_mountpoint, type) == 2) {
> -
> -		if (strcmp(type, "sysfs") == 0)
> -			sysfs_found = 1;
> -	}
> -
> -	fclose(fp);
> -
> -	return sysfs_found ? sysfs_mountpoint : NULL;
> -}
> diff --git a/tools/perf/util/sysfs.h b/tools/perf/util/sysfs.h
> deleted file mode 100644
> index a813b72..0000000
> --- a/tools/perf/util/sysfs.h
> +++ /dev/null
> @@ -1,6 +0,0 @@
> -#ifndef __SYSFS_H__
> -#define __SYSFS_H__
> -
> -const char *sysfs_find_mountpoint(void);
> -
> -#endif /* __DEBUGFS_H__ */
> -- 
> 1.7.11.7

Re: [PATCH 1/3] perf tools: Factor sysfs code into generic fs object

[Jiri Olsa]

On Tue, Nov 05, 2013 at 10:48:14AM -0200, Arnaldo Carvalho de Melo wrote:

SNIP

> > +struct perf_fs {
> 
> Ditch the 'perf', make it plain 'struct fs'.
> 
> > +	const char		*name;
> > +	const char * const	*mounts;
> > +	char			 path[PATH_MAX + 1];
> > +	bool			 found;
> > +	long			 magic;
> > +};
> > +
> > +enum {
> > +	FS_SYSFS = 0,
> 
> FS__SYSFS
> 
> > +};
> > +
> > +static struct perf_fs fss[] = {
> 
> Funny name, perhaps fs__entries instead? :-)

ok, will change all above namings :)

> 
> And here we have it static, at some point we could introduce a
> 'fs__register', that would be the counterpart of 'register_filesystem'
> in the kernel sources.
> 
> > +	[FS_SYSFS] = {
> > +		.name	= "sysfs",
> > +		.mounts	= sysfs_known_mountpoints,
> > +		.magic	= SYSFS_MAGIC,
> > +	},
> > +};
> > +
> > +static bool read_mounts(struct perf_fs *fs)
> > +{
> > +	bool found = false;
> > +	char type[100];
> > +	FILE *fp;
> > +
> > +	fp = fopen("/proc/mounts", "r");
> > +	if (fp == NULL)
> > +		return NULL;
> > +
> > +	while (!found &&
> > +	       fscanf(fp, "%*s %" STR(PATH_MAX) "s %99s %*s %*d %*d\n",
> > +		      fs->path, type) == 2) {
> > +
> > +		if (strcmp(type, fs->name) == 0)
> > +			found = true;
> > +	}
> > +
> > +	fclose(fp);
> > +	return fs->found = found;
> > +}
> 
> This is not a per instance method, I to traverse /proc/mounts once,
> checking all entries in 'fs__entries' marking the ones that are present,
> i.e. fs__entries would be a list/tree of 'struct fs'.
> 
> > +static int valid_mount(const char *fs, long magic)
> > +{
> > +	struct statfs st_fs;
> > +
> > +	if (statfs(fs, &st_fs) < 0)
> > +		return -ENOENT;
> > +	else if (st_fs.f_type != magic)
> > +		return -ENOENT;
> > +
> > +	return 0;
> > +}
> 
> Now this is getting me confused, so we will have to traverse
> /proc/mounts looking  for that .name to be what we expect in entries and
> afterwards we do a second step, checking if the magic number is the one
> expected? Can't we do both verifications in just one place?

the valid_mount is called only for preconfigured
(known mountpoints) paths

> 
> I know you haven't written this code, is just generalizing, but I got
> confused so had to comment on it :-\

yep ;-) perhaps some init code could do that

> 
> Perhaps since you're just making it useful for more filesystems just
> please address the 'perf_fs' naming suggestions and we can deal with
> these other issues later?

ok, I have already changes for the 3/3 change,
I'll send it together

jirka

Re: [PATCH 1/3] perf tools: Factor sysfs code into generic fs object

[Arnaldo Carvalho de Melo]

Em Tue, Nov 05, 2013 at 02:07:29PM +0100, Jiri Olsa escreveu:
> On Tue, Nov 05, 2013 at 10:48:14AM -0200, Arnaldo Carvalho de Melo wrote:
> > I know you haven't written this code, is just generalizing, but I got
> > confused so had to comment on it :-\
> 
> yep ;-) perhaps some init code could do that

At some point I want to have an init system similar to the kernel, i.e.
mark something as needing to be initialized with something like __init
and some linker magic (stolen from the kernel) would do it, think about
gtk, tui, symbol lib, lotsa init stuff, with some ordering
constraints...
 
> > Perhaps since you're just making it useful for more filesystems just
> > please address the 'perf_fs' naming suggestions and we can deal with
> > these other issues later?
> 
> ok, I have already changes for the 3/3 change,
> I'll send it together

Thanks!

- Arnaldo

[PATCH 3/3] perf tools: Check maximum frequency rate for record/top

[Jiri Olsa]

Adding the check for maximum allowed frequency rate
defined in following file:
  /proc/sys/kernel/perf_event_max_sample_rate

When we cross the maximum value we fail and display
detailed error message with advise.

  $ perf record -F 3000 ls
  Maximum frequency rate (2000) reached.
  Please use -F freq option with lower value or consider
  tweaking /proc/sys/kernel/perf_event_max_sample_rate.

In case user does not specify the frequency and
the default value cross the maximum, we display
warning and set the frequency value to the
current maximum.

  $ perf record ls
  Lowering default frequency rate to 2000.
  Please consider tweaking /proc/sys/kernel/perf_event_max_sample_rate.

Same messages are used for 'perf top'.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 15 +---------
 tools/perf/builtin-top.c    | 15 +---------
 tools/perf/util/evlist.h    |  1 +
 tools/perf/util/record.c    | 72 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 75 insertions(+), 28 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ab8d15e..4eaae4b 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -929,20 +929,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (perf_evlist__create_maps(evsel_list, &rec->opts.target) < 0)
 		usage_with_options(record_usage, record_options);
 
-	if (rec->opts.user_interval != ULLONG_MAX)
-		rec->opts.default_interval = rec->opts.user_interval;
-	if (rec->opts.user_freq != UINT_MAX)
-		rec->opts.freq = rec->opts.user_freq;
-
-	/*
-	 * User specified count overrides default frequency.
-	 */
-	if (rec->opts.default_interval)
-		rec->opts.freq = 0;
-	else if (rec->opts.freq) {
-		rec->opts.default_interval = rec->opts.freq;
-	} else {
-		ui__error("frequency and count are zero, aborting\n");
+	if (perf_opts__config(&rec->opts)) {
 		err = -EINVAL;
 		goto out_free_fd;
 	}
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 04f5bf2..8736cb3 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1203,20 +1203,7 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (top.delay_secs < 1)
 		top.delay_secs = 1;
 
-	if (opts->user_interval != ULLONG_MAX)
-		opts->default_interval = opts->user_interval;
-	if (opts->user_freq != UINT_MAX)
-		opts->freq = opts->user_freq;
-
-	/*
-	 * User specified count overrides default frequency.
-	 */
-	if (opts->default_interval)
-		opts->freq = 0;
-	else if (opts->freq) {
-		opts->default_interval = opts->freq;
-	} else {
-		ui__error("frequency and count are zero, aborting\n");
+	if (perf_opts__config(opts)) {
 		status = -EINVAL;
 		goto out_delete_maps;
 	}
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 6e8acc9..ebd6409 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -99,6 +99,7 @@ void perf_evlist__set_id_pos(struct perf_evlist *evlist);
 bool perf_can_sample_identifier(void);
 void perf_evlist__config(struct perf_evlist *evlist,
 			 struct perf_record_opts *opts);
+int perf_opts__config(struct perf_record_opts *opts);
 
 int perf_evlist__prepare_workload(struct perf_evlist *evlist,
 				  struct perf_target *target,
diff --git a/tools/perf/util/record.c b/tools/perf/util/record.c
index 18d73aa..1569641 100644
--- a/tools/perf/util/record.c
+++ b/tools/perf/util/record.c
@@ -2,6 +2,8 @@
 #include "evsel.h"
 #include "cpumap.h"
 #include "parse-events.h"
+#include "fs.h"
+#include "util.h"
 
 typedef void (*setup_probe_fn_t)(struct perf_evsel *evsel);
 
@@ -106,3 +108,73 @@ void perf_evlist__config(struct perf_evlist *evlist,
 
 	perf_evlist__set_id_pos(evlist);
 }
+
+static int get_max_rate(unsigned int *rate)
+{
+	const char *procfs;
+	char path[PATH_MAX];
+
+	procfs = procfs_find_mountpoint();
+	if (!procfs)
+		return -1;
+
+	snprintf(path, PATH_MAX,
+		 "%s/sys/kernel/perf_event_max_sample_rate", procfs);
+
+	return filename__read_int(path, (int *) rate);
+}
+
+static int perf_opts__config_freq(struct perf_record_opts *opts)
+{
+	bool user_freq = opts->user_freq != UINT_MAX;
+	unsigned int max_rate;
+
+	if (opts->user_interval != ULLONG_MAX)
+		opts->default_interval = opts->user_interval;
+	if (user_freq)
+		opts->freq = opts->user_freq;
+
+	/*
+	 * User specified count overrides default frequency.
+	 */
+	if (opts->default_interval)
+		opts->freq = 0;
+	else if (opts->freq) {
+		opts->default_interval = opts->freq;
+	} else {
+		pr_err("frequency and count are zero, aborting\n");
+		return -1;
+	}
+
+	if (get_max_rate(&max_rate))
+		return 0;
+
+	/*
+	 * User specified frequency is over current maximum.
+	 */
+	if (user_freq && (max_rate < opts->freq)) {
+		pr_err("Maximum frequency rate (%u) reached.\n"
+		   "Please use -F freq option with lower value or consider\n"
+		   "tweaking /proc/sys/kernel/perf_event_max_sample_rate.\n",
+		   max_rate);
+		return -1;
+	}
+
+	/*
+	 * Default frequency is over current maximum.
+	 */
+	if (max_rate < opts->freq) {
+		pr_warning("Lowering default frequency rate to %u.\n"
+			   "Please consider tweaking "
+			   "/proc/sys/kernel/perf_event_max_sample_rate.\n",
+			   max_rate);
+		opts->freq = max_rate;
+	}
+
+	return 0;
+}
+
+int perf_opts__config(struct perf_record_opts *opts)
+{
+	return perf_opts__config_freq(opts);
+}
-- 
1.8.3.1