[PATCH 0/13] tracing: various cleanups and small fixes

[PATCH 0/13] tracing: various cleanups and small fixes

[Li Zefan]

Here are some patches that I've collected. All are cleanups and
small fixes, no change in functionality.

---
 include/linux/ftrace_event.h       |    3 +-
 include/linux/syscalls.h           |    2 -
 include/trace/ftrace.h             |   55 ++++--------
 kernel/trace/ftrace.c              |   30 ++++---
 kernel/trace/power-traces.c        |    2 -
 kernel/trace/trace.c               |  166 +++++++++++++-----------------------
 kernel/trace/trace.h               |   23 +++---
 kernel/trace/trace_event_profile.c |    6 +-
 kernel/trace/trace_events.c        |   24 ++++--
 kernel/trace/trace_export.c        |    4 -
 kernel/trace/trace_kprobe.c        |    9 --
 kernel/trace/trace_ksym.c          |   13 ++--
 kernel/trace/trace_syscalls.c      |   18 +----
 13 files changed, 138 insertions(+), 217 deletions(-)

[PATCH 01/13] tracing: Extract duplicate ftrace_raw_init_event_foo()

[Li Zefan]

Define ftrace_raw_init_event_foo() for each event class rather
than for each event:

   text    data     bss     dec     hex filename
5459553 2005772 7103796 14569121         de4ea1 vmlinux.o.old
5456157 2005772 7103796 14565725         de415d vmlinux.o

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 include/trace/ftrace.h |   34 +++++++++++++++-------------------
 1 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index d1b3de9..4d114c9 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -647,9 +647,6 @@ static void ftrace_profile_disable_##name(struct ftrace_event_call *unused)\
  *
  */
 
-#undef TP_FMT
-#define TP_FMT(fmt, args...)	fmt "\n", ##args
-
 #ifdef CONFIG_EVENT_PROFILE
 
 #define _TRACE_PROFILE_INIT(call)					\
@@ -716,6 +713,18 @@ static void ftrace_raw_event_id_##call(struct ftrace_event_call *event_call, \
 	if (!filter_current_check_discard(buffer, event_call, entry, event)) \
 		trace_nowake_buffer_unlock_commit(buffer,		\
 						  event, irq_flags, pc); \
+}									\
+									\
+static int ftrace_raw_init_event_##call(struct ftrace_event_call *event_call)\
+{									\
+	int id;								\
+									\
+	id = register_ftrace_event(event_call->event);			\
+	if (!id)							\
+		return -ENODEV;						\
+	event_call->id = id;						\
+	INIT_LIST_HEAD(&event_call->fields);				\
+	return 0;							\
 }
 
 #undef DEFINE_EVENT
@@ -744,19 +753,7 @@ static void ftrace_raw_unreg_event_##call(struct ftrace_event_call *unused)\
 									\
 static struct trace_event ftrace_event_type_##call = {			\
 	.trace			= ftrace_raw_output_##call,		\
-};									\
-									\
-static int ftrace_raw_init_event_##call(struct ftrace_event_call *unused)\
-{									\
-	int id;								\
-									\
-	id = register_ftrace_event(&ftrace_event_type_##call);		\
-	if (!id)							\
-		return -ENODEV;						\
-	event_##call.id = id;						\
-	INIT_LIST_HEAD(&event_##call.fields);				\
-	return 0;							\
-}
+};
 
 #undef DEFINE_EVENT_PRINT
 #define DEFINE_EVENT_PRINT(template, name, proto, args, print)	\
@@ -776,7 +773,7 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.name			= #call,				\
 	.system			= __stringify(TRACE_SYSTEM),		\
 	.event			= &ftrace_event_type_##call,		\
-	.raw_init		= ftrace_raw_init_event_##call,		\
+	.raw_init		= ftrace_raw_init_event_##template,	\
 	.regfunc		= ftrace_raw_reg_event_##call,		\
 	.unregfunc		= ftrace_raw_unreg_event_##call,	\
 	.show_format		= ftrace_format_##template,		\
@@ -793,7 +790,7 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.name			= #call,				\
 	.system			= __stringify(TRACE_SYSTEM),		\
 	.event			= &ftrace_event_type_##call,		\
-	.raw_init		= ftrace_raw_init_event_##call,		\
+	.raw_init		= ftrace_raw_init_event_##template,	\
 	.regfunc		= ftrace_raw_reg_event_##call,		\
 	.unregfunc		= ftrace_raw_unreg_event_##call,	\
 	.show_format		= ftrace_format_##call,			\
@@ -953,7 +950,6 @@ end:									\
 	perf_swevent_put_recursion_context(rctx);			\
 end_recursion:								\
 	local_irq_restore(irq_flags);					\
-									\
 }
 
 #undef DEFINE_EVENT
-- 
1.6.3

[PATCH 02/13] tracing: Extract calls to trace_define_common_fields()

[Li Zefan]

Call trace_define_common_fields() in event_create_dir() only.

   text    data     bss     dec     hex filename
5456157 2005772 7103796 14565725         de415d vmlinux.o
5454538 2005772 7103796 14564106         de3b0a vmlinux.o

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 include/linux/ftrace_event.h  |    1 -
 include/trace/ftrace.h        |    4 ----
 kernel/trace/trace_events.c   |    7 ++++---
 kernel/trace/trace_export.c   |    4 ----
 kernel/trace/trace_kprobe.c   |    8 --------
 kernel/trace/trace_syscalls.c |    8 --------
 6 files changed, 4 insertions(+), 28 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 47bbdf9..02de70f 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -157,7 +157,6 @@ enum {
 	FILTER_PTR_STRING,
 };
 
-extern int trace_define_common_fields(struct ftrace_event_call *call);
 extern int trace_define_field(struct ftrace_event_call *call, const char *type,
 			      const char *name, int offset, int size,
 			      int is_signed, int filter_type);
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 4d114c9..84d6f23 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -436,10 +436,6 @@ ftrace_define_fields_##call(struct ftrace_event_call *event_call)	\
 	struct ftrace_raw_##call field;					\
 	int ret;							\
 									\
-	ret = trace_define_common_fields(event_call);			\
-	if (ret)							\
-		return ret;						\
-									\
 	tstruct;							\
 									\
 	return ret;							\
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 1d18315..9fa6736 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -78,7 +78,7 @@ EXPORT_SYMBOL_GPL(trace_define_field);
 	if (ret)							\
 		return ret;
 
