Skip to contentSkip to conversation

perf-probe: Add filtering features

PATCH Superseded Masami Hiramatsu 2 participants 13 messages Apr 21, 2015 – Apr 23, 2015 perf events

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Masami Hiramatsu

author
#0 lore raw

cover perf-probe: Add filtering features

Hi,

Here is a series of patches which improves perf-probe to add filtering features for --list,--del and --funcs. This also includes some bugfixes and cleanups.

E.g. --list/--funcs accepts filter rules.
----

  # ./perf probe -l vfs\*
    probe:vfs_read       (on vfs_read@ksrc/linux-3/fs/read_write.c)
  # ./perf probe -l \*libc:\*
    probe_libc:malloc    (on __libc_malloc@malloc/malloc.c in /usr/lib64/libc-2.17.so)
  ----
  ----
  # ./perf probe -F *kmalloc
  __kmalloc
  devm_kmalloc
  mempool_kmalloc
  sg_kmalloc
  sock_kmalloc
  ----

Also, --del accepts filter rules too which is more flexible than simple wildcard.
----

  # ./perf probe -d 'vfs*|malloc'
  Removed event: probe:vfs_read
  Removed event: probe_libc:malloc
  ----

Thank you,

---

Masami Hiramatsu (8):
[BUGFIX] perf probe: Make --funcs option exclusive [BUGFIX] perf probe: Remove all probes matches given pattern at once perf probe: Accept multiple filter options
perf probe: Accept filter argument for --list
perf probe: Allow to use filter on --del command
perf probe: Accept filter argument for --funcs
perf probe: Remove redundant cleanup of params.filter perf probe: Cleanup and consolidate command parsers

 tools/perf/Documentation/perf-probe.txt |   11 +-
 tools/perf/builtin-probe.c              |  162 +++++++++++++++----------------
 tools/perf/util/probe-event.c           |  113 +++++++++-------------
 tools/perf/util/probe-event.h           |    4 -
 tools/perf/util/strfilter.c             |  100 +++++++++++++++++++
 tools/perf/util/strfilter.h             |   22 ++++
 6 files changed, 258 insertions(+), 154 deletions(-)
Signature · 5 lines
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering 
Hitachi, Ltd., Research & Development Group
E-mail: [email protected]
author
#1 lore raw

PATCH 1/8 perf probe: Make --funcs option exclusive

--funcs option is a command which should be given exclusively. This adds PARSE_OPT_EXCUSIVE flag on --funcs (-F) option.

Without this, 'perf probe --funcs -l' just shows the list of probes. With this, it shows error message correctly.

This also fixes the help message and the documentation.

Signed-off-by Masami Hiramatsu <[email protected]>
---
 tools/perf/Documentation/perf-probe.txt |    2 ++
 tools/perf/builtin-probe.c              |    2 ++
 2 files changed, 4 insertions(+)
tools/perf/Documentation/perf-probe.txt +2 -0
index 239609c..a4a3cc7 100644
@@ -19,6 +19,8 @@ or
 'perf probe' [options] --line='LINE'
 or
 'perf probe' [options] --vars='PROBEPOINT'
+or
+'perf probe' [options] --funcs
 
 DESCRIPTION
 -----------
tools/perf/builtin-probe.c +2 -0
index f7b1af6..92dcce0 100644
@@ -321,6 +321,7 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 		"perf probe [<options>] --line 'LINEDESC'",
 		"perf probe [<options>] --vars 'PROBEPOINT'",
 #endif
