linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] perf ui/browser: Add browser for perf script
@ 2012-09-03  8:14 Feng Tang
  2012-09-03  8:14 ` [PATCH 1/7] perf symbols: Filter samples with unresolved symbol when "--symbols" option is used Feng Tang
                   ` (7 more replies)
  0 siblings, 8 replies; 14+ messages in thread
From: Feng Tang @ 2012-09-03  8:14 UTC (permalink / raw)
  To: acme
  Cc: mingo, a.p.zijlstra, andi, namhyung, dsahern, linux-kernel, 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.

Patch 1-4 are some preparation for adding the script
patch 5 introduce the script browser
patch 6-7 integrate the browser to hists browser and annotation browser

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

Feng
-----------------------
Feng Tang (7):
  perf symbols: Filter samples with unresolved symbol when "--symbols"
    option is used
  perf scripts: Add --symbols option to handle specific symbols
  perf scripts: Add event_analyzing_sample-record/report
  perf scripts: Export a find_scripts() function
  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

 tools/perf/Makefile                                |    5 +
 tools/perf/builtin-script.c                        |   49 ++++++
 tools/perf/builtin.h                               |    1 +
 .../python/bin/event_analyzing_sample-record       |    8 +
 .../python/bin/event_analyzing_sample-report       |    3 +
 tools/perf/ui/browsers/annotate.c                  |    8 +
 tools/perf/ui/browsers/hists.c                     |   39 +++++
 tools/perf/ui/browsers/scripts.c                   |  159 ++++++++++++++++++++
 tools/perf/ui/browsers/scripts.h                   |    5 +
 tools/perf/util/event.c                            |    5 +-
 10 files changed, 280 insertions(+), 2 deletions(-)
 create mode 100644 tools/perf/scripts/python/bin/event_analyzing_sample-record
 create mode 100644 tools/perf/scripts/python/bin/event_analyzing_sample-report
 create mode 100644 tools/perf/ui/browsers/scripts.c
 create mode 100644 tools/perf/ui/browsers/scripts.h


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/7] perf symbols: Filter samples with unresolved symbol when "--symbols" option is used
  2012-09-03  8:14 [PATCH 0/7] perf ui/browser: Add browser for perf script Feng Tang
@ 2012-09-03  8:14 ` Feng Tang
  2012-09-03  8:14 ` [PATCH 2/7] perf scripts: Add --symbols option to handle specific symbols Feng Tang
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Feng Tang @ 2012-09-03  8:14 UTC (permalink / raw)
  To: acme
  Cc: mingo, a.p.zijlstra, andi, namhyung, dsahern, linux-kernel, Feng Tang

Report/top commands support to only handle specific symbols with
"--symbols" option, but current code will keep those samples whose
symbol can't be resolved, which should actually be filtered.

If we run following commands:
$perf record -a tree
$perf report --symbols intel_idle -n
the output will be:

Without the patch:
==================
    46.27%        156     sshd  [unknown]
    26.05%         48  swapper  [kernel.kallsyms]
    17.26%         38     tree  libc-2.12.1.so
     7.69%         17     tree  tree
     2.73%          6     tree  ld-2.12.1.so

With the patch:
===============
   100.00%         48  swapper  [kernel.kallsyms]

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 tools/perf/util/event.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index 3a0f1a5..08910f0 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -886,8 +886,9 @@ int perf_event__preprocess_sample(const union perf_event *event,
 		al->sym = map__find_symbol(al->map, al->addr, filter);
 	}
 
-	if (symbol_conf.sym_list && al->sym &&
-	    !strlist__has_entry(symbol_conf.sym_list, al->sym->name))
+	if (symbol_conf.sym_list &&
+		(!al->sym || !strlist__has_entry(symbol_conf.sym_list,
+						al->sym->name)))
 		goto out_filtered;
 
 	return 0;
