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

[Denis V. Lunev]

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