Re: [Qemu-devel] [PATCHv2] [RFC 6/7] aio / timers: Switch to ppoll, run AioContext timers in aio_poll/aio_dispatch

[Alex Bligh]

Stefan,

I missed a couple of comments.

--On 25 July 2013 11:33:43 +0200 Stefan Hajnoczi <address@hidden> wrote:

> > @@ -214,6 +221,15 @@ bool aio_poll(AioContext *ctx, bool blocking)
> >          events[ret - WAIT_OBJECT_0] = events[--count];
> >      }
> >
> > +    if (blocking) {
> > +        /* Run the timers a second time. We do this because otherwise
> > aio_wait +         * will not note progress - and will stop a drain
> > early - if we have +         * a timer that was not ready to run
> > entering g_poll but is ready +         * after g_poll. This will only do
> > anything if a timer has expired. +         */
> > +        progress |= qemu_run_timers(ctx->clock);
> > +    }
> > +
> >      assert(progress || busy);
> >      return true;
>
> You didn't update this to return just progress.

Will fix

> >  }
> > diff --git a/async.c b/async.c
> > index 0d41431..cb6b1d4 100644
> > --- a/async.c
> > +++ b/async.c
> > @@ -123,13 +123,16 @@ aio_ctx_prepare(GSource *source, gint    *timeout)
> >  {
> >      AioContext *ctx = (AioContext *) source;
> >      QEMUBH *bh;
> > +    int deadline;
> >
> >      for (bh = ctx->first_bh; bh; bh = bh->next) {
> >          if (!bh->deleted && bh->scheduled) {
> >              if (bh->idle) {
> >                  /* idle bottom halves will be polled at least
> >                   * every 10ms */
> > -                *timeout = 10;
> > +                if ((*timeout < 0) || (*timeout > 10)) {
> > +                    *timeout = 10;
> > +                }
>
> Use the function you introduced earlier to return the nearest timeout?

The function I introduced takes an int64_t not an int. I think that's OK
though.

> >              } else {
> >                  /* non-idle bottom halves will be executed
> >                   * immediately */
> > @@ -139,6 +142,15 @@ aio_ctx_prepare(GSource *source, gint    *timeout)
> >          }
> >      }
> >
> > +    deadline =
> > qemu_timeout_ns_to_ms(qemu_clock_deadline_ns(ctx->clock)); +    if
> > (deadline == 0) {
> > +        *timeout = 0;
> > +        return true;
> > +    } else if ((deadline > 0) &&
> > +               ((*timeout < 0) || (deadline < *timeout))) {
> > +        *timeout = deadline;
>
> Same here.

Ditto

> > @@ -170,9 +171,13 @@ static void glib_pollfds_fill(uint32_t *cur_timeout)
> >                                   glib_n_poll_fds);
> >      } while (n != glib_n_poll_fds);
> >
> > -    if (timeout >= 0 && timeout < *cur_timeout) {
> > -        *cur_timeout = timeout;
> > +    if (timeout < 0) {
> > +        timeout_ns = -1;
> > +    } else {
> > +      timeout_ns = (int64_t)timeout * (int64_t)SCALE_MS;
>
> Indentation.

Will fix

--
Alex Bligh