[PATCH 0/5] ftrace: rebase patches on top of tip/master + some extras

[PATCH 0/5] ftrace: rebase patches on top of tip/master + some extras

[Steven Rostedt]

Ingo,

Here's my patches rebased against latest tip/master. I also
added a couple of goodies at the end to enhance the function-graph
tracer.

The first patch is a repost of the set_ftrace_pid, so I am Cc'ing
the container guys so they can abuse me some more ;-)

The following patches are in:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: tip/devel


Steven Rostedt (5):
      ftrace: add function tracing to single thread
      ftrace: use code patching for ftrace graph tracer
      ftrace: let function tracing and function return run together
      ftrace: add thread comm to function graph tracer
      ftrace: add cpu annotation for function graph tracer

----
 Documentation/ftrace.txt             |   79 ++++++++++
 arch/x86/kernel/entry_32.S           |    5 +
 arch/x86/kernel/ftrace.c             |   48 ++++++-
 include/linux/ftrace.h               |    5 +
 kernel/trace/ftrace.c                |  264 +++++++++++++++++++++++++--------
 kernel/trace/trace.c                 |    2 +-
 kernel/trace/trace.h                 |    1 +
 kernel/trace/trace_functions_graph.c |   51 +++++--
 8 files changed, 373 insertions(+), 82 deletions(-)

-- 

[PATCH 1/5] ftrace: add function tracing to single thread

[Steven Rostedt]

[-- Attachment #1: 0001-ftrace-add-function-tracing-to-single-thread.patch --]
[-- Type: text/plain, Size: 12047 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Impact: feature to function trace a single thread

This patch adds the ability to function trace a single thread.
The file:

  /debugfs/tracing/set_ftrace_pid

contains the pid to trace. Valid pids are any positive integer.
Writing any negative number to this file will disable the pid
tracing and the function tracer will go back to tracing all of
threads.

This feature works with both static and dynamic function tracing.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 Documentation/ftrace.txt |   79 +++++++++++++++++
 kernel/trace/ftrace.c    |  209 ++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 262 insertions(+), 26 deletions(-)

diff --git a/Documentation/ftrace.txt b/Documentation/ftrace.txt
index 35a78bc..de05042 100644
--- a/Documentation/ftrace.txt
+++ b/Documentation/ftrace.txt
@@ -127,6 +127,8 @@ of ftrace. Here is a list of some of the key files:
 		be traced. If a function exists in both set_ftrace_filter
 		and set_ftrace_notrace,	the function will _not_ be traced.
 
+  set_ftrace_pid: Have the function tracer only trace a single thread.
+
   available_filter_functions: This lists the functions that ftrace
 		has processed and can trace. These are the function
 		names that you can pass to "set_ftrace_filter" or
@@ -1073,6 +1075,83 @@ For simple one time traces, the above is sufficent. For anything else,
 a search through /proc/mounts may be needed to find where the debugfs
 file-system is mounted.
 
+
+Single thread tracing
+---------------------
+
+By writing into /debug/tracing/set_ftrace_pid you can trace a
+single thread. For example:
+
+# cat /debug/tracing/set_ftrace_pid
+no pid
+# echo 3111 > /debug/tracing/set_ftrace_pid
+# cat /debug/tracing/set_ftrace_pid
+3111
+# echo function > /debug/tracing/current_tracer
+# cat /debug/tracing/trace | head
+ # tracer: function
+ #
+ #           TASK-PID    CPU#    TIMESTAMP  FUNCTION
+ #              | |       |          |         |
+     yum-updatesd-3111  [003]  1637.254676: finish_task_switch <-thread_return
+     yum-updatesd-3111  [003]  1637.254681: hrtimer_cancel <-schedule_hrtimeout_range
+     yum-updatesd-3111  [003]  1637.254682: hrtimer_try_to_cancel <-hrtimer_cancel
+     yum-updatesd-3111  [003]  1637.254683: lock_hrtimer_base <-hrtimer_try_to_cancel
+     yum-updatesd-3111  [003]  1637.254685: fget_light <-do_sys_poll
+     yum-updatesd-3111  [003]  1637.254686: pipe_poll <-do_sys_poll
+# echo -1 > /debug/tracing/set_ftrace_pid
+# cat /debug/tracing/trace |head
+ # tracer: function
+ #
+ #           TASK-PID    CPU#    TIMESTAMP  FUNCTION
+ #              | |       |          |         |
+ ##### CPU 3 buffer started ####
+     yum-updatesd-3111  [003]  1701.957688: free_poll_entry <-poll_freewait
+     yum-updatesd-3111  [003]  1701.957689: remove_wait_queue <-free_poll_entry
+     yum-updatesd-3111  [003]  1701.957691: fput <-free_poll_entry
+     yum-updatesd-3111  [003]  1701.957692: audit_syscall_exit <-sysret_audit
+     yum-updatesd-3111  [003]  1701.957693: path_put <-audit_syscall_exit
+
+If you want to trace a function when executing, you could use
+something like this simple program:
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+
+int main (int argc, char **argv)
+{
+        if (argc < 1)
+                exit(-1);
+
+        if (fork() > 0) {
+                int fd, ffd;
+                char line[64];
+                int s;
+
+                ffd = open("/debug/tracing/current_tracer", O_WRONLY);
+                if (ffd < 0)
+                        exit(-1);
+                write(ffd, "nop", 3);
+
+                fd = open("/debug/tracing/set_ftrace_pid", O_WRONLY);
+                s = sprintf(line, "%d\n", getpid());
+                write(fd, line, s);
+
+                write(ffd, "function", 8);
+
+                close(fd);
+                close(ffd);
+
+                execvp(argv[1], argv+1);
+        }
+
+        return 0;
+}
+
 dynamic ftrace
 --------------
 
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 7e2d3b9..00d98c6 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -47,6 +47,9 @@
 int ftrace_enabled __read_mostly;
 static int last_ftrace_enabled;
 
+/* ftrace_pid_trace >= 0 will only trace threads with this pid */
+static int ftrace_pid_trace = -1;
+
 /* Quick disabling of function tracer. */
 int function_trace_stop;
 
@@ -61,6 +64,7 @@ static int ftrace_disabled __read_mostly;
 
 static DEFINE_SPINLOCK(ftrace_lock);
 static DEFINE_MUTEX(ftrace_sysctl_lock);
+static DEFINE_MUTEX(ftrace_start_lock);
 
 static struct ftrace_ops ftrace_list_end __read_mostly =
 {
@@ -70,6 +74,7 @@ static struct ftrace_ops ftrace_list_end __read_mostly =
 static struct ftrace_ops *ftrace_list __read_mostly = &ftrace_list_end;
 ftrace_func_t ftrace_trace_function __read_mostly = ftrace_stub;
 ftrace_func_t __ftrace_trace_function __read_mostly = ftrace_stub;
+ftrace_func_t ftrace_pid_function __read_mostly = ftrace_stub;
 
 static void ftrace_list_func(unsigned long ip, unsigned long parent_ip)
 {
@@ -86,6 +91,21 @@ static void ftrace_list_func(unsigned long ip, unsigned long parent_ip)
 	};
 }
 
+static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip)
+{
+	if (current->pid != ftrace_pid_trace)
+		return;
+
+	ftrace_pid_function(ip, parent_ip);
+}
+
+static void set_ftrace_pid_function(ftrace_func_t func)
+{
+	/* do not set ftrace_pid_function to itself! */
+	if (func != ftrace_pid_func)
+		ftrace_pid_function = func;
+}
+
 /**
  * clear_ftrace_function - reset the ftrace function
  *
@@ -96,6 +116,7 @@ void clear_ftrace_function(void)
 {
 	ftrace_trace_function = ftrace_stub;
 	__ftrace_trace_function = ftrace_stub;
+	ftrace_pid_function = ftrace_stub;
 }
 
 #ifndef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
@@ -128,20 +149,26 @@ static int __register_ftrace_function(struct ftrace_ops *ops)
 	ftrace_list = ops;
 
 	if (ftrace_enabled) {
+		ftrace_func_t func;
+
+		if (ops->next == &ftrace_list_end)
+			func = ops->func;
+		else
+			func = ftrace_list_func;
+
+		if (ftrace_pid_trace >= 0) {
+			set_ftrace_pid_function(func);
+			func = ftrace_pid_func;
+		}
+
 		/*
 		 * For one func, simply call it directly.
 		 * For more than one func, call the chain.
 		 */
 #ifdef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
-		if (ops->next == &ftrace_list_end)
-			ftrace_trace_function = ops->func;
-		else
-			ftrace_trace_function = ftrace_list_func;
+		ftrace_trace_function = func;
 #else
-		if (ops->next == &ftrace_list_end)
-			__ftrace_trace_function = ops->func;
-		else
-			__ftrace_trace_function = ftrace_list_func;
+		__ftrace_trace_function = func;
 		ftrace_trace_function = ftrace_test_stop_func;
 #endif
 	}
@@ -182,8 +209,19 @@ static int __unregister_ftrace_function(struct ftrace_ops *ops)
 
 	if (ftrace_enabled) {
 		/* If we only have one func left, then call that directly */
-		if (ftrace_list->next == &ftrace_list_end)
-			ftrace_trace_function = ftrace_list->func;
+		if (ftrace_list->next == &ftrace_list_end) {
+			ftrace_func_t func = ftrace_list->func;
+
+			if (ftrace_pid_trace >= 0) {
+				set_ftrace_pid_function(func);
+				func = ftrace_pid_func;
+			}
+#ifdef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
+			ftrace_trace_function = func;
+#else
+			__ftrace_trace_function = func;
+#endif
+		}
 	}
 
  out:
@@ -192,6 +230,38 @@ static int __unregister_ftrace_function(struct ftrace_ops *ops)
 	return ret;
 }
 
+static void ftrace_update_pid_func(void)
+{
+	ftrace_func_t func;
+
+	/* should not be called from interrupt context */
+	spin_lock(&ftrace_lock);
+
+	if (ftrace_trace_function == ftrace_stub)
+		goto out;
+
+	func = ftrace_trace_function;
+
+	if (ftrace_pid_trace >= 0) {
+		set_ftrace_pid_function(func);
+		func = ftrace_pid_func;
+	} else {
+		if (func != ftrace_pid_func)
+			goto out;
+
+		set_ftrace_pid_function(func);
+	}
+
+#ifdef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
+	ftrace_trace_function = func;
+#else
+	__ftrace_trace_function = func;
+#endif
+
+ out:
+	spin_unlock(&ftrace_lock);
+}
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 #ifndef CONFIG_FTRACE_MCOUNT_RECORD
 # error Dynamic ftrace depends on MCOUNT_RECORD
@@ -545,7 +615,19 @@ static void ftrace_run_update_code(int command)
 
 static ftrace_func_t saved_ftrace_func;
 static int ftrace_start_up;
-static DEFINE_MUTEX(ftrace_start_lock);
+
+static void ftrace_startup_enable(int command)
+{
+	if (saved_ftrace_func != ftrace_trace_function) {
+		saved_ftrace_func = ftrace_trace_function;
+		command |= FTRACE_UPDATE_TRACE_FUNC;
+	}
+
+	if (!command || !ftrace_enabled)
+		return;
+
+	ftrace_run_update_code(command);
+}
 
 static void ftrace_startup(void)
 {
@@ -558,16 +640,8 @@ static void ftrace_startup(void)
 	ftrace_start_up++;
 	command |= FTRACE_ENABLE_CALLS;
 
-	if (saved_ftrace_func != ftrace_trace_function) {
-		saved_ftrace_func = ftrace_trace_function;
-		command |= FTRACE_UPDATE_TRACE_FUNC;
-	}
-
-	if (!command || !ftrace_enabled)
-		goto out;
+	ftrace_startup_enable(command);
 
-	ftrace_run_update_code(command);
- out:
 	mutex_unlock(&ftrace_start_lock);
 }
 
@@ -1262,13 +1336,10 @@ static struct file_operations ftrace_notrace_fops = {
 	.release = ftrace_notrace_release,
 };
 
