Re: better atomics - v0.6 - Mailing list pgsql-hackers

On 2014-09-24 14:59:19 +0300, Heikki Linnakangas wrote:
> 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.

Huh. Unsure how they crept in. Will fix.

> >+ * 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.

Hm. The background here is that postgres doesn't need memory barriers
affect sse et al registers - which is why read/write barriers currently
can be defined to be empty on TSO architectures like x86. But e.g. gcc's
barrier intrinsics don't assume that, so they do a mfence or lock xaddl
for read/write barriers. Which isn't cheap.

Will try to find a way to rephrase.

> >+ * 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?

Yes, that's what I'm envisioning. E.g. old versions of solaris studio
don't provide compiler intrinsics and also don't provide reliable ways
to barrier semantics. It doesn't seem worthwile to add a inline assembly
version for that special case.

> 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.

It's the reason I added a inline assembly version of atomics for x86
gcc...

> >+#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.

Sounds good.

> >+ * Spinloop delay -
> 
> Spurious dash.

Probably should rather add a bit more explanation...

> >+/*
> >+ * 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.

It was included from the first version on, and I'd mentioned it a couple
times.

> None of the subsequent patches seem to use it either. Can we just
> leave this out?

I have a further patch (lockless buffer header manipulation) that uses
it to manipulate the usagecount. I can add it back later if you feel
strongly about it.

> 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?

That bit should actually be in the read_u64 implementation, not write...

If you do a compare and exchange you have to provide an 'expected'
value. So, if you do a compare and exchange and the previous value is 0
the value will be written superflously - but the actual value won't change.

> >+    /*
> >+     * 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?

No, it's not. The semaphore emulation doesn't implement
pg_atomic_unlocked_test_flag() (as there's no sane way to do that with
semaphores that I know of). Instead it always returns true. Which
disturbs the test. I guess I'll expand that comment...

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services