Re: [Qemu-devel] [PATCH] [RFC] aio/async: Add timed bottom-halves

[Alex Bligh]

Paolo,

> > 3. aio_poll calls aio_bh_poll. If this returns true, this indicates
> >   at least one non-idle bh exists, which causes aio_poll not to
> >   block.
>
> No, this indicates that at least one scheduled non-idle bh exist*ed*,
> which causes aio_poll not to block (because some progress has been done).

Ah yes, in the sense the callback will have been called and it now
doesn't exist.

> > 4. aio_poll then calls g_poll (POSIX) or WaitForMultipleObjects
> >   (Windows). However, the timeout is either 0 or infinite.
> >   Both functions take a milliseconds (yuck) timeout, but that
> >   is not used.
>
> I agree with the yuck. :)  But Linux has the nanoseconds-resolution
> ppoll, too.

Sadly I don't think we have a g_ppoll.

> > So, the first thing I don't understand is why aio_poll needs the
> > return value of aio_bh_poll at all. Firstly, after sampling it,
> > it then causes aio_dispatch, and that can presumably set its own
> > bottom half callbacks; if this happens 'int blocking' won't be
> > cleared, and it will still enter g_poll with an infinite timeout.
> > Secondly, there seems to be an entirely separate mechanism
> > (aio_notify) in any case. If a non-idle bh has been scheduled,
> > this will cause g_poll to exit immediately as a read will be
> > ready. I believe this is cleared by the bh being used.

Sorry - forgot to delete the questions from above when I moved
them down below. Yes that make sense.

> > The second thing I don't understand is why we aren't using
> > the timeout on g_poll / WaitForMultipleObjects.
>
> Because so far it wasn't needed (insert rant about idle BHs being a
> hack).  This is a good occasion to use it.

OK

> But I wouldn't introduce a
> new one-off concept (almost as much of a hack as idle BHs), I would
> rather reuse as much code as possible from QEMUTimer/QEMUClock.  I must
> admit I don't have a clear idea of how the API would look like.

So the reason I was trying to avoid using QEMUTimer stuff was that
bh's get called from aio_poll and it was not evident that all timers
would be safe to call from aio_poll.

My original approach was simply to call qemu_run_all_timers from
aio_poll at the top. I now know I'd have to (slightly) modify
the poll routine too use a specific timeout if there are any timers
ready to run and the timeout would otherwise be infinity. However
Stefan H pointed out on IRC that we'd then have to audit every
timer usage to check it was safe to call from aio_poll, or have
a 'isSafeToCallFromAioPoll' type flag (yuck). Hence we thought the
timed bh would be less intrusive as actually what I and other block
driver folks might want is something more like a bh. Equally happy
to recode it as a QEMUTimer.

What do you think? In the end I thought the schedule_bh_at stuff
was simpler.

> > It would
> > seem to be reasonably easy to make aio_poll call aio_ctx_prepare
> > or something that does the same calculation. This would fix
> > idle bh's to be more reliable (we know it's safe to call them
> > within aio_poll anyway, it's just a question of whether
> > we turn an infinite wait into a 10ms wait).
>
> Idle BHs could be changed to timers as well, and then they would
> disappear.

Yes I was in essence hoping an idle BH would be treated as an
epsilon millisecond timed bh, or (if above) an epsilon millisecond
timer. By that I mean poll() would be called at least once before
it ran, but with a 0 ms timeout.

--
Alex Bligh