[PATCH v3 0/9] perf tools: Add script browser and runtime data file switch

[PATCH v3 0/9] perf tools: Add script browser and runtime data file switch

[Feng Tang]

Hi Arnaldo and all,

This is a patch set mainly to add a browser for perf script, which
will be integrated into the main hists and annotation browser. It
also add the inital support for runtime perf data file switch in the
'perf report' window.

Patch 1    is a simple cleanup not related with the main part
Patch 2    add the global variable "input_name"
patch 3-6  introduce the script browser and integrate it to
           hists/annotation browser
patch 7-9  add the runtime data file switch for 'perf report' 

The patches are on top of current perf/core branch of your git tree. 
Please help to review. 

Thanks to Andi/Arnaldo/Namhyung for the great suggestions/reviews.


Changelog:
	Since v2:
	* add more filter to find_scripts()
	* add runtime data file switch for 'perf report'
	* emphasize the script browser doesn't cover 'record'
	  in commit log
	
	Since v1:
	* Add filter for scripts can't be run in script browser
	* Fix some bugs about buffer handling and error check 

Feng
----------------------
Feng Tang (9):
  perf hists: Move hists_init() from util/evsel.c to util/hist.c
  perf tool: Add a global variable "const char *input_name"
  perf script: Add more filter to find_scripts()
  perf ui/browser: Add a browser for perf script
  perf ui/browser: Integrate script browser into annotation browser
  perf ui/browser: Integrate script browser into main hists browser
  perf header: Add check_perf_magic() func
  perf hists browser: Add option for runtime switching perf data file
  perf report: Enable the runtime switching of perf data file

 tools/perf/Makefile               |    4 +
 tools/perf/builtin-annotate.c     |    5 +-
 tools/perf/builtin-buildid-list.c |    1 -
 tools/perf/builtin-evlist.c       |    5 +-
 tools/perf/builtin-inject.c       |    3 +-
 tools/perf/builtin-kmem.c         |    2 -
 tools/perf/builtin-lock.c         |    2 -
 tools/perf/builtin-report.c       |   31 +++++--
 tools/perf/builtin-sched.c        |    5 +-
 tools/perf/builtin-script.c       |   78 +++++++++++++++-
 tools/perf/builtin-timechart.c    |    2 -
 tools/perf/perf.c                 |    1 +
 tools/perf/perf.h                 |    1 +
 tools/perf/ui/browsers/annotate.c |    6 +
 tools/perf/ui/browsers/hists.c    |  110 ++++++++++++++++++++++
 tools/perf/ui/browsers/scripts.c  |  182 +++++++++++++++++++++++++++++++++++++
 tools/perf/ui/keysyms.h           |    1 +
 tools/perf/util/evsel.c           |   10 --
 tools/perf/util/evsel.h           |    2 -
 tools/perf/util/header.c          |   11 ++
 tools/perf/util/header.h          |    1 +
 tools/perf/util/hist.c            |   10 ++
 tools/perf/util/hist.h            |    8 ++
 23 files changed, 437 insertions(+), 44 deletions(-)
 create mode 100644 tools/perf/ui/browsers/scripts.c

[PATCH v3 1/9] perf hists: Move hists_init() from util/evsel.c to util/hist.c

[Feng Tang]

Which looks more natural

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 tools/perf/util/evsel.c |   10 ----------
 tools/perf/util/evsel.h |    2 --
 tools/perf/util/hist.c  |   10 ++++++++++
 tools/perf/util/hist.h  |    1 +
 4 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 1506ba0..4c7328d 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -39,16 +39,6 @@ static int __perf_evsel__sample_size(u64 sample_type)
 	return size;
 }
 
-void hists__init(struct hists *hists)
-{
-	memset(hists, 0, sizeof(*hists));
-	hists->entries_in_array[0] = hists->entries_in_array[1] = RB_ROOT;
-	hists->entries_in = &hists->entries_in_array[0];
-	hists->entries_collapsed = RB_ROOT;
-	hists->entries = RB_ROOT;
-	pthread_mutex_init(&hists->lock, NULL);
-}
-
 void perf_evsel__init(struct perf_evsel *evsel,
 		      struct perf_event_attr *attr, int idx)
 {
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 93876ba..af0bc97 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -197,8 +197,6 @@ static inline int perf_evsel__read_scaled(struct perf_evsel *evsel,
 	return __perf_evsel__read(evsel, ncpus, nthreads, true);
 }
 
-void hists__init(struct hists *hists);
-
 int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
 			     struct perf_sample *sample, bool swapped);
 
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 6ec5398..d3689e5 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -690,3 +690,13 @@ void hists__inc_nr_events(struct hists *hists, u32 type)
 	++hists->stats.nr_events[0];
 	++hists->stats.nr_events[type];
 }
+
+void hists__init(struct hists *hists)
+{
+	memset(hists, 0, sizeof(*hists));
+	hists->entries_in_array[0] = hists->entries_in_array[1] = RB_ROOT;
+	hists->entries_in = &hists->entries_in_array[0];
+	hists->entries_collapsed = RB_ROOT;
+	hists->entries = RB_ROOT;
+	pthread_mutex_init(&hists->lock, NULL);
+}
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index f011ad4..4edfb74 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -70,6 +70,7 @@ struct hists {
 	u16			col_len[HISTC_NR_COLS];
 };
 
+void hists__init(struct hists *hists);
 struct hist_entry *__hists__add_entry(struct hists *self,
 				      struct addr_location *al,
 				      struct symbol *parent, u64 period);
-- 
1.7.1

[PATCH v3 2/9] perf tool: Add a global variable "const char *input_name"

[Feng Tang]

Currently many perf ommands annotate/evlist/report/script/lock etc
all support "-i" option to chose a specific perf data, and all
of them create a local "input_name" to save the file name for
that perf data.