-static __init int ftrace_init_debugfs(void)
+static __init int ftrace_init_dyn_debugfs(struct dentry *d_tracer)
 {
-	struct dentry *d_tracer;
 	struct dentry *entry;
 
-	d_tracer = tracing_init_dentry();
-
 	entry = debugfs_create_file("available_filter_functions", 0444,
 				    d_tracer, NULL, &ftrace_avail_fops);
 	if (!entry)
@@ -1295,8 +1366,6 @@ static __init int ftrace_init_debugfs(void)
 	return 0;
 }
 
-fs_initcall(ftrace_init_debugfs);
-
 static int ftrace_convert_nops(struct module *mod,
 			       unsigned long *start,
 			       unsigned long *end)
@@ -1382,12 +1451,100 @@ static int __init ftrace_nodyn_init(void)
 }
 device_initcall(ftrace_nodyn_init);
 
+static inline int ftrace_init_dyn_debugfs(struct dentry *d_tracer) { return 0; }
+static inline void ftrace_startup_enable(int command) { }
 # define ftrace_startup()		do { } while (0)
 # define ftrace_shutdown()		do { } while (0)
 # define ftrace_startup_sysctl()	do { } while (0)
 # define ftrace_shutdown_sysctl()	do { } while (0)
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
+static ssize_t
+ftrace_pid_read(struct file *file, char __user *ubuf,
+		       size_t cnt, loff_t *ppos)
+{
+	char buf[64];
+	int r;
+
+	if (ftrace_pid_trace >= 0)
+		r = sprintf(buf, "%u\n", ftrace_pid_trace);
+	else
+		r = sprintf(buf, "no pid\n");
+
+	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
+}
+
+static ssize_t
+ftrace_pid_write(struct file *filp, const char __user *ubuf,
+		   size_t cnt, loff_t *ppos)
+{
+	char buf[64];
+	long val;
+	int ret;
+
+	if (cnt >= sizeof(buf))
+		return -EINVAL;
+
+	if (copy_from_user(&buf, ubuf, cnt))
+		return -EFAULT;
+
+	buf[cnt] = 0;
+
+	ret = strict_strtol(buf, 10, &val);
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&ftrace_start_lock);
+	if (ret < 0) {
+		/* disable pid tracing */
+		if (ftrace_pid_trace < 0)
+			goto out;
+		ftrace_pid_trace = -1;
+
+	} else {
+
+		if (ftrace_pid_trace == val)
+			goto out;
+
+		ftrace_pid_trace = val;
+	}
+
+	/* update the function call */
+	ftrace_update_pid_func();
+	ftrace_startup_enable(0);
+
+ out:
+	mutex_unlock(&ftrace_start_lock);
+
+	return cnt;
+}
+
+static struct file_operations ftrace_pid_fops = {
+	.read = ftrace_pid_read,
+	.write = ftrace_pid_write,
+};
+
+static __init int ftrace_init_debugfs(void)
+{
+	struct dentry *d_tracer;
+	struct dentry *entry;
+
+	d_tracer = tracing_init_dentry();
+	if (!d_tracer)
+		return 0;
+
+	ftrace_init_dyn_debugfs(d_tracer);
+
+	entry = debugfs_create_file("set_ftrace_pid", 0644, d_tracer,
+				    NULL, &ftrace_pid_fops);
+	if (!entry)
+		pr_warning("Could not create debugfs "
+			   "'set_ftrace_pid' entry\n");
+	return 0;
+}
+
+fs_initcall(ftrace_init_debugfs);
+
 /**
  * ftrace_kill - kill ftrace
  *
-- 
1.5.6.5

-- 

[PATCH 2/5] ftrace: use code patching for ftrace graph tracer

[Steven Rostedt]

[-- Attachment #1: 0002-ftrace-use-code-patching-for-ftrace-graph-tracer.patch --]
[-- Type: text/plain, Size: 6921 bytes --]

From: Steven Rostedt <rostedt@goodmis.org>

Impact: more efficient code for ftrace graph tracer

This patch uses the dynamic patching, when available, to patch
the function graph code into the kernel.

This patch will ease the way for letting both function tracing
and function graph tracing run together.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/x86/kernel/entry_32.S |    5 ++++
 arch/x86/kernel/ftrace.c   |   48 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/ftrace.h     |    5 ++++
 kernel/trace/ftrace.c      |   35 ++++++++++++++-----------------
 4 files changed, 72 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index 71cdda1..37feecf 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -1185,6 +1185,11 @@ ftrace_call:
 	popl %edx
 	popl %ecx
 	popl %eax
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+.globl ftrace_graph_call
+ftrace_graph_call:
+	jmp ftrace_stub
+#endif
 
 .globl ftrace_stub
 ftrace_stub:
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 26b2d92..7ef914e 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -111,7 +111,6 @@ static void ftrace_mod_code(void)
 	 */
 	mod_code_status = probe_kernel_write(mod_code_ip, mod_code_newcode,
 					     MCOUNT_INSN_SIZE);
-
 }
 
 void ftrace_nmi_enter(void)
@@ -325,7 +324,51 @@ int __init ftrace_dyn_arch_init(void *data)
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
-#ifndef CONFIG_DYNAMIC_FTRACE
+#ifdef CONFIG_DYNAMIC_FTRACE
+extern void ftrace_graph_call(void);
+
+static int ftrace_mod_jmp(unsigned long ip,
+			  int old_offset, int new_offset)
+{
+	unsigned char code[MCOUNT_INSN_SIZE];
+
+	if (probe_kernel_read(code, (void *)ip, MCOUNT_INSN_SIZE))
+		return -EFAULT;
+
+	if (code[0] != 0xe9 || old_offset != *(int *)(&code[1]))
+		return -EINVAL;
+
+	*(int *)(&code[1]) = new_offset;
+
+	if (do_ftrace_mod_code(ip, &code))
+		return -EPERM;
+
+	return 0;
+}
+
+int ftrace_enable_ftrace_graph_caller(void)
+{
+	unsigned long ip = (unsigned long)(&ftrace_graph_call);
+	int old_offset, new_offset;
+
+	old_offset = (unsigned long)(&ftrace_stub) - (ip + MCOUNT_INSN_SIZE);
+	new_offset = (unsigned long)(&ftrace_graph_caller) - (ip + MCOUNT_INSN_SIZE);
+
+	return ftrace_mod_jmp(ip, old_offset, new_offset);
+}
+
+int ftrace_disable_ftrace_graph_caller(void)
+{
+	unsigned long ip = (unsigned long)(&ftrace_graph_call);
+	int old_offset, new_offset;
+
+	old_offset = (unsigned long)(&ftrace_graph_caller) - (ip + MCOUNT_INSN_SIZE);
+	new_offset = (unsigned long)(&ftrace_stub) - (ip + MCOUNT_INSN_SIZE);
+
+	return ftrace_mod_jmp(ip, old_offset, new_offset);
+}
+
+#else /* CONFIG_DYNAMIC_FTRACE */
 
 /*
  * These functions are picked from those used on
@@ -343,6 +386,7 @@ void ftrace_nmi_exit(void)
 {
 	atomic_dec(&in_nmi);
 }
+
 #endif /* !CONFIG_DYNAMIC_FTRACE */
 
 /* Add a function return address to the trace stack on thread info.*/
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index fc2d549..f9792c0 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -117,6 +117,11 @@ extern void ftrace_call(void);
 extern void mcount_call(void);
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 extern void ftrace_graph_caller(void);
+extern int ftrace_enable_ftrace_graph_caller(void);
+extern int ftrace_disable_ftrace_graph_caller(void);
+#else
+static inline int ftrace_enable_ftrace_graph_caller(void) { return 0; }
+static inline int ftrace_disable_ftrace_graph_caller(void) { return 0; }
 #endif
 
 /**
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 00d98c6..5f7c864 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -281,6 +281,8 @@ enum {
 	FTRACE_UPDATE_TRACE_FUNC	= (1 << 2),
 	FTRACE_ENABLE_MCOUNT		= (1 << 3),
 	FTRACE_DISABLE_MCOUNT		= (1 << 4),
+	FTRACE_START_FUNC_RET		= (1 << 5),
+	FTRACE_STOP_FUNC_RET		= (1 << 6),
 };
 
 static int ftrace_filtered;
@@ -465,14 +467,7 @@ __ftrace_replace_code(struct dyn_ftrace *rec, int enable)
 	unsigned long ip, fl;
 	unsigned long ftrace_addr;
 
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	if (ftrace_tracing_type == FTRACE_TYPE_ENTER)
-		ftrace_addr = (unsigned long)ftrace_caller;
-	else
-		ftrace_addr = (unsigned long)ftrace_graph_caller;
-#else
 	ftrace_addr = (unsigned long)ftrace_caller;
-#endif
 
 	ip = rec->ip;
 
@@ -605,6 +600,11 @@ static int __ftrace_modify_code(void *data)
 	if (*command & FTRACE_UPDATE_TRACE_FUNC)
 		ftrace_update_ftrace_func(ftrace_trace_function);
 
+	if (*command & FTRACE_START_FUNC_RET)
+		ftrace_enable_ftrace_graph_caller();
+	else if (*command & FTRACE_STOP_FUNC_RET)
+		ftrace_disable_ftrace_graph_caller();
+
 	return 0;
 }
 
@@ -629,10 +629,8 @@ static void ftrace_startup_enable(int command)
 	ftrace_run_update_code(command);
 }
 
-static void ftrace_startup(void)
+static void ftrace_startup(int command)
 {
-	int command = 0;
-
 	if (unlikely(ftrace_disabled))
 		return;
 
@@ -645,10 +643,8 @@ static void ftrace_startup(void)
 	mutex_unlock(&ftrace_start_lock);
 }
 
-static void ftrace_shutdown(void)
+static void ftrace_shutdown(int command)
 {
-	int command = 0;
-
 	if (unlikely(ftrace_disabled))
 		return;
 
@@ -1453,8 +1449,9 @@ device_initcall(ftrace_nodyn_init);
 
 static inline int ftrace_init_dyn_debugfs(struct dentry *d_tracer) { return 0; }
 static inline void ftrace_startup_enable(int command) { }
-# define ftrace_startup()		do { } while (0)
-# define ftrace_shutdown()		do { } while (0)
+/* Keep as macros so we do not need to define the commands */
+# define ftrace_startup(command)	do { } while (0)
+# define ftrace_shutdown(command)	do { } while (0)
 # define ftrace_startup_sysctl()	do { } while (0)
 # define ftrace_shutdown_sysctl()	do { } while (0)
 #endif /* CONFIG_DYNAMIC_FTRACE */
@@ -1585,7 +1582,7 @@ int register_ftrace_function(struct ftrace_ops *ops)
 	}
 
 	ret = __register_ftrace_function(ops);
-	ftrace_startup();
+	ftrace_startup(0);
 
 out:
 	mutex_unlock(&ftrace_sysctl_lock);
@@ -1604,7 +1601,7 @@ int unregister_ftrace_function(struct ftrace_ops *ops)
 
 	mutex_lock(&ftrace_sysctl_lock);
 	ret = __unregister_ftrace_function(ops);
-	ftrace_shutdown();
+	ftrace_shutdown(0);
 	mutex_unlock(&ftrace_sysctl_lock);
 
 	return ret;
@@ -1751,7 +1748,7 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
 	ftrace_tracing_type = FTRACE_TYPE_RETURN;
 	ftrace_graph_return = retfunc;
 	ftrace_graph_entry = entryfunc;
-	ftrace_startup();
+	ftrace_startup(FTRACE_START_FUNC_RET);
 
 out:
 	mutex_unlock(&ftrace_sysctl_lock);
@@ -1765,7 +1762,7 @@ void unregister_ftrace_graph(void)
 	atomic_dec(&ftrace_graph_active);
 	ftrace_graph_return = (trace_func_graph_ret_t)ftrace_stub;
 	ftrace_graph_entry = (trace_func_graph_ent_t)ftrace_stub;
-	ftrace_shutdown();
+	ftrace_shutdown(FTRACE_STOP_FUNC_RET);
 	/* Restore normal tracing type */
 	ftrace_tracing_type = FTRACE_TYPE_ENTER;
 
