From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:33251)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1Zst7C-0008Rn-2n
	for qemu-devel@nongnu.org; Sun, 01 Nov 2015 08:56:10 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1Zst78-0006c8-TN
	for qemu-devel@nongnu.org; Sun, 01 Nov 2015 08:56:10 -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>
From: "Denis V. Lunev" <den@openvz.org>
Message-ID: <563619E7.9060305@openvz.org>
Date: Sun, 1 Nov 2015 16:55:51 +0300
MIME-Version: 1.0
In-Reply-To: <20151030154135.GA16864@stefanha-x1.localdomain>
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: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, qemu-devel@nongnu.org, qemu-stable@nongnu.org

On 10/30/2015 06:41 PM, Stefan Hajnoczi wrote:
> On Wed, Oct 28, 2015 at 06:01:02PM +0300, Denis V. Lunev wrote:
>> +int  rfifolock_is_locked(RFifoLock *r);
> Please use bool instead of int.
>
>> diff --git a/util/rfifolock.c b/util/rfifolock.c
>> index afbf748..8ac58cb 100644
>> --- a/util/rfifolock.c
>> +++ b/util/rfifolock.c
>> @@ -48,7 +48,7 @@ void rfifolock_lock(RFifoLock *r)
>>       /* Take a ticket */
>>       unsigned int ticket = r->tail++;
>>   
>> -    if (r->nesting > 0 && qemu_thread_is_self(&r->owner_thread)) {
>> +    if (r->nesting > 0 && rfifolock_is_locked(r)) {
>>           r->tail--; /* put ticket back, we're nesting */
>>       } else {
>>           while (ticket != r->head) {
>> @@ -69,10 +69,15 @@ 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_locked(r));
>>       if (--r->nesting == 0) {
>>           r->head++;
>>           qemu_cond_broadcast(&r->cond);
>>       }
>>       qemu_mutex_unlock(&r->lock);
>>   }
>> +
>> +int rfifolock_is_locked(RFifoLock *r)
>> +{
>> +    return qemu_thread_is_self(&r->owner_thread);
>> +}
> The function name confused me since "does the current thread hold the
> lock?" != "does anyone currently hold the lock?".
>
> I suggest:
>
> bool rfifolock_held_by_current_thread(RFifoLock *r) {
>      return r->nesting > 0 && qemu_thread_is_self(&r->owner_thread);
> }
>
> Then the r->nesting > 0 testing can also be dropped by callers, which is
> good since rfifolock_is_locked() does not return a meaningful result
> when r->nesting == 0.
actually the function is broken in the current state:
     aio_context_acquire()
     aio_context_release()
     aio_context_is_locked() will return true.
the problem is that owner thread is not reset on lock release.

with your proposal the function become racy if called from outer
thread. I need to think a bit here.

May be we'll need 2 versions - locked and unlocked or
something other should be added. I am in progress of
invention :)

Den