[PATCH 1/5] ftrace: remove redundant strsep in mod_callback

[PATCH 1/5] ftrace: remove redundant strsep in mod_callback

[Dmitry Safonov]

By now there isn't any subcommand for mod.

Before:
	sh$ echo '*:mod:ipv6:a' > set_ftrace_filter
	sh$ echo '*:mod:ipv6' > set_ftrace_filter
had the same results, but now first will result in:
	sh$ echo '*:mod:ipv6:a' > set_ftrace_filter
	-bash: echo: write error: Invalid argument

Also, I clarified ftrace_mod_callback code a little.

Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
---
 kernel/trace/ftrace.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index b0623ac..f87401b 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -3564,8 +3564,7 @@ static int
 ftrace_mod_callback(struct ftrace_hash *hash,
 		    char *func, char *cmd, char *param, int enable)
 {
-	char *mod;
-	int ret = -EINVAL;
+	int ret;
 
 	/*
 	 * cmd == 'mod' because we only registered this func
@@ -3576,16 +3575,12 @@ ftrace_mod_callback(struct ftrace_hash *hash,
 	 */
 
 	/* we must have a module name */
-	if (!param)
-		return ret;
-
-	mod = strsep(&param, ":");
-	if (!strlen(mod))
-		return ret;
+	if (!param || !strlen(param))
+		return -EINVAL;
 
-	ret = ftrace_match_module_records(hash, func, mod);
-	if (!ret)
-		ret = -EINVAL;
+	ret = ftrace_match_module_records(hash, func, param);
+	if (ret == 0)
+		return -EINVAL;
 	if (ret < 0)
 		return ret;
 
-- 
2.5.3

[PATCH 2/5] ftrace: clarify code for mod command

[Dmitry Safonov]

"Not" is too abstract variable name - changed to clear_filter.
Removed ftrace_match_module_records function: comparison with !* or *
not does the general code in filter_parse_regex() as it works without
mod command for
  sh# echo '!*' > /sys/kernel/debug/tracing/set_ftrace_filter

Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
---
 kernel/trace/ftrace.c | 37 ++++++++++---------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f87401b..a56f028 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -3449,13 +3449,13 @@ static int ftrace_match(char *str, char *regex, int len, int type)
 }
 
 static int
