[PATCH 0/2] tracing/urgent: fixes for nop patching omissions

[PATCH 0/2] tracing/urgent: fixes for nop patching omissions

[Frederic Weisbecker]

Hi Ingo, Steven,

The first patch in the set fixes a bug on which ftrace silently
ommitted to recover nop patching after function tracing.

The issue is present in .31 and .30 as well

The second patch just adds a warning to prevent further mistakes like
that.

Thanks.

Frederic Weisbecker (2):
  tracing/urgent: fix unbalanced ftrace_start_up
  tracing/urgent: warn in case of ftrace_start_up inbalance

 kernel/trace/ftrace.c          |    7 +++++++
 kernel/trace/trace_functions.c |    8 +++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

The following changes since commit 44ad18e0a65e296b2e68a1452509f6222cdce743:
  Steven Rostedt (1):
        tracing: update sample event documentation

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
	tracing/urgent

Frederic Weisbecker (2):
      tracing/urgent: fix unbalanced ftrace_start_up
      tracing/urgent: warn in case of ftrace_start_up inbalance

 kernel/trace/ftrace.c          |    7 +++++++
 kernel/trace/trace_functions.c |    8 +++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

[PATCH 1/2] tracing/urgent: fix unbalanced ftrace_start_up

[Frederic Weisbecker]

Perfcounter reports the following stats for a wide system
profiling:

 #
 # (2364 samples)
 #
 # Overhead  Symbol
 # ........  ......
 #
    15.40%  [k] mwait_idle_with_hints
     8.29%  [k] read_hpet
     5.75%  [k] ftrace_caller
     3.60%  [k] ftrace_call
     [...]

This snapshot has been taken while neither the function tracer nor
the function graph tracer was running.
With dynamic ftrace, such results show a wrong ftrace behaviour
because all calls to ftrace_caller or ftrace_graph_caller (the patched
calls to mcount) are supposed to be patched into nop if none of those
tracers are running.

The problem occurs after the first run of the function tracer. Once we
launch it a second time, the callsites will never be nopped back,
unless you set custom filters.
For example it happens during the self tests at boot time.
The function tracer selftest runs, and then the dynamic tracing is
tested too. After that, the callsites are left un-nopped.

This is because the reset callback of the function tracer tries to
unregister two ftrace callbacks in once: the common function tracer
and the function tracer with stack backtrace, regardless of which
one is currently in use.
It then creates an unbalance on ftrace_start_up value which is expected
to be zero when the last ftrace callback is unregistered. When it
reaches zero, the FTRACE_DISABLE_CALLS is set on the next ftrace
command, triggering the patching into nop. But since it becomes
unbalanced, ie becomes lower than zero, if the kernel functions
are patched again (as in every further function tracer runs), they
won't ever be nopped back.

Note that ftrace_call and ftrace_graph_call are still patched back
to ftrace_stub in the off case, but not the callers of ftrace_call
and ftrace_graph_caller. It means that the tracing is well deactivated
but we waste a useless call into every kernel function.

This patch just unregisters the right ftrace_ops for the function
tracer on its reset callback and ignores the other one which is
not registered, fixing the unbalance. The problem also happens
is .30

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: stable@kernel.org
---
 kernel/trace/trace_functions.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index c9a0b7d..90f1347 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -193,9 +193,11 @@ static void tracing_start_function_trace(void)
 static void tracing_stop_function_trace(void)
 {
 	ftrace_function_enabled = 0;
-	/* OK if they are not registered */
-	unregister_ftrace_function(&trace_stack_ops);
-	unregister_ftrace_function(&trace_ops);
+
+	if (func_flags.val & TRACE_FUNC_OPT_STACK)
+		unregister_ftrace_function(&trace_stack_ops);
+	else
+		unregister_ftrace_function(&trace_ops);
 }
 
 static int func_set_flag(u32 old_flags, u32 bit, int set)
-- 
1.6.2.3

[PATCH 2/2] tracing/urgent: warn in case of ftrace_start_up inbalance

[Frederic Weisbecker]

Prevent from further ftrace_start_up inbalances so that we avoid
future nop patching omissions with dynamic ftrace.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index bb60732..3718d55 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1224,6 +1224,13 @@ static void ftrace_shutdown(int command)
 		return;
 
 	ftrace_start_up--;
+	/*
+	 * Just warn in case of unbalance, no need to kill ftrace, it's not
+	 * critical but the ftrace_call callers may be never nopped again after
+	 * further ftrace uses.
+	 */
+	WARN_ON_ONCE(ftrace_start_up < 0);
+
 	if (!ftrace_start_up)
 		command |= FTRACE_DISABLE_CALLS;
 
-- 
1.6.2.3

Re: [PATCH 0/2] tracing/urgent: fixes for nop patching omissions

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Hi Ingo, Steven,
> 
> The first patch in the set fixes a bug on which ftrace silently 
> ommitted to recover nop patching after function tracing.
> 
> The issue is present in .31 and .30 as well
> 
> The second patch just adds a warning to prevent further mistakes 
> like that.
> 
> Thanks.
> 
> Frederic Weisbecker (2):
>   tracing/urgent: fix unbalanced ftrace_start_up
>   tracing/urgent: warn in case of ftrace_start_up inbalance
> 
>  kernel/trace/ftrace.c          |    7 +++++++
>  kernel/trace/trace_functions.c |    8 +++++---
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> The following changes since commit 44ad18e0a65e296b2e68a1452509f6222cdce743:
>   Steven Rostedt (1):
>         tracing: update sample event documentation
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> 	tracing/urgent
> 
> Frederic Weisbecker (2):
>       tracing/urgent: fix unbalanced ftrace_start_up
>       tracing/urgent: warn in case of ftrace_start_up inbalance
> 
>  kernel/trace/ftrace.c          |    7 +++++++
>  kernel/trace/trace_functions.c |    8 +++++---
>  2 files changed, 12 insertions(+), 3 deletions(-)

Pulled, thanks Frederic!

	Ingo