[Qemu-devel] [PATCH 0/6] Spread the use of QEMU threading & locking API

[Qemu-devel] [PATCH 0/6] Spread the use of QEMU threading & locking API

[Jan Kiszka]

This adds two missing features to our QEMU threading and locking
abstractions, qemu_thread_join and qemu_cond_timedwait, and then
converts compat AIO, compatfd, and several audio subsystems. This not
only saves a few lines of code, it also allows to apply certain thread
and lock parameters centrally which is specifically important when
using SCHED_FIFO.

Note that patch 6 is untested. Am I right that the other threads
coreaudioVoiceOut::mutex addresses are created by the coreaudio core?
Just curious.

Furthermore note that the changes to posix-aio-compat.c depend on
patches in Kevin's latest pull request.

CC: Andreas Färber <andreas.faerber@web.de>
CC: Kevin Wolf <kwolf@redhat.com>
CC: malc <av1474@comtv.ru>

Jan Kiszka (6):
  Enable joinable POSIX threads
  Introduce qemu_cond_timedwait
  Switch POSIX compat AIO to QEMU abstractions
  Switch compatfd to QEMU thread
  audio: Use QEMU threads & synchronization
  audio: Switch coreaudio to QemuMutex

 audio/audio_pt_int.c    |  167 ++---------------------------------------------
 audio/audio_pt_int.h    |   45 +++++++++----
 audio/coreaudio.c       |   56 ++--------------
 audio/esdaudio.c        |   92 +++++++-------------------
 audio/paaudio.c         |   84 +++++++----------------
 compatfd.c              |   16 +----
 cpus.c                  |    6 +-
 hw/ccid-card-emulated.c |    5 +-
 posix-aio-compat.c      |  115 +++++++++-----------------------
 qemu-thread-posix.c     |   53 ++++++++++++++-
 qemu-thread-posix.h     |    3 +
 qemu-thread-win32.c     |   20 +++++-
 qemu-thread.h           |    9 ++-
 ui/vnc-jobs-async.c     |    2 +-
 14 files changed, 219 insertions(+), 454 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads

[Jan Kiszka]

Allow to control if a QEMU thread is created joinable or not. Make it
not joinable by default to avoid that we keep the associated resources
around when terminating a thread without joining it (what we couldn't do
so far for obvious reasons).

The audio subsystem will need the join feature when converting it to
QEMU threading/locking abstractions, so provide that service. Join will
only be provided for POSIX as there is no setup in sight yet where
Windows would need it as well (+implementation is non-trivial).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpus.c                  |    6 ++++--
 hw/ccid-card-emulated.c |    5 +++--
 qemu-thread-posix.c     |   31 +++++++++++++++++++++++++++----
 qemu-thread-posix.h     |    3 +++
 qemu-thread-win32.c     |    4 +++-
 qemu-thread.h           |    7 +++++--
 ui/vnc-jobs-async.c     |    2 +-
 7 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/cpus.c b/cpus.c
index d0cfe91..3f7724a 100644
--- a/cpus.c
+++ b/cpus.c
@@ -822,7 +822,8 @@ static void qemu_tcg_init_vcpu(void *_env)
         env->halt_cond = g_malloc0(sizeof(QemuCond));
         qemu_cond_init(env->halt_cond);
         tcg_halt_cond = env->halt_cond;
-        qemu_thread_create(env->thread, qemu_tcg_cpu_thread_fn, env);
+        qemu_thread_create(env->thread, qemu_tcg_cpu_thread_fn, env,
+                           QEMU_THREAD_DETACHED);
         while (env->created == 0) {
             qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
         }
@@ -838,7 +839,8 @@ static void qemu_kvm_start_vcpu(CPUState *env)
     env->thread = g_malloc0(sizeof(QemuThread));
     env->halt_cond = g_malloc0(sizeof(QemuCond));
     qemu_cond_init(env->halt_cond);
-    qemu_thread_create(env->thread, qemu_kvm_cpu_thread_fn, env);
+    qemu_thread_create(env->thread, qemu_kvm_cpu_thread_fn, env,
+                       QEMU_THREAD_DETACHED);
     while (env->created == 0) {
         qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
     }
diff --git a/hw/ccid-card-emulated.c b/hw/ccid-card-emulated.c
index 092301b..9fe9db5 100644
--- a/hw/ccid-card-emulated.c
+++ b/hw/ccid-card-emulated.c
@@ -541,8 +541,9 @@ static int emulated_initfn(CCIDCardState *base)
         printf("%s: failed to initialize vcard\n", EMULATED_DEV_NAME);
         return -1;
     }
-    qemu_thread_create(&thread_id, event_thread, card);
-    qemu_thread_create(&thread_id, handle_apdu_thread, card);
+    qemu_thread_create(&thread_id, event_thread, card, QEMU_THREAD_DETACHED);
+    qemu_thread_create(&thread_id, handle_apdu_thread, card,
+                       QEMU_THREAD_DETACHED);
     return 0;
 }
 
diff --git a/qemu-thread-posix.c b/qemu-thread-posix.c
index 2bd02ef..f76427e 100644
--- a/qemu-thread-posix.c
+++ b/qemu-thread-posix.c
@@ -117,20 +117,33 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
 
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void*),
-                       void *arg)
+                       void *arg, int mode)
 {
+    sigset_t set, oldset;
+    pthread_attr_t attr;
     int err;
 
-    /* Leave signal handling to the iothread.  */
-    sigset_t set, oldset;
+    err = pthread_attr_init(&attr);
+    if (err) {
+        error_exit(err, __func__);
+    }
+    if (mode == QEMU_THREAD_DETACHED) {
+        err = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
+        if (err) {
+            error_exit(err, __func__);
+        }
+    }
 
+    /* Leave signal handling to the iothread.  */
     sigfillset(&set);
     pthread_sigmask(SIG_SETMASK, &set, &oldset);
-    err = pthread_create(&thread->thread, NULL, start_routine, arg);
+    err = pthread_create(&thread->thread, &attr, start_routine, arg);
     if (err)
         error_exit(err, __func__);
 
     pthread_sigmask(SIG_SETMASK, &oldset, NULL);
+
+    pthread_attr_destroy(&attr);
 }
 
 void qemu_thread_get_self(QemuThread *thread)
@@ -147,3 +160,13 @@ void qemu_thread_exit(void *retval)
 {
     pthread_exit(retval);
 }
+
+void qemu_thread_join(QemuThread *thread)
+{
+    int err;
+
+    err = pthread_join(thread->thread, NULL);
+    if (err) {
+        error_exit(err, __func__);
+    }
+}
diff --git a/qemu-thread-posix.h b/qemu-thread-posix.h
index ee4618e..540fa0b 100644
--- a/qemu-thread-posix.h
+++ b/qemu-thread-posix.h
@@ -14,4 +14,7 @@ struct QemuThread {
     pthread_t thread;
 };
 
+/* only provided for posix so far */
+void qemu_thread_join(QemuThread *thread);
+
 #endif
diff --git a/qemu-thread-win32.c b/qemu-thread-win32.c
index a27332e..4819ff4 100644
--- a/qemu-thread-win32.c
+++ b/qemu-thread-win32.c
@@ -243,10 +243,12 @@ static inline void qemu_thread_init(void)
 
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void *),
-                       void *arg)
+                       void *arg, int mode)
 {
     HANDLE hThread;
 
+    assert(mode == QEMU_THREAD_DETACHED);
+
     struct QemuThreadData *data;
     qemu_thread_init();
     data = g_malloc(sizeof *data);
diff --git a/qemu-thread.h b/qemu-thread.h
index 0a73d50..aa1ae55 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -13,6 +13,9 @@ typedef struct QemuThread QemuThread;
 #include "qemu-thread-posix.h"
 #endif
 
+#define QEMU_THREAD_JOINABLE 0
+#define QEMU_THREAD_DETACHED 1
+
 void qemu_mutex_init(QemuMutex *mutex);
 void qemu_mutex_destroy(QemuMutex *mutex);
 void qemu_mutex_lock(QemuMutex *mutex);
@@ -32,8 +35,8 @@ void qemu_cond_broadcast(QemuCond *cond);
 void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex);
 
 void qemu_thread_create(QemuThread *thread,
-                       void *(*start_routine)(void*),
-                       void *arg);
+                        void *(*start_routine)(void *),
+                        void *arg, int mode);
 void qemu_thread_get_self(QemuThread *thread);
 int qemu_thread_is_self(QemuThread *thread);
 void qemu_thread_exit(void *retval);
diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index de5ea6b..9b3016c 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -318,7 +318,7 @@ void vnc_start_worker_thread(void)
         return ;
 
     q = vnc_queue_init();
-    qemu_thread_create(&q->thread, vnc_worker_thread, q);
+    qemu_thread_create(&q->thread, vnc_worker_thread, q, QEMU_THREAD_DETACHED);
     queue = q; /* Set global queue */
 }
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 2/6] Introduce qemu_cond_timedwait

[Jan Kiszka]

First user will be posix compat aio.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 qemu-thread-posix.c |   22 ++++++++++++++++++++++
 qemu-thread-win32.c |   16 +++++++++++++++-
 qemu-thread.h       |    2 ++
 3 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/qemu-thread-posix.c b/qemu-thread-posix.c
