lkml.org 
[lkml]   [2015]   [Mar]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH RFC tip/perf/core] ftrace/x86: Let dynamic trampolines call ops->func even for dynamic fops
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
>


\
 
 \ /
  Last update: 2015-03-05 17:21    [W:0.156 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site