-int trace_define_common_fields(struct ftrace_event_call *call)
+static int trace_define_common_fields(struct ftrace_event_call *call)
 {
 	int ret;
 	struct trace_entry ent;
@@ -91,7 +91,6 @@ int trace_define_common_fields(struct ftrace_event_call *call)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(trace_define_common_fields);
 
 void trace_destroy_fields(struct ftrace_event_call *call)
 {
@@ -913,7 +912,9 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
 		 		  id);
 
 	if (call->define_fields) {
-		ret = call->define_fields(call);
+		ret = trace_define_common_fields(call);
+		if (!ret)
+			ret = call->define_fields(call);
 		if (ret < 0) {
 			pr_warning("Could not initialize trace point"
 				   " events/%s\n", call->name);
diff --git a/kernel/trace/trace_export.c b/kernel/trace/trace_export.c
index dff8c84..458e5bf 100644
--- a/kernel/trace/trace_export.c
+++ b/kernel/trace/trace_export.c
@@ -184,10 +184,6 @@ ftrace_define_fields_##name(struct ftrace_event_call *event_call)	\
 	struct struct_name field;					\
 	int ret;							\
 									\
-	ret = trace_define_common_fields(event_call);			\
-	if (ret)							\
-		return ret;						\
-									\
 	tstruct;							\
 									\
 	return ret;							\
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index aff5f80..e3c80e9 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1113,10 +1113,6 @@ static int kprobe_event_define_fields(struct ftrace_event_call *event_call)
 	struct kprobe_trace_entry field;
 	struct trace_probe *tp = (struct trace_probe *)event_call->data;
 
-	ret = trace_define_common_fields(event_call);
-	if (!ret)
-		return ret;
-
 	DEFINE_FIELD(unsigned long, ip, FIELD_STRING_IP, 0);
 	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
 	/* Set argument names as fields */
@@ -1131,10 +1127,6 @@ static int kretprobe_event_define_fields(struct ftrace_event_call *event_call)
 	struct kretprobe_trace_entry field;
 	struct trace_probe *tp = (struct trace_probe *)event_call->data;
 
-	ret = trace_define_common_fields(event_call);
-	if (!ret)
-		return ret;
-
 	DEFINE_FIELD(unsigned long, func, FIELD_STRING_FUNC, 0);
 	DEFINE_FIELD(unsigned long, ret_ip, FIELD_STRING_RETIP, 0);
 	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 57501d9..b957edd 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -217,10 +217,6 @@ int syscall_enter_define_fields(struct ftrace_event_call *call)
 	int i;
 	int offset = offsetof(typeof(trace), args);
 
-	ret = trace_define_common_fields(call);
-	if (ret)
-		return ret;
-
 	ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER);
 	if (ret)
 		return ret;
@@ -241,10 +237,6 @@ int syscall_exit_define_fields(struct ftrace_event_call *call)
 	struct syscall_trace_exit trace;
 	int ret;
 
-	ret = trace_define_common_fields(call);
-	if (ret)
-		return ret;
-
 	ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER);
 	if (ret)
 		return ret;
-- 
1.6.3

[PATCH 03/13] tracing: Move a printk out of ftrace_raw_reg_event_foo()

[Li Zefan]

Move the printk from each ftrace_raw_reg_event_foo() to
its caller ftrace_event_enable_disable().

See how much space this saves:

   text    data     bss     dec     hex filename
5454538 2005772 7103796 14564106         de3b0a vmlinux.o
5440766 2005772 7103796 14550334         de053e vmlinux.o

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 include/trace/ftrace.h        |   16 ++--------------
 kernel/trace/trace_events.c   |   17 +++++++++++++----
 kernel/trace/trace_syscalls.c |   10 ++--------
 3 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 84d6f23..4aac981 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -555,13 +555,7 @@ static void ftrace_profile_disable_##name(struct ftrace_event_call *unused)\
  *
  * static int ftrace_reg_event_<call>(struct ftrace_event_call *unused)
  * {
- *	int ret;
- *
- *	ret = register_trace_<call>(ftrace_event_<call>);
- *	if (!ret)
- *		pr_info("event trace: Could not activate trace point "
- *			"probe to  <call>");
- *	return ret;
+ *	return register_trace_<call>(ftrace_event_<call>);
  * }
  *
  * static void ftrace_unreg_event_<call>(struct ftrace_event_call *unused)
@@ -733,13 +727,7 @@ static void ftrace_raw_event_##call(proto)				\
 									\
 static int ftrace_raw_reg_event_##call(struct ftrace_event_call *unused)\
 {									\
-	int ret;							\
-									\
-	ret = register_trace_##call(ftrace_raw_event_##call);		\
-	if (ret)							\
-		pr_info("event trace: Could not activate trace point "	\
-			"probe to " #call "\n");			\
-	return ret;							\
+	return register_trace_##call(ftrace_raw_event_##call);		\
 }									\
 									\
 static void ftrace_raw_unreg_event_##call(struct ftrace_event_call *unused)\
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 9fa6736..f22eaec 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -104,9 +104,11 @@ void trace_destroy_fields(struct ftrace_event_call *call)
 	}
 }
 
-static void ftrace_event_enable_disable(struct ftrace_event_call *call,
+static int ftrace_event_enable_disable(struct ftrace_event_call *call,
 					int enable)
 {
+	int ret = 0;
+
 	switch (enable) {
 	case 0:
 		if (call->enabled) {
@@ -117,12 +119,19 @@ static void ftrace_event_enable_disable(struct ftrace_event_call *call,
 		break;
 	case 1:
 		if (!call->enabled) {
+			if (ret) {
+				pr_info("event trace: Could not enable event "
+					"%s\n", call->name);
+				break;
+			}
 			call->enabled = 1;
 			tracing_start_cmdline_record();
-			call->regfunc(call);
+			ret = call->regfunc(call);
 		}
 		break;
 	}
+
+	return ret;
 }
 
 static void ftrace_clear_events(void)
@@ -401,7 +410,7 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	case 0:
 	case 1:
 		mutex_lock(&event_mutex);
-		ftrace_event_enable_disable(call, val);
+		ret = ftrace_event_enable_disable(call, val);
 		mutex_unlock(&event_mutex);
 		break;
 
@@ -411,7 +420,7 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt,
 
 	*ppos += cnt;
 
-	return cnt;
+	return ret ? ret : cnt;
 }
 
 static ssize_t
diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index b957edd..75289f3 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -325,10 +325,7 @@ int reg_event_syscall_enter(struct ftrace_event_call *call)
 	mutex_lock(&syscall_trace_lock);
 	if (!sys_refcount_enter)
 		ret = register_trace_sys_enter(ftrace_syscall_enter);
-	if (ret) {
-		pr_info("event trace: Could not activate"
-				"syscall entry trace point");
-	} else {
+	if (!ret) {
 		set_bit(num, enabled_enter_syscalls);
 		sys_refcount_enter++;
 	}
@@ -362,10 +359,7 @@ int reg_event_syscall_exit(struct ftrace_event_call *call)
 	mutex_lock(&syscall_trace_lock);
 	if (!sys_refcount_exit)
 		ret = register_trace_sys_exit(ftrace_syscall_exit);
-	if (ret) {
-		pr_info("event trace: Could not activate"
-				"syscall exit trace point");
-	} else {
+	if (!ret) {
 		set_bit(num, enabled_exit_syscalls);
 		sys_refcount_exit++;
 	}
-- 
1.6.3

[PATCH 04/13] ftrace: Return EINVAL when writing invalid val to set_ftrace_filter

[Li Zefan]

Currently it doesn't warn user on invald value:

 # echo nonexist_symbol > set_ftrace_filter
or:
 # echo 'nonexist_symbol:mod:fuse' > set_ftrace_filter

Better make it return failure.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/trace/ftrace.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index e51a1bc..08a3fb5 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1724,7 +1724,7 @@ ftrace_match_record(struct dyn_ftrace *rec, char *regex, int len, int type)
 	return ftrace_match(str, regex, len, type);
 }
 
-static void ftrace_match_records(char *buff, int len, int enable)
+static int ftrace_match_records(char *buff, int len, int enable)
 {
 	unsigned int search_len;
 	struct ftrace_page *pg;
@@ -1733,6 +1733,7 @@ static void ftrace_match_records(char *buff, int len, int enable)
 	char *search;
 	int type;
 	int not;
+	int found = 0;
 
 	flag = enable ? FTRACE_FL_FILTER : FTRACE_FL_NOTRACE;
 	type = filter_parse_regex(buff, len, &search, &not);
@@ -1750,6 +1751,7 @@ static void ftrace_match_records(char *buff, int len, int enable)
 				rec->flags &= ~flag;
 			else
 				rec->flags |= flag;
+			found = 1;
 		}
 		/*
 		 * Only enable filtering if we have a function that
@@ -1759,6 +1761,8 @@ static void ftrace_match_records(char *buff, int len, int enable)
 			ftrace_filtered = 1;
 	} while_for_each_ftrace_rec();
 	mutex_unlock(&ftrace_lock);
+
+	return found;
 }
 
 static int
@@ -1780,7 +1784,7 @@ ftrace_match_module_record(struct dyn_ftrace *rec, char *mod,
 		return 1;
 }
 
-static void ftrace_match_module_records(char *buff, char *mod, int enable)
+static int ftrace_match_module_records(char *buff, char *mod, int enable)
 {
 	unsigned search_len = 0;
 	struct ftrace_page *pg;
@@ -1789,6 +1793,7 @@ static void ftrace_match_module_records(char *buff, char *mod, int enable)
 	char *search = buff;
 	unsigned long flag;
 	int not = 0;
+	int found = 0;
 
 	flag = enable ? FTRACE_FL_FILTER : FTRACE_FL_NOTRACE;
 
@@ -1819,12 +1824,15 @@ static void ftrace_match_module_records(char *buff, char *mod, int enable)
 				rec->flags &= ~flag;
 			else
 				rec->flags |= flag;
+			found = 1;
 		}
 		if (enable && (rec->flags & FTRACE_FL_FILTER))
 			ftrace_filtered = 1;
 
 	} while_for_each_ftrace_rec();
 	mutex_unlock(&ftrace_lock);
+
+	return found;
 }
 
 /*
@@ -1853,8 +1861,9 @@ ftrace_mod_callback(char *func, char *cmd, char *param, int enable)
 	if (!strlen(mod))
 		return -EINVAL;
 
-	ftrace_match_module_records(func, mod, enable);
-	return 0;
+	if (ftrace_match_module_records(func, mod, enable))
+		return 0;
+	return -EINVAL;
 }
 
 static struct ftrace_func_command ftrace_mod_cmd = {
@@ -2151,8 +2160,9 @@ static int ftrace_process_regex(char *buff, int len, int enable)
 	func = strsep(&next, ":");
 
 	if (!next) {
-		ftrace_match_records(func, len, enable);
-		return 0;
+		if (ftrace_match_records(func, len, enable))
+			return 0;
+		return ret;
 	}
 
 	/* command found */
-- 
1.6.3

[PATCH 05/13] ftrace: Call trace_parser_clear() properly

[Li Zefan]

I found a weird behavior:

  # echo 'fuse:*' > set_ftrace_filter
  bash: echo: write error: Invalid argument
  # cat set_ftrace_filter
  fuse_dev_fasync
  fuse_dev_poll
  fuse_copy_do

We should call trace_parser_clear() no matter ftrace_process_regex()
returns 0 or -errno.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/ftrace.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 08a3fb5..ff8aecd 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2208,10 +2208,9 @@ ftrace_regex_write(struct file *file, const char __user *ubuf,
 	    !trace_parser_cont(parser)) {
 		ret = ftrace_process_regex(parser->buffer,
 					   parser->idx, enable);
+		trace_parser_clear(parser);
 		if (ret)
 			goto out_unlock;
-
-		trace_parser_clear(parser);
 	}
 
 	ret = read;
-- 
1.6.3

[PATCH 06/13] function-graph: Allow writing the same val to set_graph_function

[Li Zefan]

 # echo 'do_open' > set_graph_function
 # echo 'do_open' >> set_graph_function
 bash: echo: write error: Invalid argument

Make it valid to write the same value to set_graph_function,
which is consistent with set_ftrace_filter interface.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/ftrace.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index ff8aecd..7968762 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2552,10 +2552,9 @@ ftrace_set_func(unsigned long *array, int *idx, char *buffer)
 					exists = true;
 					break;
 				}
-			if (!exists) {
+			if (!exists)
 				array[(*idx)++] = rec->ip;
-				found = 1;
-			}
+			found = 1;
 		}
 	} while_for_each_ftrace_rec();
 
