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

[Alex Bligh]

Stefan,

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

OK

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

Each AioContext knows which clock it has but each clock doesn't know if
it's part of an AioContext. I suggest this is infrequent enough that always
using qemu_notify_event() would be OK. That should interrupt any poll.

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

I think I just need to delete the FIXME comment.

> > 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.
>  */

OK

--
Alex Bligh