-- 
1.7.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/7] perf scripts: Add --symbols option to handle specific symbols
  2012-09-03  8:14 [PATCH 0/7] perf ui/browser: Add browser for perf script Feng Tang
  2012-09-03  8:14 ` [PATCH 1/7] perf symbols: Filter samples with unresolved symbol when "--symbols" option is used Feng Tang
@ 2012-09-03  8:14 ` Feng Tang
  2012-09-03  8:14 ` [PATCH 3/7] perf scripts: Add event_analyzing_sample-record/report Feng Tang
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Feng Tang @ 2012-09-03  8:14 UTC (permalink / raw)
  To: acme
  Cc: mingo, a.p.zijlstra, andi, namhyung, dsahern, linux-kernel, Feng Tang

Since perf script no longer only handle the trace points, we can
add the symbol filter option so that scripts can handle specified
samples.

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

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 2d6e3b2..7ab86bb 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -14,6 +14,7 @@
 #include "util/util.h"
 #include "util/evlist.h"
 #include "util/evsel.h"
+#include "util/sort.h"
 #include <linux/bitmap.h>
 
 static char const		*script_name;
@@ -1142,6 +1143,8 @@ static const struct option options[] = {
 		     parse_output_fields),
 	OPT_BOOLEAN('a', "all-cpus", &system_wide,
 		     "system-wide collection from all CPUs"),
+	OPT_STRING('S', "symbols", &symbol_conf.sym_list_str, "symbol[,symbol...]",
+		   "only consider these symbols"),
 	OPT_STRING('C', "cpu", &cpu_list, "cpu", "list of cpus to profile"),
 	OPT_STRING('c', "comms", &symbol_conf.comm_list_str, "comm[,comm...]",
 		   "only display events for these comms"),
-- 
1.7.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 3/7] perf scripts: Add event_analyzing_sample-record/report
  2012-09-03  8:14 [PATCH 0/7] perf ui/browser: Add browser for perf script Feng Tang
  2012-09-03  8:14 ` [PATCH 1/7] perf symbols: Filter samples with unresolved symbol when "--symbols" option is used Feng Tang
  2012-09-03  8:14 ` [PATCH 2/7] perf scripts: Add --symbols option to handle specific symbols Feng Tang
@ 2012-09-03  8:14 ` Feng Tang
  2012-09-03  8:14 ` [PATCH 4/7] perf scripts: Export a find_scripts() function Feng Tang
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Feng Tang @ 2012-09-03  8:14 UTC (permalink / raw)
  To: acme
  Cc: mingo, a.p.zijlstra, andi, namhyung, dsahern, linux-kernel, Feng Tang

So that event_analyzing_sample.py can be shown by "perf script -l"

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 .../python/bin/event_analyzing_sample-record       |    8 ++++++++
 .../python/bin/event_analyzing_sample-report       |    3 +++
 2 files changed, 11 insertions(+), 0 deletions(-)
 create mode 100644 tools/perf/scripts/python/bin/event_analyzing_sample-record
 create mode 100644 tools/perf/scripts/python/bin/event_analyzing_sample-report

diff --git a/tools/perf/scripts/python/bin/event_analyzing_sample-record b/tools/perf/scripts/python/bin/event_analyzing_sample-record
new file mode 100644
index 0000000..0d03b41
--- /dev/null
+++ b/tools/perf/scripts/python/bin/event_analyzing_sample-record
@@ -0,0 +1,8 @@
+#!/bin/bash
+
+#
+# event_analyzing_sample.py can cover all type of perf samples including
+# the tracepoints, so no special record requirements, just record what
+# you want to analyze.
+#
+perf record $@
diff --git a/tools/perf/scripts/python/bin/event_analyzing_sample-report b/tools/perf/scripts/python/bin/event_analyzing_sample-report
new file mode 100644
index 0000000..e04f3a6
--- /dev/null
+++ b/tools/perf/scripts/python/bin/event_analyzing_sample-report
@@ -0,0 +1,3 @@
+#!/bin/bash
+# description: analyze all perf samples
+perf script $@ -s "$PERF_EXEC_PATH"/scripts/python/event_analyzing_sample.py
--
1.7.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 4/7] perf scripts: Export a find_scripts() function
  2012-09-03  8:14 [PATCH 0/7] perf ui/browser: Add browser for perf script Feng Tang
                   ` (2 preceding siblings ...)
  2012-09-03  8:14 ` [PATCH 3/7] perf scripts: Add event_analyzing_sample-record/report Feng Tang
@ 2012-09-03  8:14 ` Feng Tang
  2012-09-04  1:09   ` Namhyung Kim
  2012-09-04  1:34   ` Namhyung Kim
  2012-09-03  8:14 ` [PATCH 5/7] perf ui/browser: Add a browser for perf script Feng Tang
                   ` (3 subsequent siblings)
  7 siblings, 2 replies; 14+ messages in thread
From: Feng Tang @ 2012-09-03  8:14 UTC (permalink / raw)
  To: acme
  Cc: mingo, a.p.zijlstra, andi, namhyung, dsahern, linux-kernel, Feng Tang

So that other perf commands/browser has a way to dig out the available
scripts info in system, this is a preparation for the script browser.

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

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 7ab86bb..d46056c 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1031,6 +1031,52 @@ static int list_available_scripts(const struct option *opt __used,
 	exit(0);
 }
 
+/*
+ * Return 0 if none is found, otherwise the actual scripts number.
+ *
+ * Currently we just return all the raw scripts name, and their full path.
+ *
+ */
+int find_scripts(char **scripts_array, char **scripts_path_array)
+{
+	struct dirent *script_next, *lang_next, script_dirent, lang_dirent;
+	char scripts_path[MAXPATHLEN];
+	DIR *scripts_dir, *lang_dir;
+	char lang_path[MAXPATHLEN];
+	struct script_desc *desc;
+	char first_half[BUFSIZ];
+	char *temp;
+	int i = 0;
+
+	first_half[0] = 0;
+	desc = NULL;
+
+	snprintf(scripts_path, MAXPATHLEN, "%s/scripts", perf_exec_path());
+
+	scripts_dir = opendir(scripts_path);
+	if (!scripts_dir)
+		return -1;
+
+	for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next) {
+		snprintf(lang_path, MAXPATHLEN, "%s/%s", scripts_path,
+			 lang_dirent.d_name);
+		lang_dir = opendir(lang_path);
+		if (!lang_dir)
+			continue;
+
+		for_each_script(lang_path, lang_dir, script_dirent, script_next) {
+			sprintf(scripts_path_array[i], "%s/%s", lang_path,
+				script_dirent.d_name);
+			temp = strchr(script_dirent.d_name, '.');
+			snprintf(scripts_array[i], (temp - script_dirent.d_name) + 1,
+				"%s", script_dirent.d_name);
+			i++;
+		}
+	}
+
+	return i;
+}
+
 static char *get_script_path(const char *script_root, const char *suffix)
 {
 	struct dirent *script_next, *lang_next, script_dirent, lang_dirent;
diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
index b382bd5..3ea74ed 100644
--- a/tools/perf/builtin.h
+++ b/tools/perf/builtin.h
@@ -36,4 +36,5 @@ extern int cmd_kvm(int argc, const char **argv, const char *prefix);
 extern int cmd_test(int argc, const char **argv, const char *prefix);
 extern int cmd_inject(int argc, const char **argv, const char *prefix);
 
+extern int find_scripts(char **scripts_array, char **scripts_path_array);
 #endif
-- 
1.7.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 5/7] perf ui/browser: Add a browser for perf script
  2012-09-03  8:14 [PATCH 0/7] perf ui/browser: Add browser for perf script Feng Tang
                   ` (3 preceding siblings ...)
  2012-09-03  8:14 ` [PATCH 4/7] perf scripts: Export a find_scripts() function Feng Tang
