Messages in this thread | | | Date | Mon, 29 Nov 2010 14:22:38 -0500 | From | Mathieu Desnoyers <> | Subject | Re: [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
| |