[lkml]   [2009]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH 1/2] ring-buffer: add NMI protection for spinlocks

    [ added Arjan ]

    On Thu, 5 Feb 2009, Andrew Morton wrote:
    > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
    > > index 4d33224..4c68358 100644
    > > --- a/arch/x86/kernel/ftrace.c
    > > +++ b/arch/x86/kernel/ftrace.c
    > > @@ -113,7 +113,7 @@ static void ftrace_mod_code(void)
    > > }
    > >
    > > -void ftrace_nmi_enter(void)
    > > +void arch_ftrace_nmi_enter(void)
    > > {
    > > atomic_inc(&in_nmi);
    > This in_nmi thing looks a bit awkward. It's not very clear from the
    > code what it does or why it exists, but it looks like I'm going to have
    > to get used to that...
    > Surely it is buggy? It's incremented by _any_ cpu, so code which reads
    > it (eg, prepare_ftrace_return()) will bale out if some other CPU
    > happened to be in the middle of taking an NMI.

    And that is exactly what we want it to do ;-)

    This is code modification. If you modify code that another cpu is
    executing with, the result is undefined. In my tests to cause this type of
    error, I usually get a general protection fault.

    To modify code safely, you must act like a uniprocessor. That is, the code
    you modify must not be executing on another CPU. We use stop_machine to
    help us out here, but under NMI stress tests, we found that we can crash,
    because the NMI will execute code being modified. Stop_machine does not
    disable NMIs on other CPUS.

    To solve this, we sync up the modification with the NMI with a couple of
    variables. That's what all those memory barriers are for.

    When we go to modify a line of code, we set a variable that we are doing
    so, wait for all NMIs to finish, then modify the code, wait for NMIs
    again, and repeat on the next line to modify.

    If the NMI goes off on any CPU, it increments the in_nmi counter. If the
    mod code flag is set, the NMI modifies the code.

    The trick here is that the crash only occurs if the code is modified as it
    is being executed on. But it will not crash if the code is written to with
    the same data as it was. In other words, it is fine to write the code and
    execute it, if what you are writing is what is already there. No
    modification being done.

    All NMIs that go off will modify the same code. Even if the code being
    modified is in a NMI code path, only the first NMI that goes off will
    actually modify it, the rest will just be writting the same data to the
    location. Even the main code will be writing the same data. No races.

    > Would it not be better to make in_nmi a per-cpu thing, make it a piece
    > of first-class kernel infrastructure, add a nice interface to it, hoist
    > it up into generic code, call that code from arch NMI handlers, call
    > that interface from ftrace, etc? Or perhaps it could be implemented in
    > task_struct, alongside in_irq(), in_softirq(), etc. There are three bits
    > left in ->preempt_count.

    I agree, we should have a per cpu nmi code. I can write that up and use
    it for the ring buffer. But the call back for the ftrace code that I
    explained here must still be made. We need a "global" nmi counter for it
    to work.

    -- Steve

    > This approach would remove the duplication which this patch is
    > attempting to add.
    > Plus there's already quite a bit of duplication in
    > arch/x86/kernel/ftrace.c between the
    > in_nmi, duplicated inc/dec thereof?

     \ /
      Last update: 2009-02-06 03:27    [W:0.040 / U:1.192 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site