index f76427e..1d970fb 100644
--- a/qemu-thread-posix.c
+++ b/qemu-thread-posix.c
@@ -17,6 +17,7 @@
 #include <signal.h>
 #include <stdint.h>
 #include <string.h>
+#include <sys/time.h>
 #include "qemu-thread.h"
 
 static void error_exit(int err, const char *msg)
@@ -115,6 +116,27 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
         error_exit(err, __func__);
 }
 
+int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex,
+                        unsigned int timeout_ms)
+{
+    struct timespec ts;
+    struct timeval tv;
+    int err;
+
+    gettimeofday(&tv, NULL);
+    ts.tv_sec = tv.tv_sec + timeout_ms / 1000;
+    ts.tv_nsec = tv.tv_usec * 1000 + timeout_ms % 1000;
+    if (ts.tv_nsec > 1000000000) {
+        ts.tv_sec++;
+        ts.tv_nsec -= 1000000000;
+    }
+    err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
+    if (err && err != ETIMEDOUT) {
+        error_exit(err, __func__);
+    }
+    return err == 0;
+}
+
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void*),
                        void *arg, int mode)
diff --git a/qemu-thread-win32.c b/qemu-thread-win32.c
index 4819ff4..1b01a8b 100644
--- a/qemu-thread-win32.c
+++ b/qemu-thread-win32.c
@@ -158,6 +158,14 @@ void qemu_cond_broadcast(QemuCond *cond)
 
 void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
 {
+    qemu_cond_timedwait(cond, mutex, INFINITE);
+}
+
+int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex,
+                        unsigned int timeout_ms)
+{
+    DWORD result;
+
     /*
      * This access is protected under the mutex.
      */
@@ -170,7 +178,11 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
      * leaving mutex unlocked before we wait on semaphore.
      */
     qemu_mutex_unlock(mutex);
-    WaitForSingleObject(cond->sema, INFINITE);
+    result = WaitForSingleObject(cond->sema, timeout_ms);
+
+    if (result != WAIT_OBJECT_0 && result != WAIT_TIMEOUT) {
+        error_exit(GetLastError(), __func__);
+    }
 
     /* Now waiters must rendez-vous with the signaling thread and
      * let it continue.  For cond_broadcast this has heavy contention
@@ -190,6 +202,8 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
     }
 
     qemu_mutex_lock(mutex);
+
+    return result == WAIT_OBJECT_0;
 }
 
 struct QemuThreadData {
diff --git a/qemu-thread.h b/qemu-thread.h
index aa1ae55..8be48e4 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -33,6 +33,8 @@ void qemu_cond_destroy(QemuCond *cond);
 void qemu_cond_signal(QemuCond *cond);
 void qemu_cond_broadcast(QemuCond *cond);
 void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex);
+int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex,
+                        unsigned int timeout_ms);
 
 void qemu_thread_create(QemuThread *thread,
                         void *(*start_routine)(void *),
-- 
1.7.3.4

[Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions

[Jan Kiszka]

Although there is nothing to wrap for non-POSIX here, redirecting thread
and synchronization services to our core simplifies managements jobs
like scheduling parameter adjustment. It also frees compat AIO from some
duplicate code (/wrt qemu-thread).

CC: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 posix-aio-compat.c |  115 ++++++++++++++-------------------------------------
 1 files changed, 32 insertions(+), 83 deletions(-)

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index d3c1174..0715aba 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -13,7 +13,6 @@
 
 #include <sys/ioctl.h>
 #include <sys/types.h>
-#include <pthread.h>
 #include <unistd.h>
 #include <errno.h>
 #include <time.h>
@@ -27,9 +26,12 @@
 #include "qemu-common.h"
 #include "trace.h"
 #include "block_int.h"
+#include "qemu-thread.h"
 
 #include "block/raw-posix-aio.h"
 
+#define AIO_THREAD_IDLE_TIMEOUT 10000 /* 10 s */
+
 static void do_spawn_thread(void);
 
 struct qemu_paiocb {
@@ -57,10 +59,9 @@ typedef struct PosixAioState {
 } PosixAioState;
 
 
-static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
-static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
-static pthread_t thread_id;
-static pthread_attr_t attr;
+static QemuMutex lock;
+static QemuCond cond;
+static QemuThread thread;
 static int max_threads = 64;
 static int cur_threads = 0;
 static int idle_threads = 0;
@@ -86,39 +87,6 @@ static void die(const char *what)
     die2(errno, what);
 }
 
-static void mutex_lock(pthread_mutex_t *mutex)
-{
-    int ret = pthread_mutex_lock(mutex);
-    if (ret) die2(ret, "pthread_mutex_lock");
-}
-
-static void mutex_unlock(pthread_mutex_t *mutex)
-{
-    int ret = pthread_mutex_unlock(mutex);
-    if (ret) die2(ret, "pthread_mutex_unlock");
-}
-
-static int cond_timedwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
-                           struct timespec *ts)
-{
-    int ret = pthread_cond_timedwait(cond, mutex, ts);
-    if (ret && ret != ETIMEDOUT) die2(ret, "pthread_cond_timedwait");
-    return ret;
-}
-
-static void cond_signal(pthread_cond_t *cond)
-{
-    int ret = pthread_cond_signal(cond);
-    if (ret) die2(ret, "pthread_cond_signal");
-}
-
-static void thread_create(pthread_t *thread, pthread_attr_t *attr,
-                          void *(*start_routine)(void*), void *arg)
-{
-    int ret = pthread_create(thread, attr, start_routine, arg);
-    if (ret) die2(ret, "pthread_create");
-}
-
 static ssize_t handle_aiocb_ioctl(struct qemu_paiocb *aiocb)
 {
     int ret;
@@ -311,27 +279,22 @@ static void posix_aio_notify_event(void);
 
 static void *aio_thread(void *unused)
 {
-    mutex_lock(&lock);
+    qemu_mutex_lock(&lock);
     pending_threads--;
-    mutex_unlock(&lock);
+    qemu_mutex_unlock(&lock);
     do_spawn_thread();
 
     while (1) {
         struct qemu_paiocb *aiocb;
-        ssize_t ret = 0;
-        qemu_timeval tv;
-        struct timespec ts;
-
-        qemu_gettimeofday(&tv);
-        ts.tv_sec = tv.tv_sec + 10;
-        ts.tv_nsec = 0;
+        bool timed_out = false;
+        ssize_t ret;
 
-        mutex_lock(&lock);
+        qemu_mutex_lock(&lock);
 
-        while (QTAILQ_EMPTY(&request_list) &&
-               !(ret == ETIMEDOUT)) {
+        while (QTAILQ_EMPTY(&request_list) && !timed_out) {
             idle_threads++;
-            ret = cond_timedwait(&cond, &lock, &ts);
+            timed_out = qemu_cond_timedwait(&cond, &lock,
+                                            AIO_THREAD_IDLE_TIMEOUT) != 0;
             idle_threads--;
         }
 
@@ -341,7 +304,7 @@ static void *aio_thread(void *unused)
         aiocb = QTAILQ_FIRST(&request_list);
         QTAILQ_REMOVE(&request_list, aiocb, node);
         aiocb->active = 1;
-        mutex_unlock(&lock);
+        qemu_mutex_unlock(&lock);
 
         switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
         case QEMU_AIO_READ:
@@ -373,41 +336,33 @@ static void *aio_thread(void *unused)
             break;
         }
 
-        mutex_lock(&lock);
+        qemu_mutex_lock(&lock);
         aiocb->ret = ret;
-        mutex_unlock(&lock);
+        qemu_mutex_unlock(&lock);
 
         posix_aio_notify_event();
     }
 
     cur_threads--;
-    mutex_unlock(&lock);
+    qemu_mutex_unlock(&lock);
 
     return NULL;
 }
 
 static void do_spawn_thread(void)
 {
-    sigset_t set, oldset;
-
-    mutex_lock(&lock);
+    qemu_mutex_lock(&lock);
     if (!new_threads) {
-        mutex_unlock(&lock);
+        qemu_mutex_unlock(&lock);
         return;
     }
 
     new_threads--;
     pending_threads++;
 
-    mutex_unlock(&lock);
+    qemu_mutex_unlock(&lock);
 
-    /* block all signals */
-    if (sigfillset(&set)) die("sigfillset");
-    if (sigprocmask(SIG_SETMASK, &set, &oldset)) die("sigprocmask");
-
-    thread_create(&thread_id, &attr, aio_thread, NULL);
-
-    if (sigprocmask(SIG_SETMASK, &oldset, NULL)) die("sigprocmask restore");
+    qemu_thread_create(&thread, aio_thread, NULL, QEMU_THREAD_DETACHED);
 }
 
 static void spawn_thread_bh_fn(void *opaque)
@@ -435,21 +390,21 @@ static void qemu_paio_submit(struct qemu_paiocb *aiocb)
 {
     aiocb->ret = -EINPROGRESS;
     aiocb->active = 0;
-    mutex_lock(&lock);
+    qemu_mutex_lock(&lock);
     if (idle_threads == 0 && cur_threads < max_threads)
         spawn_thread();
     QTAILQ_INSERT_TAIL(&request_list, aiocb, node);
-    mutex_unlock(&lock);
-    cond_signal(&cond);
+    qemu_mutex_unlock(&lock);
+    qemu_cond_signal(&cond);
 }
 
 static ssize_t qemu_paio_return(struct qemu_paiocb *aiocb)
 {
     ssize_t ret;
 
-    mutex_lock(&lock);
+    qemu_mutex_lock(&lock);
     ret = aiocb->ret;
-    mutex_unlock(&lock);
+    qemu_mutex_unlock(&lock);
 
     return ret;
 }
@@ -580,14 +535,14 @@ static void paio_cancel(BlockDriverAIOCB *blockacb)
 
     trace_paio_cancel(acb, acb->common.opaque);
 
-    mutex_lock(&lock);
+    qemu_mutex_lock(&lock);
     if (!acb->active) {
         QTAILQ_REMOVE(&request_list, acb, node);
         acb->ret = -ECANCELED;
     } else if (acb->ret == -EINPROGRESS) {
         active = 1;
     }
-    mutex_unlock(&lock);
+    qemu_mutex_unlock(&lock);
 
     if (active) {
         /* fail safe: if the aio could not be canceled, we wait for
@@ -657,11 +612,13 @@ int paio_init(void)
 {
     PosixAioState *s;
     int fds[2];
-    int ret;
 
     if (posix_aio_state)
         return 0;
 
+    qemu_mutex_init(&lock);
+    qemu_cond_init(&cond);
+
     s = g_malloc(sizeof(PosixAioState));
 
     s->first_aio = NULL;
@@ -679,14 +636,6 @@ int paio_init(void)
     qemu_aio_set_fd_handler(s->rfd, posix_aio_read, NULL, posix_aio_flush,
         posix_aio_process_queue, s);
 
-    ret = pthread_attr_init(&attr);
-    if (ret)
-        die2(ret, "pthread_attr_init");
-
-    ret = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
-    if (ret)
-        die2(ret, "pthread_attr_setdetachstate");
-
     QTAILQ_INIT(&request_list);
     new_thread_bh = qemu_bh_new(spawn_thread_bh_fn, NULL);
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 4/6] Switch compatfd to QEMU thread

[Jan Kiszka]

qemu_thread_create already does signal blocking and detaching for us.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 compatfd.c |   16 +++-------------
 1 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/compatfd.c b/compatfd.c
index 31654c6..5431e4b 100644
--- a/compatfd.c
+++ b/compatfd.c
@@ -12,10 +12,10 @@
  */
 
 #include "qemu-common.h"
+#include "qemu-thread.h"
 #include "compatfd.h"
 
 #include <sys/syscall.h>
-#include <pthread.h>
 
 struct sigfd_compat_info
 {
@@ -26,10 +26,6 @@ struct sigfd_compat_info
 static void *sigwait_compat(void *opaque)
 {
     struct sigfd_compat_info *info = opaque;
-    sigset_t all;
-
-    sigfillset(&all);
-    pthread_sigmask(SIG_BLOCK, &all, NULL);
 
     while (1) {
         int sig;
@@ -69,9 +65,8 @@ static void *sigwait_compat(void *opaque)
 
 static int qemu_signalfd_compat(const sigset_t *mask)
 {
-    pthread_attr_t attr;
-    pthread_t tid;
     struct sigfd_compat_info *info;
+    QemuThread thread;
     int fds[2];
 
     info = malloc(sizeof(*info));
@@ -91,12 +86,7 @@ static int qemu_signalfd_compat(const sigset_t *mask)
     memcpy(&info->mask, mask, sizeof(*mask));
     info->fd = fds[1];
 
-    pthread_attr_init(&attr);
-    pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
-
-    pthread_create(&tid, &attr, sigwait_compat, info);
-
-    pthread_attr_destroy(&attr);
+    qemu_thread_create(&thread, sigwait_compat, info, QEMU_THREAD_DETACHED);
 
     return fds[0];
 }
-- 
1.7.3.4

[Qemu-devel] [PATCH 5/6] audio: Use QEMU threads & synchronization

[Jan Kiszka]

This both simplifies error handling and enables central management of
threads and locks.

CC: malc <av1474@comtv.ru>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 audio/audio_pt_int.c |  167 ++-----------------------------------------------
 audio/audio_pt_int.h |   45 ++++++++++----
 audio/esdaudio.c     |   92 ++++++++--------------------
 audio/paaudio.c      |   84 ++++++++-----------------
 4 files changed, 91 insertions(+), 297 deletions(-)

diff --git a/audio/audio_pt_int.c b/audio/audio_pt_int.c
index 9a9c306..d47bd63 100644
--- a/audio/audio_pt_int.c
+++ b/audio/audio_pt_int.c
@@ -6,168 +6,15 @@
 #include "audio_int.h"
 #include "audio_pt_int.h"
 
-static void GCC_FMT_ATTR(3, 4) logerr (struct audio_pt *pt, int err,
-                                       const char *fmt, ...)
+void audio_pt_init (struct audio_pt *p, void *(*func) (void *), void *opaque)
 {
-    va_list ap;
-
-    va_start (ap, fmt);
-    AUD_vlog (pt->drv, fmt, ap);
-    va_end (ap);
-
-    AUD_log (NULL, "\n");
-    AUD_log (pt->drv, "Reason: %s\n", strerror (err));
-}
-
-int audio_pt_init (struct audio_pt *p, void *(*func) (void *),
-                   void *opaque, const char *drv, const char *cap)
-{
-    int err, err2;
-    const char *efunc;
-    sigset_t set, old_set;
-
-    p->drv = drv;
-
-    err = sigfillset (&set);
-    if (err) {
-        logerr (p, errno, "%s(%s): sigfillset failed", cap, AUDIO_FUNC);
-        return -1;
-    }
-
-    err = pthread_mutex_init (&p->mutex, NULL);
-    if (err) {
-        efunc = "pthread_mutex_init";
-        goto err0;
-    }
-
-    err = pthread_cond_init (&p->cond, NULL);
-    if (err) {
-        efunc = "pthread_cond_init";
-        goto err1;
-    }
-
-    err = pthread_sigmask (SIG_BLOCK, &set, &old_set);
-    if (err) {
-        efunc = "pthread_sigmask";
-        goto err2;
-    }
-
-    err = pthread_create (&p->thread, NULL, func, opaque);
-
-    err2 = pthread_sigmask (SIG_SETMASK, &old_set, NULL);
-    if (err2) {
-        logerr (p, err2, "%s(%s): pthread_sigmask (restore) failed",
-                cap, AUDIO_FUNC);
-        /* We have failed to restore original signal mask, all bets are off,
-           so terminate the process */
-        exit (EXIT_FAILURE);
-    }
-
-    if (err) {
-        efunc = "pthread_create";
-        goto err2;
-    }
-
-    return 0;
-
- err2:
-    err2 = pthread_cond_destroy (&p->cond);
-    if (err2) {
-        logerr (p, err2, "%s(%s): pthread_cond_destroy failed", cap, AUDIO_FUNC);
-    }
-
- err1:
-    err2 = pthread_mutex_destroy (&p->mutex);
-    if (err2) {
-        logerr (p, err2, "%s(%s): pthread_mutex_destroy failed", cap, AUDIO_FUNC);
-    }
-
- err0:
-    logerr (p, err, "%s(%s): %s failed", cap, AUDIO_FUNC, efunc);
-    return -1;
+    qemu_mutex_init(&p->mutex);
+    qemu_cond_init(&p->cond);
+    qemu_thread_create(&p->thread, func, opaque, QEMU_THREAD_JOINABLE);
 }
 
-int audio_pt_fini (struct audio_pt *p, const char *cap)
+void audio_pt_fini (struct audio_pt *p)
 {
-    int err, ret = 0;
-
-    err = pthread_cond_destroy (&p->cond);
-    if (err) {
-        logerr (p, err, "%s(%s): pthread_cond_destroy failed", cap, AUDIO_FUNC);
-        ret = -1;
-    }
-
-    err = pthread_mutex_destroy (&p->mutex);
-    if (err) {
-        logerr (p, err, "%s(%s): pthread_mutex_destroy failed", cap, AUDIO_FUNC);
-        ret = -1;
-    }
-    return ret;
-}
-
-int audio_pt_lock (struct audio_pt *p, const char *cap)
-{
-    int err;
-
-    err = pthread_mutex_lock (&p->mutex);
-    if (err) {
-        logerr (p, err, "%s(%s): pthread_mutex_lock failed", cap, AUDIO_FUNC);
-        return -1;
-    }
-    return 0;
-}
-
-int audio_pt_unlock (struct audio_pt *p, const char *cap)
-{
-    int err;
-
-    err = pthread_mutex_unlock (&p->mutex);
-    if (err) {
-        logerr (p, err, "%s(%s): pthread_mutex_unlock failed", cap, AUDIO_FUNC);
-        return -1;
-    }
-    return 0;
-}
-
-int audio_pt_wait (struct audio_pt *p, const char *cap)
-{
-    int err;
-
-    err = pthread_cond_wait (&p->cond, &p->mutex);
-    if (err) {
-        logerr (p, err, "%s(%s): pthread_cond_wait failed", cap, AUDIO_FUNC);
-        return -1;
-    }
-    return 0;
-}
-
-int audio_pt_unlock_and_signal (struct audio_pt *p, const char *cap)
-{
-    int err;
-
-    err = pthread_mutex_unlock (&p->mutex);
-    if (err) {
-        logerr (p, err, "%s(%s): pthread_mutex_unlock failed", cap, AUDIO_FUNC);
-        return -1;
-    }
-    err = pthread_cond_signal (&p->cond);
-    if (err) {
-        logerr (p, err, "%s(%s): pthread_cond_signal failed", cap, AUDIO_FUNC);
-        return -1;
-    }
-    return 0;
-}
-
-int audio_pt_join (struct audio_pt *p, void **arg, const char *cap)
-{
-    int err;
-    void *ret;
-
-    err = pthread_join (p->thread, &ret);
-    if (err) {
-        logerr (p, err, "%s(%s): pthread_join failed", cap, AUDIO_FUNC);
-        return -1;
-    }
-    *arg = ret;
-    return 0;
+    qemu_cond_destroy(&p->cond);
+    qemu_mutex_destroy(&p->mutex);
 }
diff --git a/audio/audio_pt_int.h b/audio/audio_pt_int.h
index 0dfff76..e395f7d 100644
--- a/audio/audio_pt_int.h
+++ b/audio/audio_pt_int.h
@@ -1,22 +1,41 @@
 #ifndef QEMU_AUDIO_PT_INT_H
 #define QEMU_AUDIO_PT_INT_H
 
-#include <pthread.h>
+#include "qemu-thread.h"
 
 struct audio_pt {
-    const char *drv;
-    pthread_t thread;
-    pthread_cond_t cond;
-    pthread_mutex_t mutex;
+    QemuThread thread;
+    QemuCond cond;
+    QemuMutex mutex;
 };
 
-int audio_pt_init (struct audio_pt *, void *(*) (void *), void *,
-                   const char *, const char *);
-int audio_pt_fini (struct audio_pt *, const char *);
-int audio_pt_lock (struct audio_pt *, const char *);
-int audio_pt_unlock (struct audio_pt *, const char *);
-int audio_pt_wait (struct audio_pt *, const char *);
-int audio_pt_unlock_and_signal (struct audio_pt *, const char *);
-int audio_pt_join (struct audio_pt *, void **, const char *);
+void audio_pt_init (struct audio_pt *, void *(*) (void *), void *);
+void audio_pt_fini (struct audio_pt *);
+
+static inline void audio_pt_lock (struct audio_pt *p)
+{
+    qemu_mutex_lock(&p->mutex);
+}
+
+static inline void audio_pt_unlock (struct audio_pt *p)
+{
+    qemu_mutex_unlock(&p->mutex);
+}
+
+static inline void audio_pt_wait (struct audio_pt *p)
+{
+    qemu_cond_wait(&p->cond, &p->mutex);
+}
+
+static inline void audio_pt_unlock_and_signal (struct audio_pt *p)
+{
+    qemu_mutex_unlock(&p->mutex);
+    qemu_cond_signal(&p->cond);
+}
+
+static inline void audio_pt_join (struct audio_pt *p)
+{
+    qemu_thread_join(&p->thread);
+}
 
 #endif /* audio_pt_int.h */
diff --git a/audio/esdaudio.c b/audio/esdaudio.c
index bd6e1cc..fd1b25d 100644
--- a/audio/esdaudio.c
+++ b/audio/esdaudio.c
@@ -81,9 +81,7 @@ static void *qesd_thread_out (void *arg)
 
     threshold = conf.divisor ? hw->samples / conf.divisor : 0;
 
-    if (audio_pt_lock (&esd->pt, AUDIO_FUNC)) {
-        return NULL;
-    }
+    audio_pt_lock (&esd->pt);
 
     for (;;) {
         int decr, to_mix, rpos;
@@ -97,17 +95,13 @@ static void *qesd_thread_out (void *arg)
                 break;
             }
 
-            if (audio_pt_wait (&esd->pt, AUDIO_FUNC)) {
-                goto exit;
-            }
+            audio_pt_wait (&esd->pt);
         }
 
         decr = to_mix = esd->live;
         rpos = hw->rpos;
 
-        if (audio_pt_unlock (&esd->pt, AUDIO_FUNC)) {
-            return NULL;
-        }
+        audio_pt_unlock (&esd->pt);
 
         while (to_mix) {
             ssize_t written;
@@ -143,9 +137,7 @@ static void *qesd_thread_out (void *arg)
             to_mix -= chunk;
         }
 
-        if (audio_pt_lock (&esd->pt, AUDIO_FUNC)) {
-            return NULL;
-        }
+        audio_pt_lock (&esd->pt);
 
         esd->rpos = rpos;
         esd->live -= decr;
@@ -153,7 +145,7 @@ static void *qesd_thread_out (void *arg)
     }
 
  exit:
-    audio_pt_unlock (&esd->pt, AUDIO_FUNC);
+    audio_pt_unlock (&esd->pt);
     return NULL;
 }
 
@@ -162,19 +154,17 @@ static int qesd_run_out (HWVoiceOut *hw, int live)
     int decr;
     ESDVoiceOut *esd = (ESDVoiceOut *) hw;
 
-    if (audio_pt_lock (&esd->pt, AUDIO_FUNC)) {
-        return 0;
-    }
+    audio_pt_lock (&esd->pt);
 
     decr = audio_MIN (live, esd->decr);
     esd->decr -= decr;
     esd->live = live - decr;
     hw->rpos = esd->rpos;
     if (esd->live > 0) {
-        audio_pt_unlock_and_signal (&esd->pt, AUDIO_FUNC);
+        audio_pt_unlock_and_signal (&esd->pt);
     }
     else {
-        audio_pt_unlock (&esd->pt, AUDIO_FUNC);
+        audio_pt_unlock (&esd->pt);
     }
     return decr;
 }
@@ -232,19 +222,10 @@ static int qesd_init_out (HWVoiceOut *hw, struct audsettings *as)
         goto fail1;
     }
 
