Re: [PATCH 5/5] IA64 dynamic ftrace support

Shaohua Li
2008-12-24T00:54:55Z

On Tue, Dec 23, 2008 at 10:35:49PM +0800, Steven Rostedt wrote:
>
> On Tue, 2008-12-23 at 14:08 +0800, Shaohua Li wrote:
> > IA64 dynamic ftrace support. The main tricky thing here is to support module.
> > In a module, each routine's mcount call will call a PLT stub, which
> > will call kernel mcount. We can't simply make the mcount call call into
> > kernel mcount, as kernel and mocule have different gp and the
> > instruction just supports 25bit offset. So I introduced a new PLT stub,
> > which will call into kernel ftrace_caller. When module loading, all
> > mcount call will be converted to nop. When the nop is converted to call,
> > we make the call to the new PLT stub instead of old mcount PLT stub.
> >
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> > ---
> > arch/ia64/Kconfig | 2
> > arch/ia64/include/asm/ftrace.h | 17 ++
> > arch/ia64/include/asm/module.h | 4
> > arch/ia64/kernel/Makefile | 5
> > arch/ia64/kernel/entry.S | 37 ++++++
> > arch/ia64/kernel/ftrace.c | 234 +++++++++++++++++++++++++++++++++++++++++
> > arch/ia64/kernel/module.c | 15 ++
> > scripts/recordmcount.pl | 7 +
> > 8 files changed, 321 insertions(+)
> >
> > Index: linux/arch/ia64/Kconfig
> > =================================> > --- linux.orig/arch/ia64/Kconfig 2008-12-23 13:13:17.000000000 +0800
> > +++ linux/arch/ia64/Kconfig 2008-12-23 13:30:09.000000000 +0800
> > @@ -21,6 +21,8 @@ config IA64
> > select HAVE_OPROFILE
> > select HAVE_KPROBES
> > select HAVE_KRETPROBES
> > + select HAVE_FTRACE_MCOUNT_RECORD
> > + select HAVE_DYNAMIC_FTRACE
> > select HAVE_FUNCTION_TRACER
> > select HAVE_DMA_ATTRS
> > select HAVE_KVM
> > Index: linux/arch/ia64/kernel/Makefile
> > =================================> > --- linux.orig/arch/ia64/kernel/Makefile 2008-12-23 13:11:27.000000000 +0800
> > +++ linux/arch/ia64/kernel/Makefile 2008-12-23 13:30:09.000000000 +0800
> > @@ -2,6 +2,10 @@
> > # Makefile for the linux kernel.
> > #
> >
> > +ifdef CONFIG_DYNAMIC_FTRACE
> > +CFLAGS_REMOVE_ftrace.o = -pg
> > +endif
> > +
> > extra-y := head.o init_task.o vmlinux.lds
> >
> > obj-y := acpi.o entry.o efi.o efi_stub.o gate-data.o fsys.o ia64_ksyms.o irq.o irq_ia64.o \
> > @@ -28,6 +32,7 @@ obj-$(CONFIG_IA64_CYCLONE) += cyclone.o
> > obj-$(CONFIG_CPU_FREQ) += cpufreq/
> > obj-$(CONFIG_IA64_MCA_RECOVERY) += mca_recovery.o
> > obj-$(CONFIG_KPROBES) += kprobes.o jprobes.o
> > +obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
> > obj-$(CONFIG_KEXEC) += machine_kexec.o relocate_kernel.o crash.o
> > obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
> > obj-$(CONFIG_IA64_UNCACHED_ALLOCATOR) += uncached.o
> > Index: linux/arch/ia64/kernel/ftrace.c
> > =================================> > --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> > +++ linux/arch/ia64/kernel/ftrace.c 2008-12-23 13:30:09.000000000 +0800
> > @@ -0,0 +1,234 @@
> > +/*
> > + * Dynamic function tracing support.
> > + *
> > + * Copyright (C) 2008 Shaohua Li <shaohua.li@intel.com>
> > + *
> > + * For licencing details, see COPYING.
> > + *
> > + * Defines low-level handling of mcount calls when the kernel
> > + * is compiled with the -pg flag. When using dynamic ftrace, the
> > + * mcount call-sites get patched lazily with NOP till they are
> > + * enabled. All code mutation routines here take effect atomically.
> > + */
> > +
> > +#include <linux/uaccess.h>
> > +#include <linux/ftrace.h>
> > +
> > +#include <asm/cacheflush.h>
> > +#include <asm/ftrace.h>
> > +
> > +static unsigned char ftrace_nop_code[MCOUNT_INSN_SIZE] = {
> > + 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, /* nop.m 0x0 */
> > + 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, /* nop.i 0x0 */
> > + 0x00, 0x00, 0x04, 0x00, /* nop.i 0x0 */
> > + 0x01, 0x00, 0x00, 0x00, 0x01, 0x00, /* nop.m 0x0 */
> > + 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, /* nop.i 0x0 */
> > + 0x00, 0x00, 0x04, 0x00 /* nop.i 0x0;; */
> > +};
>
> As I stated before, you can not have a multi-line nop. The best you can
> do is add a jump over the entire call, and only if that jump is a single
> instruction.
>
> Think about it, lets do a simple scenario.
>
> Some process is running inside the kernel and starts to execute one of
> these 'nop' sequences. After executing the third nop, it is preempted
> (before finishing the other nops). During this preemption, you happen to
> start the tracer. Kstop_machine is called and all processes are now
> stopped. The kstop_machine changes the nop to the call of a function
> tracer and resumes. Now that original process gets scheduled back in,
> but the code is no longer nops, it has actual code and a call. But we
> missed the first 3 commands. Not to mention, it looks like the op codes
> are not even 4 or 8 byte aligned. So we could be executing something
> that is not even a command. BAM! Crash! kernel oops! ;-)
Makes sense, I'll try a jump approach.

