ftrace: updates for tip

(quoted diff) +2 -0

+	return 0;
+}

kernel/trace/ftrace.c static struct file_operations ftrace_notrace_fops = { · line 1370

Quoted 11 lines · On Wed, 03 Dec 2008 15:36:57 -0500 Steven Rostedt <[email protected]> wrote:

kernel/trace/ftrace.c +11 -0

+static int g_show(struct seq_file *m, void *v)
+{
+	unsigned long *ptr = v;
+	char str[KSYM_SYMBOL_LEN];
+
+	if (!ptr)
+		return 0;
+
+	kallsyms_lookup(*ptr, NULL, NULL, NULL, str);
+
+	seq_printf(m, "%s\n", str);

Can we use %pF here?

Quoted 16 lines · Andrew Morton

kernel/trace/ftrace.c static struct file_operations ftrace_notrace_fops = { · line 1370

Quoted 11 lines · On Wed, 03 Dec 2008 15:36:57 -0500 Steven Rostedt <[email protected]> wrote:

kernel/trace/ftrace.c +11 -0

+static int g_show(struct seq_file *m, void *v)
+{
+	unsigned long *ptr = v;
+	char str[KSYM_SYMBOL_LEN];
+
+	if (!ptr)
+		return 0;
+
+	kallsyms_lookup(*ptr, NULL, NULL, NULL, str);
+
+	seq_printf(m, "%s\n", str);

Can we use %pF here?

If there's a way to not print the "+offset".

blk_unplug looks much nicer than blk_unplug+0x78/0x278

Quoted 6 lines · On Wed, 3 Dec 2008 16:10:38 -0500 (EST) Steven Rostedt <[email protected]>…

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	seq_printf(m, "%s\n", str);

Can we use %pF here?

If there's a way to not print the "+offset".

Could be added, I guess. I wonder if it would be
reused elsewhere.

Quoted 2 lines

blk_unplug looks much nicer than blk_unplug+0x78/0x278

<mutters something about in-kernel pretty-printers>

Quoted 7 lines · Andrew Morton

Quoted 5 lines · On Wed, 3 Dec 2008 16:10:38 -0500 (EST) Steven Rostedt <[email protected]>…

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	seq_printf(m, "%s\n", str);

Can we use %pF here?

Quoted 4 lines

Quoted 1 line

If there's a way to not print the "+offset".

Could be added, I guess. I wonder if it would be
reused elsewhere.

There's lots of places in ftrace that would use it, and probably clean up a bunch of code in the process.

kernel/trace/ftrace.c static struct file_operations ftrace_notrace_fops = { · line 1380

Quoted 6 lines · On Wed, 03 Dec 2008 15:36:57 -0500 Steven Rostedt <[email protected]> wrote:

kernel/trace/ftrace.c +6 -0

+static struct seq_operations ftrace_graph_seq_ops = {
+	.start = g_start,
+	.next = g_next,
+	.stop = g_stop,
+	.show = g_show,
+};

This could be static I think.

It's probably not worth even commenting on or fixing this sort of thing. Just rely on someone(tm) doing periodic sweeps to fix them all up.

Quoted 11 lines · Andrew Morton

kernel/trace/ftrace.c static struct file_operations ftrace_notrace_fops = { · line 1380

Quoted 6 lines · On Wed, 03 Dec 2008 15:36:57 -0500 Steven Rostedt <[email protected]> wrote:

kernel/trace/ftrace.c +6 -0

+static struct seq_operations ftrace_graph_seq_ops = {
+	.start = g_start,
+	.next = g_next,
+	.stop = g_stop,
+	.show = g_show,
+};

This could be static I think.

s/static/const/

Damn damn damn damn!!!! I said to myself, I need to add const there and still forgot :-(

Quoted 15 lines · Steven Rostedt

Quoted 11 lines · Andrew Morton

kernel/trace/ftrace.c static struct file_operations ftrace_notrace_fops = { · line 1380

Quoted 6 lines · On Wed, 03 Dec 2008 15:36:57 -0500 Steven Rostedt <[email protected]> wrote:

kernel/trace/ftrace.c +6 -0

+static struct seq_operations ftrace_graph_seq_ops = {
+	.start = g_start,
+	.next = g_next,
+	.stop = g_stop,
+	.show = g_show,
+};

This could be static I think.

Quoted 4 lines

s/static/const/

Damn damn damn damn!!!! I said to myself, I need to add const there and still forgot :-(

No need to get stressed up about such details - we need checkpatch help for this.

Quoted 15 lines · Steven Rostedt

Quoted 11 lines · Andrew Morton

kernel/trace/ftrace.c static struct file_operations ftrace_notrace_fops = { · line 1380

Quoted 6 lines · On Wed, 03 Dec 2008 15:36:57 -0500 Steven Rostedt <[email protected]> wrote:

kernel/trace/ftrace.c +6 -0

+static struct seq_operations ftrace_graph_seq_ops = {
+	.start = g_start,
+	.next = g_next,
+	.stop = g_stop,
+	.show = g_show,
+};

This could be static I think.

Quoted 4 lines

s/static/const/

Damn damn damn damn!!!! I said to myself, I need to add const there and still forgot :-(

No need to get stressed up about such details - we need checkpatch help for this.

Impact: fix a bug

when writing function to set_graph_function, we should check whether it has existed in set_graph_function to avoid duplicating.

Signed-off-by Liming Wang <[email protected]>

---
 kernel/trace/ftrace.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

From: Steven Rostedt <[email protected]>

Impact: clean up

Use the new task struct trace flags to determine if a process should be traced or not.

Note: this moves the searching of the pid to the slow path of setting the pid field. This needs to be converted to the pid name space.

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/ftrace.c |   28 +++++++++++++++++++++++++---
 1 files changed, 25 insertions(+), 3 deletions(-)

kernel/trace/ftrace.c +2 -2

index b17a303..c5049f5 100644
@@ -47,7 +47,7 @@
 int ftrace_enabled __read_mostly;
 static int last_ftrace_enabled;
 
-/* ftrace_pid_trace >= 0 will only trace threads with this pid */
+/* set when tracing only a pid */
 static int ftrace_pid_trace = -1;
 
 /* Quick disabling of function tracer. */

@@ -90,7 +90,7 @@ 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)
+	if (!test_tsk_trace_trace(current))
 		return;
 

kernel/trace/ftrace.c +5 -1

 	ftrace_pid_function(ip, parent_ip);

@@ -1714,11 +1714,33 @@ ftrace_pid_write(struct file *filp, const char __user
 *ubuf,
 		ftrace_pid_trace = -1;
 
 	} else {
+		struct task_struct *p;
+		int found = 0;
 
 		if (ftrace_pid_trace == val)
 			goto out;
 
-		ftrace_pid_trace = val;
+		/*
+		 * Find the task that matches this pid.
+		 * TODO: use pid namespaces instead.

This patch makes the changes to set_ftrace_pid apply to the function graph tracer.

 # echo $$ > /debugfs/tracing/set_ftrace_pid
 # echo function_graph > /debugfs/tracing/current_tracer

Will cause only the current task to be traced. Note, the trace flags are also inherited by child processes, so the children of the shell will also be traced.

Quoted 2 lines · On Wed, 03 Dec 2008 15:36:59 -0500 Steven Rostedt <[email protected]> wrote:

This patch makes the changes to set_ftrace_pid apply to the function graph tracer.

That sentence needs help.

Quoted 7 lines

 # echo $$ > /debugfs/tracing/set_ftrace_pid
 # echo function_graph > /debugfs/tracing/current_tracer

Will cause only the current task to be traced. Note, the trace flags are also inherited by child processes, so the children of the shell will also be traced.

Where did we end up on the pids-arent-unique issue? Gave up?

Quoted 8 lines

Quoted 6 lines

 # echo $$ > /debugfs/tracing/set_ftrace_pid
 # echo function_graph > /debugfs/tracing/current_tracer

Will cause only the current task to be traced. Note, the trace flags are also inherited by child processes, so the children of the shell will also be traced.

Where did we end up on the pids-arent-unique issue? Gave up?

Nah, patch 2 opens the door for a solution. I'm just pawning the work off to the container folks ;-)

Steven Rostedt <[email protected]> writes:

The way patch 2 uses pids is just stupid. It has nothing to do with pids aren't unique. You do a full walk of the process list instead of using the hash table.

It makes me think that task->pid really should go away because with it there people don't bother to look and see how things normally work.

Quoted 5 lines · Eric W. Biederman

Steven Rostedt <[email protected]> writes:

The way patch 2 uses pids is just stupid. It has nothing to do with pids aren't unique. You do a full walk of the process list instead of using the hash table.

The way patch 2 uses the pids is stupid, it was just the easiest way to implement it correctly ;-) I work with, do it stupid but correct first, then optimize.

Quoted 2 lines

It makes me think that task->pid really should go away because with it there people don't bother to look and see how things normally work.

This is far from a fast path, and I can easily fix it. The hard work was the rest of the patch not this part. I even did it stupid knowing that I would be rewriting it to handle namespaces. I stated that this needed to be fixed in the patch itself.

One thing I never got an answer for, using the namespace pid path, can I still select the idle task to trace, i.e. pid == 0.

Quoted 8 lines · Steven Rostedt

Quoted 5 lines · Eric W. Biederman

Steven Rostedt <[email protected]> writes:

The way patch 2 uses pids is just stupid. It has nothing to do with pids aren't unique. You do a full walk of the process list instead of using the hash table.

Quoted 3 lines

The way patch 2 uses the pids is stupid, it was just the easiest way to implement it correctly ;-) I work with, do it stupid but correct first, then optimize.

Which is a reasonable and very unix approach. As long as I don't need to do more than advise about what to do.

Quoted 9 lines

Quoted 3 lines

It makes me think that task->pid really should go away because with it there people don't bother to look and see how things normally work.

This is far from a fast path, and I can easily fix it. The hard work was the rest of the patch not this part. I even did it stupid knowing that I would be rewriting it to handle namespaces. I stated that this needed to be fixed in the patch itself.

Quoted 2 lines

One thing I never got an answer for, using the namespace pid path, can I still select the idle task to trace, i.e. pid == 0.

Sorry I didn't see this question. As I recall we have a special pid that is not hashed that is used by the idle task.

So it should be possible but it would need to be a special case. Although please note we have one idle task per cpu.

Is the idle task doing anything interesting enough to warrant tracing? We don't report the idle tasks in proc or in any of our other user space interfaces.

Ingo,

This series has three patches.

The first patch adds a new feature that I've been wanting to have for some time and Arjan even requested. That is to pick a function and only trace that function and its children. Dynamic ftrace and function graph needs to be enabled for this.

To do the above, I added a "trace" flags field in the task structure. The second patch uses this for the ftrace pid code. It searches for the task based on the pid and sets the trace flag, then in the ftrace function caller it only needs to check this flag.

This means we can now trace more than one pid without any more overhead. It also means that we should be able to use the name space code that the container guys want us to. But since I'm not very up on the namespace code, I'm still using just the normal 'pid'. I've Cc'd the container folks so perhaps they could write up a patch for me ;-)

Note: When writing to the set_ftrace_pid two things happen.
- The task with the matching pid gets the trace flag set.
- Any other task has its trace flag cleared.
#2 needs to be addressed when converting to pid name spaces. Just because it is not enough to simply find the matching task. It may be good enough to just clear all tasks and then find the one that matches.

The last patch makes the function graph tracer honor the set_ftrace_pid.

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 (3):
ftrace: graph of a single function
ftrace: use task struct trace flag to filter on pid
ftrace: trace single pid for function graph tracer

----

 include/linux/ftrace.h |   46 +++++++++
 include/linux/sched.h  |    4 +
 kernel/trace/ftrace.c  |  257 +++++++++++++++++++++++++++++++++++++++++++++++-
 kernel/trace/trace.c   |   11 ++
 kernel/trace/trace.h   |   40 +++++++-
 5 files changed, 353 insertions(+), 5 deletions(-)

Ingo,

This series has three patches.

The first patch adds a new feature that I've been wanting to have for some time and Arjan even requested. That is to pick a function and only trace that function and its children. Dynamic ftrace and function graph needs to be enabled for this.

To do the above, I added a "trace" flags field in the task structure. The second patch uses this for the ftrace pid code. It searches for the task based on the pid and sets the trace flag, then in the ftrace function caller it only needs to check this flag.

Quoted 9 lines · Steven Rostedt

Ingo,

This series has three patches.

The first patch adds a new feature that I've been wanting to have for some time and Arjan even requested. That is to pick a function and only trace that function and its children. Dynamic ftrace and function graph needs to be enabled for this.

Quoted 4 lines

To do the above, I added a "trace" flags field in the task structure. The second patch uses this for the ftrace pid code. It searches for the task based on the pid and sets the trace flag, then in the ftrace function caller it only needs to check this flag.

Btw., i'd love to see this done via the regular regexp interface though, if possible - instead of the add-on interface you added.

( Also perhaps enable to toggle tracing via the /proc/<PID>/ hierarchy -
a /proc/<PID>/tracing_enabled switch or so. )

Regarding the filter functions, the basic principle should be mathematical set operations, like we have it now: add and remove, union, wildcards, etc.

I'd suggest a natural and intuitive extension of the current syntax. (while keeping all the current bits)

I already suggested a 'inverse' filter in a previous mail:

echo "-schedule*" >> set_ftrace_filter

This rule operates on the current set of filter functions: it strikes out all existing filter functions that match this pattern.

To handle PIDs, we could do something like:

echo "sshd-312:schedule" > set_ftrace_filter

This would restrict tracing to the sshd-pid:312 task.

Note: the PID portion of the filter rules still stay separate from the function names - we dont want per task function filter rules.

A natural variation would be:

echo "312:schedule" > set_ftrace_filter

to only specify the PID, or:

echo "312,313:schedule" > set_ftrace_filter

to specify two PIDs, or:

echo "sshd:schedule" > set_ftrace_filter

to only specify the 'comm' part, which expands to all PIDs where task->comm matches sshd. Another variation would be:

echo "loop*:schedule" > set_ftrace_filter

that matches all PIDs where task->comm matches loop*.

To specify recursive tracing, we could use something like:

echo "loop*+schedule" > set_ftrace_filter

the '+' would signal that the 'schedule' function is 'expanded' and all its child functions are traced as well.

btw., maybe it makes sense to separate the regexp rule-set from the set of functions that we are tracing right now. For example:

  $ echo "schedule*" >  set_ftrace_filter
  $ echo "time*"     >> set_ftrace_filter
  $ echo "sys_*"     >> set_ftrace_filter

  $ cat set_ftrace_filter
  schedule*
  time*
  sys_*

We'd also have a separate, current_ftrace_functions file as well which shows all traced functions. (on a global basis - with possible PID filter rules added where applicable)

I know this will be hellishly hard to implement, but it would be _very_ elegant, and _very_ usable.

What do you think?