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