Re: [PATCH 0/2]: Add sparc64 ftrace support.

From: Ingo Molnar
Date: Wed May 14 2008 - 02:31:39 EST

Next message: Andrew Morton: "Re: [PATCH] common implementation of iterative div/mod"
Previous message: David Miller: "Re: [PATCH 1/1] [net/irda]: Fix a misalign access issue. (v2)"
In reply to: David Miller: "[PATCH 0/2]: Add sparc64 ftrace support."
Next in thread: Sam Ravnborg: "Re: [PATCH 0/2]: Add sparc64 ftrace support."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

* David Miller <davem@xxxxxxxxxxxxx> wrote:

> This was a lot more trivial than I expected, about a 20 minute hack.
> Most of the time was spent on test boots :)

applied, thanks David. Cool stuff! :-)

> The first patch removes the packed attribute from the ftrace_page blob
> of dynamic ftrace entries, because not only does it cause unaligned
> accesses on sparc64 it's also totally useless.

ok. It was so for purely histeric raisons, when we still tried to stuff
it all into 32 bytes per trace entry, 3-4 years ago or so.

> The second patch adds sparc64 ftrace support.
>
> One thing I noticed is that sparc64 uses an mcount implementation
> already for a quick-and-dirty stack usage checker. I tried to make
> them live alongside eachother.

my thinking would be to perhaps also do an ftrace/max-stack plugin that
would just measure the maximum stack footprint - coupled with emitting
the stack trace itself to the trace buffer. This way we could measure
the maximum stack footprint even of very yucky codepaths where we
couldnt do a printk or other ad-hoc things.

While such an ftrace/max-stack plugin would be something very similar to
and would reuse much of trace_functions.c, i think on the UI side it
should be a separate tracer plugin. I.e. we could tell people "activate
ftrace/max-stack" and they'd know what to do, and it would just do the
right thing by default after a:

echo max-stack > /debug/tracing/current_tracer

> Next, I think the mcount symbol export needs some tweaking. On sparc,
> the symbol _mcount is what the compiler references (this seems to be a
> sparc sysv4'ism) whereas on x86 it appears that plain "mcount" is
> used. I provide both symbols and we already have a local export of
> "_mcount" to take care of this. I think architectures should deal
> with this symbol exporting since it is different on every system.

ok, agreed, fixed this bug on x86 - see the first patch below.

[ also a patch integration detail: i took the liberty to move your
arch/sparc64/Kconfig's HAVE_FTRACE to the first spot of the SPARC64
select lines as per the second patch below - your patch had a conflict
against current mainline (which probably means you already changed
this file in your Sparc64 tree). By moving that entry to the first
line your tree and the ftrace tree should auto-merge just fine without
any conflicts. ]

btw., if you have any ftrace usability observations (or any observations
about it), let us know. ftrace tries to be self-contained (no user-space
tools required) and 'intuitive by default' - i.e. someone looking into
/debug/tracing/ and at the trace output for the first time should find
his way around and should be able to operate most of its functionality
without much effort.

But there lies the minor complication: it is pretty hard for us ftrace
developers to look into /debug/tracing/ "for the first time", so we rely
on others to tell us their first impressions, to improve the code :-)

Ingo

------------>
Subject: ftrace: fix mcount export bug
From: Ingo Molnar <mingo@xxxxxxx>
Date: Wed May 14 08:10:31 CEST 2008

David S. Miller noticed the following bug: the -pg instrumentation
function callback is named differently on each platform. On x86 it
is mcount, on sparc it is _mcount. So the export does not make sense
in kernel/trace/ftrace.c - move it to x86.

Signed-off-by: Ingo Molnar <mingo@xxxxxxx>
---
arch/x86/kernel/i386_ksyms_32.c | 3 +++
arch/x86/kernel/x8664_ksyms_64.c | 3 +++
kernel/trace/ftrace.c | 3 ---
3 files changed, 6 insertions(+), 3 deletions(-)

Index: linux/arch/x86/kernel/i386_ksyms_32.c
===================================================================
--- linux.orig/arch/x86/kernel/i386_ksyms_32.c
+++ linux/arch/x86/kernel/i386_ksyms_32.c
@@ -3,6 +3,9 @@
#include <asm/desc.h>
#include <asm/pgtable.h>

+/* mcount is defined in assembly */
+EXPORT_SYMBOL(mcount);
+
/* Networking helper routines. */
EXPORT_SYMBOL(csum_partial_copy_generic);

