From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59009) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtFLJ-0001Bk-Rf for qemu-devel@nongnu.org; Mon, 02 Nov 2015 08:40:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtFLI-0006mX-WE for qemu-devel@nongnu.org; Mon, 02 Nov 2015 08:40:13 -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> From: "Denis V. Lunev" Message-ID: <563767A5.5030801@openvz.org> Date: Mon, 2 Nov 2015 16:39:49 +0300 MIME-Version: 1.0 In-Reply-To: <20151102131236.GC10139@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: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Paolo Bonzini , qemu-devel@nongnu.org, Stefan Hajnoczi , qemu-stable@nongnu.org On 11/02/2015 04:12 PM, Stefan Hajnoczi wrote: > On Sun, Nov 01, 2015 at 04:55:51PM +0300, Denis V. Lunev wrote: >> 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. > The owner thread field is only valid when nesting > 0. > >> with your proposal the function become racy if called from outer >> thread. I need to think a bit here. > 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. 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. Den