Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
---
 src/util/virthreadpool.c | 39 +++++++++++++--------------------------
 1 file changed, 13 insertions(+), 26 deletions(-)

diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
index 426840e435..b6d154802a 100644
--- a/src/util/virthreadpool.c
+++ b/src/util/virthreadpool.c
@@ -306,15 +306,13 @@ void virThreadPoolFree(virThreadPool *pool)
     if (!pool)
         return;
 
-    virMutexLock(&pool->mutex);
-    virThreadPoolDrainLocked(pool);
+    virThreadPoolDrain(pool);
 
     if (pool->identity)
         g_object_unref(pool->identity);
 
     g_free(pool->jobName);
     g_free(pool->workers);
-    virMutexUnlock(&pool->mutex);
     virMutexDestroy(&pool->mutex);
     virCondDestroy(&pool->quit_cond);
     virCondDestroy(&pool->cond);
@@ -326,66 +324,60 @@ void virThreadPoolFree(virThreadPool *pool)
 
 size_t virThreadPoolGetMinWorkers(virThreadPool *pool)
 {
+    VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex);
     size_t ret;
 
-    virMutexLock(&pool->mutex);
     ret = pool->minWorkers;
-    virMutexUnlock(&pool->mutex);
 
     return ret;
 }
 
 size_t virThreadPoolGetMaxWorkers(virThreadPool *pool)
 {
+    VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex);
     size_t ret;
 
-    virMutexLock(&pool->mutex);
     ret = pool->maxWorkers;
-    virMutexUnlock(&pool->mutex);
 
     return ret;
 }
 
 size_t virThreadPoolGetPriorityWorkers(virThreadPool *pool)
 {
+    VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex);
     size_t ret;
 
-    virMutexLock(&pool->mutex);
     ret = pool->nPrioWorkers;
-    virMutexUnlock(&pool->mutex);
 
     return ret;
 }
 
 size_t virThreadPoolGetCurrentWorkers(virThreadPool *pool)
 {
+    VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex);
     size_t ret;
 
-    virMutexLock(&pool->mutex);
     ret = pool->nWorkers;
-    virMutexUnlock(&pool->mutex);
 
     return ret;
 }
 
 size_t virThreadPoolGetFreeWorkers(virThreadPool *pool)
 {
+    VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex);
     size_t ret;
 
-    virMutexLock(&pool->mutex);
     ret = pool->freeWorkers;
-    virMutexUnlock(&pool->mutex);
 
     return ret;
 }
 
 size_t virThreadPoolGetJobQueueDepth(virThreadPool *pool)
 {
+    VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex);
     size_t ret;
 
-    virMutexLock(&pool->mutex);
     ret = pool->jobQueueDepth;
-    virMutexUnlock(&pool->mutex);
 
     return ret;
 }
@@ -398,9 +390,9 @@ int virThreadPoolSendJob(virThreadPool *pool,
                          unsigned int priority,
                          void *jobData)
 {
+    VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex);
     virThreadPoolJob *job;
 
-    virMutexLock(&pool->mutex);
     if (pool->quit)
         goto error;
 
@@ -431,11 +423,9 @@ int virThreadPoolSendJob(virThreadPool *pool,
     if (priority)
         virCondSignal(&pool->prioCond);
 
-    virMutexUnlock(&pool->mutex);
     return 0;
 
  error:
-    virMutexUnlock(&pool->mutex);
     return -1;
 }
 
@@ -445,11 +435,10 @@ virThreadPoolSetParameters(virThreadPool *pool,
                            long long int maxWorkers,
                            long long int prioWorkers)
 {
+    VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex);
     size_t max;
     size_t min;
 
-    virMutexLock(&pool->mutex);
-
     max = maxWorkers >= 0 ? maxWorkers : pool->maxWorkers;
     min = minWorkers >= 0 ? minWorkers : pool->minWorkers;
     if (min > max) {
@@ -490,26 +479,24 @@ virThreadPoolSetParameters(virThreadPool *pool,
         pool->maxPrioWorkers = prioWorkers;
     }
 
-    virMutexUnlock(&pool->mutex);
     return 0;
 
  error:
-    virMutexUnlock(&pool->mutex);
     return -1;
 }
 
 void
 virThreadPoolStop(virThreadPool *pool)
 {
-    virMutexLock(&pool->mutex);
+    VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex);
+
     virThreadPoolStopLocked(pool);
-    virMutexUnlock(&pool->mutex);
 }
 
 void
 virThreadPoolDrain(virThreadPool *pool)
 {
-    virMutexLock(&pool->mutex);
+    VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex);
+
     virThreadPoolDrainLocked(pool);
-    virMutexUnlock(&pool->mutex);
 }
-- 
2.31.1

On 2/7/22 14:12, Tim Wiederhake wrote:
> Signed-off-by: Tim Wiederhake <twiederh@redhat.com>

s/memory/lock/ in $SUBJ.

> ---
>  src/util/virthreadpool.c | 39 +++++++++++++--------------------------
>  1 file changed, 13 insertions(+), 26 deletions(-)
> 
> diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
> index 426840e435..b6d154802a 100644
> --- a/src/util/virthreadpool.c
> +++ b/src/util/virthreadpool.c
> @@ -306,15 +306,13 @@ void virThreadPoolFree(virThreadPool *pool)
>      if (!pool)
>          return;
>  
> -    virMutexLock(&pool->mutex);

Look at this made me realize we need to check pool->quit in
virThreadPoolSetParameters(). I was trying to come up with a path where
removing this lock acquire would create a problem (because removing a
mutex makes all the control lights flash for me), but failed. I'm still
trying to convince myself this is safe thing to do.

On the other hand, if we used automatic mutex management, then ...

> -    virThreadPoolDrainLocked(pool);
> +    virThreadPoolDrain(pool);
>  
>      if (pool->identity)
>          g_object_unref(pool->identity);
>  
>      g_free(pool->jobName);
>      g_free(pool->workers);
> -    virMutexUnlock(&pool->mutex);
>      virMutexDestroy(&pool->mutex);

.. this would destroy a locked mutex. So I guess your code is right.

>      virCondDestroy(&pool->quit_cond);
>      virCondDestroy(&pool->cond);

Michal