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

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Jul 18, 2013 at 07:48:28PM +0100, Alex Bligh wrote:
> Stefan,
> 
> --On 17 July 2013 11:02:30 +0800 Stefan Hajnoczi <stefanha@gmail.com> wrote:
> 
> >The steps to achieving this:
> >
> >1. Drop alarm timers from qemu-timer.c and calculate g_poll() timeout
> >   instead for the main loop.
> >
> >2. Introduce a per-AioContext aio_ctx_clock that can be used with
> >   qemu_new_timer() to create a QEMUTimer that expires during
> >   aio_poll().
> >
> >3. Calculate g_poll() timeout for aio_ctx_clock in aio_poll().
> 
> I've pretty much written this.
> 
> Two issues:
> 
> 1. I very much enjoyed deleting all the alarm timers code. However, it was
>   doing something vaguely useful, which was calling qemu_notify_event
>   when the timer expired. Under the new regime above, if the AioContext
>   clock's timers expires within aio_poll, life is good as the timeout
>   to g_poll will expire. However, this won't apply if any of the other
>   three static clock's timers expire. Also, it won't work within the
>   mainloop poll. Also, we lose the ability to respond to timers in a sub
>   millisecond way.
> 
>   Options:
> 
>   a) restore alarm timers (at least for the time being). Make all
>      alarm timers do qemu_notify_event. However, only run the AioContext
>      clock's timers within aio_poll. This is the least intrusive change.
> 
>   b) calculate the timeout in aio_poll with respect to the minimum
>      deadline across all clocks, not just AioContext's clock. Use the
>      same logic in mainloop.
> 
>   I'd go for (b), except for the millisecond accuracy thing. So my
>   temptation (sadly) is (a).

I think this is a non-issue because host_alarm_handler() can only be
called from the main loop:

main-loop.c:qemu_signal_init() sets up signalfd to monitor SIGALRM.
Therefore we do not asynchronously invoke the SIGALRM signals handler.
It is only invoked from main-loop.c:sigfd_handler() when the main loop
runs.

That's how I read the code.  I haven't checked a running process to be
sure.

> 2. If we do delete alarm timers, I'll need to delete the -clock option.

I noticed this too because I think we should stub it out for
compatibility.  Accept existing options but ignore them, update
documentation to state that they are kept for compatibility.

Stefan