On 09/23/2014 12:01 AM, Andres Freund wrote:

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.
+ */
+pg_atomic_fetch_add_until_u32(volatile pg_atomic_uint32 *ptr, int32 add_,
+                                                         uint32 until)
+       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/

+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);

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.
+        */
+       test_atomic_flag();

Huh? Is the semaphore emulation broken, then?

- Heikki

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to