linux-kernel - Re: [thisops uV2 04/10] x86: Support for this_cpu_add,sub,dec,inc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101129192238.GD25610@Krystal>
Date:	Mon, 29 Nov 2010 14:22:38 -0500
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Christoph Lameter <cl@...ux.com>
Cc:	akpm@...ux-foundation.org, Pekka Enberg <penberg@...helsinki.fi>,
	linux-kernel@...r.kernel.org,
	Eric Dumazet <eric.dumazet@...il.com>,
	Tejun Heo <tj@...nel.org>
Subject: Re: [thisops uV2 04/10] x86: Support for
	this_cpu_add,sub,dec,inc_return

* Christoph Lameter (cl@...ux.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/