> > +static unsigned char *ftrace_nop_replace(void)
> > +{
> > + return ftrace_nop_code;
> > +}
> > +
> > +/* In IA64, each function will be added below two bundles with -pg option */
> > +static unsigned char __attribute__((aligned(8)))
> > +ftrace_call_code[MCOUNT_INSN_SIZE] = {
> > + 0x02, 0x40, 0x31, 0x10, 0x80, 0x05, /* alloc r40=ar.pfs,12,8,0 */
> > + 0xb0, 0x02, 0x00, 0x00, 0x42, 0x40, /* mov r43=r0;; */
> > + 0x05, 0x00, 0xc4, 0x00, /* mov r42° */
> > + 0x11, 0x48, 0x01, 0x02, 0x00, 0x21, /* mov r41=r1 */
> > + 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, /* nop.i 0x0 */
>
> If you made your own PLT stub, could you just change the one line to
> jump to that stub?
A simple jump to PLT stub doesn't work in IA64, as a lot of registers
should be saved. I'll do more investigation.

> > Index: linux/scripts/recordmcount.pl
> > =================================> > --- linux.orig/scripts/recordmcount.pl 2008-12-23 13:24:59.000000000 +0800
> > +++ linux/scripts/recordmcount.pl 2008-12-23 13:30:09.000000000 +0800
> > @@ -206,6 +206,13 @@ if ($arch eq "x86_64") {
> > $alignment = 2;
> > $section_type = '%progbits';
> >
> > +} elsif ($arch eq "ia64") {
> > + $mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\s_mcount\$";
> > + $type = "data8";
> > +
> > + if ($is_module eq "0") {
> > + $cc .= " -mconstant-gp";
> > + }
>
> I wonder if it would be better to pass in CFLAGS and then be able to
> parse that instead. Then we can find out a lot more about what we are
> working on.
CFLAGS seems have a lot of useless flags here.

Thanks,
Shaohua