Re: [Qemu-devel] [RFC] [PATCHv6 00/16] aio / timers: Add AioContext timers and use ppoll

[Stefan Hajnoczi]

On Tue, Aug 06, 2013 at 10:16:16AM +0100, Alex Bligh wrote:
> This patch series adds support for timers attached to an AioContext clock
> which get called within aio_poll.
> 
> In doing so it removes alarm timers and moves to use ppoll where possible.
> 
> This patch set 'sort of' passes make check (see below for caveat)
> including a new test harness for the aio timers, but has not been
> tested much beyond that. In particular, the win32 changes have not
> even been compile tested. Equally, alterations to use_icount
> are untested.
> 
> Caveat: I have had to alter tests/test-aio.c so the following error
> no longer occurs.
> 
> ERROR:tests/test-aio.c:346:test_wait_event_notifier_noflush: assertion 
> failed: (aio_poll(ctx, false))
> 
> As gar as I can tell, this check was incorrect, in that it checking
> aio_poll makes progress when in fact it should not make progress. I
> fixed an issue where aio_poll was (as far as I can tell) wrongly
> returning true on a timeout, and that generated this error.
> 
> Note also the comment on patch 15 in relation to a possible bug
> in cpus.c.
> 
> Changes since v5:
> * Rebase onto master (b9ac5d9)
> * Fix spacing in typedef QEMUTimerList
> * Rename 'QEMUClocks' extern to 'qemu_clocks'
> 
> Changes since v4:
> * Rename qemu_timerlist_ functions to timer_list (per Paolo Bonzini)
> * Rename qemu_timer_.*timerlist.* to timer_ (per Paolo Bonzini)
> * Use enum for QEMUClockType
> * Put clocks into an array; remove global variables
> * Introduce QEMUTimerListGroup - a timeliest of each type
> * Add a QEMUTimerListGroup to AioContext
> * Use a callback on timer modification, rather than binding in
>   AioContext into the timeliest
> * Make cpus.c iterate over all timerlists when it does a notify
> * Make cpus.c icount timeout use soonest timeout
>   across all timerlists
> 
> Changes since v3:
> * Split up QEMUClock and QEMUClock list
> * Improve commenting
> * Fix comment in vl.c
> * Change test/test-aio.c to reflect correct behaviour in aio_poll.
> 
> Changes since v2:
> * Reordered to remove alarm timers last
> * Added prctl(PR_SET_TIMERSLACK, 1, ...)
> * Renamed qemu_g_poll_ns to qemu_poll_ns
> * Moved declaration of above & drop glib types
> * Do not use a global list of qemu clocks
> * Add AioContext * to QEMUClock
> * Split up conversion to use ppoll and timers
> * Indentation fix
> * Fix aio_win32.c aio_poll to return progress
> * aio_notify / qemu_notify when timers are modified
> * change comment in deprecation of clock options
> 
> Alex Bligh (16):
>   aio / timers: add qemu-timer.c utility functions
>   aio / timers: add ppoll support with qemu_poll_ns
>   aio / timers: Add prctl(PR_SET_TIMERSLACK, 1, ...) to reduce timer
>     slack
>   aio / timers: Make qemu_run_timers and qemu_run_all_timers return
>     progress
>   aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList
>   aio / timers: Untangle include files
>   aio / timers: Add QEMUTimerListGroup and helper functions
>   aio / timers: Add QEMUTimerListGroup to AioContext
>   aio / timers: Add a notify callback to QEMUTimerList
>   aio / timers: aio_ctx_prepare sets timeout from AioContext timers
>   aio / timers: Convert aio_poll to use AioContext timers' deadline
>   aio / timers: Convert mainloop to use timeout
>   aio / timers: On timer modification, qemu_notify or aio_notify
>   aio / timers: Use all timerlists in icount warp calculations
>   aio / timers: Remove alarm timers
>   aio / timers: Add test harness for AioContext timers
> 
>  aio-posix.c               |   20 +-
>  aio-win32.c               |   22 +-
>  async.c                   |   20 +-
>  configure                 |   37 +++
>  cpus.c                    |   44 ++-
>  dma-helpers.c             |    1 +
>  hw/dma/xilinx_axidma.c    |    1 +
>  hw/timer/arm_timer.c      |    1 +
>  hw/timer/grlib_gptimer.c  |    2 +
>  hw/timer/imx_epit.c       |    1 +
>  hw/timer/imx_gpt.c        |    1 +
>  hw/timer/lm32_timer.c     |    1 +
>  hw/timer/puv3_ost.c       |    1 +
>  hw/timer/slavio_timer.c   |    1 +
>  hw/timer/xilinx_timer.c   |    1 +
>  hw/usb/hcd-uhci.c         |    1 +
>  include/block/aio.h       |    4 +
>  include/block/block_int.h |    1 +
>  include/block/coroutine.h |    2 +
>  include/qemu/timer.h      |  122 ++++++-
>  main-loop.c               |   49 ++-
>  migration-exec.c          |    1 +
>  migration-fd.c            |    1 +
>  migration-tcp.c           |    1 +
>  migration-unix.c          |    1 +
>  migration.c               |    1 +
>  nbd.c                     |    1 +
>  net/net.c                 |    1 +
>  net/socket.c              |    1 +
>  qemu-coroutine-io.c       |    1 +
>  qemu-io-cmds.c            |    1 +
>  qemu-nbd.c                |    1 +
>  qemu-timer.c              |  803 
> ++++++++++++++++-----------------------------
>  slirp/misc.c              |    1 +
>  tests/test-aio.c          |  141 +++++++-
>  tests/test-thread-pool.c  |    3 +
>  thread-pool.c             |    1 +
>  ui/vnc-auth-vencrypt.c    |    2 +-
>  ui/vnc-ws.c               |    1 +
>  vl.c                      |    4 +-
>  40 files changed, 736 insertions(+), 564 deletions(-)
> 
> -- 
> 1.7.9.5
> 
> *** BLRUB STOP ***
> 
> Alex Bligh (16):
>   aio / timers: add qemu-timer.c utility functions
>   aio / timers: add ppoll support with qemu_poll_ns
>   aio / timers: Add prctl(PR_SET_TIMERSLACK, 1, ...) to reduce timer
>     slack
>   aio / timers: Make qemu_run_timers and qemu_run_all_timers return
>     progress
>   aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList
>   aio / timers: Untangle include files
>   aio / timers: Add QEMUTimerListGroup and helper functions
>   aio / timers: Add QEMUTimerListGroup to AioContext
>   aio / timers: Add a notify callback to QEMUTimerList
>   aio / timers: aio_ctx_prepare sets timeout from AioContext timers
>   aio / timers: Convert aio_poll to use AioContext timers' deadline
>   aio / timers: Convert mainloop to use timeout
>   aio / timers: On timer modification, qemu_notify or aio_notify
>   aio / timers: Use all timerlists in icount warp calculations
>   aio / timers: Remove alarm timers
>   aio / timers: Add test harness for AioContext timers
> 
>  aio-posix.c               |   20 +-
>  aio-win32.c               |   22 +-
>  async.c                   |   20 +-
>  configure                 |   37 +++
>  cpus.c                    |   44 ++-
>  dma-helpers.c             |    1 +
>  hw/dma/xilinx_axidma.c    |    1 +
>  hw/timer/arm_timer.c      |    1 +
>  hw/timer/grlib_gptimer.c  |    2 +
>  hw/timer/imx_epit.c       |    1 +
>  hw/timer/imx_gpt.c        |    1 +
>  hw/timer/lm32_timer.c     |    1 +
>  hw/timer/puv3_ost.c       |    1 +
>  hw/timer/slavio_timer.c   |    1 +
>  hw/timer/xilinx_timer.c   |    1 +
>  hw/usb/hcd-uhci.c         |    1 +
>  include/block/aio.h       |    4 +
>  include/block/block_int.h |    1 +
>  include/block/coroutine.h |    2 +
>  include/qemu/timer.h      |  122 ++++++-
>  main-loop.c               |   49 ++-
>  migration-exec.c          |    1 +
>  migration-fd.c            |    1 +
>  migration-tcp.c           |    1 +
>  migration-unix.c          |    1 +
>  migration.c               |    1 +
>  nbd.c                     |    1 +
>  net/net.c                 |    1 +
>  net/socket.c              |    1 +
>  qemu-coroutine-io.c       |    1 +
>  qemu-io-cmds.c            |    1 +
>  qemu-nbd.c                |    1 +
>  qemu-timer.c              |  803 
> ++++++++++++++++-----------------------------
>  slirp/misc.c              |    1 +
>  tests/test-aio.c          |  141 +++++++-
>  tests/test-thread-pool.c  |    3 +
>  thread-pool.c             |    1 +
>  ui/vnc-auth-vencrypt.c    |    2 +-
>  ui/vnc-ws.c               |    1 +
>  vl.c                      |    4 +-
>  40 files changed, 736 insertions(+), 564 deletions(-)

For anyone wishing to review this series, here's a diagram showing the
new relationships and a summary of this series:

http://vmsplice.net/~stefan/timerlist.jpg

TimerList is the list of QEMUTimers which are pending on a QEMUClock
clock source.

Previously QEMUClock contained this list inline but the goal is to let
AioContexts have their own independent timer lists.  This makes timers
much more thread-friendly since we don't work on global lists and
callbacks are dispatched in the same thread's AioContext where they were
created.

TimerListGroup holds a timerlist for each clock source ("rt", "vm", and
"host").  The main loop has the default TimerListGroup.  Each AioContext
has its own TimerListGroup.

The other major change is that QEMU now uses the event loop's poll()
timeout value instead of a separate OS-specific timer mechanism (aka the
"alarm timer").

Stefan