[Openmp-commits] [PATCH] D19878: Use C++11 atomics for ticket locks implementation

[John Mellor-Crummey via Openmp-commits]

jmellorcrummey added a subscriber: jmellorcrummey.
jmellorcrummey added a comment.

I have a few unsolicited comments to offer:

(1) The atomic load in kmp_bakery_check should use memory_order_acquire rather than the more restrictive default memory_order_seq_cst.  Since this condition is tested with inlined code in quite a few places (checking for == or != my_ticket), I recommend that this detail be implemented in an inlined function or macro that is used ubiquitously.
(2) The atomic loads in __kmp_get_ticket_lock_owner and __kmp_is_ticket_lock_nestable should use memory_order_relaxed since they aren't used to enforce ordering for synchronization. The owner is written with atomic_store, which could also be memory_order_relaxed. In __kmp_release_ticket_lock_with_checks, owner_id is written as 0 without an atomic store. Either you need atomic store and load to manipulate this field or not. I dislike mixing the two. If you are concerned about atomicity, you should use an atomic_store with memory_order_relaxed here too.
(3) I recommend renaming the parameters of kmp_bakery_check to more clearly indicate their intent: current_value and target_value, or now_serving and my_ticket.
(4) I dislike the way gtid+1 is written into a ticket lock's owner_id throughout the file and then __kmp_get_ticket_lock_owner returns owner_id -1.  I understand that the range of gtids and that unowned is indicated by 0.  I recommend using macros like GTID_TO_LOCK_OWNER(gtid) and LOCK_OWNER_TO_GTID(owner_id) to replace the ugly -1 and +1 everywhere. Also, I think that  __kmp_get_ticket_lock_owner should return the owner_id and the subtraction should take place outside.
(5) The value for for non-nestable should be defined as something like LOCK_NOT_NESTABLE and not used with a raw -1.
(6) Why not correct the spelling of "checks" in the function prototype __kmp_test_ticket_lock_with_cheks?
(7) I dislike checking the lock owner against 0 to determine if it is unowned. Why not use a macro set to 0: LOCK_NO_OWNER?

I'm sure that there is more worth saying, but I think that I've said enough.


Repository:
  rL LLVM

http://reviews.llvm.org/D19878