[PATCH 0/2] ftrace: fixes for PPC

[PATCH 0/2] ftrace: fixes for PPC

[Steven Rostedt]

I spent the day chasing a bug that would hang PPC on boot up when ftrace
is configured in. I found that it was simply a stupid bug I did to
handle the non MCOUNT_RECORD case. Since I was testing only on x86,
and the MCOUNT_RECORD is automatically set for dynamic ftrace if
it is available, I did not test the case where MCOUNT_RECORD was not
set.

I have not finished porting MCOUNT_RECORD to PPC, but have found
that it has caused some issues for archs that do not support it yet.

This patch series handles these cases.

-- Steve

[PATCH 1/2] ftrace: use ftrace_release for all dynamic ftrace functions

[Steven Rostedt]

ftrace_release is necessary for all uses of dynamic ftrace and not just
the archs that have CONFIG_FTRACE_MCOUNT_RECORD defined.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 include/linux/ftrace.h |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: linux-tip.git/include/linux/ftrace.h
===================================================================
--- linux-tip.git.orig/include/linux/ftrace.h	2008-09-05 16:04:34.000000000 -0700
+++ linux-tip.git/include/linux/ftrace.h	2008-09-05 20:10:47.000000000 -0700
@@ -77,8 +77,10 @@ extern void mcount_call(void);
 
 extern int skip_trace(unsigned long ip);
 
-void ftrace_disable_daemon(void);
-void ftrace_enable_daemon(void);
+extern void ftrace_release(void *start, unsigned long size);
+
+extern void ftrace_disable_daemon(void);
+extern void ftrace_enable_daemon(void);
 
 #else
 # define skip_trace(ip)				({ 0; })
@@ -86,6 +88,7 @@ void ftrace_enable_daemon(void);
 # define ftrace_set_filter(buf, len, reset)	do { } while (0)
 # define ftrace_disable_daemon()		do { } while (0)
 # define ftrace_enable_daemon()			do { } while (0)
+static inline void ftrace_release(void *start, unsigned long size) { }
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 /* totally disable ftrace - can not re-enable after this */
@@ -199,12 +202,10 @@ static inline void ftrace_dump(void) { }
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 extern void ftrace_init(void);
 extern void ftrace_init_module(unsigned long *start, unsigned long *end);
-extern void ftrace_release(void *start, unsigned long size);
 #else
 static inline void ftrace_init(void) { }
 static inline void
 ftrace_init_module(unsigned long *start, unsigned long *end) { }
-static inline void ftrace_release(void *start, unsigned long size) { }
 #endif
 
 

-- 

[PATCH 2/2] ftrace: fix unlocking of hash

[Steven Rostedt]

This must be brown paper bag week for Steven Rostedt!

While working on ftrace for PPC, I discovered that the hash locking done
when CONFIG_FTRACE_MCOUNT_RECORD is not set, is totally incorrect.

With a cut and paste error, I had the hash lock macro to lock for both
hash_lock _and_ hash_unlock!

This bug did not affect x86 since this bug was introduced when
CONFIG_FTRACE_MCOUNT_RECORD was added to x86.

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

