2009-09-24 19:49:38

by Frederic Weisbecker

[permalink] [raw]
Subject: [GIT PULL v2] bkl tracepoints + filter regex support

Ingo,

This is new iteration of the bkl tracepoints + filter
regex support. It addresses the reviews that were posted
in the previous RFC version.

Please pull from:

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

Thanks;
Frederic.

Frederic Weisbecker (5):
tracing/bkl: Add bkl ftrace events
tracing/filters: Cleanup useless headers
tracing/event: Cleanup the useless dentry variable
tracing/filters: Provide basic regex support
tracing/filters: Unify the regex parsing helpers

include/linux/smp_lock.h | 19 ++++-
include/trace/events/bkl.h | 61 ++++++++++++++
kernel/trace/ftrace.c | 64 +--------------
kernel/trace/trace.h | 36 ++++++--
kernel/trace/trace_events.c | 23 +++---
kernel/trace/trace_events_filter.c | 155 +++++++++++++++++++++++++++++++----
lib/kernel_lock.c | 11 ++-
7 files changed, 262 insertions(+), 107 deletions(-)
create mode 100644 include/trace/events/bkl.h


2009-09-24 19:49:44

by Frederic Weisbecker

[permalink] [raw]
Subject: [PATCH 1/5 v2] tracing/bkl: Add bkl ftrace events

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.

v2: Use of FILTER_PTR_STRING attribute for files and lines fields to
make them traceable.

Signed-off-by: Frederic Weisbecker <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Li Zefan <[email protected]>
---
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..8abd620
--- /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_ext( const char *, func, FILTER_PTR_STRING )
+ __field_ext( const char *, file, FILTER_PTR_STRING )
+ __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

2009-09-24 19:49:46

by Frederic Weisbecker

[permalink] [raw]
Subject: [PATCH 2/5 v2] tracing/filters: Cleanup useless headers

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

v2: Keep module.h, needed for EXPORT_SYMBOL_GPL

Signed-off-by: Frederic Weisbecker <[email protected]>
Cc: Tom Zanussi <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Li Zefan <[email protected]>
---
kernel/trace/trace_events_filter.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 2324578..189663d 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -18,8 +18,6 @@
* Copyright (C) 2009 Tom Zanussi <[email protected]>
*/

-#include <linux/debugfs.h>
-#include <linux/uaccess.h>
#include <linux/module.h>
#include <linux/ctype.h>
#include <linux/mutex.h>
--
1.6.2.3

2009-09-24 19:49:48

by Frederic Weisbecker

[permalink] [raw]
Subject: [PATCH 3/5 v2] tracing/event: Cleanup the useless dentry variable

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.

v2: Fix a small conflict in kernel/trace/trace_events.c

Signed-off-by: Frederic Weisbecker <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Li Zefan <[email protected]>
---
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 56c260b..8c91b7c 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -898,9 +898,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;
}
@@ -912,7 +912,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;

