Fix alarm_timer race with select

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

---
 vl.c |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Index: b/vl.c
===================================================================

vl.c +5 -1

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

vl.c +1 -0

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

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?

Regards,

Anthony Liguori

Quoted 4 lines · Anthony Liguori

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.

Quoted 7 lines · Anthony Liguori

Quoted 5 lines · Anthony Liguori

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?

Quoted 4 lines

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.

Quoted 11 lines · Anthony Liguori

Quoted 5 lines · Anthony Liguori

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

---
 vl.c |   21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Index: b/vl.c
===================================================================

vl.c +14 -1

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

Quoted 84 lines · Jan Kiszka

Quoted 11 lines · Anthony Liguori

Quoted 5 lines · Anthony Liguori

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

---
 vl.c |   21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Index: b/vl.c
===================================================================

vl.c +5 -1

@@ -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) {

vl.c +9 -0

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

vl.c void main_loop_wait(int timeout) · line 4444

Quoted 8 lines

vl.c +1 -0

     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.

vl.c static void init_timer_alarm(void) · line 1683

Quoted 2 lines · Jan Kiszka

vl.c +2 -0

+    if (pipe(fds) || fcntl(fds[0], F_SETFL, O_NONBLOCK)
+        || fcntl(fds[1], F_SETFL, O_NONBLOCK)) {

Is this a portable way to set O_NONBLOCK on all host platforms? I'm under the impression you need to ues F_GETFL first, in case other flags are important to the functioning of the file descriptor.