From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:36009)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1ZtFh8-0002oA-8t
	for qemu-devel@nongnu.org; Mon, 02 Nov 2015 09:02:53 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1ZtFh2-0003Tr-Gq
	for qemu-devel@nongnu.org; Mon, 02 Nov 2015 09:02:46 -0500
References: <1446044465-19312-1-git-send-email-den@openvz.org>
	<1446044465-19312-2-git-send-email-den@openvz.org>
	<20151030154135.GA16864@stefanha-x1.localdomain>
	<563619E7.9060305@openvz.org>
	<20151102131236.GC10139@stefanha-x1.localdomain>
	<563767A5.5030801@openvz.org> <56376B57.3090806@redhat.com>
From: "Denis V. Lunev" <den@openvz.org>
Message-ID: <56376CEA.5060301@openvz.org>
Date: Mon, 2 Nov 2015 17:02:18 +0300
MIME-Version: 1.0
In-Reply-To: <56376B57.3090806@redhat.com>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 1/4] fifolock: create rfifolock_is_locked
 helper
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paolo Bonzini <pbonzini@redhat.com>, Stefan Hajnoczi <stefanha@gmail.com>
Cc: qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>, qemu-stable@nongnu.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