/*
@@ -930,12 +929,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 && call->profile_enable)
- 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(call);
@@ -944,16 +943,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

2009-09-24 19:49:51

by Frederic Weisbecker

[permalink] [raw]
Subject: [PATCH 4/5 v2] tracing/filters: Provide basic regex support

This patch provides basic support for regular expressions in filters.

It supports the following types of regexp:

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

Example:
cd /debug/tracing/events/bkl/lock_kernel
echo 'file == "*reiserfs*"' > filter
echo 1 > enable

gedit-4941 [000] 457.735437: lock_kernel: depth: 0, fs/reiserfs/namei.c:334 reiserfs_lookup()
sync_supers-227 [001] 461.379985: lock_kernel: depth: 0, fs/reiserfs/super.c:69 reiserfs_sync_fs()
sync_supers-227 [000] 461.383096: lock_kernel: depth: 0, fs/reiserfs/journal.c:1069 flush_commit_list()
reiserfs/1-1369 [001] 461.479885: lock_kernel: depth: 0, fs/reiserfs/journal.c:3509 flush_async_commits()

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 parsing code has been wildly cherry picked from ftrace.c
written by Steve.)

v2: Simplify the whole and drop the filter_regex file

Signed-off-by: Frederic Weisbecker <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Tom Zanussi <[email protected]>
Cc: Li Zefan <[email protected]>
---
kernel/trace/trace.h | 27 ++++--
kernel/trace/trace_events_filter.c | 155 ++++++++++++++++++++++++++++++++----
2 files changed, 157 insertions(+), 25 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 86bcff9..8d0db60 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -702,20 +702,29 @@ 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, int len);
+
+struct regex {
+ char pattern[MAX_FILTER_STR_VAL];
+ int len;
+ int field_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,
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 189663d..d3c94c1 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -195,9 +195,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, pred->regex.field_len);

- match = (!cmp) ^ pred->not;
+ match = cmp ^ pred->not;

return match;
}
@@ -209,9 +209,9 @@ static int filter_pred_pchar(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, pred->regex.field_len);

- match = (!cmp) ^ pred->not;
+ match = cmp ^ pred->not;

return match;
}
@@ -235,9 +235,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, str_len);
+ cmp = pred->regex.match(addr, &pred->regex, str_len);

- match = (!cmp) ^ pred->not;
+ match = cmp ^ pred->not;

return match;
}
@@ -248,6 +248,126 @@ 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, int len)
+{
+ if (strncmp(str, r->pattern, len) == 0)
+ return 1;
+ return 0;
+}
+
+static int regex_match_front(char *str, struct regex *r, int len)
+{
+ if (strncmp(str, r->pattern, len) == 0)
+ return 1;
+ return 0;
+}
+
+static int regex_match_middle(char *str, struct regex *r, int len)
+{
+ if (strstr(str, r->pattern))
+ return 1;
+ return 0;
+}
+
+static int regex_match_end(char *str, struct regex *r, int len)
+{
+ char *ptr = strstr(str, r->pattern);
+
+ if (ptr && (ptr[r->len] == 0))
+ return 1;
+ return 0;
+}
+
+enum regex_type {
+ MATCH_FULL,
+ MATCH_FRONT_ONLY,
+ MATCH_MIDDLE_ONLY,
+ MATCH_END_ONLY,
+};
+
+/*
+ * Pass in a buffer containing a regex and this function will
+ * set search to point to the search part of the buffer and
+ * return the type of search it is (see enum above).
+ * This does modify buff.
+ *
+ * Returns enum type.
+ * search returns the pointer to use for comparison.
+ * not returns 1 if buff started with a '!'
+ * 0 otherwise.
+ */
+static enum regex_type
+filter_parse_regex(char *buff, int len, char **search, int *not)
+{
+ int type = MATCH_FULL;
+ int i;
+
+ if (buff[0] == '!') {
+ *not = 1;
+ buff++;
+ len--;
+ } else
+ *not = 0;
+
+ *search = buff;
+
+ for (i = 0; i < len; i++) {
+ if (buff[i] == '*') {
+ if (!i) {
+ *search = buff + 1;
+ type = MATCH_END_ONLY;
+ } else {
+ if (type == MATCH_END_ONLY)
+ type = MATCH_MIDDLE_ONLY;
+ else
+ type = MATCH_FRONT_ONLY;
+ buff[i] = 0;
+ break;
+ }
+ }
+ }
+
+ return type;
+}
+
+static int filter_build_regex(struct filter_pred *pred)
+{
+ struct regex *r = &pred->regex;
+ char *search, *dup;
+ enum regex_type type;
+ int not;
+
+ type = filter_parse_regex(r->pattern, r->len, &search, &not);
+ dup = kstrdup(search, GFP_KERNEL);
+ if (!dup)
+ return -ENOMEM;
+
+ strcpy(r->pattern, dup);
+ kfree(dup);
+
+ r->len = strlen(r->pattern);
+
+ switch (type) {
+ case MATCH_FULL:
+ r->match = regex_match_full;
+ break;
+ case MATCH_FRONT_ONLY:
+ r->match = regex_match_front;
+ break;
+ case MATCH_MIDDLE_ONLY:
+ r->match = regex_match_middle;
+ break;
+ case MATCH_END_ONLY:
+ r->match = regex_match_end;
+ break;
+ }
+
+ pred->not ^= not;
+
+ return 0;
+}
+
/* return 1 if event matches, 0 otherwise (discard) */
int filter_match_preds(struct ftrace_event_call *call, void *rec)
{
@@ -394,7 +514,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,
@@ -658,21 +778,24 @@ static int filter_add_pred(struct filter_parse_state *ps,
}

if (is_string_field(field)) {
- pred->str_len = field->size;
+ ret = filter_build_regex(pred);
+ if (ret)
+ return ret;

- if (field->filter_type == FILTER_STATIC_STRING)
+ if (field->filter_type == FILTER_STATIC_STRING) {
fn = filter_pred_string;
- else if (field->filter_type == FILTER_DYN_STRING)
- fn = filter_pred_strloc;
+ pred->regex.field_len = field->size;
+ } else if (field->filter_type == FILTER_DYN_STRING)
+ fn = filter_pred_strloc;
else {
fn = filter_pred_pchar;
- pred->str_len = strlen(pred->str_val);
+ pred->regex.field_len = strlen(pred->regex.pattern);
}
} else {
if (field->is_signed)
- ret = strict_strtoll(pred->str_val, 0, &val);
+ ret = strict_strtoll(pred->regex.pattern, 0, &val);
else
- ret = strict_strtoull(pred->str_val, 0, &val);
+ ret = strict_strtoull(pred->regex.pattern, 0, &val);
if (ret) {
parse_error(ps, FILT_ERR_ILLEGAL_INTVAL, 0);
return -EINVAL;
@@ -1042,8 +1165,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(pred->regex.pattern);

pred->op = op;

--
1.6.2.3

2009-09-24 19:49:55

by Frederic Weisbecker

[permalink] [raw]
Subject: [PATCH 5/5] tracing/filters: Unify the regex parsing helpers

The filter code has stolen the regex parsing function from ftrace to
get the regex support.
We have duplicated this code, so factorize it in the filter area and
make it generally available, as the filter code is the most suited to
host this feature.

Signed-off-by: Frederic Weisbecker <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Tom Zanussi <[email protected]>
Cc: Li Zefan <[email protected]>
---
kernel/trace/ftrace.c | 64 +++---------------------------------
kernel/trace/trace.h | 9 +++++
kernel/trace/trace_events_filter.c | 20 +++++------
3 files changed, 23 insertions(+), 70 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index cc615f8..ddf23a2 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1655,60 +1655,6 @@ ftrace_regex_lseek(struct file *file, loff_t offset, int origin)
return ret;
}

-enum {
- MATCH_FULL,
- MATCH_FRONT_ONLY,
- MATCH_MIDDLE_ONLY,
- MATCH_END_ONLY,
-};
-
-/*
- * (static function - no need for kernel doc)
- *
- * Pass in a buffer containing a glob and this function will
- * set search to point to the search part of the buffer and
- * return the type of search it is (see enum above).
- * This does modify buff.
- *
- * Returns enum type.
- * search returns the pointer to use for comparison.
- * not returns 1 if buff started with a '!'
- * 0 otherwise.
- */
-static int
-ftrace_setup_glob(char *buff, int len, char **search, int *not)
-{
- int type = MATCH_FULL;
- int i;
-
- if (buff[0] == '!') {
- *not = 1;
- buff++;
- len--;
- } else
- *not = 0;
-
- *search = buff;
-
- for (i = 0; i < len; i++) {
- if (buff[i] == '*') {
- if (!i) {
- *search = buff + 1;
- type = MATCH_END_ONLY;
- } else {
- if (type == MATCH_END_ONLY)
- type = MATCH_MIDDLE_ONLY;
- else
- type = MATCH_FRONT_ONLY;
- buff[i] = 0;
- break;
- }
- }
- }
-
- return type;
-}
-
static int ftrace_match(char *str, char *regex, int len, int type)
{
int matched = 0;
@@ -1757,7 +1703,7 @@ static void ftrace_match_records(char *buff, int len, int enable)
int not;

flag = enable ? FTRACE_FL_FILTER : FTRACE_FL_NOTRACE;
- type = ftrace_setup_glob(buff, len, &search, &not);
+ type = filter_parse_regex(buff, len, &search, &not);

search_len = strlen(search);

@@ -1825,7 +1771,7 @@ static void ftrace_match_module_records(char *buff, char *mod, int enable)
}

if (strlen(buff)) {
- type = ftrace_setup_glob(buff, strlen(buff), &search, &not);
+ type = filter_parse_regex(buff, strlen(buff), &search, &not);
search_len = strlen(search);
}

@@ -1990,7 +1936,7 @@ register_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
int count = 0;
char *search;

- type = ftrace_setup_glob(glob, strlen(glob), &search, &not);
+ type = filter_parse_regex(glob, strlen(glob), &search, &not);
len = strlen(search);

/* we do not support '!' for function probes */
@@ -2067,7 +2013,7 @@ __unregister_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
else if (glob) {
int not;

- type = ftrace_setup_glob(glob, strlen(glob), &search, &not);
+ type = filter_parse_regex(glob, strlen(glob), &search, &not);
len = strlen(search);

/* we do not support '!' for function probes */
@@ -2520,7 +2466,7 @@ ftrace_set_func(unsigned long *array, int *idx, char *buffer)
return -ENODEV;

/* decode regex */
- type = ftrace_setup_glob(buffer, strlen(buffer), &search, &not);
+ type = filter_parse_regex(buffer, strlen(buffer), &search, &not);
if (not)
return -EINVAL;

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 8d0db60..db6b83e 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -709,6 +709,13 @@ typedef int (*filter_pred_fn_t) (struct filter_pred *pred, void *event,

typedef int (*regex_match_func)(char *str, struct regex *r, int len);

+enum regex_type {
+ MATCH_FULL,
+ MATCH_FRONT_ONLY,
+ MATCH_MIDDLE_ONLY,
+ MATCH_END_ONLY,
+};
+
struct regex {
char pattern[MAX_FILTER_STR_VAL];
int len;
@@ -727,6 +734,8 @@ struct filter_pred {
int pop_n;
};

+extern enum regex_type
+filter_parse_regex(char *buff, int len, char **search, int *not);
extern void print_event_filter(struct ftrace_event_call *call,
struct trace_seq *s);
extern int apply_event_filter(struct ftrace_event_call *call,
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index d3c94c1..8c194de 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -279,15 +279,14 @@ static int regex_match_end(char *str, struct regex *r, int len)
return 0;
}

-enum regex_type {
- MATCH_FULL,
- MATCH_FRONT_ONLY,
- MATCH_MIDDLE_ONLY,
- MATCH_END_ONLY,
-};
-
-/*
- * Pass in a buffer containing a regex and this function will
+/**
+ * filter_parse_regex - parse a basic regex
+ * @buff: the raw regex
+ * @len: length of the regex
+ * @search: will point to the beginning of the string to compare
+ * @not: tell whether the match will have to be inverted
+ *
+ * This passes in a buffer containing a regex and this function will
* set search to point to the search part of the buffer and
* return the type of search it is (see enum above).
* This does modify buff.
@@ -297,8 +296,7 @@ enum regex_type {
* not returns 1 if buff started with a '!'
* 0 otherwise.
*/
-static enum regex_type
-filter_parse_regex(char *buff, int len, char **search, int *not)
+enum regex_type filter_parse_regex(char *buff, int len, char **search, int *not)
{
int type = MATCH_FULL;
int i;
--
1.6.2.3

2009-09-24 20:15:20

by Ingo Molnar

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support


* Frederic Weisbecker <[email protected]> wrote:

> Ingo,
>
> This is new iteration of the bkl tracepoints + filter
> regex support. It addresses the reviews that were posted
> in the previous RFC version.
>
> Please pull from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> tracing/core
>
> Thanks;
> Frederic.
>
> Frederic Weisbecker (5):
> tracing/bkl: Add bkl ftrace events
> tracing/filters: Cleanup useless headers
> tracing/event: Cleanup the useless dentry variable
> tracing/filters: Provide basic regex support
> tracing/filters: Unify the regex parsing helpers
>
> include/linux/smp_lock.h | 19 ++++-
> include/trace/events/bkl.h | 61 ++++++++++++++
> kernel/trace/ftrace.c | 64 +--------------
> kernel/trace/trace.h | 36 ++++++--
> kernel/trace/trace_events.c | 23 +++---
> kernel/trace/trace_events_filter.c | 155 +++++++++++++++++++++++++++++++----
> lib/kernel_lock.c | 11 ++-
> 7 files changed, 262 insertions(+), 107 deletions(-)
> create mode 100644 include/trace/events/bkl.h

Pulled, thanks a lot Frederic! These bits look very useful.

It would be perf-ect now to complete the filters-via-perf-events changes
Li Zefan is working on ;-)

Ingo

2009-09-24 20:16:27

by Ingo Molnar

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support


* Ingo Molnar <[email protected]> wrote:

>
> * Frederic Weisbecker <[email protected]> wrote:
>
> > Ingo,
> >
> > This is new iteration of the bkl tracepoints + filter
> > regex support. It addresses the reviews that were posted
> > in the previous RFC version.
> >
> > Please pull from:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> > tracing/core
> >
> > Thanks;
> > Frederic.
> >
> > Frederic Weisbecker (5):
> > tracing/bkl: Add bkl ftrace events
> > tracing/filters: Cleanup useless headers
> > tracing/event: Cleanup the useless dentry variable
> > tracing/filters: Provide basic regex support
> > tracing/filters: Unify the regex parsing helpers
> >
> > include/linux/smp_lock.h | 19 ++++-
> > include/trace/events/bkl.h | 61 ++++++++++++++
> > kernel/trace/ftrace.c | 64 +--------------
> > kernel/trace/trace.h | 36 ++++++--
> > kernel/trace/trace_events.c | 23 +++---
> > kernel/trace/trace_events_filter.c | 155 +++++++++++++++++++++++++++++++----
> > lib/kernel_lock.c | 11 ++-
> > 7 files changed, 262 insertions(+), 107 deletions(-)
> > create mode 100644 include/trace/events/bkl.h
>
> Pulled, thanks a lot Frederic! These bits look very useful.
>
> It would be perf-ect now to complete the filters-via-perf-events changes
> Li Zefan is working on ;-)

There's one thing Peter noticed: this is not C syntax anymore. It would
be really nice to keep filter expressions a subset of C.

Ingo

2009-09-24 20:22:43

by Ingo Molnar

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support


also, some build problems:

include/trace/events/bkl.h: In function
?ftrace_profile_enable_lock_kernel?:
include/trace/events/bkl.h:9: error: implicit declaration of function
?register_trace_lock_kernel?
include/trace/events/bkl.h: In function
?ftrace_profile_disable_lock_kernel?:
include/trace/events/bkl.h:9: error: implicit declaration of function
?unregister_trace_lock_kernel?
include/trace/events/bkl.h: In function
?ftrace_profile_enable_unlock_kernel?:
include/trace/events/bkl.h:34: error: implicit declaration of function
?register_trace_unlock_kernel?
include/trace/events/bkl.h: In function
?ftrace_profile_disable_unlock_kernel?:
include/trace/events/bkl.h:34: error: implicit declaration of function
?unregister_trace_unlock_kernel?

config attached.

Ingo


Attachments:
(No filename) (782.00 B)
config (68.07 kB)
Download all attachments

2009-09-24 20:30:11

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Thu, 2009-09-24 at 22:16 +0200, Ingo Molnar wrote:

> There's one thing Peter noticed: this is not C syntax anymore. It would
> be really nice to keep filter expressions a subset of C.

Also:

> This patch provides basic support for regular expressions in filters.
>
> It supports the following types of regexp:
>
> - *match_beginning
> - *match_middle*
> - match_end*
> - !don't match
>
> Example:
> cd /debug/tracing/events/bkl/lock_kernel
> echo 'file == "*reiserfs*"' > filter
> echo 1 > enable

It says regex, but its not.

Regex would look like: "^.*reiserfs.*$", or simply "reiserfs"

What you implemented is called glob-matching.

If you want to keep this C syntax, you could consider something like:

glob_match(file, "*reiserfs*")

or something.

2009-09-24 20:33:24

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Thu, Sep 24, 2009 at 10:16:22PM +0200, Ingo Molnar wrote:
>
> * Ingo Molnar <[email protected]> wrote:
>
> >
> > * Frederic Weisbecker <[email protected]> wrote:
> >
> > > Ingo,
> > >
> > > This is new iteration of the bkl tracepoints + filter
> > > regex support. It addresses the reviews that were posted
> > > in the previous RFC version.
> > >
> > > Please pull from:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> > > tracing/core
> > >
> > > Thanks;
> > > Frederic.
> > >
> > > Frederic Weisbecker (5):
> > > tracing/bkl: Add bkl ftrace events
> > > tracing/filters: Cleanup useless headers
> > > tracing/event: Cleanup the useless dentry variable
> > > tracing/filters: Provide basic regex support
> > > tracing/filters: Unify the regex parsing helpers
> > >
> > > include/linux/smp_lock.h | 19 ++++-
> > > include/trace/events/bkl.h | 61 ++++++++++++++
> > > kernel/trace/ftrace.c | 64 +--------------
> > > kernel/trace/trace.h | 36 ++++++--
> > > kernel/trace/trace_events.c | 23 +++---
> > > kernel/trace/trace_events_filter.c | 155 +++++++++++++++++++++++++++++++----
> > > lib/kernel_lock.c | 11 ++-
> > > 7 files changed, 262 insertions(+), 107 deletions(-)
> > > create mode 100644 include/trace/events/bkl.h
> >
> > Pulled, thanks a lot Frederic! These bits look very useful.
> >
> > It would be perf-ect now to complete the filters-via-perf-events changes
> > Li Zefan is working on ;-)
>
> There's one thing Peter noticed: this is not C syntax anymore. It would
> be really nice to keep filter expressions a subset of C.
>
> Ingo


You mean the use of stars for the regexes?
Hm, but that looks a intuitive way to define a regex string.
We can't stricly imitate the C without sticking into its
limitations. But it's still a subset of C with this feature.

Or I'm probably missing something?

2009-09-24 20:44:08

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Thu, Sep 24, 2009 at 10:30:00PM +0200, Peter Zijlstra wrote:
> On Thu, 2009-09-24 at 22:16 +0200, Ingo Molnar wrote:
>
> > There's one thing Peter noticed: this is not C syntax anymore. It would
> > be really nice to keep filter expressions a subset of C.
>
> Also:
>
> > This patch provides basic support for regular expressions in filters.
> >
> > It supports the following types of regexp:
> >
> > - *match_beginning
> > - *match_middle*
> > - match_end*
> > - !don't match
> >
> > Example:
> > cd /debug/tracing/events/bkl/lock_kernel
> > echo 'file == "*reiserfs*"' > filter
> > echo 1 > enable
>
> It says regex, but its not.
>
> Regex would look like: "^.*reiserfs.*$", or simply "reiserfs"
>
> What you implemented is called glob-matching.


Ouch, right...


> If you want to keep this C syntax, you could consider something like:
>
> glob_match(file, "*reiserfs*")
>
> or something.
>


I don't quite understand why.

Typing file == "*reiserfs*" looks more intuitive.

It's true that the filters should stay tight to the C syntax,
but following this guideline up to the point that we are forced to
use function expressions to do something that can be expressed
much more easily and more intuitively (IMHO), that all sounds like
an overkill.

The use of glob is a very primary need for filters, it's
so much a basic requirement for it that it should be native
in its language.

2009-09-24 20:51:37

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Thu, 2009-09-24 at 22:44 +0200, Frederic Weisbecker wrote:

> I don't quite understand why.
>
> Typing file == "*reiserfs*" looks more intuitive.
>
> It's true that the filters should stay tight to the C syntax,
> but following this guideline up to the point that we are forced to
> use function expressions to do something that can be expressed
> much more easily and more intuitively (IMHO), that all sounds like
> an overkill.
>
> The use of glob is a very primary need for filters, it's
> so much a basic requirement for it that it should be native
> in its language.

The thing that my fingers know is awk syntax:

file ~ /reiserfs/

I'd very much prefer to keep == for strict equality, however the above
requires actual regex bits.

Remember, there are no second chances for the filter syntax anymore.

2009-09-24 21:36:34

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Thu, Sep 24, 2009 at 10:51:29PM +0200, Peter Zijlstra wrote:
> On Thu, 2009-09-24 at 22:44 +0200, Frederic Weisbecker wrote:
>
> > I don't quite understand why.
> >
> > Typing file == "*reiserfs*" looks more intuitive.
> >
> > It's true that the filters should stay tight to the C syntax,
> > but following this guideline up to the point that we are forced to
> > use function expressions to do something that can be expressed
> > much more easily and more intuitively (IMHO), that all sounds like
> > an overkill.
> >
> > The use of glob is a very primary need for filters, it's
> > so much a basic requirement for it that it should be native
> > in its language.
>
> The thing that my fingers know is awk syntax:
>
> file ~ /reiserfs/
>
> I'd very much prefer to keep == for strict equality, however the above
> requires actual regex bits.


The strict equality issue was the reason that made me first
split the filter file into "filter" and "filter_regex", that
actually should have been "filter_glob" :-)

But the general opinion was in favour of a single file
supporting these globs, especially since it's a non-abi thing.

In the current state, in its current scope and use, I think these
native globs are the right choice.

But if we start to consider it in a larger scope, used by perf and
may be for even wider uses than trace events, then yes we should
double-check the impact of this syntax change.


> Remember, there are no second chances for the filter syntax anymore.


But well, it's not yet an ABI. It's still a baby, although powerful
for filtering, it's not yet a whole scripting language.

Before thinking about it as an ABI, we should develop it, extend
it, use these extensions, spot the weaknesses in the syntax, fix them,
etc...

2009-09-25 07:55:32

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Thu, Sep 24, 2009 at 10:22:25PM +0200, Ingo Molnar wrote:
>
> also, some build problems:
>
> include/trace/events/bkl.h: In function
> ?ftrace_profile_enable_lock_kernel?:
> include/trace/events/bkl.h:9: error: implicit declaration of function
> ?register_trace_lock_kernel?
> include/trace/events/bkl.h: In function
> ?ftrace_profile_disable_lock_kernel?:
> include/trace/events/bkl.h:9: error: implicit declaration of function
> ?unregister_trace_lock_kernel?
> include/trace/events/bkl.h: In function
> ?ftrace_profile_enable_unlock_kernel?:
> include/trace/events/bkl.h:34: error: implicit declaration of function
> ?register_trace_unlock_kernel?
> include/trace/events/bkl.h: In function
> ?ftrace_profile_disable_unlock_kernel?:
> include/trace/events/bkl.h:34: error: implicit declaration of function
> ?unregister_trace_unlock_kernel?
>
> config attached.
>
> Ingo


Ok, I see. The problem is that include/ftrace_event.h includes hardirq.h
which includes smp_lock.h, then each time we define another tracepoint, we
redefine the bkl tracepoints.

Thanks, I'll fix this.

2009-09-25 08:13:51

by Andrey Panin

[permalink] [raw]
Subject: Re: [PATCH 4/5 v2] tracing/filters: Provide basic regex support

On 267, 09 24, 2009 at 09:49:34PM +0200, Frederic Weisbecker wrote:
> This patch provides basic support for regular expressions in filters.

> +/* Basic regex callbacks */
> +static int regex_match_full(char *str, struct regex *r, int len)
> +{
> + if (strncmp(str, r->pattern, len) == 0)
> + return 1;
> + return 0;
> +}
> +
> +static int regex_match_front(char *str, struct regex *r, int len)
> +{
> + if (strncmp(str, r->pattern, len) == 0)
> + return 1;
> + return 0;
> +}

These two functions are identical, what's the reason to keep them separate ?
And why not simply return strncmp(str, r->pattern, len) == 0 ?

2009-09-25 08:16:21

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [PATCH 4/5 v2] tracing/filters: Provide basic regex support

On Fri, Sep 25, 2009 at 12:13:45PM +0400, Andrey Panin wrote:
> On 267, 09 24, 2009 at 09:49:34PM +0200, Frederic Weisbecker wrote:
> > This patch provides basic support for regular expressions in filters.
>
> > +/* Basic regex callbacks */
> > +static int regex_match_full(char *str, struct regex *r, int len)
> > +{
> > + if (strncmp(str, r->pattern, len) == 0)
> > + return 1;
> > + return 0;
> > +}
> > +
> > +static int regex_match_front(char *str, struct regex *r, int len)
> > +{
> > + if (strncmp(str, r->pattern, len) == 0)
> > + return 1;
> > + return 0;
> > +}
>
> These two functions are identical, what's the reason to keep them separate ?
> And why not simply return strncmp(str, r->pattern, len) == 0 ?


Good catch! I'll fix that, thanks.

2009-09-25 08:19:17

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Thu, 2009-09-24 at 23:36 +0200, Frederic Weisbecker wrote:

> > Remember, there are no second chances for the filter syntax anymore.
>
>
> But well, it's not yet an ABI. It's still a baby, although powerful
> for filtering, it's not yet a whole scripting language.
>
> Before thinking about it as an ABI, we should develop it, extend
> it, use these extensions, spot the weaknesses in the syntax, fix them,
> etc...

Then this is a NACK for the perf ioctl for setting a filter. Fine with
me.

2009-09-25 09:12:31

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Fri, Sep 25, 2009 at 10:19:52AM +0200, Peter Zijlstra wrote:
> On Thu, 2009-09-24 at 23:36 +0200, Frederic Weisbecker wrote:
>
> > > Remember, there are no second chances for the filter syntax anymore.
> >
> >
> > But well, it's not yet an ABI. It's still a baby, although powerful
> > for filtering, it's not yet a whole scripting language.
> >
> > Before thinking about it as an ABI, we should develop it, extend
> > it, use these extensions, spot the weaknesses in the syntax, fix them,
> > etc...
>
> Then this is a NACK for the perf ioctl for setting a filter. Fine with
> me.


Ftrace side (debugfs use):

I think the native filter glob is good for ftrace use through debugfs.
As I said, IMO it's so much a primary requirement for events filtering
that it should be a default.
But if others have mixed feelings about it, tell me and I will
reconsider. I've done this native glob in this patchset because the
general opinion (yours included) was in favour of that, instead of
a split into a filter and another filter_glob file.
That said, the future plans have evolved, and I'm fine if you have
changed your opinion and think about a better way to develop this.


Perf side:

But the use from perf would be for a larger scope. And I agree we may
want to break this glob default from it to get a flexible usability
and use a pure string match by default that we can override with
functions-like expressions.


So why not keeping the default native blob for ftrace and throw away
this default for perf? It's a matter of a flag in the filters.

You don't need to drop a NACK rock on the ground and laconically go out
like that to wake me up. I think I've already enough shown how much I'm
willing to help building a nice bridge between ftrace and perf.
I just don't want that this bridge turns out any ftrace uses through debugfs
into an overkill.
Instead I'd prefer to satisfy both, hence the above proposition.

Thanks,
Frederic.

2009-09-25 09:39:21

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Fri, 2009-09-25 at 11:12 +0200, Frederic Weisbecker wrote:
> On Fri, Sep 25, 2009 at 10:19:52AM +0200, Peter Zijlstra wrote:
> > On Thu, 2009-09-24 at 23:36 +0200, Frederic Weisbecker wrote:
> >
> > > > Remember, there are no second chances for the filter syntax anymore.
> > >
> > >
> > > But well, it's not yet an ABI. It's still a baby, although powerful
> > > for filtering, it's not yet a whole scripting language.
> > >
> > > Before thinking about it as an ABI, we should develop it, extend
> > > it, use these extensions, spot the weaknesses in the syntax, fix them,
> > > etc...
> >
> > Then this is a NACK for the perf ioctl for setting a filter. Fine with
> > me.
>
>
> Ftrace side (debugfs use):
>
> I think the native filter glob is good for ftrace use through debugfs.
> As I said, IMO it's so much a primary requirement for events filtering
> that it should be a default.
> But if others have mixed feelings about it, tell me and I will
> reconsider. I've done this native glob in this patchset because the
> general opinion (yours included) was in favour of that, instead of
> a split into a filter and another filter_glob file.

Agreed, a single filter syntax is much preferred.

> That said, the future plans have evolved, and I'm fine if you have
> changed your opinion and think about a better way to develop this.

No, but the thing is, IF we're going to freeze this into ABI, then
there's no second chances.

Using globs in string matches most certainly is useful, no question
about that.

But I had understood from previous communications we were going to have
a C syntax, and there == is a straight comparison.

If however people have changed their minds (fine with me) and we're now
going to script like things..

Anyway, a glob in == just means we have to use another operator if we
ever want to support actual regexes, ~ would then be recommened I think,
since that's what awk and I think perl do.

Personally I wouldn't mind things like:

glob_match(string, pattern)
regex_match(string, pattern)

But everybody involved in this filter stuff needs to agree what
direction you want to take the language in.

> Perf side:
>
> But the use from perf would be for a larger scope. And I agree we may
> want to break this glob default from it to get a flexible usability
> and use a pure string match by default that we can override with
> functions-like expressions.
>
>
> So why not keeping the default native blob for ftrace and throw away
> this default for perf? It's a matter of a flag in the filters.
>
> You don't need to drop a NACK rock on the ground and laconically go out
> like that to wake me up. I think I've already enough shown how much I'm
> willing to help building a nice bridge between ftrace and perf.

Sure, and thanks for that, your efforts really are appreciated.

> I just don't want that this bridge turns out any ftrace uses through debugfs
> into an overkill.
> Instead I'd prefer to satisfy both, hence the above proposition.

So you're proposing to split the filter language? I'm sure that's going
to confuse a few people ;-)

Thing is, if you (or others) have a need to experiment with the
language, then I'm not sure its the right moment to freeze bits into an
ABI.

I'm really fine with thing, as long as everybody on the filter side
knows experimenting isn't really an option and agrees on the direction
they want to take the language.

Is there no existing language with a proper license and clean code-base
we can 'borrow'? That would avoid creating yet another funny language,
and learning how to implement things all over again.

Personally I don't think the kernel is the place to experiment in script
language design, but that's me ;-)

2009-09-25 10:38:11

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Fri, Sep 25, 2009 at 11:40:00AM +0200, Peter Zijlstra wrote:
> On Fri, 2009-09-25 at 11:12 +0200, Frederic Weisbecker wrote:
> > That said, the future plans have evolved, and I'm fine if you have
> > changed your opinion and think about a better way to develop this.
>
> No, but the thing is, IF we're going to freeze this into ABI, then
> there's no second chances.



Right. Once it becomes an ioctl, it becomes an ABI :-/



> Using globs in string matches most certainly is useful, no question
> about that.
>
> But I had understood from previous communications we were going to have
> a C syntax, and there == is a straight comparison.
>
> If however people have changed their minds (fine with me) and we're now
> going to script like things..



Well, indeed we talked about C syntax, but I didn't think the idea
was that fixed in the rock, hence why I was suprised.


> Anyway, a glob in == just means we have to use another operator if we
> ever want to support actual regexes, ~ would then be recommened I think,
> since that's what awk and I think perl do.


Yeah. For example one may know python but not perl or awk,
other people may be in the opposite situation. But most
developers know the C (at least its basic syntax).

So I'm not sure using such ~ operator is a good idea. I think you're
right in the fact we should stay tight to the C syntax.


> Personally I wouldn't mind things like:
>
> glob_match(string, pattern)
> regex_match(string, pattern)



Yeah, actually that sounds more flexible and more something that people
are familar with, once we consider the future evolutions.



> But everybody involved in this filter stuff needs to agree what
> direction you want to take the language in.



Right!



> > I just don't want that this bridge turns out any ftrace uses through debugfs
> > into an overkill.
> > Instead I'd prefer to satisfy both, hence the above proposition.
>
> So you're proposing to split the filter language? I'm sure that's going
> to confuse a few people ;-)



