[RFC][GIT PULL] bkl ftrace events + filter regex support

[RFC][GIT PULL] bkl ftrace events + filter regex support

[Frederic Weisbecker]

Hi everyone,

This patchset provides the support for bkl tracing, with the
support of regex in ftrace filters, so that one can easily filter
the bkl event traces per subsystem/directory/file.

Thanks,
Frederic.


The following changes since commit d34a4debef933061924ee17c2ede33f5c44925fb:
  jolsa@redhat.com (1):
        tracing: Remove .globl in the scripts/recordmcount.pl doc

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git tracing/bkl

Frederic Weisbecker (5):
      tracing/bkl: Add bkl ftrace events
      tracing/event: Cleanup the useless dentry variable
      tracing/filters: Cleanup useless headers
      tracing/filters: Provide basic regex support
      tracing/filters: Provide support for char * pointers

 include/linux/smp_lock.h           |   19 +++-
 include/trace/events/bkl.h         |   61 +++++++++++
 kernel/trace/trace.h               |   33 ++++--
 kernel/trace/trace_events.c        |  106 +++++++++++++++-----
 kernel/trace/trace_events_filter.c |  192 +++++++++++++++++++++++++++++-------
 lib/kernel_lock.c                  |   11 +-
 6 files changed, 340 insertions(+), 82 deletions(-)
 create mode 100644 include/trace/events/bkl.h

[RFC][PATCH 1/5] tracing/bkl: Add bkl ftrace events

[Frederic Weisbecker]

Add two events lock_kernel and unlock_kernel() to trace the bkl uses.
This opens the door for userspace tools to perform statistics about
the callsites that use it, dependencies with other locks (by pairing
the trace with lock events), use with recursivity and so on...

The {__reacquire,release}_kernel_lock() events are not traced because
these are called from schedule, thus the sched events are sufficient
to trace them.

Example of a trace:

hald-addon-stor-4152  [000]   165.875501: unlock_kernel: depth: 0, fs/block_dev.c:1358 __blkdev_put()
hald-addon-stor-4152  [000]   167.832974: lock_kernel: depth: 0, fs/block_dev.c:1167 __blkdev_get()

How to get the callsites that acquire it recursively:

cd /debug/tracing/events/bkl
echo "lock_depth > 0" > filter

firefox-4951  [001]   206.276967: unlock_kernel: depth: 1, fs/reiserfs/super.c:575 reiserfs_dirty_inode()

You can also filter by file and/or line.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/smp_lock.h   |   19 +++++++++++---
 include/trace/events/bkl.h |   61 ++++++++++++++++++++++++++++++++++++++++++++
 lib/kernel_lock.c          |   11 ++++---
 3 files changed, 82 insertions(+), 9 deletions(-)
 create mode 100644 include/trace/events/bkl.h

diff --git a/include/linux/smp_lock.h b/include/linux/smp_lock.h
index 813be59..d48cc77 100644
--- a/include/linux/smp_lock.h
+++ b/include/linux/smp_lock.h
@@ -3,6 +3,7 @@
 
 #ifdef CONFIG_LOCK_KERNEL
 #include <linux/sched.h>
+#include <trace/events/bkl.h>
 
 #define kernel_locked()		(current->lock_depth >= 0)
 
@@ -24,8 +25,18 @@ static inline int reacquire_kernel_lock(struct task_struct *task)
 	return 0;
 }
 
-extern void __lockfunc lock_kernel(void)	__acquires(kernel_lock);
-extern void __lockfunc unlock_kernel(void)	__releases(kernel_lock);
+extern void __lockfunc _lock_kernel(void)	__acquires(kernel_lock);
+extern void __lockfunc _unlock_kernel(void)	__releases(kernel_lock);
+
+#define lock_kernel()	{					\
+	trace_lock_kernel(__func__, __FILE__, __LINE__);	\
+	_lock_kernel();						\
+}
+
+#define unlock_kernel()	{					\
+	trace_unlock_kernel(__func__, __FILE__, __LINE__);	\
+	_unlock_kernel();					\
+}
 
 /*
  * Various legacy drivers don't really need the BKL in a specific
@@ -41,8 +52,8 @@ static inline void cycle_kernel_lock(void)
 
 #else
 
-#define lock_kernel()				do { } while(0)
-#define unlock_kernel()				do { } while(0)
+#define lock_kernel()	   trace_lock_kernel(__func__, __FILE__, __LINE__);
+#define unlock_kernel()    trace_unlock_kernel(__func__, __FILE__, __LINE__);
 #define release_kernel_lock(task)		do { } while(0)
 #define cycle_kernel_lock()			do { } while(0)
 #define reacquire_kernel_lock(task)		0
diff --git a/include/trace/events/bkl.h b/include/trace/events/bkl.h
new file mode 100644
index 0000000..f4504cd
--- /dev/null
+++ b/include/trace/events/bkl.h
@@ -0,0 +1,61 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM bkl
+
+#if !defined(_TRACE_BKL_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_BKL_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(lock_kernel,
+
+	TP_PROTO(const char *func, const char *file, int line),
+
+	TP_ARGS(func, file, line),
+
+	TP_STRUCT__entry(
+		__field(int,		lock_depth)
+		__field(const char *,	func)
+		__field(const char *,	file)
+		__field(int,		line)
+	),
+
+	TP_fast_assign(
+		/* We want to record the lock_depth after lock is acquired */
+		__entry->lock_depth = current->lock_depth + 1;
+		__entry->func = func;
+		__entry->file = file;
+		__entry->line = line;
+	),
+
+	TP_printk("depth: %d, %s:%d %s()", __entry->lock_depth,
+		  __entry->file, __entry->line, __entry->func)
+);
+
+TRACE_EVENT(unlock_kernel,
+
+	TP_PROTO(const char *func, const char *file, int line),
+
+	TP_ARGS(func, file, line),
+
+	TP_STRUCT__entry(
+		__field(int,		lock_depth)
+		__field(const char *,	func)
+		__field(const char *,	file)
+		__field(int,		line)
+	),
+
+	TP_fast_assign(
+		__entry->lock_depth = current->lock_depth;
+		__entry->func = func;
+		__entry->file = file;
+		__entry->line = line;
+	),
+
+	TP_printk("depth: %d, %s:%d %s()", __entry->lock_depth,
+		  __entry->file, __entry->line, __entry->func)
+);
+
+#endif /* _TRACE_BKL_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
diff --git a/lib/kernel_lock.c b/lib/kernel_lock.c
index 39f1029..5c10b2e 100644
--- a/lib/kernel_lock.c
+++ b/lib/kernel_lock.c
@@ -5,10 +5,11 @@
  * relegated to obsolescence, but used by various less
  * important (or lazy) subsystems.
  */
-#include <linux/smp_lock.h>
 #include <linux/module.h>
 #include <linux/kallsyms.h>
 #include <linux/semaphore.h>
+#define CREATE_TRACE_POINTS
+#include <linux/smp_lock.h>
 
 /*
  * The 'big kernel lock'
@@ -113,7 +114,7 @@ static inline void __unlock_kernel(void)
  * This cannot happen asynchronously, so we only need to
  * worry about other CPU's.
  */
