From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49175) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R6NNo-0005rt-UB for qemu-devel@nongnu.org; Wed, 21 Sep 2011 10:02:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R6NNf-0002z1-86 for qemu-devel@nongnu.org; Wed, 21 Sep 2011 10:02:40 -0400 Received: from goliath.siemens.de ([192.35.17.28]:19995) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R6NNe-0002xw-Vf for qemu-devel@nongnu.org; Wed, 21 Sep 2011 10:02:31 -0400 Message-ID: <4E79EE72.3090509@siemens.com> Date: Wed, 21 Sep 2011 16:02:26 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <51c937a96190990937b0d5788e6ea3208e9c8565.1316537591.git.jan.kiszka@siemens.com> <4E79ED2D.5000008@redhat.com> In-Reply-To: <4E79ED2D.5000008@redhat.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: 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 >> 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? 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