From owner-svn-src-projects@FreeBSD.ORG Tue Mar 6 04:58:50 2012 Return-Path: Delivered-To: svn-src-projects@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C4796106566B; Tue, 6 Mar 2012 04:58:50 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail05.syd.optusnet.com.au (mail05.syd.optusnet.com.au [211.29.132.186]) by mx1.freebsd.org (Postfix) with ESMTP id 56FB38FC08; Tue, 6 Mar 2012 04:58:50 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail05.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q264waFO013636 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 6 Mar 2012 15:58:38 +1100 Date: Tue, 6 Mar 2012 15:58:36 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Olivier Houchard In-Reply-To: <20120303165737.GA26775@ci0.org> Message-ID: <20120306140646.S940@besplex.bde.org> References: <201203031223.q23CN73s081573@svn.freebsd.org> <20120304011922.G5792@besplex.bde.org> <20120303165737.GA26775@ci0.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-projects@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans Subject: Re: svn commit: r232456 - projects/armv6/sys/arm/include X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Mar 2012 04:58:50 -0000 On Sat, 3 Mar 2012, Olivier Houchard wrote: > On Sun, Mar 04, 2012 at 02:14:46AM +1100, Bruce Evans wrote: >> On Sat, 3 Mar 2012, Olivier Houchard wrote: Sorry this reply took so long. >>> Log: >>> Get the right casts for long operations >> >> I think you mean "the wrong casts". i386 was broken similarly, but amd64 >> is still correct -- amd64 has no casts at all in its big list of #defines >> (but the list is excessively complete, with must cases unused). > > I understand your concerns. These casts are indeed bogus, and will become even > more when we'll support 64bits arm, which should come with 64bits long. Won't they just fail then, and need separate functions which mostly make the problem go away, as on amd64? The patch seems to add a lot of 64-bit support, which confused me at first. > I can't do much for long which should be 64bits even on 32bits machines, that > is set in stone now, however I can certainly remove the bogus casts. > > Would the attached patch be OK for you ? > It duplicates the various atomic functions to add a _long variant (for armv6 > at least, for armv5 it just introduces _long variants which calls the _32 > version, but at least it should catch any signedness/type error), and it > removes the bogus casts for the ptr version, and just #defines it to the __32 > version, since that's what uintptr_t is. This seems to be essentially correct -- don't use blind casts in macros, and don't duplicate the functions (at least large ones), but provide wrappers implemented as inline functions. The wrappers do the same thing as the macros, but in a type-safe way. Perhaps they can be generated a bit more automatically or otherwise reduced to 1 line each, but I don't want to use complicated macros for this. % Index: atomic.h % =================================================================== % --- atomic.h (revision 232462) % +++ atomic.h (working copy) % @@ -74,6 +74,21 @@ % #endif % } % % +#define ATOMIC_ACQ_REL_LONG(NAME) \ % +static __inline void \ % +atomic_##NAME##_acq_long(__volatile u_long *p, u_long v) \ % +{ \ % + atomic_##NAME##_long(p, v); \ % + __do_dmb(); \ % +} \ % + \ % +static __inline void \ % +atomic_##NAME##_rel_long(__volatile u_long *p, u_long v) \ % +{ \ % + __do_dmb(); \ % + atomic_##NAME##_long(p, v); \ % +} % + % #define ATOMIC_ACQ_REL(NAME, WIDTH) \ % static __inline void \ % atomic_##NAME##_acq_##WIDTH(__volatile uint##WIDTH##_t *p, uint##WIDTH##_t v)\ Small functions can be done like this, by duplicating the code for longs, and putting it in macros, but I think the above already has too much macro-ization. Code like this is hard to read, and probably not even much shorter and easier to write, if you organize the wrappers in the best way. % ... % @@ -489,9 +649,64 @@ % #define atomic_subtract_rel_32 atomic_subtract_32 % #define atomic_subtract_acq_32 atomic_subtract_32 % #define atomic_store_rel_32 atomic_store_32 % +#define atomic_store_rel_long atomic_store_long % #define atomic_load_acq_32 atomic_load_32 % +#define atomic_load_acq_long atomic_load_long % #undef __with_interrupts_disabled % % +static __inline void % +atomic_add_long(volatile u_long *p, u_long v) % +{ % + % + atomic_add_32((volatile uint32_t *)p, (volatile uint32_t)v); % +} Most of the previous changes seem to be for adding 64-bit longs. Now I think we're back in the 32-bit long case, with wrappers like I want (more verbose than I want). Casting v seems bogus. How can a function parameter that is passed by value be volatile? Old arm code has just 2 simimlar volatiles, both for the non-pointer parameter in 2 cmpset_32's. Maybe it needs to be volatile in the asm that accesses it, but I doubt this, and casting it here doesn't make it volatile there. amd64 only uses volatiles for pointed-to variables. % ... % #endif /* _LOCORE */ LOCORE ifdefs in atomic.h are bogus, and were removed on amd64. The comment on this one is backwards, at least in the main arm tree (the code says #ifndef, not #ifdef). Adding C functions unused LOCORE part of the code ensure that it is unusable as well as unused in asm files. Bruce