-enter_record(struct ftrace_hash *hash, struct dyn_ftrace *rec, int not)
+enter_record(struct ftrace_hash *hash, struct dyn_ftrace *rec, int clear_filter)
 {
 	struct ftrace_func_entry *entry;
 	int ret = 0;
 
 	entry = ftrace_lookup_ip(hash, rec->ip);
-	if (not) {
+	if (clear_filter) {
 		/* Do nothing if it doesn't exist */
 		if (!entry)
 			return 0;
@@ -3494,8 +3494,7 @@ ftrace_match_record(struct dyn_ftrace *rec, char *mod,
 }
 
 static int
-match_records(struct ftrace_hash *hash, char *buff,
-	      int len, char *mod, int not)
+match_records(struct ftrace_hash *hash, char *buff, int len, char *mod)
 {
 	unsigned search_len = 0;
 	struct ftrace_page *pg;
@@ -3504,9 +3503,10 @@ match_records(struct ftrace_hash *hash, char *buff,
 	char *search = buff;
 	int found = 0;
 	int ret;
+	int clear_filter;
 
 	if (len) {
-		type = filter_parse_regex(buff, len, &search, &not);
+		type = filter_parse_regex(buff, len, &search, &clear_filter);
 		search_len = strlen(search);
 	}
 
@@ -3517,7 +3517,7 @@ match_records(struct ftrace_hash *hash, char *buff,
 
 	do_for_each_ftrace_rec(pg, rec) {
 		if (ftrace_match_record(rec, mod, search, search_len, type)) {
-			ret = enter_record(hash, rec, not);
+			ret = enter_record(hash, rec, clear_filter);
 			if (ret < 0) {
 				found = ret;
 				goto out_unlock;
@@ -3534,26 +3534,9 @@ match_records(struct ftrace_hash *hash, char *buff,
 static int
 ftrace_match_records(struct ftrace_hash *hash, char *buff, int len)
 {
-	return match_records(hash, buff, len, NULL, 0);
+	return match_records(hash, buff, len, NULL);
 }
 
-static int
-ftrace_match_module_records(struct ftrace_hash *hash, char *buff, char *mod)
-{
-	int not = 0;
-
-	/* blank or '*' mean the same */
-	if (strcmp(buff, "*") == 0)
-		buff[0] = 0;
-
-	/* handle the case of 'dont filter this module' */
-	if (strcmp(buff, "!") == 0 || strcmp(buff, "!*") == 0) {
-		buff[0] = 0;
-		not = 1;
-	}
-
-	return match_records(hash, buff, strlen(buff), mod, not);
-}
 
 /*
  * We register the module command as a template to show others how
@@ -3562,7 +3545,7 @@ ftrace_match_module_records(struct ftrace_hash *hash, char *buff, char *mod)
 
 static int
 ftrace_mod_callback(struct ftrace_hash *hash,
-		    char *func, char *cmd, char *param, int enable)
+		    char *func, char *cmd, char *module, int enable)
 {
 	int ret;
 
@@ -3575,10 +3558,10 @@ ftrace_mod_callback(struct ftrace_hash *hash,
 	 */
 
 	/* we must have a module name */
-	if (!param || !strlen(param))
+	if (!module || !strlen(module))
 		return -EINVAL;
 
-	ret = ftrace_match_module_records(hash, func, param);
+	ret = match_records(hash, func, strlen(func), module);
 	if (ret == 0)
 		return -EINVAL;
 	if (ret < 0)
-- 
2.5.3

[PATCH 3/5] ftrace: introduce ftrace_glob structure

[Dmitry Safonov]

ftrace_match parameters are very related and I reduce the number of local
variables & parameters with it.
This is also preparation for module globbing as it would introduce more
realated variables & parameters.

Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
---
 kernel/trace/ftrace.c | 84 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 46 insertions(+), 38 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a56f028..6ffc1a2 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -3420,27 +3420,35 @@ ftrace_notrace_open(struct inode *inode, struct file *file)
 				 inode, file);
 }
 
-static int ftrace_match(char *str, char *regex, int len, int type)
+/* Type for quick search ftrace basic regexes (globs) from filter_parse_regex */
+struct ftrace_glob {
+	char *search;
+	unsigned len;
+	int type;
+};
+
+static int ftrace_match(char *str, struct ftrace_glob *g)
 {
 	int matched = 0;
 	int slen;
 
-	switch (type) {
+	switch (g->type) {
 	case MATCH_FULL:
-		if (strcmp(str, regex) == 0)
+		if (strcmp(str, g->search) == 0)
 			matched = 1;
 		break;
 	case MATCH_FRONT_ONLY:
-		if (strncmp(str, regex, len) == 0)
+		if (strncmp(str, g->search, g->len) == 0)
 			matched = 1;
 		break;
 	case MATCH_MIDDLE_ONLY:
-		if (strstr(str, regex))
+		if (strstr(str, g->search))
 			matched = 1;
 		break;
 	case MATCH_END_ONLY:
 		slen = strlen(str);
-		if (slen >= len && memcmp(str + slen - len, regex, len) == 0)
+		if (slen >= g->len && memcmp(str + slen - g->len,
+					g->search, g->len) == 0)
 			matched = 1;
 		break;
 	}
@@ -3472,8 +3480,8 @@ enter_record(struct ftrace_hash *hash, struct dyn_ftrace *rec, int clear_filter)
 }
 
 static int
-ftrace_match_record(struct dyn_ftrace *rec, char *mod,
-		    char *regex, int len, int type)
+ftrace_match_record(struct dyn_ftrace *rec,
+		char *mod, struct ftrace_glob *func_g)
 {
 	char str[KSYM_SYMBOL_LEN];
 	char *modname;
@@ -3486,28 +3494,27 @@ ftrace_match_record(struct dyn_ftrace *rec, char *mod,
 			return 0;
 
 		/* blank search means to match all funcs in the mod */
-		if (!len)
+		if (!func_g->len)
 			return 1;
 	}
 
-	return ftrace_match(str, regex, len, type);
+	return ftrace_match(str, func_g);
 }
 
 static int
-match_records(struct ftrace_hash *hash, char *buff, int len, char *mod)
+match_records(struct ftrace_hash *hash, char *func, int len, char *mod)
 {
-	unsigned search_len = 0;
 	struct ftrace_page *pg;
 	struct dyn_ftrace *rec;
-	int type = MATCH_FULL;
-	char *search = buff;
+	struct ftrace_glob func_g = { .type = MATCH_FULL };
 	int found = 0;
 	int ret;
 	int clear_filter;
 
 	if (len) {
-		type = filter_parse_regex(buff, len, &search, &clear_filter);
-		search_len = strlen(search);
+		func_g.type = filter_parse_regex(func, len,
+				&func_g.search, &clear_filter);
+		func_g.len = strlen(func_g.search);
 	}
 
 	mutex_lock(&ftrace_lock);
@@ -3516,7 +3523,7 @@ match_records(struct ftrace_hash *hash, char *buff, int len, char *mod)
 		goto out_unlock;
 
 	do_for_each_ftrace_rec(pg, rec) {
-		if (ftrace_match_record(rec, mod, search, search_len, type)) {
+		if (ftrace_match_record(rec, mod, &func_g)) {
 			ret = enter_record(hash, rec, clear_filter);
 			if (ret < 0) {
 				found = ret;
@@ -3682,14 +3689,15 @@ register_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
 	struct ftrace_hash *hash;
 	struct ftrace_page *pg;
 	struct dyn_ftrace *rec;
-	int type, len, not;
+	int not;
 	unsigned long key;
 	int count = 0;
-	char *search;
 	int ret;
+	struct ftrace_glob func_g;
 
-	type = filter_parse_regex(glob, strlen(glob), &search, &not);
-	len = strlen(search);
+	func_g.type = filter_parse_regex(glob, strlen(glob),
+			&func_g.search, &not);
+	func_g.len = strlen(func_g.search);
 
 	/* we do not support '!' for function probes */
 	if (WARN_ON(not))
@@ -3716,7 +3724,7 @@ register_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
 
 	do_for_each_ftrace_rec(pg, rec) {
 
-		if (!ftrace_match_record(rec, NULL, search, len, type))
+		if (!ftrace_match_record(rec, NULL, &func_g))
 			continue;
 
 		entry = kmalloc(sizeof(*entry), GFP_KERNEL);
@@ -3795,18 +3803,18 @@ __unregister_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
 	struct ftrace_hash *hash;
 	struct hlist_node *tmp;
 	char str[KSYM_SYMBOL_LEN];
-	int type = MATCH_FULL;
-	int i, len = 0;
-	char *search;
-	int ret;
+	int i, ret;
+	struct ftrace_glob func_g;
 
 	if (glob && (strcmp(glob, "*") == 0 || !strlen(glob)))
-		glob = NULL;
+		func_g.search = NULL;
 	else if (glob) {
 		int not;
 
-		type = filter_parse_regex(glob, strlen(glob), &search, &not);
-		len = strlen(search);
+		func_g.type = filter_parse_regex(glob, strlen(glob),
+				&func_g.search, &not);
+		func_g.len = strlen(func_g.search);
+		func_g.search = glob;
 
 		/* we do not support '!' for function probes */
 		if (WARN_ON(not))
@@ -3835,10 +3843,10 @@ __unregister_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
 				continue;
 
 			/* do this last, since it is the most expensive */
-			if (glob) {
+			if (func_g.search) {
 				kallsyms_lookup(entry->ip, NULL, NULL,
 						NULL, str);
-				if (!ftrace_match(str, glob, len, type))
+				if (!ftrace_match(str, &func_g))
 					continue;
 			}
 
@@ -3867,7 +3875,7 @@ __unregister_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
 		ftrace_free_entry(entry);
 	}
 	mutex_unlock(&ftrace_lock);
-		
+
  out_unlock:
 	mutex_unlock(&trace_probe_ops.func_hash->regex_lock);
 	free_ftrace_hash(hash);
@@ -4585,19 +4593,19 @@ ftrace_set_func(unsigned long *array, int *idx, int size, char *buffer)
 {
 	struct dyn_ftrace *rec;
 	struct ftrace_page *pg;
-	int search_len;
 	int fail = 1;
-	int type, not;
-	char *search;
+	int not;
 	bool exists;
 	int i;
+	struct ftrace_glob func_g;
 
 	/* decode regex */
-	type = filter_parse_regex(buffer, strlen(buffer), &search, &not);
+	func_g.type = filter_parse_regex(buffer, strlen(buffer),
+			&func_g.search, &not);
 	if (!not && *idx >= size)
 		return -EBUSY;
 
-	search_len = strlen(search);
+	func_g.len = strlen(func_g.search);
 
 	mutex_lock(&ftrace_lock);
 
@@ -4608,7 +4616,7 @@ ftrace_set_func(unsigned long *array, int *idx, int size, char *buffer)
 
 	do_for_each_ftrace_rec(pg, rec) {
 
-		if (ftrace_match_record(rec, NULL, search, search_len, type)) {
+		if (ftrace_match_record(rec, NULL, &func_g)) {
 			/* if it is in the array */
 			exists = false;
 			for (i = 0; i < *idx; i++) {
-- 
2.5.3

[PATCH 4/5] ftrace: add module globbing

[Dmitry Safonov]

Extend module command for function filter selection with globbing.
It uses the same globbing as function filter.

  sh# echo '*alloc*:mod:*' > set_ftrace_filter

Will trace any function with the letters 'alloc' in the name in any
module but not in kernel.

  sh# echo '!*alloc*:mod:ipv6' >> set_ftrace_filter

Will prevent from tracing functions with 'alloc' in the name from module
ipv6 (do not forget to append to set_ftrace_filter file).

  sh# echo '*alloc*:mod:!ipv6' > set_ftrace_filter

Will trace functions with 'alloc' in the name from kernel and any
module except ipv6.

  sh# echo '*alloc*:mod:!*' > set_ftrace_filter

Will trace any function with the letters 'alloc' in the name only from
kernel, but not from any module.

  sh# echo '*:mod:!*' > set_ftrace_filter
or
  sh# echo ':mod:!' > set_ftrace_filter

Will trace every function in the kernel, but will not trace functions
from any module.

  sh# echo '*:mod:*' > set_ftrace_filter
or
  sh# echo ':mod:' > set_ftrace_filter

As the opposite will trace all functions from all modules, but not from
kernel.

  sh# echo '*:mod:*snd*' > set_ftrace_filter

Will trace your sound drivers only (if any).

Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
---
 kernel/trace/ftrace.c | 54 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 17 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 6ffc1a2..a2a4e57 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -3480,19 +3480,37 @@ enter_record(struct ftrace_hash *hash, struct dyn_ftrace *rec, int clear_filter)
 }
 
 static int
-ftrace_match_record(struct dyn_ftrace *rec,
-		char *mod, struct ftrace_glob *func_g)
+ftrace_match_record(struct dyn_ftrace *rec, struct ftrace_glob *func_g,
+		struct ftrace_glob *mod_g, int exclude_mod)
 {
 	char str[KSYM_SYMBOL_LEN];
 	char *modname;
 
 	kallsyms_lookup(rec->ip, NULL, NULL, &modname, str);
 
-	if (mod) {
-		/* module lookup requires matching the module */
-		if (!modname || strcmp(modname, mod))
+	if (mod_g) {
+		int mod_matches = (modname) ? ftrace_match(modname, mod_g) : 0;
+
+		/* blank module name to match all modules */
+		if (!mod_g->len) {
+			/* blank module globbing: modname xor exclude_mod */
+			if ((!exclude_mod) != (!modname))
+				goto func_match;
+			return 0;
+		}
+
+		/* not matching the module */
+		if (!modname || !mod_matches) {
+			if (exclude_mod)
+				goto func_match;
+			else
+				return 0;
+		}
+
+		if (mod_matches && exclude_mod)
 			return 0;
 
+func_match:
 		/* blank search means to match all funcs in the mod */
 		if (!func_g->len)
 			return 1;
@@ -3507,23 +3525,31 @@ match_records(struct ftrace_hash *hash, char *func, int len, char *mod)
 	struct ftrace_page *pg;
 	struct dyn_ftrace *rec;
 	struct ftrace_glob func_g = { .type = MATCH_FULL };
+	struct ftrace_glob mod_g = { .type = MATCH_FULL };
+	struct ftrace_glob *mod_match = (mod) ? &mod_g : NULL;
 	int found = 0;
 	int ret;
-	int clear_filter;
+	int clear_filter, exclude_mod = 0;
 
-	if (len) {
+	if (func) {
 		func_g.type = filter_parse_regex(func, len,
 				&func_g.search, &clear_filter);
 		func_g.len = strlen(func_g.search);
 	}
 
+	if (mod) {
+		mod_g.type = filter_parse_regex(mod, strlen(mod),
+				&mod_g.search, &exclude_mod);
+		mod_g.len = strlen(mod_g.search);
+	}
+
 	mutex_lock(&ftrace_lock);
 
 	if (unlikely(ftrace_disabled))
 		goto out_unlock;
 
 	do_for_each_ftrace_rec(pg, rec) {
-		if (ftrace_match_record(rec, mod, &func_g)) {
+		if (ftrace_match_record(rec, &func_g, mod_match, exclude_mod)) {
 			ret = enter_record(hash, rec, clear_filter);
 			if (ret < 0) {
 				found = ret;
@@ -3554,8 +3580,6 @@ static int
 ftrace_mod_callback(struct ftrace_hash *hash,
 		    char *func, char *cmd, char *module, int enable)
 {
-	int ret;
-
 	/*
 	 * cmd == 'mod' because we only registered this func
 	 * for the 'mod' ftrace_func_command.
@@ -3564,16 +3588,12 @@ ftrace_mod_callback(struct ftrace_hash *hash,
 	 * parameter.
 	 */
 
-	/* we must have a module name */
-	if (!module || !strlen(module))
-		return -EINVAL;
+	int ret = match_records(hash, func, strlen(func), module);
 
-	ret = match_records(hash, func, strlen(func), module);
 	if (ret == 0)
 		return -EINVAL;
 	if (ret < 0)
 		return ret;
-
 	return 0;
 }
 
@@ -3724,7 +3744,7 @@ register_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
 
 	do_for_each_ftrace_rec(pg, rec) {
 
-		if (!ftrace_match_record(rec, NULL, &func_g))
+		if (!ftrace_match_record(rec, &func_g, NULL, 0))
 			continue;
 
 		entry = kmalloc(sizeof(*entry), GFP_KERNEL);
@@ -4616,7 +4636,7 @@ ftrace_set_func(unsigned long *array, int *idx, int size, char *buffer)
 
 	do_for_each_ftrace_rec(pg, rec) {
 
-		if (ftrace_match_record(rec, NULL, &func_g)) {
+		if (ftrace_match_record(rec, &func_g, NULL, 0)) {
 			/* if it is in the array */
 			exists = false;
 			for (i = 0; i < *idx; i++) {
-- 
2.5.3

[PATCH 5/5] Documentation: ftrace: module globbing usage

[Dmitry Safonov]

Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
---
 Documentation/trace/ftrace.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/trace/ftrace.txt b/Documentation/trace/ftrace.txt
index ef621d3..db18362 100644
--- a/Documentation/trace/ftrace.txt
+++ b/Documentation/trace/ftrace.txt
@@ -2437,6 +2437,23 @@ The following commands are supported:
 
    echo '!writeback*:mod:ext3' >> set_ftrace_filter
 
+  Mod command supports module globbing. Disable tracing for all
+  functions except a specific module:
+
+   echo '!*:mod:!ext3' >> set_ftrace_filter
+
+  Disable tracing for all modules, but still trace kernel:
+
+   echo '!*:mod:*' >> set_ftrace_filter
+
+  Enable filter only for kernel:
+
+   echo '*write*:mod:!*' >> set_ftrace_filter
+
+  Enable filter for module globbing:
+
+   echo '*write*:mod:*snd*' >> set_ftrace_filter
+
 - traceon/traceoff
   These commands turn tracing on and off when the specified
   functions are hit. The parameter determines how many times the
-- 
2.5.3

Re: [PATCH 1/5] ftrace: remove redundant strsep in mod_callback

[Steven Rostedt]

On Tue, 29 Sep 2015 19:46:12 +0300
Dmitry Safonov <0x7f454c46@gmail.com> wrote:

> By now there isn't any subcommand for mod.
> 
> Before:
> 	sh$ echo '*:mod:ipv6:a' > set_ftrace_filter
> 	sh$ echo '*:mod:ipv6' > set_ftrace_filter
> had the same results, but now first will result in:
> 	sh$ echo '*:mod:ipv6:a' > set_ftrace_filter
> 	-bash: echo: write error: Invalid argument
> 
> Also, I clarified ftrace_mod_callback code a little.

Thanks for the patches. I don't have time to look at them at the moment
as I'm trying to finish up some stuff before I leave for LinuxCon EU.

If you don't hear from me by Monday, feel free to ping me again. I'll
be in Dublin (for LinuxCon), but I should be able to take a look at the
patches while I'm there. What do you think keynotes are for ;-)

-- Steve

> 
> Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
> ---
>  kernel/trace/ftrace.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index b0623ac..f87401b 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -3564,8 +3564,7 @@ static int
>  ftrace_mod_callback(struct ftrace_hash *hash,
>  		    char *func, char *cmd, char *param, int enable)
>  {
> -	char *mod;
> -	int ret = -EINVAL;
> +	int ret;
>  
>  	/*
>  	 * cmd == 'mod' because we only registered this func
> @@ -3576,16 +3575,12 @@ ftrace_mod_callback(struct ftrace_hash *hash,
>  	 */
>  
>  	/* we must have a module name */
> -	if (!param)
> -		return ret;
> -
> -	mod = strsep(&param, ":");
> -	if (!strlen(mod))
> -		return ret;
> +	if (!param || !strlen(param))
> +		return -EINVAL;
>  
> -	ret = ftrace_match_module_records(hash, func, mod);
> -	if (!ret)
> -		ret = -EINVAL;
> +	ret = ftrace_match_module_records(hash, func, param);
> +	if (ret == 0)
> +		return -EINVAL;
>  	if (ret < 0)
>  		return ret;
>  

Re: [PATCH 1/5] ftrace: remove redundant strsep in mod_callback

[Дмитрий Сафонов]

Hello again, Steven.
I'm pinging :)

2015-09-29 22:11 GMT+03:00 Steven Rostedt <rostedt@goodmis.org>:
> On Tue, 29 Sep 2015 19:46:12 +0300
> Dmitry Safonov <0x7f454c46@gmail.com> wrote:
>
>> By now there isn't any subcommand for mod.
>>
>> Before:
>>       sh$ echo '*:mod:ipv6:a' > set_ftrace_filter
>>       sh$ echo '*:mod:ipv6' > set_ftrace_filter
>> had the same results, but now first will result in:
>>       sh$ echo '*:mod:ipv6:a' > set_ftrace_filter
>>       -bash: echo: write error: Invalid argument
>>
>> Also, I clarified ftrace_mod_callback code a little.
>
> Thanks for the patches. I don't have time to look at them at the moment
> as I'm trying to finish up some stuff before I leave for LinuxCon EU.
>
> If you don't hear from me by Monday, feel free to ping me again. I'll
> be in Dublin (for LinuxCon), but I should be able to take a look at the
> patches while I'm there. What do you think keynotes are for ;-)
>
> -- Steve
>
>>
>> Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
>> ---
>>  kernel/trace/ftrace.c | 17 ++++++-----------
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> index b0623ac..f87401b 100644
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -3564,8 +3564,7 @@ static int
>>  ftrace_mod_callback(struct ftrace_hash *hash,
>>                   char *func, char *cmd, char *param, int enable)
>>  {
>> -     char *mod;
>> -     int ret = -EINVAL;
>> +     int ret;
>>
>>       /*
>>        * cmd == 'mod' because we only registered this func
>> @@ -3576,16 +3575,12 @@ ftrace_mod_callback(struct ftrace_hash *hash,
>>        */
>>
>>       /* we must have a module name */
>> -     if (!param)
>> -             return ret;
>> -
>> -     mod = strsep(&param, ":");
>> -     if (!strlen(mod))
>> -             return ret;
>> +     if (!param || !strlen(param))
>> +             return -EINVAL;
>>
>> -     ret = ftrace_match_module_records(hash, func, mod);
>> -     if (!ret)
>> -             ret = -EINVAL;
>> +     ret = ftrace_match_module_records(hash, func, param);
>> +     if (ret == 0)
>> +             return -EINVAL;
>>       if (ret < 0)
>>               return ret;
>>
>

Re: [PATCH 1/5] ftrace: remove redundant strsep in mod_callback

[Steven Rostedt]

On Tue, 29 Sep 2015 19:46:12 +0300
Dmitry Safonov <0x7f454c46@gmail.com> wrote:
  
> -	ret = ftrace_match_module_records(hash, func, mod);
> -	if (!ret)
> -		ret = -EINVAL;
> +	ret = ftrace_match_module_records(hash, func, param);
> +	if (ret == 0)

Small nit, personally, I prefer the if (!ret) version.

For me "== 0" is for counters and strcmp. I may change this to be
consistent with the rest of the code.

-- Steve

> +		return -EINVAL;
>  	if (ret < 0)
>  		return ret;
>  

Re: [PATCH 1/5] ftrace: remove redundant strsep in mod_callback

[Dmitry Safonov]

2015-10-14 3:48 GMT+03:00 Steven Rostedt <rostedt@goodmis.org>:
>> -     ret = ftrace_match_module_records(hash, func, mod);
>> -     if (!ret)
>> -             ret = -EINVAL;
>> +     ret = ftrace_match_module_records(hash, func, param);
>> +     if (ret == 0)
>
> Small nit, personally, I prefer the if (!ret) version.
>
> For me "== 0" is for counters and strcmp. I may change this to be
> consistent with the rest of the code.
>
> -- Steve
Ok, I thought "== 0" will be more readable near comparison "< 0",
but anyway, (!ret) is good.

>
>> +             return -EINVAL;
>>       if (ret < 0)
>>               return ret;
>>

Re: [PATCH 3/5] ftrace: introduce ftrace_glob structure

[Steven Rostedt]

On Tue, 29 Sep 2015 19:46:14 +0300
Dmitry Safonov <0x7f454c46@gmail.com> wrote:

> ftrace_match parameters are very related and I reduce the number of local
> variables & parameters with it.
> This is also preparation for module globbing as it would introduce more
> realated variables & parameters.
> 
> Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
> ---
>  kernel/trace/ftrace.c | 84 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 46 insertions(+), 38 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index a56f028..6ffc1a2 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -3420,27 +3420,35 @@ ftrace_notrace_open(struct inode *inode, struct file *file)
>  				 inode, file);
>  }
>  
> -static int ftrace_match(char *str, char *regex, int len, int type)
> +/* Type for quick search ftrace basic regexes (globs) from filter_parse_regex */
> +struct ftrace_glob {
> +	char *search;
> +	unsigned len;
> +	int type;
> +};
> +
> +static int ftrace_match(char *str, struct ftrace_glob *g)
>  {
>  	int matched = 0;
>  	int slen;
>  
> -	switch (type) {
> +	switch (g->type) {
>  	case MATCH_FULL:
> -		if (strcmp(str, regex) == 0)
> +		if (strcmp(str, g->search) == 0)
>  			matched = 1;
>  		break;
>  	case MATCH_FRONT_ONLY:
> -		if (strncmp(str, regex, len) == 0)
> +		if (strncmp(str, g->search, g->len) == 0)
>  			matched = 1;
>  		break;
>  	case MATCH_MIDDLE_ONLY:
> -		if (strstr(str, regex))
> +		if (strstr(str, g->search))
>  			matched = 1;
>  		break;
>  	case MATCH_END_ONLY:
>  		slen = strlen(str);
> -		if (slen >= len && memcmp(str + slen - len, regex, len) == 0)
> +		if (slen >= g->len && memcmp(str + slen - g->len,
> +					g->search, g->len) == 0)

I'm going to modify this a little to:

		if (slen >= g->len &&
		    memcmp(str + slen - g->len, g->search, g->len) == 0)

As it's a bit easier to read.

>  			matched = 1;
>  		break;
>  	}
> @@ -3472,8 +3480,8 @@ enter_record(struct ftrace_hash *hash, struct dyn_ftrace *rec, int clear_filter)
>  }
>  
>  static int
> -ftrace_match_record(struct dyn_ftrace *rec, char *mod,
> -		    char *regex, int len, int type)
> +ftrace_match_record(struct dyn_ftrace *rec,
> +		char *mod, struct ftrace_glob *func_g)
>  {
>  	char str[KSYM_SYMBOL_LEN];
>  	char *modname;
> @@ -3486,28 +3494,27 @@ ftrace_match_record(struct dyn_ftrace *rec, char *mod,
>  			return 0;
>  
>  		/* blank search means to match all funcs in the mod */
> -		if (!len)
> +		if (!func_g->len)
>  			return 1;
>  	}
>  
> -	return ftrace_match(str, regex, len, type);
> +	return ftrace_match(str, func_g);
>  }
>  
>  static int
> -match_records(struct ftrace_hash *hash, char *buff, int len, char *mod)
> +match_records(struct ftrace_hash *hash, char *func, int len, char *mod)
>  {
> -	unsigned search_len = 0;
>  	struct ftrace_page *pg;
>  	struct dyn_ftrace *rec;
> -	int type = MATCH_FULL;
> -	char *search = buff;
> +	struct ftrace_glob func_g = { .type = MATCH_FULL };
>  	int found = 0;
>  	int ret;
>  	int clear_filter;
>  
>  	if (len) {
> -		type = filter_parse_regex(buff, len, &search, &clear_filter);
> -		search_len = strlen(search);
> +		func_g.type = filter_parse_regex(func, len,
> +				&func_g.search, &clear_filter);

I'm reformatting this too.

> +		func_g.len = strlen(func_g.search);
>  	}
>  
>  	mutex_lock(&ftrace_lock);
> @@ -3516,7 +3523,7 @@ match_records(struct ftrace_hash *hash, char *buff, int len, char *mod)
>  		goto out_unlock;
>  
>  	do_for_each_ftrace_rec(pg, rec) {
> -		if (ftrace_match_record(rec, mod, search, search_len, type)) {
> +		if (ftrace_match_record(rec, mod, &func_g)) {
>  			ret = enter_record(hash, rec, clear_filter);
>  			if (ret < 0) {
>  				found = ret;
> @@ -3682,14 +3689,15 @@ register_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
>  	struct ftrace_hash *hash;
>  	struct ftrace_page *pg;
>  	struct dyn_ftrace *rec;
> -	int type, len, not;
> +	int not;
>  	unsigned long key;
>  	int count = 0;
> -	char *search;
>  	int ret;
> +	struct ftrace_glob func_g;

I'm moving this up to keep the "upside down x-mas tree" effect.

>  
> -	type = filter_parse_regex(glob, strlen(glob), &search, &not);
> -	len = strlen(search);
> +	func_g.type = filter_parse_regex(glob, strlen(glob),
> +			&func_g.search, &not);
> +	func_g.len = strlen(func_g.search);
>  
>  	/* we do not support '!' for function probes */
>  	if (WARN_ON(not))
> @@ -3716,7 +3724,7 @@ register_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
>  
>  	do_for_each_ftrace_rec(pg, rec) {
>  
> -		if (!ftrace_match_record(rec, NULL, search, len, type))
> +		if (!ftrace_match_record(rec, NULL, &func_g))
>  			continue;
>  
>  		entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> @@ -3795,18 +3803,18 @@ __unregister_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
>  	struct ftrace_hash *hash;
>  	struct hlist_node *tmp;
>  	char str[KSYM_SYMBOL_LEN];
> -	int type = MATCH_FULL;
> -	int i, len = 0;
> -	char *search;
> -	int ret;
> +	int i, ret;
> +	struct ftrace_glob func_g;

Same here.

>  
>  	if (glob && (strcmp(glob, "*") == 0 || !strlen(glob)))
> -		glob = NULL;
> +		func_g.search = NULL;
>  	else if (glob) {
>  		int not;
>  
> -		type = filter_parse_regex(glob, strlen(glob), &search, &not);
> -		len = strlen(search);
> +		func_g.type = filter_parse_regex(glob, strlen(glob),
> +				&func_g.search, &not);

And this I'm formatting a bit differently, making the &func_g.search
equal to "glob".


> +		func_g.len = strlen(func_g.search);
> +		func_g.search = glob;
>  
>  		/* we do not support '!' for function probes */
>  		if (WARN_ON(not))
> @@ -3835,10 +3843,10 @@ __unregister_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
>  				continue;
>  
>  			/* do this last, since it is the most expensive */
> -			if (glob) {
> +			if (func_g.search) {
>  				kallsyms_lookup(entry->ip, NULL, NULL,
>  						NULL, str);
> -				if (!ftrace_match(str, glob, len, type))
> +				if (!ftrace_match(str, &func_g))
>  					continue;
>  			}
>  
> @@ -3867,7 +3875,7 @@ __unregister_ftrace_function_probe(char *glob, struct ftrace_probe_ops *ops,
>  		ftrace_free_entry(entry);
>  	}
>  	mutex_unlock(&ftrace_lock);
> -		
> +
>   out_unlock:
>  	mutex_unlock(&trace_probe_ops.func_hash->regex_lock);
>  	free_ftrace_hash(hash);
> @@ -4585,19 +4593,19 @@ ftrace_set_func(unsigned long *array, int *idx, int size, char *buffer)
>  {
>  	struct dyn_ftrace *rec;
>  	struct ftrace_page *pg;
> -	int search_len;
>  	int fail = 1;
> -	int type, not;
> -	char *search;
> +	int not;
>  	bool exists;
>  	int i;
> +	struct ftrace_glob func_g;

Again, I'm moving that up.

>  
>  	/* decode regex */
> -	type = filter_parse_regex(buffer, strlen(buffer), &search, &not);
> +	func_g.type = filter_parse_regex(buffer, strlen(buffer),
> +			&func_g.search, &not);

Format change.

OK, patch looks good, but I'm going to do the formatting changes. I'm
not changing the code itself, except for the few places I moved the
declaration of func_g up to keep the declarations in
upside-down-x-mass-tree format.

Thanks,

-- Steve


>  	if (!not && *idx >= size)
>  		return -EBUSY;
>  
> -	search_len = strlen(search);
> +	func_g.len = strlen(func_g.search);
>  
>  	mutex_lock(&ftrace_lock);
>  
> @@ -4608,7 +4616,7 @@ ftrace_set_func(unsigned long *array, int *idx, int size, char *buffer)
>  
>  	do_for_each_ftrace_rec(pg, rec) {
>  
> -		if (ftrace_match_record(rec, NULL, search, search_len, type)) {
> +		if (ftrace_match_record(rec, NULL, &func_g)) {
>  			/* if it is in the array */
>  			exists = false;
>  			for (i = 0; i < *idx; i++) {

Re: [PATCH 3/5] ftrace: introduce ftrace_glob structure

[Dmitry Safonov]

> Format change.
>
> OK, patch looks good, but I'm going to do the formatting changes. I'm
> not changing the code itself, except for the few places I moved the
> declaration of func_g up to keep the declarations in
> upside-down-x-mass-tree format.

Thank you! Seems better, I think.