[Qemu-devel] [5633] Fix alarm_timer race with select - v3 (Jan Kiszka)

[Qemu-devel] [5633] Fix alarm_timer race with select - v3 (Jan Kiszka)

[Anthony Liguori]

Revision: 5633
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5633
Author:   aliguori
Date:     2008-11-05 20:29:45 +0000 (Wed, 05 Nov 2008)

Log Message:
-----------
Fix alarm_timer race with select - v3 (Jan Kiszka)

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 <jan.kiszka@web.de>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Modified Paths:
--------------
    trunk/vl.c

Modified: trunk/vl.c
===================================================================
--- trunk/vl.c	2008-11-05 20:24:35 UTC (rev 5632)
+++ trunk/vl.c	2008-11-05 20:29:45 UTC (rev 5633)
@@ -885,6 +885,7 @@
 #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 @@
                                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,7 +1678,21 @@
 {
     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 @@
 
     /* 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 @@
                     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);
     }

Re: [Qemu-devel] [5633] Fix alarm_timer race with select - v3 (Jan Kiszka)

[Anthony Liguori]

> @@ -1674,7 +1678,21 @@
>  {
>      struct qemu_alarm_timer *t = NULL;
>      int i, err = -1;
> +    int fds[2];
>  
> +    if (pipe(fds) < 0) {
> +    fail:
> +        perror("creating timer pipe");
> +        exit(1);
> +    }
>   

pipe() doesn't exist on Windows apparently.  I'm open to suggestions on 
how to fix this properly.  host_alarm_handler isn't a signal handler on 
Windows so perhaps we can just not to the pipe trickery on Windows.

Thoughts?

Regards,

Anthony Liguori

> +    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 @@
>  
>      /* 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 @@
>                      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);
>      }
>
>
>
>
>