-- 
1.5.6.5

-- 

[PATCH 3/5] ftrace: let function tracing and function return run together

[Steven Rostedt]

[-- Attachment #1: 0003-ftrace-let-function-tracing-and-function-return-run.patch --]
[-- Type: text/plain, Size: 2246 bytes --]

From: Steven Rostedt <rostedt@goodmis.org>

Impact: feature

This patch enables function tracing and function return to run together.
I've tested this by enabling the stack tracer and return tracer, where
both the function entry and function return are used together with
dynamic ftrace.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/ftrace.c |   22 ++--------------------
 1 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 5f7c864..cbf8b09 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -53,9 +53,6 @@ static int ftrace_pid_trace = -1;
 /* Quick disabling of function tracer. */
 int function_trace_stop;
 
-/* By default, current tracing type is normal tracing. */
-enum ftrace_tracing_type_t ftrace_tracing_type = FTRACE_TYPE_ENTER;
-
 /*
  * ftrace_disabled is set when an anomaly is discovered.
  * ftrace_disabled is much stronger than ftrace_enabled.
@@ -1576,15 +1573,9 @@ int register_ftrace_function(struct ftrace_ops *ops)
 
 	mutex_lock(&ftrace_sysctl_lock);
 
-	if (ftrace_tracing_type == FTRACE_TYPE_RETURN) {
-		ret = -EBUSY;
-		goto out;
-	}
-
 	ret = __register_ftrace_function(ops);
 	ftrace_startup(0);
 
-out:
 	mutex_unlock(&ftrace_sysctl_lock);
 	return ret;
 }
@@ -1731,23 +1722,16 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
 
 	mutex_lock(&ftrace_sysctl_lock);
 
-	/*
-	 * Don't launch return tracing if normal function
-	 * tracing is already running.
-	 */
-	if (ftrace_trace_function != ftrace_stub) {
-		ret = -EBUSY;
-		goto out;
-	}
 	atomic_inc(&ftrace_graph_active);
 	ret = start_graph_tracing();
 	if (ret) {
 		atomic_dec(&ftrace_graph_active);
 		goto out;
 	}
-	ftrace_tracing_type = FTRACE_TYPE_RETURN;
+
 	ftrace_graph_return = retfunc;
 	ftrace_graph_entry = entryfunc;
+
 	ftrace_startup(FTRACE_START_FUNC_RET);
 
 out:
@@ -1763,8 +1747,6 @@ void unregister_ftrace_graph(void)
 	ftrace_graph_return = (trace_func_graph_ret_t)ftrace_stub;
 	ftrace_graph_entry = (trace_func_graph_ent_t)ftrace_stub;
 	ftrace_shutdown(FTRACE_STOP_FUNC_RET);
-	/* Restore normal tracing type */
-	ftrace_tracing_type = FTRACE_TYPE_ENTER;
 
 	mutex_unlock(&ftrace_sysctl_lock);
 }
-- 
1.5.6.5

-- 

[PATCH 4/5] ftrace: add thread comm to function graph tracer

[Steven Rostedt]

[-- Attachment #1: 0004-ftrace-add-thread-comm-to-function-graph-tracer.patch --]
[-- Type: text/plain, Size: 2458 bytes --]

From: Steven Rostedt <rostedt@goodmis.org>

Impact: enhancement to function graph tracer

Export the trace_find_cmdline so the function graph tracer can
use it to print the comms of the threads.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/trace.c                 |    2 +-
 kernel/trace/trace.h                 |    1 +
 kernel/trace/trace_functions_graph.c |   21 ++++++++++++++++-----
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 9d5f7c9..5811e0a 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -804,7 +804,7 @@ static void trace_save_cmdline(struct task_struct *tsk)
 	spin_unlock(&trace_cmdline_lock);
 }
 
