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

[Anthony Liguori <anthony@codemonkey.ws>]

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.
>
> 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 |   20 +++++++++++++++++++-
>  1 file changed, 19 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,14 @@ 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)) {
> +        perror("creating timer pipe");
> +        exit(1);
> +    }
> +    alarm_timer_rfd = fds[0];
> +    alarm_timer_wfd = fds[1];
>   

It's important to have the write file descriptor also be non-blocking, 
otherwise the signal handler could block indefinitely.  Getting EAGAIN 
in the signal handler is fine too since you only care that there is 
something to be read from the pipe.  If you get an EAGAIN, you can be 
assured there is something in the pipe.

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

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