Hmm, just at this level. That could even be a trace option.
Anyway, it would nice to have other tracing developers
opinion.



> Thing is, if you (or others) have a need to experiment with the
> language, then I'm not sure its the right moment to freeze bits into an
> ABI.
>
> I'm really fine with thing, as long as everybody on the filter side
> knows experimenting isn't really an option and agrees on the direction
> they want to take the language.


Well, I talked about experimenting the language before pushing it as
an ABI because I was afraid we were going too fast.

But I guess the ABI is a requirement to use it through perf ioctl,
and delay that would keep it as a hostage, may be even slow its
development.


> Is there no existing language with a proper license and clean code-base
> we can 'borrow'? That would avoid creating yet another funny language,
> and learning how to implement things all over again.
>
> Personally I don't think the kernel is the place to experiment in script
> language design, but that's me ;-)


Python? :-)

More seriously, as I said above, I think most developers are familiar with C
syntax, so IMHO this is one of our best possibility.

2009-09-26 10:31:25

by Steven Rostedt

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Fri, 2009-09-25 at 11:40 +0200, Peter Zijlstra wrote:

> Using globs in string matches most certainly is useful, no question
> about that.
>
> But I had understood from previous communications we were going to have
> a C syntax, and there == is a straight comparison.
>
> If however people have changed their minds (fine with me) and we're now
> going to script like things..
>
> Anyway, a glob in == just means we have to use another operator if we
> ever want to support actual regexes, ~ would then be recommened I think,
> since that's what awk and I think perl do.

