Date: Mon, 09 May 2011 15:40:49 +0200 From: Andreas Tobler <andreast@FreeBSD.org> To: Attilio Rao <attilio@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Andreas Tobler <andreast@FreeBSD.org>, Nathan Whitehorn <nwhitehorn@FreeBSD.org> Subject: Re: svn commit: r221550 - head/sys/powerpc/conf Message-ID: <4DC7EEE1.8000901@FreeBSD.org> In-Reply-To: <BANLkTik6P8CkOCjzJ4pt0vyMOAZCPRkF1Q@mail.gmail.com> References: <201105062043.p46Kh2Vs065320@svn.freebsd.org> <BANLkTimiKGWeJfdCQrcPvujdfuyvMX8wbQ@mail.gmail.com> <BANLkTimNC4rZvvaH5Dk%2BQpNgE90BjoQ-Sg@mail.gmail.com> <4DC691AE.701@FreeBSD.org> <BANLkTik6P8CkOCjzJ4pt0vyMOAZCPRkF1Q@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 09.05.11 00:15, Attilio Rao wrote: > 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 6 20:43:02 2011 >>>>> New Revision: 221550 >>>>> URL: http://svn.freebsd.org/changeset/base/221550 >>>>> >>>>> Log: >>>>> SMP has worked perfectly for a very long time on 32-bit PowerPC on both >>>>> UP 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 -> atomic_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 system >> 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_long >> 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 functions. > > I'm sorry I couldn't even test-compile it, but I'm confident you can > fix edge cases alone. > Let me know. Both builds ok and booted. Thanks, Andreas
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4DC7EEE1.8000901>