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

[Jan Kiszka <jan.kiszka@web.de>]

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