I agree that any use of '==' should be a direct match and if you want to
add a glob expression you can use something else. Like what Peter showed
(~) or even better =~ which is what perl uses.

/me runs

;-)

-- Steve

>
> Personally I wouldn't mind things like:
>
> glob_match(string, pattern)
> regex_match(string, pattern)
>
> But everybody involved in this filter stuff needs to agree what
> direction you want to take the language in.

2009-09-26 10:45:33

by Steven Rostedt

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Fri, 2009-09-25 at 12:38 +0200, Frederic Weisbecker wrote:

>
> > Using globs in string matches most certainly is useful, no question
> > about that.
> >
> > But I had understood from previous communications we were going to have
> > a C syntax, and there == is a straight comparison.
> >
> > If however people have changed their minds (fine with me) and we're now
> > going to script like things..
>
>
>
> Well, indeed we talked about C syntax, but I didn't think the idea
> was that fixed in the rock, hence why I was suprised.

Once we add globs, we just blew away C syntax.

>
>
> > Anyway, a glob in == just means we have to use another operator if we
> > ever want to support actual regexes, ~ would then be recommened I think,
> > since that's what awk and I think perl do.

Perhaps when we put full perl regex into the kernel (my goal ;-) then we
should look to keep different kinds of equals.

== - is direct match. Only use of strcmp is needed.

