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

[Stefan Hajnoczi]

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;

signature.asc
Description: PGP signature