lkml.org 
[lkml]   [2012]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
    On 06/08/2012 09:41 AM, Steven Rostedt wrote:
    >>
    >> The cost of this on real hardware better be zero (which I cannot
    >> immediately judge.)
    >
    > Is dec_and_test cheaper than cmpxchg? I would think so.
    >

    Should be more or less the same (but see below w.r.t. _local).

    >>
    >> Why? Because cmpxchg has been in every CPU since the i486, the i386 is
    >> royally crippled on Linux anyway (due to minor architectural defects,
    >> the main one being the write protect issue) and NMI is almost never used
    >> on i386 as anything other than a fatal error indication.
    >>
    >> Most "real" NMI users generate the NMI from the local APIC, but the i386
    >> has no local APIC, and unlike the i486 cannot even have an external
    >> local APIC to the best of my knowledge.
    >
    > Yeah, this is why I didn't rush to do this change. But it does seem to
    > make the code simpler and it may actually speed things up. It replaces a
    > cmpxchg with a local_dec_and_test, which, I believe, doesn't even lock
    > the cachelines.
    >

    Yeah, the cmpxchg here rather than cmpxchg_local seems like it just was
    a plain bug, no?

    > So lets look at the patch in detail, shall we?
    >
    >
    >> enum nmi_states {
    >> - NMI_NOT_RUNNING,
    >> + NMI_NOT_RUNNING = 0,
    >
    > This change was done more for documenting that the first element must be
    > zero. Even though C guarantees this. I wanted to point out that we
    > expect it to be zero and that it being zero really does matter. No
    > functionality change whats-so-ever.
    >

    Yes, that makes sense.

    >> NMI_EXECUTING,
    >> NMI_LATCHED,
    >> };
    >> -static DEFINE_PER_CPU(enum nmi_states, nmi_state);
    >> +static DEFINE_PER_CPU(local_t, nmi_state);
    >
    > local_t is is just an atomic_long_t, which on i386 is nothing different
    > than what an enum would be.
    >
    >>
    >> #define nmi_nesting_preprocess(regs) \
    >> do { \
    >> - if (__get_cpu_var(nmi_state) != NMI_NOT_RUNNING) { \
    >> - __get_cpu_var(nmi_state) = NMI_LATCHED; \
    >> + local_t *__state = &__get_cpu_var(nmi_state); \
    >> + if (local_read(__state) != NMI_NOT_RUNNING) { \
    >> + local_set(__state, NMI_LATCHED); \
    >
    > The above change is probably a little bit of a speed up because we
    > remove the double '__get_cpu_var()' and replace it with a pointer that
    > is reused. I haven't looked at the assembly for this, but it is either
    > the same or better with the patch.
    >
    > Sure, we could improve this by using this_cpu_var() which may make it
    > better than the patch. But the patch is currently the same or better
    > than what is there now.

    But yes, if you're going to modify this use this_cpu_read() and
    this_cpu_write() and avoid the pointer completely.

    >> return; \
    >> } \
    >> - nmi_restart: \
    >> - __get_cpu_var(nmi_state) = NMI_EXECUTING; \
    >> - } while (0)
    >> + local_set(__state, NMI_EXECUTING); \
    >> + } while (0); \
    >> + nmi_restart:
    >
    > Here it's better or the same than what is there now as we again remove
    > the reference to getting the pointer. In case gcc doesn't optimize it
    > nicely. But again we could have switched to this_cpu_write() which could
    > be better.
    >
    > The movement of nmi_restart does help too. I'll explain that below.
    >
    >>
    >> #define nmi_nesting_postprocess() \
    >> do { \
    >> - if (cmpxchg(&__get_cpu_var(nmi_state), \
    >> - NMI_EXECUTING, NMI_NOT_RUNNING) != NMI_EXECUTING) \
    >> + if (!local_dec_and_test(&__get_cpu_var(nmi_state))) \
    >
    > Now this is where I think the patch helps. I'm almost certain that
    > local_dec_and_test is faster than a cmpxchg by many cycles. Especially
    > on i386.
    >

    On i386 it's infinite, but again, I don't think the code will ever be
    exercised on i386. I'm much more concerned about performance on current
    processors.

    But yes, local_dec_and_test should at least not be more expensive. Even
    better, use this_cpu_dec_return().

    -hpa

    --
    H. Peter Anvin, Intel Open Source Technology Center
    I work for Intel. I don't speak on their behalf.



    \
     
     \ /
      Last update: 2012-06-08 20:01    [W:0.034 / U:32.536 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site