-static char *trace_find_cmdline(int pid)
+char *trace_find_cmdline(int pid)
 {
 	char *cmdline = "<...>";
 	unsigned map;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index ffe1bb1..7adacf3 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -450,6 +450,7 @@ struct tracer_switch_ops {
 	struct tracer_switch_ops	*next;
 };
 
+char *trace_find_cmdline(int pid);
 #endif /* CONFIG_CONTEXT_SWITCH_TRACER */
 
 #ifdef CONFIG_DYNAMIC_FTRACE
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index b6f0cc2..bbb81e7 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -32,29 +32,40 @@ static pid_t last_pid = -1;
 
 static int graph_trace_init(struct trace_array *tr)
 {
-	int cpu;
+	int cpu, ret;
+
 	for_each_online_cpu(cpu)
 		tracing_reset(tr, cpu);
 
-	return register_ftrace_graph(&trace_graph_return,
+	ret = register_ftrace_graph(&trace_graph_return,
 					&trace_graph_entry);
+	if (ret)
+		return ret;
+	tracing_start_cmdline_record();
+
+	return 0;
 }
 
 static void graph_trace_reset(struct trace_array *tr)
 {
-		unregister_ftrace_graph();
+	tracing_stop_cmdline_record();
+	unregister_ftrace_graph();
 }
 
 /* If the pid changed since the last trace, output this event */
 static int verif_pid(struct trace_seq *s, pid_t pid)
 {
+	char *comm;
+
 	if (last_pid != -1 && last_pid == pid)
 		return 1;
 
 	last_pid = pid;
-	return trace_seq_printf(s, "\n------------8<---------- thread %d"
+	comm = trace_find_cmdline(pid);
+
+	return trace_seq_printf(s, "\n------------8<---------- thread %s-%d"
 				    " ------------8<----------\n\n",
-				  pid);
+				    comm, pid);
 }
 
 static enum print_line_t
-- 
1.5.6.5

-- 

[PATCH 5/5] ftrace: add cpu annotation for function graph tracer

[Steven Rostedt]

[-- Attachment #1: 0005-ftrace-add-cpu-annotation-for-function-graph-tracer.patch --]
[-- Type: text/plain, Size: 4641 bytes --]

From: Steven Rostedt <rostedt@goodmis.org>

Impact: enhancement for function graph tracer

When run on a SMP box, the function graph tracer is confusing because
it shows the different CPUS as changes in the trace.

This patch adds the annotation of 'CPU[###]' where ### is a three digit
number. The output will look similar to this:

CPU[001]     dput() {
CPU[000] } 726
CPU[001]     } 487
CPU[000] do_softirq() {
CPU[001]   } 2221
CPU[000]   __do_softirq() {
CPU[000]     __local_bh_disable() {
CPU[001]   unroll_tree_refs() {
CPU[000]     } 569
CPU[001]   } 501
CPU[000]     rcu_process_callbacks() {
CPU[001]   kfree() {

What makes this nice is that now you can grep the file and produce
readable format for a particular CPU.

 # cat /debug/tracing/trace > /tmp/trace
 # grep '^CPU\[000\]' /tmp/trace > /tmp/trace0
 # grep '^CPU\[001\]' /tmp/trace > /tmp/trace1

Will give you:

 # head /tmp/trace0
CPU[000] ------------8<---------- thread sshd-3899 ------------8<----------
CPU[000]     inotify_dentry_parent_queue_event() {
CPU[000]     } 2531
CPU[000]     inotify_inode_queue_event() {
CPU[000]     } 505
CPU[000]   } 69626
CPU[000] } 73089
CPU[000] audit_syscall_exit() {
CPU[000]   path_put() {
CPU[000]     dput() {

 # head /tmp/trace1
CPU[001] ------------8<---------- thread pcscd-3446 ------------8<----------
CPU[001]               } 4186
CPU[001]               dput() {
CPU[001]               } 543
CPU[001]               vfs_permission() {
CPU[001]                 inode_permission() {
CPU[001]                   shmem_permission() {
CPU[001]                     generic_permission() {
CPU[001]                     } 501
CPU[001]                   } 2205

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/trace_functions_graph.c |   34 ++++++++++++++++++++++------------
 1 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index bbb81e7..d31d695 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -28,7 +28,7 @@ static struct tracer_flags tracer_flags = {
 };
 
 /* pid on the last trace processed */
-static pid_t last_pid = -1;
+static pid_t last_pid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 };
 
 static int graph_trace_init(struct trace_array *tr)
 {
@@ -53,29 +53,34 @@ static void graph_trace_reset(struct trace_array *tr)
 }
 
 /* If the pid changed since the last trace, output this event */
-static int verif_pid(struct trace_seq *s, pid_t pid)
+static int verif_pid(struct trace_seq *s, pid_t pid, int cpu)
 {
 	char *comm;
 
-	if (last_pid != -1 && last_pid == pid)
+	if (last_pid[cpu] != -1 && last_pid[cpu] == pid)
 		return 1;
 
-	last_pid = pid;
+	last_pid[cpu] = pid;
 	comm = trace_find_cmdline(pid);
 
-	return trace_seq_printf(s, "\n------------8<---------- thread %s-%d"
+	return trace_seq_printf(s, "\nCPU[%03d]"
+				    " ------------8<---------- thread %s-%d"
 				    " ------------8<----------\n\n",
-				    comm, pid);
+				    cpu, comm, pid);
 }
 
 static enum print_line_t
 print_graph_entry(struct ftrace_graph_ent *call, struct trace_seq *s,
-		struct trace_entry *ent)
+		  struct trace_entry *ent, int cpu)
 {
 	int i;
 	int ret;
 
-	if (!verif_pid(s, ent->pid))
+	if (!verif_pid(s, ent->pid, cpu))
+		return TRACE_TYPE_PARTIAL_LINE;
+
+	ret = trace_seq_printf(s, "CPU[%03d] ", cpu);
+	if (!ret)
 		return TRACE_TYPE_PARTIAL_LINE;
 
 	for (i = 0; i < call->depth * TRACE_GRAPH_INDENT; i++) {
@@ -96,12 +101,16 @@ print_graph_entry(struct ftrace_graph_ent *call, struct trace_seq *s,
 
 static enum print_line_t
 print_graph_return(struct ftrace_graph_ret *trace, struct trace_seq *s,
-		   struct trace_entry *ent)
+		   struct trace_entry *ent, int cpu)
 {
 	int i;
 	int ret;
 
-	if (!verif_pid(s, ent->pid))
+	if (!verif_pid(s, ent->pid, cpu))
+		return TRACE_TYPE_PARTIAL_LINE;
+
+	ret = trace_seq_printf(s, "CPU[%03d] ", cpu);
+	if (!ret)
 		return TRACE_TYPE_PARTIAL_LINE;
 
 	for (i = 0; i < trace->depth * TRACE_GRAPH_INDENT; i++) {
@@ -137,12 +146,13 @@ print_graph_function(struct trace_iterator *iter)
 	case TRACE_GRAPH_ENT: {
 		struct ftrace_graph_ent_entry *field;
 		trace_assign_type(field, entry);
-		return print_graph_entry(&field->graph_ent, s, entry);
+		return print_graph_entry(&field->graph_ent, s, entry,
+					 iter->cpu);
 	}
 	case TRACE_GRAPH_RET: {
 		struct ftrace_graph_ret_entry *field;
 		trace_assign_type(field, entry);
-		return print_graph_return(&field->ret, s, entry);
+		return print_graph_return(&field->ret, s, entry, iter->cpu);
 	}
 	default:
 		return TRACE_TYPE_UNHANDLED;
-- 
1.5.6.5

-- 

Re: [PATCH 1/5] ftrace: add function tracing to single thread

[Andrew Morton]

On Wed, 26 Nov 2008 00:16:23 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <srostedt@redhat.com>
> 
> Impact: feature to function trace a single thread
> 
> This patch adds the ability to function trace a single thread.
> The file:
> 
>   /debugfs/tracing/set_ftrace_pid
> 
> contains the pid to trace. Valid pids are any positive integer.
> Writing any negative number to this file will disable the pid
> tracing and the function tracer will go back to tracing all of
> threads.
> 
> This feature works with both static and dynamic function tracing.
> 
> ...
>
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -47,6 +47,9 @@
>  int ftrace_enabled __read_mostly;
>  static int last_ftrace_enabled;
>  
> +/* ftrace_pid_trace >= 0 will only trace threads with this pid */
> +static int ftrace_pid_trace = -1;
> +
>  /* Quick disabling of function tracer. */
>  int function_trace_stop;
>  
> @@ -61,6 +64,7 @@ static int ftrace_disabled __read_mostly;
>  
>  static DEFINE_SPINLOCK(ftrace_lock);
>  static DEFINE_MUTEX(ftrace_sysctl_lock);
> +static DEFINE_MUTEX(ftrace_start_lock);
>  
>  static struct ftrace_ops ftrace_list_end __read_mostly =
>  {
> @@ -70,6 +74,7 @@ static struct ftrace_ops ftrace_list_end __read_mostly =
>  static struct ftrace_ops *ftrace_list __read_mostly = &ftrace_list_end;
>  ftrace_func_t ftrace_trace_function __read_mostly = ftrace_stub;
>  ftrace_func_t __ftrace_trace_function __read_mostly = ftrace_stub;
> +ftrace_func_t ftrace_pid_function __read_mostly = ftrace_stub;
>  
>  static void ftrace_list_func(unsigned long ip, unsigned long parent_ip)
>  {
> @@ -86,6 +91,21 @@ static void ftrace_list_func(unsigned long ip, unsigned long parent_ip)
>  	};
>  }
>  
> +static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip)
> +{
> +	if (current->pid != ftrace_pid_trace)
> +		return;

What happened with this?

It would reeeeely help if the changelog was updated to cover such
frequently-occurring controversies as this.

> +#ifdef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
> +	ftrace_trace_function = func;
> +#else
> +	__ftrace_trace_function = func;
> +#endif

Pulling this assignment out into a helper fuction would clean things
up.  It happens at least twice.

>
> ...
>
> +static ssize_t
> +ftrace_pid_read(struct file *file, char __user *ubuf,
> +		       size_t cnt, loff_t *ppos)
> +{
> +	char buf[64];
> +	int r;
> +
> +	if (ftrace_pid_trace >= 0)
> +		r = sprintf(buf, "%u\n", ftrace_pid_trace);

ftrace_pid_trace is signed, and we print it as unsigned.  Can this be
improved?

> +	else
> +		r = sprintf(buf, "no pid\n");
> +
> +	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
> +}
> +
> +static ssize_t
> +ftrace_pid_write(struct file *filp, const char __user *ubuf,
> +		   size_t cnt, loff_t *ppos)
> +{
> +	char buf[64];
> +	long val;
> +	int ret;
> +
> +	if (cnt >= sizeof(buf))
> +		return -EINVAL;
> +
> +	if (copy_from_user(&buf, ubuf, cnt))
> +		return -EFAULT;
> +
> +	buf[cnt] = 0;

Might be able to use strncpy_from_user() here?

> +	ret = strict_strtol(buf, 10, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&ftrace_start_lock);
> +	if (ret < 0) {
> +		/* disable pid tracing */
> +		if (ftrace_pid_trace < 0)
> +			goto out;
> +		ftrace_pid_trace = -1;
> +
> +	} else {
> +
> +		if (ftrace_pid_trace == val)
> +			goto out;
> +
> +		ftrace_pid_trace = val;
> +	}
> +
> +	/* update the function call */
> +	ftrace_update_pid_func();
> +	ftrace_startup_enable(0);
> +
> + out:
> +	mutex_unlock(&ftrace_start_lock);
> +
> +	return cnt;
> +}
> +
> +static struct file_operations ftrace_pid_fops = {

const, please.

> +	.read = ftrace_pid_read,
> +	.write = ftrace_pid_write,
> +};
> +

Re: [PATCH 2/5] ftrace: use code patching for ftrace graph tracer

[Andrew Morton]

On Wed, 26 Nov 2008 00:16:24 -0500 Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org> wrote:

> From: Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org>
> 
> Impact: more efficient code for ftrace graph tracer
> 
> This patch uses the dynamic patching, when available, to patch
> the function graph code into the kernel.
> 
> This patch will ease the way for letting both function tracing
> and function graph tracing run together.
> 
> ...
>
> +static int ftrace_mod_jmp(unsigned long ip,
> +			  int old_offset, int new_offset)
> +{
> +	unsigned char code[MCOUNT_INSN_SIZE];
> +
> +	if (probe_kernel_read(code, (void *)ip, MCOUNT_INSN_SIZE))
> +		return -EFAULT;
> +
> +	if (code[0] != 0xe9 || old_offset != *(int *)(&code[1]))

erk.  I suspect that there's a nicer way of doing this amongst our
forest of get_unaligned_foo() interfaces.  Harvey will know.

> +		return -EINVAL;
> +
> +	*(int *)(&code[1]) = new_offset;

Might be able to use put_unaligned_foo() here.

The problem is that these functions use sizeof(*ptr) to work out what
to do, so a cast is still needed.  A get_unaligned32(ptr) would be
nice.  One which takes a void* and assumes CPU ordering.

> +	if (do_ftrace_mod_code(ip, &code))
> +		return -EPERM;
> +
> +	return 0;
> +}
> +

Re: [PATCH 4/5] ftrace: add thread comm to function graph tracer

[Andrew Morton]

On Wed, 26 Nov 2008 00:16:26 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <rostedt@goodmis.org>
> 
> Impact: enhancement to function graph tracer
> 
> Export the trace_find_cmdline so the function graph tracer can
> use it to print the comms of the threads.
> 
> -static char *trace_find_cmdline(int pid)
> +char *trace_find_cmdline(int pid)
>
> ...
>
>  static int verif_pid(struct trace_seq *s, pid_t pid)
>  {
> +	char *comm;
> +
>  	if (last_pid != -1 && last_pid == pid)
>  		return 1;
>  
>  	last_pid = pid;
> -	return trace_seq_printf(s, "\n------------8<---------- thread %d"
> +	comm = trace_find_cmdline(pid);
> +
> +	return trace_seq_printf(s, "\n------------8<---------- thread %s-%d"
>  				    " ------------8<----------\n\n",
> -				  pid);
> +				    comm, pid);
>  }

This code gets its int's and pid_t's all mixed up.  It's a bit cosmetic, but
nice to get it right for readability's sake.

Re: [PATCH 5/5] ftrace: add cpu annotation for function graph tracer

[Andrew Morton]

On Wed, 26 Nov 2008 00:16:27 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:

> This patch adds the annotation of 'CPU[###]' where ### is a three digit
> number

Is this a plot to get yourself sent a 1024-way test box?

Re: [PATCH 5/5] ftrace: add cpu annotation for function graph tracer

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 26 Nov 2008 00:16:27 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > This patch adds the annotation of 'CPU[###]' where ### is a three 
> > digit number
> 
> Is this a plot to get yourself sent a 1024-way test box?

So you are suggesting to change it to 4 digit printout?

(Is this a plot to get yourself snt a 16384-way test box? ;-)

	Ingo

Re: [PATCH 5/5] ftrace: add cpu annotation for function graph tracer

[Andrew Morton]

On Wed, 26 Nov 2008 06:47:28 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Wed, 26 Nov 2008 00:16:27 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > This patch adds the annotation of 'CPU[###]' where ### is a three 
> > > digit number
> > 
> > Is this a plot to get yourself sent a 1024-way test box?
> 
> So you are suggesting to change it to 4 digit printout?

Not really.  I somehow doubt that it's economic to do your software tuning
on the 1024-way ;)

> (Is this a plot to get yourself snt a 16384-way test box? ;-)

I'd settle for 1.6384 testers.

Re: [PATCH 1/5] ftrace: add function tracing to single thread

[Eric W. Biederman]

Steven Rostedt <rostedt@goodmis.org> writes:

> From: Steven Rostedt <srostedt@redhat.com>
>
> Impact: feature to function trace a single thread
>
> This patch adds the ability to function trace a single thread.
> The file:
>
>   /debugfs/tracing/set_ftrace_pid
>
> contains the pid to trace. Valid pids are any positive integer.

Please look below.  Using find_get_pid causes this to use the
same logic as the rest of the kernel on what a valid pid is.
i.e. 0 is not a valid pid.

I believe I have sketched in what is needed to use the struct
pid api properly.  Certainly enough for you to get the jist
of the idea.

> Writing any negative number to this file will disable the pid
> tracing and the function tracer will go back to tracing all of
> threads.
>
> This feature works with both static and dynamic function tracing.
>
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  Documentation/ftrace.txt |   79 +++++++++++++++++
>  kernel/trace/ftrace.c    |  209 ++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 262 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/ftrace.txt b/Documentation/ftrace.txt
> index 35a78bc..de05042 100644
> --- a/Documentation/ftrace.txt
> +++ b/Documentation/ftrace.txt
> @@ -127,6 +127,8 @@ of ftrace. Here is a list of some of the key files:
>  		be traced. If a function exists in both set_ftrace_filter
>  		and set_ftrace_notrace,	the function will _not_ be traced.
>  
> +  set_ftrace_pid: Have the function tracer only trace a single thread.
> +
>    available_filter_functions: This lists the functions that ftrace
>  		has processed and can trace. These are the function
>  		names that you can pass to "set_ftrace_filter" or
> @@ -1073,6 +1075,83 @@ For simple one time traces, the above is sufficent. For
> anything else,
>  a search through /proc/mounts may be needed to find where the debugfs
>  file-system is mounted.
>  
> +
> +Single thread tracing
> +---------------------
> +
> +By writing into /debug/tracing/set_ftrace_pid you can trace a
> +single thread. For example:
> +
> +# cat /debug/tracing/set_ftrace_pid
> +no pid
> +# echo 3111 > /debug/tracing/set_ftrace_pid
> +# cat /debug/tracing/set_ftrace_pid
> +3111
> +# echo function > /debug/tracing/current_tracer
> +# cat /debug/tracing/trace | head
> + # tracer: function
> + #
> + #           TASK-PID    CPU#    TIMESTAMP  FUNCTION
> + #              | |       |          |         |
> +     yum-updatesd-3111  [003]  1637.254676: finish_task_switch <-thread_return
> + yum-updatesd-3111 [003] 1637.254681: hrtimer_cancel <-schedule_hrtimeout_range
> + yum-updatesd-3111 [003] 1637.254682: hrtimer_try_to_cancel <-hrtimer_cancel
> + yum-updatesd-3111 [003] 1637.254683: lock_hrtimer_base <-hrtimer_try_to_cancel
> +     yum-updatesd-3111  [003]  1637.254685: fget_light <-do_sys_poll
> +     yum-updatesd-3111  [003]  1637.254686: pipe_poll <-do_sys_poll
> +# echo -1 > /debug/tracing/set_ftrace_pid
> +# cat /debug/tracing/trace |head
> + # tracer: function
> + #
> + #           TASK-PID    CPU#    TIMESTAMP  FUNCTION
> + #              | |       |          |         |
> + ##### CPU 3 buffer started ####
> +     yum-updatesd-3111  [003]  1701.957688: free_poll_entry <-poll_freewait
> + yum-updatesd-3111 [003] 1701.957689: remove_wait_queue <-free_poll_entry
> +     yum-updatesd-3111  [003]  1701.957691: fput <-free_poll_entry
> +     yum-updatesd-3111  [003]  1701.957692: audit_syscall_exit <-sysret_audit
> +     yum-updatesd-3111  [003]  1701.957693: path_put <-audit_syscall_exit
> +
> +If you want to trace a function when executing, you could use
> +something like this simple program:
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +
> +int main (int argc, char **argv)
> +{
> +        if (argc < 1)
> +                exit(-1);
> +
> +        if (fork() > 0) {
> +                int fd, ffd;
> +                char line[64];
> +                int s;
> +
> +                ffd = open("/debug/tracing/current_tracer", O_WRONLY);
> +                if (ffd < 0)
> +                        exit(-1);
> +                write(ffd, "nop", 3);
> +
> +                fd = open("/debug/tracing/set_ftrace_pid", O_WRONLY);
> +                s = sprintf(line, "%d\n", getpid());
> +                write(fd, line, s);
> +
> +                write(ffd, "function", 8);
> +
> +                close(fd);
> +                close(ffd);
> +
> +                execvp(argv[1], argv+1);
> +        }
> +
> +        return 0;
> +}
> +
>  dynamic ftrace
>  --------------
>  
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 7e2d3b9..00d98c6 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -47,6 +47,9 @@
>  int ftrace_enabled __read_mostly;
>  static int last_ftrace_enabled;
>  
> +/* ftrace_pid_trace >= 0 will only trace threads with this pid */
> +static int ftrace_pid_trace = -1;

Why not?
static struct pid *ftrace_pid_trace = NULL;
> +
>  /* Quick disabling of function tracer. */
>  int function_trace_stop;
>  
> @@ -61,6 +64,7 @@ static int ftrace_disabled __read_mostly;
>  
>  static DEFINE_SPINLOCK(ftrace_lock);
>  static DEFINE_MUTEX(ftrace_sysctl_lock);
> +static DEFINE_MUTEX(ftrace_start_lock);
>  
>  static struct ftrace_ops ftrace_list_end __read_mostly =
>  {
> @@ -70,6 +74,7 @@ static struct ftrace_ops ftrace_list_end __read_mostly =
>  static struct ftrace_ops *ftrace_list __read_mostly = &ftrace_list_end;
>  ftrace_func_t ftrace_trace_function __read_mostly = ftrace_stub;
>  ftrace_func_t __ftrace_trace_function __read_mostly = ftrace_stub;
> +ftrace_func_t ftrace_pid_function __read_mostly = ftrace_stub;
>  
>  static void ftrace_list_func(unsigned long ip, unsigned long parent_ip)
>  {
> @@ -86,6 +91,21 @@ static void ftrace_list_func(unsigned long ip, unsigned long
> parent_ip)
>  	};
>  }
>  
> +static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip)
> +{
> +	if (current->pid != ftrace_pid_trace)
> +		return;

And why not?
        if (task_pid(current) != ftrace_pid_trace)
        	return;

Racy or not.  This is only comparing a different field in struct task_struct.
So there should be not real difference.
> +
> +	ftrace_pid_function(ip, parent_ip);
> +}
> +
> +static void set_ftrace_pid_function(ftrace_func_t func)
> +{
> +	/* do not set ftrace_pid_function to itself! */
> +	if (func != ftrace_pid_func)
> +		ftrace_pid_function = func;
> +}
> +
>  /**
>   * clear_ftrace_function - reset the ftrace function
>   *
> @@ -96,6 +116,7 @@ void clear_ftrace_function(void)
>  {
>  	ftrace_trace_function = ftrace_stub;
>  	__ftrace_trace_function = ftrace_stub;
> +	ftrace_pid_function = ftrace_stub;
>  }
>  
>  #ifndef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
> @@ -128,20 +149,26 @@ static int __register_ftrace_function(struct ftrace_ops
> *ops)
>  	ftrace_list = ops;
>  
>  	if (ftrace_enabled) {
> +		ftrace_func_t func;
> +
> +		if (ops->next == &ftrace_list_end)
> +			func = ops->func;
> +		else
> +			func = ftrace_list_func;
> +
> +		if (ftrace_pid_trace >= 0) {
And why not:
		if (ftrace_pid_trace) {
> +			set_ftrace_pid_function(func);
> +			func = ftrace_pid_func;
> +		}
> +
>  		/*
>  		 * For one func, simply call it directly.
>  		 * For more than one func, call the chain.
>  		 */
>  #ifdef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
> -		if (ops->next == &ftrace_list_end)
> -			ftrace_trace_function = ops->func;
> -		else
> -			ftrace_trace_function = ftrace_list_func;
> +		ftrace_trace_function = func;
>  #else
> -		if (ops->next == &ftrace_list_end)
> -			__ftrace_trace_function = ops->func;
> -		else
> -			__ftrace_trace_function = ftrace_list_func;
> +		__ftrace_trace_function = func;
>  		ftrace_trace_function = ftrace_test_stop_func;
>  #endif
>  	}
> @@ -182,8 +209,19 @@ static int __unregister_ftrace_function(struct ftrace_ops
> *ops)
>  
>  	if (ftrace_enabled) {
>  		/* If we only have one func left, then call that directly */
> -		if (ftrace_list->next == &ftrace_list_end)
> -			ftrace_trace_function = ftrace_list->func;
> +		if (ftrace_list->next == &ftrace_list_end) {
> +			ftrace_func_t func = ftrace_list->func;
> +
> +			if (ftrace_pid_trace >= 0) {
And why not:
			if (ftrace_pid_trace) {
> +				set_ftrace_pid_function(func);
> +				func = ftrace_pid_func;
> +			}
> +#ifdef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
> +			ftrace_trace_function = func;
> +#else
> +			__ftrace_trace_function = func;
> +#endif
> +		}
>  	}
>  
>   out:
> @@ -192,6 +230,38 @@ static int __unregister_ftrace_function(struct ftrace_ops
> *ops)
>  	return ret;
>  }
>  
> +static void ftrace_update_pid_func(void)
> +{
> +	ftrace_func_t func;
> +
> +	/* should not be called from interrupt context */
> +	spin_lock(&ftrace_lock);
> +
> +	if (ftrace_trace_function == ftrace_stub)
> +		goto out;
> +
> +	func = ftrace_trace_function;
> +
> +	if (ftrace_pid_trace >= 0) {
And why not:
	if (ftrace_pid_trace) {
> +		set_ftrace_pid_function(func);
> +		func = ftrace_pid_func;
> +	} else {
> +		if (func != ftrace_pid_func)
> +			goto out;
> +
> +		set_ftrace_pid_function(func);
> +	}
> +
> +#ifdef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
> +	ftrace_trace_function = func;
> +#else
> +	__ftrace_trace_function = func;
> +#endif
> +
> + out:
> +	spin_unlock(&ftrace_lock);
> +}
> +
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  #ifndef CONFIG_FTRACE_MCOUNT_RECORD
>  # error Dynamic ftrace depends on MCOUNT_RECORD
> @@ -545,7 +615,19 @@ static void ftrace_run_update_code(int command)
>  
>  static ftrace_func_t saved_ftrace_func;
>  static int ftrace_start_up;
> -static DEFINE_MUTEX(ftrace_start_lock);
> +
> +static void ftrace_startup_enable(int command)
> +{
> +	if (saved_ftrace_func != ftrace_trace_function) {
> +		saved_ftrace_func = ftrace_trace_function;
> +		command |= FTRACE_UPDATE_TRACE_FUNC;
> +	}
> +
> +	if (!command || !ftrace_enabled)
> +		return;
> +
> +	ftrace_run_update_code(command);
> +}
>  
>  static void ftrace_startup(void)
>  {
> @@ -558,16 +640,8 @@ static void ftrace_startup(void)
>  	ftrace_start_up++;
>  	command |= FTRACE_ENABLE_CALLS;
>  
> -	if (saved_ftrace_func != ftrace_trace_function) {
> -		saved_ftrace_func = ftrace_trace_function;
> -		command |= FTRACE_UPDATE_TRACE_FUNC;
> -	}
> -
> -	if (!command || !ftrace_enabled)
> -		goto out;
> +	ftrace_startup_enable(command);
>  
> -	ftrace_run_update_code(command);
> - out:
>  	mutex_unlock(&ftrace_start_lock);
>  }
>  
> @@ -1262,13 +1336,10 @@ static struct file_operations ftrace_notrace_fops = {
>  	.release = ftrace_notrace_release,
>  };
>  
> -static __init int ftrace_init_debugfs(void)
> +static __init int ftrace_init_dyn_debugfs(struct dentry *d_tracer)
>  {
> -	struct dentry *d_tracer;
>  	struct dentry *entry;
>  
> -	d_tracer = tracing_init_dentry();
> -
>  	entry = debugfs_create_file("available_filter_functions", 0444,
>  				    d_tracer, NULL, &ftrace_avail_fops);
>  	if (!entry)
> @@ -1295,8 +1366,6 @@ static __init int ftrace_init_debugfs(void)
>  	return 0;
>  }
>  
> -fs_initcall(ftrace_init_debugfs);
> -
>  static int ftrace_convert_nops(struct module *mod,
>  			       unsigned long *start,
>  			       unsigned long *end)
> @@ -1382,12 +1451,100 @@ static int __init ftrace_nodyn_init(void)
>  }
>  device_initcall(ftrace_nodyn_init);
>  
> +static inline int ftrace_init_dyn_debugfs(struct dentry *d_tracer) { return 0;
> }
> +static inline void ftrace_startup_enable(int command) { }
>  # define ftrace_startup()		do { } while (0)
>  # define ftrace_shutdown()		do { } while (0)
>  # define ftrace_startup_sysctl()	do { } while (0)
>  # define ftrace_shutdown_sysctl()	do { } while (0)
>  #endif /* CONFIG_DYNAMIC_FTRACE */
>  
> +static ssize_t
> +ftrace_pid_read(struct file *file, char __user *ubuf,
> +		       size_t cnt, loff_t *ppos)
> +{
> +	char buf[64];
> +	int r;
> +
> +	if (ftrace_pid_trace >= 0)
> +		r = sprintf(buf, "%u\n", ftrace_pid_trace);
And why not:
    	if (ftrace_pid_trace)
		r = sprintf(buf, "%u\n", pid_vmr(ftrace_pid_trace))
> +	else
> +		r = sprintf(buf, "no pid\n");
> +
> +	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
> +}
> +
> +static ssize_t
> +ftrace_pid_write(struct file *filp, const char __user *ubuf,
> +		   size_t cnt, loff_t *ppos)
> +{
> +	char buf[64];
> +	long val;
> +	int ret;
> +
> +	if (cnt >= sizeof(buf))
> +		return -EINVAL;
> +
> +	if (copy_from_user(&buf, ubuf, cnt))
> +		return -EFAULT;
> +
> +	buf[cnt] = 0;
> +
> +	ret = strict_strtol(buf, 10, &val);
> +	if (ret < 0)
> +		return ret;

Why not?
	pid = find_get_pid(ret);
        mutex_lock(&ftrace_start_lock);
        if (pid == ftrace_pid_trace)
		goto out;

        struct pid *tmp;
        tmp = ftrace_pid_trace;
        ftrace_pid_trace = pid;
        pid = tmp;        

	/* update the function call */
	ftrace_update_pid_func();
	ftrace_startup_enable(0);
out:
	mutex_unlock(&ftrace_start_lock);
        put_pid(pid);

> +
> +	mutex_lock(&ftrace_start_lock);
> +	if (ret < 0) {
> +		/* disable pid tracing */
> +		if (ftrace_pid_trace < 0)
> +			goto out;
> +		ftrace_pid_trace = -1;
> +
> +	} else {
> +
> +		if (ftrace_pid_trace == val)
> +			goto out;
> +
> +		ftrace_pid_trace = val;
> +	}
> +
> +	/* update the function call */
> +	ftrace_update_pid_func();
> +	ftrace_startup_enable(0);
> +
> + out:
> +	mutex_unlock(&ftrace_start_lock);
> +
> +	return cnt;
> +}
> +
> +static struct file_operations ftrace_pid_fops = {
> +	.read = ftrace_pid_read,
> +	.write = ftrace_pid_write,
> +};
> +
> +static __init int ftrace_init_debugfs(void)
> +{
> +	struct dentry *d_tracer;
> +	struct dentry *entry;
> +
> +	d_tracer = tracing_init_dentry();
> +	if (!d_tracer)
> +		return 0;
> +
> +	ftrace_init_dyn_debugfs(d_tracer);
> +
> +	entry = debugfs_create_file("set_ftrace_pid", 0644, d_tracer,
> +				    NULL, &ftrace_pid_fops);
> +	if (!entry)
> +		pr_warning("Could not create debugfs "
> +			   "'set_ftrace_pid' entry\n");
> +	return 0;
> +}
> +
> +fs_initcall(ftrace_init_debugfs);
> +
>  /**
>   * ftrace_kill - kill ftrace
>   *
> -- 
> 1.5.6.5
>
> -- 

Re: [PATCH 2/5] ftrace: use code patching for ftrace graph tracer

[Harvey Harrison]

On Tue, 2008-11-25 at 21:35 -0800, Andrew Morton wrote:
> On Wed, 26 Nov 2008 00:16:24 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt <rostedt@goodmis.org>
> > 
> > Impact: more efficient code for ftrace graph tracer
> > 
> > This patch uses the dynamic patching, when available, to patch
> > the function graph code into the kernel.
> > 
> > This patch will ease the way for letting both function tracing
> > and function graph tracing run together.
> > 
> > ...
> >
> > +static int ftrace_mod_jmp(unsigned long ip,
> > +			  int old_offset, int new_offset)
> > +{
> > +	unsigned char code[MCOUNT_INSN_SIZE];
> > +
> > +	if (probe_kernel_read(code, (void *)ip, MCOUNT_INSN_SIZE))
> > +		return -EFAULT;
> > +
> > +	if (code[0] != 0xe9 || old_offset != *(int *)(&code[1]))
> 
> erk.  I suspect that there's a nicer way of doing this amongst our
> forest of get_unaligned_foo() interfaces.  Harvey will know.
> 

if (code[0] != 0xe9 || old_offset != get_unaligned((int *)(&code[1])))

> > +		return -EINVAL;
> > +
> > +	*(int *)(&code[1]) = new_offset;
> 
> Might be able to use put_unaligned_foo() here.
> 

	put_unaligned(new_offset, (int *)(&code[1]));

> The problem is that these functions use sizeof(*ptr) to work out what
> to do, so a cast is still needed.  A get_unaligned32(ptr) would be
> nice.  One which takes a void* and assumes CPU ordering.

I've been thinking similarly, I could investigate something that
goes in with the _noalign stuff?

I'll finish the documentation patch for the _noalign stuff and then see
about doing the host-order bits to fit in as well.

Harvey

Re: [PATCH 1/5] ftrace: add function tracing to single thread

[Ingo Molnar]

* Eric W. Biederman <ebiederm@xmission.com> wrote:

> > +/* ftrace_pid_trace >= 0 will only trace threads with this pid */
> > +static int ftrace_pid_trace = -1;
> 
> Why not?
> static struct pid *ftrace_pid_trace = NULL;

ok, looks reasonably simple at first sight.

Steve, mind sending the conversion of this facility to struct pid as a 
delta patch? (that would have education purposes as well, for similar 
situations)

	Ingo

Re: [PATCH 2/5] ftrace: use code patching for ftrace graph tracer

[Andrew Morton]

On Tue, 25 Nov 2008 22:52:29 -0800 Harvey Harrison <harvey.harrison-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, 2008-11-25 at 21:35 -0800, Andrew Morton wrote:
> > On Wed, 26 Nov 2008 00:16:24 -0500 Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org> wrote:
> > 
> > > From: Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org>
> > > 
> > > Impact: more efficient code for ftrace graph tracer
> > > 
> > > This patch uses the dynamic patching, when available, to patch
> > > the function graph code into the kernel.
> > > 
> > > This patch will ease the way for letting both function tracing
> > > and function graph tracing run together.
> > > 
> > > ...
> > >
> > > +static int ftrace_mod_jmp(unsigned long ip,
> > > +			  int old_offset, int new_offset)
> > > +{
> > > +	unsigned char code[MCOUNT_INSN_SIZE];
> > > +
> > > +	if (probe_kernel_read(code, (void *)ip, MCOUNT_INSN_SIZE))
> > > +		return -EFAULT;
> > > +
> > > +	if (code[0] != 0xe9 || old_offset != *(int *)(&code[1]))
> > 
> > erk.  I suspect that there's a nicer way of doing this amongst our
> > forest of get_unaligned_foo() interfaces.  Harvey will know.
> > 
> 
> if (code[0] != 0xe9 || old_offset != get_unaligned((int *)(&code[1])))

urgh, OK, that didn't really improve anything except to document
something which was already rather obvious.

> > > +		return -EINVAL;
> > > +
> > > +	*(int *)(&code[1]) = new_offset;
> > 
> > Might be able to use put_unaligned_foo() here.
> > 
> 
> 	put_unaligned(new_offset, (int *)(&code[1]));
> 
> > The problem is that these functions use sizeof(*ptr) to work out what
> > to do, so a cast is still needed.  A get_unaligned32(ptr) would be
> > nice.  One which takes a void* and assumes CPU ordering.
> 
> I've been thinking similarly, I could investigate something that
> goes in with the _noalign stuff?

If it's a commonly used pattern (you'd know better than I) then sure,
it would be clean to have

u32 just_gimme_the_u32_at(void *this_address);

> I'll finish the documentation patch for the _noalign stuff and then see
> about doing the host-order bits to fit in as well.
> 
> Harvey

Re: [PATCH 2/5] ftrace: use code patching for ftrace graph tracer

[Harvey Harrison]

On Wed, 2008-11-26 at 00:04 -0800, Andrew Morton wrote:
> On Tue, 25 Nov 2008 22:52:29 -0800 Harvey Harrison <harvey.harrison@gmail.com> wrote:
> > > > +	if (code[0] != 0xe9 || old_offset != *(int *)(&code[1]))
> > > 
> > > erk.  I suspect that there's a nicer way of doing this amongst our
> > > forest of get_unaligned_foo() interfaces.  Harvey will know.
> > > 
> > 
> > if (code[0] != 0xe9 || old_offset != get_unaligned((int *)(&code[1])))
> 
> urgh, OK, that didn't really improve anything except to document
> something which was already rather obvious.
> 

Yeah, it really isn't that great.  It could be somewhat nicer if we had
the API for host-endian taking void and being explicit about the size:

if (code[0] != 0xe9 || old_offset != load32_noalign(&code[1]))

This is similar to the new API in -mm load_le32_noalign, but I
don't think it would be worth load_u32_noalign...load32 should
be enough.

> > > > +		return -EINVAL;
> > > > +
> > > > +	*(int *)(&code[1]) = new_offset;
> > > 
> > > Might be able to use put_unaligned_foo() here.
> > > 
> > 
> > 	put_unaligned(new_offset, (int *)(&code[1]));
> > 

In a similar vein to above, this becomes:

	store32_noalign(&code[1], new_offset);

I rather like this as it would allow most (if not all) of the figure
out what size based on pointer type versions to be chucked.

Turns out all the pieces are there in -mm that the patch is pretty
trivial (not for application, just a heads-up):

 include/asm-generic/unaligned.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/include/asm-generic/unaligned.h b/include/asm-generic/unaligned.h
index d2f3998..5a055f7 100644
--- a/include/asm-generic/unaligned.h
+++ b/include/asm-generic/unaligned.h
@@ -343,9 +343,23 @@ extern void __bad_unaligned_access_size(void);
 #ifdef __LITTLE_ENDIAN
 # define get_unaligned __get_unaligned_le
 # define put_unaligned __put_unaligned_le
+
+# define load16_noalign(p) load_le16_noalign((void *)(p))
+# define load32_noalign(p) load_le32_noalign((void *)(p))
+# define load64_noalign(p) load_le64_noalign((void *)(p))
+# define store16_noalign(p, val) store_le16_noalign((void *)(p), (val))
+# define store32_noalign(p, val) store_le32_noalign((void *)(p), (val))
+# define store64_noalign(p, val) store_le64_noalign((void *)(p), (val))
 #else
 # define get_unaligned __get_unaligned_be
 # define put_unaligned __put_unaligned_be
+
+# define load16_noalign(p) load_be16_noalign((void *)(p))
+# define load32_noalign(p) load_be32_noalign((void *)(p))
+# define load64_noalign(p) load_be64_noalign((void *)(p))
+# define store16_noalign(p, val) store_be16_noalign((void *)(p), (val))
+# define store32_noalign(p, val) store_be32_noalign((void *)(p), (val))
+# define store64_noalign(p, val) store_be64_noalign((void *)(p), (val))
 #endif
 
 #endif /* _ASM_GENERIC_UNALIGNED_H */
-- 
1.6.0.4.1044.g77718

Re: [PATCH 2/5] ftrace: use code patching for ftrace graph tracer

[Andrew Morton]

On Wed, 26 Nov 2008 00:27:04 -0800 Harvey Harrison <harvey.harrison@gmail.com> wrote:

> if (code[0] != 0xe9 || old_offset != load32_noalign(&code[1]))
> 
> This is similar to the new API in -mm load_le32_noalign, but I
> don't think it would be worth load_u32_noalign...load32 should
> be enough.
> 
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	*(int *)(&code[1]) = new_offset;
> > > > 
> > > > Might be able to use put_unaligned_foo() here.
> > > > 
> > > 
> > > 	put_unaligned(new_offset, (int *)(&code[1]));
> > > 
> 
> In a similar vein to above, this becomes:
> 
> 	store32_noalign(&code[1], new_offset);
> 

yes, that's much better than the party tricks with magical sizeof,
which forces you to run around and check the types of everything.

I've seen people doing get_user() on `long' types and such things
occasionally.

Re: [PATCH 2/5] ftrace: use code patching for ftrace graph tracer

[Harvey Harrison]

On Wed, 2008-11-26 at 00:35 -0800, Andrew Morton wrote:
> On Wed, 26 Nov 2008 00:27:04 -0800 Harvey Harrison <harvey.harrison-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > if (code[0] != 0xe9 || old_offset != load32_noalign(&code[1]))
> > 
> > This is similar to the new API in -mm load_le32_noalign, but I
> > don't think it would be worth load_u32_noalign...load32 should
> > be enough.
> > 
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	*(int *)(&code[1]) = new_offset;
> > > > > 
> > > > > Might be able to use put_unaligned_foo() here.
> > > > > 
> > > > 
> > > > 	put_unaligned(new_offset, (int *)(&code[1]));
> > > > 
> > 
> > In a similar vein to above, this becomes:
> > 
> > 	store32_noalign(&code[1], new_offset);
> > 
> 
> yes, that's much better than the party tricks with magical sizeof,
> which forces you to run around and check the types of everything.
> 
> I've seen people doing get_user() on `long' types and such things
> occasionally.
> 

Do you want to carry the patches to move to the new helpers until they
hit mainline, or would you rather I waited until they can go through
maintainer trees?

I've got the pile ready removing all of the get/put_{endian} and moving
to the load/store API.  Not much left after that to just remove the
magical sizeof versions too.  Just let me know what timing you'd prefer.

Also, all of this ends up being so intertwined in the aligned/unaligned
cases that I'd like to move most of Documentation/unaligned_memory_access.txt
into a new alignment_and_byteorder.txt to cover all of these new helpers.

I started most of a byteorder document, but constantly referring to the other
file made it a bit tiresome, would you mind a consolidated document?

Harvey

Re: [PATCH 2/5] ftrace: use code patching for ftrace graph tracer

[Andrew Morton]

On Wed, 26 Nov 2008 00:44:48 -0800 Harvey Harrison <harvey.harrison-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, 2008-11-26 at 00:35 -0800, Andrew Morton wrote:
> > On Wed, 26 Nov 2008 00:27:04 -0800 Harvey Harrison <harvey.harrison-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > 
> > > if (code[0] != 0xe9 || old_offset != load32_noalign(&code[1]))
> > > 
> > > This is similar to the new API in -mm load_le32_noalign, but I
> > > don't think it would be worth load_u32_noalign...load32 should
> > > be enough.
> > > 
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	*(int *)(&code[1]) = new_offset;
> > > > > > 
> > > > > > Might be able to use put_unaligned_foo() here.
> > > > > > 
> > > > > 
> > > > > 	put_unaligned(new_offset, (int *)(&code[1]));
> > > > > 
> > > 
> > > In a similar vein to above, this becomes:
> > > 
> > > 	store32_noalign(&code[1], new_offset);
> > > 
> > 
> > yes, that's much better than the party tricks with magical sizeof,
> > which forces you to run around and check the types of everything.
> > 
> > I've seen people doing get_user() on `long' types and such things
> > occasionally.
> > 
> 
> Do you want to carry the patches to move to the new helpers until they
> hit mainline, or would you rather I waited until they can go through
> maintainer trees?

I don't know what that means, really.

> I've got the pile ready removing all of the get/put_{endian} and moving
> to the load/store API.  Not much left after that to just remove the
> magical sizeof versions too.  Just let me know what timing you'd prefer.

I don't understand the dependencies and relationships here.

If you have a bunch of patches which are applicable to current mainline
then just spray 'em out with suitable cc's and we'll see which bits
stick where.  Or is it more complicated than that?

> Also, all of this ends up being so intertwined in the aligned/unaligned
> cases that I'd like to move most of Documentation/unaligned_memory_access.txt
> into a new alignment_and_byteorder.txt to cover all of these new helpers.
> 
> I started most of a byteorder document, but constantly referring to the other
> file made it a bit tiresome, would you mind a consolidated document?

Whatever you think is best.  Propose something...

Re: [PATCH 2/5] ftrace: use code patching for ftrace graph tracer

[Harvey Harrison]

On Wed, 2008-11-26 at 01:05 -0800, Andrew Morton wrote:
> > 
> > Do you want to carry the patches to move to the new helpers until they
> > hit mainline, or would you rather I waited until they can go through
> > maintainer trees?
> 
> I don't know what that means, really.
> 
> > I've got the pile ready removing all of the get/put_{endian} and moving
> > to the load/store API.  Not much left after that to just remove the
> > magical sizeof versions too.  Just let me know what timing you'd prefer.
> 
> I don't understand the dependencies and relationships here.

The pile I refer to depends on the full set of patches in -mm which
includes (I can give all the patch names if you want):

1) byteorder patches moving all remaining arches to use
include/linux/byteorder.h

2) unaligned access patches consolidating arches in
asm-generic/unaligned.h with no changes

3) move the memset-using arches and ARM to the asm-generic version
by moving __packed onto the struct rather than the struct members

4) introduce the load/store_{endian} API

Timing:

1) - 2) I hope all gets into mainline in 2.6.29. 

