Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/thread-pool.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/util/thread-pool.c b/util/thread-pool.c
index 610646d131..06ada38376 100644
--- a/util/thread-pool.c
+++ b/util/thread-pool.c
@@ -78,7 +78,7 @@ static void *worker_thread(void *opaque)
 {
     ThreadPool *pool = opaque;
 
-    qemu_mutex_lock(&pool->lock);
+    QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
     pool->pending_threads--;
     do_spawn_thread(pool);
 
@@ -88,9 +88,9 @@ static void *worker_thread(void *opaque)
 
         do {
             pool->idle_threads++;
-            qemu_mutex_unlock(&pool->lock);
+            qemu_lock_guard_unlock(&pool_guard);
             ret = qemu_sem_timedwait(&pool->sem, 10000);
-            qemu_mutex_lock(&pool->lock);
+            qemu_lock_guard_lock(&pool_guard);
             pool->idle_threads--;
         } while (ret == -1 && !QTAILQ_EMPTY(&pool->request_list));
         if (ret == -1 || pool->stopping) {
@@ -100,7 +100,7 @@ static void *worker_thread(void *opaque)
         req = QTAILQ_FIRST(&pool->request_list);
         QTAILQ_REMOVE(&pool->request_list, req, reqs);
         req->state = THREAD_ACTIVE;
-        qemu_mutex_unlock(&pool->lock);
+        qemu_lock_guard_unlock(&pool_guard);
 
         ret = req->func(req->arg);
 
@@ -109,14 +109,13 @@ static void *worker_thread(void *opaque)
         smp_wmb();
         req->state = THREAD_DONE;
 
-        qemu_mutex_lock(&pool->lock);
+        qemu_lock_guard_lock(&pool_guard);
 
         qemu_bh_schedule(pool->completion_bh);
     }
 
     pool->cur_threads--;
     qemu_cond_signal(&pool->worker_stopped);
-    qemu_mutex_unlock(&pool->lock);
     return NULL;
 }
 
@@ -139,9 +138,8 @@ static void spawn_thread_bh_fn(void *opaque)
 {
     ThreadPool *pool = opaque;
 
-    qemu_mutex_lock(&pool->lock);
+    QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
     do_spawn_thread(pool);
-    qemu_mutex_unlock(&pool->lock);
 }
 
 static void spawn_thread(ThreadPool *pool)