~ - is globing. We can add a '*' which means match anything.

and if we do add true regex...

=~ could be that. field =~ '^spin.*{lock|unlock}$'

>
>
> Yeah. For example one may know python but not perl or awk,
> other people may be in the opposite situation. But most
> developers know the C (at least its basic syntax).

awk is much more known than either python nor perl. It is expected that
any unix person have a basic idea of sed and awk. If not a simple search
on the internet can help them.

It takes 5 minutes to figure out how to do something with awk, where as
we all know it takes a much longer time to figure out python or perl.

>
> So I'm not sure using such ~ operator is a good idea. I think you're
> right in the fact we should stay tight to the C syntax.

I disagree.

>
>
> > Personally I wouldn't mind things like:
> >
> > glob_match(string, pattern)
> > regex_match(string, pattern)

In a filter string. Yuck!

note I don't like python, which is probably why I don't like the above.

>
>
>
> Yeah, actually that sounds more flexible and more something that people
> are familar with, once we consider the future evolutions.

please no! I hate that syntax. Again, this is probably one of the major
reasons I avoid python. (that and column forcing)


>
>
>
> > But everybody involved in this filter stuff needs to agree what
> > direction you want to take the language in.
>
>
>
> Right!

Yes, and I agree that == should not mean globing. We should have another
syntax, but I really don't want "functions" for matching.