-    if (audio_pt_init (&esd->pt, qesd_thread_out, esd, AUDIO_CAP, AUDIO_FUNC)) {
-        goto fail2;
-    }
+    audio_pt_init (&esd->pt, qesd_thread_out, esd);
 
     return 0;
 
- fail2:
-    if (close (esd->fd)) {
-        qesd_logerr (errno, "%s: close on esd socket(%d) failed\n",
-                     AUDIO_FUNC, esd->fd);
-    }
-    esd->fd = -1;
-
  fail1:
     g_free (esd->pcm_buf);
     esd->pcm_buf = NULL;
@@ -253,13 +234,12 @@ static int qesd_init_out (HWVoiceOut *hw, struct audsettings *as)
 
 static void qesd_fini_out (HWVoiceOut *hw)
 {
-    void *ret;
     ESDVoiceOut *esd = (ESDVoiceOut *) hw;
 
-    audio_pt_lock (&esd->pt, AUDIO_FUNC);
+    audio_pt_lock (&esd->pt);
     esd->done = 1;
-    audio_pt_unlock_and_signal (&esd->pt, AUDIO_FUNC);
-    audio_pt_join (&esd->pt, &ret, AUDIO_FUNC);
+    audio_pt_unlock_and_signal (&esd->pt);
+    audio_pt_join (&esd->pt);
 
     if (esd->fd >= 0) {
         if (close (esd->fd)) {
@@ -268,7 +248,7 @@ static void qesd_fini_out (HWVoiceOut *hw)
         esd->fd = -1;
     }
 
-    audio_pt_fini (&esd->pt, AUDIO_FUNC);
+    audio_pt_fini (&esd->pt);
 
     g_free (esd->pcm_buf);
     esd->pcm_buf = NULL;
@@ -290,9 +270,7 @@ static void *qesd_thread_in (void *arg)
 
     threshold = conf.divisor ? hw->samples / conf.divisor : 0;
 
-    if (audio_pt_lock (&esd->pt, AUDIO_FUNC)) {
-        return NULL;
-    }
+    audio_pt_lock (&esd->pt);
 
     for (;;) {
         int incr, to_grab, wpos;
@@ -306,17 +284,13 @@ static void *qesd_thread_in (void *arg)
                 break;
             }
 
-            if (audio_pt_wait (&esd->pt, AUDIO_FUNC)) {
-                goto exit;
-            }
+            audio_pt_wait (&esd->pt);
         }
 
         incr = to_grab = esd->dead;
         wpos = hw->wpos;
 
-        if (audio_pt_unlock (&esd->pt, AUDIO_FUNC)) {
-            return NULL;
-        }
+        audio_pt_unlock (&esd->pt);
 
         while (to_grab) {
             ssize_t nread;
@@ -351,9 +325,7 @@ static void *qesd_thread_in (void *arg)
             to_grab -= chunk;
         }
 
-        if (audio_pt_lock (&esd->pt, AUDIO_FUNC)) {
-            return NULL;
-        }
+        audio_pt_lock (&esd->pt);
 
         esd->wpos = wpos;
         esd->dead -= incr;
@@ -361,7 +333,7 @@ static void *qesd_thread_in (void *arg)
     }
 
  exit:
