Re: [Qemu-devel] [PATCH 1/4] fifolock: create rfifolock_is_locked helper

["Denis V. Lunev" <den@openvz.org>]

On 11/02/2015 04:55 PM, Paolo Bonzini wrote:
>
> On 02/11/2015 14:39, Denis V. Lunev wrote:
>>> This is thread-safe:
>>>
>>> bool owner;
>>>
>>> qemu_mutex_lock(&r->lock);
>>> owner = r->nesting > 0 && qemu_thread_is_self(&r->owner_thread);
>>> qemu_mutex_unlock(&r->lock);
>>>
>>> return owner;
>> yep, I know.
>>
>> But I do not want to take the lock for check.
> You can use a trylock.  If it fails, it is definitely safe to return false.
>
>> IMHO it would be better to
>>
>> @@ -68,11 +68,16 @@ void rfifolock_lock(RFifoLock *r)
>>   void rfifolock_unlock(RFifoLock *r)
>>   {
>>       qemu_mutex_lock(&r->lock);
>> -    assert(r->nesting > 0);
>> -    assert(qemu_thread_is_self(&r->owner_thread));
>> +    assert(rfifolock_is_owner(r));
>>       if (--r->nesting == 0) {
>> +        qemu_thread_clear(&r->owner_thread);
>>           r->head++;
>>           qemu_cond_broadcast(&r->cond);
>>       }
>>       qemu_mutex_unlock(&r->lock);
>>   }
>> +
>> +bool rfifolock_is_owner(RFifoLock *r)
>> +{
>> +    return r->nesting > 0 && qemu_thread_is_self(&r->owner_thread);
>> +}
>>
>> which does not require lock and thread safe.
> I think it requires memory barriers though.  But it can be simplified:
> if you clear owner_thread, you do not need to check r->nesting in
> rfifolock_is_owner.
>
> Clearing owner_thread can be done with a simple memset.
this does not require memory barrier as call to qemu_call_broadcast
will do the job just fine.

The check for ownership is actually enough as:
- current thread could be set in the current thread only and
   cleared in the same current thread only. This is not racy at all :)
- count check is moved here for convenience only by request of
   Stefan, it is not required at all to make a decision with after
   clearing the thread.

OK, I can use memset for sure if it will be accepted :)
This was my first opinion.

Den