@@ -208,10 +206,10 @@ static void thread_pool_cancel(BlockAIOCB *acb)
 {
     ThreadPoolElement *elem = (ThreadPoolElement *)acb;
     ThreadPool *pool = elem->pool;
+    QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
 
     trace_thread_pool_cancel(elem, elem->common.opaque);
 
-    qemu_mutex_lock(&pool->lock);
     if (elem->state == THREAD_QUEUED &&
         /* No thread has yet started working on elem. we can try to "steal"
          * the item from the worker if we can get a signal from the
@@ -225,8 +223,6 @@ static void thread_pool_cancel(BlockAIOCB *acb)
         elem->state = THREAD_DONE;
         elem->ret = -ECANCELED;
     }
-
-    qemu_mutex_unlock(&pool->lock);
 }
 
 static AioContext *thread_pool_get_aio_context(BlockAIOCB *acb)
@@ -258,12 +254,12 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPool *pool,
 
     trace_thread_pool_submit(pool, req, arg);
 
-    qemu_mutex_lock(&pool->lock);
+    QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
     if (pool->idle_threads == 0 && pool->cur_threads < pool->max_threads) {
         spawn_thread(pool);
     }
     QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
-    qemu_mutex_unlock(&pool->lock);
+    qemu_lock_guard_unlock(&pool_guard);
     qemu_sem_post(&pool->sem);
     return &req->common;
 }
@@ -330,7 +326,7 @@ void thread_pool_free(ThreadPool *pool)
 
     assert(QLIST_EMPTY(&pool->head));
 
-    qemu_mutex_lock(&pool->lock);
+    QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
 
     /* Stop new threads from spawning */
     qemu_bh_delete(pool->new_thread_bh);
@@ -344,7 +340,7 @@ void thread_pool_free(ThreadPool *pool)
         qemu_cond_wait(&pool->worker_stopped, &pool->lock);
     }
 
-    qemu_mutex_unlock(&pool->lock);
+    qemu_lock_guard_unlock(&pool_guard);
 
     qemu_bh_delete(pool->completion_bh);
     qemu_sem_destroy(&pool->sem);
-- 
2.14.3

On Fri, Dec 08, 2017 at 11:55:53AM +0100, Paolo Bonzini wrote:
> @@ -88,9 +88,9 @@ static void *worker_thread(void *opaque)
>  
>          do {
>              pool->idle_threads++;
> -            qemu_mutex_unlock(&pool->lock);
> +            qemu_lock_guard_unlock(&pool_guard);
>              ret = qemu_sem_timedwait(&pool->sem, 10000);
> -            qemu_mutex_lock(&pool->lock);
> +            qemu_lock_guard_lock(&pool_guard);

Or:

  QEMU_WITHOUT_LOCK_GUARD(pool_guard) {
      ret = qemu_sem_timedwait(&pool->sem, 10000);
  }

Seems funny but I like it. :)

Unfortunately it's tricky to come up with good semantics.  A 'return'
statement inside QEMU_WITHOUT_LOCK_GUARD() should leave the lock
unlocked.  But a 'break' statement may need to reacquire the lock...

> @@ -258,12 +254,12 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPool *pool,
>  
>      trace_thread_pool_submit(pool, req, arg);
>  
> -    qemu_mutex_lock(&pool->lock);
> +    QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
>      if (pool->idle_threads == 0 && pool->cur_threads < pool->max_threads) {
>          spawn_thread(pool);
>      }
>      QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
> -    qemu_mutex_unlock(&pool->lock);
> +    qemu_lock_guard_unlock(&pool_guard);

Why not QEMU_WITH_LOCK()?  Then you can get rid of the explicit unlock.

> @@ -330,7 +326,7 @@ void thread_pool_free(ThreadPool *pool)
>  
>      assert(QLIST_EMPTY(&pool->head));
>  
> -    qemu_mutex_lock(&pool->lock);
> +    QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
>  
>      /* Stop new threads from spawning */
>      qemu_bh_delete(pool->new_thread_bh);
> @@ -344,7 +340,7 @@ void thread_pool_free(ThreadPool *pool)
>          qemu_cond_wait(&pool->worker_stopped, &pool->lock);
>      }
>  
> -    qemu_mutex_unlock(&pool->lock);
> +    qemu_lock_guard_unlock(&pool_guard);

Here too.  I don't see the advantage of replacing a single lock/unlock
with QEMU_LOCK_GUARD/unlock, if it cannot be made shorter/safer then
it's fine to use QemuMutex directly.

On 08/12/2017 16:13, Stefan Hajnoczi wrote:
>> -    qemu_mutex_lock(&pool->lock);
>> +    QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
>>      if (pool->idle_threads == 0 && pool->cur_threads < pool->max_threads) {
>>          spawn_thread(pool);
>>      }
>>      QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
>> -    qemu_mutex_unlock(&pool->lock);
>> +    qemu_lock_guard_unlock(&pool_guard);
> Why not QEMU_WITH_LOCK()?  Then you can get rid of the explicit unlock.

I agree that QEMU_WITH_LOCK_GUARD is better in this case. (IIRC I wrote
this patch before coming up with the is_taken trick!).

My main question for the series is what you think the balance should be
between a more widely applicable API and a simpler one.

Thanks,

Paolo

>> @@ -330,7 +326,7 @@ void thread_pool_free(ThreadPool *pool)
>>  
>>      assert(QLIST_EMPTY(&pool->head));
>>  
>> -    qemu_mutex_lock(&pool->lock);
>> +    QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
>>  
>>      /* Stop new threads from spawning */
>>      qemu_bh_delete(pool->new_thread_bh);
>> @@ -344,7 +340,7 @@ void thread_pool_free(ThreadPool *pool)
>>          qemu_cond_wait(&pool->worker_stopped, &pool->lock);
>>      }
>>  
>> -    qemu_mutex_unlock(&pool->lock);
>> +    qemu_lock_guard_unlock(&pool_guard);
> Here too.  I don't see the advantage of replacing a single lock/unlock
> with QEMU_LOCK_GUARD/unlock, if it cannot be made shorter/safer then
> it's fine to use QemuMutex directly.