-    audio_pt_unlock (&esd->pt, AUDIO_FUNC);
+    audio_pt_unlock (&esd->pt);
     return NULL;
 }
 
@@ -370,9 +342,7 @@ static int qesd_run_in (HWVoiceIn *hw)
     int live, incr, dead;
     ESDVoiceIn *esd = (ESDVoiceIn *) hw;
 
-    if (audio_pt_lock (&esd->pt, AUDIO_FUNC)) {
-        return 0;
-    }
+    audio_pt_lock (&esd->pt);
 
     live = audio_pcm_hw_get_live_in (hw);
     dead = hw->samples - live;
@@ -381,10 +351,10 @@ static int qesd_run_in (HWVoiceIn *hw)
     esd->dead = dead - incr;
     hw->wpos = esd->wpos;
     if (esd->dead > 0) {
-        audio_pt_unlock_and_signal (&esd->pt, AUDIO_FUNC);
+        audio_pt_unlock_and_signal (&esd->pt);
     }
     else {
-        audio_pt_unlock (&esd->pt, AUDIO_FUNC);
+        audio_pt_unlock (&esd->pt);
     }
     return incr;
 }
@@ -439,19 +409,10 @@ static int qesd_init_in (HWVoiceIn *hw, struct audsettings *as)
         goto fail1;
     }
 
