[Qemu-devel] [PATCH] Fix alarm_timer race with select - v3

[Qemu-devel] [PATCH] Fix alarm_timer race with select - v3

[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 <jan.kiszka@web.de>
---
 vl.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

Index: b/vl.c
===================================================================
--- a/vl.c
+++ 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);
     }
-- 
Siemens AG, Corporate Technology, CT SE 2 ES-OS
Corporate Competence Center Embedded Linux

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

[Blue Swirl]

On 11/5/08, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> [ 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 <jan.kiszka@web.de>

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

For the write case, we could save one initialization write access to
the "byte" for every alarm trigger if it's static const.

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

[Jan Kiszka]

Blue Swirl wrote:
> On 11/5/08, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> [ 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 <jan.kiszka@web.de>
> 
>>  @@ -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));
> 
> 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 <jan.kiszka@web.de>
---
 vl.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

Index: b/vl.c
===================================================================
--- a/vl.c
+++ 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);
     }

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

[Anthony Liguori]

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, 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>
>   

Applied.  Thanks.

Regards,

Anthony Liguori