[GIT PULL] tracing/urgent for v2.6.31

[GIT PULL] tracing/urgent for v2.6.31

[Ingo Molnar]

Linus,

Please pull the latest tracing-urgent-for-linus git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git tracing-urgent-for-linus

These are the leftover commits in the tracing/urgent queue that 
didnt end up making it into .30. There's a trivial conflict in 
kernel/trace/ftrace.c.

The reason there's an internal conflict between two tracing-tree 
topics is that we tried a new trick to reduce merge commits: we 
never merged tracing/urgent into tracing/core - we waited for it to 
hit upstream, and then merged upstream back at one of the -rc 
points. The inevitable latency of this process opens up a window for 
conflicts. (Let me know if we should instead avoid the conflict by 
backmerging tracing/urgent more frequently.)

 Thanks,

	Ingo

------------------>
Steven Rostedt (5):
      function-graph: only allocate init tasks if it was not already done
      function-graph: enable the stack after initialization of other variables
      function-graph: add memory barriers for accessing task's ret_stack
      function-graph: move initialization of new tasks up in fork
      function-graph: always initialize task ret_stack


 kernel/fork.c                        |   10 ++++------
 kernel/trace/ftrace.c                |   29 +++++++++++++++++++----------
 kernel/trace/trace_functions_graph.c |    6 ++++++
 3 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index b9e2edd..c4b1e35 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -982,6 +982,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	if (!p)
 		goto fork_out;
 
+	ftrace_graph_init_task(p);
+
 	rt_mutex_init_task(p);
 
 #ifdef CONFIG_PROVE_LOCKING
@@ -1131,8 +1133,6 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		}
 	}
 
-	ftrace_graph_init_task(p);
-
 	p->pid = pid_nr(pid);
 	p->tgid = p->pid;
 	if (clone_flags & CLONE_THREAD)
@@ -1141,7 +1141,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	if (current->nsproxy != p->nsproxy) {
 		retval = ns_cgroup_clone(p, pid);
 		if (retval)
-			goto bad_fork_free_graph;
+			goto bad_fork_free_pid;
 	}
 
 	p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidptr : NULL;
@@ -1233,7 +1233,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		spin_unlock(&current->sighand->siglock);
 		write_unlock_irq(&tasklist_lock);
 		retval = -ERESTARTNOINTR;
-		goto bad_fork_free_graph;
+		goto bad_fork_free_pid;
 	}
 
 	if (clone_flags & CLONE_THREAD) {
@@ -1268,8 +1268,6 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	cgroup_post_fork(p);
 	return p;
 
-bad_fork_free_graph:
-	ftrace_graph_exit_task(p);
 bad_fork_free_pid:
 	if (pid != &init_struct_pid)
 		free_pid(pid);
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f1ed080..bb081f3 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2580,12 +2580,12 @@ static int alloc_retstack_tasklist(struct ftrace_ret_stack **ret_stack_list)
 		}
 
 		if (t->ret_stack == NULL) {
-			t->curr_ret_stack = -1;
-			/* Make sure IRQs see the -1 first: */
-			barrier();
-			t->ret_stack = ret_stack_list[start++];
 			atomic_set(&t->tracing_graph_pause, 0);
 			atomic_set(&t->trace_overrun, 0);
+			t->curr_ret_stack = -1;
+			/* Make sure the tasks see the -1 first: */
+			smp_wmb();
+			t->ret_stack = ret_stack_list[start++];
 		}
 	} while_each_thread(g, t);
 
@@ -2643,8 +2643,10 @@ static int start_graph_tracing(void)
 		return -ENOMEM;
 
 	/* The cpu_boot init_task->ret_stack will never be freed */
-	for_each_online_cpu(cpu)
-		ftrace_graph_init_task(idle_task(cpu));
+	for_each_online_cpu(cpu) {
+		if (!idle_task(cpu)->ret_stack)
+			ftrace_graph_init_task(idle_task(cpu));
+	}
 
 	do {
 		ret = alloc_retstack_tasklist(ret_stack_list);
@@ -2736,18 +2738,25 @@ void unregister_ftrace_graph(void)
 /* Allocate a return stack for newly created task */
 void ftrace_graph_init_task(struct task_struct *t)
 {
+	/* Make sure we do not use the parent ret_stack */
+	t->ret_stack = NULL;
+
 	if (atomic_read(&ftrace_graph_active)) {
-		t->ret_stack = kmalloc(FTRACE_RETFUNC_DEPTH
+		struct ftrace_ret_stack *ret_stack;
+
+		ret_stack = kmalloc(FTRACE_RETFUNC_DEPTH
 				* sizeof(struct ftrace_ret_stack),
 				GFP_KERNEL);
-		if (!t->ret_stack)
+		if (!ret_stack)
 			return;
 		t->curr_ret_stack = -1;
 		atomic_set(&t->tracing_graph_pause, 0);
 		atomic_set(&t->trace_overrun, 0);
 		t->ftrace_timestamp = 0;
-	} else
-		t->ret_stack = NULL;
+		/* make curr_ret_stack visable before we add the ret_stack */
+		smp_wmb();
+		t->ret_stack = ret_stack;
+	}
 }
 
 void ftrace_graph_exit_task(struct task_struct *t)
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index d28687e..baeb5fe 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -65,6 +65,12 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, int *depth)
 	if (!current->ret_stack)
 		return -EBUSY;
 