-void __lockfunc lock_kernel(void)
+void __lockfunc _lock_kernel(void)
 {
 	int depth = current->lock_depth+1;
 	if (likely(!depth))
@@ -121,13 +122,13 @@ void __lockfunc lock_kernel(void)
 	current->lock_depth = depth;
 }
 
-void __lockfunc unlock_kernel(void)
+void __lockfunc _unlock_kernel(void)
 {
 	BUG_ON(current->lock_depth < 0);
 	if (likely(--current->lock_depth < 0))
 		__unlock_kernel();
 }
 
-EXPORT_SYMBOL(lock_kernel);
-EXPORT_SYMBOL(unlock_kernel);
+EXPORT_SYMBOL(_lock_kernel);
+EXPORT_SYMBOL(_unlock_kernel);
 
-- 
1.6.2.3

[RFC][PATCH 2/5] tracing/event: Cleanup the useless dentry variable

[Frederic Weisbecker]

Cleanup the useless dentry variable while creating a kernel
event set of files. trace_create_file() warns if it fails to
create the file anyway, and we don't store the dentry anywhere.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_events.c |   23 +++++++++++------------
 1 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 90cf936..d8d8434 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -901,9 +901,9 @@ event_subsystem_dir(const char *name, struct dentry *d_events)
 			   "'%s/filter' entry\n", name);
 	}
 
-	entry = trace_create_file("enable", 0644, system->entry,
-				  (void *)system->name,
-				  &ftrace_system_enable_fops);
+	trace_create_file("enable", 0644, system->entry,
+			  (void *)system->name,
+			  &ftrace_system_enable_fops);
 
 	return system->entry;
 }
@@ -941,7 +941,6 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
 		 const struct file_operations *filter,
 		 const struct file_operations *format)
 {
-	struct dentry *entry;
 	int ret;
 
 	/*
@@ -968,12 +967,12 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
 	}
 
 	if (call->regfunc)
-		entry = trace_create_file("enable", 0644, call->dir, call,
-					  enable);
+		trace_create_file("enable", 0644, call->dir, call,
+				  enable);
 
 	if (call->id)
-		entry = trace_create_file("id", 0444, call->dir, call,
-					  id);
+		trace_create_file("id", 0444, call->dir, call,
+		 		  id);
 
 	if (call->define_fields) {
 		ret = call->define_fields();
@@ -982,16 +981,16 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
 				   " events/%s\n", call->name);
 			return ret;
 		}
-		entry = trace_create_file("filter", 0644, call->dir, call,
-					  filter);
+		trace_create_file("filter", 0644, call->dir, call,
+				  filter);
 	}
 
 	/* A trace may not want to export its format */
 	if (!call->show_format)
 		return 0;
 
-	entry = trace_create_file("format", 0444, call->dir, call,
-				  format);
+	trace_create_file("format", 0444, call->dir, call,
+			  format);
 
 	return 0;
 }
-- 
1.6.2.3

[RFC][PATCH 3/5] tracing/filters: Cleanup useless headers

[Frederic Weisbecker]

Cleanup remaining headers inclusion that were only useful when
the filter framework and its tracing related filesystem user interface
weren't yet separated.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Tom Zanussi <tzanussi@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_events_filter.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index b9aae72..a2f8bef 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -18,9 +18,6 @@
  * Copyright (C) 2009 Tom Zanussi <tzanussi@gmail.com>
  */
 
-#include <linux/debugfs.h>
-#include <linux/uaccess.h>
-#include <linux/module.h>
 #include <linux/ctype.h>
 #include <linux/mutex.h>
 
-- 
1.6.2.3

[RFC][PATCH 4/5] tracing/filters: Provide basic regex support

[Frederic Weisbecker]

This patch provides basic support for regular expressions in filters.
The common filter file doesn't support any regex but a new
filter_regex file is created for each subsystem/event.

It supports the following types of regexp:

- *match_beginning
- *match_middle*
- match_end*
- !don't match

Every string is now handled as a regexp in the filter framework, which
helps to factorize the code for handling both simple strings and
regexp comparisons.

(The regexp code has been savagely cherry picked from ftrace.c
writtent by Steve. If this patch makes its way, I'll be happy
to change ftrace to use the new filter helpers)

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Tom Zanussi <tzanussi@gmail.com>
---
 kernel/trace/trace.h               |   33 +++++---
 kernel/trace/trace_events.c        |   83 ++++++++++++++++---
 kernel/trace/trace_events_filter.c |  163 ++++++++++++++++++++++++++++--------
 3 files changed, 222 insertions(+), 57 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 94305c7..d96a54c 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -761,28 +761,37 @@ struct event_subsystem {
 };
 
 struct filter_pred;
+struct regex;
 
 typedef int (*filter_pred_fn_t) (struct filter_pred *pred, void *event,
 				 int val1, int val2);
+typedef int (*regex_match_func)(char *str, struct regex *r);
+
+struct regex {
+	char			pattern[MAX_FILTER_STR_VAL];
+	int			len;
+	char			*search;
+	int			search_len;
+	regex_match_func	match;
+};
 
 struct filter_pred {
-	filter_pred_fn_t fn;
-	u64 val;
-	char str_val[MAX_FILTER_STR_VAL];
-	int str_len;
-	char *field_name;
-	int offset;
-	int not;
-	int op;
-	int pop_n;
+	filter_pred_fn_t 	fn;
+	u64 			val;
+	struct regex		regex;
+	char 			*field_name;
+	int 			offset;
+	int 			not;
+	int 			op;
+	int 			pop_n;
 };
 
 extern void print_event_filter(struct ftrace_event_call *call,
 			       struct trace_seq *s);
 extern int apply_event_filter(struct ftrace_event_call *call,
-			      char *filter_string);
+			      char *filter_string, bool regex);
 extern int apply_subsystem_event_filter(struct event_subsystem *system,
-					char *filter_string);
+					char *filter_string, bool regex);
 extern void print_subsystem_event_filter(struct event_subsystem *system,
 					 struct trace_seq *s);
 
@@ -840,6 +849,8 @@ static int filter_pred_##size(struct filter_pred *pred, void *event,	\
 	return match;							\
 }
 
+void filter_parse_regex(char *buff, int len, struct regex *r, int *not);
+
 extern struct mutex event_mutex;
 extern struct list_head ftrace_events;
 
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index d8d8434..199ee49 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -644,7 +644,7 @@ event_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 
 static ssize_t
 event_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
-		   loff_t *ppos)
+		   loff_t *ppos, bool regex)
 {
 	struct ftrace_event_call *call = filp->private_data;
 	char *buf;
@@ -663,7 +663,7 @@ event_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	}
 	buf[cnt] = '\0';
 
-	err = apply_event_filter(call, buf);
+	err = apply_event_filter(call, buf, regex);
 	free_page((unsigned long) buf);
 	if (err < 0)
 		return err;
@@ -674,6 +674,20 @@ event_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
 }
 
 static ssize_t
