lkml.org 
[lkml]   [2015]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC][PATCH] mips: Fix arch_spin_unlock()
    On 11/12/2015 04:31 AM, Peter Zijlstra wrote:
    > Hi
    >
    > I think the MIPS arch_spin_unlock() is borken.
    >
    > spin_unlock() must have RELEASE semantics, these require that no LOADs
    > nor STOREs leak out from the critical section.
    >
    > From what I know MIPS has a relaxed memory model which allows reads to
    > pass stores, and as implemented arch_spin_unlock() only issues a wmb
    > which doesn't order prior reads vs later stores.
    >
    > Therefore upgrade the wmb() to smp_mb().
    >
    > (Also, why the unconditional wmb, as opposed to smp_wmb() ?)

    asm/spinlock.h is only used for !CONFIG_SMP. So, smp_wmb() would imply
    that special handling for non-SMP is needed, when this is already only
    used for the SMP build case.

    >
    > Maybe-Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
    > ---
    > diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
    > index 40196bebe849..b2ca13f06152 100644
    > --- a/arch/mips/include/asm/spinlock.h
    > +++ b/arch/mips/include/asm/spinlock.h
    > @@ -140,7 +140,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
    > static inline void arch_spin_unlock(arch_spinlock_t *lock)
    > {
    > unsigned int serving_now = lock->h.serving_now + 1;
    > - wmb();
    > + smp_mb();

    That is too heavy.

    It implies a full MIPS "SYNC" operation which stalls execution until all
    previous writes are committed and globally visible.

    We really want just release semantics, and there is no standard named
    primitive that gives us that.

    For CONFIG_CPU_CAVIUM_OCTEON the proper thing would be:

    smp_wmb();
    smp_rmb();

    Which expands to exactly the same thing as wmb() because smp_rmb()
    expands to nothing.

    For CPUs that have out-of-order loads, smp_rmb() should expand to
    something lighter weight than "SYNC"

    Certainly we can load up the code with "SYNC" all over the place, but it
    will kill performance on SMP systems. So, my vote would be to make it
    as light weight as possible, but no lighter. That will mean inventing
    the proper barrier primitives.


    You yourself seem to have added smp_store_release(), so we could even do:

    smp_store_release(&lock->h.serving_now, lock->h.serving_now + 1);

    That would leave us to cook up a proper definition of smp_store_release().

    David Daney

    > lock->h.serving_now = (u16)serving_now;
    > nudge_writes();
    > }
    > --
    > 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/
    >



    \
     
     \ /
      Last update: 2015-11-12 19:01    [W:3.915 / U:0.208 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site