Index: linux/arch/x86/kernel/x8664_ksyms_64.c
===================================================================
--- linux.orig/arch/x86/kernel/x8664_ksyms_64.c
+++ linux/arch/x86/kernel/x8664_ksyms_64.c
@@ -10,6 +10,9 @@
#include <asm/pgtable.h>
#include <asm/desc.h>

+/* mcount is defined in assembly */
+EXPORT_SYMBOL(mcount);
+
EXPORT_SYMBOL(kernel_thread);

EXPORT_SYMBOL(__get_user_1);
Index: linux/kernel/trace/ftrace.c
===================================================================
--- linux.orig/kernel/trace/ftrace.c
+++ linux/kernel/trace/ftrace.c
@@ -50,9 +50,6 @@ static struct ftrace_ops ftrace_list_end
static struct ftrace_ops *ftrace_list __read_mostly = &ftrace_list_end;
ftrace_func_t ftrace_trace_function __read_mostly = ftrace_stub;

-/* mcount is defined per arch in assembly */
-EXPORT_SYMBOL(mcount);
-
void ftrace_list_func(unsigned long ip, unsigned long parent_ip)
{
struct ftrace_ops *op = ftrace_list;

------------------->
Subject: sparc64: add ftrace support.
From: David Miller <davem@xxxxxxxxxxxxx>
Date: Tue, 13 May 2008 22:06:59 -0700 (PDT)

Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
Signed-off-by: Ingo Molnar <mingo@xxxxxxx>
---
arch/sparc64/Kconfig | 1
arch/sparc64/Kconfig.debug | 2
arch/sparc64/kernel/Makefile | 1
arch/sparc64/kernel/ftrace.c | 99 +++++++++++++++++++++++++++++++++++++++++++
arch/sparc64/lib/mcount.S | 58 +++++++++++++++++++++++--
5 files changed, 156 insertions(+), 5 deletions(-)
create mode 100644 arch/sparc64/kernel/ftrace.c

Index: linux/arch/sparc64/Kconfig
===================================================================
--- linux.orig/arch/sparc64/Kconfig
+++ linux/arch/sparc64/Kconfig
@@ -11,6 +11,7 @@ config SPARC
config SPARC64
bool
default y
+ select HAVE_FTRACE
select HAVE_IDE
select HAVE_LMB
select HAVE_ARCH_KGDB
Index: linux/arch/sparc64/Kconfig.debug
===================================================================
--- linux.orig/arch/sparc64/Kconfig.debug
+++ linux/arch/sparc64/Kconfig.debug
@@ -33,7 +33,7 @@ config DEBUG_PAGEALLOC

config MCOUNT
bool
- depends on STACK_DEBUG
+ depends on STACK_DEBUG || FTRACE
default y

config FRAME_POINTER
Index: linux/arch/sparc64/kernel/Makefile
===================================================================
--- linux.orig/arch/sparc64/kernel/Makefile
+++ linux/arch/sparc64/kernel/Makefile
@@ -14,6 +14,7 @@ obj-y := process.o setup.o cpu.o idprom
power.o sbus.o sparc64_ksyms.o chmc.o \
visemul.o prom.o of_device.o hvapi.o sstate.o mdesc.o

+obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
obj-$(CONFIG_STACKTRACE) += stacktrace.o
obj-$(CONFIG_PCI) += ebus.o pci_common.o \
pci_psycho.o pci_sabre.o pci_schizo.o \
Index: linux/arch/sparc64/kernel/ftrace.c
===================================================================
--- /dev/null
+++ linux/arch/sparc64/kernel/ftrace.c
@@ -0,0 +1,99 @@
+#include <linux/spinlock.h>
+#include <linux/hardirq.h>
+#include <linux/ftrace.h>
+#include <linux/percpu.h>
+#include <linux/init.h>
+#include <linux/list.h>
+
+static const u32 ftrace_nop = 0x01000000;
+
+notrace int ftrace_ip_converted(unsigned long ip)
+{
+ u32 insn = *(u32 *) ip;
+
+ return (insn == ftrace_nop);
+}
+
+notrace unsigned char *ftrace_nop_replace(void)
+{
+ return (char *)&ftrace_nop;
+}
+
+notrace unsigned char *ftrace_call_replace(unsigned long ip, unsigned long addr)
+{
+ static u32 call;
+ s32 off;
+
+ off = ((s32)addr - (s32)ip);
+ call = 0x40000000 | ((u32)off >> 2);
+
+ return (unsigned char *) &call;
+}
+
+notrace int
+ftrace_modify_code(unsigned long ip, unsigned char *old_code,
+ unsigned char *new_code)
+{
+ u32 old = *(u32 *)old_code;
+ u32 new = *(u32 *)new_code;
+ u32 replaced;
+ int faulted;
+
+ __asm__ __volatile__(
+ "1: cas [%[ip]], %[old], %[new]\n"
+ " flush %[ip]\n"
+ " mov 0, %[faulted]\n"
+ "2:\n"
+ " .section .fixup,#alloc,#execinstr\n"
+ " .align 4\n"
+ "3: sethi %%hi(2b), %[faulted]\n"
+ " jmpl %[faulted] + %%lo(2b), %%g0\n"
+ " mov 1, %[faulted]\n"
+ " .previous\n"
+ " .section __ex_table,\"a\"\n"
+ " .align 4\n"
+ " .word 1b, 3b\n"
+ " .previous\n"
+ : "=r" (replaced), [faulted] "=r" (faulted)
+ : [new] "0" (new), [old] "r" (old), [ip] "r" (ip)
+ : "memory");
+
+ if (replaced != old && replaced != new)
+ faulted = 2;
+
+ return faulted;
+}
+
+notrace int ftrace_update_ftrace_func(ftrace_func_t func)
+{
+ unsigned long ip = (unsigned long)(&ftrace_call);
+ unsigned char old[4], *new;
+
+ memcpy(old, &ftrace_call, 4);
+ new = ftrace_call_replace(ip, (unsigned long)func);
+ return ftrace_modify_code(ip, old, new);
+}
+
+notrace int ftrace_mcount_set(unsigned long *data)
+{
+ unsigned long ip = (long)(&mcount_call);
+ unsigned long *addr = data;
+ unsigned char old[4], *new;
+
+ /*
+ * Replace the mcount stub with a pointer to the
+ * ip recorder function.
+ */
+ memcpy(old, &mcount_call, 4);
+ new = ftrace_call_replace(ip, *addr);
+ *addr = ftrace_modify_code(ip, old, new);
+
+ return 0;
+}
+
+
+int __init ftrace_dyn_arch_init(void *data)
+{
+ ftrace_mcount_set(data);
+ return 0;
+}
Index: linux/arch/sparc64/lib/mcount.S
===================================================================
--- linux.orig/arch/sparc64/lib/mcount.S
+++ linux/arch/sparc64/lib/mcount.S
@@ -28,10 +28,13 @@ ovstack:
.skip OVSTACKSIZE
#endif
.text
- .align 32
- .globl mcount, _mcount
-mcount:
+ .align 32
+ .globl _mcount
+ .type _mcount,#function
+ .globl mcount
+ .type mcount,#function
_mcount:
+mcount:
#ifdef CONFIG_STACK_DEBUG
/*
* Check whether %sp is dangerously low.
@@ -55,6 +58,53 @@ _mcount:
or %g3, %lo(panicstring), %o0
call prom_halt
nop
+1:
+#endif
+#ifdef CONFIG_FTRACE
+#ifdef CONFIG_DYNAMIC_FTRACE
+ mov %o7, %o0
+ .globl mcount_call
+mcount_call:
+ call ftrace_stub
+ mov %o0, %o7
+#else
+ sethi %hi(ftrace_trace_function), %g1
+ sethi %hi(ftrace_stub), %g2
+ ldx [%g1 + %lo(ftrace_trace_function)], %g1
+ or %g2, %lo(ftrace_stub), %g2
+ cmp %g1, %g2
+ be,pn %icc, 1f
+ mov %i7, %o1
+ jmpl %g1, %g0
+ mov %o7, %o0
+ /* not reached */
+1:
+#endif
#endif
-1: retl
+ retl
nop
+ .size _mcount,.-_mcount
+ .size mcount,.-mcount
+
+#ifdef CONFIG_FTRACE
+ .globl ftrace_stub
+ .type ftrace_stub,#function
+ftrace_stub:
+ retl
+ nop
+ .size ftrace_stub,.-ftrace_stub
+#ifdef CONFIG_DYNAMIC_FTRACE
+ .globl ftrace_caller
+ .type ftrace_caller,#function
+ftrace_caller:
+ mov %i7, %o1
+ mov %o7, %o0
+ .globl ftrace_call
+ftrace_call:
+ call ftrace_stub
+ mov %o0, %o7
+ retl
+ nop
+ .size ftrace_caller,.-ftrace_caller
+#endif
+#endif

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Andrew Morton: "Re: [PATCH] common implementation of iterative div/mod"
Previous message: David Miller: "Re: [PATCH 1/1] [net/irda]: Fix a misalign access issue. (v2)"
In reply to: David Miller: "[PATCH 0/2]: Add sparc64 ftrace support."
Next in thread: Sam Ravnborg: "Re: [PATCH 0/2]: Add sparc64 ftrace support."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]