Date: Wed, 1 Feb 2017 22:12:57 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Evans <brde@optusnet.com.au> Cc: Mateusz Guzik <mjg@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r312975 - head/sys/i386/include Message-ID: <20170201214349.H1136@besplex.bde.org> In-Reply-To: <20170130142123.V953@besplex.bde.org> References: <201701300224.v0U2Osj1010421@repo.freebsd.org> <20170130142123.V953@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 30 Jan 2017, Bruce Evans wrote:
> On Mon, 30 Jan 2017, Mateusz Guzik wrote:
>
>> Log:
>> i386: add atomic_fcmpset
>>
>> Tested by: pho
>
> This is has some bugs and style bugs.
This is still broken. The invalid asm breaks building at least atomic.o
with gcc-4.2.1.
Tested fix:
X Index: i386/include/atomic.h
X ===================================================================
X --- i386/include/atomic.h (revision 313007)
X +++ i386/include/atomic.h (working copy)
X @@ -225,9 +225,9 @@
X " cmpxchgl %3,%1 ; "
X " sete %0 ; "
X "# atomic_cmpset_int"
X - : "=r" (res), /* 0 */
X + : "=q" (res), /* 0 */
X "+m" (*dst), /* 1 */
X - "+a" (*expect) /* 2 */
X + "+a" (*expect) /* 2 */
X : "r" (src) /* 3 */
X : "memory", "cc");
X return (res);
The semantics of fcmpset seem to be undocumented. On x86, *expect is
updated non-atomically by a store in the output parameter. I think
cmpxchg updates the "a" register atomically, but then the output
parameter causes this to be stored non-atomically to *expect. A better
API would somehow return the "a" register and let the caller store it
if it wants. Ordinary cmpset can be built on this by not storing, and
the caller can do the store atomically to a different place if *expect
is too volatile to be atomic.
Maybe just decouple the input parameter from the output parameter. The
following works right (for an amd64 API):
Y static __inline int
Y atomic_xfcmpset_long(volatile u_long *dst, u_long *expect_out, u_long expect_in,
Y u_long src)
Y {
Y u_long expect;
Y u_char res;
Y
Y expect = expect_in;
Y __asm __volatile(
Y " " MPLOCKED " "
Y " cmpxchgq %3,%1 ; "
Y " sete %0 ; "
Y "# atomic_fcmpset_long"
Y : "=r" (res), /* 0 */
Y "+m" (*dst), /* 1 */
Y "+a" (expect) /* 2 */
Y : "r" (src) /* 3 */
Y : "memory", "cc");
Y *expect_out = expect;
If the caller doesn't want to use *expect_out, it passes a pointer to an
unused local variable. The compiler can then optimize away the store
since it is not hidden in the asm.
Y return (res);
Y }
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170201214349.H1136>