3) I believe could go in 2.6.29, and I'm pretty confident it is OK after
talking to some compiler folks.

4) can go in with 3)

My pile is just removing users of get_unaligned/put_unaligned/{endian}_to_cpup
and using the new typesafe versions.  At the end of the series we remove the old API.

> 
> If you have a bunch of patches which are applicable to current mainline
> then just spray 'em out with suitable cc's and we'll see which bits
> stick where.  Or is it more complicated than that?

See above, not suitable for mainline.

> 
> > Also, all of this ends up being so intertwined in the aligned/unaligned
> > cases that I'd like to move most of Documentation/unaligned_memory_access.txt
> > into a new alignment_and_byteorder.txt to cover all of these new helpers.
> > 
> > I started most of a byteorder document, but constantly referring to the other
> > file made it a bit tiresome, would you mind a consolidated document?
> 
> Whatever you think is best.  Propose something...

Thinking some more, I'll just dump the new endian docs into unaligned_access.txt
and that at least gets the documentation out there.  Later on it be renamed to
something more reflective of its contents.

Harvey

Re: [PATCH 1/5] ftrace: add function tracing to single thread

[Steven Rostedt]

On Tue, 25 Nov 2008, Andrew Morton wrote:
> On Wed, 26 Nov 2008 00:16:23 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
> >  
> > +static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip)
> > +{
> > +	if (current->pid != ftrace_pid_trace)
> > +		return;
> 
> What happened with this?
> 
> It would reeeeely help if the changelog was updated to cover such
> frequently-occurring controversies as this.

I just posted this again because Ingo did not pull it the first time.
This patch did not change (nor did the change log) from my first
posting, except that I rebased it on top of tip.

I would like to add new patches to solve this controversy. This way
I can focus on the approach without cluttering up the patch itself.
Also, this way works for the cases I currently care about, and should not 
break any other case. That is, the side effect of not selecting the right
pid is that we either trace a process we do not want to trace, or we
do not trace a process we want to trace. Nothing that will bring down
the system ;-)