+	/*
+	 * We must make sure the ret_stack is tested before we read
+	 * anything else.
+	 */
+	smp_rmb();
+
 	/* The return trace stack is full */
 	if (current->curr_ret_stack == FTRACE_RETFUNC_DEPTH - 1) {
 		atomic_inc(&current->trace_overrun);

Re: [GIT PULL] tracing/urgent for v2.6.31

[Linus Torvalds]

On Thu, 11 Jun 2009, Ingo Molnar wrote:
>
> Let me know if we should instead avoid the conflict by backmerging 
> tracing/urgent more frequently.

No. So far, the merges from -tip this time around have been a pleasure.  
And I'd _much_ rather get a few conflicts, if that means that we can keep 
the things more separate, and have a more readable history. I think I had 
a total of two conflicts, and both of them were totally trivial.

[ Famous last words. Maybe I screwed them up. Whatever. They were simple 
  enough that even if I _did_ screw them up, it won't be anything serious ]

The only one that could have been better (modulo bugs or other 
misfeatures, of course - let's see how this merge window turns out) is the 
tracing one, which I really wish could have been split up more. That was a 
big half-meg diff, which meant that unlike the other ones, I couldn't 
really scan through it and feel like I got a feel for it. 

Aside from that one not being as obvious as the others, no big issues at 
least so far.

The only (small) complaint is that some new things seem to default to 'y', 
which I'm dubious about, but having several different and independent git 
trees, with small enough diffs that I can actually look at them, and with 
a history that is clearly "one area", makes it just _soo_ much easier for 
me to merge.

As to config options - why in the world does something like X86_PTRACE_BTS 
have a 'default y'? Yes, it got disabled again with a 'depends on BROKEN', 
but on the whole, that whole thing seems crazy. And I'm not talking about 
the code, I'm talking about whoever decided that it should have that 
strange 'default y' thing.

New features should _never_ default to 'y'.

I think defaulting CONFIG_FTRACE to 'y' is dubious too, but at least 
that's not a new feature as much as a new config option making other 
config options visible. But again, I do think it should likely default to 
off, even so.

Similarly, CONFIG_MARKERS seems to be forced to 'y' by something. Why?

Anyway - apart from small details like that, so far so good. If tracing is 
going to get this much churn in the future, I do wish that in the future 
different tracers be split up. But the merges have been way more readable 
this time. Good job.

		Linus

PS. Ok, I also do hate the prefix "ds". It tells nothing. There's a lot of 
new global functions etc that have that prefix. Sure, if you read the 
intel debug documentation, the whole "debug store" thing makes sense, but 
I think prefixes like that should be longer and more telling.

Re: [GIT PULL] tracing/urgent for v2.6.31

[Steven Rostedt]

On Wed, 10 Jun 2009, Linus Torvalds wrote:
> 
> I think defaulting CONFIG_FTRACE to 'y' is dubious too, but at least 
> that's not a new feature as much as a new config option making other 
> config options visible. But again, I do think it should likely default to 
> off, even so.

Well it was:

	default y if DEBUG_KERNEL

Where the rational was that if you want to debug the kernel, you might 
want to see some tracing features that are available to you.

-- Steve

Re: [GIT PULL] tracing/urgent for v2.6.31

[Linus Torvalds]

On Wed, 10 Jun 2009, Steven Rostedt wrote:
> 
> Well it was:
> 
> 	default y if DEBUG_KERNEL
> 
> Where the rational was that if you want to debug the kernel, you might 
> want to see some tracing features that are available to you.

Sure. I think that FTRACE config is borderline fine, although I think 
tracing is pretty different from the debug options that don't need the 
user to actually do anything (like PAGEALLOC_DEBUG).

It's the other ones I think were clearly dubious (ok, the MARKERS one 
isn't "clearly" dubious, since I have no idea why it got enabled, so I 
guess it would count as "unclearly dubious" then).

			Linus

Re: [GIT PULL] tracing/urgent for v2.6.31

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, 10 Jun 2009, Steven Rostedt wrote:
> > 
> > Well it was:
> > 
> > 	default y if DEBUG_KERNEL
> > 
> > Where the rational was that if you want to debug the kernel, you 
> > might want to see some tracing features that are available to 
> > you.
> 
> Sure. I think that FTRACE config is borderline fine, although I 
> think tracing is pretty different from the debug options that 
> don't need the user to actually do anything (like 
> PAGEALLOC_DEBUG).
> 
> It's the other ones I think were clearly dubious (ok, the MARKERS 
> one isn't "clearly" dubious, since I have no idea why it got 
> enabled, so I guess it would count as "unclearly dubious" then).

I agree that we have too many default-y options - we'll have a sweep 
of all options and will default-disable the dubious (and unclearly 
dubious) ones in a way that truly re-triggers user selection even if 
the option is already enabled in the .config.

This particular new menu didnt actually change any of the existing 
options or defaults: it just organized/grouped an already existing, 
somewhat disorganized set of options and made it easier to disable 
them summarily. Making _that_ default-n could have caused people to 
accidentally disable a lot of options they had enabled in their 
configs intentionally.

	Ingo