>
>
>
> > > I just don't want that this bridge turns out any ftrace uses through debugfs
> > > into an overkill.
> > > Instead I'd prefer to satisfy both, hence the above proposition.
> >
> > So you're proposing to split the filter language? I'm sure that's going
> > to confuse a few people ;-)
>
>
>
> Hmm, just at this level. That could even be a trace option.
> Anyway, it would nice to have other tracing developers
> opinion.

Finally getting around to it ;-)

>
>
>
> > Thing is, if you (or others) have a need to experiment with the
> > language, then I'm not sure its the right moment to freeze bits into an
> > ABI.

Correct, and this is why I propose a "tracefs" that can become the place
that we add a stable API, and let debugfs be our playground.

> >
> > I'm really fine with thing, as long as everybody on the filter side
> > knows experimenting isn't really an option and agrees on the direction
> > they want to take the language.
>
>
> Well, I talked about experimenting the language before pushing it as
> an ABI because I was afraid we were going too fast.
>
> But I guess the ABI is a requirement to use it through perf ioctl,
> and delay that would keep it as a hostage, may be even slow its
> development.
>
>
> > Is there no existing language with a proper license and clean code-base
> > we can 'borrow'? That would avoid creating yet another funny language,
> > and learning how to implement things all over again.
> >
> > Personally I don't think the kernel is the place to experiment in script
> > language design, but that's me ;-)
>
>
> Python? :-)