Index: linux-tip.git/kernel/trace/ftrace.c
===================================================================
--- linux-tip.git.orig/kernel/trace/ftrace.c	2008-09-05 21:53:46.000000000 -0700
+++ linux-tip.git/kernel/trace/ftrace.c	2008-09-05 21:57:57.000000000 -0700
@@ -170,7 +170,8 @@ static int __unregister_ftrace_function(
  */
 static DEFINE_SPINLOCK(ftrace_hash_lock);
 #define ftrace_hash_lock(flags)	  spin_lock_irqsave(&ftrace_hash_lock, flags)
-#define ftrace_hash_unlock(flags) spin_lock_irqsave(&ftrace_hash_lock, flags)
+#define ftrace_hash_unlock(flags) \
+			spin_unlock_irqrestore(&ftrace_hash_lock, flags)
 #else
 /* This is protected via the ftrace_lock with MCOUNT_RECORD. */
 #define ftrace_hash_lock(flags)   do { (void)(flags); } while (0)

-- 

Re: [PATCH 0/2] ftrace: fixes for PPC

[Ingo Molnar]

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

> I spent the day chasing a bug that would hang PPC on boot up when 
> ftrace is configured in. I found that it was simply a stupid bug I did 
> to handle the non MCOUNT_RECORD case. Since I was testing only on x86, 
> and the MCOUNT_RECORD is automatically set for dynamic ftrace if it is 
> available, I did not test the case where MCOUNT_RECORD was not set.
> 
> I have not finished porting MCOUNT_RECORD to PPC, but have found that 
> it has caused some issues for archs that do not support it yet.
> 
> This patch series handles these cases.

applied to tip/tracing/ftrace, thanks Steve.

	Ingo

Re: [PATCH 0/2] ftrace: fixes for PPC

[Josh Boyer]

On Sat, Sep 06, 2008 at 02:03:47PM +0200, Ingo Molnar wrote:
>
>* Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> I spent the day chasing a bug that would hang PPC on boot up when 
>> ftrace is configured in. I found that it was simply a stupid bug I did 
>> to handle the non MCOUNT_RECORD case. Since I was testing only on x86, 
>> and the MCOUNT_RECORD is automatically set for dynamic ftrace if it is 
>> available, I did not test the case where MCOUNT_RECORD was not set.
>> 
>> I have not finished porting MCOUNT_RECORD to PPC, but have found that 
>> it has caused some issues for archs that do not support it yet.
>> 
>> This patch series handles these cases.
>
>applied to tip/tracing/ftrace, thanks Steve.

Are these two merged yet?  I just spent the better part of the morning
trying to figure out why various Fedora kernels based on 2.6.27-rcX and
2.6.27 final hung on my G5 and finally got one booting with FTRACE
disabled.

josh

Re: [PATCH 0/2] ftrace: fixes for PPC

[Steven Rostedt]

On Thu, 16 Oct 2008, Josh Boyer wrote:

> On Sat, Sep 06, 2008 at 02:03:47PM +0200, Ingo Molnar wrote:
> >
> >* Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >> I spent the day chasing a bug that would hang PPC on boot up when 
> >> ftrace is configured in. I found that it was simply a stupid bug I did 
> >> to handle the non MCOUNT_RECORD case. Since I was testing only on x86, 
> >> and the MCOUNT_RECORD is automatically set for dynamic ftrace if it is 
> >> available, I did not test the case where MCOUNT_RECORD was not set.
> >> 
> >> I have not finished porting MCOUNT_RECORD to PPC, but have found that 
> >> it has caused some issues for archs that do not support it yet.
> >> 
> >> This patch series handles these cases.
> >
> >applied to tip/tracing/ftrace, thanks Steve.
> 
> Are these two merged yet?  I just spent the better part of the morning
> trying to figure out why various Fedora kernels based on 2.6.27-rcX and
> 2.6.27 final hung on my G5 and finally got one booting with FTRACE
> disabled.

My powerbook does not boot with ftrace enabled. I'm currently debugging 
it. When I get it working, you'll see patches.

They will be to linux-tip though, and not for the current mainline (unless 
the code in linux-tip gets merged).

-- Steve

Re: [PATCH 0/2] ftrace: fixes for PPC

[Josh Boyer]

On Thu, Oct 16, 2008 at 02:22:48PM -0400, Steven Rostedt wrote:
>
>On Thu, 16 Oct 2008, Josh Boyer wrote:
>
>> On Sat, Sep 06, 2008 at 02:03:47PM +0200, Ingo Molnar wrote:
>> >
>> >* Steven Rostedt <rostedt@goodmis.org> wrote:
>> >
>> >> I spent the day chasing a bug that would hang PPC on boot up when 
>> >> ftrace is configured in. I found that it was simply a stupid bug I did 
>> >> to handle the non MCOUNT_RECORD case. Since I was testing only on x86, 
>> >> and the MCOUNT_RECORD is automatically set for dynamic ftrace if it is 
>> >> available, I did not test the case where MCOUNT_RECORD was not set.
>> >> 
>> >> I have not finished porting MCOUNT_RECORD to PPC, but have found that 
>> >> it has caused some issues for archs that do not support it yet.
>> >> 
>> >> This patch series handles these cases.
>> >
>> >applied to tip/tracing/ftrace, thanks Steve.
>> 
>> Are these two merged yet?  I just spent the better part of the morning
>> trying to figure out why various Fedora kernels based on 2.6.27-rcX and
>> 2.6.27 final hung on my G5 and finally got one booting with FTRACE
>> disabled.
>
>My powerbook does not boot with ftrace enabled. I'm currently debugging 
>it. When I get it working, you'll see patches.

Well, that's why I asked.  You sent 2 patches out over a month ago that
don't appear to have shown up in any Linus or PowerPC tree.

>They will be to linux-tip though, and not for the current mainline (unless 
>the code in linux-tip gets merged).

If linux-tip is some x86 topic branch, I think that's the wrong place for
fixes that are needed on non-x86 arches.

josh

Re: [PATCH 0/2] ftrace: fixes for PPC

[Steven Rostedt]

On Thu, 16 Oct 2008, Josh Boyer wrote:
> >
> >My powerbook does not boot with ftrace enabled. I'm currently debugging 
> >it. When I get it working, you'll see patches.
> 
> Well, that's why I asked.  You sent 2 patches out over a month ago that
> don't appear to have shown up in any Linus or PowerPC tree.

OK, I'll see if those patches fixes my problem.

> 
> >They will be to linux-tip though, and not for the current mainline (unless 
> >the code in linux-tip gets merged).
> 
> If linux-tip is some x86 topic branch, I think that's the wrong place for
> fixes that are needed on non-x86 arches.

linux-tip has been used by more that x86 specific changes. It's where we 
have been developing ftrace itself.

My original port to PPC first landed into linux-tip.

-- Steve

Re: [PATCH 0/2] ftrace: fixes for PPC

[Steven Rostedt]

On Thu, 16 Oct 2008, Josh Boyer wrote:
> 
> Well, that's why I asked.  You sent 2 patches out over a month ago that
> don't appear to have shown up in any Linus or PowerPC tree.

Oh, the patches I sent on here, are not to solve this issue. It was 
actually solving issues in linux-tip itself.

I'm still looking into the cause for ftrace not to boot on PPC.

-- Steve

Re: [PATCH 0/2] ftrace: fixes for PPC

[Josh Boyer]

On Thu, 16 Oct 2008 14:45:15 -0400 (EDT)
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> On Thu, 16 Oct 2008, Josh Boyer wrote:
> > 
> > Well, that's why I asked.  You sent 2 patches out over a month ago that
> > don't appear to have shown up in any Linus or PowerPC tree.
> 
> Oh, the patches I sent on here, are not to solve this issue. It was 
> actually solving issues in linux-tip itself.

Totally confused as to what linux-tip is, but ok.

> I'm still looking into the cause for ftrace not to boot on PPC.

There were issues with -pg and some other compile flag on PPC at one
point.  I think you worked that out with Ben, but I don't recall.

Anyway, if you want a tester let me know.  It seems 2.6.27.1 should be
fine since FTRACE was disabled, but for .28-rc1 it would be cool if it
worked :).

