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

[Steven Rostedt]

On Mon, 2011-12-05 at 18:22 +0100, Jiri Olsa wrote:
>  
> +static inline void enable_ftrace_function(struct ftrace_ops *ops)
> +{
> +	atomic_t *disabled = this_cpu_ptr(ops->disabled);
> +	atomic_dec(disabled);
> +}
> +
> +static inline void disable_ftrace_function(struct ftrace_ops *ops)
> +{
> +	atomic_t *disabled = this_cpu_ptr(ops->disabled);
> +	atomic_inc(disabled);
> +}
> +

The above should be renamed to ftrace_function_enable/disable(), and
they should pass in the cpu. There may be a case we want to disable
ftrace functions on another CPU.

Not to mention, this is the perfect example of "this_cpu_ptr" being used
incorrectly. It's not made for this purpose, and again the naming of
"this_cpu" totally confuses other kernel developers. We need to change
this name as it was agreed to at Kernel Summit.

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.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/