On 12/08/2017 12:12 PM, Paolo Bonzini wrote:
> On 08/12/2017 16:13, Stefan Hajnoczi wrote:
>>> -    qemu_mutex_lock(&pool->lock);
>>> +    QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
>>>      if (pool->idle_threads == 0 && pool->cur_threads < pool->max_threads) {
>>>          spawn_thread(pool);
>>>      }
>>>      QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
>>> -    qemu_mutex_unlock(&pool->lock);
>>> +    qemu_lock_guard_unlock(&pool_guard);
>> Why not QEMU_WITH_LOCK()?  Then you can get rid of the explicit unlock.
> 
> I agree that QEMU_WITH_LOCK_GUARD is better in this case. (IIRC I wrote
> this patch before coming up with the is_taken trick!).
> 
> My main question for the series is what you think the balance should be
> between a more widely applicable API and a simpler one.

If you require the user to provide the scope, this could be:

@@ -258,12 +254,12 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPool *pool,

     trace_thread_pool_submit(pool, req, arg);

-    qemu_mutex_lock(&pool->lock);
-    if (pool->idle_threads == 0 && pool->cur_threads < pool->max_threads) {
-        spawn_thread(pool);
-    QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
+    {
+        QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
+        if (pool->idle_threads == 0 && pool->cur_threads <
pool->max_threads) {
+            spawn_thread(pool);
+        }
+        QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
     }
-    qemu_mutex_unlock(&pool->lock);
     qemu_sem_post(&pool->sem);
     return &req->common;
 }

In other words, I don't see what 'QEMU_WITH_LOCK_GUARD() {}' buys us
over '{ QEMU_LOCK_GUARD() }'.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Fri, Dec 08, 2017 at 02:02:32PM -0600, Eric Blake wrote:
> On 12/08/2017 12:12 PM, Paolo Bonzini wrote:
> > On 08/12/2017 16:13, Stefan Hajnoczi wrote:
> >>> -    qemu_mutex_lock(&pool->lock);
> >>> +    QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
> >>>      if (pool->idle_threads == 0 && pool->cur_threads < pool->max_threads) {
> >>>          spawn_thread(pool);
> >>>      }
> >>>      QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
> >>> -    qemu_mutex_unlock(&pool->lock);
> >>> +    qemu_lock_guard_unlock(&pool_guard);
> >> Why not QEMU_WITH_LOCK()?  Then you can get rid of the explicit unlock.
> > 
> > I agree that QEMU_WITH_LOCK_GUARD is better in this case. (IIRC I wrote
> > this patch before coming up with the is_taken trick!).
> > 
> > My main question for the series is what you think the balance should be
> > between a more widely applicable API and a simpler one.
> 
> If you require the user to provide the scope, this could be:
> 
> @@ -258,12 +254,12 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPool *pool,
> 
>      trace_thread_pool_submit(pool, req, arg);
> 
> -    qemu_mutex_lock(&pool->lock);
> -    if (pool->idle_threads == 0 && pool->cur_threads < pool->max_threads) {
> -        spawn_thread(pool);
> -    QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
> +    {
> +        QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
> +        if (pool->idle_threads == 0 && pool->cur_threads <
> pool->max_threads) {
> +            spawn_thread(pool);
> +        }
> +        QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
>      }
> -    qemu_mutex_unlock(&pool->lock);
>      qemu_sem_post(&pool->sem);
>      return &req->common;
>  }
> 
> In other words, I don't see what 'QEMU_WITH_LOCK_GUARD() {}' buys us
> over '{ QEMU_LOCK_GUARD() }'.

The QEMU_WITH_LOCK_GUARD() {} syntax is nice because it's similar to
if/while/for statements.

However, { QEMU_LOCK_GUARD() } doesn't hide a for statement in a macro
so the break statement works inside the scope.  Less chance of bugs.

I'd be okay without QEMU_WITH_LOCK_GUARD().

Stefan

On 11/12/2017 11:23, Stefan Hajnoczi wrote:
>>
>> In other words, I don't see what 'QEMU_WITH_LOCK_GUARD() {}' buys us
>> over '{ QEMU_LOCK_GUARD() }'.
> The QEMU_WITH_LOCK_GUARD() {} syntax is nice because it's similar to
> if/while/for statements.
> 
> However, { QEMU_LOCK_GUARD() } doesn't hide a for statement in a macro
> so the break statement works inside the scope.  Less chance of bugs.

