Date: Fri, 3 Feb 2017 05:14:15 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Bruce Evans <brde@optusnet.com.au>, 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: <20170203040751.I2354@besplex.bde.org> In-Reply-To: <20170202014204.GA992@dft-labs.eu> References: <201701300224.v0U2Osj1010421@repo.freebsd.org> <20170130142123.V953@besplex.bde.org> <20170201214349.H1136@besplex.bde.org> <20170202014204.GA992@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 2 Feb 2017, Mateusz Guzik wrote:
> 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:
>> ...
>
> Uh, I ended up with the same fix. Committed in r313080.
Thanks.
> 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
> }
You won't want to ifdef this for SMP, so the i386 implementation has
further bugs like I expected (fcmpset is not implemented in the
CPU_DISABLE_CMPXCHG case).
>> 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)
The output parameter is not well named in this or in fcmpset, since
when the comparison fails it holds the compared copy of *dst, not of
*expect_in, and otherwise it is not useful and holds a copy of src.
>> 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.
The main caller of _xfcmpset would be the _cmpset variantL
static __inline int
atomic_cmpset_int(volatile u_int *dst, u_int expect, u_int src)
{
u_int dummy __unused;
return (atomic_xfcmpset_int(dst, &dummy, expect, src));
}
Actually, _cmpset can be built out of _fcmpset even more easily:
static __inline int
atomic_cmpset_int(volatile u_int *dst, u_int expect, u_int src)
{
return (atomic_fcmpset_int(dst, &expect, src));
}
This has to be a function since a macro would expose *&expect to
clobbering.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170203040751.I2354>