@ 2012-09-03  8:14 ` Feng Tang
  2012-09-04  1:57   ` Namhyung Kim
  2012-09-03  8:14 ` [PATCH 6/7] perf ui/browser: Integrate script browser into annotation browser Feng Tang
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 14+ messages in thread
From: Feng Tang @ 2012-09-03  8:14 UTC (permalink / raw)
  To: acme
  Cc: mingo, a.p.zijlstra, andi, namhyung, dsahern, linux-kernel, Feng Tang

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

The work flow is, users can use function key to list all the available
scripts 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.

Please be noted, most of the in-tree scripts with name xxxtop are
dynamically run and not supported by this static browser. You can run
the event_analyzing_sample.py for example.

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

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 722ddee..1fe23b9 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -569,6 +569,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
@@ -576,6 +577,7 @@ else
 		LIB_OBJS += $(OUTPUT)ui/tui/helpline.o
 		LIB_H += ui/browser.h
 		LIB_H += ui/browsers/map.h
+		LIB_H += ui/browsers/scripts.h
 		LIB_H += ui/keysyms.h
 		LIB_H += ui/libslang.h
 		LIB_H += ui/progress.h
@@ -878,6 +880,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..7183795
--- /dev/null
+++ b/tools/perf/ui/browsers/scripts.c
@@ -0,0 +1,159 @@
+#include <elf.h>
+#include <newt.h>
+#include <inttypes.h>
+#include <sys/ttydefaults.h>
+#include <string.h>
+#include <linux/bitops.h>
+#include "../../util/sort.h"
+#include "../../util/util.h"
+#include "../../util/debug.h"
+#include "../../util/symbol.h"
+#include "../browser.h"
+#include "../helpline.h"
+#include "../libslang.h"
+#include "scripts.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	40
+#define SCRIPT_MAX_NO	32
+#define SCRIPT_FULLPATH_LEN 128
+
+/* Return 0 on success */
+static int list_scripts(char *script_name)
+{
+	char *buffer, *scripts[SCRIPT_MAX_NO], *scripts_path[SCRIPT_MAX_NO];
+	int i, num, choice, ret = -1;
+
+	/* Preset the script name to SCRIPT_NAMELEN */
+	buffer = zalloc(SCRIPT_MAX_NO * (SCRIPT_NAMELEN + SCRIPT_FULLPATH_LEN));
+	if (!buffer)
+		return ret;
+
+	for (i = 0; i < SCRIPT_MAX_NO; i++) {
+		scripts[i] = buffer + i * (SCRIPT_NAMELEN + SCRIPT_FULLPATH_LEN);
+		scripts_path[i] = scripts[i] + SCRIPT_NAMELEN;
+	}
+
+	num = find_scripts(scripts, scripts_path);
+	if (num) {
+		choice = ui__popup_menu(num, scripts);
+		if (choice < num && choice >= 0) {
+			strcpy(script_name, scripts_path[choice]);
+			ret = 0;
+		}
+	}
+
+	free(buffer);
+	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[160], script_name[128];
+	char *line = NULL;
+	size_t len = 0;
+	ssize_t retlen;
+	int ret = -1, nr_entries = 0;
+	FILE *fp;
+	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. */
+	sline = malloc((sizeof(*sline)) * MAX_LINES);
+	if (!sline)
+		return -1;
+
+	memset(script_name, 0, 128);
+	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) {
+		strcpy(sline[nr_entries].line, line);
+		list_add_tail(&sline[nr_entries].node, &script.entries);
+
+		if (script.b.width < retlen)
+			script.b.width = retlen;
+		nr_entries++;
+	}
+
+	script.nr_lines = nr_entries;
+	script.b.nr_entries = nr_entries;
+	script.b.entries = &script.entries;
+
+	ret = script_browser__run(&script);
+
+	if (line)
+		free(line);
+
+exit:
+	free(sline);
+	return ret;
+}
diff --git a/tools/perf/ui/browsers/scripts.h b/tools/perf/ui/browsers/scripts.h
new file mode 100644
index 0000000..011baa8
--- /dev/null
+++ b/tools/perf/ui/browsers/scripts.h
@@ -0,0 +1,5 @@
+#ifndef _PERF_UI_BROWSER_BROWSER_H_
+#define _PERF_UI_BROWSER_BROWSER_H_ 1
+
+int script_browse(const char *script_opt);
+#endif /* _PERF_UI_BROWSER_BROWSER_H_ */
--
1.7.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 6/7] perf ui/browser: Integrate script browser into annotation browser
  2012-09-03  8:14 [PATCH 0/7] perf ui/browser: Add browser for perf script Feng Tang
                   ` (4 preceding siblings ...)
  2012-09-03  8:14 ` [PATCH 5/7] perf ui/browser: Add a browser for perf script Feng Tang
@ 2012-09-03  8:14 ` Feng Tang
  2012-09-03  8:14 ` [PATCH 7/7] perf ui/browser: Integrate script browser into main hists browser Feng Tang
  2012-09-04  0:50 ` [PATCH 0/7] perf ui/browser: Add browser for perf script Namhyung Kim
  7 siblings, 0 replies; 14+ messages in thread