The same is true of a "switch" statement.  Being able to break out of
QEMU_WITH_LOCK_GUARD could also be a feature...

Paolo


> I'd be okay without QEMU_WITH_LOCK_GUARD().

On 12/08/2017 09:13 AM, Stefan Hajnoczi wrote:
> On Fri, Dec 08, 2017 at 11:55:53AM +0100, Paolo Bonzini wrote:
>> @@ -88,9 +88,9 @@ static void *worker_thread(void *opaque)
>>  
>>          do {
>>              pool->idle_threads++;
>> -            qemu_mutex_unlock(&pool->lock);
>> +            qemu_lock_guard_unlock(&pool_guard);
>>              ret = qemu_sem_timedwait(&pool->sem, 10000);
>> -            qemu_mutex_lock(&pool->lock);
>> +            qemu_lock_guard_lock(&pool_guard);
> 
> Or:
> 
>   QEMU_WITHOUT_LOCK_GUARD(pool_guard) {
>       ret = qemu_sem_timedwait(&pool->sem, 10000);
>   }
> 
> Seems funny but I like it. :)
> 
> Unfortunately it's tricky to come up with good semantics.  A 'return'
> statement inside QEMU_WITHOUT_LOCK_GUARD() should leave the lock
> unlocked.  But a 'break' statement may need to reacquire the lock...

But isn't that what happens already?

QEMU_WITH_LOCK_GUARD(guard) { // 1
  do {
    QEMU_WITHOUT_LOCK_GUARD(guard) {// 2
      if (cond1) {
        break;
      } else if (cond2) {
        goto endlock;
      } else {
        return;
      }
    } // 3
  } while (cond3);
} // 4
endlock:
  ; // 5

Point 2 introduces a new scope which says to unlock the guard at entry,
and to relock the guard at all exits from the scope.  If cond1 is true,
and we break, control flows to point 3, (because of we provided the
scope via a for loop - so the break exits only the lock guard!), and
relocks the guard, so the bulk of the do/while loop that is not
sub-scoped by the WITHOUT_LOCK_GUARD remains locked.  If cond2 is true,
then we leave the WITHOUT_LOCK_GUARD scope (and temporarily lock the
guard), then immediately leave the WITH_LOCK_GUARD scope (and unlock the
guard again) - the cleanup handlers ensure that unwinding out of
multiple scopes performs all of the proper cleanups, as we skip to point
5.  The same is true if cond2 is false and we return from the function -
the lock is temporarily restored then released.

Or did you want semantics where you can return with the lock still
locked in spite of the outer loop, and/or an optimization to avoid the
contention on temporarily re-locking/unlocking a loop when jumping out
of two nested scopes?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Fri, Dec 08, 2017 at 03:13:06PM +0000, Stefan Hajnoczi wrote:

[...]

> > @@ -330,7 +326,7 @@ void thread_pool_free(ThreadPool *pool)
> >  
> >      assert(QLIST_EMPTY(&pool->head));
> >  
> > -    qemu_mutex_lock(&pool->lock);
> > +    QEMU_LOCK_GUARD(QemuMutex, pool_guard, &pool->lock);
> >  
> >      /* Stop new threads from spawning */
> >      qemu_bh_delete(pool->new_thread_bh);
> > @@ -344,7 +340,7 @@ void thread_pool_free(ThreadPool *pool)
> >          qemu_cond_wait(&pool->worker_stopped, &pool->lock);
> >      }
> >  
> > -    qemu_mutex_unlock(&pool->lock);
> > +    qemu_lock_guard_unlock(&pool_guard);
> 
> Here too.  I don't see the advantage of replacing a single lock/unlock
> with QEMU_LOCK_GUARD/unlock, if it cannot be made shorter/safer then
> it's fine to use QemuMutex directly.

Agree.

For such use cases (and maybe mostly the cases that can use
QEMU_ADOPT_LOCK, QEMU_RETURN_LOCK, QEMU_TAKEN_LOCK) for me I would
still prefer the old ways, which is clearer to me (and faster?).  But
I do think the guard can really help for the specific case when
e.g. we need to take the lock at the entry of the function but need to
make sure it's released before leaving, especially when the function
contains multiple return points.

Maybe we can do this incrementally?  Say, we can at least start to use
it in new codes, and replace some obvious scenarios where proper.
After all it sounds good to have such an API that QEMU code can use
directly.

Thanks,

-- 
Peter Xu