Re: [PATCH] tracing/function-return-tracer: don't trace kfree while it frees the return stack

[Ingo Molnar <mingo@elte.hu>]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> On Sun, 23 Nov 2008, Ingo Molnar wrote:
> > > 
> > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > index 90d99fb..ffff7ec 100644
> > > --- a/kernel/trace/ftrace.c
> > > +++ b/kernel/trace/ftrace.c
> > > @@ -1628,8 +1628,9 @@ void ftrace_retfunc_init_task(struct task_struct *t)
> > >  
> > >  void ftrace_retfunc_exit_task(struct task_struct *t)
> > >  {
> > > -	kfree(t->ret_stack);
> > > +	struct ftrace_ret_stack	*ret_stack = t->ret_stack;
> > >  	t->ret_stack = NULL;
> > > +	kfree(ret_stack);
> > 
> > heh, nice one :)
> > 
> > note that we also need to keep gcc from reordering things here (no 
> > matter how unlikely in this particular case).
> 
> I first thought that too, but thinking about it, if gcc does do that, then
> it will break the logic for a correct C program.
> 
> t is passed in as a pointer, then it modifies the contents of t 
> (which could be a global pointer), then it calls a external 
> function, that might also reference the global pointer.
> 
> This means that if it were to reorder the two, it would break C, 
> because the compiler can not assume that the called function will 
> read the global pointer either.
> 
> In other words, the compiler should not need to worry about SMP or 
> modifications done by interrupts or other threads. But the compiler 
> should always preserve the order that is assumed by a single 
> context.

Correct, but this assumes that kfree is a C function. Which it might 
not necessarily be: it could be optimized via an inline in certain 
cases, etc. It's best to document such cases explicitly.

In any case, the real solution is what i suggested in the previous 
mail, to do the freeing from the task-struct freeing path in 
kernel/fork.c:free_task() - that has other advantages as well.

	Ingo