Re: [Qemu-devel] [RFC v2 3/5] timer: make qemu_clock_enable sync between disable and timer's cb

[liu ping fan]

On Mon, Jul 29, 2013 at 2:30 PM, Paolo Bonzini <address@hidden> wrote:
> Il 29/07/2013 05:16, Liu Ping Fan ha scritto:
>> After disabling the QemuClock, we should make sure that no QemuTimers
>> are still in flight. To implement that, the caller of disabling will
>> wait until the last user's exit.
>>
>> Note, the callers of qemu_clock_enable() should be sync by themselves,
>> not protected by this patch.
>>
>> Signed-off-by: Liu Ping Fan <address@hidden>
>
> This is an interesting approach.
>
>> -    if (!clock->enabled)
>> -        return;
>> +    atomic_inc(&clock->using);
>> +    if (unlikely(!clock->enabled)) {
>> +        goto exit;
>> +    }
>
> This can return directly, it doesn't need to increment and decrement
> clock->using.
>
Here is a race condition like the following, so I swap the sequence

     qemu_clock_enable(false)           run timers


if(!clock->enabled)  return;
                                      ------------>
  clock->enabled=false
  if(atomic_read(using)==0)
                                                        atomic_inc(using)


Regards,
Pingfan
> Paolo
>
>>
>>      current_time = qemu_get_clock_ns(clock);
>>      tlist = clock_to_timerlist(clock);
>> @@ -461,6 +482,15 @@ void qemu_run_timers(QEMUClock *clock)
>>          /* run the callback (the timer list can be modified) */
>>          ts->cb(ts->opaque);
>>      }
>> +
>> +exit:
>> +    qemu_mutex_lock(&clock->lock);
>> +    if (atomic_fetch_dec(&clock->using) == 1) {
>> +        if (unlikely(!clock->enabled)) {
>> +            qemu_cond_signal(&clock->wait_using);
>> +        }
>> +    }
>> +    qemu_mutex_unlock(&clock->lock);
>>  }
>>
>>  int64_t qemu_get_clock_ns(QEMUClock *clock)
>>
>