-    if (audio_pt_init (&esd->pt, qesd_thread_in, esd, AUDIO_CAP, AUDIO_FUNC)) {
-        goto fail2;
-    }
+    audio_pt_init (&esd->pt, qesd_thread_in, esd);
 
     return 0;
 
- fail2:
-    if (close (esd->fd)) {
-        qesd_logerr (errno, "%s: close on esd socket(%d) failed\n",
-                     AUDIO_FUNC, esd->fd);
-    }
-    esd->fd = -1;
-
  fail1:
     g_free (esd->pcm_buf);
     esd->pcm_buf = NULL;
@@ -460,13 +421,12 @@ static int qesd_init_in (HWVoiceIn *hw, struct audsettings *as)
 
 static void qesd_fini_in (HWVoiceIn *hw)
 {
-    void *ret;
     ESDVoiceIn *esd = (ESDVoiceIn *) hw;
 
-    audio_pt_lock (&esd->pt, AUDIO_FUNC);
+    audio_pt_lock (&esd->pt);
     esd->done = 1;
-    audio_pt_unlock_and_signal (&esd->pt, AUDIO_FUNC);
-    audio_pt_join (&esd->pt, &ret, AUDIO_FUNC);
+    audio_pt_unlock_and_signal (&esd->pt);
+    audio_pt_join (&esd->pt);
 
     if (esd->fd >= 0) {
         if (close (esd->fd)) {
@@ -475,7 +435,7 @@ static void qesd_fini_in (HWVoiceIn *hw)
         esd->fd = -1;
     }
 
-    audio_pt_fini (&esd->pt, AUDIO_FUNC);
+    audio_pt_fini (&esd->pt);
 
     g_free (esd->pcm_buf);
     esd->pcm_buf = NULL;
diff --git a/audio/paaudio.c b/audio/paaudio.c
index d1f3912..b539a72 100644
--- a/audio/paaudio.c
+++ b/audio/paaudio.c
@@ -56,9 +56,7 @@ static void *qpa_thread_out (void *arg)
     PAVoiceOut *pa = arg;
     HWVoiceOut *hw = &pa->hw;
 
-    if (audio_pt_lock (&pa->pt, AUDIO_FUNC)) {
-        return NULL;
-    }
+    audio_pt_lock (&pa->pt);
 
     for (;;) {
         int decr, to_mix, rpos;
@@ -72,17 +70,13 @@ static void *qpa_thread_out (void *arg)
                 break;
             }
 
-            if (audio_pt_wait (&pa->pt, AUDIO_FUNC)) {
-                goto exit;
-            }
+            audio_pt_wait (&pa->pt);
         }
 
         decr = to_mix = audio_MIN (pa->live, conf.samples >> 2);
         rpos = pa->rpos;
 
-        if (audio_pt_unlock (&pa->pt, AUDIO_FUNC)) {
-            return NULL;
-        }
+        audio_pt_unlock (&pa->pt);
 
         while (to_mix) {
             int error;
@@ -101,9 +95,7 @@ static void *qpa_thread_out (void *arg)
             to_mix -= chunk;
         }
 
-        if (audio_pt_lock (&pa->pt, AUDIO_FUNC)) {
-            return NULL;
-        }
+        audio_pt_lock (&pa->pt);
 
         pa->rpos = rpos;
         pa->live -= decr;
@@ -111,7 +103,7 @@ static void *qpa_thread_out (void *arg)
     }
 
  exit:
-    audio_pt_unlock (&pa->pt, AUDIO_FUNC);
+    audio_pt_unlock (&pa->pt);
     return NULL;
 }
 
@@ -120,19 +112,17 @@ static int qpa_run_out (HWVoiceOut *hw, int live)
     int decr;
     PAVoiceOut *pa = (PAVoiceOut *) hw;
 
-    if (audio_pt_lock (&pa->pt, AUDIO_FUNC)) {
-        return 0;
-    }
+    audio_pt_lock (&pa->pt);
 
     decr = audio_MIN (live, pa->decr);
     pa->decr -= decr;
     pa->live = live - decr;
     hw->rpos = pa->rpos;
     if (pa->live > 0) {
-        audio_pt_unlock_and_signal (&pa->pt, AUDIO_FUNC);
+        audio_pt_unlock_and_signal (&pa->pt);
     }
     else {
-        audio_pt_unlock (&pa->pt, AUDIO_FUNC);
+        audio_pt_unlock (&pa->pt);
     }
     return decr;
 }
@@ -148,9 +138,7 @@ static void *qpa_thread_in (void *arg)
     PAVoiceIn *pa = arg;
     HWVoiceIn *hw = &pa->hw;
 
-    if (audio_pt_lock (&pa->pt, AUDIO_FUNC)) {
-        return NULL;
-    }
+    audio_pt_lock (&pa->pt);
 
     for (;;) {
         int incr, to_grab, wpos;
@@ -164,17 +152,13 @@ static void *qpa_thread_in (void *arg)
                 break;
             }
 
-            if (audio_pt_wait (&pa->pt, AUDIO_FUNC)) {
-                goto exit;
-            }
+            audio_pt_wait (&pa->pt);
         }
 
         incr = to_grab = audio_MIN (pa->dead, conf.samples >> 2);
         wpos = pa->wpos;
 
-        if (audio_pt_unlock (&pa->pt, AUDIO_FUNC)) {
-            return NULL;
-        }
+        audio_pt_unlock (&pa->pt);
 
         while (to_grab) {
             int error;
@@ -192,9 +176,7 @@ static void *qpa_thread_in (void *arg)
             to_grab -= chunk;
         }
 
-        if (audio_pt_lock (&pa->pt, AUDIO_FUNC)) {
-            return NULL;
-        }
+        audio_pt_lock (&pa->pt);
 
         pa->wpos = wpos;
         pa->dead -= incr;
@@ -202,7 +184,7 @@ static void *qpa_thread_in (void *arg)
     }
 
  exit:
-    audio_pt_unlock (&pa->pt, AUDIO_FUNC);
+    audio_pt_unlock (&pa->pt);
     return NULL;
 }
 
@@ -211,9 +193,7 @@ static int qpa_run_in (HWVoiceIn *hw)
     int live, incr, dead;
     PAVoiceIn *pa = (PAVoiceIn *) hw;
 
-    if (audio_pt_lock (&pa->pt, AUDIO_FUNC)) {
-        return 0;
-    }
+    audio_pt_lock (&pa->pt);
 
     live = audio_pcm_hw_get_live_in (hw);
     dead = hw->samples - live;
@@ -222,10 +202,10 @@ static int qpa_run_in (HWVoiceIn *hw)
     pa->dead = dead - incr;
     hw->wpos = pa->wpos;
     if (pa->dead > 0) {
-        audio_pt_unlock_and_signal (&pa->pt, AUDIO_FUNC);
+        audio_pt_unlock_and_signal (&pa->pt);
     }
     else {
-        audio_pt_unlock (&pa->pt, AUDIO_FUNC);
+        audio_pt_unlock (&pa->pt);
     }
     return incr;
 }
@@ -332,15 +312,10 @@ static int qpa_init_out (HWVoiceOut *hw, struct audsettings *as)
         goto fail2;
     }
 
-    if (audio_pt_init (&pa->pt, qpa_thread_out, hw, AUDIO_CAP, AUDIO_FUNC)) {
-        goto fail3;
-    }
+    audio_pt_init (&pa->pt, qpa_thread_out, hw);
 
     return 0;
 
- fail3:
-    g_free (pa->pcm_buf);
-    pa->pcm_buf = NULL;
  fail2:
     pa_simple_free (pa->s);
     pa->s = NULL;
@@ -387,15 +362,10 @@ static int qpa_init_in (HWVoiceIn *hw, struct audsettings *as)
         goto fail2;
     }
 
-    if (audio_pt_init (&pa->pt, qpa_thread_in, hw, AUDIO_CAP, AUDIO_FUNC)) {
-        goto fail3;
-    }
+    audio_pt_init (&pa->pt, qpa_thread_in, hw);
 
     return 0;
 
- fail3:
-    g_free (pa->pcm_buf);
-    pa->pcm_buf = NULL;
  fail2:
     pa_simple_free (pa->s);
     pa->s = NULL;
