On Thu, Jul 18, 2013 at 07:48:28PM +0100, Alex Bligh wrote: > Stefan, > > --On 17 July 2013 11:02:30 +0800 Stefan Hajnoczi <stefa...@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