+		"perf probe [<options>] --funcs",
 		NULL
 };
 	struct option options[] = {

@@ -402,6 +403,7 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	set_option_flag(options, 'L', "line", PARSE_OPT_EXCLUSIVE);
 	set_option_flag(options, 'V', "vars", PARSE_OPT_EXCLUSIVE);
 #endif
+	set_option_flag(options, 'F', "funcs", PARSE_OPT_EXCLUSIVE);
 
 	argc = parse_options(argc, argv, options, probe_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
author
#2 lore raw

PATCH 2/8 perf probe: Remove all probes matches given pattern at once

In reply to Masami Hiramatsu

Fix perf-probe --del option to delete all matched probes in both of kprobes and uprobes at once.

When we have 2 or more events on different binaries as below,

----

  # ./perf probe -l
  probe:vfs_read       (on vfs_read@ksrc/linux-3/fs/read_write.c)
  probe_libc:malloc    (on __libc_malloc@malloc/malloc.c in /usr/lib64/libc-2.17
  ----

Trying to remove all event with '*' just removes kprobe events at first.
----

  # ./perf probe -d \*
  Removed event: probe:vfs_read
  ----

And in 2nd try, it removes all uprobe events.
----

  # ./perf probe -d \*
  Removed event: probe_libc:malloc
  ----

This fixes to remove all event at once as below.
----

  # ./perf probe -d \*
  Removed event: probe:vfs_read
  Removed event: probe_libc:malloc
  ----
Signed-off-by Masami Hiramatsu <[email protected]>
Reported-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/probe-event.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
tools/perf/util/probe-event.c +11 -6
index d8bb616..cae1fd7 100644
@@ -2710,7 +2710,7 @@ static int del_trace_probe_event(int fd, const char *buf,
 
 int del_perf_probe_events(struct strlist *dellist)
 {
-	int ret = -1, ufd = -1, kfd = -1;
+	int ret = -1, ret2, ufd = -1, kfd = -1;
 	char buf[128];
 	const char *group, *event;
 	char *p, *str;

@@ -2731,8 +2731,10 @@ int del_perf_probe_events(struct strlist *dellist)
 		goto error;
 	}
 
-	if (namelist == NULL && unamelist == NULL)
+	if (namelist == NULL && unamelist == NULL) {
+		ret = -ENOENT;
 		goto error;
+	}
 
 	strlist__for_each(ent, dellist) {
 		str = strdup(ent->s);

@@ -2760,14 +2762,17 @@ int del_perf_probe_events(struct strlist *dellist)
 
 		pr_debug("Group: %s, Event: %s\n", group, event);
 
+		ret = ret2 = -ENOENT;
 		if (namelist)
 			ret = del_trace_probe_event(kfd, buf, namelist);
 
-		if (unamelist && ret != 0)
-			ret = del_trace_probe_event(ufd, buf, unamelist);
+		if (unamelist)
+			ret2 = del_trace_probe_event(ufd, buf, unamelist);
 
-		if (ret != 0)
-			pr_info("Info: Event \"%s\" does not exist.\n", buf);
+		/* Since we can remove probes which already removed, no error  */
+		if (ret != 0 && ret2 != 0)
+			pr_debug("Event \"%s\" does not exist.\n", buf);
+		ret = 0;
 
 		free(str);
 	}
maintainer
#3 lore raw

Re: [PATCH perf/core 2/8] [BUGFIX] perf probe: Remove all probes matches given pattern at once

Hi Masami,

Quoted 8 lines · Masami Hiramatsu
tools/perf/util/probe-event.c +1 -0
@@ -2760,14 +2762,17 @@ int del_perf_probe_events(struct strlist *dellist)
 
 		pr_debug("Group: %s, Event: %s\n", group, event);
 
+		ret = ret2 = -ENOENT;
 		if (namelist)
 			ret = del_trace_probe_event(kfd, buf, namelist);

tools/perf/util/probe-event.c int del_perf_probe_events(struct strlist *dellist) · line 2774

Quoted 10 lines
tools/perf/util/probe-event.c +5 -4
-		if (unamelist && ret != 0)
-			ret = del_trace_probe_event(ufd, buf, unamelist);
+		if (unamelist)
+			ret2 = del_trace_probe_event(ufd, buf, unamelist);
 
-		if (ret != 0)
-			pr_info("Info: Event \"%s\" does not exist.\n", buf);
+		/* Since we can remove probes which already removed, no error  */
+		if (ret != 0 && ret2 != 0)
+			pr_debug("Event \"%s\" does not exist.\n", buf);

I think it'd be better checking 'ret == -ENOENT && ret2 == -ENOENT' here since del_trace_probe_event() can return other error codes.

Thanks,
Namhyung

Quoted 5 lines
tools/perf/util/probe-event.c +1 -0
+		ret = 0;
 
 		free(str);
 	}
author
#4 lore raw

Re: [PATCH perf/core 2/8] [BUGFIX] perf probe: Remove all probes matches given pattern at once

Quoted 16 lines · Namhyung Kim

Hi Masami,

Quoted 13 lines · Masami Hiramatsu
tools/perf/util/probe-event.c +3 -2
@@ -2760,14 +2762,17 @@ int del_perf_probe_events(struct strlist *dellist)
 
 		pr_debug("Group: %s, Event: %s\n", group, event);
 
+		ret = ret2 = -ENOENT;
 		if (namelist)
 			ret = del_trace_probe_event(kfd, buf, namelist);
 
-		if (unamelist && ret != 0)
-			ret = del_trace_probe_event(ufd, buf, unamelist);
+		if (unamelist)
+			ret2 = del_trace_probe_event(ufd, buf, unamelist);
Quoted 8 lines

tools/perf/util/probe-event.c int del_perf_probe_events(struct strlist *dellist) · line 2774

Quoted 5 lines
tools/perf/util/probe-event.c +3 -2
-		if (ret != 0)
-			pr_info("Info: Event \"%s\" does not exist.\n", buf);
+		/* Since we can remove probes which already removed, no error  */
+		if (ret != 0 && ret2 != 0)
+			pr_debug("Event \"%s\" does not exist.\n", buf);

I think it'd be better checking 'ret == -ENOENT && ret2 == -ENOENT' here since del_trace_probe_event() can return other error codes.

Indeed. BTW, this code is replaced by patch 5/8, so I'll update it too.

Thanks!

Signature · 5 lines
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: [email protected]
author
#5 lore raw

PATCH 3/8 perf probe: Accept multiple filter options

In reply to Masami Hiramatsu

Accept multiple filter options. Each filters are combined by logical-or. E.g. --filter abc* --filter *def is same as --filter abc*|*def

Signed-off-by Masami Hiramatsu <[email protected]>
---
 tools/perf/builtin-probe.c  |   14 +++++++++-----
 tools/perf/util/strfilter.c |   34 ++++++++++++++++++++++++++++++++++
 tools/perf/util/strfilter.h |   13 +++++++++++++
 3 files changed, 56 insertions(+), 5 deletions(-)
tools/perf/builtin-probe.c +9 -5
index 92dcce0..be17075 100644
@@ -262,21 +262,25 @@ static int opt_set_filter(const struct option *opt __maybe_unused,
 			  const char *str, int unset __maybe_unused)
 {
 	const char *err;
+	int ret = 0;
 
 	if (str) {
 		pr_debug2("Set filter: %s\n", str);
-		if (params.filter)
-			strfilter__delete(params.filter);
-		params.filter = strfilter__new(str, &err);
 		if (!params.filter) {
+			params.filter = strfilter__new(str, &err);
+			if (!params.filter)
+				ret = err ? -EINVAL : -ENOMEM;
+		} else
+			ret = strfilter__or(params.filter, str, &err);
+
+		if (ret == -EINVAL) {
 			pr_err("Filter parse error at %td.\n", err - str + 1);
 			pr_err("Source: \"%s\"\n", str);
 			pr_err("         %*c\n", (int)(err - str + 1), '^');
-			return -EINVAL;
 		}
 	}
 
-	return 0;
+	return ret;
 }
 
 static int init_params(void)
tools/perf/util/strfilter.c +34 -0
index 79a757a..cd659e6 100644
@@ -170,6 +170,40 @@ struct strfilter *strfilter__new(const char *rules, const char **err)
 	return filter;
 }
 
+static int strfilter__add(struct strfilter *filter, bool _or, const char *rules,
+			  const char **err)
+{
+	struct strfilter_node *right, *root;
+	const char *ep = NULL;
+
+	if (!filter || !rules)
+		return -EINVAL;
+
+	right = strfilter_node__new(rules, &ep);
+	if (!right || *ep != '\0') {
+		if (err)
+			*err = ep;
+		goto error;
+	}
+	root = strfilter_node__alloc(_or ? OP_or : OP_and, filter->root, right);
+	if (!root) {
+		ep = NULL;
+		goto error;
+	}
+
+	filter->root = root;
+	return 0;
+
+error:
+	strfilter_node__delete(right);
+	return ep ? -EINVAL : -ENOMEM;
+}
+
+int strfilter__or(struct strfilter *filter, const char *rules, const char **err)
+{
+	return strfilter__add(filter, true, rules, err);
+}
+
 static bool strfilter_node__compare(struct strfilter_node *node,
 				    const char *str)
 {
tools/perf/util/strfilter.h +13 -0
index fe611f3..c81ff97 100644
@@ -29,6 +29,19 @@ struct strfilter {
 struct strfilter *strfilter__new(const char *rules, const char **err);
 
 /**
+ * strfilter__or - Add an additional rule by logical-or
+ * @filter: Original string filter
+ * @rules: Filter rule to add at left of the root of @filter
+ *         by using logical-and.
+ * @err: Pointer which points an error detected on @rules
+ *
+ * Parse @rules and join it to the @filter by using logical-or.
+ * Return 0 if success, or return the error code.
+ */
+int strfilter__or(struct strfilter *filter,
+		  const char *rules, const char **err);
+
+/**
  * strfilter__compare - compare given string and a string filter
  * @filter: String filter
  * @str: target string
author
#6 lore raw

PATCH 4/8 perf probe: Accept filter argument for --list

In reply to Masami Hiramatsu

Currently, perf-probe --list option ignores given event filter.
----

  # ./perf probe -l vfs\*
    probe:vfs_read       (on vfs_read@ksrc/linux-3/fs/read_write.c)
    probe_libc:malloc    (on __libc_malloc@malloc/malloc.c in /usr/lib64/libc-2.17.so)
  ----

This changes --list option to accept the event filter argument as below.
----

  # ./perf probe -l vfs\*
    probe:vfs_read       (on vfs_read@ksrc/linux-3/fs/read_write.c)
  # ./perf probe -l \*libc:\*
    probe_libc:malloc    (on __libc_malloc@malloc/malloc.c in /usr/lib64/libc-2.17.so)
  ----
Signed-off-by Masami Hiramatsu <[email protected]>
---
 tools/perf/Documentation/perf-probe.txt |    6 +--
 tools/perf/builtin-probe.c              |   70 ++++++++++++++++++++-----------
 tools/perf/util/probe-event.c           |   27 ++++++++++--
 tools/perf/util/probe-event.h           |    2 -
 4 files changed, 73 insertions(+), 32 deletions(-)
tools/perf/Documentation/perf-probe.txt +3 -3
index a4a3cc7..d0feb8e 100644
@@ -14,7 +14,7 @@ or
 or
 'perf probe' [options] --del='[GROUP:]EVENT' [...]
 or
-'perf probe' --list
+'perf probe' --list[=[GROUP:]EVENT]
 or
 'perf probe' [options] --line='LINE'
 or

@@ -66,8 +66,8 @@ OPTIONS
 	classes(e.g. [a-z], [!A-Z]).
 
 -l::
---list::
-	List up current probe events.
+--list[=[GROUP:]EVENT]::
+	List up current probe events. This can also accept filtering patterns of event names.
 
 -L::
 --line=::
tools/perf/builtin-probe.c +46 -24

118 lines not on this page · raw message

tools/perf/util/probe-event.c +23 -4
index cae1fd7..38bb282 100644
@@ -2129,7 +2129,23 @@ static int show_perf_probe_event(struct perf_probe_event *pev,
 	return ret;
 }
 
-static int __show_perf_probe_events(int fd, bool is_kprobe)
+static bool filter_probe_trace_event(struct probe_trace_event *tev,
+				     struct strfilter *filter)
+{
+	char tmp[128];
+
+	/* At first, check the event name itself */
+	if (strfilter__compare(filter, tev->event))
+		return true;
+
+	/* Next, check the combination of name and group */
+	if (e_snprintf(tmp, 128, "%s:%s", tev->group, tev->event) < 0)
+		return false;
+	return strfilter__compare(filter, tmp);
+}
+
+static int __show_perf_probe_events(int fd, bool is_kprobe,
+				    struct strfilter *filter)
 {
 	int ret = 0;
 	struct probe_trace_event tev;

@@ -2147,12 +2163,15 @@ static int __show_perf_probe_events(int fd, bool is_kprobe)
 	strlist__for_each(ent, rawlist) {
 		ret = parse_probe_trace_command(ent->s, &tev);
 		if (ret >= 0) {
+			if (!filter_probe_trace_event(&tev, filter))
+				goto next;
 			ret = convert_to_perf_probe_event(&tev, &pev,
 								is_kprobe);
 			if (ret >= 0)
 				ret = show_perf_probe_event(&pev,
 							    tev.point.module);
 		}
+next:
 		clear_perf_probe_event(&pev);
 		clear_probe_trace_event(&tev);
 		if (ret < 0)

@@ -2164,7 +2183,7 @@ static int __show_perf_probe_events(int fd, bool is_kprobe)
 }
 
 /* List up current perf-probe events */
-int show_perf_probe_events(void)
+int show_perf_probe_events(struct strfilter *filter)
 {
 	int kp_fd, up_fd, ret;
 

@@ -2176,7 +2195,7 @@ int show_perf_probe_events(void)
 
 	kp_fd = open_kprobe_events(false);
 	if (kp_fd >= 0) {
-		ret = __show_perf_probe_events(kp_fd, true);
+		ret = __show_perf_probe_events(kp_fd, true, filter);
 		close(kp_fd);
 		if (ret < 0)
 			goto out;

@@ -2190,7 +2209,7 @@ int show_perf_probe_events(void)
 	}
 
 	if (up_fd >= 0) {
-		ret = __show_perf_probe_events(up_fd, false);
+		ret = __show_perf_probe_events(up_fd, false, filter);
 		close(up_fd);
 	}
 out:
tools/perf/util/probe-event.h +1 -1
index d6b7834..819e825 100644
@@ -127,7 +127,7 @@ extern const char *kernel_get_module_path(const char *module);
 extern int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
 				 int max_probe_points, bool force_add);
 extern int del_perf_probe_events(struct strlist *dellist);
-extern int show_perf_probe_events(void);
+extern int show_perf_probe_events(struct strfilter *filter);
 extern int show_line_range(struct line_range *lr, const char *module,
 			   bool user);
 extern int show_available_vars(struct perf_probe_event *pevs, int npevs,
author
#7 lore raw

PATCH 5/8 perf probe: Allow to use filter on --del command

In reply to Masami Hiramatsu

This makes perf-probe --del option to accept filter rules not only simple glob pattern. This simplifies the code and improve the flexibility.

E.g. if we remove 2 different pattern events, we need 2
-d options.
----

  # ./perf probe -d vfs\* -d malloc
  Removed event: probe_libc:malloc
  Removed event: probe:vfs_read
  ----

This allows you to joint the 2 patterns with '|'.

----

  # ./perf probe -d 'vfs*|malloc'
  Removed event: probe:vfs_read
  Removed event: probe_libc:malloc
  ----
Signed-off-by Masami Hiramatsu <[email protected]>
---
 tools/perf/builtin-probe.c    |   16 +++----
 tools/perf/util/probe-event.c |   89 +++++++++++------------------------------
 tools/perf/util/probe-event.h |    2 -
 tools/perf/util/strfilter.c   |   66 ++++++++++++++++++++++++++++++
 tools/perf/util/strfilter.h   |    9 ++++
 5 files changed, 106 insertions(+), 76 deletions(-)
tools/perf/builtin-probe.c +6 -10
index feca316..1f41b4e 100644
@@ -55,12 +55,12 @@ static struct {
 	bool show_ext_vars;
 	bool show_funcs;
 	bool mod_events;
+	bool del_events;
 	bool uprobes;
 	bool quiet;
 	bool target_used;
 	int nevents;
 	struct perf_probe_event events[MAX_PROBES];
-	struct strlist *dellist;
 	struct line_range line_range;
 	char *target;
 	int max_probe_points;

@@ -195,10 +195,8 @@ static int opt_del_probe_event(const struct option *opt __maybe_unused,
 			       const char *str, int unset __maybe_unused)
 {
 	if (str) {
-		params.mod_events = true;
-		if (!params.dellist)
-			params.dellist = strlist__new(true, NULL);
-		strlist__add(params.dellist, str);
+		params.del_events = true;
+		return params_add_filter(str);
 	}
 	return 0;
 }

@@ -313,8 +311,6 @@ static void cleanup_params(void)
 
 	for (i = 0; i < params.nevents; i++)
 		clear_perf_probe_event(params.events + i);
-	if (params.dellist)
-		strlist__delete(params.dellist);
 	line_range__clear(&params.line_range);
 	free(params.target);
 	if (params.filter)

@@ -454,7 +450,7 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (params.max_probe_points == 0)
 		params.max_probe_points = MAX_PROBES;
 
-	if ((!params.nevents && !params.dellist && !params.list_events &&
+	if ((!params.nevents && !params.del_events && !params.list_events &&
 	     !params.show_lines && !params.show_funcs))
 		usage_with_options(probe_usage, options);
 

@@ -514,8 +510,8 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	}
 #endif
 
-	if (params.dellist) {
-		ret = del_perf_probe_events(params.dellist);
+	if (params.del_events) {
+		ret = del_perf_probe_events(params.filter);
 		if (ret < 0) {
 			pr_err_with_code("  Error: Failed to delete events.", ret);
 			return ret;
tools/perf/util/probe-event.c +24 -65

123 lines not on this page · raw message

tools/perf/util/probe-event.h +1 -1
index 819e825..027356f 100644
@@ -126,7 +126,7 @@ extern const char *kernel_get_module_path(const char *module);
 
 extern int add_perf_probe_events(struct perf_probe_event *pevs, int npevs,
 				 int max_probe_points, bool force_add);
-extern int del_perf_probe_events(struct strlist *dellist);
+extern int del_perf_probe_events(struct strfilter *filter);
 extern int show_perf_probe_events(struct strfilter *filter);
 extern int show_line_range(struct line_range *lr, const char *module,
 			   bool user);
tools/perf/util/strfilter.c +66 -0

69 lines not on this page · raw message

tools/perf/util/strfilter.h +9 -0
index c81ff97..d0b772c 100644
@@ -58,4 +58,13 @@ bool strfilter__compare(struct strfilter *filter, const char *str);
  */
 void strfilter__delete(struct strfilter *filter);
 
+/**
+ * strfilter__string - Reconstruct a rule string from filter
+ * @filter: String filter to reconstruct
+ *
+ * Reconstruct a rule string from @filter. This will be good for
+ * debug messages. Note that returning string must be freed afterward.
+ */
+char *strfilter__string(struct strfilter *filter);
+
 #endif
maintainer
#8 lore raw

Re: [PATCH perf/core 5/8] perf probe: Allow to use filter on --del command

tools/perf/util/probe-event.c error: · line 2731

Quoted 11 lines · Masami Hiramatsu
tools/perf/util/probe-event.c +3 -6
-int del_perf_probe_events(struct strlist *dellist)
+int del_perf_probe_events(struct strfilter *filter)
 {
-	int ret = -1, ret2, ufd = -1, kfd = -1;
-	char buf[128];
-	const char *group, *event;
-	char *p, *str;
-	struct str_node *ent;
+	int ret = 0, ufd = -1, kfd = -1;
 	struct strlist *namelist = NULL, *unamelist = NULL;
+	char *str = strfilter__string(filter);

You may want to check the return value here - or in the strfilter__string() ?

Thanks,
Namhyung

Quoted 2 lines
(quoted diff) +2 -0
+
+	pr_debug("Filter: \'%s\'\n", str);

[SNIP]

tools/perf/util/strfilter.c bool strfilter__compare(struct strfilter *filter, const char *str) · line 295

Quoted 10 lines
tools/perf/util/strfilter.c +10 -0
+char *strfilter__string(struct strfilter *filter)
+{
+	int len;
+	char *ret = NULL;
+
+	len = strfilter_node__sprint(filter->root, NULL);
+	if (len < 0)
+		return NULL;
+
+	ret = malloc(len + 1);

Here?

Quoted 4 lines
tools/perf/util/strfilter.c +4 -0
+	strfilter_node__sprint(filter->root, ret);
+
+	return ret;
+}
author
#9 lore raw

Re: Re: [PATCH perf/core 5/8] perf probe: Allow to use filter on --del command

Quoted 15 lines · Namhyung Kim

tools/perf/util/probe-event.c error: · line 2731

Quoted 11 lines · Masami Hiramatsu
tools/perf/util/probe-event.c +3 -6
-int del_perf_probe_events(struct strlist *dellist)
+int del_perf_probe_events(struct strfilter *filter)
 {
-	int ret = -1, ret2, ufd = -1, kfd = -1;
-	char buf[128];
-	const char *group, *event;
-	char *p, *str;
-	struct str_node *ent;
+	int ret = 0, ufd = -1, kfd = -1;
 	struct strlist *namelist = NULL, *unamelist = NULL;
+	char *str = strfilter__string(filter);

You may want to check the return value here - or in the strfilter__string() ?

Ah, right. Even though this str is only for debugging, it is better to verify the filter is correctly built by strfilter__string.

I'll check the return value here.

Quoted 9 lines

Thanks,
Namhyung

Quoted 2 lines
(quoted diff) +2 -0
+
+	pr_debug("Filter: \'%s\'\n", str);

[SNIP]

Quoted 12 lines

tools/perf/util/strfilter.c bool strfilter__compare(struct strfilter *filter, const char *str) · line 295

Quoted 10 lines
tools/perf/util/strfilter.c +10 -0
+char *strfilter__string(struct strfilter *filter)
+{
+	int len;
+	char *ret = NULL;
+
+	len = strfilter_node__sprint(filter->root, NULL);
+	if (len < 0)
+		return NULL;
+
+	ret = malloc(len + 1);

Here?

Oops, of course there need another check :)
this is just a simple care-less miss.

Thanks again!

Quoted 11 lines

tools/perf/util/strfilter.c bool strfilter__compare(struct strfilter *filter, const char *str) · line 299

Quoted 5 lines
tools/perf/util/strfilter.c +4 -0
+	strfilter_node__sprint(filter->root, ret);
+
+	return ret;
+}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 5 lines
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: [email protected]
author
#10 lore raw

PATCH 6/8 perf probe: Accept filter argument for --funcs

In reply to Masami Hiramatsu

This allows user to pass the filter pattern directly to --funcs option as below.
----

  # ./perf probe -F *kmalloc
  __kmalloc
  devm_kmalloc
  mempool_kmalloc
  sg_kmalloc
  sock_kmalloc
  ----

We previously need --filter option for that.

Signed-off-by Masami Hiramatsu <[email protected]>
---
 tools/perf/Documentation/perf-probe.txt |    3 ++-
 tools/perf/builtin-probe.c              |   19 ++++++++++++++-----
 2 files changed, 16 insertions(+), 6 deletions(-)
tools/perf/Documentation/perf-probe.txt +2 -1
index d0feb8e..a272f2e 100644
@@ -84,9 +84,10 @@ OPTIONS
 	variables.
 
 -F::
---funcs::
+--funcs[=FILTER]::
 	Show available functions in given module or kernel. With -x/--exec,
 	can also list functions in a user space executable / shared library.
+	This also can accept a FILTER rule argument.
 
 --filter=FILTER::
 	(Only for --vars and --funcs) Set filter. FILTER is a combination of glob
tools/perf/builtin-probe.c +14 -5
index 1f41b4e..5a0e8f1 100644
@@ -290,6 +290,17 @@ static int opt_show_vars(const struct option *opt __maybe_unused,
 	return ret;
 }
 #endif
+static int opt_show_funcs(const struct option *opt __maybe_unused,
+			  const char *str, int unset)
+{
+	if (!unset)
+		params.show_funcs = true;
+
+	if (str)
+		return params_add_filter(str);
+
+	return 0;
+}
 
 static int opt_set_filter(const struct option *opt __maybe_unused,
 			  const char *str, int unset __maybe_unused)

@@ -399,8 +410,9 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT__DRY_RUN(&probe_event_dry_run),
 	OPT_INTEGER('\0', "max-probes", &params.max_probe_points,
 		 "Set how many probe points can be found for a probe."),
-	OPT_BOOLEAN('F', "funcs", &params.show_funcs,
-		    "Show potential probe-able functions."),
+	OPT_CALLBACK_DEFAULT('F', "funcs", NULL, "[FILTER]",
+			     "Show potential probe-able functions.",
+			     opt_show_funcs, DEFAULT_FUNC_FILTER),
 	OPT_CALLBACK('\0', "filter", NULL,
 		     "[!]FILTER", "Set a filter (with --vars/funcs only)\n"
 		     "\t\t\t(default: \"" DEFAULT_VAR_FILTER "\" for --vars,\n"

@@ -472,9 +484,6 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 		return ret;
 	}
 	if (params.show_funcs) {
-		if (!params.filter)
-			params.filter = strfilter__new(DEFAULT_FUNC_FILTER,
-						       NULL);
 		ret = show_available_funcs(params.target, params.filter,
 					params.uprobes);
 		strfilter__delete(params.filter);
author
#11 lore raw

PATCH 7/8 perf probe: Remove redundant cleanup of params.filter

In reply to Masami Hiramatsu

Since params.filter will be released in cleanup_params, we don't need to clear it in each command.

Signed-off-by Masami Hiramatsu <[email protected]>
---
 tools/perf/builtin-probe.c |    6 ------
 1 file changed, 6 deletions(-)
tools/perf/builtin-probe.c +0 -6
index 5a0e8f1..08c9481 100644
@@ -477,8 +477,6 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 			usage_with_options(probe_usage, options);
 		}
 		ret = show_perf_probe_events(params.filter);
-		strfilter__delete(params.filter);
-		params.filter = NULL;
 		if (ret < 0)
 			pr_err_with_code("  Error: Failed to show event list.", ret);
 		return ret;

@@ -486,8 +484,6 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (params.show_funcs) {
 		ret = show_available_funcs(params.target, params.filter,
 					params.uprobes);
-		strfilter__delete(params.filter);
-		params.filter = NULL;
 		if (ret < 0)
 			pr_err_with_code("  Error: Failed to show functions.", ret);
 		return ret;

@@ -511,8 +507,6 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 					  params.target,
 					  params.filter,
 					  params.show_ext_vars);
-		strfilter__delete(params.filter);
-		params.filter = NULL;
 		if (ret < 0)
 			pr_err_with_code("  Error: Failed to show vars.", ret);
 		return ret;
author
#12 lore raw

PATCH 8/8 perf probe: Cleanup and consolidate command parsers

In reply to Masami Hiramatsu

To simplify the perf-probe command code, consolidate some similar functions and use command short-name for command classification, instead of separated booleans.

Signed-off-by Masami Hiramatsu <[email protected]>
---
 tools/perf/builtin-probe.c |  105 ++++++++++++++++----------------------------
 1 file changed, 37 insertions(+), 68 deletions(-)
tools/perf/builtin-probe.c +37 -68
index 08c9481..17870b8 100644
@@ -48,14 +48,10 @@
 
 /* Session management structure */
 static struct {
+	int command;	/* Command short_name */
 	bool list_events;
 	bool force_add;
-	bool show_lines;
-	bool show_vars;
 	bool show_ext_vars;
-	bool show_funcs;
-	bool mod_events;
-	bool del_events;
 	bool uprobes;
 	bool quiet;
 	bool target_used;

@@ -175,44 +171,11 @@ static int parse_probe_event_argv(int argc, const char **argv)
 
 		len += sprintf(&buf[len], "%s ", argv[i]);
 	}
-	params.mod_events = true;
 	ret = parse_probe_event(buf);
 	free(buf);
 	return ret;
 }
 
-static int opt_add_probe_event(const struct option *opt __maybe_unused,
-			      const char *str, int unset __maybe_unused)
-{
-	if (str) {
-		params.mod_events = true;
-		return parse_probe_event(str);
-	} else
-		return 0;
-}
-
-static int opt_del_probe_event(const struct option *opt __maybe_unused,
-			       const char *str, int unset __maybe_unused)
-{
-	if (str) {
-		params.del_events = true;
-		return params_add_filter(str);
-	}
-	return 0;
-}
-
-static int opt_list_probe_event(const struct option *opt __maybe_unused,
-				const char *str, int unset)
-{
-	if (!unset)
-		params.list_events = true;
-
-	if (str)
-		return params_add_filter(str);
-
-	return 0;
-}
-
 static int opt_set_target(const struct option *opt, const char *str,
 			int unset __maybe_unused)
 {

@@ -250,8 +213,10 @@ static int opt_set_target(const struct option *opt, const char *str,
 	return ret;
 }
 
+/* Command option callbacks */
+
 #ifdef HAVE_DWARF_SUPPORT
-static int opt_show_lines(const struct option *opt __maybe_unused,
+static int opt_show_lines(const struct option *opt,
 			  const char *str, int unset __maybe_unused)
 {
 	int ret = 0;

@@ -259,19 +224,19 @@ static int opt_show_lines(const struct option *opt __maybe_unused,
 	if (!str)
 		return 0;
 
-	if (params.show_lines) {
+	if (params.command == 'L') {
 		pr_warning("Warning: more than one --line options are"
 			   " detected. Only the first one is valid.\n");
 		return 0;
 	}
 
-	params.show_lines = true;
+	params.command = opt->short_name;
 	ret = parse_line_range_desc(str, &params.line_range);
 
 	return ret;
 }
 
-static int opt_show_vars(const struct option *opt __maybe_unused,
+static int opt_show_vars(const struct option *opt,
 			 const char *str, int unset __maybe_unused)
 {
 	struct perf_probe_event *pev = &params.events[params.nevents];

@@ -285,16 +250,27 @@ static int opt_show_vars(const struct option *opt __maybe_unused,
 		pr_err("  Error: '--vars' doesn't accept arguments.\n");
 		return -EINVAL;
 	}
-	params.show_vars = true;
+	params.command = opt->short_name;
 
 	return ret;
 }
 #endif
-static int opt_show_funcs(const struct option *opt __maybe_unused,
-			  const char *str, int unset)
+static int opt_add_probe_event(const struct option *opt,
+			      const char *str, int unset __maybe_unused)
+{
+	if (str) {
+		params.command = opt->short_name;
+		return parse_probe_event(str);
+	}
+
+	return 0;
+}
+
+static int opt_set_filter_with_command(const struct option *opt,
+				       const char *str, int unset)
 {
 	if (!unset)
-		params.show_funcs = true;
+		params.command = opt->short_name;
 
 	if (str)
 		return params_add_filter(str);

@@ -360,10 +336,10 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_BOOLEAN('q', "quiet", &params.quiet,
 		    "be quiet (do not show any mesages)"),
 	OPT_CALLBACK_DEFAULT('l', "list", NULL, "[GROUP:]EVENT",
-			     "list up probe events", opt_list_probe_event,
-			     DEFAULT_LIST_FILTER),
+			     "list up probe events",
+			     opt_set_filter_with_command, DEFAULT_LIST_FILTER),
 	OPT_CALLBACK('d', "del", NULL, "[GROUP:]EVENT", "delete a probe event.",
-		opt_del_probe_event),
+		     opt_set_filter_with_command),
 	OPT_CALLBACK('a', "add", NULL,
 #ifdef HAVE_DWARF_SUPPORT
 		"[EVENT=]FUNC[@SRC][+OFF|%return|:RL|;PT]|SRC:AL|SRC;PT"

@@ -412,7 +388,7 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 		 "Set how many probe points can be found for a probe."),
 	OPT_CALLBACK_DEFAULT('F', "funcs", NULL, "[FILTER]",
 			     "Show potential probe-able functions.",
-			     opt_show_funcs, DEFAULT_FUNC_FILTER),
+			     opt_set_filter_with_command, DEFAULT_FUNC_FILTER),
 	OPT_CALLBACK('\0', "filter", NULL,
 		     "[!]FILTER", "Set a filter (with --vars/funcs only)\n"
 		     "\t\t\t(default: \"" DEFAULT_VAR_FILTER "\" for --vars,\n"

@@ -462,16 +438,13 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (params.max_probe_points == 0)
 		params.max_probe_points = MAX_PROBES;
 
-	if ((!params.nevents && !params.del_events && !params.list_events &&
-	     !params.show_lines && !params.show_funcs))
-		usage_with_options(probe_usage, options);
-
 	/*
 	 * Only consider the user's kernel image path if given.
 	 */
 	symbol_conf.try_vmlinux_path = (symbol_conf.vmlinux_name == NULL);
 
-	if (params.list_events) {
+	switch (params.command) {
+	case 'l':
 		if (params.uprobes) {
 			pr_warning("  Error: Don't use --list with --exec.\n");
 			usage_with_options(probe_usage, options);

@@ -480,24 +453,20 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 		if (ret < 0)
 			pr_err_with_code("  Error: Failed to show event list.", ret);
 		return ret;
-	}
-	if (params.show_funcs) {
+	case 'F':
 		ret = show_available_funcs(params.target, params.filter,
 					params.uprobes);
 		if (ret < 0)
 			pr_err_with_code("  Error: Failed to show functions.", ret);
 		return ret;
-	}
-
 #ifdef HAVE_DWARF_SUPPORT
-	if (params.show_lines) {
+	case 'L':
 		ret = show_line_range(&params.line_range, params.target,
 				      params.uprobes);
 		if (ret < 0)
 			pr_err_with_code("  Error: Failed to show lines.", ret);
 		return ret;
-	}
-	if (params.show_vars) {
+	case 'V':
 		if (!params.filter)
 			params.filter = strfilter__new(DEFAULT_VAR_FILTER,
 						       NULL);

@@ -510,18 +479,15 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 		if (ret < 0)
 			pr_err_with_code("  Error: Failed to show vars.", ret);
 		return ret;
-	}
 #endif
-
-	if (params.del_events) {
+	case 'd':
 		ret = del_perf_probe_events(params.filter);
 		if (ret < 0) {
 			pr_err_with_code("  Error: Failed to delete events.", ret);
 			return ret;
 		}
-	}
-
-	if (params.nevents) {
+		break;
+	case 'a':
 		/* Ensure the last given target is used */
 		if (params.target && !params.target_used) {
 			pr_warning("  Error: -x/-m must follow the probe definitions.\n");

@@ -535,6 +501,9 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
 			pr_err_with_code("  Error: Failed to add events.", ret);
 			return ret;
 		}
+		break;
+	default:
+		usage_with_options(probe_usage, options);
 	}
 	return 0;
 }