@@ -405,40 +375,38 @@ static int qpa_init_in (HWVoiceIn *hw, struct audsettings *as)
 
 static void qpa_fini_out (HWVoiceOut *hw)
 {
-    void *ret;
     PAVoiceOut *pa = (PAVoiceOut *) hw;
 
-    audio_pt_lock (&pa->pt, AUDIO_FUNC);
+    audio_pt_lock (&pa->pt);
     pa->done = 1;
-    audio_pt_unlock_and_signal (&pa->pt, AUDIO_FUNC);
-    audio_pt_join (&pa->pt, &ret, AUDIO_FUNC);
+    audio_pt_unlock_and_signal (&pa->pt);
+    audio_pt_join (&pa->pt);
 
     if (pa->s) {
         pa_simple_free (pa->s);
         pa->s = NULL;
     }
 
-    audio_pt_fini (&pa->pt, AUDIO_FUNC);
+    audio_pt_fini (&pa->pt);
     g_free (pa->pcm_buf);
     pa->pcm_buf = NULL;
 }
 
 static void qpa_fini_in (HWVoiceIn *hw)
 {
-    void *ret;
     PAVoiceIn *pa = (PAVoiceIn *) hw;
 
-    audio_pt_lock (&pa->pt, AUDIO_FUNC);
+    audio_pt_lock (&pa->pt);
     pa->done = 1;
-    audio_pt_unlock_and_signal (&pa->pt, AUDIO_FUNC);
-    audio_pt_join (&pa->pt, &ret, AUDIO_FUNC);
+    audio_pt_unlock_and_signal (&pa->pt);
+    audio_pt_join (&pa->pt);
 
     if (pa->s) {
         pa_simple_free (pa->s);
         pa->s = NULL;
     }
 
-    audio_pt_fini (&pa->pt, AUDIO_FUNC);
+    audio_pt_fini (&pa->pt);
     g_free (pa->pcm_buf);
     pa->pcm_buf = NULL;
 }
-- 
1.7.3.4

[Qemu-devel] [PATCH 6/6] audio: Switch coreaudio to QemuMutex

[Jan Kiszka]

Using the error management of QemuMutex allows to simplify the code.

CC: malc <av1474@comtv.ru>
CC: Andreas Färber <andreas.faerber@web.de>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 audio/coreaudio.c |   56 +++++++---------------------------------------------
 1 files changed, 8 insertions(+), 48 deletions(-)

diff --git a/audio/coreaudio.c b/audio/coreaudio.c
index 5964c62..c34a593 100644
--- a/audio/coreaudio.c
+++ b/audio/coreaudio.c
@@ -24,9 +24,9 @@
 
 #include <CoreAudio/CoreAudio.h>
 #include <string.h>             /* strerror */
-#include <pthread.h>            /* pthread_X */
 
 #include "qemu-common.h"
+#include "qemu-thread.h"
 #include "audio.h"
 
 #define AUDIO_CAP "coreaudio"
@@ -44,7 +44,7 @@ struct {
 
 typedef struct coreaudioVoiceOut {
     HWVoiceOut hw;
-    pthread_mutex_t mutex;
+    QemuMutex mutex;
     int isAtexit;
     AudioDeviceID outputDeviceID;
     UInt32 audioDevicePropertyBufferFrameSize;
@@ -164,40 +164,12 @@ static void coreaudio_atexit (void)
     conf.isAtexit = 1;
 }
 
-static int coreaudio_lock (coreaudioVoiceOut *core, const char *fn_name)
-{
-    int err;
-
-    err = pthread_mutex_lock (&core->mutex);
-    if (err) {
-        dolog ("Could not lock voice for %s\nReason: %s\n",
-               fn_name, strerror (err));
-        return -1;
-    }
-    return 0;
-}
-
-static int coreaudio_unlock (coreaudioVoiceOut *core, const char *fn_name)
-{
-    int err;
-
-    err = pthread_mutex_unlock (&core->mutex);
-    if (err) {
-        dolog ("Could not unlock voice for %s\nReason: %s\n",
-               fn_name, strerror (err));
-        return -1;
-    }
-    return 0;
-}
-
 static int coreaudio_run_out (HWVoiceOut *hw, int live)
 {
     int decr;
     coreaudioVoiceOut *core = (coreaudioVoiceOut *) hw;
 
-    if (coreaudio_lock (core, "coreaudio_run_out")) {
-        return 0;
-    }
+    qemu_mutex_lock(&core->mutex);
 
     if (core->decr > live) {
         ldebug ("core->decr %d live %d core->live %d\n",
@@ -240,10 +212,7 @@ static OSStatus audioDeviceIOProc(
 #endif
 #endif
 
-    if (coreaudio_lock (core, "audioDeviceIOProc")) {
-        inInputTime = 0;
-        return 0;
-    }
+    qemu_mutex_lock(&core->mutex);
 
     frameCount = core->audioDevicePropertyBufferFrameSize;
     live = core->live;
@@ -251,7 +220,7 @@ static OSStatus audioDeviceIOProc(
     /* if there are not enough samples, set signal and return */
     if (live < frameCount) {
         inInputTime = 0;
-        coreaudio_unlock (core, "audioDeviceIOProc(empty)");
+        qemu_mutex_unlock(&core->mutex);
         return 0;
     }
 
@@ -278,7 +247,7 @@ static OSStatus audioDeviceIOProc(
     core->decr += frameCount;
     core->rpos = rpos;
 
-    coreaudio_unlock (core, "audioDeviceIOProc");
+    qemu_mutex_unlock(&core->mutex);
     return 0;
 }
 
@@ -296,12 +265,7 @@ static int coreaudio_init_out (HWVoiceOut *hw, struct audsettings *as)
     const char *typ = "playback";
     AudioValueRange frameRange;
 
-    /* create mutex */
-    err = pthread_mutex_init(&core->mutex, NULL);
-    if (err) {
-        dolog("Could not create mutex\nReason: %s\n", strerror (err));
-        return -1;
-    }
+    qemu_mutex_init(&core->mutex);
 
     audio_pcm_init_info (&hw->info, as);
 
@@ -461,11 +425,7 @@ static void coreaudio_fini_out (HWVoiceOut *hw)
     }
     core->outputDeviceID = kAudioDeviceUnknown;
 
-    /* destroy mutex */
-    err = pthread_mutex_destroy(&core->mutex);
-    if (err) {
-        dolog("Could not destroy mutex\nReason: %s\n", strerror (err));
-    }
+    qemu_mutex_destroy(&core->mutex);
 }
 
 static int coreaudio_ctl_out (HWVoiceOut *hw, int cmd, ...)
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 2/6] Introduce qemu_cond_timedwait

[Paolo Bonzini]

On 09/20/2011 06:53 PM, Jan Kiszka wrote:
> First user will be posix compat aio.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>

I'm pretty sure the win32 version is not thread-safe, but posix compat 
aio is currently POSIX only.  Just leave it out.

Paolo

[Qemu-devel] [PATCH v2 2/6] Introduce qemu_cond_timedwait for POSIX

[Jan Kiszka]

On 2011-09-20 20:22, Paolo Bonzini wrote:
> On 09/20/2011 06:53 PM, Jan Kiszka wrote:
>> First user will be posix compat aio.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> 
> I'm pretty sure the win32 version is not thread-safe,

Yeah, I would even say it's completely broken. Was a naive hack.

> but posix compat
> aio is currently POSIX only.  Just leave it out.
> 

-------8<-------

From: Jan Kiszka <jan.kiszka@siemens.com>

First user will be POSIX compat aio. Windows use cases aren't in sight,
so this remains a POSIX-only service for now.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 qemu-thread-posix.c |   22 ++++++++++++++++++++++
 qemu-thread-posix.h |    2 ++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/qemu-thread-posix.c b/qemu-thread-posix.c
index f76427e..1d970fb 100644
--- a/qemu-thread-posix.c
+++ b/qemu-thread-posix.c
@@ -17,6 +17,7 @@
 #include <signal.h>
 #include <stdint.h>
 #include <string.h>
+#include <sys/time.h>
 #include "qemu-thread.h"
 
 static void error_exit(int err, const char *msg)
@@ -115,6 +116,27 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
         error_exit(err, __func__);
 }
 
+int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex,
+                        unsigned int timeout_ms)
+{
+    struct timespec ts;
+    struct timeval tv;
+    int err;
+
+    gettimeofday(&tv, NULL);
+    ts.tv_sec = tv.tv_sec + timeout_ms / 1000;
+    ts.tv_nsec = tv.tv_usec * 1000 + timeout_ms % 1000;
+    if (ts.tv_nsec > 1000000000) {
+        ts.tv_sec++;
+        ts.tv_nsec -= 1000000000;
+    }
+    err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
+    if (err && err != ETIMEDOUT) {
+        error_exit(err, __func__);
+    }
+    return err == 0;
+}
+
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void*),
                        void *arg, int mode)
diff --git a/qemu-thread-posix.h b/qemu-thread-posix.h
index 540fa0b..b4ae5ad 100644
--- a/qemu-thread-posix.h
+++ b/qemu-thread-posix.h
@@ -16,5 +16,7 @@ struct QemuThread {
 
 /* only provided for posix so far */
 void qemu_thread_join(QemuThread *thread);
+int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex,
+                        unsigned int timeout_ms);
 
 #endif

Re: [Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads

[Paolo Bonzini]

On 09/20/2011 06:53 PM, Jan Kiszka wrote:
> -    qemu_thread_create(&thread_id, event_thread, card);
> -    qemu_thread_create(&thread_id, handle_apdu_thread, card);
> +    qemu_thread_create(&thread_id, event_thread, card, QEMU_THREAD_DETACHED);
> +    qemu_thread_create(&thread_id, handle_apdu_thread, card,
> +                       QEMU_THREAD_DETACHED);
>       return 0;
>   }