From: Feng Tang @ 2012-09-03  8:14 UTC (permalink / raw)
  To: acme
  Cc: mingo, a.p.zijlstra, andi, namhyung, dsahern, linux-kernel, 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 |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 67a2703..29a273a 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -11,6 +11,8 @@
 #include <pthread.h>
 #include <newt.h>

+#include "scripts.h"
+
 struct browser_disasm_line {
 	struct rb_node	rb_node;
 	double		percent;
@@ -675,8 +677,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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 7/7] perf ui/browser: Integrate script browser into main hists browser
  2012-09-03  8:14 [PATCH 0/7] perf ui/browser: Add browser for perf script Feng Tang
                   ` (5 preceding siblings ...)
  2012-09-03  8:14 ` [PATCH 6/7] perf ui/browser: Integrate script browser into annotation browser Feng Tang
@ 2012-09-03  8:14 ` Feng Tang
  2012-09-04  0:50 ` [PATCH 0/7] perf ui/browser: Add browser for perf script Namhyung Kim
  7 siblings, 0 replies; 14+ messages in thread
From: Feng Tang @ 2012-09-03  8:14 UTC (permalink / raw)
  To: acme
  Cc: mingo, a.p.zijlstra, andi, namhyung, dsahern, linux-kernel, 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 |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 81bd8c2..1bf5725 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -18,6 +18,8 @@
 #include "../ui.h"
 #include "map.h"
 
+#include "scripts.h"
+
 struct hist_browser {
 	struct ui_browser   b;
 	struct hists	    *hists;
@@ -1089,6 +1091,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;
@@ -1099,6 +1102,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) {
@@ -1107,6 +1111,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;
 
@@ -1159,6 +1164,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 '?':
@@ -1177,6 +1184,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");
@@ -1265,6 +1273,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:
@@ -1359,6 +1386,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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/7] perf ui/browser: Add browser for perf script
  2012-09-03  8:14 [PATCH 0/7] perf ui/browser: Add browser for perf script Feng Tang
                   ` (6 preceding siblings ...)
  2012-09-03  8:14 ` [PATCH 7/7] perf ui/browser: Integrate script browser into main hists browser Feng Tang
@ 2012-09-04  0:50 ` Namhyung Kim
  7 siblings, 0 replies; 14+ messages in thread
From: Namhyung Kim @ 2012-09-04  0:50 UTC (permalink / raw)
  To: Feng Tang; +Cc: acme, mingo, a.p.zijlstra, andi, dsahern, linux-kernel

Hi, Feng

On Mon,  3 Sep 2012 16:14:26 +0800, Feng Tang wrote:
> 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.
>
> Patch 1-4 are some preparation for adding the script
> patch 5 introduce the script browser
> patch 6-7 integrate the browser to hists browser and annotation browser
>
> The patches are on top of current perf/core branch of your git tree. 
> Please help to review, thanks
>

There was a build failure on my F17 box:

      CC builtin-script.o
  builtin-script.c: In function ‘find_scripts’:
  builtin-script.c:1047:7: error: variable ‘first_half’ set but not used [-Werror=unused-but-set-variable]
  builtin-script.c:1046:22: error: variable ‘desc’ set but not used [-Werror=unused-but-set-variable]
  cc1: all warnings being treated as errors
  make: *** [builtin-script.o] Error 1

Thanks,
Namhyung


> Feng
> -----------------------
> Feng Tang (7):
>   perf symbols: Filter samples with unresolved symbol when "--symbols"
>     option is used
>   perf scripts: Add --symbols option to handle specific symbols
>   perf scripts: Add event_analyzing_sample-record/report
>   perf scripts: Export a find_scripts() function
>   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
>
>  tools/perf/Makefile                                |    5 +
>  tools/perf/builtin-script.c                        |   49 ++++++
>  tools/perf/builtin.h                               |    1 +
>  .../python/bin/event_analyzing_sample-record       |    8 +
>  .../python/bin/event_analyzing_sample-report       |    3 +
>  tools/perf/ui/browsers/annotate.c                  |    8 +
>  tools/perf/ui/browsers/hists.c                     |   39 +++++
>  tools/perf/ui/browsers/scripts.c                   |  159 ++++++++++++++++++++
>  tools/perf/ui/browsers/scripts.h                   |    5 +
>  tools/perf/util/event.c                            |    5 +-
>  10 files changed, 280 insertions(+), 2 deletions(-)
>  create mode 100644 tools/perf/scripts/python/bin/event_analyzing_sample-record
>  create mode 100644 tools/perf/scripts/python/bin/event_analyzing_sample-report
>  create mode 100644 tools/perf/ui/browsers/scripts.c
>  create mode 100644 tools/perf/ui/browsers/scripts.h

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/7] perf scripts: Export a find_scripts() function
  2012-09-03  8:14 ` [PATCH 4/7] perf scripts: Export a find_scripts() function Feng Tang
@ 2012-09-04  1:09   ` Namhyung Kim
  2012-09-04  1:34   ` Namhyung Kim
  1 sibling, 0 replies; 14+ messages in thread
From: Namhyung Kim @ 2012-09-04  1:09 UTC (permalink / raw)
  To: Feng Tang; +Cc: acme, mingo, a.p.zijlstra, andi, dsahern, linux-kernel

