[PATCH 00/11] ftrace: clean ups and fixes - Kernel

This is a discussion on [PATCH 00/11] ftrace: clean ups and fixes - Kernel ; This is a series of patches to make ftrace more robust and clean ups. The first couple of patches fix the recordmount.pl script and changes it to only record the .text section functions. This means that the init sections will ...

+ Reply to Thread
Page 1 of 2 1 2 LastLast
Results 1 to 20 of 31

Thread: [PATCH 00/11] ftrace: clean ups and fixes

  1. [PATCH 00/11] ftrace: clean ups and fixes

    This is a series of patches to make ftrace more robust and clean ups.

    The first couple of patches fix the recordmount.pl script and changes
    it to only record the .text section functions. This means that
    the init sections will not be processed.

    I still have a patch to add notrace to the init sections, and not for
    safety reasons, but for perfomance. Since the init sections will not be
    processed, they will still call mcount. Note, mcount is just a ret,
    but why have the init code waste CPU cycles to call a stub function?

    A FTRACE_WARN_ON is added to change all WARN_ONS to not only print a
    warning, but also to disable ftrace as well.

    The later patches are a bit more drastic. Since the daemon is error prone,
    I stripped it out. In doing so, I have to disable dynamic ftrace from all
    archs that use it. The archs can get dynamic ftrace reenabled when they
    are ported to the recordmcount.pl method. (Arch maintainers, please contact
    me if you want help. I can do it with some information about your arch).

    Since the hash was created to work with the daemon, that too was stripped
    out, making the remaining code smaller and cleaner. The kprobe hooks
    in ftrace may need to be reworked.

    -- Steve

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

  2. [PATCH 03/11] ftrace: return error on failed modified text.

    Return -1 on failed modified text.

    Signed-off-by: Steven Rostedt
    ---
    arch/x86/kernel/ftrace.c | 12 +++++++-----
    kernel/trace/ftrace.c | 29 ++++++++++-------------------
    2 files changed, 17 insertions(+), 24 deletions(-)

    Index: linux-compile.git/arch/x86/kernel/ftrace.c
    ================================================== =================
    --- linux-compile.git.orig/arch/x86/kernel/ftrace.c 2008-10-22 13:10:58.000000000 -0400
    +++ linux-compile.git/arch/x86/kernel/ftrace.c 2008-10-22 13:15:36.000000000 -0400
    @@ -71,14 +71,16 @@ ftrace_modify_code(unsigned long ip, uns
    * No real locking needed, this code is run through
    * kstop_machine, or before SMP starts.
    */
    - if (__copy_from_user_inatomic(replaced, (char __user *)ip, MCOUNT_INSN_SIZE))
    - return 1;
    + if (__copy_from_user_inatomic(replaced, (char __user *)ip,
    + MCOUNT_INSN_SIZE))
    + return -1;

    if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0)
    - return 2;
    + return -1;

    - WARN_ON_ONCE(__copy_to_user_inatomic((char __user *)ip, new_code,
    - MCOUNT_INSN_SIZE));
    + if (__copy_to_user_inatomic((char __user *)ip, new_code,
    + MCOUNT_INSN_SIZE))
    + return -1;

    sync_core();

    Index: linux-compile.git/kernel/trace/ftrace.c
    ================================================== =================
    --- linux-compile.git.orig/kernel/trace/ftrace.c 2008-10-22 13:10:58.000000000 -0400
    +++ linux-compile.git/kernel/trace/ftrace.c 2008-10-22 13:14:04.000000000 -0400
    @@ -591,31 +591,22 @@ ftrace_code_disable(struct dyn_ftrace *r
    {
    unsigned long ip;
    unsigned char *nop, *call;
    - int failed;
    + int ret;

    ip = rec->ip;

    nop = ftrace_nop_replace();
    call = ftrace_call_replace(ip, mcount_addr);

    - 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;
    - }
    + ret = ftrace_modify_code(ip, call, nop);
    + if (ret) {
    + WARN_ON_ONCE(1);
    + pr_info("ftrace failed to modify ");
    + print_ip_sym(ip);
    + print_ip_ins(" expected: ", call);
    + print_ip_ins(" replace: ", nop);
    + printk(KERN_CONT "\n");
    + break;

    rec->flags |= FTRACE_FL_FAILED;
    return 0;

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

  3. Re: [PATCH 03/11] ftrace: return error on failed modified text.


    On Wed, 22 Oct 2008, Steven Rostedt wrote:
    > - break;
    > - }
    > + ret = ftrace_modify_code(ip, call, nop);
    > + if (ret) {
    > + WARN_ON_ONCE(1);
    > + pr_info("ftrace failed to modify ");
    > + print_ip_sym(ip);
    > + print_ip_ins(" expected: ", call);
    > + print_ip_ins(" replace: ", nop);
    > + printk(KERN_CONT "\n");
    > + break;


    This break is an error. I refreshed it in patch 5/11 by mistake.

    -- Steve

    >
    > rec->flags |= FTRACE_FL_FAILED;
    > return 0;
    >
    > --
    >
    >

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

  4. [PATCH 01/11] ftrace: handle generic arch calls

    The recordmcount script requires that the actual arch is passed in.
    This works well when ARCH=i386 or ARCH=x86_64 but does not handle the
    case of ARCH=x86.

    This patch adds a parameter to the function to pass in the number of
    bits of the architecture. So that it can determine if x86 should be
    run for x86_64 or i386 archs.

    Signed-off-by: Steven Rostedt
    ---
    scripts/Makefile.build | 10 ++++++++--
    scripts/recordmcount.pl | 11 ++++++++++-
    2 files changed, 18 insertions(+), 3 deletions(-)

    Index: linux-compile.git/scripts/Makefile.build
    ================================================== =================
    --- linux-compile.git.orig/scripts/Makefile.build 2008-10-21 09:25:45.000000000 -0400
    +++ linux-compile.git/scripts/Makefile.build 2008-10-21 09:26:05.000000000 -0400
    @@ -198,10 +198,16 @@ cmd_modversions = \
    fi;
    endif

    +ifdef CONFIG_64BIT
    +arch_bits = 64
    +else
    +arch_bits = 32
    +endif
    +
    ifdef CONFIG_FTRACE_MCOUNT_RECORD
    cmd_record_mcount = perl $(srctree)/scripts/recordmcount.pl \
    - "$(ARCH)" "$(OBJDUMP)" "$(OBJCOPY)" "$(CC)" "$(LD)" "$(NM)" "$(RM)" \
    - "$(MV)" "$(@)";
    + "$(ARCH)" "$(arch_bits)" "$(OBJDUMP)" "$(OBJCOPY)" "$(CC)" "$(LD)" \
    + "$(NM)" "$(RM)" "$(MV)" "$(@)";
    endif

    define rule_cc_o_c
    Index: linux-compile.git/scripts/recordmcount.pl
    ================================================== =================
    --- linux-compile.git.orig/scripts/recordmcount.pl 2008-10-21 09:25:45.000000000 -0400
    +++ linux-compile.git/scripts/recordmcount.pl 2008-10-21 09:28:36.000000000 -0400
    @@ -106,7 +106,8 @@ if ($#ARGV < 6) {
    exit(1);
    }

    -my ($arch, $objdump, $objcopy, $cc, $ld, $nm, $rm, $mv, $inputfile) = @ARGV;
    +my ($arch, $bits, $objdump, $objcopy, $cc,
    + $ld, $nm, $rm, $mv, $inputfile) = @ARGV;

    $objdump = "objdump" if ((length $objdump) == 0);
    $objcopy = "objcopy" if ((length $objcopy) == 0);
    @@ -129,6 +130,14 @@ my $function_regex; # Find the name of a
    # (return offset and func name)
    my $mcount_regex; # Find the call site to mcount (return offset)

    +if ($arch eq "x86") {
    + if ($bits == 64) {
    + $arch = "x86_64";
    + } else {
    + $arch = "xi386";
    + }
    +}
    +
    if ($arch eq "x86_64") {
    $section_regex = "Disassembly of section";
    $function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";

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

  5. [PATCH 04/11] ftrace: comment arch ftrace code

    Add comments to explain what is happening in the x86 arch ftrace code.

    Signed-off-by: Steven Rostedt
    ---
    arch/x86/kernel/ftrace.c | 7 ++++++-
    1 file changed, 6 insertions(+), 1 deletion(-)

    Index: linux-compile.git/arch/x86/kernel/ftrace.c
    ================================================== =================
    --- linux-compile.git.orig/arch/x86/kernel/ftrace.c 2008-10-22 13:15:36.000000000 -0400
    +++ linux-compile.git/arch/x86/kernel/ftrace.c 2008-10-22 13:16:35.000000000 -0400
    @@ -66,18 +66,23 @@ ftrace_modify_code(unsigned long ip, uns
    /*
    * Note: Due to modules and __init, code can
    * disappear and change, we need to protect against faulting
    - * as well as code changing.
    + * as well as code changing. We do this by using the
    + * __copy_*_user functions.
    *
    * No real locking needed, this code is run through
    * kstop_machine, or before SMP starts.
    */
    +
    + /* read the text we want to modify */
    if (__copy_from_user_inatomic(replaced, (char __user *)ip,
    MCOUNT_INSN_SIZE))
    return -1;

    + /* Make sure it is what we expect it to be */
    if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0)
    return -1;

    + /* replace the text with the new text */
    if (__copy_to_user_inatomic((char __user *)ip, new_code,
    MCOUNT_INSN_SIZE))
    return -1;

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

  6. Re: [PATCH 01/11] ftrace: handle generic arch calls

    On Wed, 22 Oct 2008 14:43:14 -0400
    Steven Rostedt wrote:

    > +if ($arch eq "x86") {
    > + if ($bits == 64) {
    > + $arch = "x86_64";
    > + } else {
    > + $arch = "xi386";


    ?

    > + }


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

  7. [PATCH 05/11] ftrace: only have ftrace_kill atomic

    Only have a way to completely disable ftrace in atomic sections.

    Signed-off-by: Steven Rostedt
    ---
    include/linux/ftrace.h | 3 +--
    kernel/trace/ftrace.c | 43 ++-----------------------------------------
    kernel/trace/trace.c | 2 +-
    3 files changed, 4 insertions(+), 44 deletions(-)

    Index: linux-compile.git/include/linux/ftrace.h
    ================================================== =================
    --- linux-compile.git.orig/include/linux/ftrace.h 2008-10-22 13:17:26.000000000 -0400
    +++ linux-compile.git/include/linux/ftrace.h 2008-10-22 13:17:29.000000000 -0400
    @@ -40,7 +40,7 @@ extern void ftrace_stub(unsigned long a0
    # define register_ftrace_function(ops) do { } while (0)
    # define unregister_ftrace_function(ops) do { } while (0)
    # define clear_ftrace_function(ops) do { } while (0)
    -static inline void ftrace_kill_atomic(void) { }
    +static inline void ftrace_kill(void) { }
    #endif /* CONFIG_FTRACE */

    #ifdef CONFIG_DYNAMIC_FTRACE
    @@ -97,7 +97,6 @@ static inline void ftrace_release(void *

    /* totally disable ftrace - can not re-enable after this */
    void ftrace_kill(void);
    -void ftrace_kill_atomic(void);

    static inline void tracer_disable(void)
    {
    Index: linux-compile.git/kernel/trace/ftrace.c
    ================================================== =================
    --- linux-compile.git.orig/kernel/trace/ftrace.c 2008-10-22 13:17:26.000000000 -0400
    +++ linux-compile.git/kernel/trace/ftrace.c 2008-10-22 13:17:29.000000000 -0400
    @@ -606,7 +606,6 @@ ftrace_code_disable(struct dyn_ftrace *r
    print_ip_ins(" expected: ", call);
    print_ip_ins(" replace: ", nop);
    printk(KERN_CONT "\n");
    - break;

    rec->flags |= FTRACE_FL_FAILED;
    return 0;
    @@ -1526,22 +1525,6 @@ int ftrace_force_update(void)
    return ret;
    }

    -static void ftrace_force_shutdown(void)
    -{
    - struct task_struct *task;
    - int command = FTRACE_DISABLE_CALLS | FTRACE_UPDATE_TRACE_FUNC;
    -
    - mutex_lock(&ftraced_lock);
    - task = ftraced_task;
    - ftraced_task = NULL;
    - ftraced_suspend = -1;
    - ftrace_run_update_code(command);
    - mutex_unlock(&ftraced_lock);
    -
    - if (task)
    - kthread_stop(task);
    -}
    -
    static __init int ftrace_init_debugfs(void)
    {
    struct dentry *d_tracer;
    @@ -1734,17 +1717,16 @@ core_initcall(ftrace_dynamic_init);
    # define ftrace_shutdown() do { } while (0)
    # define ftrace_startup_sysctl() do { } while (0)
    # define ftrace_shutdown_sysctl() do { } while (0)
    -# define ftrace_force_shutdown() do { } while (0)
    #endif /* CONFIG_DYNAMIC_FTRACE */

    /**
    - * ftrace_kill_atomic - kill ftrace from critical sections
    + * ftrace_kill - kill ftrace
    *
    * This function should be used by panic code. It stops ftrace
    * but in a not so nice way. If you need to simply kill ftrace
    * from a non-atomic section, use ftrace_kill.
    */
    -void ftrace_kill_atomic(void)
    +void ftrace_kill(void)
    {
    ftrace_disabled = 1;
    ftrace_enabled = 0;
    @@ -1755,27 +1737,6 @@ void ftrace_kill_atomic(void)
    }

    /**
    - * ftrace_kill - totally shutdown ftrace
    - *
    - * This is a safety measure. If something was detected that seems
    - * wrong, calling this function will keep ftrace from doing
    - * any more modifications, and updates.
    - * used when something went wrong.
    - */
    -void ftrace_kill(void)
    -{
    - mutex_lock(&ftrace_sysctl_lock);
    - ftrace_disabled = 1;
    - ftrace_enabled = 0;
    -
    - clear_ftrace_function();
    - mutex_unlock(&ftrace_sysctl_lock);
    -
    - /* Try to totally disable ftrace */
    - ftrace_force_shutdown();
    -}
    -
    -/**
    * register_ftrace_function - register a function for profiling
    * @ops - ops structure that holds the function for profiling.
    *
    Index: linux-compile.git/kernel/trace/trace.c
    ================================================== =================
    --- linux-compile.git.orig/kernel/trace/trace.c 2008-10-22 13:17:26.000000000 -0400
    +++ linux-compile.git/kernel/trace/trace.c 2008-10-22 13:17:29.000000000 -0400
    @@ -3097,7 +3097,7 @@ void ftrace_dump(void)
    dump_ran = 1;

    /* No turning back! */
    - ftrace_kill_atomic();
    + ftrace_kill();

    for_each_tracing_cpu(cpu) {
    atomic_inc(&global_trace.data[cpu]->disabled);

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

  8. [PATCH 02/11] ftrace: dynamic ftrace process only text section

    The text section stays in memory without ever leaving. With the exception
    of modules, but modules know how to handle that case. With the dynamic
    ftrace tracer, we need to make sure that it does not try to modify code
    that no longer exists. The only safe section is .text.

    This patch changes the recordmcount script to only record the mcount calls
    in the .text sections.

    Signed-off-by: Steven Rostedt
    ---
    scripts/recordmcount.pl | 17 ++++++++++++++---
    1 file changed, 14 insertions(+), 3 deletions(-)

    Index: linux-compile.git/scripts/recordmcount.pl
    ================================================== =================
    --- linux-compile.git.orig/scripts/recordmcount.pl 2008-10-22 11:45:59.000000000 -0400
    +++ linux-compile.git/scripts/recordmcount.pl 2008-10-22 11:46:33.000000000 -0400
    @@ -109,6 +109,11 @@ if ($#ARGV < 6) {
    my ($arch, $bits, $objdump, $objcopy, $cc,
    $ld, $nm, $rm, $mv, $inputfile) = @ARGV;

    +# Acceptible sections to record.
    +my %text_sections = (
    + ".text" => 1,
    +);
    +
    $objdump = "objdump" if ((length $objdump) == 0);
    $objcopy = "objcopy" if ((length $objcopy) == 0);
    $cc = "gcc" if ((length $cc) == 0);
    @@ -139,7 +144,7 @@ if ($arch eq "x86") {
    }

    if ($arch eq "x86_64") {
    - $section_regex = "Disassembly of section";
    + $section_regex = "Disassembly of section\\s+(\\S+):";
    $function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";
    $mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\smcount([+-]0x[0-9a-zA-Z]+)?\$";
    $type = ".quad";
    @@ -151,7 +156,7 @@ if ($arch eq "x86_64") {
    $cc .= " -m64";

    } elsif ($arch eq "i386") {
    - $section_regex = "Disassembly of section";
    + $section_regex = "Disassembly of section\\s+(\\S+):";
    $function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";
    $mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\smcount\$";
    $type = ".long";
    @@ -298,7 +303,13 @@ my $text;
    while () {
    # is it a section?
    if (/$section_regex/) {
    - $read_function = 1;
    +
    + # Only record text sections that we know are safe
    + if (defined($text_sections{$1})) {
    + $read_function = 1;
    + } else {
    + $read_function = 0;
    + }
    # print out any recorded offsets
    update_funcs() if ($text_found);


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

  9. [PATCH 07/11] ftrace: do not trace init sections

    The recordmcount script is now robust enough not to process any sections
    but the .text section. But the gcc compiler still adds a call to mcount.

    Note: The function mcount looks like:

    ENTRY(mcount)
    ret
    END(mcount)

    Which means the overhead is just a return.

    This patch adds notrace to the init sections to not even bother calling
    mcount (which simply returns).


    Signed-off-by: Steven Rostedt
    ---
    include/linux/init.h | 10 +++++-----
    1 file changed, 5 insertions(+), 5 deletions(-)

    Index: linux-compile.git/include/linux/init.h
    ================================================== =================
    --- linux-compile.git.orig/include/linux/init.h 2008-10-22 11:49:44.000000000 -0400
    +++ linux-compile.git/include/linux/init.h 2008-10-22 12:27:19.000000000 -0400
    @@ -75,15 +75,15 @@


    #ifdef MODULE
    -#define __exitused
    +#define __exitused notrace
    #else
    -#define __exitused __used
    +#define __exitused __used notrace
    #endif

    #define __exit __section(.exit.text) __exitused __cold

    /* Used for HOTPLUG */
    -#define __devinit __section(.devinit.text) __cold
    +#define __devinit __section(.devinit.text) __cold notrace
    #define __devinitdata __section(.devinit.data)
    #define __devinitconst __section(.devinit.rodata)
    #define __devexit __section(.devexit.text) __exitused __cold
    @@ -91,7 +91,7 @@
    #define __devexitconst __section(.devexit.rodata)

    /* Used for HOTPLUG_CPU */
    -#define __cpuinit __section(.cpuinit.text) __cold
    +#define __cpuinit __section(.cpuinit.text) __cold notrace
    #define __cpuinitdata __section(.cpuinit.data)
    #define __cpuinitconst __section(.cpuinit.rodata)
    #define __cpuexit __section(.cpuexit.text) __exitused __cold
    @@ -99,7 +99,7 @@
    #define __cpuexitconst __section(.cpuexit.rodata)

    /* Used for MEMORY_HOTPLUG */
    -#define __meminit __section(.meminit.text) __cold
    +#define __meminit __section(.meminit.text) __cold notrace
    #define __meminitdata __section(.meminit.data)
    #define __meminitconst __section(.meminit.rodata)
    #define __memexit __section(.memexit.text) __exitused __cold

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

  10. [PATCH 10/11] ftrace: remove mcount set

    The arch dependent function ftrace_mcount_set was only used by the daemon
    start up code. This patch removes it.

    Signed-off-by: Steven Rostedt
    ---
    arch/arm/kernel/ftrace.c | 13 -------------
    arch/powerpc/kernel/ftrace.c | 17 -----------------
    arch/x86/kernel/ftrace.c | 7 -------
    include/linux/ftrace.h | 1 -
    kernel/trace/ftrace.c | 9 ---------
    5 files changed, 47 deletions(-)

    Index: linux-compile.git/arch/arm/kernel/ftrace.c
    ================================================== =================
    --- linux-compile.git.orig/arch/arm/kernel/ftrace.c 2008-10-22 13:10:52.000000000 -0400
    +++ linux-compile.git/arch/arm/kernel/ftrace.c 2008-10-22 13:23:54.000000000 -0400
    @@ -95,19 +95,6 @@ int ftrace_update_ftrace_func(ftrace_fun
    return ret;
    }

    -int ftrace_mcount_set(unsigned long *data)
    -{
    - unsigned long pc, old;
    - unsigned long *addr = data;
    - unsigned char *new;
    -
    - pc = (unsigned long)&mcount_call;
    - memcpy(&old, &mcount_call, MCOUNT_INSN_SIZE);
    - new = ftrace_call_replace(pc, *addr);
    - *addr = ftrace_modify_code(pc, (unsigned char *)&old, new);
    - return 0;
    -}
    -
    /* run from kstop_machine */
    int __init ftrace_dyn_arch_init(void *data)
    {
    Index: linux-compile.git/arch/powerpc/kernel/ftrace.c
    ================================================== =================
    --- linux-compile.git.orig/arch/powerpc/kernel/ftrace.c 2008-10-22 13:10:52.000000000 -0400
    +++ linux-compile.git/arch/powerpc/kernel/ftrace.c 2008-10-22 13:23:54.000000000 -0400
    @@ -126,23 +126,6 @@ notrace int ftrace_update_ftrace_func(ft
    return ret;
    }

    -notrace int ftrace_mcount_set(unsigned long *data)
    -{
    - unsigned long ip = (long)(&mcount_call);
    - unsigned long *addr = data;
    - unsigned char old[MCOUNT_INSN_SIZE], *new;
    -
    - /*
    - * Replace the mcount stub with a pointer to the
    - * ip recorder function.
    - */
    - memcpy(old, &mcount_call, MCOUNT_INSN_SIZE);
    - new = ftrace_call_replace(ip, *addr);
    - *addr = ftrace_modify_code(ip, old, new);
    -
    - return 0;
    -}
    -
    int __init ftrace_dyn_arch_init(void *data)
    {
    /* This is running in kstop_machine */
    Index: linux-compile.git/arch/x86/kernel/ftrace.c
    ================================================== =================
    --- linux-compile.git.orig/arch/x86/kernel/ftrace.c 2008-10-22 13:16:35.000000000 -0400
    +++ linux-compile.git/arch/x86/kernel/ftrace.c 2008-10-22 13:23:54.000000000 -0400
    @@ -105,13 +105,6 @@ notrace int ftrace_update_ftrace_func(ft
    return ret;
    }

    -notrace int ftrace_mcount_set(unsigned long *data)
    -{
    - /* mcount is initialized as a nop */
    - *data = 0;
    - return 0;
    -}
    -
    int __init ftrace_dyn_arch_init(void *data)
    {
    extern const unsigned char ftrace_test_p6nop[];
    Index: linux-compile.git/include/linux/ftrace.h
    ================================================== =================
    --- linux-compile.git.orig/include/linux/ftrace.h 2008-10-22 13:17:29.000000000 -0400
    +++ linux-compile.git/include/linux/ftrace.h 2008-10-22 13:23:54.000000000 -0400
    @@ -71,7 +71,6 @@ extern int ftrace_ip_converted(unsigned
    extern unsigned char *ftrace_nop_replace(void);
    extern unsigned char *ftrace_call_replace(unsigned long ip, unsigned long addr);
    extern int ftrace_dyn_arch_init(void *data);
    -extern int ftrace_mcount_set(unsigned long *data);
    extern int ftrace_modify_code(unsigned long ip, unsigned char *old_code,
    unsigned char *new_code);
    extern int ftrace_update_ftrace_func(ftrace_func_t func);
    Index: linux-compile.git/kernel/trace/ftrace.c
    ================================================== =================
    --- linux-compile.git.orig/kernel/trace/ftrace.c 2008-10-22 13:21:46.000000000 -0400
    +++ linux-compile.git/kernel/trace/ftrace.c 2008-10-22 13:23:54.000000000 -0400
    @@ -606,7 +606,6 @@ static int ftrace_update_code(void *igno

    static int __ftrace_modify_code(void *data)
    {
    - unsigned long addr;
    int *command = data;

    if (*command & FTRACE_ENABLE_CALLS) {
    @@ -625,14 +624,6 @@ static int __ftrace_modify_code(void *da
    if (*command & FTRACE_UPDATE_TRACE_FUNC)
    ftrace_update_ftrace_func(ftrace_trace_function);

    - if (*command & FTRACE_ENABLE_MCOUNT) {
    - addr = (unsigned long)ftrace_record_ip;
    - ftrace_mcount_set(&addr);
    - } else if (*command & FTRACE_DISABLE_MCOUNT) {
    - addr = (unsigned long)ftrace_stub;
    - ftrace_mcount_set(&addr);
    - }
    -
    return 0;
    }


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

  11. Re: [PATCH 03/11] ftrace: return error on failed modified text.


    On Wed, 22 Oct 2008, Andrew Morton wrote:

    > On Wed, 22 Oct 2008 14:43:16 -0400
    > Steven Rostedt wrote:
    >
    > > Return -1 on failed modified text.
    > >

    >
    > changelog fails to explain the reason for the change.
    >
    > > + if (__copy_to_user_inatomic((char __user *)ip, new_code,
    > > + MCOUNT_INSN_SIZE))
    > > + return -1;

    >
    > why not -EFAULT?


    Indeed, why not. I'll update it in v2.

    -- Steve

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

  12. Re: [PATCH 01/11] ftrace: handle generic arch calls



    On Wed, 22 Oct 2008, Andrew Morton wrote:

    > On Wed, 22 Oct 2008 14:43:14 -0400
    > Steven Rostedt wrote:
    >
    > > +if ($arch eq "x86") {
    > > + if ($bits == 64) {
    > > + $arch = "x86_64";
    > > + } else {
    > > + $arch = "xi386";

    >
    > ?
    >
    > > + }

    >
    > (how well was this tested?)


    I've tested this with !CONFIG_FTRACE, CONFIG_FTRACE &
    !CONFIG_DYNAMIC_FTRACE, and CONFIG_FTRACE & CONFIG_DYNAMIC_FTRACE. I did
    only test this on x86_64, so that explains why the i386 was broken :-(

    I'll boot up my i386 box, and start running the tests there too.

    -- Steve



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

  13. Re: [PATCH 04/11] ftrace: comment arch ftrace code


    On Wed, 22 Oct 2008, Andrew Morton wrote:
    >
    > I dunno.
    >
    > __copy_to_user_inatomic() is for "copying memory from userspace while
    > in an atomic context".
    >
    > But what you're doing here is "modifying some kernel text which might
    > generate a fault". It seems somewhat interface-abusive to use a
    > userspace access function for that just because it happens right now to
    > do the right thing.
    >
    > I'd suggest that for clarity and for future-safety, you create some new
    > interface function which does that thing. Right now it can be a simple
    > wrapper around __copy_from_user_inatomic().
    >
    >
    >
    > oh, someone added one - probe_kernel_write(). Why not use that?


    I didn't know about that code. That is what I want.

    -- Steve

    >
    >
    >
    >
    > Also, I hope that the above code is called from within a
    > pagefault_disable()d region? Or are relying upon some magical
    > side-effect of something which happens to do the same thing as
    > pagefault_disable()? IOW: by what means does the above code ensure
    > that do_page_fault() will see in_atomic()==true?


    This code is called from kstop_machine, or simply has interrupts disabled.

    -- Steve

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

  14. Re: [PATCH 04/11] ftrace: comment arch ftrace code

    On Wed, 22 Oct 2008 14:43:17 -0400
    Steven Rostedt wrote:

    > Add comments to explain what is happening in the x86 arch ftrace code.
    >
    > Signed-off-by: Steven Rostedt
    > ---
    > arch/x86/kernel/ftrace.c | 7 ++++++-
    > 1 file changed, 6 insertions(+), 1 deletion(-)
    >
    > Index: linux-compile.git/arch/x86/kernel/ftrace.c
    > ================================================== =================
    > --- linux-compile.git.orig/arch/x86/kernel/ftrace.c 2008-10-22 13:15:36.000000000 -0400
    > +++ linux-compile.git/arch/x86/kernel/ftrace.c 2008-10-22 13:16:35.000000000 -0400
    > @@ -66,18 +66,23 @@ ftrace_modify_code(unsigned long ip, uns
    > /*
    > * Note: Due to modules and __init, code can
    > * disappear and change, we need to protect against faulting
    > - * as well as code changing.
    > + * as well as code changing. We do this by using the
    > + * __copy_*_user functions.
    > *
    > * No real locking needed, this code is run through
    > * kstop_machine, or before SMP starts.
    > */
    > +
    > + /* read the text we want to modify */
    > if (__copy_from_user_inatomic(replaced, (char __user *)ip,
    > MCOUNT_INSN_SIZE))
    > return -1;
    >
    > + /* Make sure it is what we expect it to be */
    > if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0)
    > return -1;
    >
    > + /* replace the text with the new text */
    > if (__copy_to_user_inatomic((char __user *)ip, new_code,
    > MCOUNT_INSN_SIZE))
    > return -1;
    >


    I dunno.

    __copy_to_user_inatomic() is for "copying memory from userspace while
    in an atomic context".

    But what you're doing here is "modifying some kernel text which might
    generate a fault". It seems somewhat interface-abusive to use a
    userspace access function for that just because it happens right now to
    do the right thing.

    I'd suggest that for clarity and for future-safety, you create some new
    interface function which does that thing. Right now it can be a simple
    wrapper around __copy_from_user_inatomic().



    oh, someone added one - probe_kernel_write(). Why not use that?




    Also, I hope that the above code is called from within a
    pagefault_disable()d region? Or are relying upon some magical
    side-effect of something which happens to do the same thing as
    pagefault_disable()? IOW: by what means does the above code ensure
    that do_page_fault() will see in_atomic()==true?



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

  15. Re: [PATCH 05/11] ftrace: only have ftrace_kill atomic


    On Wed, 22 Oct 2008, Andrew Morton wrote:

    > On Wed, 22 Oct 2008 14:43:18 -0400
    > Steven Rostedt wrote:
    >
    > > Only have a way to completely disable ftrace in atomic sections.
    > >

    >
    > I have NFI what that means.
    >
    > More importantly there is no explanation for *why* this change is being
    > made.


    Sorry, I'll explain it better in the next release. This was just something
    that Ingo wanted cleaned up. He prefers the name "ftrace_turn_off". I
    really don't care what the name is, but the "atomic" part was something
    that had to go.

    What's your preference fo a name of a function that should completely
    disable ftrace on anomaly detection? Unfortunately, ftrace_stop,
    ftrace_disable, and ftrace_shutdown are already in use.

    -- Steve

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

  16. Re: [PATCH 06/11] ftrace: add ftrace warn on to disable ftrace

    On Wed, 22 Oct 2008 14:43:19 -0400
    Steven Rostedt wrote:

    > Add ftrace warn on to disable ftrace as well as report a warning.
    >
    > Signed-off-by: Steven Rostedt
    > ---
    > kernel/trace/ftrace.c | 29 ++++++++++++++++++++++-------
    > 1 file changed, 22 insertions(+), 7 deletions(-)
    >
    > Index: linux-compile.git/kernel/trace/ftrace.c
    > ================================================== =================
    > --- linux-compile.git.orig/kernel/trace/ftrace.c 2008-10-22 13:17:29.000000000 -0400
    > +++ linux-compile.git/kernel/trace/ftrace.c 2008-10-22 13:20:11.000000000 -0400
    > @@ -32,6 +32,24 @@
    >
    > #include "trace.h"
    >
    > +#define FTRACE_WARN_ON(cond) \
    > + do { \
    > + if (unlikely(cond)) { \
    > + WARN_ON(1); \
    > + ftrace_kill(); \
    > + } \
    > + } while (0)


    This could be coded as

    if (WARN_ON(cond))
    ftrace_kill();

    > +#define FTRACE_WARN_ON_ONCE(cond) \
    > + do { \
    > + static int once; \
    > + if (unlikely(cond) && !once) { \
    > + once++; \
    > + WARN_ON(1); \
    > + ftrace_kill(); \
    > + } \
    > + } while (0)



    if (WARN_ON_ONCE(cond))
    ftrace_kill();


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

  17. Re: [PATCH 05/11] ftrace: only have ftrace_kill atomic

    On Wed, 22 Oct 2008 14:43:18 -0400
    Steven Rostedt wrote:

    > Only have a way to completely disable ftrace in atomic sections.
    >


    I have NFI what that means.

    More importantly there is no explanation for *why* this change is being
    made.


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

  18. Re: [PATCH 06/11] ftrace: add ftrace warn on to disable ftrace


    On Wed, 22 Oct 2008, Andrew Morton wrote:
    > > Index: linux-compile.git/kernel/trace/ftrace.c
    > > ================================================== =================
    > > --- linux-compile.git.orig/kernel/trace/ftrace.c 2008-10-22 13:17:29.000000000 -0400
    > > +++ linux-compile.git/kernel/trace/ftrace.c 2008-10-22 13:20:11.000000000 -0400
    > > @@ -32,6 +32,24 @@
    > >
    > > #include "trace.h"
    > >
    > > +#define FTRACE_WARN_ON(cond) \
    > > + do { \
    > > + if (unlikely(cond)) { \
    > > + WARN_ON(1); \
    > > + ftrace_kill(); \
    > > + } \
    > > + } while (0)

    >
    > This could be coded as
    >
    > if (WARN_ON(cond))
    > ftrace_kill();


    Ah, I didn't realize that WARN_ON returned the value of the condition.
    Will update in v2.

    -- Steve

    >
    > > +#define FTRACE_WARN_ON_ONCE(cond) \
    > > + do { \
    > > + static int once; \
    > > + if (unlikely(cond) && !once) { \
    > > + once++; \
    > > + WARN_ON(1); \
    > > + ftrace_kill(); \
    > > + } \
    > > + } while (0)

    >
    >
    > if (WARN_ON_ONCE(cond))
    > ftrace_kill();
    >
    >
    >
    >

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

  19. Re: [PATCH 04/11] ftrace: comment arch ftrace code

    On Wed, 22 Oct 2008 15:16:33 -0400 (EDT)
    Steven Rostedt wrote:

    > > Also, I hope that the above code is called from within a
    > > pagefault_disable()d region? Or are relying upon some magical
    > > side-effect of something which happens to do the same thing as
    > > pagefault_disable()? IOW: by what means does the above code ensure
    > > that do_page_fault() will see in_atomic()==true?

    >
    > This code is called from kstop_machine, or simply has interrupts disabled.


    in_atomic() doesn't test irqs_disabled()!

    Still, probe_kernel_write() correctly handles the
    secret-argument-passing to do_page_fault().

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

  20. Re: [PATCH 05/11] ftrace: only have ftrace_kill atomic

    On Wed, 22 Oct 2008 15:18:39 -0400 (EDT)
    Steven Rostedt wrote:

    > What's your preference fo a name of a function that should completely
    > disable ftrace on anomaly detection? Unfortunately, ftrace_stop,
    > ftrace_disable, and ftrace_shutdown are already in use.


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

+ Reply to Thread
Page 1 of 2 1 2 LastLast