-- 
1.6.3

[PATCH 07/13] tracing: Use seq file for trace_options

[Li Zefan]

Code simplification for reading trace_options.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/trace.c |   60 ++++++++++++++-----------------------------------
 1 files changed, 17 insertions(+), 43 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 874f289..e989fc9 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2291,67 +2291,32 @@ static const struct file_operations tracing_cpumask_fops = {
 	.write		= tracing_cpumask_write,
 };
 
-static ssize_t
-tracing_trace_options_read(struct file *filp, char __user *ubuf,
-		       size_t cnt, loff_t *ppos)
+static int tracing_trace_options_show(struct seq_file *m, void *v)
 {
 	struct tracer_opt *trace_opts;
 	u32 tracer_flags;
-	int len = 0;
-	char *buf;
-	int r = 0;
 	int i;
 
-
-	/* calculate max size */
-	for (i = 0; trace_options[i]; i++) {
-		len += strlen(trace_options[i]);
-		len += 3; /* "no" and newline */
-	}
-
 	mutex_lock(&trace_types_lock);
 	tracer_flags = current_trace->flags->val;
 	trace_opts = current_trace->flags->opts;
 
-	/*
-	 * Increase the size with names of options specific
-	 * of the current tracer.
-	 */
-	for (i = 0; trace_opts[i].name; i++) {
-		len += strlen(trace_opts[i].name);
-		len += 3; /* "no" and newline */
-	}
-
-	/* +1 for \0 */
-	buf = kmalloc(len + 1, GFP_KERNEL);
-	if (!buf) {
-		mutex_unlock(&trace_types_lock);
-		return -ENOMEM;
-	}
-
 	for (i = 0; trace_options[i]; i++) {
 		if (trace_flags & (1 << i))
-			r += sprintf(buf + r, "%s\n", trace_options[i]);
+			seq_printf(m, "%s\n", trace_options[i]);
 		else
-			r += sprintf(buf + r, "no%s\n", trace_options[i]);
+			seq_printf(m, "no%s\n", trace_options[i]);
 	}
 
 	for (i = 0; trace_opts[i].name; i++) {
 		if (tracer_flags & trace_opts[i].bit)
-			r += sprintf(buf + r, "%s\n",
-				trace_opts[i].name);
+			seq_printf(m, "%s\n", trace_opts[i].name);
 		else
-			r += sprintf(buf + r, "no%s\n",
-				trace_opts[i].name);
+			seq_printf(m, "no%s\n", trace_opts[i].name);
 	}
 	mutex_unlock(&trace_types_lock);
 
-	WARN_ON(r >= len + 1);
-
-	r = simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
-
-	kfree(buf);
-	return r;
+	return 0;
 }
 
 /* Try to assign a tracer specific option */
@@ -2446,9 +2411,18 @@ tracing_trace_options_write(struct file *filp, const char __user *ubuf,
 	return cnt;
 }
 
+static int tracing_trace_options_open(struct inode *inode, struct file *file)
+{
+	if (tracing_disabled)
+		return -ENODEV;
+	return single_open(file, tracing_trace_options_show, NULL);
+}
+
 static const struct file_operations tracing_iter_fops = {
-	.open		= tracing_open_generic,
-	.read		= tracing_trace_options_read,
+	.open		= tracing_trace_options_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
 	.write		= tracing_trace_options_write,
 };
 
-- 
1.6.3

[PATCH 08/13] tracing: Use seq file for trace_clock

[Li Zefan]

The buffer for the output is as small as 64 bytes, so it'll
overflow if we add more clock types. Use seq file instead.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/trace.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index e989fc9..75871e5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3327,21 +3327,18 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
 	return cnt;
 }
 
