From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Kxnfo-0004v7-Va for qemu-devel@nongnu.org; Wed, 05 Nov 2008 14:04:13 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Kxnfo-0004ul-8a for qemu-devel@nongnu.org; Wed, 05 Nov 2008 14:04:12 -0500 Received: from [199.232.76.173] (port=60385 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Kxnfo-0004ui-56 for qemu-devel@nongnu.org; Wed, 05 Nov 2008 14:04:12 -0500 Received: from lizzard.sbs.de ([194.138.37.39]:17573) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Kxnfn-0005ID-DB for qemu-devel@nongnu.org; Wed, 05 Nov 2008 14:04:11 -0500 Received: from mail2.sbs.de (localhost [127.0.0.1]) by lizzard.sbs.de (8.12.11.20060308/8.12.11) with ESMTP id mA5J48Hh007275 for ; Wed, 5 Nov 2008 20:04:09 +0100 Received: from [139.25.109.167] (mchn012c.ww002.siemens.net [139.25.109.167] (may be forged)) by mail2.sbs.de (8.12.11.20060308/8.12.11) with ESMTP id mA5J48MX027858 for ; Wed, 5 Nov 2008 20:04:08 +0100 Message-ID: <4911EE29.7040300@siemens.com> Date: Wed, 05 Nov 2008 20:04:09 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <4911E5AA.1000408@siemens.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] Fix alarm_timer race with select - v3 Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Blue Swirl wrote: > On 11/5/08, Jan Kiszka wrote: >> [ changes: correct nfds initialization, more robust O_NONBLOCK setup ] >> >> Changing the default IO timeout to 5 s (#5578) made a race visible >> between the alarm_timer and select() in main_loop_wait(): If the timer >> fired before select() was able to block, the full select() timeout could >> have been applied instead of returning immediately. Since #5578, this >> causes heavy problems to the Musicpal board emulation with stalls up to >> 5 s, but also with some older Linux guest kernels. >> >> The following patch introduces a pipe that is written to by >> host_alarm_handler and select()'ed in main_loop_wait(). This avoids >> prevents that select() blocks though a timer has fired and waits for >> processing. >> >> Signed-off-by: Jan Kiszka > >> @@ -1304,12 +1305,15 @@ static void host_alarm_handler(int host_ >> qemu_get_clock(vm_clock))) || >> qemu_timer_expired(active_timers[QEMU_TIMER_REALTIME], >> qemu_get_clock(rt_clock))) { >> + CPUState *env = next_cpu; >> + char byte = 0; >> + >> #ifdef _WIN32 >> struct qemu_alarm_win32 *data = ((struct qemu_alarm_timer*)dwUser)->priv; >> SetEvent(data->host_alarm); >> #endif >> - CPUState *env = next_cpu; >> >> + write(alarm_timer_wfd, &byte, sizeof(byte)); > > For the write case, we could save one initialization write access to > the "byte" for every alarm trigger if it's static const. > So shall it be. --------> Changing the default IO timeout to 5 s (#5578) made a race visible between the alarm_timer and select() in main_loop_wait(): If the timer fired before select was able to block, the full select() timeout could have been applied instead of returning immediately. Since #5578, this causes heavy problems to the Musicpal board emulation with stalls up to 5 s, but also with some older Linux guest kernels. The following patch introduces a pipe that is written to by host_alarm_handler and select()'ed in main_loop_wait(). This avoids prevents that select() blocks though a timer has fired and waits for processing. Signed-off-by: Jan Kiszka --- vl.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) Index: b/vl.c =================================================================== --- a/vl.c +++ b/vl.c @@ -885,6 +885,7 @@ static void qemu_rearm_alarm_timer(struc #define MIN_TIMER_REARM_US 250 static struct qemu_alarm_timer *alarm_timer; +static int alarm_timer_rfd, alarm_timer_wfd; #ifdef _WIN32 @@ -1304,12 +1305,15 @@ static void host_alarm_handler(int host_ qemu_get_clock(vm_clock))) || qemu_timer_expired(active_timers[QEMU_TIMER_REALTIME], qemu_get_clock(rt_clock))) { + CPUState *env = next_cpu; + static const char byte = 0; + #ifdef _WIN32 struct qemu_alarm_win32 *data = ((struct qemu_alarm_timer*)dwUser)->priv; SetEvent(data->host_alarm); #endif - CPUState *env = next_cpu; + write(alarm_timer_wfd, &byte, sizeof(byte)); alarm_timer->flags |= ALARM_FLAG_EXPIRED; if (env) { @@ -1674,6 +1678,20 @@ static void init_timer_alarm(void) { struct qemu_alarm_timer *t = NULL; int i, err = -1; + int fds[2]; + + if (pipe(fds) < 0) { + fail: + perror("creating timer pipe"); + exit(1); + } + for (i = 0; i < 2; i++) { + int flags = fcntl(fds[i], F_GETFL); + if (flags == -1 || fcntl(fds[i], F_SETFL, flags | O_NONBLOCK)) + goto fail; + } + alarm_timer_rfd = fds[0]; + alarm_timer_wfd = fds[1]; for (i = 0; alarm_timers[i].name; i++) { t = &alarm_timers[i]; @@ -4426,8 +4444,9 @@ void main_loop_wait(int timeout) /* poll any events */ /* XXX: separate device handlers from system ones */ - nfds = -1; + nfds = alarm_timer_rfd; FD_ZERO(&rfds); + FD_SET(alarm_timer_rfd, &rfds); FD_ZERO(&wfds); FD_ZERO(&xfds); for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) { @@ -4501,6 +4520,11 @@ void main_loop_wait(int timeout) qemu_get_clock(rt_clock)); if (alarm_timer->flags & ALARM_FLAG_EXPIRED) { + char byte; + do { + ret = read(alarm_timer_rfd, &byte, sizeof(byte)); + } while (ret != -1 || errno != EAGAIN); + alarm_timer->flags &= ~(ALARM_FLAG_EXPIRED); qemu_rearm_alarm_timer(alarm_timer); }