[PATCH] tracing, sched: mark preempt_schedule() notrace

[PATCH] tracing, sched: mark preempt_schedule() notrace

[Lai Jiangshan]

Current preempt_schedule() is not marked notrace. It may be
infinite recursion in __trace_graph_return().

preempt_schedule()
  __trace_graph_return()
    ftrace_preempt_disable() (!!return false!!)
    ftrace_preempt_enable()
      preempt_enable_notrace()
         preempt_schedule() (need_resched() may be true again)


It hardly happens, but marking preempt_schedule() notrace
makes it safer.

One interesting thing is that preempt_schedule() is in
the blacklist of kprobe subsystem. "__kprobes" implies "notrace".
But preempt_schedule() cannot be marked __kprobes for it
has been marked __sched. It is in the blacklist makes me
consider this: should it be marked "notrace" -- YES.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
diff --git a/kernel/sched.c b/kernel/sched.c
index 5184580..2e9e209 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -5534,7 +5534,7 @@ out:
  * off of preempt_enable. Kernel preemptions off return from interrupt
  * occur there and call schedule directly.
  */
-asmlinkage void __sched preempt_schedule(void)
+asmlinkage void __sched notrace preempt_schedule(void)
 {
 	struct thread_info *ti = current_thread_info();
 






     

Re: [PATCH] tracing, sched: mark preempt_schedule() notrace

[Frederic Weisbecker]

On Tue, Aug 18, 2009 at 04:01:57PM +0800, Lai Jiangshan wrote:
> 
> Current preempt_schedule() is not marked notrace. It may be
> infinite recursion in __trace_graph_return().
> 
> preempt_schedule()
>   __trace_graph_return()
>     ftrace_preempt_disable() (!!return false!!)
>     ftrace_preempt_enable()
>       preempt_enable_notrace()
>          preempt_schedule() (need_resched() may be true again)



It would happen in __trace_graph_return() , when preempt_schedule()
has finished its job. It's very unlikely the TIF_NEED_RESCHED is
set just after (because it has just been cleared).
But why not. In that case, preempt_schedule() is called again but it's
not a real tracing recursion.

That seems like a normal behaviour actually.

 
> 
> It hardly happens, but marking preempt_schedule() notrace
> makes it safer.
> 
> One interesting thing is that preempt_schedule() is in
> the blacklist of kprobe subsystem. "__kprobes" implies "notrace".
> But preempt_schedule() cannot be marked __kprobes for it
> has been marked __sched. It is in the blacklist makes me
> consider this: should it be marked "notrace" -- YES.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 5184580..2e9e209 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -5534,7 +5534,7 @@ out:
>   * off of preempt_enable. Kernel preemptions off return from interrupt
>   * occur there and call schedule directly.
>   */
> -asmlinkage void __sched preempt_schedule(void)
> +asmlinkage void __sched notrace preempt_schedule(void)
>  {
>  	struct thread_info *ti = current_thread_info();
>  
> 
> 
> 
> 
> 
> 
>      
> 

Re: [PATCH] tracing, sched: mark preempt_schedule() notrace

[Steven Rostedt]

On Tue, 18 Aug 2009, Frederic Weisbecker wrote:

> On Tue, Aug 18, 2009 at 04:01:57PM +0800, Lai Jiangshan wrote:
> > 
> > Current preempt_schedule() is not marked notrace. It may be
> > infinite recursion in __trace_graph_return().
> > 
> > preempt_schedule()
> >   __trace_graph_return()
> >     ftrace_preempt_disable() (!!return false!!)
> >     ftrace_preempt_enable()
> >       preempt_enable_notrace()
> >          preempt_schedule() (need_resched() may be true again)
> 
> 
> 
> It would happen in __trace_graph_return() , when preempt_schedule()
> has finished its job. It's very unlikely the TIF_NEED_RESCHED is
> set just after (because it has just been cleared).
> But why not. In that case, preempt_schedule() is called again but it's
> not a real tracing recursion.
> 
> That seems like a normal behaviour actually.

In fact, this is actually something to be traced. I would not add a 
notrace to it. This is not saving anything, the trace_graph_return does 
not need to worry about stack overflow, since the return code already 
freed the stack.

-- Steve

Re: [PATCH] tracing, sched: mark preempt_schedule() notrace

[Lai Jiangshan]

Frederic Weisbecker wrote:
> On Tue, Aug 18, 2009 at 04:01:57PM +0800, Lai Jiangshan wrote:
>> Current preempt_schedule() is not marked notrace. It may be
>> infinite recursion in __trace_graph_return().
>>
>> preempt_schedule()
>>   __trace_graph_return()
>>     ftrace_preempt_disable() (!!return false!!)
>>     ftrace_preempt_enable()
>>       preempt_enable_notrace()
>>          preempt_schedule() (need_resched() may be true again)
> 
> 
> 
> It would happen in __trace_graph_return() , when preempt_schedule()
> has finished its job. It's very unlikely the TIF_NEED_RESCHED is
> set just after (because it has just been cleared).

It hardly happen ...
This doesn't mean it'll never happen.

> But why not. In that case, preempt_schedule() is called again but it's
> not a real tracing recursion.
> 
> That seems like a normal behaviour actually.
> 
> 

It's not normal behavior, preempt_schedule() will not call
preempt_schedule() recursively in any situation when trace is off.

Here, preempt_schedule() is called from __trace_graph_return()
when trace_function_graph is on.

preempt_schedule()
  __trace_graph_return()
    preempt_schedule()
      __trace_graph_return()
        ....

So, it's a real tracing recursion.
It hurts the stack.

Re: [PATCH] tracing, sched: mark preempt_schedule() notrace

[Lai Jiangshan]

Steven Rostedt wrote:
> 
> On Tue, 18 Aug 2009, Frederic Weisbecker wrote:
> 
>> On Tue, Aug 18, 2009 at 04:01:57PM +0800, Lai Jiangshan wrote:
>>> Current preempt_schedule() is not marked notrace. It may be
>>> infinite recursion in __trace_graph_return().
>>>
>>> preempt_schedule()
>>>   __trace_graph_return()
>>>     ftrace_preempt_disable() (!!return false!!)
>>>     ftrace_preempt_enable()
>>>       preempt_enable_notrace()
>>>          preempt_schedule() (need_resched() may be true again)
>>
>>
>> It would happen in __trace_graph_return() , when preempt_schedule()
>> has finished its job. It's very unlikely the TIF_NEED_RESCHED is
>> set just after (because it has just been cleared).
>> But why not. In that case, preempt_schedule() is called again but it's
>> not a real tracing recursion.
>>
>> That seems like a normal behaviour actually.
> 
> In fact, this is actually something to be traced. I would not add a 
> notrace to it. This is not saving anything, the trace_graph_return does 
> not need to worry about stack overflow, since the return code already 
> freed the stack.
> 

You're right, I forgot to read trace_graph_return().
trace_graph_return() protects itself from recursion.

Since there is no may-recursive trace_func_graph_ret_t.
Just omit this patch.
Sorry for bothered you all.