On Thu, Mar 18, 2010 at 11:16:09AM -0600, Eric Blake wrote:
On 03/18/2010 11:04 AM, Eric Blake wrote:
But what does a true non-recursive mutex buy you? The only difference between recursive and true non-recursive is whether you declare that an attempt to relock a mutex that you already own is a fatal deadlock error, rather than incrementing a counter for matching unlocks. It's just that non-recursive mutexes typically have faster implementations.
Actually, it DOES buy something. virCondWait DEPENDS on getting a true non-recursive function (PTHREAD_MUTEX_NORMAL or PTHREAD_MUTEX_ERRORCHECK, although the latter has better guaranteed behavior in the case of deadlock), because POSIX is clear that:
It is advised that an application should not use a PTHREAD_MUTEX_RECURSIVE mutex with condition variables because the implicit unlock performed for a pthread_cond_timedwait() or pthread_cond_wait() may not actually release the mutex (if it had been locked multiple times). If this happens, no other thread can satisfy the condition of the predicate.
For that matter, do we even need the distinction? Maybe ALL our code should be using recursive mutexes by default, by changing virMutexInit to be recursive no matter what, and not worry about introducing virMutexInitRecursive. Looking more closely at virMutexInit in the pthreads version, we use pthread_mutex_init(,NULL), which requests PTHREAD_MUTEX_DEFAULT.
That is, our current implementation of virMutexInit is NOT a true non-recursive mutex, so much as a mutex that is unspecified whether it is recursive or not.
And that means we have a bug in threads-pthread.c - we should be explicitly requesting a pthread_mutexattr with PTHREAD_MUTEX_ERRORCHECK rather than relying on NULL.
No, we should set PTHREAD_MUTEX_NORMAL - we don't want it returning an error code on failure, because all our code assumes pthread_mutex_lock will not fail. Deadlock is what we want. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|