Since most of these commands need it, we can add a global variable
for it, also it can some other benefits:
1. When calling script browser inside hists/annotation browser, it
needs to know the perf data file name to run that script.
2. For further feature like runtime switching to another perf data
file, this variable can also help.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 tools/perf/builtin-annotate.c     |    5 ++---
 tools/perf/builtin-buildid-list.c |    1 -
 tools/perf/builtin-evlist.c       |    5 ++---
 tools/perf/builtin-inject.c       |    3 +--
 tools/perf/builtin-kmem.c         |    2 --
 tools/perf/builtin-lock.c         |    2 --
 tools/perf/builtin-report.c       |   13 ++++++-------
 tools/perf/builtin-sched.c        |    5 ++---
 tools/perf/builtin-script.c       |    2 --
 tools/perf/builtin-timechart.c    |    2 --
 tools/perf/perf.c                 |    1 +
 tools/perf/perf.h                 |    1 +
 12 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 9ea3854..b3d60ae 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -33,7 +33,6 @@
 
 struct perf_annotate {
 	struct perf_tool tool;
-	char const *input_name;
 	bool	   force, use_tui, use_stdio;
 	bool	   full_paths;
 	bool	   print_line;
@@ -174,7 +173,7 @@ static int __cmd_annotate(struct perf_annotate *ann)
 	struct perf_evsel *pos;
 	u64 total_nr_samples;
 
-	session = perf_session__new(ann->input_name, O_RDONLY,
+	session = perf_session__new(input_name, O_RDONLY,
 				    ann->force, false, &ann->tool);
 	if (session == NULL)
 		return -ENOMEM;
@@ -252,7 +251,7 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 		},
 	};
 	const struct option options[] = {
-	OPT_STRING('i', "input", &annotate.input_name, "file",
+	OPT_STRING('i', "input", &input_name, "file",
 		    "input file name"),
 	OPT_STRING('d', "dsos", &symbol_conf.dso_list_str, "dso[,dso...]",
 		   "only consider symbols in these dsos"),
diff --git a/tools/perf/builtin-buildid-list.c b/tools/perf/builtin-buildid-list.c
index 1159fee..6940264 100644
--- a/tools/perf/builtin-buildid-list.c
+++ b/tools/perf/builtin-buildid-list.c
@@ -16,7 +16,6 @@
 #include "util/session.h"
 #include "util/symbol.h"
 
-static const char *input_name;
 static bool force;
 static bool show_kernel;
 static bool with_hits;
diff --git a/tools/perf/builtin-evlist.c b/tools/perf/builtin-evlist.c
index 1fb1641..46420c0 100644
--- a/tools/perf/builtin-evlist.c
+++ b/tools/perf/builtin-evlist.c
@@ -48,12 +48,12 @@ static int __if_print(bool *first, const char *field, u64 value)
 
 #define if_print(field) __if_print(&first, #field, pos->attr.field)
 
-static int __cmd_evlist(const char *input_name, struct perf_attr_details *details)
+static int __cmd_evlist(const char *file_name, struct perf_attr_details *details)
 {
 	struct perf_session *session;
 	struct perf_evsel *pos;
 
-	session = perf_session__new(input_name, O_RDONLY, 0, false, NULL);
+	session = perf_session__new(file_name, O_RDONLY, 0, false, NULL);
 	if (session == NULL)
 		return -ENOMEM;
 
@@ -116,7 +116,6 @@ static const char * const evlist_usage[] = {
 int cmd_evlist(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	struct perf_attr_details details = { .verbose = false, };
-	const char *input_name = NULL;
 	const struct option options[] = {
 		OPT_STRING('i', "input", &input_name, "file",
 			    "Input file name"),
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 1eaa661..307ad23 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -14,7 +14,6 @@
 
 #include "util/parse-options.h"
 
-static char		const *input_name = "-";
 static bool		inject_build_ids;
 
 static int perf_event__repipe_synth(struct perf_tool *tool __maybe_unused,
@@ -245,7 +244,7 @@ static int __cmd_inject(void)
 		perf_inject.tracing_data = perf_event__repipe_tracing_data;
 	}
 
-	session = perf_session__new(input_name, O_RDONLY, false, true, &perf_inject);
+	session = perf_session__new("-", O_RDONLY, false, true, &perf_inject);
 	if (session == NULL)
 		return -ENOMEM;
 
diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index f5f8a6b..dae3dab 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -20,8 +20,6 @@
 struct alloc_stat;
 typedef int (*sort_fn_t)(struct alloc_stat *, struct alloc_stat *);
 
-static const char		*input_name;
-
 static int			alloc_flag;
 static int			caller_flag;
 
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index a803520..0505ef3 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -334,8 +334,6 @@ alloc_failed:
 	return NULL;
 }
 
-static const char *input_name;
-
 struct raw_event_sample {
 	u32			size;
 	char			data[0];
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 1da243d..e4b2246 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -39,7 +39,6 @@
 struct perf_report {
 	struct perf_tool	tool;
 	struct perf_session	*session;
-	char const		*input_name;
 	bool			force, use_tui, use_gtk, use_stdio;
 	bool			hide_unresolved;
 	bool			dont_use_callchains;
@@ -570,7 +569,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		.pretty_printing_style	 = "normal",
 	};
 	const struct option options[] = {
-	OPT_STRING('i', "input", &report.input_name, "file",
+	OPT_STRING('i', "input", &input_name, "file",
 		    "input file name"),
 	OPT_INCR('v', "verbose", &verbose,
 		    "be more verbose (show symbol address, etc)"),
@@ -656,13 +655,13 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (report.inverted_callchain)
 		callchain_param.order = ORDER_CALLER;
 
-	if (!report.input_name || !strlen(report.input_name)) {
+	if (!input_name || !strlen(input_name)) {
 		if (!fstat(STDIN_FILENO, &st) && S_ISFIFO(st.st_mode))
-			report.input_name = "-";
+			input_name = "-";
 		else
-			report.input_name = "perf.data";
+			input_name = "perf.data";
 	}
-	session = perf_session__new(report.input_name, O_RDONLY,
+	session = perf_session__new(input_name, O_RDONLY,
 				    report.force, false, &report.tool);
 	if (session == NULL)
 		return -ENOMEM;
@@ -687,7 +686,7 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	}
 
-	if (strcmp(report.input_name, "-") != 0)
+	if (strcmp(input_name, "-") != 0)
 		setup_browser(true);
 	else {
 		use_browser = 0;
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 9b9e32e..72f1c51 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -120,7 +120,6 @@ struct trace_sched_handler {
 
 struct perf_sched {
 	struct perf_tool tool;
-	const char	 *input_name;
 	const char	 *sort_order;
 	unsigned long	 nr_tasks;
 	struct task_desc *pid_to_task[MAX_PID];
@@ -1460,7 +1459,7 @@ static int perf_sched__read_events(struct perf_sched *sched, bool destroy,
 	};
 	struct perf_session *session;
 
-	session = perf_session__new(sched->input_name, O_RDONLY, 0, false, &sched->tool);
+	session = perf_session__new(input_name, O_RDONLY, 0, false, &sched->tool);
 	if (session == NULL) {
 		pr_debug("No Memory for session\n");
 		return -1;
@@ -1707,7 +1706,7 @@ int cmd_sched(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_END()
 	};
 	const struct option sched_options[] = {
-	OPT_STRING('i', "input", &sched.input_name, "file",
+	OPT_STRING('i', "input", &input_name, "file",
 		    "input file name"),
 	OPT_INCR('v', "verbose", &verbose,
 		    "be more verbose (show symbol address, etc)"),
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 1be843a..4e2f10f 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -473,8 +473,6 @@ static int cleanup_scripting(void)
 	return scripting_ops->stop_script();
 }
 
-static const char *input_name;
-
 static int process_sample_event(struct perf_tool *tool __maybe_unused,
 				union perf_event *event,
 				struct perf_sample *sample,
diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
index 55a3a6c..b3fc023 100644
--- a/tools/perf/builtin-timechart.c
+++ b/tools/perf/builtin-timechart.c
@@ -37,8 +37,6 @@
 #define SUPPORT_OLD_POWER_EVENTS 1
 #define PWR_EVENT_EXIT -1
 
-
-static const char	*input_name;
 static const char	*output_name = "output.svg";
 
 static unsigned int	numcpus;
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index fb8578c..7442390 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -24,6 +24,7 @@ const char perf_more_info_string[] =
 
 int use_browser = -1;
 static int use_pager = -1;
+const char *input_name;
 
 struct cmd_struct {
 	const char *cmd;
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 87f4ec6..d754247 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -202,6 +202,7 @@ struct branch_stack {
 	struct branch_entry	entries[0];
 };
 
+extern const char *input_name;
 extern bool perf_host, perf_guest;
 extern const char perf_version_string[];
 
-- 
1.7.1

[PATCH v3 3/9] perf script: Add more filter to find_scripts()

[Feng Tang]

As suggested by Arnaldo, many scripts have their own usages and need
capture specific events or tracepoints, so only those scripts whose
targe events match the events in current perf data file should be
listed in the script browser menu.

This patch will add the event match checking.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 tools/perf/builtin-script.c |   76 +++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 4e2f10f..37a0df8 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1031,6 +1031,63 @@ static int list_available_scripts(const struct option *opt __maybe_unused,
 }
 
 /*
+ * Some scripts specify the required events in their "xxx-record" file,
+ * this function will check if the events in perf.data match those
+ * mentioned in the "xxx-record".
+ */
+static int check_ev_match(char *dir_name, char *scriptname,
+			struct perf_session *session)
+{
+	char filename[MAXPATHLEN], evname[128];
+	char line[BUFSIZ], *p, *temp;
+	struct perf_evsel *pos;
+	int match;
+	FILE *fp;
+
+	sprintf(filename, "%s/bin/%s-record", dir_name, scriptname);
+
+	fp = fopen(filename, "r");
+	if (!fp)
+		return -1;
+
+	while (fgets(line, sizeof(line), fp)) {
+		p = ltrim(line);
+		if (strlen(p) == 0 ||*p == '#')
+			continue;
+
+		while (strlen(p)) {
+			temp = strstr(p, "-e");
+			if (!temp)
+				break;
+
+			p = temp + 3;
+			temp = strchr(p, ' ');
+			snprintf(evname, (temp - p) + 1, "%s", p);
+
+			match = 0;
+			list_for_each_entry(pos,
+					&session->evlist->entries, node) {
+				if (!strcmp(perf_evsel__name(pos), evname)) {
+					match = 1;
+					break;
+				}
+			}
+
+			if (!match) {
+				fclose(fp);
+				return -1;
+			}
+
+			p = temp + 1;
+		}
+
+	}
+
+	fclose(fp);
+	return 0;
+}
+
+/*
  * Return -1 if none is found, otherwise the actual scripts number.
  *
  * Currently the only user of this function is the script browser, which
@@ -1040,17 +1097,23 @@ static int list_available_scripts(const struct option *opt __maybe_unused,
 int find_scripts(char **scripts_array, char **scripts_path_array)
 {
 	struct dirent *script_next, *lang_next, script_dirent, lang_dirent;
-	char scripts_path[MAXPATHLEN];
+	char scripts_path[MAXPATHLEN], lang_path[MAXPATHLEN];
 	DIR *scripts_dir, *lang_dir;
-	char lang_path[MAXPATHLEN];
+	struct perf_session *session;
 	char *temp;
 	int i = 0;
 
+	session = perf_session__new(input_name, O_RDONLY, 0, false, NULL);
+	if (!session)
+		return -1;
+
 	snprintf(scripts_path, MAXPATHLEN, "%s/scripts", perf_exec_path());
 
 	scripts_dir = opendir(scripts_path);
-	if (!scripts_dir)
+	if (!scripts_dir) {
+		perf_session__delete(session);
 		return -1;
+	}
 
 	for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next) {
 		snprintf(lang_path, MAXPATHLEN, "%s/%s", scripts_path,
@@ -1078,10 +1141,17 @@ int find_scripts(char **scripts_array, char **scripts_path_array)
 			snprintf(scripts_array[i],
 				(temp - script_dirent.d_name) + 1,
 				"%s", script_dirent.d_name);
+
+			if (check_ev_match(lang_path, scripts_array[i], session))
+				continue;
+
 			i++;
 		}
+		closedir(lang_dir);
 	}
 
+	closedir(scripts_dir);
+	perf_session__delete(session);
 	return i;
 }
 
-- 
1.7.1

[PATCH v3 4/9] perf ui/browser: Add a browser for perf script

[Feng Tang]

Create a script browser, so that user can check all the available
scripts for current perf data file and run them inside the main perf
report or annotation browsers, for all perf samples or for samples
belong to one thread/symbol.

Please be noted: current script browser is only for report use, and
doesn't cover the record phase, IOW it must run againt one existing
perf data file.

The work flow is, users can use function key to list all the available
scripts for current perf data file in system and chose one, which will
be executed with popen("perf script -s xxx.xx",) and all the output
lines are put into one ui browser, pressing 'q' or left arrow key will
make it return to previous browser.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 tools/perf/Makefile              |    4 +
 tools/perf/ui/browsers/scripts.c |  182 ++++++++++++++++++++++++++++++++++++++
 tools/perf/util/hist.h           |    7 ++
 3 files changed, 193 insertions(+), 0 deletions(-)
 create mode 100644 tools/perf/ui/browsers/scripts.c

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 3ae6a59..9c549b5 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -575,6 +575,7 @@ else
 		LIB_OBJS += $(OUTPUT)ui/browsers/annotate.o
 		LIB_OBJS += $(OUTPUT)ui/browsers/hists.o
 		LIB_OBJS += $(OUTPUT)ui/browsers/map.o
+		LIB_OBJS += $(OUTPUT)ui/browsers/scripts.o
 		LIB_OBJS += $(OUTPUT)ui/progress.o
 		LIB_OBJS += $(OUTPUT)ui/util.o
 		LIB_OBJS += $(OUTPUT)ui/tui/setup.o
@@ -892,6 +893,9 @@ $(OUTPUT)ui/browsers/hists.o: ui/browsers/hists.c $(OUTPUT)PERF-CFLAGS
 $(OUTPUT)ui/browsers/map.o: ui/browsers/map.c $(OUTPUT)PERF-CFLAGS
 	$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) -DENABLE_SLFUTURE_CONST $<
 
+$(OUTPUT)ui/browsers/scripts.o: ui/browsers/scripts.c $(OUTPUT)PERF-CFLAGS
+	$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) -DENABLE_SLFUTURE_CONST $<
+
 $(OUTPUT)util/rbtree.o: ../../lib/rbtree.c $(OUTPUT)PERF-CFLAGS
 	$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) -DETC_PERFCONFIG='"$(ETC_PERFCONFIG_SQ)"' $<
 
diff --git a/tools/perf/ui/browsers/scripts.c b/tools/perf/ui/browsers/scripts.c
new file mode 100644
index 0000000..28a58fc
--- /dev/null
+++ b/tools/perf/ui/browsers/scripts.c
@@ -0,0 +1,182 @@
+#include <elf.h>
+#include <newt.h>
+#include <inttypes.h>
+#include <sys/ttydefaults.h>
+#include <string.h>
+#include "../../util/sort.h"
+#include "../../util/util.h"
+#include "../../util/hist.h"
+#include "../../util/debug.h"
+#include "../../util/symbol.h"
+#include "../browser.h"
+#include "../helpline.h"
+#include "../libslang.h"
+
+/* 2048 lines should be enough for a script output */
+#define MAX_LINES		2048
+
+/* 160 bytes for one output line */
+#define AVERAGE_LINE_LEN	160
+
+struct script_line {
+	struct list_head node;
+	char line[AVERAGE_LINE_LEN];
+};
+
+struct perf_script_browser {
+	struct ui_browser b;
+	struct list_head entries;
+	const char *script_name;
+	int nr_lines;
+};
+
+#define SCRIPT_NAMELEN	128
+#define SCRIPT_MAX_NO	64
+/*
+ * Usually the full path for a script is:
+ *	/home/username/libexec/perf-core/scripts/python/xxx.py
+ *	/home/username/libexec/perf-core/scripts/perl/xxx.pl
+ * So 256 should be long enough to contain the full path.
+ */
+#define SCRIPT_FULLPATH_LEN	256
+
+/*
+ * When success, will copy the full path of the selected script
+ * into  the buffer pointed by script_name, and return 0.
+ * Return -1 on failure.
+ */
+static int list_scripts(char *script_name)
+{
+	char *buf, *names[SCRIPT_MAX_NO], *paths[SCRIPT_MAX_NO];
+	int i, num, choice, ret = -1;
+
+	/* Preset the script name to SCRIPT_NAMELEN */
+	buf = malloc(SCRIPT_MAX_NO * (SCRIPT_NAMELEN + SCRIPT_FULLPATH_LEN));
+	if (!buf)
+		return ret;
+
+	for (i = 0; i < SCRIPT_MAX_NO; i++) {
+		names[i] = buf + i * (SCRIPT_NAMELEN + SCRIPT_FULLPATH_LEN);
+		paths[i] = names[i] + SCRIPT_NAMELEN;
+	}
+
+	num = find_scripts(names, paths);
+	if (num > 0) {
+		choice = ui__popup_menu(num, names);
+		if (choice < num && choice >= 0) {
+			strcpy(script_name, paths[choice]);
+			ret = 0;
+		}
+	}
+
+	free(buf);
+	return ret;
+}
+
+static void script_browser__write(struct ui_browser *browser,
+				   void *entry, int row)
+{
+	struct script_line *sline = list_entry(entry, struct script_line, node);
+	bool current_entry = ui_browser__is_current_entry(browser, row);
+
+	ui_browser__set_color(browser, current_entry ? HE_COLORSET_SELECTED :
+						       HE_COLORSET_NORMAL);
+
+	slsmg_write_nstring(sline->line, browser->width);
+}
+
+static int script_browser__run(struct perf_script_browser *self)
+{
+	int key;
+
+	if (ui_browser__show(&self->b, self->script_name,
+			     "Press <- or ESC to exit") < 0)
+		return -1;
+
+	while (1) {
+		key = ui_browser__run(&self->b, 0);
+
+		/* We can add some special key handling here if needed */
+		break;
+	}
+
+	ui_browser__hide(&self->b);
+	return key;
+}
+
+
+int script_browse(const char *script_opt)
+{
+	char cmd[SCRIPT_FULLPATH_LEN*2], script_name[SCRIPT_FULLPATH_LEN];
+	char *line = NULL;
+	size_t len = 0;
+	ssize_t retlen;
+	int ret = -1, nr_entries = 0;
+	FILE *fp;
+	void *buf;
+	struct script_line *sline;
+
+	struct perf_script_browser script = {
+		.b = {
+			.refresh    = ui_browser__list_head_refresh,
+			.seek	    = ui_browser__list_head_seek,
+			.write	    = script_browser__write,
+		},
+		.script_name = script_name,
+	};
+
+	INIT_LIST_HEAD(&script.entries);
+
+	/* Save each line of the output in one struct script_line object. */
+	buf = zalloc((sizeof(*sline)) * MAX_LINES);
+	if (!buf)
+		return -1;
+	sline = buf;
+
+	memset(script_name, 0, SCRIPT_FULLPATH_LEN);
+	if (list_scripts(script_name))
+		goto exit;
+
+	if (script_opt)
+		sprintf(cmd, "perf script %s -s %s 2>&1", script_opt, script_name);
+	else
+		sprintf(cmd, "perf script -s %s 2>&1", script_name);
+
+	fp = popen(cmd, "r");
+	if (!fp)
+		goto exit;
+
+	while ((retlen = getline(&line, &len, fp)) != -1) {
+		strncpy(sline->line, line, AVERAGE_LINE_LEN);
+
+		/* If one output line is very large, just cut it short */
+		if (retlen >= AVERAGE_LINE_LEN) {
+			sline->line[AVERAGE_LINE_LEN - 1] = '\0';
+			sline->line[AVERAGE_LINE_LEN - 2] = '\n';
+		}
+		list_add_tail(&sline->node, &script.entries);
+
+		if (script.b.width < retlen)
+			script.b.width = retlen;
+
+		if (nr_entries++ >= MAX_LINES)
+			break;
+		sline++;
+	}
+
+	if (script.b.width > AVERAGE_LINE_LEN)
+		script.b.width = AVERAGE_LINE_LEN;
+
+	if (line)
+		free(line);
+	pclose(fp);
+
+	script.nr_lines = nr_entries;
+	script.b.nr_entries = nr_entries;
+	script.b.entries = &script.entries;
+
+	ret = script_browser__run(&script);
+exit:
+	free(buf);
+	return ret;
+}
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 4edfb74..9a2b140 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -176,6 +176,12 @@ static inline int hist_entry__tui_annotate(struct hist_entry *self
 {
 	return 0;
 }
+
+static inline int script_browse(const char *script_opt)
+{
+	return 0;
+}
+
 #define K_LEFT -1
 #define K_RIGHT -2
 #else
@@ -203,6 +209,7 @@ int perf_evlist__gtk_browse_hists(struct perf_evlist *evlist __maybe_unused,
 int perf_evlist__gtk_browse_hists(struct perf_evlist *evlist, const char *help,
 				  void(*timer)(void *arg), void *arg,
 				  int refresh);
+int script_browse(const char *script_opt);
 #endif
 
 unsigned int hists__sort_list_width(struct hists *self);
-- 
1.7.1

[PATCH v3 5/9] perf ui/browser: Integrate script browser into annotation browser

[Feng Tang]

Integrate the script browser into annotation, users can press function
key 'r' to list all perf scripts and select one of them to run that
script, the output will be shown in a separate browser.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 tools/perf/ui/browsers/annotate.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 8f8cd2d..28f8aab 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -676,8 +676,14 @@ static int annotate_browser__run(struct annotate_browser *browser, int evidx,
 		"o             Toggle disassembler output/simplified view\n"
 		"s             Toggle source code view\n"
 		"/             Search string\n"
+		"r             Run available scripts\n"
 		"?             Search previous string\n");
 			continue;
+		case 'r':
+			{
+				script_browse(NULL);
+				continue;
+			}
 		case 'H':
 			nd = browser->curr_hot;
 			break;
-- 
1.7.1

[PATCH v3 6/9] perf ui/browser: Integrate script browser into main hists browser

[Feng Tang]

Integrate the script browser into "perf report" framework, users can
use function key 'r' or the drop down menu to list all perf scripts
and select one of them, just like they did for the annotation.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 tools/perf/ui/browsers/hists.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index a21f40b..c491b78 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1139,6 +1139,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 	int nr_options = 0;
 	int key = -1;
 	char buf[64];
+	char script_opt[64];
 
 	if (browser == NULL)
 		return -1;
@@ -1149,6 +1150,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 
 	ui_helpline__push(helpline);
 
+	memset(script_opt, 0, 64);
 	memset(options, 0, sizeof(options));
 
 	while (1) {
@@ -1157,6 +1159,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		int choice = 0,
 		    annotate = -2, zoom_dso = -2, zoom_thread = -2,
 		    annotate_f = -2, annotate_t = -2, browse_map = -2;
+		int scripts_comm = -2, scripts_symbol = -2, scripts_all = -2;
 
 		nr_options = 0;
 
@@ -1209,6 +1212,8 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 				hist_browser__reset(browser);
 			}
 			continue;
+		case 'r':
+			goto do_scripts;
 		case K_F1:
 		case 'h':
 		case '?':
@@ -1227,6 +1232,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 					"E             Expand all callchains\n"
 					"d             Zoom into current DSO\n"
 					"t             Zoom into current Thread\n"
+					"r             Run available scripts\n"
 					"P             Print histograms to perf.hist.N\n"
 					"V             Verbose (DSO names in callchains, etc)\n"
 					"/             Filter symbol by name");
@@ -1315,6 +1321,25 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		    browser->selection->map != NULL &&
 		    asprintf(&options[nr_options], "Browse map details") > 0)
 			browse_map = nr_options++;
+
+		/* perf script support */
+		if (browser->he_selection) {
+			struct symbol *sym;
+
+			if (asprintf(&options[nr_options], "Run scripts for samples of thread [%s]",
+				browser->he_selection->thread->comm) > 0)
+				scripts_comm = nr_options++;
+
+			sym = browser->he_selection->ms.sym;
+			if (sym && sym->namelen &&
+				asprintf(&options[nr_options], "Run scripts for samples of symbol [%s]",
+						sym->name) > 0)
+				scripts_symbol = nr_options++;
+		}
+
+		if (asprintf(&options[nr_options], "Run scripts for all samples") > 0)
+			scripts_all = nr_options++;
+
 add_exit_option:
 		options[nr_options++] = (char *)"Exit";
 retry_popup_menu:
@@ -1409,6 +1434,18 @@ zoom_out_thread:
 			hists__filter_by_thread(hists);
 			hist_browser__reset(browser);
 		}
+		/* perf scripts support */
+		else if (choice == scripts_all || choice == scripts_comm ||
+				choice == scripts_symbol) {
+do_scripts:
+			if (choice == scripts_comm)
+				sprintf(script_opt, " -c %s ", browser->he_selection->thread->comm);
+
+			if (choice == scripts_symbol)
+				sprintf(script_opt, " -S %s ", browser->he_selection->ms.sym->name);
+
+			script_browse(script_opt);
+		}
 	}
 out_free_stack:
 	pstack__delete(fstack);
-- 
1.7.1

[PATCH v3 7/9] perf header: Add check_perf_magic() func

[Feng Tang]

With this func, other modules can basically check whether a file
is a legal perf data file by checking its first 8 bytes aginst
all possible perf magic nunbers.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 tools/perf/util/header.c |   11 +++++++++++
 tools/perf/util/header.h |    1 +
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index ad72b28..555cb68 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2174,6 +2174,17 @@ static int try_all_pipe_abis(uint64_t hdr_sz, struct perf_header *ph)
 	return -1;
 }
 
+/* Return 0 if matched */
+int check_perf_magic(u64 magic)
+{
+	if (!memcmp(&magic, __perf_magic1, sizeof(magic))
+		|| magic == __perf_magic2
+		|| magic == __perf_magic2_sw)
+		return 0;
+
+	return -1;
+}
+
 static int check_magic_endian(u64 magic, uint64_t hdr_sz,
 			      bool is_pipe, struct perf_header *ph)
 {
diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index 58de08b..af1a51c 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -131,6 +131,7 @@ int perf_event__synthesize_build_id(struct perf_tool *tool,
 int perf_event__process_build_id(struct perf_tool *tool,
 				 union perf_event *event,
 				 struct perf_session *session);
+int check_perf_magic(u64 magic);
 
 /*
  * arch specific callback
-- 
1.7.1

[PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file

[Feng Tang]

Based on perf report/top/scripts browser integration idea from acme.

This will enable user to runtime swich the data file, when this option
is selected, it will popup all the legal data files in current working
directory, and the filename selected by user is saved in the global
variable "input_name", and a new key 'K_SWITCH_INPUT_DATA' will be
passed back and notify the builtin command that there is a pending data
switch, andd the command will perform the switch.

This initial version only enables it for 'perf report', by checking
the "timer" parameter of perf_evsel__hists_browser() equals NULL.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 tools/perf/ui/browsers/hists.c |   75 +++++++++++++++++++++++++++++++++++++++-
 tools/perf/ui/keysyms.h        |    1 +
 2 files changed, 75 insertions(+), 1 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index c491b78..7390614 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1125,6 +1125,67 @@ static inline void free_popup_options(char **options, int n)
 	}
 }
 
+static int switch_data_file(void)
+{
+	char *pwd, *options[256], *abs_path[256];
+	DIR *pwd_dir;
+	int nr_options = 0, choice = -1, ret = -1;
+
+	struct dirent *dent;
+
+	pwd = getenv("PWD");
+	if (!pwd)
+		return ret;
+
+	pwd_dir = opendir(pwd);
+	if (!pwd_dir)
+		return ret;
+
+	memset(options, 0, sizeof(options));
+	memset(options, 0, sizeof(abs_path));
+
+	while ((dent = readdir(pwd_dir))) {
+		char path[PATH_MAX];
+		u64 magic;
+		char *name = dent->d_name;
+		FILE *file;
+
+		if (!(dent->d_type == DT_REG))
+			continue;
+
+		snprintf(path, PATH_MAX, "%s/%s", pwd, name);
+
+		file = fopen(path, "r");
+		if (!file)
+			continue;
+
+		if (fread(&magic, 1, 8, file) < 8) {
+			fclose(file);
+			continue;
+		}
+
+		if (!check_perf_magic(magic)) {
+			options[nr_options] = strdup(name);
+			abs_path[nr_options++] = strdup(path);
+		}
+		fclose(file);
+	}
+	closedir(pwd_dir);
+
+	if (nr_options) {
+		choice = ui__popup_menu(nr_options, options);
+		if (choice < nr_options && choice >= 0) {
+			input_name = strdup(abs_path[choice]);
+			ret = 0;
+		}
+	}
+
+	free_popup_options(options, nr_options);
+	free_popup_options(abs_path, nr_options);
+	return ret;
+}
+
+
 static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 				    const char *helpline, const char *ev_name,
 				    bool left_exits,
@@ -1159,7 +1220,8 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		int choice = 0,
 		    annotate = -2, zoom_dso = -2, zoom_thread = -2,
 		    annotate_f = -2, annotate_t = -2, browse_map = -2;
-		int scripts_comm = -2, scripts_symbol = -2, scripts_all = -2;
+		int scripts_comm = -2, scripts_symbol = -2,
+		    scripts_all = -2, switch_data = -2;
 
 		nr_options = 0;
 
@@ -1340,6 +1402,9 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 		if (asprintf(&options[nr_options], "Run scripts for all samples") > 0)
 			scripts_all = nr_options++;
 
+		if (!timer && asprintf(&options[nr_options],
+				"Switch to another data file in current dir") > 0)
+			switch_data = nr_options++;
 add_exit_option:
 		options[nr_options++] = (char *)"Exit";
 retry_popup_menu:
@@ -1446,6 +1511,13 @@ do_scripts:
 
 			script_browse(script_opt);
 		}
+		/* Switch to another data file */
+		else if (choice == switch_data) {
+			if (!switch_data_file()) {
+				key = K_SWITCH_INPUT_DATA;
+				break;
+			}
+		}
 	}
 out_free_stack:
 	pstack__delete(fstack);
@@ -1560,6 +1632,7 @@ browse_hists:
 						"Do you really want to exit?"))
 					continue;
 				/* Fall thru */
+			case K_SWITCH_INPUT_DATA:
 			case 'q':
 			case CTRL('c'):
 				goto out;
diff --git a/tools/perf/ui/keysyms.h b/tools/perf/ui/keysyms.h
index 809eca5..65092d5 100644
--- a/tools/perf/ui/keysyms.h
+++ b/tools/perf/ui/keysyms.h
@@ -23,5 +23,6 @@
 #define K_TIMER	 -1
 #define K_ERROR	 -2
 #define K_RESIZE -3
+#define K_SWITCH_INPUT_DATA -4
 
 #endif /* _PERF_KEYSYMS_H_ */
-- 
1.7.1

[PATCH v3 9/9] perf report: Enable the runtime switching of perf data file

[Feng Tang]

This is for tui browser only. This patch will check the returned
key of tui hists browser, if it's K_SWITH_INPUT_DATA, then recreate
a session for the new selected data file.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 tools/perf/builtin-report.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index e4b2246..b4d216b 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -426,8 +426,14 @@ static int __cmd_report(struct perf_report *rep)
 
 	if (use_browser > 0) {
 		if (use_browser == 1) {
-			perf_evlist__tui_browse_hists(session->evlist, help,
-						      NULL, NULL, 0);
+			ret = perf_evlist__tui_browse_hists(session->evlist,
+						help, NULL, NULL, 0);
+			/*
+			 * Usually "ret" is the last pressed key, and we only
+			 * care whether we need to switch data file
+			 */
+			if (ret != K_SWITCH_INPUT_DATA)
+				ret = 0;
 		} else if (use_browser == 2) {
 			perf_evlist__gtk_browse_hists(session->evlist, help,
 						      NULL, NULL, 0);
@@ -661,6 +667,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		else
 			input_name = "perf.data";
 	}
+
+repeat:
 	session = perf_session__new(input_name, O_RDONLY,
 				    report.force, false, &report.tool);
 	if (session == NULL)
@@ -761,6 +769,12 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 	}
 
 	ret = __cmd_report(&report);
+	if (ret == K_SWITCH_INPUT_DATA) {
+		perf_session__delete(session);
+		goto repeat;
+	} else
+		ret = 0;
+
 error:
 	perf_session__delete(session);
 	return ret;
-- 
1.7.1

Re: [PATCH v3 7/9] perf header: Add check_perf_magic() func

[Arnaldo Carvalho de Melo]

Em Mon, Sep 24, 2012 at 11:24:09PM +0800, Feng Tang escreveu:
> With this func, other modules can basically check whether a file
> is a legal perf data file by checking its first 8 bytes aginst
> all possible perf magic nunbers.

Please consider submitting a patch to http://www.darwinsys.com/file/ :-)

- Arnaldo
 
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
>  tools/perf/util/header.c |   11 +++++++++++
>  tools/perf/util/header.h |    1 +
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index ad72b28..555cb68 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2174,6 +2174,17 @@ static int try_all_pipe_abis(uint64_t hdr_sz, struct perf_header *ph)
>  	return -1;
>  }
>  
> +/* Return 0 if matched */
> +int check_perf_magic(u64 magic)
> +{
> +	if (!memcmp(&magic, __perf_magic1, sizeof(magic))
> +		|| magic == __perf_magic2
> +		|| magic == __perf_magic2_sw)
> +		return 0;
> +
> +	return -1;
> +}
> +
>  static int check_magic_endian(u64 magic, uint64_t hdr_sz,
>  			      bool is_pipe, struct perf_header *ph)
>  {
> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
> index 58de08b..af1a51c 100644
> --- a/tools/perf/util/header.h
> +++ b/tools/perf/util/header.h
> @@ -131,6 +131,7 @@ int perf_event__synthesize_build_id(struct perf_tool *tool,
>  int perf_event__process_build_id(struct perf_tool *tool,
>  				 union perf_event *event,
>  				 struct perf_session *session);
> +int check_perf_magic(u64 magic);
>  
>  /*
>   * arch specific callback
> -- 
> 1.7.1

Re: [PATCH v3 1/9] perf hists: Move hists_init() from util/evsel.c to util/hist.c

[Arnaldo Carvalho de Melo]

Em Mon, Sep 24, 2012 at 11:24:03PM +0800, Feng Tang escreveu:
> Which looks more natural

It is there to avoid dragging the hist code into the python binding :-\

- Arnaldo
 
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
>  tools/perf/util/evsel.c |   10 ----------
>  tools/perf/util/evsel.h |    2 --
>  tools/perf/util/hist.c  |   10 ++++++++++
>  tools/perf/util/hist.h  |    1 +
>  4 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 1506ba0..4c7328d 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -39,16 +39,6 @@ static int __perf_evsel__sample_size(u64 sample_type)
>  	return size;
>  }
>  
> -void hists__init(struct hists *hists)
> -{
> -	memset(hists, 0, sizeof(*hists));
> -	hists->entries_in_array[0] = hists->entries_in_array[1] = RB_ROOT;
> -	hists->entries_in = &hists->entries_in_array[0];
> -	hists->entries_collapsed = RB_ROOT;
> -	hists->entries = RB_ROOT;
> -	pthread_mutex_init(&hists->lock, NULL);
> -}
> -
>  void perf_evsel__init(struct perf_evsel *evsel,
>  		      struct perf_event_attr *attr, int idx)
>  {
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 93876ba..af0bc97 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -197,8 +197,6 @@ static inline int perf_evsel__read_scaled(struct perf_evsel *evsel,
>  	return __perf_evsel__read(evsel, ncpus, nthreads, true);
>  }
>  
> -void hists__init(struct hists *hists);
> -
>  int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
>  			     struct perf_sample *sample, bool swapped);
>  
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 6ec5398..d3689e5 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -690,3 +690,13 @@ void hists__inc_nr_events(struct hists *hists, u32 type)
>  	++hists->stats.nr_events[0];
>  	++hists->stats.nr_events[type];
>  }
> +
> +void hists__init(struct hists *hists)
> +{
> +	memset(hists, 0, sizeof(*hists));
> +	hists->entries_in_array[0] = hists->entries_in_array[1] = RB_ROOT;
> +	hists->entries_in = &hists->entries_in_array[0];
> +	hists->entries_collapsed = RB_ROOT;
> +	hists->entries = RB_ROOT;
> +	pthread_mutex_init(&hists->lock, NULL);
> +}
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index f011ad4..4edfb74 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -70,6 +70,7 @@ struct hists {
>  	u16			col_len[HISTC_NR_COLS];
>  };
>  
> +void hists__init(struct hists *hists);
>  struct hist_entry *__hists__add_entry(struct hists *self,
>  				      struct addr_location *al,
>  				      struct symbol *parent, u64 period);
> -- 
> 1.7.1

Re: [PATCH v3 1/9] perf hists: Move hists_init() from util/evsel.c to util/hist.c

[Namhyung Kim]

On Mon, 24 Sep 2012 13:02:39 -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Sep 24, 2012 at 11:24:03PM +0800, Feng Tang escreveu:
>> Which looks more natural
>
> It is there to avoid dragging the hist code into the python binding :-\

Hmm... it's so hairy.  Can't we do better?

Thanks,
Namhyung

Re: [PATCH v3 3/9] perf script: Add more filter to find_scripts()

[Namhyung Kim]

Hi Feng,

On Mon, 24 Sep 2012 23:24:05 +0800, Feng Tang wrote:
> As suggested by Arnaldo, many scripts have their own usages and need
> capture specific events or tracepoints, so only those scripts whose
> targe events match the events in current perf data file should be
> listed in the script browser menu.
>
> This patch will add the event match checking.
>
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
>  tools/perf/builtin-script.c |   76 +++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 73 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 4e2f10f..37a0df8 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -1031,6 +1031,63 @@ static int list_available_scripts(const struct option *opt __maybe_unused,
>  }
>  
>  /*
> + * Some scripts specify the required events in their "xxx-record" file,
> + * this function will check if the events in perf.data match those
> + * mentioned in the "xxx-record".
> + */
> +static int check_ev_match(char *dir_name, char *scriptname,
> +			struct perf_session *session)
> +{
> +	char filename[MAXPATHLEN], evname[128];
> +	char line[BUFSIZ], *p, *temp;
> +	struct perf_evsel *pos;
> +	int match;
> +	FILE *fp;
> +
> +	sprintf(filename, "%s/bin/%s-record", dir_name, scriptname);
> +
> +	fp = fopen(filename, "r");
> +	if (!fp)
> +		return -1;
> +
> +	while (fgets(line, sizeof(line), fp)) {
> +		p = ltrim(line);
> +		if (strlen(p) == 0 ||*p == '#')
> +			continue;
> +
> +		while (strlen(p)) {
> +			temp = strstr(p, "-e");
> +			if (!temp)
> +				break;
> +
> +			p = temp + 3;
> +			temp = strchr(p, ' ');
> +			snprintf(evname, (temp - p) + 1, "%s", p);

It can't recognize extra spaces, multiple events connected by commas,
event groups and probably more..  So I think it'd better if we can use
parse_events() here - but w/o an evlist.  Jiri, what do you think?

Thanks,
Namhyung

Re: [PATCH v3 7/9] perf header: Add check_perf_magic() func

[Namhyung Kim]

On Mon, 24 Sep 2012 23:24:09 +0800, Feng Tang wrote:
[snip]
> +/* Return 0 if matched */
> +int check_perf_magic(u64 magic)
> +{
> +	if (!memcmp(&magic, __perf_magic1, sizeof(magic))
> +		|| magic == __perf_magic2
> +		|| magic == __perf_magic2_sw)
> +		return 0;
> +
> +	return -1;
> +}

Just an idea.  How about returning version number instead of 0 so that
it can be used elsewhere those check is needed and possibly wants to
know the version number also?

Thanks,
Namhyung

Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file

[Namhyung Kim]

On Mon, 24 Sep 2012 23:24:10 +0800, Feng Tang wrote:
[snip]
> +		if (!check_perf_magic(magic)) {
> +			options[nr_options] = strdup(name);
> +			abs_path[nr_options++] = strdup(path);

Need to check return values.


> +		}
> +		fclose(file);
> +	}
> +	closedir(pwd_dir);
> +
> +	if (nr_options) {
> +		choice = ui__popup_menu(nr_options, options);
> +		if (choice < nr_options && choice >= 0) {
> +			input_name = strdup(abs_path[choice]);

Ditto.  Plus it might leak previous input_name.

Thanks,
Namhyung


> +			ret = 0;
> +		}
> +	}
> +
> +	free_popup_options(options, nr_options);
> +	free_popup_options(abs_path, nr_options);
> +	return ret;
> +}
> +
> +

Re: [PATCH v3 1/9] perf hists: Move hists_init() from util/evsel.c to util/hist.c

[Feng Tang]

On Mon, 24 Sep 2012 13:02:39 -0300
Arnaldo Carvalho de Melo <acme@redhat.com> wrote:

> Em Mon, Sep 24, 2012 at 11:24:03PM +0800, Feng Tang escreveu:
> > Which looks more natural
> 
> It is there to avoid dragging the hist code into the python binding :-\

Didn't notice this, please ignore this one.

Thanks,
Feng

> 
> - Arnaldo
>  
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > ---
> >  tools/perf/util/evsel.c |   10 ----------
> >  tools/perf/util/evsel.h |    2 --
> >  tools/perf/util/hist.c  |   10 ++++++++++
> >  tools/perf/util/hist.h  |    1 +
> >  4 files changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index 1506ba0..4c7328d 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -39,16 +39,6 @@ static int __perf_evsel__sample_size(u64 sample_type)
> >  	return size;
> >  }
> >  
> > -void hists__init(struct hists *hists)
> > -{
> > -	memset(hists, 0, sizeof(*hists));
> > -	hists->entries_in_array[0] = hists->entries_in_array[1] = RB_ROOT;
> > -	hists->entries_in = &hists->entries_in_array[0];
> > -	hists->entries_collapsed = RB_ROOT;
> > -	hists->entries = RB_ROOT;
> > -	pthread_mutex_init(&hists->lock, NULL);
> > -}
> > -
> >  void perf_evsel__init(struct perf_evsel *evsel,
> >  		      struct perf_event_attr *attr, int idx)
> >  {
> > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > index 93876ba..af0bc97 100644
> > --- a/tools/perf/util/evsel.h
> > +++ b/tools/perf/util/evsel.h
> > @@ -197,8 +197,6 @@ static inline int perf_evsel__read_scaled(struct perf_evsel *evsel,
> >  	return __perf_evsel__read(evsel, ncpus, nthreads, true);
> >  }
> >  
> > -void hists__init(struct hists *hists);
> > -
> >  int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event *event,
> >  			     struct perf_sample *sample, bool swapped);
> >  
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index 6ec5398..d3689e5 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -690,3 +690,13 @@ void hists__inc_nr_events(struct hists *hists, u32 type)
> >  	++hists->stats.nr_events[0];
> >  	++hists->stats.nr_events[type];
> >  }
> > +
> > +void hists__init(struct hists *hists)
> > +{
> > +	memset(hists, 0, sizeof(*hists));
> > +	hists->entries_in_array[0] = hists->entries_in_array[1] = RB_ROOT;
> > +	hists->entries_in = &hists->entries_in_array[0];
> > +	hists->entries_collapsed = RB_ROOT;
> > +	hists->entries = RB_ROOT;
> > +	pthread_mutex_init(&hists->lock, NULL);
> > +}
> > diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> > index f011ad4..4edfb74 100644
> > --- a/tools/perf/util/hist.h
> > +++ b/tools/perf/util/hist.h
> > @@ -70,6 +70,7 @@ struct hists {
> >  	u16			col_len[HISTC_NR_COLS];
> >  };
> >  
> > +void hists__init(struct hists *hists);
> >  struct hist_entry *__hists__add_entry(struct hists *self,
> >  				      struct addr_location *al,
> >  				      struct symbol *parent, u64 period);
> > -- 
> > 1.7.1

Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file

[Feng Tang]

On Tue, 25 Sep 2012 11:11:21 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> On Mon, 24 Sep 2012 23:24:10 +0800, Feng Tang wrote:
> [snip]
> > +		if (!check_perf_magic(magic)) {
> > +			options[nr_options] = strdup(name);
> > +			abs_path[nr_options++] = strdup(path);
> 
> Need to check return values.
> 
> 
> > +		}
> > +		fclose(file);
> > +	}
> > +	closedir(pwd_dir);
> > +
> > +	if (nr_options) {
> > +		choice = ui__popup_menu(nr_options, options);
> > +		if (choice < nr_options && choice >= 0) {
> > +			input_name = strdup(abs_path[choice]);
> 
> Ditto.  Plus it might leak previous input_name.

Nice catch, will check the return value of "strdup". 

For input_name mem leak, in some cases the input_name can't be called
 with free(), like those got from parse "-i" option. In case the old
input_name is got from malloc through strdup, I think it's not a big
issue given that buffer will be freed any way when the application exit.

Thanks,
Feng

> 
> Thanks,
> Namhyung
> 
> 
> > +			ret = 0;
> > +		}
> > +	}
> > +
> > +	free_popup_options(options, nr_options);
> > +	free_popup_options(abs_path, nr_options);
> > +	return ret;
> > +}
> > +
> > +

Re: [PATCH v3 7/9] perf header: Add check_perf_magic() func

[Feng Tang]

On Tue, 25 Sep 2012 11:07:15 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> On Mon, 24 Sep 2012 23:24:09 +0800, Feng Tang wrote:
> [snip]
> > +/* Return 0 if matched */
> > +int check_perf_magic(u64 magic)
> > +{
> > +	if (!memcmp(&magic, __perf_magic1, sizeof(magic))
> > +		|| magic == __perf_magic2
> > +		|| magic == __perf_magic2_sw)
> > +		return 0;
> > +
> > +	return -1;
> > +}
> 
> Just an idea.  How about returning version number instead of 0 so that
> it can be used elsewhere those check is needed and possibly wants to
> know the version number also?

Sounds ok to me, I can add that if nobody has objection.

Thanks,
Feng

Re: [PATCH v3 1/9] perf hists: Move hists_init() from util/evsel.c to util/hist.c

[Arnaldo Carvalho de Melo]

Em Tue, Sep 25, 2012 at 10:25:13AM +0900, Namhyung Kim escreveu:
> On Mon, 24 Sep 2012 13:02:39 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Sep 24, 2012 at 11:24:03PM +0800, Feng Tang escreveu:
> >> Which looks more natural
> >
> > It is there to avoid dragging the hist code into the python binding :-\
> 
> Hmm... it's so hairy.  Can't we do better?

We always can do better :-)

I just stated why it was at that place.

When doing refactorings we're all the time trying to make it better in
many senses, one of them is trying to isolate code that is useful in a
general way and thus should be made available via a library/scripting
binding.

- Arnaldo

Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file

[Arnaldo Carvalho de Melo]

Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu:
> On Tue, 25 Sep 2012 11:11:21 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
> > Ditto.  Plus it might leak previous input_name.
> 
> Nice catch, will check the return value of "strdup". 
> 
> For input_name mem leak, in some cases the input_name can't be called
>  with free(), like those got from parse "-i" option. In case the old
> input_name is got from malloc through strdup, I think it's not a big
> issue given that buffer will be freed any way when the application exit.

I think this is a matter of discipline, leaking is bad, kernel or
userspace, why special case it?

- Arnaldo

Re: [PATCH v3 7/9] perf header: Add check_perf_magic() func

[Arnaldo Carvalho de Melo]

Em Tue, Sep 25, 2012 at 04:21:40PM +0800, Feng Tang escreveu:
> On Tue, 25 Sep 2012 11:07:15 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
> 
> > On Mon, 24 Sep 2012 23:24:09 +0800, Feng Tang wrote:
> > [snip]
> > > +/* Return 0 if matched */
> > > +int check_perf_magic(u64 magic)
> > > +{
> > > +	if (!memcmp(&magic, __perf_magic1, sizeof(magic))
> > > +		|| magic == __perf_magic2
> > > +		|| magic == __perf_magic2_sw)
> > > +		return 0;
> > > +
> > > +	return -1;
> > > +}
> > 
> > Just an idea.  How about returning version number instead of 0 so that
> > it can be used elsewhere those check is needed and possibly wants to
> > know the version number also?
 
> Sounds ok to me, I can add that if nobody has objection.

Leave it for when we need it?

What we need now is to see if a file is a perf.data file, so I think the
function is good as is.

Minor nitpicks: use bool, rename it to is_perf_magic(), no need for that
comment then, it becomes obvious from the function name :-)

- Arnaldo

Re: [PATCH v3 1/9] perf hists: Move hists_init() from util/evsel.c to util/hist.c

[Namhyung Kim]

2012-09-25 (화), 08:05 -0300, Arnaldo Carvalho de Melo:
> Em Tue, Sep 25, 2012 at 10:25:13AM +0900, Namhyung Kim escreveu:
> > On Mon, 24 Sep 2012 13:02:39 -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Mon, Sep 24, 2012 at 11:24:03PM +0800, Feng Tang escreveu:
> > >> Which looks more natural
> > >
> > > It is there to avoid dragging the hist code into the python binding :-\
> > 
> > Hmm... it's so hairy.  Can't we do better?
> 
> We always can do better :-)
> 
> I just stated why it was at that place.
> 
> When doing refactorings we're all the time trying to make it better in
> many senses, one of them is trying to isolate code that is useful in a
> general way and thus should be made available via a library/scripting
> binding.

Yeah, but the isolation sometimes got broken as code getting added like
this.  So we need a automatic way of detecting breakage.

I thought about adding a perf test entry running python/twatch.py, but
it will not work for an installed perf binary since it cannot find the
twatch.py script and perf.so extension files which are not installed.

Now I'm thinking of making it build-time test so that it can be executed
by make when specific argument is given - e.g. make C=1 ?

Thanks
Namhyung

perf tools regression testing was Re: [PATCH v3 1/9] perf hists: Move hists_init() from util/evsel.c to util/hist.c

[Arnaldo Carvalho de Melo]

Em Tue, Sep 25, 2012 at 09:59:02PM +0900, Namhyung Kim escreveu:
> 2012-09-25 (화), 08:05 -0300, Arnaldo Carvalho de Melo:
> > Em Tue, Sep 25, 2012 at 10:25:13AM +0900, Namhyung Kim escreveu:
> > > On Mon, 24 Sep 2012 13:02:39 -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Mon, Sep 24, 2012 at 11:24:03PM +0800, Feng Tang escreveu:
> > > >> Which looks more natural

> > > > It is there to avoid dragging the hist code into the python binding :-\

> > > Hmm... it's so hairy.  Can't we do better?

> > We always can do better :-)

> > I just stated why it was at that place.

> > When doing refactorings we're all the time trying to make it better in
> > many senses, one of them is trying to isolate code that is useful in a
> > general way and thus should be made available via a library/scripting
> > binding.
 
> Yeah, but the isolation sometimes got broken as code getting added like
> this.  So we need a automatic way of detecting breakage.

Agreed.
 
> I thought about adding a perf test entry running python/twatch.py, but
> it will not work for an installed perf binary since it cannot find the
> twatch.py script and perf.so extension files which are not installed.

I thought about doing it the way 'perf script' works with python, i.e.
calling the python interpreter, setting the PYTHONPATH thru it, then
asking for 'use python' to happen, catch the result.
 
> Now I'm thinking of making it build-time test so that it can be executed
> by make when specific argument is given - e.g. make C=1 ?

I think there is room for a 'make -C tools/perf check' that would use
the 'expect' tool to do not just this but also run record, report, etc
and check its output against what is expected, perf test is ok for
checking the APIs, but we need a test suite for the actual builtins as
called from the command line.

- Arnaldo

Re: perf tools regression testing was Re: [PATCH v3 1/9] perf hists: Move hists_init() from util/evsel.c to util/hist.c

[Namhyung Kim]

2012-09-25 (화), 10:30 -0300, Arnaldo Carvalho de Melo:
> Em Tue, Sep 25, 2012 at 09:59:02PM +0900, Namhyung Kim escreveu:
> > Now I'm thinking of making it build-time test so that it can be executed
> > by make when specific argument is given - e.g. make C=1 ?
> 
> I think there is room for a 'make -C tools/perf check' that would use
> the 'expect' tool to do not just this but also run record, report, etc
> and check its output against what is expected, perf test is ok for
> checking the APIs, but we need a test suite for the actual builtins as
> called from the command line.

Hmm.. we have 'make check' but running it ended up tons of macro
redefinition and unknown attribute warnings from sparse. :/

Thanks,
Namhyung

Re: perf tools regression testing was Re: [PATCH v3 1/9] perf hists: Move hists_init() from util/evsel.c to util/hist.c

[Arnaldo Carvalho de Melo]

Em Tue, Sep 25, 2012 at 10:47:48PM +0900, Namhyung Kim escreveu:
> 2012-09-25 (화), 10:30 -0300, Arnaldo Carvalho de Melo:
> > Em Tue, Sep 25, 2012 at 09:59:02PM +0900, Namhyung Kim escreveu:
> > > Now I'm thinking of making it build-time test so that it can be executed
> > > by make when specific argument is given - e.g. make C=1 ?

> > I think there is room for a 'make -C tools/perf check' that would use
> > the 'expect' tool to do not just this but also run record, report, etc
> > and check its output against what is expected, perf test is ok for
> > checking the APIs, but we need a test suite for the actual builtins as
> > called from the command line.

> Hmm.. we have 'make check' but running it ended up tons of macro
> redefinition and unknown attribute warnings from sparse. :/

Ok, then 'make test', that would run 'perf test' + the expect like
tests.

I'm trying to figure out if http://www.noah.org/python/pexpect/ is a
better choice, that way we don't have to learn yet another scripting
language.

- Arnaldo

Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file

[Feng Tang]

On Tue, Sep 25, 2012 at 08:17:03AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu:
> > On Tue, 25 Sep 2012 11:11:21 +0900
> > Namhyung Kim <namhyung@kernel.org> wrote:
> > > Ditto.  Plus it might leak previous input_name.
> > 
> > Nice catch, will check the return value of "strdup". 
> > 
> > For input_name mem leak, in some cases the input_name can't be called
> >  with free(), like those got from parse "-i" option. In case the old
> > input_name is got from malloc through strdup, I think it's not a big
> > issue given that buffer will be freed any way when the application exit.
> 
> I think this is a matter of discipline, leaking is bad, kernel or
> userspace, why special case it?
>

I see, then we need make sure all "input_name" point to a malloced buffer, 
here is a initial debug patch will only touch the annotate/report/script
cmds, pls review and more suggestions are welcomed:

----
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index b3d60ae..5165660 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -288,6 +288,8 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	argc = parse_options(argc, argv, options, annotate_usage, 0);
 
+	try_dup_input_name();
+
 	if (annotate.use_stdio)
 		use_browser = 0;
 	else if (annotate.use_tui)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index b4d216b..bec614c 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -668,6 +668,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 			input_name = "perf.data";
 	}
 
+	try_dup_input_name();
+
 repeat:
 	session = perf_session__new(input_name, O_RDONLY,
 				    report.force, false, &report.tool);
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 30e880c..f13df66 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1477,6 +1477,8 @@ int cmd_script(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (!script_name)
 		setup_pager();
 
+	try_dup_input_name();
+
 	session = perf_session__new(input_name, O_RDONLY, 0, false,
 				    &perf_script);
 	if (session == NULL)
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 2a09de0..d6a097e 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -64,6 +64,20 @@ struct pager_config {
 	int val;
 };
 
+/*
+ * "input_name" may point to a memory regsion got or not got from
+ * malloc(), this fun will enforce it to point to a malloced region,
+ * this is to avoid memory leak when runtime siwtching perf data file.
+ */
+void try_dup_input_name(void)
+{
+	if (input_name) {
+		input_name = strdup(input_name);
+		if (!input_name)
+			printf("Warning: Fail to duplicate input_name\n");
+	}
+}
+
 static int pager_command_config(const char *var, const char *value, void *data)
 {
 	struct pager_config *c = data;
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 42da932..1159213 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -207,6 +207,7 @@ extern bool perf_host, perf_guest;
 extern const char perf_version_string[];
 
 void pthread__unblock_sigwinch(void);
+void try_dup_input_name(void);
 
 #include "util/target.h"
 

Thanks,
Feng

Re: [PATCH v3 3/9] perf script: Add more filter to find_scripts()

[Feng Tang]

On Tue, 25 Sep 2012 10:47:03 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> Hi Feng,
> 
> On Mon, 24 Sep 2012 23:24:05 +0800, Feng Tang wrote:
> > As suggested by Arnaldo, many scripts have their own usages and need
> > capture specific events or tracepoints, so only those scripts whose
> > targe events match the events in current perf data file should be
> > listed in the script browser menu.
> >
> > This patch will add the event match checking.
> >
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > ---
> >  tools/perf/builtin-script.c |   76 +++++++++++++++++++++++++++++++++++++++++--
> >  1 files changed, 73 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > index 4e2f10f..37a0df8 100644
> > --- a/tools/perf/builtin-script.c
> > +++ b/tools/perf/builtin-script.c
> > @@ -1031,6 +1031,63 @@ static int list_available_scripts(const struct option *opt __maybe_unused,
> >  }
> >  
> >  /*
> > + * Some scripts specify the required events in their "xxx-record" file,
> > + * this function will check if the events in perf.data match those
> > + * mentioned in the "xxx-record".
> > + */
> > +static int check_ev_match(char *dir_name, char *scriptname,
> > +			struct perf_session *session)
> > +{
> > +	char filename[MAXPATHLEN], evname[128];
> > +	char line[BUFSIZ], *p, *temp;
> > +	struct perf_evsel *pos;
> > +	int match;
> > +	FILE *fp;
> > +
> > +	sprintf(filename, "%s/bin/%s-record", dir_name, scriptname);
> > +
> > +	fp = fopen(filename, "r");
> > +	if (!fp)
> > +		return -1;
> > +
> > +	while (fgets(line, sizeof(line), fp)) {
> > +		p = ltrim(line);
> > +		if (strlen(p) == 0 ||*p == '#')
> > +			continue;
> > +
> > +		while (strlen(p)) {
> > +			temp = strstr(p, "-e");
> > +			if (!temp)
> > +				break;
> > +
> > +			p = temp + 3;
> > +			temp = strchr(p, ' ');
> > +			snprintf(evname, (temp - p) + 1, "%s", p);
> 
> It can't recognize extra spaces, multiple events connected by commas,
> event groups and probably more..  So I think it'd better if we can use
> parse_events() here - but w/o an evlist.  Jiri, what do you think?

Yes, this func was really a pain to me :) And fortunately, current
intree "xxx-record" scripts are all in simple format: "-e eventname ",
and this lightweight func basically works fine.

I agree that we'd better have a separate parse_events() similar func 
to cherry-pick the events names in parse-events.c, to not make
hists.c too heavy. 

And frankly speaking, I'm not familiar at all with these flex/bison
handling, so can we do things in steps: fixes those space issue, add
a "fixme" in comments and push the basically working version first?

Any comments? Thanks

- Feng

Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file

[Namhyung Kim]

Hi Feng,

On Wed, 26 Sep 2012 15:57:07 +0800, Feng Tang wrote:
> On Tue, Sep 25, 2012 at 08:17:03AM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu:
>> > On Tue, 25 Sep 2012 11:11:21 +0900
>> > Namhyung Kim <namhyung@kernel.org> wrote:
>> > > Ditto.  Plus it might leak previous input_name.
>> > 
>> > Nice catch, will check the return value of "strdup". 
>> > 
>> > For input_name mem leak, in some cases the input_name can't be called
>> >  with free(), like those got from parse "-i" option. In case the old
>> > input_name is got from malloc through strdup, I think it's not a big
>> > issue given that buffer will be freed any way when the application exit.
>> 
>> I think this is a matter of discipline, leaking is bad, kernel or
>> userspace, why special case it?
>>
>
> I see, then we need make sure all "input_name" point to a malloced buffer, 
> here is a initial debug patch will only touch the annotate/report/script
> cmds, pls review and more suggestions are welcomed:

Well, how about adding a flag like "input_name_alloced" and checking it
before new allocation?  This way we can avoid needless strdup when
runtime switching is not used.

Thanks,
Namhyung

Re: [PATCH v3 3/9] perf script: Add more filter to find_scripts()

[Namhyung Kim]

On Wed, 26 Sep 2012 16:56:41 +0800, Feng Tang wrote:
> On Tue, 25 Sep 2012 10:47:03 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
>> It can't recognize extra spaces, multiple events connected by commas,
>> event groups and probably more..  So I think it'd better if we can use
>> parse_events() here - but w/o an evlist.  Jiri, what do you think?
>
> Yes, this func was really a pain to me :) And fortunately, current
> intree "xxx-record" scripts are all in simple format: "-e eventname ",
> and this lightweight func basically works fine.
>
> I agree that we'd better have a separate parse_events() similar func 
> to cherry-pick the events names in parse-events.c, to not make
> hists.c too heavy. 
>
> And frankly speaking, I'm not familiar at all with these flex/bison
> handling, so can we do things in steps: fixes those space issue, add
> a "fixme" in comments and push the basically working version first?

It'd not that hard.  How about using below (untested) patch?

Thanks,
Namhyung


diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index bf5d033ee1b4..5dc71f598028 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -817,6 +817,21 @@ int parse_events_terms(struct list_head *terms, const char *str)
 	return ret;
 }
 
+int __parse_events(struct list_head *evsel_list, const char *str)
+{
+	int ret;
+	struct parse_events_data__events data = {
+		.list = LIST_HEAD_INIT(data.list),
+	};
+
+	ret = parse_events__scanner(str, &data, PE_START_EVENTS);
+	if (!ret) {
+		list_splice(evsel_list, &data.list);
+		return 0;
+	}
+	return -1;
+}
+
 int parse_events(struct perf_evlist *evlist, const char *str,
 		 int unset __maybe_unused)
 {
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index c356e443448d..c13e9bd0f041 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -32,6 +32,7 @@ extern int parse_events_option(const struct option *opt, const char *str,
 extern int parse_events(struct perf_evlist *evlist, const char *str,
 			int unset);
 extern int parse_events_terms(struct list_head *terms, const char *str);
+extern int __parse_events(struct list_head *evsel_list, const char *str);
 extern int parse_filter(const struct option *opt, const char *str, int unset);
 
 #define EVENTS_HELP_MAX (128*1024)

Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file

[Feng Tang]

Hi Namhyung,

On Thu, Sep 27, 2012 at 01:02:05PM +0900, Namhyung Kim wrote:
> Hi Feng,
> 
> On Wed, 26 Sep 2012 15:57:07 +0800, Feng Tang wrote:
> > On Tue, Sep 25, 2012 at 08:17:03AM -0300, Arnaldo Carvalho de Melo wrote:
> >> Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu:
> >> > On Tue, 25 Sep 2012 11:11:21 +0900
> >> > Namhyung Kim <namhyung@kernel.org> wrote:
> >> > > Ditto.  Plus it might leak previous input_name.
> >> > 
> >> > Nice catch, will check the return value of "strdup". 
> >> > 
> >> > For input_name mem leak, in some cases the input_name can't be called
> >> >  with free(), like those got from parse "-i" option. In case the old
> >> > input_name is got from malloc through strdup, I think it's not a big
> >> > issue given that buffer will be freed any way when the application exit.
> >> 
> >> I think this is a matter of discipline, leaking is bad, kernel or
> >> userspace, why special case it?
> >>
> >
> > I see, then we need make sure all "input_name" point to a malloced buffer, 
> > here is a initial debug patch will only touch the annotate/report/script
> > cmds, pls review and more suggestions are welcomed:
> 
> Well, how about adding a flag like "input_name_alloced" and checking it
> before new allocation?  This way we can avoid needless strdup when
> runtime switching is not used.

Right, myself have checked that only the runtime switch will malloc a
buffer for the new "input_name", IOW "input_name" in other case can't be
called with free(). So one flag only inside hists.c is ok to cover this,
like the following code.

btw, thanks for your patch about the parse-events()! will try it out.

- Feng

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 6048820..c29335a 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1125,6 +1125,13 @@ static inline void free_popup_options(char **options, int n)
 	}
 }
 
+/*
+ * Only runtime switching of perf data file will make "input_name" point
+ * to a malloced buffer. So add "is_input_name_malloced" flag to decide
+ * whether we need to call free() for current "input_name" during the switch.
+ */
+static bool is_input_name_malloced = false;
+
 static int switch_data_file(void)
 {
 	char *pwd, *options[256], *abs_path[256], *tmp;
@@ -1186,8 +1193,10 @@ close_file_and_continue:
 		if (choice < nr_options && choice >= 0) {
 			tmp = strdup(abs_path[choice]);
 			if (tmp) {
+				if (is_input_name_malloced)
+					free((void *)input_name);
 				input_name = tmp;
+				is_input_name_malloced = true;
 				ret = 0;
 			}
 		}

Re: [PATCH v3 3/9] perf script: Add more filter to find_scripts()

[Namhyung Kim]

On Thu, 27 Sep 2012 13:45:30 +0900, Namhyung Kim wrote:
> It'd not that hard.  How about using below (untested) patch?

Hmm... it won't work for some cases.  Please ignore this.

Thanks,
Namhyung

>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index bf5d033ee1b4..5dc71f598028 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -817,6 +817,21 @@ int parse_events_terms(struct list_head *terms, const char *str)
>  	return ret;
>  }
>  
> +int __parse_events(struct list_head *evsel_list, const char *str)
> +{
> +	int ret;
> +	struct parse_events_data__events data = {
> +		.list = LIST_HEAD_INIT(data.list),
> +	};
> +
> +	ret = parse_events__scanner(str, &data, PE_START_EVENTS);
> +	if (!ret) {
> +		list_splice(evsel_list, &data.list);
> +		return 0;
> +	}
> +	return -1;
> +}
> +
>  int parse_events(struct perf_evlist *evlist, const char *str,
>  		 int unset __maybe_unused)
>  {
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index c356e443448d..c13e9bd0f041 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -32,6 +32,7 @@ extern int parse_events_option(const struct option *opt, const char *str,
>  extern int parse_events(struct perf_evlist *evlist, const char *str,
>  			int unset);
>  extern int parse_events_terms(struct list_head *terms, const char *str);
> +extern int __parse_events(struct list_head *evsel_list, const char *str);
>  extern int parse_filter(const struct option *opt, const char *str, int unset);
>  
>  #define EVENTS_HELP_MAX (128*1024)