lkml.org 
[lkml]   [2011]   [Jun]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [slubllv7 04/17] x86: Add support for cmpxchg_double
    On Wed, 15 Jun 2011, Tejun Heo wrote:

    > Hello, Christoph, Pekka. Sorry about the delay.
    >
    > On Wed, Jun 01, 2011 at 12:25:47PM -0500, Christoph Lameter wrote:
    > > Index: linux-2.6/arch/x86/include/asm/cmpxchg_64.h
    > > ===================================================================
    > > --- linux-2.6.orig/arch/x86/include/asm/cmpxchg_64.h 2011-06-01 11:01:05.002406114 -0500
    > > +++ linux-2.6/arch/x86/include/asm/cmpxchg_64.h 2011-06-01 11:01:48.222405834 -0500
    > > +#define cmpxchg_double(ptr, o1, o2, n1, n2) \
    > > +({ \
    > > + BUILD_BUG_ON(sizeof(*(ptr)) != 8); \
    > > + VM_BUG_ON((unsigned long)(ptr) % 16); \
    > > + cmpxchg16b((ptr), (o1), (o2), (n1), (n2)); \
    > > +})
    > > +
    > > +#define cmpxchg_double_local(ptr, o1, o2, n1, n2) \
    > > +({ \
    > > + BUILD_BUG_ON(sizeof(*(ptr)) != 8); \
    > > + VM_BUG_ON((unsigned long)(ptr) % 16); \
    > > + cmpxchg16b_local((ptr), (o1), (o2), (n1), (n2)); \
    > > +})
    >
    > Do we really need cmpxchg16b*() macros separately? Why not just
    > collapse them into cmpxchg_double*()? Also, it would be better if we
    > have the same level of VM_BUG_ON() checks as in percpu cmpxchg_double
    > ops. Maybe we should put them in a separate macro?

    The method here is to put all the high level checks in cmpxchg_double()
    and then do the low level asm stuff in cmpxchg16b macros. I think that is
    a good separation.

    > > +#define system_has_cmpxchg_double() cpu_has_cx16
    >
    > Where's the fallback %false definition for the above feature macro for
    > archs which don't support cmpxchg_double? Also, is system_has_*()
    > conventional? Isn't arch_has_*() more conventional for this purpose?

    There is a convention for querying processor flags from core code?

    The system_has_cmpxchg_double() is only used if the arch defines
    CONFIG_CMPXCHG_DOUBLE

    > > +#define cmpxchg8b_local(ptr, o1, o2, n1, n2) \
    > > +({ \
    > > + char __ret; \
    > > + __typeof__(o2) __dummy; \
    > > + __typeof__(*(ptr)) __old1 = (o1); \
    > > + __typeof__(o2) __old2 = (o2); \
    > > + __typeof__(*(ptr)) __new1 = (n1); \
    > > + __typeof__(o2) __new2 = (n2); \
    > > + asm volatile("cmpxchg8b %2; setz %1" \
    > > + : "=d"(__dummy), "=a"(__ret), "+m" (*ptr)\
    > > + : "a" (__old), "d"(__old2), \
    > > + "b" (__new1), "c" (__new2), \
    > > + : "memory"); \
    > > + __ret; })
    >
    > Wouldn't it be better to use cmpxchg64() for cmpxchg_double()?

    This way it is done in the same way on 32 bit than on 64 bit. The use of
    cmpxchg64 also means that some of the parameters would have to be combined
    to form 64 bit ints from the 32 bit ones before __cmpxchg64 could be used.

    __cmpxchg64 has different parameter conventions.

    > Another thing is that choosing different code path depending on
    > has_cmpxchg_double() would be quite messy and won't bode well with
    > many people. I agree that fallback implementation would be heavier
    > for SMP safe operations but some archs already do that for cmpxchg
    > (forgot which one). If we're gonna export this to generic code,
    > wouldn't it be better to implement proper generic fallbacks and
    > provide has_*() as hint?

    A generic fallback for cmpxchg_double would mean having to disable
    interrupts and then take a global spinlock. There are significant scaling
    problems with such an implementation.

    The fallback through the subsystem means that the subsystem can do locking
    that scales better. In the case of SLUB we fall back to a bit lock in the
    page struct which is a hot cache line in the hotpaths. This is the same
    approach as used before the lockless patches and we expect the performance
    on platforms not supporting cmpxchg_double to stay the same.



    \
     
     \ /
      Last update: 2011-06-15 16:29    [W:0.028 / U:0.640 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site