-static ssize_t tracing_clock_read(struct file *filp, char __user *ubuf,
-				  size_t cnt, loff_t *ppos)
+static int tracing_clock_show(struct seq_file *m, void *v)
 {
-	char buf[64];
-	int bufiter = 0;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(trace_clocks); i++)
-		bufiter += snprintf(buf + bufiter, sizeof(buf) - bufiter,
+		seq_printf(m,
 			"%s%s%s%s", i ? " " : "",
 			i == trace_clock_id ? "[" : "", trace_clocks[i].name,
 			i == trace_clock_id ? "]" : "");
-	bufiter += snprintf(buf + bufiter, sizeof(buf) - bufiter, "\n");
+	seq_putc(m, '\n');
 
-	return simple_read_from_buffer(ubuf, cnt, ppos, buf, bufiter);
+	return 0;
 }
 
 static ssize_t tracing_clock_write(struct file *filp, const char __user *ubuf,
@@ -3383,6 +3380,13 @@ static ssize_t tracing_clock_write(struct file *filp, const char __user *ubuf,
 	return cnt;
 }
 
+static int tracing_clock_open(struct inode *inode, struct file *file)
+{
+	if (tracing_disabled)
+		return -ENODEV;
+	return single_open(file, tracing_clock_show, NULL);
+}
+
 static const struct file_operations tracing_max_lat_fops = {
 	.open		= tracing_open_generic,
 	.read		= tracing_max_lat_read,
@@ -3421,8 +3425,10 @@ static const struct file_operations tracing_mark_fops = {
 };
 
 static const struct file_operations trace_clock_fops = {
-	.open		= tracing_open_generic,
-	.read		= tracing_clock_read,
+	.open		= tracing_clock_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
 	.write		= tracing_clock_write,
 };
 
-- 
1.6.3

[PATCH 09/13] tracing: Remove useless trace option

[Li Zefan]

Since commit 4d9493c90f8e6e1b164aede3814010a290161abb
("ftrace: remove add-hoc code"), option "sched-tree"
has become useless.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/trace.c |    1 -
 kernel/trace/trace.h |   23 +++++++++++------------
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 75871e5..2801b0b 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -313,7 +313,6 @@ static const char *trace_options[] = {
 	"bin",
 	"block",
 	"stacktrace",
-	"sched-tree",
 	"trace_printk",
 	"ftrace_preempt",
 	"branch",
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index ea5dda4..60f9471 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -597,18 +597,17 @@ enum trace_iterator_flags {
 	TRACE_ITER_BIN			= 0x40,
 	TRACE_ITER_BLOCK		= 0x80,
 	TRACE_ITER_STACKTRACE		= 0x100,
-	TRACE_ITER_SCHED_TREE		= 0x200,
-	TRACE_ITER_PRINTK		= 0x400,
-	TRACE_ITER_PREEMPTONLY		= 0x800,
-	TRACE_ITER_BRANCH		= 0x1000,
-	TRACE_ITER_ANNOTATE		= 0x2000,
-	TRACE_ITER_USERSTACKTRACE       = 0x4000,
-	TRACE_ITER_SYM_USEROBJ          = 0x8000,
-	TRACE_ITER_PRINTK_MSGONLY	= 0x10000,
-	TRACE_ITER_CONTEXT_INFO		= 0x20000, /* Print pid/cpu/time */
-	TRACE_ITER_LATENCY_FMT		= 0x40000,
-	TRACE_ITER_SLEEP_TIME		= 0x80000,
-	TRACE_ITER_GRAPH_TIME		= 0x100000,
+	TRACE_ITER_PRINTK		= 0x200,
+	TRACE_ITER_PREEMPTONLY		= 0x400,
+	TRACE_ITER_BRANCH		= 0x800,
+	TRACE_ITER_ANNOTATE		= 0x1000,
+	TRACE_ITER_USERSTACKTRACE       = 0x2000,
+	TRACE_ITER_SYM_USEROBJ          = 0x4000,
+	TRACE_ITER_PRINTK_MSGONLY	= 0x8000,
+	TRACE_ITER_CONTEXT_INFO		= 0x10000, /* Print pid/cpu/time */
+	TRACE_ITER_LATENCY_FMT		= 0x20000,
+	TRACE_ITER_SLEEP_TIME		= 0x40000,
+	TRACE_ITER_GRAPH_TIME		= 0x80000,
 };
 
 /*
-- 
1.6.3

[PATCH 10/13] tracing: Simplify trace_option_write()

[Li Zefan]

- remove duplicate code inside trace_options_write()
- extract duplicate code in trace_options_write() and set_tracer_option()

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/trace.c |   85 ++++++++++++++++++-------------------------------
 1 files changed, 31 insertions(+), 54 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 2801b0b..9359b74 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2318,38 +2318,39 @@ static int tracing_trace_options_show(struct seq_file *m, void *v)
 	return 0;
 }
 
+static int __set_tracer_option(struct tracer *trace,
+			       struct tracer_flags *tracer_flags,
+			       struct tracer_opt *opts, int neg)
+{
+	int ret;
+
+	ret = trace->set_flag(tracer_flags->val, opts->bit, !neg);
+	if (ret)
+		return ret;
+
+	if (neg)
+		tracer_flags->val &= ~opts->bit;
+	else
+		tracer_flags->val |= opts->bit;
+	return 0;
+}
+
 /* Try to assign a tracer specific option */
 static int set_tracer_option(struct tracer *trace, char *cmp, int neg)
 {
 	struct tracer_flags *tracer_flags = trace->flags;
 	struct tracer_opt *opts = NULL;
-	int ret = 0, i = 0;
-	int len;
+	int i;
 
 	for (i = 0; tracer_flags->opts[i].name; i++) {
 		opts = &tracer_flags->opts[i];
-		len = strlen(opts->name);
 
-		if (strncmp(cmp, opts->name, len) == 0) {
-			ret = trace->set_flag(tracer_flags->val,
-				opts->bit, !neg);
-			break;
-		}
+		if (strcmp(cmp, opts->name) == 0)
+			return __set_tracer_option(trace, trace->flags,
+						   opts, neg);
 	}
-	/* Not found */
-	if (!tracer_flags->opts[i].name)
-		return -EINVAL;
 
-	/* Refused to handle */
-	if (ret)
-		return ret;
-
-	if (neg)
-		tracer_flags->val &= ~opts->bit;
-	else
-		tracer_flags->val |= opts->bit;
-
-	return 0;
+	return -EINVAL;
 }
 
 static void set_tracer_flags(unsigned int mask, int enabled)
@@ -2369,7 +2370,7 @@ tracing_trace_options_write(struct file *filp, const char __user *ubuf,
 			size_t cnt, loff_t *ppos)
 {
 	char buf[64];
-	char *cmp = buf;
+	char *cmp;
 	int neg = 0;
 	int ret;
 	int i;
@@ -2381,16 +2382,15 @@ tracing_trace_options_write(struct file *filp, const char __user *ubuf,
 		return -EFAULT;
 
 	buf[cnt] = 0;
+	cmp = strstrip(buf);
 
-	if (strncmp(buf, "no", 2) == 0) {
+	if (strncmp(cmp, "no", 2) == 0) {
 		neg = 1;
 		cmp += 2;
 	}
 
 	for (i = 0; trace_options[i]; i++) {
-		int len = strlen(trace_options[i]);
-
-		if (strncmp(cmp, trace_options[i], len) == 0) {
+		if (strcmp(cmp, trace_options[i]) == 0) {
 			set_tracer_flags(1 << i, !neg);
 			break;
 		}
@@ -3888,39 +3888,16 @@ trace_options_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	if (ret < 0)
 		return ret;
 
-	ret = 0;
-	switch (val) {
-	case 0:
-		/* do nothing if already cleared */
-		if (!(topt->flags->val & topt->opt->bit))
-			break;
-
-		mutex_lock(&trace_types_lock);
-		if (current_trace->set_flag)
-			ret = current_trace->set_flag(topt->flags->val,
-						      topt->opt->bit, 0);
-		mutex_unlock(&trace_types_lock);
-		if (ret)
-			return ret;
-		topt->flags->val &= ~topt->opt->bit;
-		break;
-	case 1:
-		/* do nothing if already set */
-		if (topt->flags->val & topt->opt->bit)
-			break;
+	if (val != 0 && val != 1)
+		return -EINVAL;
 
+	if (!!(topt->flags->val & topt->opt->bit) != val) {
 		mutex_lock(&trace_types_lock);
-		if (current_trace->set_flag)
-			ret = current_trace->set_flag(topt->flags->val,
-						      topt->opt->bit, 1);
+		ret = __set_tracer_option(current_trace, topt->flags,
+					  topt->opt, val);
 		mutex_unlock(&trace_types_lock);
 		if (ret)
 			return ret;
-		topt->flags->val |= topt->opt->bit;
-		break;
-
-	default:
-		return -EINVAL;
 	}
 
 	*ppos += cnt;
-- 
1.6.3

[PATCH 11/13] tracing: Change event->profile_count to be int type

[Li Zefan]

Like total_profile_count, struct ftrace_event_call::profile_count
is protected by event_mutex, so it doesn't need to be atomic_t.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 include/linux/ftrace_event.h       |    2 +-
 include/linux/syscalls.h           |    2 --
 include/trace/ftrace.h             |    1 -
 kernel/trace/trace_event_profile.c |    6 +++---
 kernel/trace/trace_kprobe.c        |    1 -
 5 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 02de70f..22a04d9 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -130,7 +130,7 @@ struct ftrace_event_call {
 	void			*mod;
 	void			*data;
 
-	atomic_t		profile_count;
+	int			profile_count;
 	int			(*profile_enable)(struct ftrace_event_call *);
 	void			(*profile_disable)(struct ftrace_event_call *);
 };
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e79e2f3..d3ecfb9 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -101,12 +101,10 @@ struct perf_event_attr;
 #ifdef CONFIG_EVENT_PROFILE
 
 #define TRACE_SYS_ENTER_PROFILE_INIT(sname)				       \
-	.profile_count = ATOMIC_INIT(-1),				       \
 	.profile_enable = prof_sysenter_enable,				       \
 	.profile_disable = prof_sysenter_disable,
 
 #define TRACE_SYS_EXIT_PROFILE_INIT(sname)				       \
-	.profile_count = ATOMIC_INIT(-1),				       \
 	.profile_enable = prof_sysexit_enable,				       \
 	.profile_disable = prof_sysexit_disable,
 #else
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 4aac981..51f045b 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -640,7 +640,6 @@ static void ftrace_profile_disable_##name(struct ftrace_event_call *unused)\
 #ifdef CONFIG_EVENT_PROFILE
 
 #define _TRACE_PROFILE_INIT(call)					\
-	.profile_count = ATOMIC_INIT(-1),				\
 	.profile_enable = ftrace_profile_enable_##call,			\
 	.profile_disable = ftrace_profile_disable_##call,
 
diff --git a/kernel/trace/trace_event_profile.c b/kernel/trace/trace_event_profile.c
index d9c60f8..9e25573 100644
--- a/kernel/trace/trace_event_profile.c
+++ b/kernel/trace/trace_event_profile.c
@@ -25,7 +25,7 @@ static int ftrace_profile_enable_event(struct ftrace_event_call *event)
 	char *buf;
 	int ret = -ENOMEM;
 
-	if (atomic_inc_return(&event->profile_count))
+	if (event->profile_count++ > 0)
 		return 0;
 
 	if (!total_profile_count) {
@@ -56,7 +56,7 @@ fail_buf_nmi:
 		perf_trace_buf = NULL;
 	}
 fail_buf:
-	atomic_dec(&event->profile_count);
+	event->profile_count--;
 
 	return ret;
 }
@@ -83,7 +83,7 @@ static void ftrace_profile_disable_event(struct ftrace_event_call *event)
 {
 	char *buf, *nmi_buf;
 
-	if (!atomic_add_negative(-1, &event->profile_count))
+	if (--event->profile_count > 0)
 		return;
 
 	event->profile_disable(event);
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index e3c80e9..6ed2234 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1426,7 +1426,6 @@ static int register_probe_event(struct trace_probe *tp)
 	call->unregfunc = probe_event_disable;
 
 #ifdef CONFIG_EVENT_PROFILE
-	atomic_set(&call->profile_count, -1);
 	call->profile_enable = probe_profile_enable;
 	call->profile_disable = probe_profile_disable;
 #endif
-- 
1.6.3

[PATCH 12/13] tracing/power: Remove two exports

[Li Zefan]

trace_power_start and trace_power_end are only used in
arch/x86/kernel/power.c, and this file can't be compiled
as a module, so these two tracepoints needn't to be exported.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/power-traces.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/power-traces.c b/kernel/trace/power-traces.c
index e06c6e3..9f4f565 100644
--- a/kernel/trace/power-traces.c
+++ b/kernel/trace/power-traces.c
@@ -14,7 +14,5 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/power.h>
 
-EXPORT_TRACEPOINT_SYMBOL_GPL(power_start);
-EXPORT_TRACEPOINT_SYMBOL_GPL(power_end);
 EXPORT_TRACEPOINT_SYMBOL_GPL(power_frequency);
 
-- 
1.6.3

[PATCH 13/13] ksym_tracer: Fix compile warnings

[Li Zefan]

Fix these 2 warnings:

kernel/trace/trace_ksym.c: In function 'ksym_trace_filter_read':
kernel/trace/trace_ksym.c:239: warning: cast to pointer from integer of different size
kernel/trace/trace_ksym.c: In function 'ksym_trace_filter_write':
kernel/trace/trace_ksym.c:295: warning: ignoring return value of 'strstrip', declared with attribute warn_unused_result

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/trace_ksym.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace_ksym.c b/kernel/trace/trace_ksym.c
index acb87d4..e393146 100644
--- a/kernel/trace/trace_ksym.c
+++ b/kernel/trace/trace_ksym.c
@@ -236,7 +236,8 @@ static ssize_t ksym_trace_filter_read(struct file *filp, char __user *ubuf,
 	mutex_lock(&ksym_tracer_mutex);
 
 	hlist_for_each_entry(entry, node, &ksym_filter_head, ksym_hlist) {
-		ret = trace_seq_printf(s, "%pS:", (void *)entry->attr.bp_addr);
+		ret = trace_seq_printf(s, "%pS:",
+				(void *)(unsigned long)entry->attr.bp_addr);
 		if (entry->attr.bp_type == HW_BREAKPOINT_R)
 			ret = trace_seq_puts(s, "r--\n");
 		else if (entry->attr.bp_type == HW_BREAKPOINT_W)
@@ -278,7 +279,7 @@ static ssize_t ksym_trace_filter_write(struct file *file,
 {
 	struct trace_ksym *entry;
 	struct hlist_node *node;
-	char *input_string, *ksymname = NULL;
+	char *input_string, *buf, *ksymname = NULL;
 	unsigned long ksym_addr = 0;
 	int ret, op, changed = 0;
 
@@ -292,7 +293,7 @@ static ssize_t ksym_trace_filter_write(struct file *file,
 	}
 	input_string[count] = '\0';
 
-	strstrip(input_string);
+	buf = strstrip(input_string);
 
 	/*
 	 * Clear all breakpoints if:
@@ -300,14 +301,14 @@ static ssize_t ksym_trace_filter_write(struct file *file,
 	 * 2: echo 0 > ksym_trace_filter
 	 * 3: echo "*:---" > ksym_trace_filter
 	 */
-	if (!input_string[0] || !strcmp(input_string, "0") ||
-	    !strcmp(input_string, "*:---")) {
+	if (!buf[0] || !strcmp(buf, "0") ||
+	    !strcmp(buf, "*:---")) {
 		__ksym_trace_reset();
 		kfree(input_string);
 		return count;
 	}
 
-	ret = op = parse_ksym_trace_str(input_string, &ksymname, &ksym_addr);
+	ret = op = parse_ksym_trace_str(buf, &ksymname, &ksym_addr);
 	if (ret < 0) {
 		kfree(input_string);
 		return ret;
-- 
1.6.3

Re: [PATCH 01/13] tracing: Extract duplicate ftrace_raw_init_event_foo()

[Frederic Weisbecker]

2009/12/7 Li Zefan <lizf@cn.fujitsu.com>:
> +static int ftrace_raw_init_event_##call(struct ftrace_event_call *event_call)\> +{                                                                      \> +       int id;                                                         \> +                                                                       \> +       id = register_ftrace_event(event_call->event);                  \> +       if (!id)                                                        \> +               return -ENODEV;                                         \> +       event_call->id = id;                                            \> +       INIT_LIST_HEAD(&event_call->fields);                            \> +       return 0;                                                       \>  }

This function doesn't vary anymore in this form.May be can we define a generic one in trace_event.c and only referencethis one?
Or even better, may be can we drop this callback field and statically call thiscode when we intialize an event. IIRC, the syscall raw_init_event has thesame callback, may be it's even the same for kprobes events (I can'tcheck right now).ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 06/13] function-graph: Allow writing the same val to set_graph_function

[Frederic Weisbecker]

2009/12/7 Li Zefan <lizf@cn.fujitsu.com>:
>  # echo 'do_open' > set_graph_function
>  # echo 'do_open' >> set_graph_function
>  bash: echo: write error: Invalid argument
>
> Make it valid to write the same value to set_graph_function,
> which is consistent with set_ftrace_filter interface.
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>


Acked-by: Frederic Weisbecker <fweisbec@gmail.com>


> ---
>  kernel/trace/ftrace.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index ff8aecd..7968762 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2552,10 +2552,9 @@ ftrace_set_func(unsigned long *array, int *idx, char *buffer)
>                                        exists = true;
>                                        break;
>                                }
> -                       if (!exists) {
> +                       if (!exists)
>                                array[(*idx)++] = rec->ip;
> -                               found = 1;
> -                       }
> +                       found = 1;
>                }
>        } while_for_each_ftrace_rec();
>
> --
> 1.6.3
>
>

Re: [PATCH 12/13] tracing/power: Remove two exports

[Arjan van de Ven]

On Mon, 07 Dec 2009 15:44:19 +0800
Li Zefan <lizf@cn.fujitsu.com> wrote:

> trace_power_start and trace_power_end are only used in
> arch/x86/kernel/power.c, and this file can't be compiled
> as a module, so these two tracepoints needn't to be exported.
> 

they were originally exported if some other arch wanted to use them
modular... since nobody does

Acked-by: Arjan van de Ven <arjan@linux.intel.com>


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 02/13] tracing: Extract calls to trace_define_common_fields()

[Frederic Weisbecker]

On Mon, Dec 07, 2009 at 03:40:35PM +0800, Li Zefan wrote:
> Call trace_define_common_fields() in event_create_dir() only.
> 
>    text    data     bss     dec     hex filename
> 5456157 2005772 7103796 14565725         de415d vmlinux.o
> 5454538 2005772 7103796 14564106         de3b0a vmlinux.o
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>


Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

Re: [PATCH 03/13] tracing: Move a printk out of ftrace_raw_reg_event_foo()

[Frederic Weisbecker]

On Mon, Dec 07, 2009 at 03:40:51PM +0800, Li Zefan wrote:
> Move the printk from each ftrace_raw_reg_event_foo() to
> its caller ftrace_event_enable_disable().
> 
> See how much space this saves:
> 
>    text    data     bss     dec     hex filename
> 5454538 2005772 7103796 14564106         de3b0a vmlinux.o
> 5440766 2005772 7103796 14550334         de053e vmlinux.o
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  include/trace/ftrace.h        |   16 ++--------------
>  kernel/trace/trace_events.c   |   17 +++++++++++++----
>  kernel/trace/trace_syscalls.c |   10 ++--------
>  3 files changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index 84d6f23..4aac981 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -555,13 +555,7 @@ static void ftrace_profile_disable_##name(struct ftrace_event_call *unused)\
>   *
>   * static int ftrace_reg_event_<call>(struct ftrace_event_call *unused)
>   * {
> - *	int ret;
> - *
> - *	ret = register_trace_<call>(ftrace_event_<call>);
> - *	if (!ret)
> - *		pr_info("event trace: Could not activate trace point "
> - *			"probe to  <call>");
> - *	return ret;
> + *	return register_trace_<call>(ftrace_event_<call>);
>   * }
>   *
>   * static void ftrace_unreg_event_<call>(struct ftrace_event_call *unused)
> @@ -733,13 +727,7 @@ static void ftrace_raw_event_##call(proto)				\
>  									\
>  static int ftrace_raw_reg_event_##call(struct ftrace_event_call *unused)\
>  {									\
> -	int ret;							\
> -									\
> -	ret = register_trace_##call(ftrace_raw_event_##call);		\
> -	if (ret)							\
> -		pr_info("event trace: Could not activate trace point "	\
> -			"probe to " #call "\n");			\
> -	return ret;							\
> +	return register_trace_##call(ftrace_raw_event_##call);		\
>  }									\
>  									\
>  static void ftrace_raw_unreg_event_##call(struct ftrace_event_call *unused)\
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 9fa6736..f22eaec 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -104,9 +104,11 @@ void trace_destroy_fields(struct ftrace_event_call *call)
>  	}
>  }
>  
> -static void ftrace_event_enable_disable(struct ftrace_event_call *call,
> +static int ftrace_event_enable_disable(struct ftrace_event_call *call,
>  					int enable)
>  {
> +	int ret = 0;
> +
>  	switch (enable) {
>  	case 0:
>  		if (call->enabled) {
> @@ -117,12 +119,19 @@ static void ftrace_event_enable_disable(struct ftrace_event_call *call,
>  		break;
>  	case 1:
>  		if (!call->enabled) {
> +			if (ret) {
> +				pr_info("event trace: Could not enable event "
> +					"%s\n", call->name);
> +				break;
> +			}
>  			call->enabled = 1;
>  			tracing_start_cmdline_record();
> -			call->regfunc(call);
> +			ret = call->regfunc(call);



Heh, I'm pretty sure I will never see this warning ;-)

Re: [PATCH 01/13] tracing: Extract duplicate ftrace_raw_init_event_foo()

[Li Zefan]

Frederic Weisbecker wrote:
> 2009/12/7 Li Zefan <lizf@cn.fujitsu.com>:
> 
>> +static int ftrace_raw_init_event_##call(struct ftrace_event_call *event_call)\
>> +{                                                                      \
>> +       int id;                                                         \
>> +                                                                       \
>> +       id = register_ftrace_event(event_call->event);                  \
>> +       if (!id)                                                        \
>> +               return -ENODEV;                                         \
>> +       event_call->id = id;                                            \
>> +       INIT_LIST_HEAD(&event_call->fields);                            \
>> +       return 0;                                                       \
>>  }
> 
> 
> This function doesn't vary anymore in this form.
> May be can we define a generic one in trace_event.c and only reference
> this one?
> 

Ah, you're right. I should have noticed this.

I'll make a patch to do this later on.

> Or even better, may be can we drop this callback field and statically call this
> code when we intialize an event. IIRC, the syscall raw_init_event has the
> same callback, may be it's even the same for kprobes events (I can't
> check right now).

I'll check it.

Re: [PATCH 03/13] tracing: Move a printk out of ftrace_raw_reg_event_foo()

[Li Zefan]

>>  	case 1:
>>  		if (!call->enabled) {
>> +			if (ret) {
>> +				pr_info("event trace: Could not enable event "
>> +					"%s\n", call->name);
>> +				break;
>> +			}
>>  			call->enabled = 1;
>>  			tracing_start_cmdline_record();
>> -			call->regfunc(call);
>> +			ret = call->regfunc(call);
> 
> Heh, I'm pretty sure I will never see this warning ;-)
> 

oops! I don't know how this mistake came.

[PATCH 02/13] tracing: Extract calls to trace_define_common_fields()

[Li Zefan]

Call trace_define_common_fields() in event_create_dir() only.

   text    data     bss     dec     hex filename
5346802 1961864 7103260 14411926         dbe896 vmlinux.o.old
5345151 1961864 7103260 14410275         dbe223 vmlinux.o

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/linux/ftrace_event.h  |    1 -
 include/trace/ftrace.h        |    4 ----
 kernel/trace/trace_events.c   |    7 ++++---
 kernel/trace/trace_export.c   |    4 ----
 kernel/trace/trace_kprobe.c   |    8 --------
 kernel/trace/trace_syscalls.c |    8 --------
 6 files changed, 4 insertions(+), 28 deletions(-)

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index cdea8a9..f883406 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -158,7 +158,6 @@ enum {
 };
 
 extern int trace_event_raw_init(struct ftrace_event_call *call);
-extern int trace_define_common_fields(struct ftrace_event_call *call);
 extern int trace_define_field(struct ftrace_event_call *call, const char *type,
 			      const char *name, int offset, int size,
 			      int is_signed, int filter_type);
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index f46ad26..f978376 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -436,10 +436,6 @@ ftrace_define_fields_##call(struct ftrace_event_call *event_call)	\
 	struct ftrace_raw_##call field;					\
 	int ret;							\
 									\
-	ret = trace_define_common_fields(event_call);			\
-	if (ret)							\
-		return ret;						\
-									\
 	tstruct;							\
 									\
 	return ret;							\
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 8ed66e0..97b0b3a 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -78,7 +78,7 @@ EXPORT_SYMBOL_GPL(trace_define_field);
 	if (ret)							\
 		return ret;
 
-int trace_define_common_fields(struct ftrace_event_call *call)
+static int trace_define_common_fields(struct ftrace_event_call *call)
 {
 	int ret;
 	struct trace_entry ent;
@@ -91,7 +91,6 @@ int trace_define_common_fields(struct ftrace_event_call *call)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(trace_define_common_fields);
 
 void trace_destroy_fields(struct ftrace_event_call *call)
 {
@@ -927,7 +926,9 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
 		 		  id);
 
 	if (call->define_fields) {
-		ret = call->define_fields(call);
+		ret = trace_define_common_fields(call);
+		if (!ret)
+			ret = call->define_fields(call);
 		if (ret < 0) {
 			pr_warning("Could not initialize trace point"
 				   " events/%s\n", call->name);
diff --git a/kernel/trace/trace_export.c b/kernel/trace/trace_export.c
index dff8c84..458e5bf 100644
--- a/kernel/trace/trace_export.c
+++ b/kernel/trace/trace_export.c
@@ -184,10 +184,6 @@ ftrace_define_fields_##name(struct ftrace_event_call *event_call)	\
 	struct struct_name field;					\
 	int ret;							\
 									\
-	ret = trace_define_common_fields(event_call);			\
-	if (ret)							\
-		return ret;						\
-									\
 	tstruct;							\
 									\
 	return ret;							\
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index aff5f80..e3c80e9 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1113,10 +1113,6 @@ static int kprobe_event_define_fields(struct ftrace_event_call *event_call)
 	struct kprobe_trace_entry field;
 	struct trace_probe *tp = (struct trace_probe *)event_call->data;
 
-	ret = trace_define_common_fields(event_call);
-	if (!ret)
-		return ret;
-
 	DEFINE_FIELD(unsigned long, ip, FIELD_STRING_IP, 0);
 	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
 	/* Set argument names as fields */
@@ -1131,10 +1127,6 @@ static int kretprobe_event_define_fields(struct ftrace_event_call *event_call)
 	struct kretprobe_trace_entry field;
 	struct trace_probe *tp = (struct trace_probe *)event_call->data;
 
-	ret = trace_define_common_fields(event_call);
-	if (!ret)
-		return ret;
-
 	DEFINE_FIELD(unsigned long, func, FIELD_STRING_FUNC, 0);
 	DEFINE_FIELD(unsigned long, ret_ip, FIELD_STRING_RETIP, 0);
 	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 57501d9..b957edd 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -217,10 +217,6 @@ int syscall_enter_define_fields(struct ftrace_event_call *call)
 	int i;
 	int offset = offsetof(typeof(trace), args);
 
-	ret = trace_define_common_fields(call);
-	if (ret)
-		return ret;
-
 	ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER);
 	if (ret)
 		return ret;
@@ -241,10 +237,6 @@ int syscall_exit_define_fields(struct ftrace_event_call *call)
 	struct syscall_trace_exit trace;
 	int ret;
 
-	ret = trace_define_common_fields(call);
-	if (ret)
-		return ret;
-
 	ret = trace_define_field(call, SYSCALL_FIELD(int, nr), FILTER_OTHER);
 	if (ret)
 		return ret;
-- 
1.6.3

Re: [PATCH 02/13] tracing: Extract calls to trace_define_common_fields()

[Lai Jiangshan]

Li Zefan wrote:
> Call trace_define_common_fields() in event_create_dir() only.
> 
>    text    data     bss     dec     hex filename
> 5346802 1961864 7103260 14411926         dbe896 vmlinux.o.old
> 5345151 1961864 7103260 14410275         dbe223 vmlinux.o
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

I think this is a urgent patch for mainline.

I have found a regression at trace_kprobe, and I'm lucky to find that this
patch can also fix this regression.


> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -1113,10 +1113,6 @@ static int kprobe_event_define_fields(struct ftrace_event_call *event_call)
>  	struct kprobe_trace_entry field;
>  	struct trace_probe *tp = (struct trace_probe *)event_call->data;
>  
> -	ret = trace_define_common_fields(event_call);
> -	if (!ret)
> -		return ret;
> -
>  	DEFINE_FIELD(unsigned long, ip, FIELD_STRING_IP, 0);
>  	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
>  	/* Set argument names as fields */
> @@ -1131,10 +1127,6 @@ static int kretprobe_event_define_fields(struct ftrace_event_call *event_call)
>  	struct kretprobe_trace_entry field;
>  	struct trace_probe *tp = (struct trace_probe *)event_call->data;
>  
> -	ret = trace_define_common_fields(event_call);
> -	if (!ret)
> -		return ret;
> -
>  	DEFINE_FIELD(unsigned long, func, FIELD_STRING_FUNC, 0);
>  	DEFINE_FIELD(unsigned long, ret_ip, FIELD_STRING_RETIP, 0);
>  	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);

"if (!ret)" is wrong. trace_define_common_fields() returns zero when success.
So "unsigned long, func", "unsigned long, ret_ip" ...etc are NOT "defined"
and filters can not be applied for trace_kprobe.

Reviewed-by: Lai Jiangshan <laijs@cn.fujitsu.com>

Re: [PATCH 02/13] tracing: Extract calls to trace_define_common_fields()

[Frederic Weisbecker]

On Tue, Dec 08, 2009 at 01:36:20PM +0800, Lai Jiangshan wrote:
> Li Zefan wrote:
> > Call trace_define_common_fields() in event_create_dir() only.
> > 
> >    text    data     bss     dec     hex filename
> > 5346802 1961864 7103260 14411926         dbe896 vmlinux.o.old
> > 5345151 1961864 7103260 14410275         dbe223 vmlinux.o
> > 
> > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> > Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> I think this is a urgent patch for mainline.
> 
> I have found a regression at trace_kprobe, and I'm lucky to find that this
> patch can also fix this regression.
> 
> 
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -1113,10 +1113,6 @@ static int kprobe_event_define_fields(struct ftrace_event_call *event_call)
> >  	struct kprobe_trace_entry field;
> >  	struct trace_probe *tp = (struct trace_probe *)event_call->data;
> >  
> > -	ret = trace_define_common_fields(event_call);
> > -	if (!ret)
> > -		return ret;
> > -
> >  	DEFINE_FIELD(unsigned long, ip, FIELD_STRING_IP, 0);
> >  	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
> >  	/* Set argument names as fields */
> > @@ -1131,10 +1127,6 @@ static int kretprobe_event_define_fields(struct ftrace_event_call *event_call)
> >  	struct kretprobe_trace_entry field;
> >  	struct trace_probe *tp = (struct trace_probe *)event_call->data;
> >  
> > -	ret = trace_define_common_fields(event_call);
> > -	if (!ret)
> > -		return ret;
> > -
> >  	DEFINE_FIELD(unsigned long, func, FIELD_STRING_FUNC, 0);
> >  	DEFINE_FIELD(unsigned long, ret_ip, FIELD_STRING_RETIP, 0);
> >  	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
> 
> "if (!ret)" is wrong. trace_define_common_fields() returns zero when success.
> So "unsigned long, func", "unsigned long, ret_ip" ...etc are NOT "defined"
> and filters can not be applied for trace_kprobe.
> 
> Reviewed-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> 


Oh right!

Ok, I'm queueing this one for the tracing fixes.
Thanks!

Re: [PATCH 02/13] tracing: Extract calls to trace_define_common_fields()

[Li Zefan]

>>> --- a/kernel/trace/trace_kprobe.c
>>> +++ b/kernel/trace/trace_kprobe.c
>>> @@ -1113,10 +1113,6 @@ static int kprobe_event_define_fields(struct ftrace_event_call *event_call)
>>>  	struct kprobe_trace_entry field;
>>>  	struct trace_probe *tp = (struct trace_probe *)event_call->data;
>>>  
>>> -	ret = trace_define_common_fields(event_call);
>>> -	if (!ret)
>>> -		return ret;
>>> -
>>>  	DEFINE_FIELD(unsigned long, ip, FIELD_STRING_IP, 0);
>>>  	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
>>>  	/* Set argument names as fields */
>>> @@ -1131,10 +1127,6 @@ static int kretprobe_event_define_fields(struct ftrace_event_call *event_call)
>>>  	struct kretprobe_trace_entry field;
>>>  	struct trace_probe *tp = (struct trace_probe *)event_call->data;
>>>  
>>> -	ret = trace_define_common_fields(event_call);
>>> -	if (!ret)
>>> -		return ret;
>>> -
>>>  	DEFINE_FIELD(unsigned long, func, FIELD_STRING_FUNC, 0);
>>>  	DEFINE_FIELD(unsigned long, ret_ip, FIELD_STRING_RETIP, 0);
>>>  	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
>> "if (!ret)" is wrong. trace_define_common_fields() returns zero when success.
>> So "unsigned long, func", "unsigned long, ret_ip" ...etc are NOT "defined"
>> and filters can not be applied for trace_kprobe.
>>
>> Reviewed-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>>
> 
> Oh right!
> 
> Ok, I'm queueing this one for the tracing fixes.
> Thanks!
> 

This patch has dependency on the previous patch..So can you queue
all other patches and send a pull request to Ingo? We're still
in the early merge window, so those patches should be fine for
.33.

And as this patch fixes a bug "acidentally", the changelog
needs a bit revision.

Re: [PATCH 02/13] tracing: Extract calls to trace_define_common_fields()

[Frederic Weisbecker]

On Tue, Dec 08, 2009 at 04:03:58PM +0800, Li Zefan wrote:
> >>> --- a/kernel/trace/trace_kprobe.c
> >>> +++ b/kernel/trace/trace_kprobe.c
> >>> @@ -1113,10 +1113,6 @@ static int kprobe_event_define_fields(struct ftrace_event_call *event_call)
> >>>  	struct kprobe_trace_entry field;
> >>>  	struct trace_probe *tp = (struct trace_probe *)event_call->data;
> >>>  
> >>> -	ret = trace_define_common_fields(event_call);
> >>> -	if (!ret)
> >>> -		return ret;
> >>> -
> >>>  	DEFINE_FIELD(unsigned long, ip, FIELD_STRING_IP, 0);
> >>>  	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
> >>>  	/* Set argument names as fields */
> >>> @@ -1131,10 +1127,6 @@ static int kretprobe_event_define_fields(struct ftrace_event_call *event_call)
> >>>  	struct kretprobe_trace_entry field;
> >>>  	struct trace_probe *tp = (struct trace_probe *)event_call->data;
> >>>  
> >>> -	ret = trace_define_common_fields(event_call);
> >>> -	if (!ret)
> >>> -		return ret;
> >>> -
> >>>  	DEFINE_FIELD(unsigned long, func, FIELD_STRING_FUNC, 0);
> >>>  	DEFINE_FIELD(unsigned long, ret_ip, FIELD_STRING_RETIP, 0);
> >>>  	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
> >> "if (!ret)" is wrong. trace_define_common_fields() returns zero when success.
> >> So "unsigned long, func", "unsigned long, ret_ip" ...etc are NOT "defined"
> >> and filters can not be applied for trace_kprobe.
> >>
> >> Reviewed-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> >>
> > 
> > Oh right!
> > 
> > Ok, I'm queueing this one for the tracing fixes.
> > Thanks!
> > 
> 
> This patch has dependency on the previous patch..So can you queue
> all other patches and send a pull request to Ingo? We're still
> in the early merge window, so those patches should be fine for
> .33.
> 
> And as this patch fixes a bug "acidentally", the changelog
> needs a bit revision.


The problem is that we can't know in advance Linus will take
a second round of tracing features in this merge window.

I'd rather be careful and keep separating tracing fixes and
tracing features.

I'm preparing a separate fix.

Re: [PATCH 02/13] tracing: Extract calls to trace_define_common_fields()

[Li Zefan]

>>>>> @@ -1131,10 +1127,6 @@ static int kretprobe_event_define_fields(struct ftrace_event_call *event_call)
>>>>>  	struct kretprobe_trace_entry field;
>>>>>  	struct trace_probe *tp = (struct trace_probe *)event_call->data;
>>>>>  
>>>>> -	ret = trace_define_common_fields(event_call);
>>>>> -	if (!ret)
>>>>> -		return ret;
>>>>> -
>>>>>  	DEFINE_FIELD(unsigned long, func, FIELD_STRING_FUNC, 0);
>>>>>  	DEFINE_FIELD(unsigned long, ret_ip, FIELD_STRING_RETIP, 0);
>>>>>  	DEFINE_FIELD(int, nargs, FIELD_STRING_NARGS, 1);
>>>> "if (!ret)" is wrong. trace_define_common_fields() returns zero when success.
>>>> So "unsigned long, func", "unsigned long, ret_ip" ...etc are NOT "defined"
>>>> and filters can not be applied for trace_kprobe.
>>>>
>>>> Reviewed-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>>>>
>>> Oh right!
>>>
>>> Ok, I'm queueing this one for the tracing fixes.
>>> Thanks!
>>>
>> This patch has dependency on the previous patch..So can you queue
>> all other patches and send a pull request to Ingo? We're still
>> in the early merge window, so those patches should be fine for
>> .33.
>>
>> And as this patch fixes a bug "acidentally", the changelog
>> needs a bit revision.
> 
> The problem is that we can't know in advance Linus will take
> a second round of tracing features in this merge window.
> 
> I'd rather be careful and keep separating tracing fixes and
> tracing features.
> 
> I'm preparing a separate fix.
> 

But there is no functionality change or new feature in this
patchset, all are cleanups and fixes.

That said, I don't mind how you separate them. :)

Re: [PATCH 02/13] tracing: Extract calls to trace_define_common_fields()

[Frederic Weisbecker]

On Tue, Dec 08, 2009 at 04:21:14PM +0800, Li Zefan wrote:
> > The problem is that we can't know in advance Linus will take
> > a second round of tracing features in this merge window.
> > 
> > I'd rather be careful and keep separating tracing fixes and
> > tracing features.
> > 
> > I'm preparing a separate fix.
> > 
> 
> But there is no functionality change or new feature in this
> patchset, all are cleanups and fixes.


They are not _supposed_ to have functionality change :)
They are mostly cleanups, reorganization, better handling
of interfaces, etc...

All that is in the family of "features". They are improvements
that could possibly induce new bugs.

OTOH, pure fixes are only surgical patches and should not
expand their scope to a reorg or a code cleanup.

Well, it depends, I'm not preaching hard rules. But the
point is we never know if Linus will take another round
of the first category of patches in this merge window.
So it's better to keep pure fixes isolated, at least
they have good chances to keep beeing eligible
after -rc1.