Re: [PATCHv2 03/10] ftrace: Add enable/disable ftrace_ops control interface

[Jiri Olsa <jolsa@redhat.com>]

On Tue, Dec 20, 2011 at 10:25:53AM -0500, Steven Rostedt wrote:
> On Tue, 2011-12-20 at 15:57 +0100, Jiri Olsa wrote:
> 
> > > 
> > > If the above is called with preemption enabled, it will not do what is
> > > expected. We could disable function tracing on one CPU and then
> > > re-enable it for another CPU even though it is already enabled.
> > 
> > It is only called inside perf reg callback within the
> > schedule function where the preemption is disabled.
> > 
> > The ftrace_function_enable is called when task is scheduled in
> > on respective cpu. Likewise the ftrace_function_disable is called 
> > when task is scheduled out on respective cpu.
> 
> Yes I know how you use it, but this is an open API. It may be currently
> only used by perf today, but that doesn't mean that it wont be used by
> others. There's no documentation on how to use it. I don't look at this
> and say, "oh this is used by perf, we only need to worry about how perf
> uses it". That doesn't scale. It needs to be documented on how to use
> it, and if it requires preemption disabled when calling it, it should
> definitely be stated that fact and it may need a
> WARN_ON(preempt_count()) or something.

I've already added

        if (WARN_ON_ONCE(!(ops->flags & FTRACE_OPS_FL_CONTROL)))
                return;

will add similar for preemption and make comments for both functions

thanks,
jirka

> 
> -- Steve
> 
>