Perl is considered a much better language for regex. It has one of the
most (if not the most) powerful regex engines. I'm sure recordmcount.pl
would be much larger if I chose to do it in python. Same goes with
streamline_config.pl. They both have strong needs for complex regex.

>
> More seriously, as I said above, I think most developers are familiar with C
> syntax, so IMHO this is one of our best possibility.
>

To avoid the Python vs Perl, I say we stick with sed/awk. That is also a
requirement for most unix developers.

-- Steve

2009-09-26 15:47:23

by Frank Ch. Eigler

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support


Hi -


Frederic Weisbecker <[email protected]> writes:

>> > That said, the future plans have evolved, and I'm fine if you have
>> > changed your opinion and think about a better way to develop this.

>> No, but the thing is, IF we're going to freeze this into ABI, then
>> there's no second chances.

> Right. Once it becomes an ioctl, it becomes an ABI :-/

Are y'all convinced that a little ascii language parser/interpreter is
the right thing to put into the kernel, as opposed to bytecode (with a
userspace compiler)?

- FChE

2009-10-03 11:36:29

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Sat, Sep 26, 2009 at 06:44:21AM -0400, Steven Rostedt wrote:
>
> Perhaps when we put full perl regex into the kernel (my goal ;-) then we
> should look to keep different kinds of equals.
>
> == - is direct match. Only use of strcmp is needed.
>
> ~ - is globing. We can add a '*' which means match anything.
>
> and if we do add true regex...
>
> =~ could be that. field =~ '^spin.*{lock|unlock}$'


