Skip to contentSkip to conversation

Fix alarm_timer race with select - v3

PATCH Applied Jan Kiszka 3 participants 4 messages Nov 5, 2008

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

PATCH 1/1 Fix alarm_timer race with select - v3

[ 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 <[email protected]>
---
 vl.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)
vl.c +26 -2
Index: 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;
+        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);
     }
Signature · 2 lines
Siemens AG, Corporate Technology, CT SE 2 ES-OS
Corporate Competence Center Embedded Linux
#1 lore raw
Quoted 26 lines · Jan Kiszka

[ 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 <[email protected]>

@@ -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);
Quoted 4 lines

#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.

author
#2 lore raw
Quoted 30 lines · Blue Swirl
Quoted 29 lines · Jan Kiszka

[ 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 <[email protected]>

@@ -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;

Quoted 5 lines
Quoted 1 line

+ 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 <[email protected]>
---
 vl.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)
vl.c +26 -2
Index: 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);
     }
Applied
#3 lore raw
Quoted 7 lines · 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.

Quoted 7 lines

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 <[email protected]>

Applied. Thanks.

Regards,

Anthony Liguori