> 
> > +#ifdef CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST
> > +	ftrace_trace_function = func;
> > +#else
> > +	__ftrace_trace_function = func;
> > +#endif
> 
> Pulling this assignment out into a helper fuction would clean things
> up.  It happens at least twice.

Yes, I agree. I want to get this over to my PPC box where it does not
have the "CONFIG_HAVE_FUNCTION_TRACE_MCOUNT_TEST" set. This way I can test 
all cases. And yes, I want to make a wrapper function for that too.

> 
> >
> > ...
> >
> > +static ssize_t
> > +ftrace_pid_read(struct file *file, char __user *ubuf,
> > +		       size_t cnt, loff_t *ppos)
> > +{
> > +	char buf[64];
> > +	int r;
> > +
> > +	if (ftrace_pid_trace >= 0)
> > +		r = sprintf(buf, "%u\n", ftrace_pid_trace);
> 
> ftrace_pid_trace is signed, and we print it as unsigned.  Can this be
> improved?

We only print it if it is greater than or equal to 0. Does this matter?
It needs to be signed, because we print "no pid" when negative.

> 
> > +	else
> > +		r = sprintf(buf, "no pid\n");
> > +
> > +	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
> > +}
> > +
> > +static ssize_t
> > +ftrace_pid_write(struct file *filp, const char __user *ubuf,
> > +		   size_t cnt, loff_t *ppos)
> > +{
> > +	char buf[64];
> > +	long val;
> > +	int ret;
> > +
> > +	if (cnt >= sizeof(buf))
> > +		return -EINVAL;
> > +
> > +	if (copy_from_user(&buf, ubuf, cnt))
> > +		return -EFAULT;
> > +
> > +	buf[cnt] = 0;
> 
> Might be able to use strncpy_from_user() here?

We are reading a number. But we might later add a string. The amount to be
read has a limit. Should I switch?

> 
> > +	ret = strict_strtol(buf, 10, &val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mutex_lock(&ftrace_start_lock);
> > +	if (ret < 0) {
> > +		/* disable pid tracing */
> > +		if (ftrace_pid_trace < 0)
> > +			goto out;
> > +		ftrace_pid_trace = -1;
> > +
> > +	} else {
> > +
> > +		if (ftrace_pid_trace == val)
> > +			goto out;
> > +
> > +		ftrace_pid_trace = val;
> > +	}
> > +
> > +	/* update the function call */
> > +	ftrace_update_pid_func();
> > +	ftrace_startup_enable(0);
> > +
> > + out:
> > +	mutex_unlock(&ftrace_start_lock);
> > +
> > +	return cnt;
> > +}
> > +
> > +static struct file_operations ftrace_pid_fops = {
> 
> const, please.

Ouch, I cut and pasted. I need to fix all of them.

Thanks,

-- Steve

> 
> > +	.read = ftrace_pid_read,
> > +	.write = ftrace_pid_write,
> > +};
> > +
> 
> 
> 

Re: [PATCH 2/5] ftrace: use code patching for ftrace graph tracer

[Steven Rostedt]

On Tue, 25 Nov 2008, Andrew Morton wrote:

> On Wed, 26 Nov 2008 00:16:24 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt <rostedt@goodmis.org>
> > 
> > Impact: more efficient code for ftrace graph tracer
> > 
> > This patch uses the dynamic patching, when available, to patch
> > the function graph code into the kernel.
> > 
> > This patch will ease the way for letting both function tracing
> > and function graph tracing run together.
> > 
> > ...
> >
> > +static int ftrace_mod_jmp(unsigned long ip,
> > +			  int old_offset, int new_offset)
> > +{
> > +	unsigned char code[MCOUNT_INSN_SIZE];
> > +
> > +	if (probe_kernel_read(code, (void *)ip, MCOUNT_INSN_SIZE))
> > +		return -EFAULT;
> > +
> > +	if (code[0] != 0xe9 || old_offset != *(int *)(&code[1]))
> 
> erk.  I suspect that there's a nicer way of doing this amongst our
> forest of get_unaligned_foo() interfaces.  Harvey will know.

Hmm, I may be able to make a struct out of code.

  struct {
	unsigned char op;
	unsigned int  offset;
  } code __attribute__((packed));

Would that look better?

> 
> > +		return -EINVAL;
> > +
> > +	*(int *)(&code[1]) = new_offset;
> 
> Might be able to use put_unaligned_foo() here.
> 
> The problem is that these functions use sizeof(*ptr) to work out what
> to do, so a cast is still needed.  A get_unaligned32(ptr) would be
> nice.  One which takes a void* and assumes CPU ordering.

Is there a correctness concern here? This is arch specific code, so I'm
not worried about other archs.

-- Steve

> 
> > +	if (do_ftrace_mod_code(ip, &code))
> > +		return -EPERM;
> > +
> > +	return 0;
> > +}
> > +
> 
> 
> 

Re: [PATCH 4/5] ftrace: add thread comm to function graph tracer

[Steven Rostedt]

On Tue, 25 Nov 2008, Andrew Morton wrote:

> On Wed, 26 Nov 2008 00:16:26 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt <rostedt@goodmis.org>
> > 
> > Impact: enhancement to function graph tracer
> > 
> > Export the trace_find_cmdline so the function graph tracer can
> > use it to print the comms of the threads.
> > 
> > -static char *trace_find_cmdline(int pid)
> > +char *trace_find_cmdline(int pid)
> >
> > ...
> >
> >  static int verif_pid(struct trace_seq *s, pid_t pid)
> >  {
> > +	char *comm;
> > +
> >  	if (last_pid != -1 && last_pid == pid)
> >  		return 1;
> >  
> >  	last_pid = pid;
> > -	return trace_seq_printf(s, "\n------------8<---------- thread %d"
> > +	comm = trace_find_cmdline(pid);
> > +
> > +	return trace_seq_printf(s, "\n------------8<---------- thread %s-%d"
> >  				    " ------------8<----------\n\n",
> > -				  pid);
> > +				    comm, pid);
> >  }
> 
> This code gets its int's and pid_t's all mixed up.  It's a bit cosmetic, but
> nice to get it right for readability's sake.

Hmm, I think I would like to keep all pids as ints. Perhaps because we do 
not have namespaces here ;-)

