lkml.org 
[lkml]   [2011]   [May]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [block IO crash] Re: 2.6.39-rc5-git2 boot crashs
    On Wed, 4 May 2011, Tejun Heo wrote:
    > Hello,
    >
    > On Wed, May 04, 2011 at 03:00:37PM +0200, Thomas Gleixner wrote:
    > > On Wed, 4 May 2011, Tejun Heo wrote:
    > > > > > And that code runs with preemption enabled. So when the task gets
    > > > > > preempted _BEFORE_ it has actuallty written back the data, then the
    > > > > > race window is wide open.
    > > >
    > > > Hmmm... if it's a race caused by preemtion enabled where it shouldn't
    > > > be, it's most likely the wrong type of this_cpu_cmpxchg_double() being
    > > > used in SLUB? ie. __this_cpu_cmpxchg_double() where it should have
    > > > been this_cpu_cmpxchg_double()? Christoph?
    > >
    > > No, the problem is that ELAN prevents the cmpxchg8b, but keeps
    > > CONFIG_CMPXCHG_LOCAL=y which then results in the unprotected code for
    > > the following reason:
    > ...
    > > So the question is whether CMPXCHG_LOCAL for x86 wants to depend on
    > > X86_CMPXCHG64.
    > >
    > > The other solution is to use irqsafe_cpu_cmpxchg_double() instead of
    > > this_cpu_cmpxchg_double() in slub.c.
    >
    > I think this is the root cause. CMPXCHG_LOCAL is an optimization
    > flag, indicating that the processor provides fast local cmpxchg, it
    > doesn't say anything about local synchronization properties and if the
    > code required irq exclusion, it should have used
    > irqsafe_cpu_cmpxchg_double() whether the processor supports it
    > natively or not, so there's the bug. Pekka, can you please change the
    > offending cmpxchg_double() to irqsafe variant?

    Patch below. Ingo, can you test that please ?

    > As for CMPXCHG_LOCAL being set spuriously, maybe introduce
    > CMPXCHG_DOUBLE_LOCAL? I don't know. It's pretty nasty to implement

    Oh no, please not another CONFIG_WTF and more #ifdeffery.

    > different high-level code paths depending on CPU features. We can't
    > even determine whether the feature will be actually available at
    > compile time. But, then again, it might incur noticeable slowdown for

    Right, and the x86 implementation should not do

    #ifdef CONFIG_X86_CMPXCHG64
    #define percpu_cmpxchg8b_double(pcp1, o1, o2, n1, n2)
    #endif

    And let the code fallback to the generic variant. It should have an
    #else path using the the cmpxchg64_local() implementation which has
    alternatives for runtime selection of cmpxchg8b or the cli protected
    emulation.

    > cases where the generic implementation is used. Has anyone measured
    > the difference against before the whole this_cpu conversion?

    Yes, that really wants to be done. The whole CMPXCHG_LOCAL ifdeffery
    should have been avoided in the first place. this_cpu_cmpxchg can
    really be implemented with preempt_enable/disable and the irqsafe
    variant in any case.

    Thanks,

    tglx

    --------->
    Subject: slub-hmm.patch
    From: Thomas Gleixner <tglx@linutronix.de>
    Date: Wed, 04 May 2011 15:38:19 +0200

    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
    ---
    include/linux/percpu.h | 2 +-
    mm/slub.c | 4 ++--
    2 files changed, 3 insertions(+), 3 deletions(-)

    Index: linux-2.6/include/linux/percpu.h
    ===================================================================
    --- linux-2.6.orig/include/linux/percpu.h
    +++ linux-2.6/include/linux/percpu.h
    @@ -948,7 +948,7 @@ do { \
    irqsafe_generic_cpu_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, nval2)
    # endif
    # define irqsafe_cpu_cmpxchg_double(pcp1, pcp2, oval1, oval2, nval1, nval2) \
    - __pcpu_double_call_return_int(irqsafe_cpu_cmpxchg_double_, (pcp1), (pcp2), (oval1), (oval2), (nval1), (nval2))
    + __pcpu_double_call_return_bool(irqsafe_cpu_cmpxchg_double_, (pcp1), (pcp2), (oval1), (oval2), (nval1), (nval2))
    #endif

    #endif /* __LINUX_PERCPU_H */
    Index: linux-2.6/mm/slub.c
    ===================================================================
    --- linux-2.6.orig/mm/slub.c
    +++ linux-2.6/mm/slub.c
    @@ -1940,7 +1940,7 @@ redo:
    * Since this is without lock semantics the protection is only against
    * code executing on this cpu *not* from access by other cpus.
    */
    - if (unlikely(!this_cpu_cmpxchg_double(
    + if (unlikely(!irqsafe_cpu_cmpxchg_double(
    s->cpu_slab->freelist, s->cpu_slab->tid,
    object, tid,
    get_freepointer(s, object), next_tid(tid)))) {
    @@ -2145,7 +2145,7 @@ redo:
    set_freepointer(s, object, c->freelist);

    #ifdef CONFIG_CMPXCHG_LOCAL
    - if (unlikely(!this_cpu_cmpxchg_double(
    + if (unlikely(!irqsafe_cpu_cmpxchg_double(
    s->cpu_slab->freelist, s->cpu_slab->tid,
    c->freelist, tid,
    object, next_tid(tid)))) {

    \
     
     \ /
      Last update: 2011-05-04 16:13    [W:0.028 / U:31.328 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site