I think these two should be joinable.  Otherwise, you might be 
destroying the apdu_thread_quit_mutex while the handle_apdu_thread 
hasn't yet finished unlocking it (even though it already progressed 
enough in qemu_mutex_destroy to release the main thread).

Anyhow, the bug is not introduced by your patch, so

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads

[Paolo Bonzini]

On 09/21/2011 03:40 PM, Kevin Wolf wrote:
>> >
>> >  I think these two should be joinable.  Otherwise, you might be
>> >  destroying the apdu_thread_quit_mutex while the handle_apdu_thread
>> >  hasn't yet finished unlocking it (even though it already progressed
>> >  enough in qemu_mutex_destroy to release the main thread).
>> >
>> >  Anyhow, the bug is not introduced by your patch, so
>> >
>> >  Reviewed-by: Paolo Bonzini<pbonzini@redhat.com>
> Actually, the man page says that joinable is the default, so this patch
> does change the behaviour.

Yes, but it doesn't change the fact that mutexes are destroyed under the 
thread's feet.  i.e. it doesn't introduce the race.

Paolo

Re: [Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads

[Kevin Wolf]

Am 21.09.2011 09:16, schrieb Paolo Bonzini:
> On 09/20/2011 06:53 PM, Jan Kiszka wrote:
>> -    qemu_thread_create(&thread_id, event_thread, card);
>> -    qemu_thread_create(&thread_id, handle_apdu_thread, card);
>> +    qemu_thread_create(&thread_id, event_thread, card, QEMU_THREAD_DETACHED);
>> +    qemu_thread_create(&thread_id, handle_apdu_thread, card,
>> +                       QEMU_THREAD_DETACHED);
>>       return 0;
>>   }
> 
> I think these two should be joinable.  Otherwise, you might be 
> destroying the apdu_thread_quit_mutex while the handle_apdu_thread 
> hasn't yet finished unlocking it (even though it already progressed 
> enough in qemu_mutex_destroy to release the main thread).
> 
> Anyhow, the bug is not introduced by your patch, so
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Actually, the man page says that joinable is the default, so this patch
does change the behaviour.

Kevin