josh

[PATCH] ftrace: powerpc: remove startup functions from tracing

[Steven Rostedt]

[
  Josh, can you see if this allows you to boot on PowerPC?
  It worked on my powerbook.
]

The early init code in PowerPC is not mapped to their final locations
and all jumps and memory references must be done with relative jumps
and accesses.

The lib files in the powerpc directory are called in early boot, and
since mcount will perform direct access to memory, the lib files need
not be traced.

Signed-off-by: Steven Rostedt <srostedt@redhat.com>
---
 arch/powerpc/lib/Makefile |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux-compile.git/arch/powerpc/lib/Makefile
===================================================================
--- linux-compile.git.orig/arch/powerpc/lib/Makefile	2008-10-16 19:26:39.000000000 -0400
+++ linux-compile.git/arch/powerpc/lib/Makefile	2008-10-16 19:26:49.000000000 -0400
@@ -2,6 +2,11 @@
 # Makefile for ppc-specific library files..
 #
 
+ifdef CONFIG_FTRACE
+ORIG_CFLAGS := $(KBUILD_CFLAGS)
+KBUILD_CFLAGS = $(subst -pg,,$(ORIG_CFLAGS))
+endif
+
 ifeq ($(CONFIG_PPC64),y)
 EXTRA_CFLAGS		+= -mno-minimal-toc
 endif

Re: [PATCH] ftrace: powerpc: remove startup functions from tracing

[Benjamin Herrenschmidt]

On Thu, 2008-10-16 at 20:48 -0400, Steven Rostedt wrote:
> 
> The early init code in PowerPC is not mapped to their final locations
> and all jumps and memory references must be done with relative jumps
> and accesses.
> 
> The lib files in the powerpc directory are called in early boot, and
> since mcount will perform direct access to memory, the lib files need
> not be traced.

