Re: [Qemu-devel] [PATCH v4 3/5] icount: decouple warp calls

[Paolo Bonzini]

On 10/03/2016 12:56, Pavel Dovgalyuk wrote:
> qemu_clock_warp function is called to update virtual clock when CPU
> is sleeping. This function includes replay checkpoint to make execution
> deterministic in icount mode.
> Record/replay module flushes async event queue at checkpoints.
> Some of the events (e.g., block devices operations) include interaction
> with hardware. E.g., APIC polled by block devices sets one of IRQ flags.
> Flag to be set depends on currently executed thread (CPU or iothread).
> Therefore in replay mode we have to process the checkpoints in the same thread
> as they were recorded.
> qemu_clock_warp function (and its checkpoint) may be called from different
> thread. This patch decouples two different execution cases of this function:
> call when CPU is sleeping from iothread and call from cpu thread to update
> virtual clock.
> First task is performed by qemu_start_warp_timer function. It sets warp
> timer event to the moment of nearest pending virtual timer.
> Second function (qemu_account_warp_timer) is called from cpu thread
> before execution of the code. It advances virtual clock by adding the length
> of period while CPU was sleeping.
> 
> Signed-off-by: Pavel Dovgalyuk <address@hidden>

Lovely. :)  One question, why doesn't icount_dummy_timer need a checkpoint?

Only needs a change to the documentation:

diff --git a/docs/replay.txt b/docs/replay.txt
index 149727e..26dfb6e 100644
--- a/docs/replay.txt
+++ b/docs/replay.txt
@@ -134,11 +134,18 @@ of time. That's why we do not process a group of timers 
until the checkpoint
 event will be read from the log. Such an event allows synchronizing CPU
 execution and timer events.
 
-Another checkpoints application in record/replay is instruction counting
-while the virtual machine is idle. This function (qemu_clock_warp) is called
-from the wait loop. It changes virtual machine state and must be deterministic
-then. That is why we added checkpoint to this function to prevent its
-operation in replay mode when it does not correspond to record mode.
+Two other checkpoints govern the "warping" of the virtual clock.  While
+the virtual machine is idle, the virtual clock increments at 1 ns per
+*real time* nanosecond.  This is done by setting up a timer (called the
+warp timer) and then incrementing the virtual clock (called "warping"
+the virtual clock) as soon as the CPUs need to go out of the idle state.
+These actions change virtual machine state and must be deterministic.
+Two functions are used for this purpose, and each of them creates a
+checkpoint.  qemu_start_warp_timer checks if the CPUs are idle and if so
+starts accounting real time to virtual clock.  qemu_account_warp_timer
+is called when the CPUs get an interrupt or when a virtual clock timer
+fires, and it warps the virtual clock by the amount of real time that
+has passed since qemu_start_warp_timer.
 
 Bottom halves
 -------------



Paolo

