From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KxMYt-0005Hw-3V for qemu-devel@nongnu.org; Tue, 04 Nov 2008 09:07:15 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KxMYo-0005G1-6Y for qemu-devel@nongnu.org; Tue, 04 Nov 2008 09:07:11 -0500 Received: from [199.232.76.173] (port=47596 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KxMYo-0005Fp-0Z for qemu-devel@nongnu.org; Tue, 04 Nov 2008 09:07:10 -0500 Received: from yw-out-1718.google.com ([74.125.46.155]:42512) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KxMYn-0004OB-MU for qemu-devel@nongnu.org; Tue, 04 Nov 2008 09:07:09 -0500 Received: by yw-out-1718.google.com with SMTP id 6so1076890ywa.82 for ; Tue, 04 Nov 2008 06:07:08 -0800 (PST) Message-ID: <49105704.9070708@codemonkey.ws> Date: Tue, 04 Nov 2008 08:07:00 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Fix alarm_timer race with select References: <490FFF23.5020704@web.de> In-Reply-To: <490FFF23.5020704@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 Jan Kiszka wrote: > 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 | 20 +++++++++++++++++++- > 1 file changed, 19 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,14 @@ 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)) { > + perror("creating timer pipe"); > + exit(1); > + } > + alarm_timer_rfd = fds[0]; > + alarm_timer_wfd = fds[1]; > It's important to have the write file descriptor also be non-blocking, otherwise the signal handler could block indefinitely. Getting EAGAIN in the signal handler is fine too since you only care that there is something to be read from the pipe. If you get an EAGAIN, you can be assured there is something in the pipe. > for (i = 0; alarm_timers[i].name; i++) { > t = &alarm_timers[i]; > @@ -4427,6 +4439,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); > FD_ZERO(&wfds); > FD_ZERO(&xfds); > for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) { > @@ -4500,6 +4513,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); > } > Perhaps we should move the alarm timer check rearming out of the main loop and into a qemu_set_fd_handler2() handler? Regards, Anthony Liguori