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 <mbe...@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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
