[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstraction
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions |
Date: |
Wed, 21 Sep 2011 15:57:01 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:6.0.2) Gecko/20110906 Thunderbird/6.0.2 |
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 <address@hidden>
> Signed-off-by: Jan Kiszka <address@hidden>
> ---
> 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