qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] Fix alarm_timer race with select


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH] Fix alarm_timer race with select
Date: Tue, 04 Nov 2008 08:07:00 -0600
User-agent: Thunderbird 2.0.0.17 (X11/20080925)

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 <address@hidden>
---
 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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]