qemu-devel
[Top][All Lists]
Advanced

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

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


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

Jan Kiszka wrote:
Anthony Liguori wrote:
Anthony Liguori wrote:
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 <address@hidden>
---
 vl.c |   21 ++++++++++++++++++++-
 1 file changed, 20 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,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];
@@ -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.

Regards,

Anthony Liguori





reply via email to

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