Re: better atomics - v0.6

On 09/23/2014 12:01 AM, Andres Freund wrote:
> Hi,
>
> I've finally managed to incorporate (all the?) feedback I got for
> 0.5. Imo the current version looks pretty good.

Thanks! I agree it looks good. Some random comments after a quick 
read-through:

There are some spurious whitespace changes in spin.c.

> + * These files can provide the full set of atomics or can do pretty much
> + * nothing if all the compilers commonly used on these platforms provide
> + * useable generics. It will often make sense to define memory barrier
> + * semantics in those, since e.g. the compiler intrinsics for x86 memory
> + * barriers can't know we don't need read/write barriers anything more than a
> + * compiler barrier.

I didn't understand the latter sentence.

> + * Don't add an inline assembly of the actual atomic operations if all the
> + * common implementations of your platform provide intrinsics. Intrinsics are
> + * much easier to understand and potentially support more architectures.

What about uncommon implementations, then? I think we need to provide 
inline assembly if any supported implementation on the platform does not 
provide intrinsics, otherwise we fall back to emulation. Or is that 
exactly what you're envisioning we do?

I believe such a situation hasn't come up on the currently supported 
platforms, so we don't necessary have to have a solution for that, but 
the comment doesn't feel quite right either.

> +#define CHECK_POINTER_ALIGNMENT(ptr, bndr) \
> +    Assert(TYPEALIGN((uintptr_t)(ptr), bndr))

Would be better to call this AssertAlignment, to make it clear that this 
is an assertion, not something like CHECK_FOR_INTERRUPTS(). And perhaps 
move this to c.h where other assertions are - this seems generally useful.

> + * Spinloop delay -

Spurious dash.

> +/*
> + * pg_fetch_add_until_u32 - saturated addition to variable
> + *
> + * Returns the the value of ptr after the arithmetic operation.
> + *
> + * Full barrier semantics.
> + */
> +STATIC_IF_INLINE uint32
> +pg_atomic_fetch_add_until_u32(volatile pg_atomic_uint32 *ptr, int32 add_,
> +                              uint32 until)
> +{
> +    CHECK_POINTER_ALIGNMENT(ptr, 4);
> +    return pg_atomic_fetch_add_until_u32_impl(ptr, add_, until);
> +}
> +

This was a surprise to me, I don't recall discussion of an 
"fetch-add-until" operation, and hadn't actually ever heard of it 
before. None of the subsequent patches seem to use it either. Can we 
just leave this out?

s/the the/the/

> +#ifndef PG_HAS_ATOMIC_WRITE_U64
> +#define PG_HAS_ATOMIC_WRITE_U64
> +static inline void
> +pg_atomic_write_u64_impl(volatile pg_atomic_uint64 *ptr, uint64 val)
> +{
> +    /*
> +     * 64 bit writes aren't safe on all platforms. In the generic
> +     * implementation implement them as an atomic exchange. That might store a
> +     * 0, but only if the prev. value also was a 0.
> +     */
> +    pg_atomic_exchange_u64_impl(ptr, val);
> +}
> +#endif

Why is 0 special?

> +     * fail or suceed, but always return the old value

s/suceed/succeed/. Add a full stop to end.

> +    /*
> +     * Can't run the test under the semaphore emulation, it doesn't handle
> +     * checking edge cases well.
> +     */
> +#ifndef PG_HAVE_ATOMIC_FLAG_SIMULATION
> +    test_atomic_flag();
> +#endif

Huh? Is the semaphore emulation broken, then?

- Heikki