This is annoying though, because that means things like memcpy,
copy_to_from_user etc... can't be traced. On the other hand a lot
of that is asm and already doesn't call mcount.

Cheers,
Ben.

Re: [PATCH] ftrace: powerpc: remove startup functions from tracing

[Steven Rostedt]

On Fri, 17 Oct 2008, Benjamin Herrenschmidt wrote:

> On Thu, 2008-10-16 at 20:48 -0400, Steven Rostedt wrote:
> > 
> > The early init code in PowerPC is not mapped to their final locations
> > and all jumps and memory references must be done with relative jumps
> > and accesses.
> > 
> > The lib files in the powerpc directory are called in early boot, and
> > since mcount will perform direct access to memory, the lib files need
> > not be traced.
> 
> This is annoying though, because that means things like memcpy,
> copy_to_from_user etc... can't be traced. On the other hand a lot
> of that is asm and already doesn't call mcount.

Yeah, I know. I was going to pick and choose which files in there should 
be converted, but then I saw that they were mostly asm, and it seemed to 
be better safe than sorry.

Sure, we could probably bring it down a bit. But I'm a bit paranoid it may 
cause someone else not to boot because of something else being called at 
early boot up.

But don't worry. When we get MCOUNT_RECORD ported to PPC this no longer 
becomes an issue, and we can simply do a
"ifndef CONFIG_FTRACE_MCOUNT_RECORD" around the -pg removal in the 
Makefile. (or what ever the Makefile syntax is for config options)

-- Steve

Re: [PATCH 0/2] ftrace: fixes for PPC

[Chris Friesen]

Josh Boyer wrote:

 > Are these two merged yet?  I just spent the better part of the morning
 > trying to figure out why various Fedora kernels based on 2.6.27-rcX and
 > 2.6.27 final hung on my G5 and finally got one booting with FTRACE
 > disabled.

Until recently I could boot my G5 with FTRACE enabled, but it totally screwed 
up load balancing.

As of a couple days ago, -tip hung on boot due to the following genirq 
issue:


diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 570d1ea..1c178ae 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -132,7 +132,7 @@ int set_irq_type(unsigned int irq, unsigned int type)
    		return 0;

    	spin_lock_irqsave(&desc->lock, flags);
-	ret = __irq_set_trigger(desc, irq, flags);
+	ret = __irq_set_trigger(desc, irq, type);
    	spin_unlock_irqrestore(&desc->lock, flags);
    	return ret;
    }



Chris

Re: [PATCH 0/2] ftrace: fixes for PPC

[Steven Rostedt]

On Thu, 16 Oct 2008, Josh Boyer wrote:
> > 
> > On Thu, 16 Oct 2008, Josh Boyer wrote:
> > > 
> > > Well, that's why I asked.  You sent 2 patches out over a month ago that
> > > don't appear to have shown up in any Linus or PowerPC tree.
> > 
> > Oh, the patches I sent on here, are not to solve this issue. It was 
> > actually solving issues in linux-tip itself.
> 
> Totally confused as to what linux-tip is, but ok.
> 
> > I'm still looking into the cause for ftrace not to boot on PPC.
> 
> There were issues with -pg and some other compile flag on PPC at one
> point.  I think you worked that out with Ben, but I don't recall.
> 
> Anyway, if you want a tester let me know.  It seems 2.6.27.1 should be
> fine since FTRACE was disabled, but for .28-rc1 it would be cool if it
> worked :).

Hi Josh,

I've been looking deeper at the code for PPC. I realized that my PPC64 box 
that I've been testing on did not use modules. While looking at the module 
code it dawned on me the dynamic ftrace needs a bit of work. This is 
because the way modules are handled in PPC (and other architectures as 
well).  The jmps used by mcount is a 24 bit jump. Since the modules are 
loaded farther than 24bits away, a trampoline is needed.

A bit of rework is needed in the ftrace infrastructure to handle the 
trampoline. Too much work to go into 28. I'll start working on code that 
can hopefully be ready and tested for 29. It's not that major of a change, 
but since the merge window for 28 has already been opened, we would like 
to get a bit more testing in before we hand it over to Linus.

-- Steve

Re: [PATCH 0/2] ftrace: fixes for PPC

[Josh Boyer]

