Date: Sun, 8 May 2011 18:15:38 -0400 From: Attilio Rao <attilio@freebsd.org> To: Andreas Tobler <andreast@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Nathan Whitehorn <nwhitehorn@freebsd.org> Subject: Re: svn commit: r221550 - head/sys/powerpc/conf Message-ID: <BANLkTik6P8CkOCjzJ4pt0vyMOAZCPRkF1Q@mail.gmail.com> In-Reply-To: <4DC691AE.701@FreeBSD.org> References: <201105062043.p46Kh2Vs065320@svn.freebsd.org> <BANLkTimiKGWeJfdCQrcPvujdfuyvMX8wbQ@mail.gmail.com> <BANLkTimNC4rZvvaH5Dk%2BQpNgE90BjoQ-Sg@mail.gmail.com> <4DC691AE.701@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
2011/5/8 Andreas Tobler <andreast@freebsd.org>: > On 08.05.11 00:17, Attilio Rao wrote: >> >> 2011/5/6 Attilio Rao<attilio@freebsd.org>: >>> >>> 2011/5/6 Nathan Whitehorn<nwhitehorn@freebsd.org>: >>>> >>>> Author: nwhitehorn >>>> Date: Fri May =C2=A06 20:43:02 2011 >>>> New Revision: 221550 >>>> URL: http://svn.freebsd.org/changeset/base/221550 >>>> >>>> Log: >>>> =C2=A0SMP has worked perfectly for a very long time on 32-bit PowerPC = on both >>>> =C2=A0UP and SMP hardware. Enable it in GENERIC. >>>> >>> >>> While working on largeSMP, I think there is a breakage in atomic.h. >>> More specifically, atomic_store_rel_long() (and related functions) are >>> not going to properly work because powerpc defines them as: >>> >>> atomic_store_rel_long -> =C2=A0atomic_store_rel_32(volatile u_int *p, u= _int v) >>> >>> while this should really follow the long arguments. >>> >>> This happens because powerpc doesn't follow the other architectures >>> semantic on defining the "similar" atomic operations. >>> Other arches define an hardcode version of _type version of the >>> function and than make a macro the _32 (or whatever) version. >>> In other words this is what they do: >>> >>> void >>> atomic_store_rel_32() >>> { >>> ... >>> } >>> >>> #define atomic_store_rel_int atomic_store_rel_32 >>> >>> which si clearly dangerous for cases as reported above. Maybe that >>> could be fixed by passing sized types, rather than simply int or long >>> in numbered version, but I'd really prefer to follow the semantic by >>> other architectures and then have: >>> >>> void >>> atomic_store_rel_int() >>> { >>> ... >>> } >>> >>> #define atomic_store_rel_32 atomic_store_rel_int >>> >>> I fixed the ATOMIC_STORE_LOAD case in my code, because I needed it, >>> but the final cleanup is much bigger. >>> I can make a patch tomorrow if you can test it. >>> >> >> Can you please test and review this patch?: >> http://www.freebsd.org/~attilio/largeSMP/atomic-powerpc.diff >> >> Unfortunately I'm having issues with the toolchains in atm, so I can't >> really neither test compile it. > > I built kernel and world on both, on a 32-bit system and on a 64-bit syst= em > with 32-bit compat support. > > The 32-bit world failed due to type punning issues from umtx.h:121ff > > Attached my try to workaround these issues. With my try I can build both, > kernel and world. Right now I have a world running with it (32-bit). > I do not know if my try is correct. Even less after reading the comments > from Bruce. > But I think if it is on a somehow correct way, then we need something > similar for the other _long functions on 32-bit where we go from the u_lo= ng > to u_int. (e.g, atomic_add_long etc.) > > I'm ready for more testing. So based on your and Bruce's feedbacks I've reworked the patch a bit: http://www.freebsd.org/~attilio/largeSMP/atomic-powerpc2.diff This should make type-pun correctly and avoid auto-casting on _ptr function= s. I'm sorry I couldn't even test-compile it, but I'm confident you can fix edge cases alone. Let me know. Thanks, Attilio --=20 Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BANLkTik6P8CkOCjzJ4pt0vyMOAZCPRkF1Q>