+event_filter_write_regex(struct file *filp, const char __user *ubuf, size_t cnt,
+			 loff_t *ppos)
+{
+	return event_filter_write(filp, ubuf, cnt, ppos, true);
+}
+
+static ssize_t
+event_filter_write_common(struct file *filp, const char __user *ubuf, size_t cnt,
+			 loff_t *ppos)
+{
+	return event_filter_write(filp, ubuf, cnt, ppos, false);
+}
+
+static ssize_t
 subsystem_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 		      loff_t *ppos)
 {
@@ -700,7 +714,7 @@ subsystem_filter_read(struct file *filp, char __user *ubuf, size_t cnt,
 
 static ssize_t
 subsystem_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
-		       loff_t *ppos)
+		       loff_t *ppos, bool regex)
 {
 	struct event_subsystem *system = filp->private_data;
 	char *buf;
@@ -719,7 +733,7 @@ subsystem_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	}
 	buf[cnt] = '\0';
 
-	err = apply_subsystem_event_filter(system, buf);
+	err = apply_subsystem_event_filter(system, buf, regex);
 	free_page((unsigned long) buf);
 	if (err < 0)
 		return err;
@@ -730,6 +744,20 @@ subsystem_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
 }
 
 static ssize_t
+subsystem_filter_write_regex(struct file *filp, const char __user *ubuf, size_t cnt,
+			     loff_t *ppos)
+{
+	return subsystem_filter_write(filp, ubuf, cnt, ppos, true);
+}
+
+static ssize_t
+subsystem_filter_write_common(struct file *filp, const char __user *ubuf, size_t cnt,
+			      loff_t *ppos)
+{
+	return subsystem_filter_write(filp, ubuf, cnt, ppos, false);
+}
+
+static ssize_t
 show_header(struct file *filp, char __user *ubuf, size_t cnt, loff_t *ppos)
 {
 	int (*func)(struct trace_seq *s) = filp->private_data;
@@ -798,16 +826,28 @@ static const struct file_operations ftrace_event_id_fops = {
 	.read = event_id_read,
 };
 
-static const struct file_operations ftrace_event_filter_fops = {
+static const struct file_operations ftrace_event_filter_regex_fops = {
+	.open = tracing_open_generic,
+	.read = event_filter_read,
+	.write = event_filter_write_regex,
+};
+
+static const struct file_operations ftrace_event_filter_common_fops = {
 	.open = tracing_open_generic,
 	.read = event_filter_read,
-	.write = event_filter_write,
+	.write = event_filter_write_common,
+};
+
+static const struct file_operations ftrace_subsystem_filter_regex_fops = {
+	.open = tracing_open_generic,
+	.read = subsystem_filter_read,
+	.write = subsystem_filter_write_regex,
 };
 
-static const struct file_operations ftrace_subsystem_filter_fops = {
+static const struct file_operations ftrace_subsystem_filter_common_fops = {
 	.open = tracing_open_generic,
 	.read = subsystem_filter_read,
-	.write = subsystem_filter_write,
+	.write = subsystem_filter_write_common,
 };
 
 static const struct file_operations ftrace_system_enable_fops = {
@@ -893,7 +933,7 @@ event_subsystem_dir(const char *name, struct dentry *d_events)
 	}
 
 	entry = debugfs_create_file("filter", 0644, system->entry, system,
-				    &ftrace_subsystem_filter_fops);
+				    &ftrace_subsystem_filter_common_fops);
 	if (!entry) {
 		kfree(system->filter);
 		system->filter = NULL;
@@ -901,6 +941,15 @@ event_subsystem_dir(const char *name, struct dentry *d_events)
 			   "'%s/filter' entry\n", name);
 	}
 
+	entry = debugfs_create_file("filter_regex", 0644, system->entry, system,
+				    &ftrace_subsystem_filter_regex_fops);
+	if (!entry) {
+		kfree(system->filter);
+		system->filter = NULL;
+		pr_warning("Could not create debugfs "
+			   "'%s/filter_regex' entry\n", name);
+	}
+
 	trace_create_file("enable", 0644, system->entry,
 			  (void *)system->name,
 			  &ftrace_system_enable_fops);
@@ -939,6 +988,7 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
 		 const struct file_operations *id,
 		 const struct file_operations *enable,
 		 const struct file_operations *filter,
+		 const struct file_operations *filter_regex,
 		 const struct file_operations *format)
 {
 	int ret;
@@ -983,6 +1033,8 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
 		}
 		trace_create_file("filter", 0644, call->dir, call,
 				  filter);
+		trace_create_file("filter_regex", 0644, call->dir, call,
+				  filter_regex);
 	}
 
 	/* A trace may not want to export its format */
@@ -1015,6 +1067,7 @@ struct ftrace_module_file_ops {
 	struct file_operations		enable;
 	struct file_operations		format;
 	struct file_operations		filter;
+	struct file_operations		filter_regex;
 };
 
 static struct ftrace_module_file_ops *
@@ -1041,9 +1094,12 @@ trace_create_file_ops(struct module *mod)
 	file_ops->enable = ftrace_enable_fops;
 	file_ops->enable.owner = mod;
 
-	file_ops->filter = ftrace_event_filter_fops;
+	file_ops->filter = ftrace_event_filter_common_fops;
 	file_ops->filter.owner = mod;
 
+	file_ops->filter_regex = ftrace_event_filter_regex_fops;
+	file_ops->filter_regex.owner = mod;
+
 	file_ops->format = ftrace_event_format_fops;
 	file_ops->format.owner = mod;
 
@@ -1086,7 +1142,8 @@ static void trace_module_add_events(struct module *mod)
 		list_add(&call->list, &ftrace_events);
 		event_create_dir(call, d_events,
 				 &file_ops->id, &file_ops->enable,
-				 &file_ops->filter, &file_ops->format);
+				 &file_ops->filter, &file_ops->filter_regex,
+				 &file_ops->format);
 	}
 }
 
@@ -1226,7 +1283,9 @@ static __init int event_trace_init(void)
 			continue;
 		list_add(&call->list, &ftrace_events);
 		event_create_dir(call, d_events, &ftrace_event_id_fops,
-				 &ftrace_enable_fops, &ftrace_event_filter_fops,
+				 &ftrace_enable_fops,
+				 &ftrace_event_filter_common_fops,
+				 &ftrace_event_filter_regex_fops,
 				 &ftrace_event_format_fops);
 	}
 
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index a2f8bef..51b4e24 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -153,9 +153,9 @@ static int filter_pred_string(struct filter_pred *pred, void *event,
 	char *addr = (char *)(event + pred->offset);
 	int cmp, match;
 
-	cmp = strncmp(addr, pred->str_val, pred->str_len);
+	cmp = pred->regex.match(addr, &pred->regex);
 
-	match = (!cmp) ^ pred->not;
+	match = cmp ^ pred->not;
 
 	return match;
 }
@@ -177,9 +177,9 @@ static int filter_pred_strloc(struct filter_pred *pred, void *event,
 	char *addr = (char *)(event + str_loc);
 	int cmp, match;
 
-	cmp = strncmp(addr, pred->str_val, pred->str_len);
+	cmp = pred->regex.match(addr, &pred->regex);
 
-	match = (!cmp) ^ pred->not;
+	match = cmp ^ pred->not;
 
 	return match;
 }
@@ -190,6 +190,95 @@ static int filter_pred_none(struct filter_pred *pred, void *event,
 	return 0;
 }
 
