Date: Thu, 2 Feb 2017 02:42:04 +0100 From: Mateusz Guzik <mjguzik@gmail.com> 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: <20170202014204.GA992@dft-labs.eu> In-Reply-To: <20170201214349.H1136@besplex.bde.org> References: <201701300224.v0U2Osj1010421@repo.freebsd.org> <20170130142123.V953@besplex.bde.org> <20170201214349.H1136@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Feb 01, 2017 at 10:12:57PM +1100, Bruce Evans wrote:
> 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);
>
Uh, I ended up with the same fix. Committed in r313080.
Sorry for breakage in the first place.
> 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.
>
The primitive was modeled after atomic_compare_exchange_* from c11
atomics. I don't see what's the benefit of storing the result
separately.
As it is, the primitive fits nicely into loops like "inc not zero".
Like this:
r = *counter;
for (;;) {
if (r == 0)
break;
if (atomic_fcmpset_int(counter, &r, r + 1))
break;
// r we can loop back to re-test r
}
> 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.
>
_fcmpset is specifically for callers who want the value back. Ones which
don't can use the _cmpset variant.
--
Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170202014204.GA992>