On Thu, Jul 3, 2014 at 10:56 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: > On Tue, Jul 1, 2014 at 4:10 PM, Andres Freund <and...@2ndquadrant.com> wrote:
Further review of patch: 1. /* * pg_atomic_test_and_set_flag - TAS() * * Acquire/read barrier semantics. */ STATIC_IF_INLINE_DECLARE bool pg_atomic_test_set_flag(volatile pg_atomic_flag *ptr); a. I think Acquire and read barrier semantics aren't equal. With acquire semantics, "the results of the operation are available before the results of any operation that appears after it in code" which means it applies for both load and stores. Definition of read barrier just ensures loads. So will be right to declare like above in comments? b. I think it adheres to Acquire semantics for platforms where that semantics are supported else it defaults to full memory ordering. Example : #define TAS(lock) (InterlockedCompareExchange(lock, 1, 0)) Is it right to declare that generic version of function adheres to Acquire semantics. 2. bool pg_atomic_test_set_flag_impl(volatile pg_atomic_flag *ptr) { return TAS((slock_t *) &ptr->sema); #define TAS(lock) (InterlockedCompareExchange(lock, 1, 0)) a. In above usage (ptr->sema), sema itself is not declared as volatile, so would it be right to use it in this way for API (InterlockedCompareExchange) expecting volatile. b. Also shouldn't this implementation be moved to atomics-generic-msvc.h 3. /* * pg_atomic_unlocked_test_flag - TAS() * * No barrier semantics. */ STATIC_IF_INLINE_DECLARE bool pg_atomic_unlocked_test_flag(volatile pg_atomic_flag *ptr); a. There is no setting in this, so using TAS in function comments seems to be wrong. b. BTW, I am not able see this function in C11 specs. 4. #if !defined(PG_HAS_ATOMIC_TEST_SET_FLAG) && defined(PG_HAVE_ATOMIC_EXCHANGE_U32) .. #define PG_HAS_ATOMIC_UNLOCKED_TEST_FLAG static inline bool pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr) { return pg_atomic_read_u32_impl(ptr) == 1; } #elif !defined(PG_HAS_ATOMIC_TEST_SET_FLAG) && defined(PG_HAVE_ATOMIC_COMPARE_EXCHANGE_U32) .. #define PG_HAS_ATOMIC_UNLOCKED_TEST_FLAG static inline bool pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr) { return (bool) pg_atomic_read_u32_impl(ptr); } Is there any reason to keep these two implementations different? 5. atomics-generic-gcc.h #ifndef PG_HAS_ATOMIC_UNLOCKED_TEST_FLAG #define PG_HAS_ATOMIC_UNLOCKED_TEST_FLAG static inline bool pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr) { return ptr->value == 0; } #endif Don't we need to compare it with 1 instead of 0? 6. atomics-fallback.h pg_atomic_unlocked_test_flag_impl(volatile pg_atomic_flag *ptr) { /* * Can't do this in the semaphore based implementation - always return * true. Do this in the header so compilers can optimize the test away. */ return true; } Why we can't implement this in semaphore based implementation? 7. /* * pg_atomic_clear_flag - release lock set by TAS() * * Release/write barrier semantics. */ STATIC_IF_INLINE_DECLARE void pg_atomic_clear_flag(volatile pg_atomic_flag *ptr); a. Are Release and write barriers equivalent? With release semantics, the results of the operation are available after the results of any operation that appears before it in code. A write barrier acts as a compiler barrier, and orders stores. I think both definitions are not same. b. IIUC then current code doesn't have release semantics for unlock/clear. We are planing to introduce it in this patch, also this doesn't follow the specs which says that clear should have memory_order_seq_cst ordering semantics. As per its current usage in patch (S_UNLOCK), it seems reasonable to have *release* semantics for this API, however I think if we use it for generic purpose then it might not be right to define it with Release semantics. 8. #define PG_HAS_ATOMIC_CLEAR_FLAG static inline void pg_atomic_clear_flag_impl(volatile pg_atomic_flag *ptr) { /* XXX: release semantics suffice? */ pg_memory_barrier_impl(); pg_atomic_write_u32_impl(ptr, 0); } Are we sure that having memory barrier before clearing flag will achieve *Release* semantics; as per my understanding the definition of Release sematics is as below and above doesn't seem to follow the same. "With release semantics, the results of the operation are available after the results of any operation that appears before it in code." 9. static inline uint32 pg_atomic_exchange_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 xchg_) { uint32 old; while (true) { old = pg_atomic_read_u32_impl(ptr); if (pg_atomic_compare_exchange_u32_impl(ptr, &old, xchg_)) break; } return old; } What is the reason to implement exchange as compare-and-exchange, at least for Windows I know corresponding function (InterlockedExchange) exists. I could not see any other implementation of same. I think this at least deserves comment explaining why we have done the implementation this way. 10. STATIC_IF_INLINE_DECLARE uint32 pg_atomic_fetch_add_u32(volatile pg_atomic_uint32 *ptr, int32 add_); STATIC_IF_INLINE_DECLARE uint32 pg_atomic_fetch_sub_u32(volatile pg_atomic_uint32 *ptr, int32 sub_); The function name and input value seems to be inconsistent. The function name contains *_u32*, however the input value is *int32*. 11. Both pg_atomic_fetch_and_u32_impl() and pg_atomic_fetch_or_u32_impl() are implemented using compare_exchange routines, don't we have any native API's which we can use for implementation. 12. I am not able to see API's pg_atomic_add_fetch_u32(), pg_atomic_sub_fetch_u32() in C11 atomic ops specs, do you need it for something specific? 13. + * The non-intrinsics version is only available in vista upwards, so use the + * intrisc version. spelling of intrinsic is wrong. 14. * pg_memory_barrier - prevent the CPU from reordering memory access * * A memory barrier must act as a compiler barrier, Is it ensured in all cases that pg_memory_barrier implies compiler barrier as well? Example for msvc case, the specs doesn't seem to guarntee it. I understand that this rule (or assumption) is carried forward from existing implementation, however I am not aware if it is true for all cases, so thought of mentioning here even though we don't want to do anything considering it an existing behaviour. Links: http://en.wikipedia.org/wiki/Memory_barrier http://msdn.microsoft.com/en-us/library/windows/desktop/ms684208(v=vs.85).aspx With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com