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

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

[Miroslav Benes]

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.

[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

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

[Miroslav Benes]

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
> 

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

[Steven Rostedt]

On Thu, 5 Mar 2015 16:56:43 +0100 (CET)
Miroslav Benes <mbenes@suse.cz> wrote:

> 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?

Thanks for the reminder. It was marked as "todo" but fell in the noise.
It's worse that I was traveling when you sent this, which makes it more
likely to be missed :-/

Anyway, I'm still trying to catch up. If you don't hear from me by next
Thursday, please send me another reminder.

Thanks!

-- Steve

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

[Miroslav Benes]

On Thu, 5 Mar 2015, Steven Rostedt wrote:

> On Thu, 5 Mar 2015 16:56:43 +0100 (CET)
> Miroslav Benes <mbenes@suse.cz> wrote:
> 
> > 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?
> 
> Thanks for the reminder. It was marked as "todo" but fell in the noise.
> It's worse that I was traveling when you sent this, which makes it more
> likely to be missed :-/
> 
> Anyway, I'm still trying to catch up. If you don't hear from me by next
> Thursday, please send me another reminder.

Ah, I see. There's no hurry. Thanks for letting me know.

Miroslav

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

[Miroslav Benes]

On Thu, 5 Mar 2015, Steven Rostedt wrote:

> On Thu, 5 Mar 2015 16:56:43 +0100 (CET)
> Miroslav Benes <mbenes@suse.cz> wrote:
> 
> > 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?
> 
> Thanks for the reminder. It was marked as "todo" but fell in the noise.
> It's worse that I was traveling when you sent this, which makes it more
> likely to be missed :-/
> 
> Anyway, I'm still trying to catch up. If you don't hear from me by next
> Thursday, please send me another reminder.

ping...

Miroslav

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

[Steven Rostedt]

On Thu, 2 Apr 2015 13:11:16 +0200 (CEST)
Miroslav Benes <mbenes@suse.cz> wrote:

> On Thu, 5 Mar 2015, Steven Rostedt wrote:
> 
> > On Thu, 5 Mar 2015 16:56:43 +0100 (CET)
> > Miroslav Benes <mbenes@suse.cz> wrote:
> > 
> > > 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?
> > 
> > Thanks for the reminder. It was marked as "todo" but fell in the noise.
> > It's worse that I was traveling when you sent this, which makes it more
> > likely to be missed :-/
> > 
> > Anyway, I'm still trying to catch up. If you don't hear from me by next
> > Thursday, please send me another reminder.
> 
> ping...
> 

Thanks for the reminder. I modified your patch to this. Would this work
for you? It only needs to touch the generic ftrace.c file. I kept your
change log though.

-- Steve

>From 00ccbf2f5b7580cd7dcdaeda84828d14f0cba3c9 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Date: Thu, 19 Feb 2015 15:56:14 +0100
Subject: [PATCH] ftrace: Let dynamic trampolines call ops->func even for
 dynamic fops

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.

Link: http://lkml.kernel.org/r/alpine.LNX.2.00.1501291023000.25445@pobox.suse.cz
Link: http://lkml.kernel.org/r/1424357773-13536-1-git-send-email-mbenes@suse.cz

Reported-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 4f228024055b..d01d238d8ef4 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -249,6 +249,19 @@ static void update_function_graph_func(void);
 static inline void update_function_graph_func(void) { }
 #endif
 
+
+static ftrace_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
+{
+	/*
+	 * If this is a dynamic ops or we force list func,
+	 * then it needs to call the list anyway.
+	 */
+	if (ops->flags & FTRACE_OPS_FL_DYNAMIC || FTRACE_FORCE_LIST_FUNC)
+		return ftrace_ops_list_func;
+
+	return ftrace_ops_get_func(ops);
+}
+
 static void update_ftrace_function(void)
 {
 	ftrace_func_t func;
@@ -270,7 +283,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_list_func(ftrace_ops_list);
 
 	} else {
 		/* Just use the default ftrace_ops */
@@ -5209,13 +5222,6 @@ static void ftrace_ops_recurs_func(unsigned long ip, unsigned long parent_ip,
 ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops)
 {
 	/*
-	 * If this is a dynamic ops or we force list func,
-	 * then it needs to call the list anyway.
-	 */
-	if (ops->flags & FTRACE_OPS_FL_DYNAMIC || FTRACE_FORCE_LIST_FUNC)
-		return ftrace_ops_list_func;
-
-	/*
 	 * If the func handles its own recursion, call it directly.
 	 * Otherwise call the recursion protected function that
 	 * will call the ftrace ops function.
-- 
1.8.3.1

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

[Miroslav Benes]

On Thu, 2 Apr 2015, Steven Rostedt wrote:

> On Thu, 2 Apr 2015 13:11:16 +0200 (CEST)
> Miroslav Benes <mbenes@suse.cz> wrote:
> 
> > On Thu, 5 Mar 2015, Steven Rostedt wrote:
> > 
> > > On Thu, 5 Mar 2015 16:56:43 +0100 (CET)
> > > Miroslav Benes <mbenes@suse.cz> wrote:
> > > 
> > > > 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?
> > > 
> > > Thanks for the reminder. It was marked as "todo" but fell in the noise.
> > > It's worse that I was traveling when you sent this, which makes it more
> > > likely to be missed :-/
> > > 
> > > Anyway, I'm still trying to catch up. If you don't hear from me by next
> > > Thursday, please send me another reminder.
> > 
> > ping...
> > 
> 
> Thanks for the reminder. I modified your patch to this. Would this work
> for you? It only needs to touch the generic ftrace.c file. I kept your
> change log though.

Yes, this works but there is a slight difference to the previous behaviour 
which I tried to preserve in my patch. ftrace_ops_get_func in your patch 
does not check FTRACE_FORCE_LIST_FUNC. I do not know if that is a real 
problem. Anyway your approach looks much better and dynamic trampolines 
are now used for live patches. So in this context you can add

Tested-by: Miroslav Benes <mbenes@suse.cz>

Thanks a lot,
Miroslav


> -- Steve
> 
> From 00ccbf2f5b7580cd7dcdaeda84828d14f0cba3c9 Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> Date: Thu, 19 Feb 2015 15:56:14 +0100
> Subject: [PATCH] ftrace: Let dynamic trampolines call ops->func even for
>  dynamic fops
> 
> 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.
> 
> Link: http://lkml.kernel.org/r/alpine.LNX.2.00.1501291023000.25445@pobox.suse.cz
> Link: http://lkml.kernel.org/r/1424357773-13536-1-git-send-email-mbenes@suse.cz
> 
> Reported-by: Miroslav Benes <mbenes@suse.cz>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/ftrace.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 4f228024055b..d01d238d8ef4 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -249,6 +249,19 @@ static void update_function_graph_func(void);
>  static inline void update_function_graph_func(void) { }
>  #endif
>  
> +
> +static ftrace_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
> +{
> +	/*
> +	 * If this is a dynamic ops or we force list func,
> +	 * then it needs to call the list anyway.
> +	 */
> +	if (ops->flags & FTRACE_OPS_FL_DYNAMIC || FTRACE_FORCE_LIST_FUNC)
> +		return ftrace_ops_list_func;
> +
> +	return ftrace_ops_get_func(ops);
> +}
> +
>  static void update_ftrace_function(void)
>  {
>  	ftrace_func_t func;
> @@ -270,7 +283,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_list_func(ftrace_ops_list);
>  
>  	} else {
>  		/* Just use the default ftrace_ops */
> @@ -5209,13 +5222,6 @@ static void ftrace_ops_recurs_func(unsigned long ip, unsigned long parent_ip,
>  ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops)
>  {
>  	/*
> -	 * If this is a dynamic ops or we force list func,
> -	 * then it needs to call the list anyway.
> -	 */
> -	if (ops->flags & FTRACE_OPS_FL_DYNAMIC || FTRACE_FORCE_LIST_FUNC)
> -		return ftrace_ops_list_func;
> -
> -	/*
>  	 * If the func handles its own recursion, call it directly.
>  	 * Otherwise call the recursion protected function that
>  	 * will call the ftrace ops function.
> -- 
> 1.8.3.1
> 

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

[Steven Rostedt]

On Fri, 3 Apr 2015 11:29:04 +0200 (CEST)
Miroslav Benes <mbenes@suse.cz> wrote:

> Yes, this works but there is a slight difference to the previous behaviour 
> which I tried to preserve in my patch. ftrace_ops_get_func in your patch 
> does not check FTRACE_FORCE_LIST_FUNC. I do not know if that is a real 
> problem. Anyway your approach looks much better and dynamic trampolines 
> are now used for live patches. So in this context you can add

Hmm, I'll have to take a deeper look at that. Thanks for noticing. But
that will have to wait till Monday, as I have off today ;-)

> 
> Tested-by: Miroslav Benes <mbenes@suse.cz>
> 

Thanks for testing.

-- Steve