I'm totally confused by what to do. I'll have to think about it.

-- Steve

Re: [PATCH 5/5] ftrace: add cpu annotation for function graph tracer

[Steven Rostedt]

On Tue, 25 Nov 2008, Andrew Morton wrote:

> On Wed, 26 Nov 2008 00:16:27 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > This patch adds the annotation of 'CPU[###]' where ### is a three digit
> > number
> 
> Is this a plot to get yourself sent a 1024-way test box?


Have an extra one?

;)

-- Steve

Re: [PATCH 1/5] ftrace: add function tracing to single thread

[Steven Rostedt]

On Tue, 25 Nov 2008, Eric W. Biederman wrote:

> Steven Rostedt <rostedt@goodmis.org> writes:
> 
> > From: Steven Rostedt <srostedt@redhat.com>
> >
> > Impact: feature to function trace a single thread
> >
> > This patch adds the ability to function trace a single thread.
> > The file:
> >
> >   /debugfs/tracing/set_ftrace_pid
> >
> > contains the pid to trace. Valid pids are any positive integer.
> 
> Please look below.  Using find_get_pid causes this to use the
> same logic as the rest of the kernel on what a valid pid is.
> i.e. 0 is not a valid pid.

0 is not? What happens if I want to trace what the swapper task is doing?
I want the ability to trace kernel threads as well.

