Date: Sun, 08 May 2011 14:50:54 +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, Nathan Whitehorn <nwhitehorn@FreeBSD.org>, andreast@FreeBSD.org Subject: Re: svn commit: r221550 - head/sys/powerpc/conf Message-ID: <4DC691AE.701@FreeBSD.org> In-Reply-To: <BANLkTimNC4rZvvaH5Dk%2BQpNgE90BjoQ-Sg@mail.gmail.com> References: <201105062043.p46Kh2Vs065320@svn.freebsd.org> <BANLkTimiKGWeJfdCQrcPvujdfuyvMX8wbQ@mail.gmail.com> <BANLkTimNC4rZvvaH5Dk%2BQpNgE90BjoQ-Sg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] 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. Gruss, Andreas [-- Attachment #2 --] --- atomic.h.attilio 2011-05-08 11:00:42.000000000 +0200 +++ atomic.h.andreas 2011-05-08 14:01:00.000000000 +0200 @@ -651,16 +651,27 @@ #define atomic_cmpset_rel_ptr(dst, old, new) \ atomic_cmpset_rel_long((volatile u_long *)(dst), (u_long)(old), \ (u_long)(new)) -#else -#define atomic_cmpset_long(dst, old, new) \ - atomic_cmpset_int((volatile u_int *)(dst), (u_int)(old), \ - (u_int)(new)) -#define atomic_cmpset_acq_long(dst, old, new) \ - atomic_cmpset_acq_int((volatile u_int *)(dst), (u_int)(old), \ - (u_int)(new)) -#define atomic_cmpset_rel_long(dst, old, new) \ - atomic_cmpset_rel_int((volatile u_int *)(dst), (u_int)(old), \ - (u_int)(new)) +#else /* __powerpc64__ */ +static __inline int +atomic_cmpset_long(volatile u_long *dst, u_long old, u_long new) +{ + return (atomic_cmpset_int((volatile u_int *)dst, (u_int)old, + (u_int)new)); +} + +static __inline int +atomic_cmpset_acq_long(volatile u_long *dst, u_long old, u_long new) +{ + return (atomic_cmpset_acq_int((volatile u_int *)dst, (u_int)old, + (u_int)new)); +} + +static __inline int +atomic_cmpset_rel_long(volatile u_long *dst, u_long old, u_long new) +{ + return (atomic_cmpset_rel_int((volatile u_int *)dst, (u_int)old, + (u_int)new)); +} #define atomic_cmpset_ptr(dst, old, new) \ atomic_cmpset_int((volatile u_int *)(dst), (u_int)(old), \ @@ -671,7 +682,7 @@ #define atomic_cmpset_rel_ptr(dst, old, new) \ atomic_cmpset_rel_int((volatile u_int *)(dst), (u_int)(old), \ (u_int)(new)) -#endif +#endif /* __powerpc64__ */ static __inline u_int atomic_fetchadd_int(volatile u_int *p, u_int v)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4DC691AE.701>