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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2825 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

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

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

[Anthony Liguori]

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.

Regards,

Anthony Liguori

> Regards,
>
> Anthony Liguori

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3496 bytes --]

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 <jan.kiszka@web.de>
---
 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);
     FD_ZERO(&wfds);
     FD_ZERO(&xfds);
     for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
@@ -4500,6 +4514,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);
     }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Anthony Liguori]

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

Yeah, I can take care of that.  This is general cleanup that I've wanted 
to do to the main loop.

> Find the O_NONBLOCK remark addressed below.
>   

Great!  I'll give it a spin.

Regards,

Anthony Liguori

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

[Anthony Liguori]

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 <jan.kiszka@web.de>
> ---
>  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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3603 bytes --]

Anthony Liguori wrote:
> 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 <jan.kiszka@web.de>
>> ---
>>  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.

Oh, yes.

>  While you're at it, please don't call pipe and fcntl() twice
> from inside of an if() clause.

??? Don't see your concern yet.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 258 bytes --]

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

[Jamie Lokier]

Jan Kiszka wrote:
> +    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.

-- Jamie

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

Jamie Lokier wrote:
> Jan Kiszka wrote:
>> +    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.

[ Checking the spec ] That's likely true, read-modify-write is
recommended. Can changes this. Consequently, quite a few other spots in
qemu should then be changed as well...

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]