On Mon, Oct 20, 2008 at 12:30:33PM -0400, Steven Rostedt wrote:
>> Anyway, if you want a tester let me know.  It seems 2.6.27.1 should be
>> fine since FTRACE was disabled, but for .28-rc1 it would be cool if it
>> worked :).
>
>Hi Josh,
>
>I've been looking deeper at the code for PPC. I realized that my PPC64 box 
>that I've been testing on did not use modules. While looking at the module 
>code it dawned on me the dynamic ftrace needs a bit of work. This is 
>because the way modules are handled in PPC (and other architectures as 
>well).  The jmps used by mcount is a 24 bit jump. Since the modules are 
>loaded farther than 24bits away, a trampoline is needed.

Ah, indeed.

>A bit of rework is needed in the ftrace infrastructure to handle the 
>trampoline. Too much work to go into 28. I'll start working on code that 
>can hopefully be ready and tested for 29. It's not that major of a change, 
>but since the merge window for 28 has already been opened, we would like 
>to get a bit more testing in before we hand it over to Linus.

That seems like a good plan.  Should we mark dynamic ftrace as X86 only
in Kconfig for .28 to prevent people from inadvertently setting it?

josh

Re: [PATCH 0/2] ftrace: fixes for PPC

[Steven Rostedt]

On Mon, 20 Oct 2008, Josh Boyer wrote:

> On Mon, Oct 20, 2008 at 12:30:33PM -0400, Steven Rostedt wrote:
> >> Anyway, if you want a tester let me know.  It seems 2.6.27.1 should be
> >> fine since FTRACE was disabled, but for .28-rc1 it would be cool if it
> >> worked :).
> >
> >Hi Josh,
> >
> >I've been looking deeper at the code for PPC. I realized that my PPC64 box 
> >that I've been testing on did not use modules. While looking at the module 
> >code it dawned on me the dynamic ftrace needs a bit of work. This is 
> >because the way modules are handled in PPC (and other architectures as 
> >well).  The jmps used by mcount is a 24 bit jump. Since the modules are 
> >loaded farther than 24bits away, a trampoline is needed.
> 
> Ah, indeed.
> 
> >A bit of rework is needed in the ftrace infrastructure to handle the 
> >trampoline. Too much work to go into 28. I'll start working on code that 
> >can hopefully be ready and tested for 29. It's not that major of a change, 
> >but since the merge window for 28 has already been opened, we would like 
> >to get a bit more testing in before we hand it over to Linus.
> 
> That seems like a good plan.  Should we mark dynamic ftrace as X86 only
> in Kconfig for .28 to prevent people from inadvertently setting it?

Well, we probably should disable it for PPC at least. I don't know if 
sparc has the same issue.

David?

-- Steve

Re: [PATCH 0/2] ftrace: fixes for PPC

[Frédéric Weisbecker]

2008/10/20 Josh Boyer <jwboyer@linux.vnet.ibm.com>:
> That seems like a good plan.  Should we mark dynamic ftrace as X86 only
> in Kconfig for .28 to prevent people from inadvertently setting it?

Hi Josh.

There are other arch that support ftrace as well like sh or sparc64
(I'm currently working on
an implementation for mips).
So the choice would be better between waiting for a fix or disable
dynamic ftrace
on Kconfig only for PowerPC.

Re: [PATCH 0/2] ftrace: fixes for PPC

[Steven Rostedt]

On Mon, 20 Oct 2008, Fr?d?ric Weisbecker wrote:

> There are other arch that support ftrace as well like sh or sparc64
> (I'm currently working on
> an implementation for mips).
> So the choice would be better between waiting for a fix or disable
> dynamic ftrace
> on Kconfig only for PowerPC.

Hi Frederic,

I believe MIPS has the same issues as PPC. Doesn't it use a trampoline 
too?  I want to make the generic code handle trampolines a bit better. 
This is one of the problems with developing in x86. It avoids a lot of the 
issues that other archs might have.

-- Steve

Re: [PATCH 0/2] ftrace: fixes for PPC

[Frédéric Weisbecker]

2008/10/20 Steven Rostedt <rostedt@goodmis.org>:
> I believe MIPS has the same issues as PPC. Doesn't it use a trampoline
> too?

No it doesn't seem to. If I'm not wrong, MIPS uses only the elf
relocation table to relocate
its addresses.

> I want to make the generic code handle trampolines a bit better.

Do you think there could be a generic approach to handles these trampolines?