Ok.
I'm personnally fine with it.



> Perl is considered a much better language for regex. It has one of the
> most (if not the most) powerful regex engines. I'm sure recordmcount.pl
> would be much larger if I chose to do it in python. Same goes with
> streamline_config.pl. They both have strong needs for complex regex.


Yeah, python was a joke. I mean I think it's a nice language
but not a syntax for what we want to do. It's an object oriented
language and I guess we don't want to do:

import re

r = re.compile("blah")
m = r.match("string")
m.group(1)
....

:-)


> >
> > More seriously, as I said above, I think most developers are familiar with C
> > syntax, so IMHO this is one of our best possibility.
> >
>
> To avoid the Python vs Perl, I say we stick with sed/awk. That is also a
> requirement for most unix developers.


Yeah I'm fine with it. As long as this is something already familiar
for most users.

Let's then pick ~ for glob and ~= for regex

I'll fix that soon.

Thanks.

2009-10-03 11:49:51

by Frederic Weisbecker

[permalink] [raw]
Subject: Re: [GIT PULL v2] bkl tracepoints + filter regex support

On Sat, Sep 26, 2009 at 11:47:06AM -0400, Frank Ch. Eigler wrote:
>
> Hi -
>
>
> Frederic Weisbecker <[email protected]> writes:
>
> >> > That said, the future plans have evolved, and I'm fine if you have
> >> > changed your opinion and think about a better way to develop this.
>
> >> No, but the thing is, IF we're going to freeze this into ABI, then
> >> there's no second chances.
>
> > Right. Once it becomes an ioctl, it becomes an ABI :-/
>
> Are y'all convinced that a little ascii language parser/interpreter is
> the right thing to put into the kernel, as opposed to bytecode (with a
> userspace compiler)?
>
> - FChE


We need something directly understandable from the kernel interfaces.
As an example we don't want the ftrace users to bother about compiling
a string before applying it on an event filter through ftrace debugfs
interface, we want it directly usable without middle-state in the worklow.

That's also the same for perf syscalls users.

Moreover it's a really small language, based on predicates. Fetching
strings against bytecodes won't make that much differences in the
amount of kernel code to maintain.