Re: [PATCH RFC tip/perf/core] ftrace/x86: Let dynamic trampolines call ops->func even for dynamic fops

[Miroslav Benes <mbenes@suse.cz>]

On Thu, 19 Feb 2015, Miroslav Benes wrote:

> Dynamically allocated trampolines call ftrace_ops_get_func to get the
> function which they should call. For dynamic fops (FTRACE_OPS_FL_DYNAMIC
> flag is set) ftrace_ops_list_func is always returned. This is reasonable
> for static trampolines but goes against the main advantage of dynamic
> ones, that is avoidance of going through the list of all registered
> callbacks for functions that are only being traced by a single callback.
> 
> We can fix it by returning ops->func (or recursion safe version) from
> ftrace_ops_get_func whenever it is possible for dynamic trampolines.
> 
> Note that dynamic trampolines are not allowed for dynamic fops if
> CONFIG_PREEMPT=y.
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> ---
> 
> The patch is the result of my discussion with Steven few weeks ago [1].
> I feel content with the outcome but not with the way.
> ftrace_ops_get_func is called at two different places now. One is
> create_trampoline where dynamic trampoline is created (if allowed) and
> the other is in update_ftrace_function for other cases. I haven't found
> the way how to distinguish between these call places in the function
> using present means. Thus I introduced new parameter. I do not consider
> this optimum and that is the reason why this patch is RFC. I would
> welcome any idea which would make it suitable for merge.
> 
> Steven, if you plan to fix this issue differently and in some larger
> set, feel free to scratch this patch.

Hi Steven,

I don't know if you plan to do something about this patch or if you just 
missed it in your e-mail pile. Should I resend it or have you already 
scratched that?

Regards,
Miroslav

> 
> [1]: https://lkml.org/lkml/2015/1/29/300
> 
>  arch/x86/kernel/ftrace.c |  2 +-
>  include/linux/ftrace.h   |  2 +-
>  kernel/trace/ftrace.c    | 10 ++++++----
>  3 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 8b7b0a5..bfa9267 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -842,7 +842,7 @@ void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
>  	offset = calc_trampoline_call_offset(ops->flags & FTRACE_OPS_FL_SAVE_REGS);
>  	ip = ops->trampoline + offset;
>  
> -	func = ftrace_ops_get_func(ops);
> +	func = ftrace_ops_get_func(ops, true);
>  
>  	/* Do a safe modify in case the trampoline is executing */
>  	new = ftrace_call_replace(ip, (unsigned long)func);
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 1da6029..37444b5 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -62,7 +62,7 @@ struct ftrace_ops;
>  typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
>  			      struct ftrace_ops *op, struct pt_regs *regs);
>  
> -ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
> +ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops, bool dyntramp);
>  
>  /*
>   * FTRACE_OPS_FL_* bits denote the state of ftrace_ops struct and are
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 224e768..5d964b3 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -270,7 +270,7 @@ static void update_ftrace_function(void)
>  	 * then have the mcount trampoline call the function directly.
>  	 */
>  	} else if (ftrace_ops_list->next == &ftrace_list_end) {
> -		func = ftrace_ops_get_func(ftrace_ops_list);
> +		func = ftrace_ops_get_func(ftrace_ops_list, false);
>  
>  	} else {
>  		/* Just use the default ftrace_ops */
> @@ -5176,6 +5176,7 @@ static void ftrace_ops_recurs_func(unsigned long ip, unsigned long parent_ip,
>  /**
>   * ftrace_ops_get_func - get the function a trampoline should call
>   * @ops: the ops to get the function for
> + * @dyntramp: whether the function is for dynamic trampoline or not
>   *
>   * Normally the mcount trampoline will call the ops->func, but there
>   * are times that it should not. For example, if the ops does not
> @@ -5184,13 +5185,14 @@ static void ftrace_ops_recurs_func(unsigned long ip, unsigned long parent_ip,
>   *
>   * Returns the function that the trampoline should call for @ops.
>   */
> -ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops)
> +ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops, bool dyntramp)
>  {
>  	/*
> -	 * If this is a dynamic ops or we force list func,
> +	 * If this is a dynamic ops and static trampoline or we force list func,
>  	 * then it needs to call the list anyway.
>  	 */
> -	if (ops->flags & FTRACE_OPS_FL_DYNAMIC || FTRACE_FORCE_LIST_FUNC)
> +	if ((!dyntramp && (ops->flags & FTRACE_OPS_FL_DYNAMIC)) ||
> +	    FTRACE_FORCE_LIST_FUNC)
>  		return ftrace_ops_list_func;
>  
>  	/*
> -- 
> 2.1.4
>