lkml.org 
[lkml]   [2010]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [thisops uV2 04/10] x86: Support for this_cpu_add,sub,dec,inc_return
    * Christoph Lameter (cl@linux.com) wrote:
    > On Mon, 29 Nov 2010, Mathieu Desnoyers wrote:
    >
    > > > > > +#define percpu_add_return_op(var, val) \
    > > > > > +({ \
    > > > > > + typedef typeof(var) pao_T__; \
    > > > > > + typeof(var) pfo_ret__ = val; \
    > > > > > + if (0) { \
    > > > > > + pao_T__ pao_tmp__; \
    > > > > > + pao_tmp__ = (val); \
    > > > > > + (void)pao_tmp__; \
    > > > > > + } \
    > > > >
    > > > > OK, I'm dumb: why is the above needed ?
    > > >
    > > > Ensure that the compiler agrees that *var and val are compatible. Taken
    > > > over from percpu_add_op().
    > >
    > > Isn't that the purpose of __builtin_types_compatible_p(t1, t2) ?
    >
    > We also have a __same_type() macro in linux/compiler.h... But if I use
    > that the kernel build fails. I guess the check is too strict.

    Ah, ok, so you don't mind if var and val have different types. You just want
    the compiler to be able to cast one into the other without complaining.

    Isn't it already checked by "typeof(var) pfo_ret__ = val;" ?

    Which semantic do you expect when using different types for var and val ?

    The assembly all acts on the following pfo_ret__ :

    typeof(var) pfo_ret__ = val;

    But at the end, you return :

    pfo_ret__ + (val);

    So if pfo_ret has a type smaller than val (e.g. unsigned short vs unsigned int),
    unless I'm mistakened, the type returned by percpu_add_return_op will be an
    unsigned int (the larger of the two), but the atomic operation is performed on
    an unsigned short. This might cause confusion for the caller. Casting

    pfo_ret__ + (typeof(var)) (val);

    might be appropriate.

    Thanks,

    Mathieu

    >
    > ---
    > arch/x86/include/asm/percpu.h | 6 +-----
    > 1 file changed, 1 insertion(+), 5 deletions(-)
    >
    > Index: linux-2.6/arch/x86/include/asm/percpu.h
    > ===================================================================
    > --- linux-2.6.orig/arch/x86/include/asm/percpu.h 2010-11-29 12:46:50.000000000 -0600
    > +++ linux-2.6/arch/x86/include/asm/percpu.h 2010-11-29 12:52:25.000000000 -0600
    > @@ -127,11 +127,7 @@ do { \
    > typedef typeof(var) pao_T__; \
    > const int pao_ID__ = (__builtin_constant_p(val) && \
    > ((val) == 1 || (val) == -1)) ? (val) : 0; \
    > - if (0) { \
    > - pao_T__ pao_tmp__; \
    > - pao_tmp__ = (val); \
    > - (void)pao_tmp__; \
    > - } \
    > + BUILD_BUG_ON(!__same_type(var, val)); \
    > switch (sizeof(var)) { \
    > case 1: \
    > if (pao_ID__ == 1) \
    >
    >

    --
    Mathieu Desnoyers
    Operating System Efficiency R&D Consultant
    EfficiOS Inc.
    http://www.efficios.com


    \
     
     \ /
      Last update: 2010-11-29 20:25    [from the cache]
    ©2003-2014 Jasper Spaans. hosted at Digital Ocean