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

[Stefan Hajnoczi]

On Thu, Aug 08, 2013 at 10:41:57PM +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 18 in relation to a possible bug
> in cpus.c.
> 
> The penultimate patch is patch which is created in an automated manner
> using scripts/switch-timer-api, added in this patch set. It violates some
> coding standards (e.g. line length >= 80 characters), but this is preferable
> in terms of giving a provably correct conversion.
> 
> This patch set has been compile tested & make check tested on a
> 'christmas-tree' configuration, meaning a configuration with every
> --enable- value tested that can be easily configured on Ubuntu Precise,
> after application of each patch.
> 
> Changes since v7:
> * Rebase to master 6fdf98f281f85ae6e2883bed2f691bcfe33b1f9f
> * Add qemu_clock_get_ms and qemu_clock_get_ms
> * Rename qemu_get_clock to qemu_clock_ptr
> * Reorder qemu-timer.h to utilise the legacy API
> * Hide qemu_clock_new & qemu_clock_free
> * Rename default_timerlist to main_loop_timerlist
> * Remove main_loop_timerlist once main_loop_tlg is in
> * Add script to convert to new API
> * Make rtc_clock use new API
> * Convert tests/test-aio to use new API
> * Run script on entire source code
> * Remove legacy API functions
> 
> Changes since v6:
> * Fix build failure in vnc-auth-sasl.c
> * Split first patch into 3
> * Add assert on timerlist_free
> * Fix ==/= error on qemu_clock_use_for_deadline
> * Remove unnecessary cast in aio_timerlist_notify
> * Fix bad deadline comparison in aio_ctx_check
> * Add assert to timerlist_new_from_clock to check init_clocks
> * Use timer_list not tl
> * Change default_timerlistgroup to main_loop_timerlistgroup
> * Add comment on commit for qemu_clock_use_for_deadline
> * Fixed various include file issues
> * Convert *_has_timers and *_has_expired to return bool
> * Make loop variable consistent when looping through clock types
> * Add documentation to existing qemu_timer calls
> * Remove qemu_clock_deadline and move to qemu_clock_deadline_ns
> 
> 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 (30):
>   aio / timers: Rename qemu_new_clock and expose clock types
>   aio / timers: add qemu-timer.c utility functions
>   aio / timers: Consistent treatment of disabled clocks for deadlines
>   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: Add aio_timer_new wrapper
>   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: Introduce new API qemu_timer_new and friends
>   aio / timers: Use all timerlists in icount warp calculations
>   aio / timers: Add documentation and new format calls
>   aio / timers: Remove alarm timers
>   aio / timers: Remove legacy qemu_clock_deadline &
>     qemu_timerlist_deadline
>   aio / timers: Add qemu_clock_get_ms and qemu_clock_get_ms
>   aio / timers: Rearrange timer.h & make legacy functions call
>     non-legacy
>   aio / timers: Remove main_loop_timerlist
>   aio / timers: Convert rtc_clock to be a QEMUClockType
>   aio / timers: convert block_job_sleep_ns and co_sleep_ns to new API
>   aio / timers: Add test harness for AioContext timers
>   aio / timers: Add scripts/switch-timer-api
>   aio / timers: Switch entire codebase to the new timer API
>   aio / timers: Remove legacy interface
> 
>  aio-posix.c                 |   20 +-
>  aio-win32.c                 |   22 +-
>  arch_init.c                 |   12 +-
>  async.c                     |   20 +-
>  audio/audio.c               |    6 +-
>  audio/noaudio.c             |    4 +-
>  audio/spiceaudio.c          |    4 +-
>  audio/wavaudio.c            |    2 +-
>  backends/baum.c             |   12 +-
>  block.c                     |   14 +-
>  block/backup.c              |    4 +-
>  block/commit.c              |    2 +-
>  block/iscsi.c               |   14 +-
>  block/mirror.c              |   10 +-
>  block/qed.c                 |   10 +-
>  block/stream.c              |    2 +-
>  blockdev.c                  |    2 +-
>  blockjob.c                  |    4 +-
>  configure                   |   37 ++
>  cpus.c                      |  138 +++++---
>  dma-helpers.c               |    1 +
>  hmp.c                       |    8 +-
>  hw/acpi/core.c              |    8 +-
>  hw/acpi/piix4.c             |    2 +-
>  hw/alpha/typhoon.c          |    2 +-
>  hw/arm/omap1.c              |   52 +--
>  hw/arm/pxa2xx.c             |   61 ++--
>  hw/arm/spitz.c              |    6 +-
>  hw/arm/stellaris.c          |   10 +-
>  hw/arm/strongarm.c          |   34 +-
>  hw/audio/adlib.c            |    2 +-
>  hw/audio/intel-hda.c        |    4 +-
>  hw/audio/sb16.c             |    6 +-
>  hw/block/fdc.c              |    6 +-
>  hw/block/nvme.c             |   20 +-
>  hw/block/pflash_cfi01.c     |    2 +-
>  hw/block/pflash_cfi02.c     |   10 +-
>  hw/bt/hci-csr.c             |    4 +-
>  hw/bt/hci.c                 |   38 +-
>  hw/bt/l2cap.c               |    8 +-
>  hw/char/cadence_uart.c      |   12 +-
>  hw/char/serial.c            |   22 +-
>  hw/char/virtio-serial-bus.c |   10 +-
>  hw/core/ptimer.c            |   18 +-
>  hw/display/qxl-logger.c     |    2 +-
>  hw/display/qxl.c            |    2 +-
>  hw/display/vga.c            |    6 +-
>  hw/dma/pl330.c              |    6 +-
>  hw/dma/rc4030.c             |    4 +-
>  hw/dma/soc_dma.c            |    8 +-
>  hw/dma/xilinx_axidma.c      |    1 +
>  hw/i386/kvm/apic.c          |    2 +-
>  hw/i386/kvm/i8254.c         |    6 +-
>  hw/i386/xen_domainbuild.c   |    6 +-
>  hw/ide/core.c               |    6 +-
>  hw/input/hid.c              |   10 +-
>  hw/input/lm832x.c           |    8 +-
>  hw/input/tsc2005.c          |   14 +-
>  hw/input/tsc210x.c          |   30 +-
>  hw/intc/apic.c              |   16 +-
>  hw/intc/apic_common.c       |    2 +-
>  hw/intc/armv7m_nvic.c       |   16 +-
>  hw/intc/i8259.c             |    4 +-
>  hw/mips/cputimer.c          |   12 +-
>  hw/misc/arm_sysctl.c        |    2 +-
>  hw/misc/macio/cuda.c        |   34 +-
>  hw/misc/macio/macio.c       |    4 +-
>  hw/misc/vfio.c              |   14 +-
>  hw/net/dp8393x.c            |   20 +-
>  hw/net/e1000.c              |   12 +-
>  hw/net/lan9118.c            |    4 +-
>  hw/net/pcnet-pci.c          |    4 +-
>  hw/net/pcnet.c              |   10 +-
>  hw/net/rtl8139.c            |   28 +-
>  hw/net/virtio-net.c         |   20 +-
>  hw/openrisc/cputimer.c      |   10 +-
>  hw/ppc/ppc.c                |   64 ++--
>  hw/ppc/ppc405_uc.c          |    8 +-
>  hw/ppc/ppc_booke.c          |   10 +-
>  hw/ppc/spapr.c              |    8 +-
>  hw/sd/sdhci.c               |   28 +-
>  hw/sparc64/sun4u.c          |   24 +-
>  hw/timer/arm_mptimer.c      |   12 +-
>  hw/timer/arm_timer.c        |    1 +
>  hw/timer/cadence_ttc.c      |    6 +-
>  hw/timer/etraxfs_timer.c    |    2 +-
>  hw/timer/exynos4210_mct.c   |    3 +-
>  hw/timer/exynos4210_pwm.c   |    1 +
>  hw/timer/grlib_gptimer.c    |    2 +
>  hw/timer/hpet.c             |   20 +-
>  hw/timer/i8254.c            |   26 +-
>  hw/timer/i8254_common.c     |    4 +-
>  hw/timer/imx_epit.c         |    1 +
>  hw/timer/imx_gpt.c          |    1 +
>  hw/timer/lm32_timer.c       |    1 +
>  hw/timer/m48t59.c           |   18 +-
>  hw/timer/mc146818rtc.c      |   44 +--
>  hw/timer/omap_gptimer.c     |   24 +-
>  hw/timer/omap_synctimer.c   |    2 +-
>  hw/timer/pl031.c            |   19 +-
>  hw/timer/puv3_ost.c         |    1 +
>  hw/timer/pxa2xx_timer.c     |   34 +-
>  hw/timer/sh_timer.c         |    1 +
>  hw/timer/slavio_timer.c     |    1 +
>  hw/timer/tusb6010.c         |   12 +-
>  hw/timer/twl92230.c         |   14 +-
>  hw/timer/xilinx_timer.c     |    1 +
>  hw/tpm/tpm_tis.c            |    1 +
>  hw/usb/hcd-ehci.c           |   10 +-
>  hw/usb/hcd-musb.c           |    6 +-
>  hw/usb/hcd-ohci.c           |   12 +-
>  hw/usb/hcd-uhci.c           |   15 +-
>  hw/usb/hcd-xhci.c           |   26 +-
>  hw/usb/host-libusb.c        |    6 +-
>  hw/usb/host-linux.c         |    6 +-
>  hw/usb/redirect.c           |   12 +-
>  hw/virtio/virtio-balloon.c  |    8 +-
>  hw/virtio/virtio-rng.c      |   14 +-
>  hw/watchdog/wdt_i6300esb.c  |    6 +-
>  hw/watchdog/wdt_ib700.c     |   10 +-
>  hw/xtensa/pic_cpu.c         |   10 +-
>  include/block/aio.h         |   23 ++
>  include/block/block_int.h   |    1 +
>  include/block/blockjob.h    |    2 +-
>  include/block/coroutine.h   |    4 +-
>  include/hw/acpi/acpi.h      |    2 +-
>  include/qemu/ratelimit.h    |    2 +-
>  include/qemu/timer.h        |  793 ++++++++++++++++++++++++++++++++++++++---
>  include/sysemu/sysemu.h     |    2 +-
>  main-loop.c                 |   51 ++-
>  migration-exec.c            |    1 +
>  migration-fd.c              |    1 +
>  migration-tcp.c             |    1 +
>  migration-unix.c            |    1 +
>  migration.c                 |   17 +-
>  monitor.c                   |    8 +-
>  nbd.c                       |    1 +
>  net/dump.c                  |    2 +-
>  net/net.c                   |    1 +
>  net/socket.c                |    1 +
>  pc-bios/slof.bin            |  Bin 909720 -> 909720 bytes
>  qemu-char.c                 |    2 +-
>  qemu-coroutine-io.c         |    1 +
>  qemu-coroutine-sleep.c      |   10 +-
>  qemu-io-cmds.c              |    1 +
>  qemu-nbd.c                  |    1 +
>  qemu-timer.c                |  826 
> +++++++++++++++----------------------------
>  qtest.c                     |   10 +-
>  savevm.c                    |   22 +-
>  scripts/switch-timer-api    |  178 ++++++++++
>  slirp/if.c                  |    2 +-
>  slirp/misc.c                |    1 +
>  slirp/slirp.c               |    4 +-
>  stubs/clock-warp.c          |    2 +-
>  target-alpha/sys_helper.c   |   12 +-
>  target-ppc/kvm.c            |    8 +-
>  target-ppc/kvm_ppc.c        |    6 +-
>  target-s390x/cpu.c          |    4 +-
>  target-s390x/misc_helper.c  |    6 +-
>  target-xtensa/op_helper.c   |    2 +-
>  tests/libqtest.h            |   24 +-
>  tests/test-aio.c            |  143 +++++++-
>  tests/test-thread-pool.c    |    3 +
>  thread-pool.c               |    1 +
>  ui/console.c                |   30 +-
>  ui/input.c                  |    6 +-
>  ui/spice-core.c             |   10 +-
>  ui/vnc-auth-sasl.h          |    1 +
>  ui/vnc-auth-vencrypt.c      |    2 +-
>  ui/vnc-ws.c                 |    1 +
>  vl.c                        |   14 +-
>  xen-all.c                   |   12 +-
>  172 files changed, 2370 insertions(+), 1432 deletions(-)
>  create mode 100755 scripts/switch-timer-api

Overall this looks good and I've played around with the git branch,
including running git rebase -i -x make to ensure that each commit
builds.

I posted comments on a few patches.

qemu.git/master opens for QEMU 1.7 patches at the end of next week.
This looks like good timing - the next revision could be merged and then
we'll have the QEMU 1.7 cycle to post any follow-up patches.

Thanks for working through this, I can now focus on enabling I/O
throttling in my dataplane block layer work!

Stefan