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

[Shaohua Li <shaohua.li@intel.com>]

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