Re: [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions

[Kevin Wolf]

Am 20.09.2011 18:53, schrieb Jan Kiszka:
> Although there is nothing to wrap for non-POSIX here, redirecting thread
> and synchronization services to our core simplifies managements jobs
> like scheduling parameter adjustment. It also frees compat AIO from some
> duplicate code (/wrt qemu-thread).
> 
> CC: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  posix-aio-compat.c |  115 ++++++++++++++-------------------------------------
>  1 files changed, 32 insertions(+), 83 deletions(-)
> 
> diff --git a/posix-aio-compat.c b/posix-aio-compat.c
> index d3c1174..0715aba 100644
> --- a/posix-aio-compat.c
> +++ b/posix-aio-compat.c
> @@ -13,7 +13,6 @@
>  
>  #include <sys/ioctl.h>
>  #include <sys/types.h>
> -#include <pthread.h>
>  #include <unistd.h>
>  #include <errno.h>
>  #include <time.h>
> @@ -27,9 +26,12 @@
>  #include "qemu-common.h"
>  #include "trace.h"
>  #include "block_int.h"
> +#include "qemu-thread.h"
>  
>  #include "block/raw-posix-aio.h"
>  
> +#define AIO_THREAD_IDLE_TIMEOUT 10000 /* 10 s */
> +
>  static void do_spawn_thread(void);
>  
>  struct qemu_paiocb {
> @@ -57,10 +59,9 @@ typedef struct PosixAioState {
>  } PosixAioState;
>  
>  
> -static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
> -static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
> -static pthread_t thread_id;
> -static pthread_attr_t attr;
> +static QemuMutex lock;
> +static QemuCond cond;
> +static QemuThread thread;
>  static int max_threads = 64;
>  static int cur_threads = 0;
>  static int idle_threads = 0;
> @@ -86,39 +87,6 @@ static void die(const char *what)
>      die2(errno, what);
>  }
>  
> -static void mutex_lock(pthread_mutex_t *mutex)
> -{
> -    int ret = pthread_mutex_lock(mutex);
> -    if (ret) die2(ret, "pthread_mutex_lock");
> -}
> -
> -static void mutex_unlock(pthread_mutex_t *mutex)
> -{
> -    int ret = pthread_mutex_unlock(mutex);
> -    if (ret) die2(ret, "pthread_mutex_unlock");
> -}
> -
> -static int cond_timedwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
> -                           struct timespec *ts)
> -{
> -    int ret = pthread_cond_timedwait(cond, mutex, ts);
> -    if (ret && ret != ETIMEDOUT) die2(ret, "pthread_cond_timedwait");
> -    return ret;
> -}
> -
> -static void cond_signal(pthread_cond_t *cond)
> -{
> -    int ret = pthread_cond_signal(cond);
> -    if (ret) die2(ret, "pthread_cond_signal");
> -}
> -
> -static void thread_create(pthread_t *thread, pthread_attr_t *attr,
> -                          void *(*start_routine)(void*), void *arg)
> -{
> -    int ret = pthread_create(thread, attr, start_routine, arg);
> -    if (ret) die2(ret, "pthread_create");
> -}
> -
>  static ssize_t handle_aiocb_ioctl(struct qemu_paiocb *aiocb)
>  {
>      int ret;
> @@ -311,27 +279,22 @@ static void posix_aio_notify_event(void);
>  
>  static void *aio_thread(void *unused)
>  {
> -    mutex_lock(&lock);
> +    qemu_mutex_lock(&lock);
>      pending_threads--;
> -    mutex_unlock(&lock);
> +    qemu_mutex_unlock(&lock);
>      do_spawn_thread();
>  
>      while (1) {
>          struct qemu_paiocb *aiocb;
> -        ssize_t ret = 0;
> -        qemu_timeval tv;
> -        struct timespec ts;
> -
> -        qemu_gettimeofday(&tv);
> -        ts.tv_sec = tv.tv_sec + 10;
> -        ts.tv_nsec = 0;
> +        bool timed_out = false;
> +        ssize_t ret;
>  
> -        mutex_lock(&lock);
> +        qemu_mutex_lock(&lock);
>  
> -        while (QTAILQ_EMPTY(&request_list) &&
> -               !(ret == ETIMEDOUT)) {
> +        while (QTAILQ_EMPTY(&request_list) && !timed_out) {
>              idle_threads++;
> -            ret = cond_timedwait(&cond, &lock, &ts);
> +            timed_out = qemu_cond_timedwait(&cond, &lock,
> +                                            AIO_THREAD_IDLE_TIMEOUT) != 0;

Maybe I'm confused by too many negations, but isn't this the wrong way
round?

+    err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
+    if (err && err != ETIMEDOUT) {
+        error_exit(err, __func__);
+    }
+    return err == 0;

So if there was an timeout, qemu_cond_timedwait returns 0 (should it
return a bool? Also documenting the return value wouldn't hurt) and
timed_out becomes false (0 != 0).

Kevin

Re: [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions

[Jan Kiszka]

On 2011-09-21 15:57, Kevin Wolf wrote:
> Am 20.09.2011 18:53, schrieb Jan Kiszka:
>> Although there is nothing to wrap for non-POSIX here, redirecting thread
>> and synchronization services to our core simplifies managements jobs
>> like scheduling parameter adjustment. It also frees compat AIO from some
>> duplicate code (/wrt qemu-thread).
>>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  posix-aio-compat.c |  115 ++++++++++++++-------------------------------------
>>  1 files changed, 32 insertions(+), 83 deletions(-)
>>
>> diff --git a/posix-aio-compat.c b/posix-aio-compat.c
>> index d3c1174..0715aba 100644
>> --- a/posix-aio-compat.c
>> +++ b/posix-aio-compat.c
>> @@ -13,7 +13,6 @@
>>  
>>  #include <sys/ioctl.h>
>>  #include <sys/types.h>
>> -#include <pthread.h>
>>  #include <unistd.h>
>>  #include <errno.h>
>>  #include <time.h>
>> @@ -27,9 +26,12 @@
>>  #include "qemu-common.h"
>>  #include "trace.h"
>>  #include "block_int.h"
>> +#include "qemu-thread.h"
>>  
>>  #include "block/raw-posix-aio.h"
>>  
>> +#define AIO_THREAD_IDLE_TIMEOUT 10000 /* 10 s */
>> +
>>  static void do_spawn_thread(void);
>>  
>>  struct qemu_paiocb {
>> @@ -57,10 +59,9 @@ typedef struct PosixAioState {
>>  } PosixAioState;
>>  
>>  
>> -static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
>> -static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
>> -static pthread_t thread_id;
>> -static pthread_attr_t attr;
>> +static QemuMutex lock;
>> +static QemuCond cond;
>> +static QemuThread thread;
>>  static int max_threads = 64;
>>  static int cur_threads = 0;
>>  static int idle_threads = 0;
>> @@ -86,39 +87,6 @@ static void die(const char *what)
>>      die2(errno, what);
>>  }
>>  
>> -static void mutex_lock(pthread_mutex_t *mutex)
>> -{
>> -    int ret = pthread_mutex_lock(mutex);
>> -    if (ret) die2(ret, "pthread_mutex_lock");
>> -}
>> -
>> -static void mutex_unlock(pthread_mutex_t *mutex)
>> -{
>> -    int ret = pthread_mutex_unlock(mutex);
>> -    if (ret) die2(ret, "pthread_mutex_unlock");
>> -}
>> -
>> -static int cond_timedwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
>> -                           struct timespec *ts)
>> -{
>> -    int ret = pthread_cond_timedwait(cond, mutex, ts);
>> -    if (ret && ret != ETIMEDOUT) die2(ret, "pthread_cond_timedwait");
>> -    return ret;
>> -}
>> -
>> -static void cond_signal(pthread_cond_t *cond)
>> -{
>> -    int ret = pthread_cond_signal(cond);
>> -    if (ret) die2(ret, "pthread_cond_signal");
>> -}
>> -
>> -static void thread_create(pthread_t *thread, pthread_attr_t *attr,
>> -                          void *(*start_routine)(void*), void *arg)
>> -{
>> -    int ret = pthread_create(thread, attr, start_routine, arg);
>> -    if (ret) die2(ret, "pthread_create");
>> -}
>> -
>>  static ssize_t handle_aiocb_ioctl(struct qemu_paiocb *aiocb)
>>  {
>>      int ret;
>> @@ -311,27 +279,22 @@ static void posix_aio_notify_event(void);
>>  
>>  static void *aio_thread(void *unused)
>>  {
>> -    mutex_lock(&lock);
>> +    qemu_mutex_lock(&lock);
>>      pending_threads--;
>> -    mutex_unlock(&lock);
>> +    qemu_mutex_unlock(&lock);
>>      do_spawn_thread();
>>  
>>      while (1) {
>>          struct qemu_paiocb *aiocb;
>> -        ssize_t ret = 0;
>> -        qemu_timeval tv;
>> -        struct timespec ts;
>> -
>> -        qemu_gettimeofday(&tv);
>> -        ts.tv_sec = tv.tv_sec + 10;
>> -        ts.tv_nsec = 0;
>> +        bool timed_out = false;
>> +        ssize_t ret;
>>  
>> -        mutex_lock(&lock);
>> +        qemu_mutex_lock(&lock);
>>  
>> -        while (QTAILQ_EMPTY(&request_list) &&
>> -               !(ret == ETIMEDOUT)) {
>> +        while (QTAILQ_EMPTY(&request_list) && !timed_out) {
>>              idle_threads++;
>> -            ret = cond_timedwait(&cond, &lock, &ts);
>> +            timed_out = qemu_cond_timedwait(&cond, &lock,
>> +                                            AIO_THREAD_IDLE_TIMEOUT) != 0;
> 
> Maybe I'm confused by too many negations, but isn't this the wrong way
> round?

You mean design-wise? Maybe. In any case, I think this code would also
win if we just do

	if (timed_out)
		break;

in the loop instead of testing the inverse on entry.

> 
> +    err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
> +    if (err && err != ETIMEDOUT) {
> +        error_exit(err, __func__);
> +    }
> +    return err == 0;
> 
> So if there was an timeout, qemu_cond_timedwait returns 0 (should it
> return a bool? Also documenting the return value wouldn't hurt) and
> timed_out becomes false (0 != 0).

Will switch to a bool return code (and document it).

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions

[Kevin Wolf]

Am 21.09.2011 16:02, schrieb Jan Kiszka:
> On 2011-09-21 15:57, Kevin Wolf wrote:
>> Am 20.09.2011 18:53, schrieb Jan Kiszka:
>>> Although there is nothing to wrap for non-POSIX here, redirecting thread
>>> and synchronization services to our core simplifies managements jobs
>>> like scheduling parameter adjustment. It also frees compat AIO from some
>>> duplicate code (/wrt qemu-thread).
>>>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>  posix-aio-compat.c |  115 ++++++++++++++-------------------------------------
>>>  1 files changed, 32 insertions(+), 83 deletions(-)

>>> @@ -311,27 +279,22 @@ static void posix_aio_notify_event(void);
>>>  
>>>  static void *aio_thread(void *unused)
>>>  {
>>> -    mutex_lock(&lock);
>>> +    qemu_mutex_lock(&lock);
>>>      pending_threads--;
>>> -    mutex_unlock(&lock);
>>> +    qemu_mutex_unlock(&lock);
>>>      do_spawn_thread();
>>>  
>>>      while (1) {
>>>          struct qemu_paiocb *aiocb;
>>> -        ssize_t ret = 0;
>>> -        qemu_timeval tv;
>>> -        struct timespec ts;
>>> -
>>> -        qemu_gettimeofday(&tv);
>>> -        ts.tv_sec = tv.tv_sec + 10;
>>> -        ts.tv_nsec = 0;
>>> +        bool timed_out = false;
>>> +        ssize_t ret;
>>>  
>>> -        mutex_lock(&lock);
>>> +        qemu_mutex_lock(&lock);
>>>  
>>> -        while (QTAILQ_EMPTY(&request_list) &&
>>> -               !(ret == ETIMEDOUT)) {
>>> +        while (QTAILQ_EMPTY(&request_list) && !timed_out) {
>>>              idle_threads++;
>>> -            ret = cond_timedwait(&cond, &lock, &ts);
>>> +            timed_out = qemu_cond_timedwait(&cond, &lock,
>>> +                                            AIO_THREAD_IDLE_TIMEOUT) != 0;
>>
>> Maybe I'm confused by too many negations, but isn't this the wrong way
>> round?
> 
> You mean design-wise? Maybe. In any case, I think this code would also
> win if we just do
> 
> 	if (timed_out)
> 		break;
> 
> in the loop instead of testing the inverse on entry.

Design-wise I'm not sure. Maybe it would be more consistent if
qemu_cond_timedwait returned 0/ETIMEDOUT, maybe it doesn't really make a
difference. I just felt a bit confused when reading it.

What I really meant is that I think it should be == instead of !=:

timed_out = qemu_cond_timedwait(...) == 0;

>> +    err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
>> +    if (err && err != ETIMEDOUT) {
>> +        error_exit(err, __func__);
>> +    }
>> +    return err == 0;
>>
>> So if there was an timeout, qemu_cond_timedwait returns 0 (should it
>> return a bool? Also documenting the return value wouldn't hurt) and
>> timed_out becomes false (0 != 0).
> 
> Will switch to a bool return code (and document it).

Ok.

Kevin

Re: [Qemu-devel] [PATCH 6/6] audio: Switch coreaudio to QemuMutex

[Andreas Färber]

Hello Jan,

Am 20.09.2011 um 18:53 schrieb Jan Kiszka:

> Using the error management of QemuMutex allows to simplify the code.
> 
> CC: malc <av1474@comtv.ru>
> CC: Andreas Färber <andreas.faerber@web.de>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> audio/coreaudio.c |   56 +++++++---------------------------------------------
> 1 files changed, 8 insertions(+), 48 deletions(-)

You missed one coreaudio_unlock() occurrence in line 187. Other than that looks okay, with some more softfloat workarounds it compiles and doesn't noticeably regress on Darwin/i386 v10.6.8.

diff --git a/audio/coreaudio.c b/audio/coreaudio.c
index c34a593..1cb3fce 100644
--- a/audio/coreaudio.c
+++ b/audio/coreaudio.c
@@ -184,7 +184,7 @@ static int coreaudio_run_out (HWVoiceOut *hw, int live)
    core->live = live - decr;
    hw->rpos = core->rpos;

-    coreaudio_unlock (core, "coreaudio_run_out");
+    qemu_mutex_unlock(&core->mutex);
    return decr;
}


Do you have a particular test case? Any parameters needed? I've never used audio in my guests. ;)

Regards,
Andreas

Re: [Qemu-devel] [PATCH 6/6] audio: Switch coreaudio to QemuMutex

[Jan Kiszka]

On 2011-09-26 09:58, Andreas Färber wrote:
> Hello Jan,
> 
> Am 20.09.2011 um 18:53 schrieb Jan Kiszka:
> 
>> Using the error management of QemuMutex allows to simplify the code.
>>
>> CC: malc <av1474@comtv.ru>
>> CC: Andreas Färber <andreas.faerber@web.de>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> audio/coreaudio.c |   56 +++++++---------------------------------------------
>> 1 files changed, 8 insertions(+), 48 deletions(-)
> 
> You missed one coreaudio_unlock() occurrence in line 187. Other than that looks okay, with some more softfloat workarounds it compiles and doesn't noticeably regress on Darwin/i386 v10.6.8.
> 
> diff --git a/audio/coreaudio.c b/audio/coreaudio.c
> index c34a593..1cb3fce 100644
> --- a/audio/coreaudio.c
> +++ b/audio/coreaudio.c
> @@ -184,7 +184,7 @@ static int coreaudio_run_out (HWVoiceOut *hw, int live)
>     core->live = live - decr;
>     hw->rpos = core->rpos;
> 
> -    coreaudio_unlock (core, "coreaudio_run_out");
> +    qemu_mutex_unlock(&core->mutex);
>     return decr;
> }
> 
> 
> Do you have a particular test case? Any parameters needed? I've never used audio in my guests. ;)

Nope, I hoped you had. :)

But I received indications that the implicit logging removal is not
welcome. So I will likely refrain from touching the audio subsystem in
the first step.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux