Re: [Qemu-devel] [PATCH 5/5] thread-pool: convert to use lock guards

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]