Re: [Qemu-devel] [PATCHv2] [RFC 1/7] aio / timers: Remove alarm timers

[Paolo Bonzini]

Il 25/07/2013 11:10, Stefan Hajnoczi ha scritto:
> On Sat, Jul 20, 2013 at 07:06:37PM +0100, Alex Bligh wrote:
>> Remove alarm timers from qemu-timers.c in anticipation of using
>> timeouts for g_poll / p_poll instead.
>>
>> Signed-off-by: Alex Bligh <address@hidden>
>> ---
>>  include/qemu/timer.h |    2 -
>>  main-loop.c          |    4 -
>>  qemu-timer.c         |  501 
>> +-------------------------------------------------
>>  vl.c                 |    5 +-
>>  4 files changed, 7 insertions(+), 505 deletions(-)
> 
> This should be one of the last patches so qemu.git remains bisectable.
> Only remove the alarm timer once the event loops are already using the
> timeout argument.
> 
>> @@ -245,11 +82,7 @@ static QEMUClock *qemu_new_clock(int type)
>>  
>>  void qemu_clock_enable(QEMUClock *clock, bool enabled)
>>  {
>> -    bool old = clock->enabled;
>>      clock->enabled = enabled;
>> -    if (enabled && !old) {
>> -        qemu_rearm_alarm_timer(alarm_timer);
>> -    }
> 
> If this function is supposed to work when called from another thread
> (e.g. vcpu thread), then you need to call qemu_notify_event().  For
> AioContext clocks that should be aio_notify() with the relevant
> AioContext, but we don't need that yet.

In general QEMUClock should have an AioContext pointer so that it can
call aio_notify.

Paolo

>>  }
>>  
>>  int64_t qemu_clock_has_timers(QEMUClock *clock)
>> @@ -340,10 +173,9 @@ void qemu_mod_timer_ns(QEMUTimer *ts, int64_t 
>> expire_time)
>>  
>>      /* Rearm if necessary  */
>>      if (pt == &ts->clock->active_timers) {
>> -        if (!alarm_timer->pending) {
>> -            qemu_rearm_alarm_timer(alarm_timer);
>> -        }
>> -        /* Interrupt execution to force deadline recalculation.  */
>> +        /* Interrupt execution to force deadline recalculation.
>> +         * FIXME: Do we need to do this now?
>> +         */
>>          qemu_clock_warp(ts->clock);
>>          if (use_icount) {
>>              qemu_notify_event();
> 
> Same here.
> 
>> diff --git a/vl.c b/vl.c
>> index 25b8f2f..612c609 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3714,7 +3714,10 @@ int main(int argc, char **argv, char **envp)
>>                  old_param = 1;
>>                  break;
>>              case QEMU_OPTION_clock:
>> -                configure_alarms(optarg);
>> +                /* Once upon a time we did:
>> +                 *   configure_alarms(optarg);
>> +                 * here. This is stubbed out for compatibility.
>> +                 */
>>                  break;
> 
> This could be made clearer to say outright that the options don't exist
> anymore:
> 
> /* Clock options no longer exist.  Keep this option for backward
>  * compatibility.
>  */
>