[PATCH] ftrace: warn on failure to disable mcount callers

[PATCH] ftrace: warn on failure to disable mcount callers

[Steven Rostedt]

With the recent updates to ftrace, there should not be any failures when
modifying the code. If there is, then we need to warn about it.

This patch has a cleaned up version of the code that I used to discover
that the weak symbols were causing failures. 

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/ftrace.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Index: linux-tip.git/kernel/trace/ftrace.c
===================================================================
--- linux-tip.git.orig/kernel/trace/ftrace.c	2008-08-20 11:56:12.000000000 -0400
+++ linux-tip.git/kernel/trace/ftrace.c	2008-08-20 12:20:45.000000000 -0400
@@ -574,6 +574,16 @@ static void ftrace_shutdown_replenish(vo
 	ftrace_pages->next = (void *)get_zeroed_page(GFP_KERNEL);
 }
 
+static void print_ip_ins(const char *fmt, unsigned char *p)
+{
+	int i;
+
+	printk(KERN_CONT "%s", fmt);
+
+	for (i = 0; i < MCOUNT_INSN_SIZE; i++)
+		printk(KERN_CONT "%s%02x", i ? ":" : "", p[i]);
+}
+
 static int
 ftrace_code_disable(struct dyn_ftrace *rec)
 {
@@ -588,6 +598,21 @@ ftrace_code_disable(struct dyn_ftrace *r
 
 	failed = ftrace_modify_code(ip, call, nop);
 	if (failed) {
+		switch (failed) {
+		case 1:
+			pr_info("WARNING: ftrace faulted on modifying ");
+			print_ip_sym(ip);
+			break;
+		case 2:
+			pr_info("WARNING: ftrace failed to modify ");
+			print_ip_sym(ip);
+			print_ip_ins(" expected: ", call);
+			print_ip_ins(" actual: ", (unsigned char *)ip);
+			print_ip_ins(" replace: ", nop);
+			printk(KERN_CONT "\n");
+			break;
+		}
+
 		rec->flags |= FTRACE_FL_FAILED;
 		return 0;
 	}

Re: [PATCH] ftrace: warn on failure to disable mcount callers

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> +			pr_info("WARNING: ftrace faulted on modifying ");
> +			print_ip_sym(ip);

please use the WARN() API, so that it gets picked up by automation.

	Ingo

[PATCH v2] ftrace: warn on failure to disable mcount callers

[Steven Rostedt]

On Thu, 21 Aug 2008, Ingo Molnar wrote:

> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > +			pr_info("WARNING: ftrace faulted on modifying ");
> > +			print_ip_sym(ip);
> 
> please use the WARN() API, so that it gets picked up by automation.
> 
> 	Ingo
> 

[
  Added WARN_ON as requested above.
]


With the recent updates to ftrace, there should not be any failures when
modifying the code. If there is, then we need to warn about it.

This patch has a cleaned up version of the code that I used to discover
that the weak symbols were causing failures. 

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 kernel/trace/ftrace.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Index: linux-tip.git/kernel/trace/ftrace.c
===================================================================
--- linux-tip.git.orig/kernel/trace/ftrace.c	2008-08-20 21:58:19.000000000 -0400
+++ linux-tip.git/kernel/trace/ftrace.c	2008-08-21 10:19:09.000000000 -0400
@@ -574,6 +574,16 @@ static void ftrace_shutdown_replenish(vo
 	ftrace_pages->next = (void *)get_zeroed_page(GFP_KERNEL);
 }
 
+static void print_ip_ins(const char *fmt, unsigned char *p)
+{
+	int i;
+
+	printk(KERN_CONT "%s", fmt);
+
+	for (i = 0; i < MCOUNT_INSN_SIZE; i++)
+		printk(KERN_CONT "%s%02x", i ? ":" : "", p[i]);
+}
+
 static int
 ftrace_code_disable(struct dyn_ftrace *rec)
 {
@@ -588,6 +598,23 @@ ftrace_code_disable(struct dyn_ftrace *r
 
 	failed = ftrace_modify_code(ip, call, nop);
 	if (failed) {
+		switch (failed) {
+		case 1:
+			WARN_ON_ONCE(1);
+			pr_info("ftrace faulted on modifying ");
+			print_ip_sym(ip);
+			break;
+		case 2:
+			WARN_ON_ONCE(1);
+			pr_info("ftrace failed to modify ");
+			print_ip_sym(ip);
+			print_ip_ins(" expected: ", call);
+			print_ip_ins(" actual: ", (unsigned char *)ip);
+			print_ip_ins(" replace: ", nop);
+			printk(KERN_CONT "\n");
+			break;
+		}
+
 		rec->flags |= FTRACE_FL_FAILED;
 		return 0;
 	}

Re: [PATCH v2] ftrace: warn on failure to disable mcount callers

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> +			WARN_ON_ONCE(1);
> +			pr_info("ftrace faulted on modifying ");
> +			print_ip_sym(ip);
> +			break;
> +		case 2:
> +			WARN_ON_ONCE(1);
> +			pr_info("ftrace failed to modify ");
> +			print_ip_sym(ip);
> +			print_ip_ins(" expected: ", call);
> +			print_ip_ins(" actual: ", (unsigned char *)ip);
> +			print_ip_ins(" replace: ", nop);
> +			printk(KERN_CONT "\n");
> +			break;

hm, i think it makes little sense to only print out the stacktrace once, 
but to print out the rest all the time.

If there's such a failure then ftrace should warn once, with stacktrace 
and everthing else, and turn itself off permanently. That makes it sure 
that we 1) get the report 2) dont spam the user 3) keep the system 
working 4) turn off the malfunctioning component.

	Ingo

Re: [PATCH v2] ftrace: warn on failure to disable mcount callers

[Steven Rostedt]

On Fri, 22 Aug 2008, Ingo Molnar wrote:

> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > +			WARN_ON_ONCE(1);
> > +			pr_info("ftrace faulted on modifying ");
> > +			print_ip_sym(ip);
> > +			break;
> > +		case 2:
> > +			WARN_ON_ONCE(1);
> > +			pr_info("ftrace failed to modify ");
> > +			print_ip_sym(ip);
> > +			print_ip_ins(" expected: ", call);
> > +			print_ip_ins(" actual: ", (unsigned char *)ip);
> > +			print_ip_ins(" replace: ", nop);
> > +			printk(KERN_CONT "\n");
> > +			break;
> 
> hm, i think it makes little sense to only print out the stacktrace once, 
> but to print out the rest all the time.

Actually, the more functions that are printed the easier it is to find 
where things went wrong. The backtrace is only to trigger automated tools,
and helps very little in diagnosing the problem.

I had to examine several functions to figure out the issue with the weak
symbols.

> 
> If there's such a failure then ftrace should warn once, with stacktrace 
> and everthing else, and turn itself off permanently. That makes it sure 
> that we 1) get the report 2) dont spam the user 3) keep the system 
> working 4) turn off the malfunctioning component.

The backtrace should give us 1 without a problem. In fact if we do 2, 
they are more likely to complain ;-)

I think 3 and 4 are related, and I agree, if this is detected, we turn 
ftrace off.

Note, this only prints out on boot up and module loading. The spamming
should not be that bad.

-- Steve