> 
> I believe I have sketched in what is needed to use the struct
> pid api properly.  Certainly enough for you to get the jist
> of the idea.
> 
> > Writing any negative number to this file will disable the pid
> > tracing and the function tracer will go back to tracing all of
> > threads.
> >
> > This feature works with both static and dynamic function tracing.
> >
> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> > ---
> >  Documentation/ftrace.txt |   79 +++++++++++++++++
> >  kernel/trace/ftrace.c    |  209 ++++++++++++++++++++++++++++++++++++++++------
> >  2 files changed, 262 insertions(+), 26 deletions(-)
> >
> > diff --git a/Documentation/ftrace.txt b/Documentation/ftrace.txt
> > index 35a78bc..de05042 100644
> > --- a/Documentation/ftrace.txt
> > +++ b/Documentation/ftrace.txt
> > @@ -127,6 +127,8 @@ of ftrace. Here is a list of some of the key files:
> >  		be traced. If a function exists in both set_ftrace_filter
> >  		and set_ftrace_notrace,	the function will _not_ be traced.
> >  
> > +  set_ftrace_pid: Have the function tracer only trace a single thread.
> > +
> >    available_filter_functions: This lists the functions that ftrace
> >  		has processed and can trace. These are the function
> >  		names that you can pass to "set_ftrace_filter" or
> > @@ -1073,6 +1075,83 @@ For simple one time traces, the above is sufficent. For
> > anything else,
> >  a search through /proc/mounts may be needed to find where the debugfs
> >  file-system is mounted.
> >  
> > +
> > +Single thread tracing
> > +---------------------
> > +
> > +By writing into /debug/tracing/set_ftrace_pid you can trace a
> > +single thread. For example:
> > +
> > +# cat /debug/tracing/set_ftrace_pid
> > +no pid
> > +# echo 3111 > /debug/tracing/set_ftrace_pid
> > +# cat /debug/tracing/set_ftrace_pid
> > +3111
> > +# echo function > /debug/tracing/current_tracer
> > +# cat /debug/tracing/trace | head
> > + # tracer: function
> > + #
> > + #           TASK-PID    CPU#    TIMESTAMP  FUNCTION
> > + #              | |       |          |         |
> > +     yum-updatesd-3111  [003]  1637.254676: finish_task_switch <-thread_return
> > + yum-updatesd-3111 [003] 1637.254681: hrtimer_cancel <-schedule_hrtimeout_range
> > + yum-updatesd-3111 [003] 1637.254682: hrtimer_try_to_cancel <-hrtimer_cancel
> > + yum-updatesd-3111 [003] 1637.254683: lock_hrtimer_base <-hrtimer_try_to_cancel
> > +     yum-updatesd-3111  [003]  1637.254685: fget_light <-do_sys_poll
> > +     yum-updatesd-3111  [003]  1637.254686: pipe_poll <-do_sys_poll
> > +# echo -1 > /debug/tracing/set_ftrace_pid
> > +# cat /debug/tracing/trace |head
> > + # tracer: function
> > + #
> > + #           TASK-PID    CPU#    TIMESTAMP  FUNCTION
> > + #              | |       |          |         |
> > + ##### CPU 3 buffer started ####
> > +     yum-updatesd-3111  [003]  1701.957688: free_poll_entry <-poll_freewait
> > + yum-updatesd-3111 [003] 1701.957689: remove_wait_queue <-free_poll_entry
> > +     yum-updatesd-3111  [003]  1701.957691: fput <-free_poll_entry
> > +     yum-updatesd-3111  [003]  1701.957692: audit_syscall_exit <-sysret_audit
> > +     yum-updatesd-3111  [003]  1701.957693: path_put <-audit_syscall_exit
> > +
> > +If you want to trace a function when executing, you could use
> > +something like this simple program:
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +
> > +int main (int argc, char **argv)
> > +{
> > +        if (argc < 1)
> > +                exit(-1);
> > +
> > +        if (fork() > 0) {
> > +                int fd, ffd;
> > +                char line[64];
> > +                int s;
> > +
> > +                ffd = open("/debug/tracing/current_tracer", O_WRONLY);
> > +                if (ffd < 0)
> > +                        exit(-1);
> > +                write(ffd, "nop", 3);
> > +
> > +                fd = open("/debug/tracing/set_ftrace_pid", O_WRONLY);
> > +                s = sprintf(line, "%d\n", getpid());
> > +                write(fd, line, s);
> > +
> > +                write(ffd, "function", 8);
> > +
> > +                close(fd);
> > +                close(ffd);
> > +
> > +                execvp(argv[1], argv+1);
> > +        }
> > +
> > +        return 0;
> > +}
> > +
> >  dynamic ftrace
> >  --------------
> >  
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 7e2d3b9..00d98c6 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -47,6 +47,9 @@
> >  int ftrace_enabled __read_mostly;
> >  static int last_ftrace_enabled;
> >  
> > +/* ftrace_pid_trace >= 0 will only trace threads with this pid */
> > +static int ftrace_pid_trace = -1;
> 
> Why not?
> static struct pid *ftrace_pid_trace = NULL;
> > +
> >  /* Quick disabling of function tracer. */
> >  int function_trace_stop;
> >  
> > @@ -61,6 +64,7 @@ static int ftrace_disabled __read_mostly;
> >  
> >  static DEFINE_SPINLOCK(ftrace_lock);
> >  static DEFINE_MUTEX(ftrace_sysctl_lock);
> > +static DEFINE_MUTEX(ftrace_start_lock);
> >  
> >  static struct ftrace_ops ftrace_list_end __read_mostly =
> >  {
> > @@ -70,6 +74,7 @@ static struct ftrace_ops ftrace_list_end __read_mostly =
> >  static struct ftrace_ops *ftrace_list __read_mostly = &ftrace_list_end;
> >  ftrace_func_t ftrace_trace_function __read_mostly = ftrace_stub;
> >  ftrace_func_t __ftrace_trace_function __read_mostly = ftrace_stub;
> > +ftrace_func_t ftrace_pid_function __read_mostly = ftrace_stub;
> >  
> >  static void ftrace_list_func(unsigned long ip, unsigned long parent_ip)
> >  {
> > @@ -86,6 +91,21 @@ static void ftrace_list_func(unsigned long ip, unsigned long
> > parent_ip)
> >  	};
> >  }
> >  
> > +static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip)
> > +{
> > +	if (current->pid != ftrace_pid_trace)
> > +		return;
> 
> And why not?
>         if (task_pid(current) != ftrace_pid_trace)
>         	return;
> 
> Racy or not.  This is only comparing a different field in struct task_struct.
> So there should be not real difference.

There's no race here. Do kernel threads have their own unique "task_pid"s?

I understand where you are going with this, and I have no issue with it. 
As long as I can also pick kernel threads to trace including the idle 
thread.

Thanks,

-- Steve

Re: [PATCH 1/5] ftrace: add function tracing to single thread

[Steven Rostedt]

On Wed, 26 Nov 2008, Ingo Molnar wrote:

> 
> * Eric W. Biederman <ebiederm@xmission.com> wrote:
> 
> > > +/* ftrace_pid_trace >= 0 will only trace threads with this pid */
> > > +static int ftrace_pid_trace = -1;
> > 
> > Why not?
> > static struct pid *ftrace_pid_trace = NULL;
> 
> ok, looks reasonably simple at first sight.
> 
> Steve, mind sending the conversion of this facility to struct pid as a 
> delta patch? (that would have education purposes as well, for similar 
> situations)

If I can trace specific kernel threads too, then I have no problem with 
it.

-- Steve

Re: [PATCH 2/5] ftrace: use code patching for ftrace graph tracer

[Andrew Morton]

On Wed, 26 Nov 2008 11:46:58 -0500 (EST) Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> 
> On Tue, 25 Nov 2008, Andrew Morton wrote:
> 
> > On Wed, 26 Nov 2008 00:16:24 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > From: Steven Rostedt <rostedt@goodmis.org>
> > > 
> > > Impact: more efficient code for ftrace graph tracer
> > > 
> > > This patch uses the dynamic patching, when available, to patch
> > > the function graph code into the kernel.
> > > 
> > > This patch will ease the way for letting both function tracing
> > > and function graph tracing run together.
> > > 
> > > ...
> > >
> > > +static int ftrace_mod_jmp(unsigned long ip,
> > > +			  int old_offset, int new_offset)
> > > +{
> > > +	unsigned char code[MCOUNT_INSN_SIZE];
> > > +
> > > +	if (probe_kernel_read(code, (void *)ip, MCOUNT_INSN_SIZE))
> > > +		return -EFAULT;
> > > +
> > > +	if (code[0] != 0xe9 || old_offset != *(int *)(&code[1]))
> > 
> > erk.  I suspect that there's a nicer way of doing this amongst our
> > forest of get_unaligned_foo() interfaces.  Harvey will know.
> 
> Hmm, I may be able to make a struct out of code.
> 
>   struct {
> 	unsigned char op;
> 	unsigned int  offset;
>   } code __attribute__((packed));
> 
> Would that look better?

nah, let's do something more generic for this.

> > 
> > > +		return -EINVAL;
> > > +
> > > +	*(int *)(&code[1]) = new_offset;
> > 
> > Might be able to use put_unaligned_foo() here.
> > 
> > The problem is that these functions use sizeof(*ptr) to work out what
> > to do, so a cast is still needed.  A get_unaligned32(ptr) would be
> > nice.  One which takes a void* and assumes CPU ordering.
> 
> Is there a correctness concern here? This is arch specific code, so I'm
> not worried about other archs.

No, the code is OK as-is.

It's just that "read a word from an [maybe-]unaligned address" is such
a common operation that there should be a nice clean simple function to
do it, rather than doing open-coded (and different) weird C tricks at each
and\ every site.

Re: [PATCH 4/5] ftrace: add thread comm to function graph tracer

[Andrew Morton]

On Wed, 26 Nov 2008 11:48:44 -0500 (EST) Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> On Tue, 25 Nov 2008, Andrew Morton wrote:
> 
> > On Wed, 26 Nov 2008 00:16:26 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > From: Steven Rostedt <rostedt@goodmis.org>
> > > 
> > > Impact: enhancement to function graph tracer
> > > 
> > > Export the trace_find_cmdline so the function graph tracer can
> > > use it to print the comms of the threads.
> > > 
> > > -static char *trace_find_cmdline(int pid)
> > > +char *trace_find_cmdline(int pid)
> > >
> > > ...
> > >
> > >  static int verif_pid(struct trace_seq *s, pid_t pid)
> > >  {
> > > +	char *comm;
> > > +
> > >  	if (last_pid != -1 && last_pid == pid)
> > >  		return 1;
> > >  
> > >  	last_pid = pid;
> > > -	return trace_seq_printf(s, "\n------------8<---------- thread %d"
> > > +	comm = trace_find_cmdline(pid);
> > > +
> > > +	return trace_seq_printf(s, "\n------------8<---------- thread %s-%d"
> > >  				    " ------------8<----------\n\n",
> > > -				  pid);
> > > +				    comm, pid);
> > >  }
> > 
> > This code gets its int's and pid_t's all mixed up.  It's a bit cosmetic, but
> > nice to get it right for readability's sake.
> 
> Hmm, I think I would like to keep all pids as ints. Perhaps because we do 
> not have namespaces here ;-)
> 
> I'm totally confused by what to do. I'll have to think about it.

Well.  Any variable which contains a process ID should have type pid_t.

It gets more complicated in places where these values are passed to and
from userspace (including via printk!).  It would be defensive (but
unnecessary) to cast the pid_t's to and from known-size types at the
kernel boundaries.

Re: [PATCH 2/5] ftrace: use code patching for ftrace graph tracer

[Harvey Harrison]

On Wed, 2008-11-26 at 10:02 -0800, Andrew Morton wrote:
> > > 
> > > > +		return -EINVAL;
> > > > +
> > > > +	*(int *)(&code[1]) = new_offset;
> > > 
> > > Might be able to use put_unaligned_foo() here.
> > > 
> > > The problem is that these functions use sizeof(*ptr) to work out what
> > > to do, so a cast is still needed.  A get_unaligned32(ptr) would be
> > > nice.  One which takes a void* and assumes CPU ordering.
> > 
> > Is there a correctness concern here? This is arch specific code, so I'm
> > not worried about other archs.
> 
> No, the code is OK as-is.
> 
> It's just that "read a word from an [maybe-]unaligned address" is such
> a common operation that there should be a nice clean simple function to
> do it, rather than doing open-coded (and different) weird C tricks at each
> and\ every site.
> 

Also it is arch-specific, so if you know unaligned access is OK, just doing the
cast+deref is OK.

Harvey