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

[Denis V. Lunev]

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