From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:37977) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R6NTI-0007WE-A3 for qemu-devel@nongnu.org; Wed, 21 Sep 2011 10:08:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R6NTC-0004PP-79 for qemu-devel@nongnu.org; Wed, 21 Sep 2011 10:08:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9133) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R6NTB-0004PJ-Tg for qemu-devel@nongnu.org; Wed, 21 Sep 2011 10:08:14 -0400 Message-ID: <4E79F07E.2000008@redhat.com> Date: Wed, 21 Sep 2011 16:11:10 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <51c937a96190990937b0d5788e6ea3208e9c8565.1316537591.git.jan.kiszka@siemens.com> <4E79ED2D.5000008@redhat.com> <4E79EE72.3090509@siemens.com> In-Reply-To: <4E79EE72.3090509@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 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 >>> Signed-off-by: Jan Kiszka >>> --- >>> 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