From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:34128) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R6NFg-0003vj-9j for qemu-devel@nongnu.org; Wed, 21 Sep 2011 09:54:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R6NFV-0001Rs-VE for qemu-devel@nongnu.org; Wed, 21 Sep 2011 09:54:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:19855) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R6NFV-0001RJ-HW for qemu-devel@nongnu.org; Wed, 21 Sep 2011 09:54:05 -0400 Message-ID: <4E79ED2D.5000008@redhat.com> Date: Wed, 21 Sep 2011 15:57:01 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <51c937a96190990937b0d5788e6ea3208e9c8565.1316537591.git.jan.kiszka@siemens.com> In-Reply-To: <51c937a96190990937b0d5788e6ea3208e9c8565.1316537591.git.jan.kiszka@siemens.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/6] Switch POSIX compat AIO to QEMU abstractions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Paolo Bonzini , Anthony Liguori , qemu-devel@nongnu.org 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 > Signed-off-by: Jan Kiszka > --- > 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 > #include > -#include > #include > #include > #include > @@ -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