+/* Basic regex callbacks */
+static int regex_match_full(char *str, struct regex *r)
+{
+	if (strcmp(str, r->search) == 0)
+		return 1;
+	return 0;
+}
+
+static int regex_match_front(char *str, struct regex *r)
+{
+	if (strncmp(str, r->search, r->search_len) == 0)
+		return 1;
+	return 0;
+}
+
+static int regex_match_middle(char *str, struct regex *r)
+{
+	if (strstr(str, r->search))
+		return 1;
+	return 0;
+}
+
+static int regex_match_end(char *str, struct regex *r)
+{
+	char *ptr = strstr(str, r->search);
+
+	if (ptr && (ptr[r->search_len] == 0))
+		return 1;
+	return 0;
+}
+
+/*
+ *
+ * Pass in a buffer containing a regex and this function will
+ * set search to point to the search part of the buffer and
+ * the callback to handle it properly
+ * This does modify buff.
+ *
+ */
+void filter_parse_regex(char *buff, int len, struct regex *r, int *not)
+{
+	int i;
+
+	r->match = regex_match_full;
+
+	if (buff[0] == '!') {
+		*not = 1;
+		buff++;
+		len--;
+	} else
+		*not = 0;
+
+	r->search = buff;
+
+	for (i = 0; i < len; i++) {
+		if (buff[i] != '*')
+			continue;
+
+		if (!i) {
+			r->search = buff + 1;
+			r->match = regex_match_end;
+		} else {
+			if (r->match == regex_match_end)
+				r->match = regex_match_middle;
+			else
+				r->match = regex_match_front;
+			buff[i] = 0;
+			break;
+		}
+	}
+
+	r->search_len = strlen(r->search);
+}
+
+static void filter_build_regex(struct filter_pred *pred, bool regex)
+{
+	struct regex *r = &pred->regex;
+	int not;
+
+	if (regex) {
+		filter_parse_regex(r->pattern, r->len, r, &not);
+		pred->not ^= not;
+	} else {
+		r->search = r->pattern;
+		r->search_len = r->len;
+		r->match = regex_match_full;
+	}
+}
+
 /* return 1 if event matches, 0 otherwise (discard) */
 int filter_match_preds(struct ftrace_event_call *call, void *rec)
 {
@@ -336,7 +425,7 @@ static void filter_clear_pred(struct filter_pred *pred)
 {
 	kfree(pred->field_name);
 	pred->field_name = NULL;
-	pred->str_len = 0;
+	pred->regex.len = 0;
 }
 
 static int filter_set_pred(struct filter_pred *dest,
@@ -457,7 +546,7 @@ static int filter_add_pred_fn(struct filter_parse_state *ps,
 
 enum {
 	FILTER_STATIC_STRING = 1,
-	FILTER_DYN_STRING
+	FILTER_DYN_STRING,
 };
 
 static int is_string_field(const char *type)
@@ -524,7 +613,7 @@ static filter_pred_fn_t select_comparison_fn(int op, int field_size,
 
 static int filter_add_pred(struct filter_parse_state *ps,
 			   struct ftrace_event_call *call,
-			   struct filter_pred *pred)
+			   struct filter_pred *pred, bool regex)
 {
 	struct ftrace_event_field *field;
 	filter_pred_fn_t fn;
@@ -557,26 +646,30 @@ static int filter_add_pred(struct filter_parse_state *ps,
 
 	string_type = is_string_field(field->type);
 	if (string_type) {
-		if (string_type == FILTER_STATIC_STRING)
-			fn = filter_pred_string;
-		else
+		if (string_type == FILTER_DYN_STRING)
 			fn = filter_pred_strloc;
-		pred->str_len = field->size;
+		else
+			fn = filter_pred_string;
+
+		pred->regex.len = field->size;
 		if (pred->op == OP_NE)
 			pred->not = 1;
+
+		filter_build_regex(pred, regex);
+
 		return filter_add_pred_fn(ps, call, pred, fn);
-	} else {
-		if (field->is_signed)
-			ret = strict_strtoll(pred->str_val, 0, &val);
-		else
-			ret = strict_strtoull(pred->str_val, 0, &val);
-		if (ret) {
-			parse_error(ps, FILT_ERR_ILLEGAL_INTVAL, 0);
-			return -EINVAL;
-		}
-		pred->val = val;
 	}
 
+	if (field->is_signed)
+		ret = strict_strtoll(pred->regex.pattern, 0, &val);
+	else
+		ret = strict_strtoull(pred->regex.pattern, 0, &val);
+	if (ret) {
+		parse_error(ps, FILT_ERR_ILLEGAL_INTVAL, 0);
+		return -EINVAL;
+	}
+	pred->val = val;
+
 	fn = select_comparison_fn(pred->op, field->size, field->is_signed);
 	if (!fn) {
 		parse_error(ps, FILT_ERR_INVALID_OP, 0);
@@ -592,7 +685,7 @@ static int filter_add_pred(struct filter_parse_state *ps,
 static int filter_add_subsystem_pred(struct filter_parse_state *ps,
 				     struct event_subsystem *system,
 				     struct filter_pred *pred,
-				     char *filter_string)
+				     char *filter_string, bool regex)
 {
 	struct ftrace_event_call *call;
 	int err = 0;
@@ -605,7 +698,7 @@ static int filter_add_subsystem_pred(struct filter_parse_state *ps,
 		if (strcmp(call->system, system->name))
 			continue;
 
-		err = filter_add_pred(ps, call, pred);
+		err = filter_add_pred(ps, call, pred, regex);
 		if (err) {
 			filter_free_subsystem_preds(system);
 			parse_error(ps, FILT_ERR_BAD_SUBSYS_FILTER, 0);
@@ -925,8 +1018,8 @@ static struct filter_pred *create_pred(int op, char *operand1, char *operand2)
 		return NULL;
 	}
 
-	strcpy(pred->str_val, operand2);
-	pred->str_len = strlen(operand2);
+	strcpy(pred->regex.pattern, operand2);
+	pred->regex.len = strlen(operand2);
 
 	pred->op = op;
 
@@ -973,7 +1066,7 @@ static int check_preds(struct filter_parse_state *ps)
 static int replace_preds(struct event_subsystem *system,
 			 struct ftrace_event_call *call,
 			 struct filter_parse_state *ps,
-			 char *filter_string)
+			 char *filter_string, bool regex)
 {
 	char *operand1 = NULL, *operand2 = NULL;
 	struct filter_pred *pred;
@@ -1000,10 +1093,11 @@ static int replace_preds(struct event_subsystem *system,
 		if (elt->op == OP_AND || elt->op == OP_OR) {
 			pred = create_logical_pred(elt->op);
 			if (call)
-				err = filter_add_pred(ps, call, pred);
+				err = filter_add_pred(ps, call, pred, regex);
 			else
 				err = filter_add_subsystem_pred(ps, system,
-							pred, filter_string);
+							pred, filter_string,
+							regex);
 			filter_free_pred(pred);
 			if (err)
 				return err;
@@ -1019,10 +1113,10 @@ static int replace_preds(struct event_subsystem *system,
 
 		pred = create_pred(elt->op, operand1, operand2);
 		if (call)
-			err = filter_add_pred(ps, call, pred);
+			err = filter_add_pred(ps, call, pred, regex);
 		else
 			err = filter_add_subsystem_pred(ps, system, pred,
-							filter_string);
+							filter_string, regex);
 		filter_free_pred(pred);
 		if (err)
 			return err;
@@ -1033,7 +1127,8 @@ static int replace_preds(struct event_subsystem *system,
 	return 0;
 }
 
-int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
+int apply_event_filter(struct ftrace_event_call *call, char *filter_string,
+			bool regex)
 {
 	int err;
 
@@ -1063,7 +1158,7 @@ int apply_event_filter(struct ftrace_event_call *call, char *filter_string)
 		goto out;
 	}
 
-	err = replace_preds(NULL, call, ps, filter_string);
+	err = replace_preds(NULL, call, ps, filter_string, regex);
 	if (err)
 		append_filter_err(ps, call->filter);
 
@@ -1078,7 +1173,7 @@ out_unlock:
 }
 
 int apply_subsystem_event_filter(struct event_subsystem *system,
-				 char *filter_string)
+				 char *filter_string, bool regex)
 {
 	int err;
 
@@ -1108,7 +1203,7 @@ int apply_subsystem_event_filter(struct event_subsystem *system,
 		goto out;
 	}
 
-	err = replace_preds(system, NULL, ps, filter_string);
+	err = replace_preds(system, NULL, ps, filter_string, regex);
 	if (err)
 		append_filter_err(ps, system->filter);
 
-- 
1.6.2.3

[RFC][PATCH 5/5] tracing/filters: Provide support for char * pointers

[Frederic Weisbecker]

Provide support for char * pointers in the filtering framework.
Usually, char * entries are dangerous in traces because the string
can be released whereas a pointer to it can still wait to be read from
the ring buffer. But sometimes we can assume it's safe, like in case
of RO data (eg: __file__ or __line__, used in bkl trace event). If
these RO data are in a module and so is the call to the trace event,
then it's safe, because the ring buffer will be flushed once this
module get unloaded.

Now the bkl events becomes more useful. Say that you want to trace
only the bkl use in reiserfs:

cd /debug/tracing/events/bkl/lock_kernel
echo "file == fs/reiserfs*" > filter_regex
cat /debug/tracing/trace

syslogd-3658  [001]  1874.661878: lock_kernel: depth: 1, fs/reiserfs/super.c:563 reiserfs_dirty_inode()
syslogd-3658  [001]  1874.662266: lock_kernel: depth: 0, fs/reiserfs/inode.c:2695 reiserfs_write_end()
syslogd-3658  [001]  1874.662268: lock_kernel: depth: 1, fs/reiserfs/super.c:563 reiserfs_dirty_inode()
syslogd-3658  [001]  1874.662291: lock_kernel: depth: 0, fs/reiserfs/inode.c:2695 reiserfs_write_end()

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Tom Zanussi <tzanussi@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_events_filter.c |   34 +++++++++++++++++++++++++++++-----
 1 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 51b4e24..071c93e 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -160,6 +160,20 @@ static int filter_pred_string(struct filter_pred *pred, void *event,
 	return match;
 }
 
+/* Filter predicate for char * pointers */
+static int filter_pred_pchar(struct filter_pred *pred, void *event,
+			      int val1, int val2)
+{
+	char **addr = (char **)(event + pred->offset);
+	int cmp, match;
+
+	cmp = pred->regex.match(*addr, &pred->regex);
+
+	match = (!!cmp) ^ pred->not;
+
+	return match;
+}
+
 /*
  * Filter predicate for dynamic sized arrays of characters.
  * These are implemented through a list of strings at the end
@@ -547,6 +561,7 @@ static int filter_add_pred_fn(struct filter_parse_state *ps,
 enum {
 	FILTER_STATIC_STRING = 1,
 	FILTER_DYN_STRING,
+	FILTER_PTR_STRING
 };
 
 static int is_string_field(const char *type)
@@ -557,6 +572,9 @@ static int is_string_field(const char *type)
 	if (strchr(type, '[') && strstr(type, "char"))
 		return FILTER_STATIC_STRING;
 
+	if (strstr(type, "char *"))
+		return FILTER_PTR_STRING;
+
 	return 0;
 }
 
@@ -646,12 +664,18 @@ static int filter_add_pred(struct filter_parse_state *ps,
 
 	string_type = is_string_field(field->type);
 	if (string_type) {
-		if (string_type == FILTER_DYN_STRING)
-			fn = filter_pred_strloc;
-		else
-			fn = filter_pred_string;
+		if (string_type == FILTER_PTR_STRING) {
+			fn = filter_pred_pchar;
+			pred->regex.len = strlen(pred->regex.pattern);
+		} else {
+			pred->regex.len = field->size;
+
+			if (string_type == FILTER_DYN_STRING)
+				fn = filter_pred_strloc;
+			else
+				fn = filter_pred_string;
+		}
 
-		pred->regex.len = field->size;
 		if (pred->op == OP_NE)
 			pred->not = 1;
 
-- 
1.6.2.3

Re: [RFC][PATCH 3/5] tracing/filters: Cleanup useless headers

[Li Zefan]

15:23, Frederic Weisbecker wrote:
> Cleanup remaining headers inclusion that were only useful when
> the filter framework and its tracing related filesystem user interface
> weren't yet separated.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Tom Zanussi <tzanussi@gmail.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/trace_events_filter.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index b9aae72..a2f8bef 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -18,9 +18,6 @@
>   * Copyright (C) 2009 Tom Zanussi <tzanussi@gmail.com>
>   */
>  
> -#include <linux/debugfs.h>
> -#include <linux/uaccess.h>
> -#include <linux/module.h>

But module.h is needed for the use of EXPORT_SYMBOL_GPL().

>  #include <linux/ctype.h>
>  #include <linux/mutex.h>
>  

Re: [RFC][PATCH 4/5] tracing/filters: Provide basic regex support

[Li Zefan]

Frederic Weisbecker wrote:
> This patch provides basic support for regular expressions in filters.
> The common filter file doesn't support any regex but a new
> filter_regex file is created for each subsystem/event.
> 
> It supports the following types of regexp:
> 
> - *match_beginning
> - *match_middle*
> - match_end*
> - !don't match
> 

I don't see why adding "filter_regex" is necessary, why not just make
"filter" support regex?

> Every string is now handled as a regexp in the filter framework, which
> helps to factorize the code for handling both simple strings and
> regexp comparisons.
> 
> (The regexp code has been savagely cherry picked from ftrace.c
> writtent by Steve. If this patch makes its way, I'll be happy
> to change ftrace to use the new filter helpers)

Re: [RFC][PATCH 5/5] tracing/filters: Provide support for char * pointers

[Li Zefan]

Frederic Weisbecker wrote:
> Provide support for char * pointers in the filtering framework.
> Usually, char * entries are dangerous in traces because the string
> can be released whereas a pointer to it can still wait to be read from
> the ring buffer. But sometimes we can assume it's safe, like in case
> of RO data (eg: __file__ or __line__, used in bkl trace event). If
> these RO data are in a module and so is the call to the trace event,
> then it's safe, because the ring buffer will be flushed once this
> module get unloaded.
> 

The problem is we don't distinguish dangerous char * from
safe char *... They are both defined as:
	__field(char *, str)

So for those dangerous ones, a string filter still can be applied,
which will dereference those pointers.

> Now the bkl events becomes more useful. Say that you want to trace
> only the bkl use in reiserfs:
> 
> cd /debug/tracing/events/bkl/lock_kernel
> echo "file == fs/reiserfs*" > filter_regex
> cat /debug/tracing/trace
> 
> syslogd-3658  [001]  1874.661878: lock_kernel: depth: 1, fs/reiserfs/super.c:563 reiserfs_dirty_inode()
> syslogd-3658  [001]  1874.662266: lock_kernel: depth: 0, fs/reiserfs/inode.c:2695 reiserfs_write_end()
> syslogd-3658  [001]  1874.662268: lock_kernel: depth: 1, fs/reiserfs/super.c:563 reiserfs_dirty_inode()
> syslogd-3658  [001]  1874.662291: lock_kernel: depth: 0, fs/reiserfs/inode.c:2695 reiserfs_write_end()

Re: [RFC][PATCH 3/5] tracing/filters: Cleanup useless headers

[Frederic Weisbecker]

On Mon, Aug 03, 2009 at 01:19:21PM +0800, Li Zefan wrote:
> 15:23, Frederic Weisbecker wrote:
> > Cleanup remaining headers inclusion that were only useful when
> > the filter framework and its tracing related filesystem user interface
> > weren't yet separated.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Tom Zanussi <tzanussi@gmail.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > ---
> >  kernel/trace/trace_events_filter.c |    3 ---
> >  1 files changed, 0 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> > index b9aae72..a2f8bef 100644
> > --- a/kernel/trace/trace_events_filter.c
> > +++ b/kernel/trace/trace_events_filter.c
> > @@ -18,9 +18,6 @@
> >   * Copyright (C) 2009 Tom Zanussi <tzanussi@gmail.com>
> >   */
> >  
> > -#include <linux/debugfs.h>
> > -#include <linux/uaccess.h>
> > -#include <linux/module.h>
> 
> But module.h is needed for the use of EXPORT_SYMBOL_GPL().


Oh, indeed, I'll fix that in the v2.

Thanks.


> 
> >  #include <linux/ctype.h>
> >  #include <linux/mutex.h>
> >  

Re: [RFC][PATCH 4/5] tracing/filters: Provide basic regex support

[Frederic Weisbecker]

On Mon, Aug 03, 2009 at 01:39:58PM +0800, Li Zefan wrote:
> Frederic Weisbecker wrote:
> > This patch provides basic support for regular expressions in filters.
> > The common filter file doesn't support any regex but a new
> > filter_regex file is created for each subsystem/event.
> > 
> > It supports the following types of regexp:
> > 
> > - *match_beginning
> > - *match_middle*
> > - match_end*
> > - !don't match
> > 
> 
> I don't see why adding "filter_regex" is necessary, why not just make
> "filter" support regex?


I did that because I feared about people beeing unable to filter using
* as a real character inside a filter string.
If we are using only one file, we are forced to get the '*' interpreted.
That would also break the ABI.



> > Every string is now handled as a regexp in the filter framework, which
> > helps to factorize the code for handling both simple strings and
> > regexp comparisons.
> > 
> > (The regexp code has been savagely cherry picked from ftrace.c
> > writtent by Steve. If this patch makes its way, I'll be happy
> > to change ftrace to use the new filter helpers)

Re: [RFC][PATCH 5/5] tracing/filters: Provide support for char * pointers

[Frederic Weisbecker]

On Mon, Aug 03, 2009 at 02:58:15PM +0800, Li Zefan wrote:
> Frederic Weisbecker wrote:
> > Provide support for char * pointers in the filtering framework.
> > Usually, char * entries are dangerous in traces because the string
> > can be released whereas a pointer to it can still wait to be read from
> > the ring buffer. But sometimes we can assume it's safe, like in case
> > of RO data (eg: __file__ or __line__, used in bkl trace event). If
> > these RO data are in a module and so is the call to the trace event,
> > then it's safe, because the ring buffer will be flushed once this
> > module get unloaded.
> > 
> 
> The problem is we don't distinguish dangerous char * from
> safe char *... They are both defined as:
> 	__field(char *, str)
> 
> So for those dangerous ones, a string filter still can be applied,
> which will dereference those pointers.


Yeah, but only reviewing can distinguish them. It depends on the
context.
IMO, a __builtin_constant check would be wrong. I don't remember who
posted recently tracepoints with char * types that were safe although he
didn't use string constants.



> > Now the bkl events becomes more useful. Say that you want to trace
> > only the bkl use in reiserfs:
> > 
> > cd /debug/tracing/events/bkl/lock_kernel
> > echo "file == fs/reiserfs*" > filter_regex
> > cat /debug/tracing/trace
> > 
> > syslogd-3658  [001]  1874.661878: lock_kernel: depth: 1, fs/reiserfs/super.c:563 reiserfs_dirty_inode()
> > syslogd-3658  [001]  1874.662266: lock_kernel: depth: 0, fs/reiserfs/inode.c:2695 reiserfs_write_end()
> > syslogd-3658  [001]  1874.662268: lock_kernel: depth: 1, fs/reiserfs/super.c:563 reiserfs_dirty_inode()
> > syslogd-3658  [001]  1874.662291: lock_kernel: depth: 0, fs/reiserfs/inode.c:2695 reiserfs_write_end()

Re: [RFC][PATCH 4/5] tracing/filters: Provide basic regex support

[Li Zefan]

Frederic Weisbecker wrote:
> On Mon, Aug 03, 2009 at 01:39:58PM +0800, Li Zefan wrote:
>> Frederic Weisbecker wrote:
>>> This patch provides basic support for regular expressions in filters.
>>> The common filter file doesn't support any regex but a new
>>> filter_regex file is created for each subsystem/event.
>>>
>>> It supports the following types of regexp:
>>>
>>> - *match_beginning
>>> - *match_middle*
>>> - match_end*
>>> - !don't match
>>>
>> I don't see why adding "filter_regex" is necessary, why not just make
>> "filter" support regex?
> 
> I did that because I feared about people beeing unable to filter using
> * as a real character inside a filter string.
> If we are using only one file, we are forced to get the '*' interpreted.
> That would also break the ABI.
> 

I think we don't maintain stable ABI for debugfs and we've been changing
the "ABI" in debugfs/tracing/, but of course we shouldn't change it
without good reasons.

One alternative is to use '"' to prevent '*' to be translated, but seems
using "filter_regex" is more convenient, so I agree with this patch.

Re: [RFC][PATCH 5/5] tracing/filters: Provide support for char * pointers

[Li Zefan]

Frederic Weisbecker wrote:
> On Mon, Aug 03, 2009 at 02:58:15PM +0800, Li Zefan wrote:
>> Frederic Weisbecker wrote:
>>> Provide support for char * pointers in the filtering framework.
>>> Usually, char * entries are dangerous in traces because the string
>>> can be released whereas a pointer to it can still wait to be read from
>>> the ring buffer. But sometimes we can assume it's safe, like in case
>>> of RO data (eg: __file__ or __line__, used in bkl trace event). If
>>> these RO data are in a module and so is the call to the trace event,
>>> then it's safe, because the ring buffer will be flushed once this
>>> module get unloaded.
>>>
>> The problem is we don't distinguish dangerous char * from
>> safe char *... They are both defined as:
>> 	__field(char *, str)
>>
>> So for those dangerous ones, a string filter still can be applied,
>> which will dereference those pointers.
> 
> Yeah, but only reviewing can distinguish them. It depends on the
> context.
> IMO, a __builtin_constant check would be wrong. I don't remember who
> posted recently tracepoints with char * types that were safe although he
> didn't use string constants.
> 

IMO it's really bad to rely on review to prevent wrong use of
an API..

Other developers won't know this restriction, and not all tracepoint
patches go through -tip tree, and not all trace_event source files
are in include/trace/events/.

How about add __field_type()? So we can define:

	__field_type(char *, str, FILTER_PTR_STR)

the advantage is he who wrote the code really knows this field is safe
to be used in filtering as a string.

I had some patches that does similar job. I can rewrite and post them.

Re: [RFC][PATCH 4/5] tracing/filters: Provide basic regex support

[Frederic Weisbecker]

On Thu, Aug 06, 2009 at 09:14:27AM +0800, Li Zefan wrote:
> Frederic Weisbecker wrote:
> > On Mon, Aug 03, 2009 at 01:39:58PM +0800, Li Zefan wrote:
> >> Frederic Weisbecker wrote:
> >>> This patch provides basic support for regular expressions in filters.
> >>> The common filter file doesn't support any regex but a new
> >>> filter_regex file is created for each subsystem/event.
> >>>
> >>> It supports the following types of regexp:
> >>>
> >>> - *match_beginning
> >>> - *match_middle*
> >>> - match_end*
> >>> - !don't match
> >>>
> >> I don't see why adding "filter_regex" is necessary, why not just make
> >> "filter" support regex?
> > 
> > I did that because I feared about people beeing unable to filter using
> > * as a real character inside a filter string.
> > If we are using only one file, we are forced to get the '*' interpreted.
> > That would also break the ABI.
> > 
> 
> I think we don't maintain stable ABI for debugfs and we've been changing
> the "ABI" in debugfs/tracing/, but of course we shouldn't change it
> without good reasons.


Yeah. May be I'm too much careful there but i prefer not to take the risk.

 
> One alternative is to use '"' to prevent '*' to be translated, but seems
> using "filter_regex" is more convenient, so I agree with this patch.


The problem is that most of the time, the '"' is about mandatory to delimit
a string.
Say you want to filter the lock name &REISERFS_SB(sb)->lock, you can't do that
by just typing:

echo name == &REISERFS_SB(sb)->lock > events/lockdep/filter

because the '&' character is considered as an operator and that will trigger
an error.
Instead you must type:

echo 'name == "&REISERFS_SB(sb)->lock"' > events/lockdep/filter

The '' are there to delimit the string for echo, and the "" are interpreted
by the filter api which take it as a whole string and not strings interleaved
with operators. I even fear - and > may be considered as operators if we omit
the "".

I also thought about using an antislash for those who don't want the * to
be interpreted. But although it seems intuitive, it's not as much as two
distinct and visible files.

Also, we can think about the fact the regexp support could be extended one
day if someone needs to. And we could then encounter even more ambiguous
characters such as $, ^, [, {, etc...

Re: [RFC][PATCH 5/5] tracing/filters: Provide support for char * pointers

[Frederic Weisbecker]

On Thu, Aug 06, 2009 at 09:35:39AM +0800, Li Zefan wrote:
> Frederic Weisbecker wrote:
> > On Mon, Aug 03, 2009 at 02:58:15PM +0800, Li Zefan wrote:
> >> Frederic Weisbecker wrote:
> >>> Provide support for char * pointers in the filtering framework.
> >>> Usually, char * entries are dangerous in traces because the string
> >>> can be released whereas a pointer to it can still wait to be read from
> >>> the ring buffer. But sometimes we can assume it's safe, like in case
> >>> of RO data (eg: __file__ or __line__, used in bkl trace event). If
> >>> these RO data are in a module and so is the call to the trace event,
> >>> then it's safe, because the ring buffer will be flushed once this
> >>> module get unloaded.
> >>>
> >> The problem is we don't distinguish dangerous char * from
> >> safe char *... They are both defined as:
> >> 	__field(char *, str)
> >>
> >> So for those dangerous ones, a string filter still can be applied,
> >> which will dereference those pointers.
> > 
> > Yeah, but only reviewing can distinguish them. It depends on the
> > context.
> > IMO, a __builtin_constant check would be wrong. I don't remember who
> > posted recently tracepoints with char * types that were safe although he
> > didn't use string constants.
> > 
> 
> IMO it's really bad to rely on review to prevent wrong use of
> an API..
> 
> Other developers won't know this restriction, and not all tracepoint
> patches go through -tip tree, and not all trace_event source files
> are in include/trace/events/.
> 
> How about add __field_type()? So we can define:
> 
> 	__field_type(char *, str, FILTER_PTR_STR)
> 
> the advantage is he who wrote the code really knows this field is safe
> to be used in filtering as a string.
> 
> I had some patches that does similar job. I can rewrite and post them.


Ah good idea. That may even be useful for further typedef'ed types which
filter process match existing supported types.

Just one neat however: __field_type looks too much ambiguous. __field() is
already here to define a typed field. This seems confusing.

Why not __field_ext() for "extended"? We may probably add more flags
than FILTER_PTR_STR in the future to define options for filtering or even
for larger scope.

I then wait for your patches to be posted and I'll integrate them in the
current queue.

Thanks a lot!

Re: [RFC][PATCH 5/5] tracing/filters: Provide support for char * pointers

[Li Zefan]

>> How about add __field_type()? So we can define:
>>
>> 	__field_type(char *, str, FILTER_PTR_STR)
>>
>> the advantage is he who wrote the code really knows this field is safe
>> to be used in filtering as a string.
>>
>> I had some patches that does similar job. I can rewrite and post them.
> 
> Ah good idea. That may even be useful for further typedef'ed types which
> filter process match existing supported types.
> 

That's why I wrote those patches, to allow:

	# dev is type dev_t
	echo "dev == 8:0" > filter

	# callsite is void *
	echo "callsite == kfree_skb" > filter

> Just one neat however: __field_type looks too much ambiguous. __field() is
> already here to define a typed field. This seems confusing.
> 
> Why not __field_ext() for "extended"? We may probably add more flags
> than FILTER_PTR_STR in the future to define options for filtering or even
> for larger scope.
> 

Sure. You know I'm not good at naming. ;)

> I then wait for your patches to be posted and I'll integrate them in the
> current queue.
> 

Will soon be ready.

Re: [RFC][PATCH 4/5] tracing/filters: Provide basic regex support

[Tom Zanussi]

Hi Frederic,

On Thu, 2009-08-06 at 03:49 +0200, Frederic Weisbecker wrote:
> On Thu, Aug 06, 2009 at 09:14:27AM +0800, Li Zefan wrote:
> > Frederic Weisbecker wrote:
> > > On Mon, Aug 03, 2009 at 01:39:58PM +0800, Li Zefan wrote:
> > >> Frederic Weisbecker wrote:
> > >>> This patch provides basic support for regular expressions in filters.
> > >>> The common filter file doesn't support any regex but a new
> > >>> filter_regex file is created for each subsystem/event.
> > >>>
> > >>> It supports the following types of regexp:
> > >>>
> > >>> - *match_beginning
> > >>> - *match_middle*
> > >>> - match_end*
> > >>> - !don't match
> > >>>
> > >> I don't see why adding "filter_regex" is necessary, why not just make
> > >> "filter" support regex?
> > > 
> > > I did that because I feared about people beeing unable to filter using
> > > * as a real character inside a filter string.
> > > If we are using only one file, we are forced to get the '*' interpreted.
> > > That would also break the ABI.
> > > 
> > 
> > I think we don't maintain stable ABI for debugfs and we've been changing
> > the "ABI" in debugfs/tracing/, but of course we shouldn't change it
> > without good reasons.
> 
> 
> Yeah. May be I'm too much careful there but i prefer not to take the risk.
> 
>  
> > One alternative is to use '"' to prevent '*' to be translated, but seems
> > using "filter_regex" is more convenient, so I agree with this patch.
> 
> 
> The problem is that most of the time, the '"' is about mandatory to delimit
> a string.
> Say you want to filter the lock name &REISERFS_SB(sb)->lock, you can't do that
> by just typing:
> 
> echo name == &REISERFS_SB(sb)->lock > events/lockdep/filter
> 
> because the '&' character is considered as an operator and that will trigger
> an error.
> Instead you must type:
> 
> echo 'name == "&REISERFS_SB(sb)->lock"' > events/lockdep/filter
> 
> The '' are there to delimit the string for echo, and the "" are interpreted
> by the filter api which take it as a whole string and not strings interleaved
> with operators. I even fear - and > may be considered as operators if we omit
> the "".
> 
> I also thought about using an antislash for those who don't want the * to
> be interpreted. But although it seems intuitive, it's not as much as two
> distinct and visible files.
> 
> Also, we can think about the fact the regexp support could be extended one
> day if someone needs to. And we could then encounter even more ambiguous
> characters such as $, ^, [, {, etc...
> 

This a nice new feature - I considered doing it (not complete regexp
support, just * in strings) for the original patch, but ran out of time
- glad you added it.

I still think it makes sense to have some basic support for * in the
regular filter file, so I'd vote for getting rid of the filter_regex
file for now and just adding * support with the antislash escape to the
regular filter file.  If you later wanted to add more full-fledged
regexp support and it didn't make sense to do it in the regular filter
file, then you could go crazy and add the filter_regex later...

Thanks,

Tom 

Re: [RFC][PATCH 4/5] tracing/filters: Provide basic regex support

[Frederic Weisbecker]

On Thu, Aug 06, 2009 at 11:14:57PM -0500, Tom Zanussi wrote:
> Hi Frederic,
> 
> On Thu, 2009-08-06 at 03:49 +0200, Frederic Weisbecker wrote:
> > On Thu, Aug 06, 2009 at 09:14:27AM +0800, Li Zefan wrote:
> > > Frederic Weisbecker wrote:
> > > > On Mon, Aug 03, 2009 at 01:39:58PM +0800, Li Zefan wrote:
> > > >> Frederic Weisbecker wrote:
> > > >>> This patch provides basic support for regular expressions in filters.
> > > >>> The common filter file doesn't support any regex but a new
> > > >>> filter_regex file is created for each subsystem/event.
> > > >>>
> > > >>> It supports the following types of regexp:
> > > >>>
> > > >>> - *match_beginning
> > > >>> - *match_middle*
> > > >>> - match_end*
> > > >>> - !don't match
> > > >>>
> > > >> I don't see why adding "filter_regex" is necessary, why not just make
> > > >> "filter" support regex?
> > > > 
> > > > I did that because I feared about people beeing unable to filter using
> > > > * as a real character inside a filter string.
> > > > If we are using only one file, we are forced to get the '*' interpreted.
> > > > That would also break the ABI.
> > > > 
> > > 
> > > I think we don't maintain stable ABI for debugfs and we've been changing
> > > the "ABI" in debugfs/tracing/, but of course we shouldn't change it
> > > without good reasons.
> > 
> > 
> > Yeah. May be I'm too much careful there but i prefer not to take the risk.
> > 
> >  
> > > One alternative is to use '"' to prevent '*' to be translated, but seems
> > > using "filter_regex" is more convenient, so I agree with this patch.
> > 
> > 
> > The problem is that most of the time, the '"' is about mandatory to delimit
> > a string.
> > Say you want to filter the lock name &REISERFS_SB(sb)->lock, you can't do that
> > by just typing:
> > 
> > echo name == &REISERFS_SB(sb)->lock > events/lockdep/filter
> > 
> > because the '&' character is considered as an operator and that will trigger
> > an error.
> > Instead you must type:
> > 
> > echo 'name == "&REISERFS_SB(sb)->lock"' > events/lockdep/filter
> > 
> > The '' are there to delimit the string for echo, and the "" are interpreted
> > by the filter api which take it as a whole string and not strings interleaved
> > with operators. I even fear - and > may be considered as operators if we omit
> > the "".
> > 
> > I also thought about using an antislash for those who don't want the * to
> > be interpreted. But although it seems intuitive, it's not as much as two
> > distinct and visible files.
> > 
> > Also, we can think about the fact the regexp support could be extended one
> > day if someone needs to. And we could then encounter even more ambiguous
> > characters such as $, ^, [, {, etc...
> > 
> 
> This a nice new feature - I considered doing it (not complete regexp
> support, just * in strings) for the original patch, but ran out of time
> - glad you added it.
> 
> I still think it makes sense to have some basic support for * in the
> regular filter file, so I'd vote for getting rid of the filter_regex
> file for now and just adding * support with the antislash escape to the
> regular filter file.  If you later wanted to add more full-fledged
> regexp support and it didn't make sense to do it in the regular filter
> file, then you could go crazy and add the filter_regex later...


Ok, if you and Li both prefer the single file, I'm fine with that.
It shouldn't really harm.
I'll change that in the next version.

Thanks.

Re: [RFC][PATCH 4/5] tracing/filters: Provide basic regex support

[Peter Zijlstra]

On Thu, 2009-08-06 at 23:14 -0500, Tom Zanussi wrote:

> This a nice new feature - I considered doing it (not complete regexp
> support, just * in strings) for the original patch, but ran out of time
> - glad you added it.
> 
> I still think it makes sense to have some basic support for * in the
> regular filter file, so I'd vote for getting rid of the filter_regex
> file for now and just adding * support with the antislash escape to the
> regular filter file.  If you later wanted to add more full-fledged
> regexp support and it didn't make sense to do it in the regular filter
> file, then you could go crazy and add the filter_regex later...

I'll have to second this, /debug doesn't provide an ABI and having
multiple ways of expressing filters is just a pain.

I'm thinking of adding this filter capability to the perf tracepoints by
passing along a filter expression, having multiple ways of writing this
expression is just going to cause confusion.