On Mon,  3 Sep 2012 16:14:30 +0800, Feng Tang wrote:
> So that other perf commands/browser has a way to dig out the available
> scripts info in system, this is a preparation for the script browser.
>
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
>  tools/perf/builtin-script.c |   46 +++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/builtin.h        |    1 +
>  2 files changed, 47 insertions(+), 0 deletions(-)
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 7ab86bb..d46056c 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -1031,6 +1031,52 @@ static int list_available_scripts(const struct option *opt __used,
>  	exit(0);
>  }
>  
> +/*
> + * Return 0 if none is found, otherwise the actual scripts number.
> + *
> + * Currently we just return all the raw scripts name, and their full path.
> + *
> + */
> +int find_scripts(char **scripts_array, char **scripts_path_array)
> +{
> +	struct dirent *script_next, *lang_next, script_dirent, lang_dirent;
> +	char scripts_path[MAXPATHLEN];
> +	DIR *scripts_dir, *lang_dir;
> +	char lang_path[MAXPATHLEN];
> +	struct script_desc *desc;
> +	char first_half[BUFSIZ];
> +	char *temp;
> +	int i = 0;
> +
> +	first_half[0] = 0;
> +	desc = NULL;
> +
> +	snprintf(scripts_path, MAXPATHLEN, "%s/scripts", perf_exec_path());
> +
> +	scripts_dir = opendir(scripts_path);
> +	if (!scripts_dir)
> +		return -1;
> +
> +	for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next) {

It'd be better if checking language supported - ie. by NO_LIBPERL and/or
NO_LIBPYTHON macro rather than showing all regardless.  Also you might
consider when perf was built with no script language support.

Thanks,
Namhyung


> +		snprintf(lang_path, MAXPATHLEN, "%s/%s", scripts_path,
> +			 lang_dirent.d_name);
> +		lang_dir = opendir(lang_path);
> +		if (!lang_dir)
> +			continue;
> +
> +		for_each_script(lang_path, lang_dir, script_dirent, script_next) {
> +			sprintf(scripts_path_array[i], "%s/%s", lang_path,
> +				script_dirent.d_name);
> +			temp = strchr(script_dirent.d_name, '.');
> +			snprintf(scripts_array[i], (temp - script_dirent.d_name) + 1,
> +				"%s", script_dirent.d_name);
> +			i++;
> +		}
> +	}
> +
> +	return i;
> +}
> +
>  static char *get_script_path(const char *script_root, const char *suffix)
>  {
>  	struct dirent *script_next, *lang_next, script_dirent, lang_dirent;
> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> index b382bd5..3ea74ed 100644
> --- a/tools/perf/builtin.h
> +++ b/tools/perf/builtin.h
> @@ -36,4 +36,5 @@ extern int cmd_kvm(int argc, const char **argv, const char *prefix);
>  extern int cmd_test(int argc, const char **argv, const char *prefix);
>  extern int cmd_inject(int argc, const char **argv, const char *prefix);
>  
> +extern int find_scripts(char **scripts_array, char **scripts_path_array);
>  #endif

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/7] perf scripts: Export a find_scripts() function
  2012-09-03  8:14 ` [PATCH 4/7] perf scripts: Export a find_scripts() function Feng Tang
  2012-09-04  1:09   ` Namhyung Kim
@ 2012-09-04  1:34   ` Namhyung Kim
  1 sibling, 0 replies; 14+ messages in thread
From: Namhyung Kim @ 2012-09-04  1:34 UTC (permalink / raw)
  To: Feng Tang; +Cc: acme, mingo, a.p.zijlstra, andi, dsahern, linux-kernel

On Mon,  3 Sep 2012 16:14:30 +0800, Feng Tang wrote:
> So that other perf commands/browser has a way to dig out the available
> scripts info in system, this is a preparation for the script browser.
>
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
>  tools/perf/builtin-script.c |   46 +++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/builtin.h        |    1 +
>  2 files changed, 47 insertions(+), 0 deletions(-)
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 7ab86bb..d46056c 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -1031,6 +1031,52 @@ static int list_available_scripts(const struct option *opt __used,
>  	exit(0);
>  }
>  
> +/*
> + * Return 0 if none is found, otherwise the actual scripts number.

No.  It'll return -1 if perf exec path is not set properly.

Thanks,
Namhyung


> + *
> + * Currently we just return all the raw scripts name, and their full path.
> + *
> + */
> +int find_scripts(char **scripts_array, char **scripts_path_array)
> +{
> +	struct dirent *script_next, *lang_next, script_dirent, lang_dirent;
> +	char scripts_path[MAXPATHLEN];
> +	DIR *scripts_dir, *lang_dir;
> +	char lang_path[MAXPATHLEN];
> +	struct script_desc *desc;
> +	char first_half[BUFSIZ];
> +	char *temp;
> +	int i = 0;
> +
> +	first_half[0] = 0;
> +	desc = NULL;
> +
> +	snprintf(scripts_path, MAXPATHLEN, "%s/scripts", perf_exec_path());
> +
> +	scripts_dir = opendir(scripts_path);
> +	if (!scripts_dir)
> +		return -1;
> +
> +	for_each_lang(scripts_path, scripts_dir, lang_dirent, lang_next) {
> +		snprintf(lang_path, MAXPATHLEN, "%s/%s", scripts_path,
> +			 lang_dirent.d_name);
> +		lang_dir = opendir(lang_path);
> +		if (!lang_dir)
> +			continue;
> +
> +		for_each_script(lang_path, lang_dir, script_dirent, script_next) {
> +			sprintf(scripts_path_array[i], "%s/%s", lang_path,
> +				script_dirent.d_name);
> +			temp = strchr(script_dirent.d_name, '.');
> +			snprintf(scripts_array[i], (temp - script_dirent.d_name) + 1,
> +				"%s", script_dirent.d_name);
> +			i++;
> +		}
> +	}
> +
> +	return i;
> +}
> +
>  static char *get_script_path(const char *script_root, const char *suffix)
>  {
>  	struct dirent *script_next, *lang_next, script_dirent, lang_dirent;
> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> index b382bd5..3ea74ed 100644
> --- a/tools/perf/builtin.h
> +++ b/tools/perf/builtin.h
> @@ -36,4 +36,5 @@ extern int cmd_kvm(int argc, const char **argv, const char *prefix);
>  extern int cmd_test(int argc, const char **argv, const char *prefix);
>  extern int cmd_inject(int argc, const char **argv, const char *prefix);
>  
> +extern int find_scripts(char **scripts_array, char **scripts_path_array);
>  #endif

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/7] perf ui/browser: Add a browser for perf script
  2012-09-03  8:14 ` [PATCH 5/7] perf ui/browser: Add a browser for perf script Feng Tang
