svn commit: r312975 - head/sys/i386/include

[Bruce Evans]

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