> ---
>  cpus.c                  |   53 
> +++++++++++++++++++++++++++--------------------
>  include/qemu/timer.h    |   14 +++++++++---
>  include/sysemu/replay.h |    3 ++-
>  main-loop.c             |    2 +-
>  qemu-timer.c            |    4 +++-
>  stubs/clock-warp.c      |    2 +-
>  6 files changed, 48 insertions(+), 30 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 85d0f87..3ab9e04 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -373,6 +373,7 @@ static void icount_warp_rt(void)
>  static void icount_dummy_timer(void *opaque)
>  {
>      (void)opaque;
> +    icount_warp_rt();
>  }
>  
>  void qtest_clock_warp(int64_t dest)
> @@ -396,17 +397,12 @@ void qtest_clock_warp(int64_t dest)
>      qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>  }
>  
> -void qemu_clock_warp(QEMUClockType type)
> +void qemu_start_warp_timer(void)
>  {
>      int64_t clock;
>      int64_t deadline;
>  
> -    /*
> -     * There are too many global variables to make the "warp" behavior
> -     * applicable to other clocks.  But a clock argument removes the
> -     * need for if statements all over the place.
> -     */
> -    if (type != QEMU_CLOCK_VIRTUAL || !use_icount) {
> +    if (!use_icount) {
>          return;
>      }
>  
> @@ -418,29 +414,17 @@ void qemu_clock_warp(QEMUClockType type)
>      }
>  
>      /* warp clock deterministically in record/replay mode */
> -    if (!replay_checkpoint(CHECKPOINT_CLOCK_WARP)) {
> +    if (!replay_checkpoint(CHECKPOINT_CLOCK_WARP_START)) {
>          return;
>      }
>  
> -    if (icount_sleep) {
> -        /*
> -         * If the CPUs have been sleeping, advance QEMU_CLOCK_VIRTUAL timer 
> now.
> -         * This ensures that the deadline for the timer is computed correctly
> -         * below.
> -         * This also makes sure that the insn counter is synchronized before
> -         * the CPU starts running, in case the CPU is woken by an event other
> -         * than the earliest QEMU_CLOCK_VIRTUAL timer.
> -         */
> -        icount_warp_rt();
> -        timer_del(icount_warp_timer);
> -    }
>      if (!all_cpu_threads_idle()) {
>          return;
>      }
>  
>      if (qtest_enabled()) {
>          /* When testing, qtest commands advance icount.  */
> -     return;
> +        return;
>      }
>  
>      /* We want to use the earliest deadline from ALL vm_clocks */
> @@ -496,6 +480,31 @@ void qemu_clock_warp(QEMUClockType type)
>      }
>  }
>  
> +void qemu_account_warp_timer(void)
> +{
> +    int64_t clock;
> +    int64_t warp_delta;
> +
> +    if (!use_icount || !icount_sleep) {
> +        return;
> +    }
> +
> +    /* Nothing to do if the VM is stopped: QEMU_CLOCK_VIRTUAL timers
> +     * do not fire, so computing the deadline does not make sense.
> +     */
> +    if (!runstate_is_running()) {
> +        return;
> +    }
> +
> +    /* warp clock deterministically in record/replay mode */
> +    if (!replay_checkpoint(CHECKPOINT_CLOCK_WARP_ACCOUNT)) {
> +        return;
> +    }
> +
> +    timer_del(icount_warp_timer);
> +    icount_warp_rt();
> +}
> +
>  static bool icount_state_needed(void *opaque)
>  {
>      return use_icount;
> @@ -1496,7 +1505,7 @@ static void tcg_exec_all(void)
>      int r;
>  
>      /* Account partial waits to QEMU_CLOCK_VIRTUAL.  */
> -    qemu_clock_warp(QEMU_CLOCK_VIRTUAL);
> +    qemu_account_warp_timer();
>  
>      if (next_cpu == NULL) {
>          next_cpu = first_cpu;
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index d0946cb..21ffec6 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -210,12 +210,18 @@ void qemu_clock_notify(QEMUClockType type);
>  void qemu_clock_enable(QEMUClockType type, bool enabled);
>  
>  /**
> - * qemu_clock_warp:
> - * @type: the clock type
> + * qemu_start_warp_timer:
> + *
> + * Starts a timer for virtual clock update
> + */
> +void qemu_start_warp_timer(void);
> +
> +/**
> + * qemu_account_warp_timer:
>   *
> - * Warp a clock to a new value
> + * Updates virtual clock for the time CPU was sleeping
>   */
> -void qemu_clock_warp(QEMUClockType type);
> +void qemu_account_warp_timer(void);
>  
>  /**
>   * qemu_clock_register_reset_notifier:
> diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
> index 4763e56..6c332e5 100644
> --- a/include/sysemu/replay.h
> +++ b/include/sysemu/replay.h
> @@ -27,7 +27,8 @@ typedef enum ReplayClockKind ReplayClockKind;
>  
>  /* IDs of the checkpoints */
>  enum ReplayCheckpoint {
> -    CHECKPOINT_CLOCK_WARP,
> +    CHECKPOINT_CLOCK_WARP_START,
> +    CHECKPOINT_CLOCK_WARP_ACCOUNT,
>      CHECKPOINT_RESET_REQUESTED,
>      CHECKPOINT_SUSPEND_REQUESTED,
>      CHECKPOINT_CLOCK_VIRTUAL,
> diff --git a/main-loop.c b/main-loop.c
> index 19beae7..3a7f4cd 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -509,7 +509,7 @@ int main_loop_wait(int nonblocking)
>  
>      /* CPU thread can infinitely wait for event after
>         missing the warp */
> -    qemu_clock_warp(QEMU_CLOCK_VIRTUAL);
> +    qemu_start_warp_timer();
>      qemu_clock_run_all_timers();
>  
>      return ret;
> diff --git a/qemu-timer.c b/qemu-timer.c
> index e98ecc9..4441fe6 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -394,7 +394,9 @@ static bool timer_mod_ns_locked(QEMUTimerList *timer_list,
>  static void timerlist_rearm(QEMUTimerList *timer_list)
>  {
>      /* Interrupt execution to force deadline recalculation.  */
> -    qemu_clock_warp(timer_list->clock->type);
> +    if (timer_list->clock->type == QEMU_CLOCK_VIRTUAL) {
> +        qemu_start_warp_timer();
> +    }
>      timerlist_notify(timer_list);
>  }
>  
> diff --git a/stubs/clock-warp.c b/stubs/clock-warp.c
> index 5ae32b9..8acb58a 100644
> --- a/stubs/clock-warp.c
> +++ b/stubs/clock-warp.c
> @@ -2,7 +2,7 @@
>  #include "qemu-common.h"
>  #include "qemu/timer.h"
>  
> -void qemu_clock_warp(QEMUClockType type)
> +void qemu_start_warp_timer(void)
>  {
>  }
>  
>