* [Qemu-devel] [PATCH 0/6] Spread the use of QEMU threading & locking API
@ 2011-09-20 16:53 Jan Kiszka
2011-09-20 16:53 ` [Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads Jan Kiszka
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: Jan Kiszka @ 2011-09-20 16:53 UTC (permalink / raw)
To: Anthony Liguori, qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Andreas Färber
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
^ permalink raw reply [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads 2011-09-20 16:53 [Qemu-devel] [PATCH 0/6] Spread the use of QEMU threading & locking API Jan Kiszka @ 2011-09-20 16:53 ` Jan Kiszka 2011-09-21 7:16 ` Paolo Bonzini 2011-09-20 16:53 ` [Qemu-devel] [PATCH 2/6] Introduce qemu_cond_timedwait Jan Kiszka ` (4 subsequent siblings) 5 siblings, 1 reply; 17+ messages in thread From: Jan Kiszka @ 2011-09-20 16:53 UTC (permalink / raw) To: Anthony Liguori, qemu-devel; +Cc: Paolo Bonzini 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads 2011-09-20 16:53 ` [Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads Jan Kiszka @ 2011-09-21 7:16 ` Paolo Bonzini 2011-09-21 13:40 ` Kevin Wolf 0 siblings, 1 reply; 17+ messages in thread From: Paolo Bonzini @ 2011-09-21 7:16 UTC (permalink / raw) To: Jan Kiszka; +Cc: Anthony Liguori, Alon Levy, qemu-devel 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> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads 2011-09-21 7:16 ` Paolo Bonzini @ 2011-09-21 13:40 ` Kevin Wolf 2011-09-21 13:38 ` Paolo Bonzini 0 siblings, 1 reply; 17+ messages in thread From: Kevin Wolf @ 2011-09-21 13:40 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Jan Kiszka, Anthony Liguori, Alon Levy, qemu-devel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads 2011-09-21 13:40 ` Kevin Wolf @ 2011-09-21 13:38 ` Paolo Bonzini 0 siblings, 0 replies; 17+ messages in thread From: Paolo Bonzini @ 2011-09-21 13:38 UTC (permalink / raw) To: Kevin Wolf; +Cc: Jan Kiszka, Anthony Liguori, Alon Levy, qemu-devel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/6] Introduce qemu_cond_timedwait 2011-09-20 16:53 [Qemu-devel] [PATCH 0/6] Spread the use of QEMU threading & locking API Jan Kiszka 2011-09-20 16:53 ` [Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads Jan Kiszka @ 2011-09-20 16:53 ` Jan Kiszka 2011-09-20 18:22 ` Paolo Bonzini 2011-09-20 16:53 ` [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions Jan Kiszka ` (3 subsequent siblings) 5 siblings, 1 reply; 17+ messages in thread From: Jan Kiszka @ 2011-09-20 16:53 UTC (permalink / raw) To: Anthony Liguori, qemu-devel; +Cc: Paolo Bonzini 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] Introduce qemu_cond_timedwait 2011-09-20 16:53 ` [Qemu-devel] [PATCH 2/6] Introduce qemu_cond_timedwait Jan Kiszka @ 2011-09-20 18:22 ` Paolo Bonzini 2011-09-20 19:02 ` [Qemu-devel] [PATCH v2 2/6] Introduce qemu_cond_timedwait for POSIX Jan Kiszka 0 siblings, 1 reply; 17+ messages in thread From: Paolo Bonzini @ 2011-09-20 18:22 UTC (permalink / raw) To: Jan Kiszka; +Cc: Anthony Liguori, qemu-devel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v2 2/6] Introduce qemu_cond_timedwait for POSIX 2011-09-20 18:22 ` Paolo Bonzini @ 2011-09-20 19:02 ` Jan Kiszka 0 siblings, 0 replies; 17+ messages in thread From: Jan Kiszka @ 2011-09-20 19:02 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Anthony Liguori, qemu-devel 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions 2011-09-20 16:53 [Qemu-devel] [PATCH 0/6] Spread the use of QEMU threading & locking API Jan Kiszka 2011-09-20 16:53 ` [Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads Jan Kiszka 2011-09-20 16:53 ` [Qemu-devel] [PATCH 2/6] Introduce qemu_cond_timedwait Jan Kiszka @ 2011-09-20 16:53 ` Jan Kiszka 2011-09-21 13:57 ` Kevin Wolf 2011-09-20 16:53 ` [Qemu-devel] [PATCH 4/6] Switch compatfd to QEMU thread Jan Kiszka ` (2 subsequent siblings) 5 siblings, 1 reply; 17+ messages in thread From: Jan Kiszka @ 2011-09-20 16:53 UTC (permalink / raw) To: Anthony Liguori, qemu-devel; +Cc: Kevin Wolf, Paolo Bonzini 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions 2011-09-20 16:53 ` [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions Jan Kiszka @ 2011-09-21 13:57 ` Kevin Wolf 2011-09-21 14:02 ` Jan Kiszka 0 siblings, 1 reply; 17+ messages in thread From: Kevin Wolf @ 2011-09-21 13:57 UTC (permalink / raw) To: Jan Kiszka; +Cc: Paolo Bonzini, Anthony Liguori, qemu-devel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions 2011-09-21 13:57 ` Kevin Wolf @ 2011-09-21 14:02 ` Jan Kiszka 2011-09-21 14:11 ` Kevin Wolf 0 siblings, 1 reply; 17+ messages in thread From: Jan Kiszka @ 2011-09-21 14:02 UTC (permalink / raw) To: Kevin Wolf; +Cc: Paolo Bonzini, Anthony Liguori, qemu-devel@nongnu.org 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions 2011-09-21 14:02 ` Jan Kiszka @ 2011-09-21 14:11 ` Kevin Wolf 0 siblings, 0 replies; 17+ messages in thread From: Kevin Wolf @ 2011-09-21 14:11 UTC (permalink / raw) To: Jan Kiszka; +Cc: Paolo Bonzini, Anthony Liguori, qemu-devel@nongnu.org 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 4/6] Switch compatfd to QEMU thread 2011-09-20 16:53 [Qemu-devel] [PATCH 0/6] Spread the use of QEMU threading & locking API Jan Kiszka ` (2 preceding siblings ...) 2011-09-20 16:53 ` [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions Jan Kiszka @ 2011-09-20 16:53 ` Jan Kiszka 2011-09-20 16:53 ` [Qemu-devel] [PATCH 5/6] audio: Use QEMU threads & synchronization Jan Kiszka 2011-09-20 16:53 ` [Qemu-devel] [PATCH 6/6] audio: Switch coreaudio to QemuMutex Jan Kiszka 5 siblings, 0 replies; 17+ messages in thread From: Jan Kiszka @ 2011-09-20 16:53 UTC (permalink / raw) To: Anthony Liguori, qemu-devel; +Cc: Paolo Bonzini 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 5/6] audio: Use QEMU threads & synchronization 2011-09-20 16:53 [Qemu-devel] [PATCH 0/6] Spread the use of QEMU threading & locking API Jan Kiszka ` (3 preceding siblings ...) 2011-09-20 16:53 ` [Qemu-devel] [PATCH 4/6] Switch compatfd to QEMU thread Jan Kiszka @ 2011-09-20 16:53 ` Jan Kiszka 2011-09-20 16:53 ` [Qemu-devel] [PATCH 6/6] audio: Switch coreaudio to QemuMutex Jan Kiszka 5 siblings, 0 replies; 17+ messages in thread From: Jan Kiszka @ 2011-09-20 16:53 UTC (permalink / raw) To: Anthony Liguori, qemu-devel; +Cc: Paolo Bonzini 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 6/6] audio: Switch coreaudio to QemuMutex 2011-09-20 16:53 [Qemu-devel] [PATCH 0/6] Spread the use of QEMU threading & locking API Jan Kiszka ` (4 preceding siblings ...) 2011-09-20 16:53 ` [Qemu-devel] [PATCH 5/6] audio: Use QEMU threads & synchronization Jan Kiszka @ 2011-09-20 16:53 ` Jan Kiszka 2011-09-26 7:58 ` Andreas Färber 5 siblings, 1 reply; 17+ messages in thread From: Jan Kiszka @ 2011-09-20 16:53 UTC (permalink / raw) To: Anthony Liguori, qemu-devel; +Cc: Paolo Bonzini, Andreas Färber 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] audio: Switch coreaudio to QemuMutex 2011-09-20 16:53 ` [Qemu-devel] [PATCH 6/6] audio: Switch coreaudio to QemuMutex Jan Kiszka @ 2011-09-26 7:58 ` Andreas Färber 2011-09-26 8:06 ` Jan Kiszka 0 siblings, 1 reply; 17+ messages in thread From: Andreas Färber @ 2011-09-26 7:58 UTC (permalink / raw) To: Jan Kiszka; +Cc: Paolo Bonzini, Anthony Liguori, qemu-devel 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] audio: Switch coreaudio to QemuMutex 2011-09-26 7:58 ` Andreas Färber @ 2011-09-26 8:06 ` Jan Kiszka 0 siblings, 0 replies; 17+ messages in thread From: Jan Kiszka @ 2011-09-26 8:06 UTC (permalink / raw) To: Andreas Färber; +Cc: Paolo Bonzini, Anthony Liguori, qemu-devel@nongnu.org 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2011-09-26 8:06 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-09-20 16:53 [Qemu-devel] [PATCH 0/6] Spread the use of QEMU threading & locking API Jan Kiszka 2011-09-20 16:53 ` [Qemu-devel] [PATCH 1/6] Enable joinable POSIX threads Jan Kiszka 2011-09-21 7:16 ` Paolo Bonzini 2011-09-21 13:40 ` Kevin Wolf 2011-09-21 13:38 ` Paolo Bonzini 2011-09-20 16:53 ` [Qemu-devel] [PATCH 2/6] Introduce qemu_cond_timedwait Jan Kiszka 2011-09-20 18:22 ` Paolo Bonzini 2011-09-20 19:02 ` [Qemu-devel] [PATCH v2 2/6] Introduce qemu_cond_timedwait for POSIX Jan Kiszka 2011-09-20 16:53 ` [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions Jan Kiszka 2011-09-21 13:57 ` Kevin Wolf 2011-09-21 14:02 ` Jan Kiszka 2011-09-21 14:11 ` Kevin Wolf 2011-09-20 16:53 ` [Qemu-devel] [PATCH 4/6] Switch compatfd to QEMU thread Jan Kiszka 2011-09-20 16:53 ` [Qemu-devel] [PATCH 5/6] audio: Use QEMU threads & synchronization Jan Kiszka 2011-09-20 16:53 ` [Qemu-devel] [PATCH 6/6] audio: Switch coreaudio to QemuMutex Jan Kiszka 2011-09-26 7:58 ` Andreas Färber 2011-09-26 8:06 ` Jan Kiszka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).