@ 2012-09-04  1:57   ` Namhyung Kim
  2012-09-04  9:13     ` Feng Tang
  0 siblings, 1 reply; 14+ messages in thread
From: Namhyung Kim @ 2012-09-04  1:57 UTC (permalink / raw)
  To: Feng Tang; +Cc: acme, mingo, a.p.zijlstra, andi, dsahern, linux-kernel

On Mon,  3 Sep 2012 16:14:31 +0800, Feng Tang wrote:
> Create a script browser, so that user can check all the available
> scripts and run them inside the main perf report or annotation
> browsers, for all the perf samples or samples belong to one
> thread/symbol.
>
> The work flow is, users can use function key to list all the available
> scripts 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.

It could be used for TUI interface of perf script itself too (at least
for --list option) IMHO.


>
> Please be noted, most of the in-tree scripts with name xxxtop are
> dynamically run and not supported by this static browser. 

If so, wouldn't it be better adding a filter not to show those
unsupported scripts somehow?


> You can run the event_analyzing_sample.py for example.
>
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
>  tools/perf/Makefile              |    5 +
>  tools/perf/ui/browsers/scripts.c |  159 ++++++++++++++++++++++++++++++++++++++
>  tools/perf/ui/browsers/scripts.h |    5 +
>  3 files changed, 169 insertions(+), 0 deletions(-)
>  create mode 100644 tools/perf/ui/browsers/scripts.c
>  create mode 100644 tools/perf/ui/browsers/scripts.h
>
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index 722ddee..1fe23b9 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -569,6 +569,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
> @@ -576,6 +577,7 @@ else
>  		LIB_OBJS += $(OUTPUT)ui/tui/helpline.o
>  		LIB_H += ui/browser.h
>  		LIB_H += ui/browsers/map.h
> +		LIB_H += ui/browsers/scripts.h
>  		LIB_H += ui/keysyms.h
>  		LIB_H += ui/libslang.h
>  		LIB_H += ui/progress.h
> @@ -878,6 +880,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..7183795
> --- /dev/null
> +++ b/tools/perf/ui/browsers/scripts.c
> @@ -0,0 +1,159 @@
> +#include <elf.h>
> +#include <newt.h>
> +#include <inttypes.h>
> +#include <sys/ttydefaults.h>
> +#include <string.h>
> +#include <linux/bitops.h>
> +#include "../../util/sort.h"
> +#include "../../util/util.h"
> +#include "../../util/debug.h"
> +#include "../../util/symbol.h"
> +#include "../browser.h"
> +#include "../helpline.h"
> +#include "../libslang.h"
> +#include "scripts.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	40
> +#define SCRIPT_MAX_NO	32
> +#define SCRIPT_FULLPATH_LEN 128

I'm not sure these magic numbers are enough.


> +
> +/* Return 0 on success */
> +static int list_scripts(char *script_name)
> +{
> +	char *buffer, *scripts[SCRIPT_MAX_NO], *scripts_path[SCRIPT_MAX_NO];
> +	int i, num, choice, ret = -1;
> +
> +	/* Preset the script name to SCRIPT_NAMELEN */
> +	buffer = zalloc(SCRIPT_MAX_NO * (SCRIPT_NAMELEN + SCRIPT_FULLPATH_LEN));
> +	if (!buffer)
> +		return ret;
> +
> +	for (i = 0; i < SCRIPT_MAX_NO; i++) {
> +		scripts[i] = buffer + i * (SCRIPT_NAMELEN + SCRIPT_FULLPATH_LEN);
> +		scripts_path[i] = scripts[i] + SCRIPT_NAMELEN;
> +	}
> +
> +	num = find_scripts(scripts, scripts_path);
> +	if (num) {

It should be:

	if (num > 0) {

since the find_scripts() can return -1.


> +		choice = ui__popup_menu(num, scripts);
> +		if (choice < num && choice >= 0) {
> +			strcpy(script_name, scripts_path[choice]);
> +			ret = 0;
> +		}
> +	}
> +
> +	free(buffer);
> +	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[160], script_name[128];
> +	char *line = NULL;
> +	size_t len = 0;
> +	ssize_t retlen;
> +	int ret = -1, nr_entries = 0;
> +	FILE *fp;
> +	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. */
> +	sline = malloc((sizeof(*sline)) * MAX_LINES);
> +	if (!sline)
> +		return -1;
> +
> +	memset(script_name, 0, 128);
> +	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) {
> +		strcpy(sline[nr_entries].line, line);

What if the len is larger than sline[].line?


> +		list_add_tail(&sline[nr_entries].node, &script.entries);
> +
> +		if (script.b.width < retlen)
> +			script.b.width = retlen;
> +		nr_entries++;

I think you need this too:

		if (nr_entries >= MAX_LINES)
			break;
> +	}
> +
> +	script.nr_lines = nr_entries;
> +	script.b.nr_entries = nr_entries;
> +	script.b.entries = &script.entries;
> +
> +	ret = script_browser__run(&script);
> +
> +	if (line)
> +		free(line);

No pclose(fp) ?


> +
> +exit:
> +	free(sline);
> +	return ret;
> +}
> diff --git a/tools/perf/ui/browsers/scripts.h b/tools/perf/ui/browsers/scripts.h
> new file mode 100644
> index 0000000..011baa8
> --- /dev/null
> +++ b/tools/perf/ui/browsers/scripts.h
> @@ -0,0 +1,5 @@
> +#ifndef _PERF_UI_BROWSER_BROWSER_H_
> +#define _PERF_UI_BROWSER_BROWSER_H_ 1

I guess you want _PERF_UI_BROWSER_SCRIPT_H_ ?


> +
> +int script_browse(const char *script_opt);
> +#endif /* _PERF_UI_BROWSER_BROWSER_H_ */
> --
> 1.7.1

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/7] perf ui/browser: Add a browser for perf script
  2012-09-04  1:57   ` Namhyung Kim
@ 2012-09-04  9:13     ` Feng Tang
  2012-09-05  0:33       ` Namhyung Kim
  0 siblings, 1 reply; 14+ messages in thread
From: Feng Tang @ 2012-09-04  9:13 UTC (permalink / raw)
  To: Namhyung Kim; +Cc: acme, mingo, a.p.zijlstra, andi, dsahern, linux-kernel

Hi Namhyung,

Thanks for your kind and thorough reviews.

On Tue, 4 Sep 2012 10:57:35 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> On Mon,  3 Sep 2012 16:14:31 +0800, Feng Tang wrote:
> > Create a script browser, so that user can check all the available
> > scripts and run them inside the main perf report or annotation
> > browsers, for all the perf samples or samples belong to one
> > thread/symbol.
> >
> > The work flow is, users can use function key to list all the available
> > scripts 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.
> 
> It could be used for TUI interface of perf script itself too (at least
> for --list option) IMHO.

Do you mean make the "perf script -l" to be shown in a browser, or we
use the same popen("perf script -l") way to cache and list the all the
scripts so that we don't need the find_scripts()?

> 
> 
> >
> > Please be noted, most of the in-tree scripts with name xxxtop are
> > dynamically run and not supported by this static browser. 
> 
> If so, wouldn't it be better adding a filter not to show those
> unsupported scripts somehow?

Good idea, will simply filter them in next version.
> 
> 
> > You can run the event_analyzing_sample.py for example.
> >
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > ---

> > +
> > +/* 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	40
> > +#define SCRIPT_MAX_NO	32
> > +#define SCRIPT_FULLPATH_LEN 128
> 
> I'm not sure these magic numbers are enough.
> 
> 
> > +
> > +/* Return 0 on success */
> > +static int list_scripts(char *script_name)
> > +{
> > +	char *buffer, *scripts[SCRIPT_MAX_NO], *scripts_path[SCRIPT_MAX_NO];
> > +	int i, num, choice, ret = -1;
> > +
> > +	/* Preset the script name to SCRIPT_NAMELEN */
> > +	buffer = zalloc(SCRIPT_MAX_NO * (SCRIPT_NAMELEN + SCRIPT_FULLPATH_LEN));
> > +	if (!buffer)
> > +		return ret;
> > +
> > +	for (i = 0; i < SCRIPT_MAX_NO; i++) {
> > +		scripts[i] = buffer + i * (SCRIPT_NAMELEN + SCRIPT_FULLPATH_LEN);
> > +		scripts_path[i] = scripts[i] + SCRIPT_NAMELEN;
> > +	}
> > +
> > +	num = find_scripts(scripts, scripts_path);
> > +	if (num) {
> 
> It should be:
> 
> 	if (num > 0) {
> 
> since the find_scripts() can return -1.

Yes, as you pointed out in the other review.


> > +
> > +	fp = popen(cmd, "r");
> > +	if (!fp)
> > +		goto exit;
> > +
> > +	while ((retlen = getline(&line, &len, fp)) != -1) {
> > +		strcpy(sline[nr_entries].line, line);
> 
> What if the len is larger than sline[].line?

That's a problem, how about adding a check for "retlen" and cut that
line to fit the SCRIPT_FULLPATH_LEN? Or we just wrap the long line
by using several struct script_line to save it?

> 
> 
> > +		list_add_tail(&sline[nr_entries].node, &script.entries);
> > +
> > +		if (script.b.width < retlen)
> > +			script.b.width = retlen;
> > +		nr_entries++;
> 
> I think you need this too:
> 
> 		if (nr_entries >= MAX_LINES)
> 			break;
> > +	}
> > +
> > +	script.nr_lines = nr_entries;
> > +	script.b.nr_entries = nr_entries;
> > +	script.b.entries = &script.entries;
> > +
> > +	ret = script_browser__run(&script);
> > +
> > +	if (line)
> > +		free(line);
> 
> No pclose(fp) ?
> 
> 
> > +
> > +exit:
> > +	free(sline);
> > +	return ret;
> > +}
> > diff --git a/tools/perf/ui/browsers/scripts.h b/tools/perf/ui/browsers/scripts.h
> > new file mode 100644
> > index 0000000..011baa8
> > --- /dev/null
> > +++ b/tools/perf/ui/browsers/scripts.h
> > @@ -0,0 +1,5 @@
> > +#ifndef _PERF_UI_BROWSER_BROWSER_H_
> > +#define _PERF_UI_BROWSER_BROWSER_H_ 1
> 
> I guess you want _PERF_UI_BROWSER_SCRIPT_H_ ?

Exactly. 

I'll address the comments in this email and those in other emails. thanks!

- Feng


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/7] perf ui/browser: Add a browser for perf script
  2012-09-04  9:13     ` Feng Tang
@ 2012-09-05  0:33       ` Namhyung Kim
  0 siblings, 0 replies; 14+ messages in thread
From: Namhyung Kim @ 2012-09-05  0:33 UTC (permalink / raw)
  To: Feng Tang; +Cc: acme, mingo, a.p.zijlstra, andi, dsahern, linux-kernel

On Tue, 4 Sep 2012 17:13:27 +0800, Feng Tang wrote:
> Hi Namhyung,
>
> Thanks for your kind and thorough reviews.
>
> On Tue, 4 Sep 2012 10:57:35 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
>
>> On Mon,  3 Sep 2012 16:14:31 +0800, Feng Tang wrote:
>> > Create a script browser, so that user can check all the available
>> > scripts and run them inside the main perf report or annotation
>> > browsers, for all the perf samples or samples belong to one
>> > thread/symbol.
>> >
>> > The work flow is, users can use function key to list all the available
>> > scripts 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.
>> 
>> It could be used for TUI interface of perf script itself too (at least
>> for --list option) IMHO.
>
> Do you mean make the "perf script -l" to be shown in a browser, or we
> use the same popen("perf script -l") way to cache and list the all the
> scripts so that we don't need the find_scripts()?

I meant the former.

>
>> 
>> 
>> >
>> > Please be noted, most of the in-tree scripts with name xxxtop are
>> > dynamically run and not supported by this static browser. 
>> 
>> If so, wouldn't it be better adding a filter not to show those
>> unsupported scripts somehow?
>
> Good idea, will simply filter them in next version.
>> 
>> 
[SNIP]
>> > +
>> > +	fp = popen(cmd, "r");
>> > +	if (!fp)
>> > +		goto exit;
>> > +
>> > +	while ((retlen = getline(&line, &len, fp)) != -1) {
>> > +		strcpy(sline[nr_entries].line, line);
>> 
>> What if the len is larger than sline[].line?
>
> That's a problem, how about adding a check for "retlen" and cut that
> line to fit the SCRIPT_FULLPATH_LEN? Or we just wrap the long line
> by using several struct script_line to save it?

The first option looks better for simplicity.

Thanks,
Namhyung

>
>> 
>> 
>> > +		list_add_tail(&sline[nr_entries].node, &script.entries);
>> > +
>> > +		if (script.b.width < retlen)
>> > +			script.b.width = retlen;
>> > +		nr_entries++;
>> 
>> I think you need this too:
>> 
>> 		if (nr_entries >= MAX_LINES)
>> 			break;
>> > +	}
>> > +
>> > +	script.nr_lines = nr_entries;
>> > +	script.b.nr_entries = nr_entries;
>> > +	script.b.entries = &script.entries;
>> > +
>> > +	ret = script_browser__run(&script);
>> > +
>> > +	if (line)
>> > +		free(line);
>> 
>> No pclose(fp) ?
>> 
>> 
>> > +
>> > +exit:
>> > +	free(sline);
>> > +	return ret;
>> > +}
>> > diff --git a/tools/perf/ui/browsers/scripts.h b/tools/perf/ui/browsers/scripts.h
>> > new file mode 100644
>> > index 0000000..011baa8
>> > --- /dev/null
>> > +++ b/tools/perf/ui/browsers/scripts.h
>> > @@ -0,0 +1,5 @@
>> > +#ifndef _PERF_UI_BROWSER_BROWSER_H_
>> > +#define _PERF_UI_BROWSER_BROWSER_H_ 1
>> 
>> I guess you want _PERF_UI_BROWSER_SCRIPT_H_ ?
>
> Exactly. 
>
> I'll address the comments in this email and those in other emails. thanks!
>
> - Feng

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2012-09-05  0:41 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-03  8:14 [PATCH 0/7] perf ui/browser: Add browser for perf script Feng Tang
2012-09-03  8:14 ` [PATCH 1/7] perf symbols: Filter samples with unresolved symbol when "--symbols" option is used Feng Tang
2012-09-03  8:14 ` [PATCH 2/7] perf scripts: Add --symbols option to handle specific symbols Feng Tang
2012-09-03  8:14 ` [PATCH 3/7] perf scripts: Add event_analyzing_sample-record/report Feng Tang
2012-09-03  8:14 ` [PATCH 4/7] perf scripts: Export a find_scripts() function Feng Tang
2012-09-04  1:09   ` Namhyung Kim
2012-09-04  1:34   ` Namhyung Kim
2012-09-03  8:14 ` [PATCH 5/7] perf ui/browser: Add a browser for perf script Feng Tang
2012-09-04  1:57   ` Namhyung Kim
2012-09-04  9:13     ` Feng Tang
2012-09-05  0:33       ` Namhyung Kim
2012-09-03  8:14 ` [PATCH 6/7] perf ui/browser: Integrate script browser into annotation browser Feng Tang
2012-09-03  8:14 ` [PATCH 7/7] perf ui/browser: Integrate script browser into main hists browser Feng Tang
2012-09-04  0:50 ` [PATCH 0/7] perf ui/browser: Add browser for perf script Namhyung Kim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).