[libvirt] [PATCH 1/14] Adding recursive locks

[Laine Stump]

On 03/18/2010 01:04 PM, Eric Blake wrote:
> On 03/18/2010 10:51 AM, Daniel P. Berrange wrote:
>    
>> On Thu, Mar 18, 2010 at 10:41:58AM -0600, Eric Blake wrote:
>>      
>>> On 03/18/2010 09:15 AM, Stefan Berger wrote:
>>>        
>>>> This patch adds recursive locks necessary due to the processing of
>>>> network filter XML that can reference other network filters, including
>>>> references that cause looks. Loops in the XML are prevented but their
>>>> detection requires recursive locks.
>>>>          
>>> ACK.  I had to double-check MSDN to make sure that threads-win32 already
>>> creates recursive mutex by default.
>>>        
>> I don't supposed you read if its possible to make it non-recursive, since
>> the orginal impl is supposed to be non-recursive !
>>      
> Unfortunately, I don't see a way:
>
> http://msdn.microsoft.com/en-us/library/ms684266%28VS.85%29.aspx
>
> states:
>
> After a thread obtains ownership of a mutex, it can specify the same
> mutex in repeated calls to the wait-functions without blocking its
> execution. This prevents a thread from deadlocking itself while waiting
> for a mutex that it already owns. To release its ownership under such
> circumstances, the thread must call ReleaseMutex once for each time that
> the mutex satisfied the conditions of a wait function.
>
> with no mention of any way to parameterize it to be non-recursive.
>
> 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.
>
> 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.

Not speaking in particular about libvirt code, but in general a 
non-recursive mutex can protect you against accidentally modifying a 
data structure inside a function that's called by some other function 
that's in the middle of modifying the same data structure. So it's 
useful not for any sort of concurrency resolution, but as an assertion 
(very important IMO) on top of the normal uses of a recursive lock.

Since any occurrence of a non-recursive lock failing due to the lock 
already being held by the same thread will, by definition, result in a 
dead-lock, we could achieve the same thing (with better error reporting) 
in the case of Windows by adding a simple atomic counter that's 
incremented/decremented along with the lock, and logs an error message 
(and optionally somehow attempts to abort the operation?) if the counter 
ever goes higher than 1.


> Looking more closely at virMutexInit in the
> pthreads version, we use pthread_mutex_init(,NULL), which requests
> PTHREAD_MUTEX_DEFAULT.  But POSIX states:
>
> http://www.opengroup.org/onlinepubs/9699919799/functions/pthread_mutexattr_settype.html
>
> PTHREAD_MUTEX_DEFAULT
>
>      Attempting to recursively lock a mutex of this type results in
> undefined behavior. Attempting to unlock a mutex of this type which was
> not locked by the calling thread results in undefined behavior.
> Attempting to unlock a mutex of this type which is not locked results in
> undefined behavior. An implementation may map this mutex to one of the
> other mutex types.
>
> 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.
>    

Eww. That seems a bit problematic. This has been a very productive 
discussion, eh? ;-)