From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KxROD-0005dD-5J for qemu-devel@nongnu.org; Tue, 04 Nov 2008 14:16:33 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KxROC-0005co-M4 for qemu-devel@nongnu.org; Tue, 04 Nov 2008 14:16:32 -0500 Received: from [199.232.76.173] (port=51125 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KxROC-0005cj-De for qemu-devel@nongnu.org; Tue, 04 Nov 2008 14:16:32 -0500 Received: from nf-out-0910.google.com ([64.233.182.184]:65000) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KxROC-00008q-HP for qemu-devel@nongnu.org; Tue, 04 Nov 2008 14:16:32 -0500 Received: by nf-out-0910.google.com with SMTP id b2so2108813nfb.12 for ; Tue, 04 Nov 2008 11:16:30 -0800 (PST) Message-ID: <49109F8A.8080903@codemonkey.ws> Date: Tue, 04 Nov 2008 13:16:26 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH] Fix alarm_timer race with select References: <490FFF23.5020704@web.de> <49105704.9070708@codemonkey.ws> <49105BE7.6030505@codemonkey.ws> <49108333.6080807@web.de> In-Reply-To: <49108333.6080807@web.de> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit 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 Cc: Jan Kiszka Jan Kiszka wrote: > Anthony Liguori wrote: > >> Anthony Liguori wrote: >> >>> Jan Kiszka wrote: >>> >>> Perhaps we should move the alarm timer check rearming out of the main >>> loop and into a qemu_set_fd_handler2() handler? >>> >> And now that I think about it, I see no reason why the timer expiration >> checks couldn't be moved to the same handler. I'd have to look more >> closely at how that would interact with icount though. >> > > I cannot yet follow you here. But I my impression is that this will be > an additional change that should come in a separate patch, no? > > Find the O_NONBLOCK remark addressed below. > > Jan > > ------------> > > 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. > > 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 | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > Index: b/vl.c > =================================================================== > --- a/vl.c > +++ b/vl.c > @@ -884,6 +884,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 > > @@ -1303,12 +1304,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)); > alarm_timer->flags |= ALARM_FLAG_EXPIRED; > > if (env) { > @@ -1673,6 +1677,15 @@ static void init_timer_alarm(void) > { > struct qemu_alarm_timer *t = NULL; > int i, err = -1; > + int fds[2]; > + > + if (pipe(fds) || fcntl(fds[0], F_SETFL, O_NONBLOCK) > + || fcntl(fds[1], F_SETFL, O_NONBLOCK)) { > + perror("creating timer pipe"); > + exit(1); > + } > + alarm_timer_rfd = fds[0]; > + alarm_timer_wfd = fds[1]; > > for (i = 0; alarm_timers[i].name; i++) { > t = &alarm_timers[i]; > @@ -4427,6 +4440,7 @@ void main_loop_wait(int timeout) > /* XXX: separate device handlers from system ones */ > nfds = -1; > FD_ZERO(&rfds); > + FD_SET(alarm_timer_rfd, &rfds); > Sorry, I missed this in the first patch. You have to take alarm_timer_rfd into account when computing max_fd otherwise badness could result. While you're at it, please don't call pipe and fcntl() twice from inside of an if() clause. Regards, Anthony Liguori