GNOME Bugzilla – Bug 686191
g_mutex_get_impl() should use g_atomic_pointer_get()
Last modified: 2012-10-29 15:19:18 UTC
static pthread_mutex_t * g_mutex_get_impl (GMutex *mutex) { pthread_mutex_t *impl = mutex->p; ^^^^^^^^^
Great... now I have to remember why I originally thought that this would be safe.... I should probably have written a comment about that :) ...or maybe it's actually unsafe.
First things first: on the writer/setter side this is always safe because we will use atomic operations there. The only question is about the ordering of operations on the reader side if there is a 'nearby' writer. If I were to guess, the logic would be along the lines that the mutex->p will either be NULL or non-NULL. If it's NULL then we take the slow path and we're safe. If it's non-NULL then it has had pthread_mutex_init() called on it (because the writer is well-ordered). Surely it's safe to call pthread_mutex_lock() on that right away being that is itself is an atomic/barrier/order-imposing operation on the reader side. Note also that any loads that pthread_mutex_lock() does against mutex->p would be dependent on the address of ->p. These sorts of dependent loads are defined to be ordered on all machines except alpha... but even there I could appeal to my above argument. One could imagine exotic scenarios in which the memory used by pthread_mutex_lock() is fetched before the value of the pointer but these sort of situations are exactly what mutexes are supposed to prevent from happening... Discuss?
I think that's safe in practice, but does make assumptions on how the actual lock operation is implemented. Agree? You are essentially assuming that first thing pthread_mutex_lock() does before dereferencing the pointer is a memory barrier. I can't imagine why it wouldn't, but can't think of the library providing such a promise either.
I can find myself agreeing to this line of reasoning -- particularly because the pthread implementation is about to be relegated to the non-Linux case (where we are, by definition, somewhat more likely to see exotic things...)
Created attachment 227508 [details] [review] gthread-posix: always use atomic pointer ops On platforms where dependent loads can be reordered (alpha) and we have exotic implementation of pthread_mutex_lock() it could be possible that our implementation of g_mutex_lock() is unsafe. Always use atomic operations to avoid this possibility.
Comment on attachment 227508 [details] [review] gthread-posix: always use atomic pointer ops As if we do formal reviews in GNOME :)
Attachment 227508 [details] pushed as 311e18a - gthread-posix: always use atomic pointer ops