Re: [thisops uV2 04/10] x86: Support for this_cpu_add,sub,dec,inc_return

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

* 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