Re: [Qemu-devel] [PATCH v3 4/4] Optimize record/replay checkpointing for all clocks it applies to

["Pavel Dovgalyuk" <dovgaluk@ispras.ru>]

> From: Artem Pisarenko [mailto:artem.k.pisarenko@gmail.com]
> Removes redundant checkpoints in replay log when there are no expired timers in timers list,
> associated with corresponding clock (i.e. no rr events associated with current clock value).
> This also improves performance in rr mode.
> 
> Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
> ---
> 
> Notes:
>     v3:
>     - fixed compiler warning caused non-debug build to fail
> 
>  include/qemu/timer.h |  2 +-
>  util/qemu-timer.c    | 62 +++++++++++++++++++++++++---------------------------
>  2 files changed, 31 insertions(+), 33 deletions(-)
> 
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index dc0fd14..bff8dac 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -65,7 +65,7 @@ typedef enum {
>   * QEMU_TIMER_ATTR_EXTERNAL: drives external subsystem
>   *
>   * Timers with this attribute do not recorded in rr mode, therefore it could be
> - * used for the subsystems that operate outside the guest core. Applicable only
> + * used for the subsystems that operate outside the guest core. Relevant only
>   * with virtual clock type.
>   */
> 
> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
> index e2746cf..216d107 100644
> --- a/util/qemu-timer.c
> +++ b/util/qemu-timer.c
> @@ -490,6 +490,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>      QEMUTimerCB *cb;
>      void *opaque;
>      bool need_replay_checkpoint = false;
> +    ReplayCheckpoint replay_checkpoint_id;
> 
>      if (!atomic_read(&timer_list->active_timers)) {
>          return false;
> @@ -500,43 +501,40 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>          goto out;
>      }
> 
> -    switch (timer_list->clock->type) {
> -    case QEMU_CLOCK_REALTIME:
> -        break;
> -    default:
> -    case QEMU_CLOCK_VIRTUAL:
> -        if (replay_mode != REPLAY_MODE_NONE) {
> -            /* Checkpoint for virtual clock is redundant in cases where
> -             * it's being triggered with only non-EXTERNAL timers, because
> -             * these timers don't change guest state directly.
> -             * Since it has conditional dependence on specific timers, it is
> -             * subject to race conditions and requires special handling.
> -             * See below.
> -             */
> +    if (replay_mode != REPLAY_MODE_NONE) {
> +        /* Postpone actual checkpointing to timer list processing
> +         * to properly check if we actually need it.
> +         */
> +        switch (timer_list->clock->type) {
> +        case QEMU_CLOCK_VIRTUAL:
>              need_replay_checkpoint = true;
> +            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL;
> +            break;
> +        case QEMU_CLOCK_HOST:
> +            need_replay_checkpoint = true;
> +            replay_checkpoint_id = CHECKPOINT_CLOCK_HOST;
> +            break;

This is wrong at least for QEMU_CLOCK_HOST.

> +        case QEMU_CLOCK_VIRTUAL_RT:
> +            need_replay_checkpoint = true;
> +            replay_checkpoint_id = CHECKPOINT_CLOCK_VIRTUAL_RT;
> +            break;
> +        default:
> +            break;
>          }
> -        break;
> -    case QEMU_CLOCK_HOST:
> -        if (!replay_checkpoint(CHECKPOINT_CLOCK_HOST)) {
> -            goto out;
> -        }
> -        break;
> -    case QEMU_CLOCK_VIRTUAL_RT:
> -        if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL_RT)) {
> -            goto out;
> -        }
> -        break;
>      }
> 
>      /*
> -     * Extract expired timers from active timers list and and process them.
> +     * Extract expired timers from active timers list and and process them,
> +     * taking into account checkpointing required in rr mode.
>       *
> -     * In rr mode we need "filtered" checkpointing for virtual clock.
> -     * Checkpoint must be replayed before any non-EXTERNAL timer has been
> -     * processed and only one time (virtual clock value stays same). But these
> -     * timers may appear in the timers list while it being processed, so this
> -     * must be checked until we finally decide that "no timers left - we are
> -     * done".
> +     * Checkpoint must be replayed before any timer has been processed
> +     * and only one time. But new timers may appear in the timers list while
> +     * it's being processed, so this must be checked until we finally decide
> +     * that "no timers left - we are done" (to avoid skipping checkpoint due to
> +     * possible races).
> +     * Also checkpoint for virtual clock is redundant in cases where it's being
> +     * triggered with only non-EXTERNAL timers, because these timers don't
> +     * change guest state directly.
>       */
>      current_time = qemu_clock_get_ns(timer_list->clock->type);

Reading the host clock here is not protected by the checkpoint.
Therefore it may incur the inconsistency when replaying the execution.

>      qemu_mutex_lock(&timer_list->active_timers_lock);
> @@ -552,7 +550,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
>              /* once we got here, checkpoint clock only once */
>              need_replay_checkpoint = false;
>              qemu_mutex_unlock(&timer_list->active_timers_lock);
> -            if (!replay_checkpoint(CHECKPOINT_CLOCK_VIRTUAL)) {
> +            if (!replay_checkpoint(replay_checkpoint_id)) {
>                  goto out;
>              }
>              qemu_mutex_lock(&timer_list->active_timers_lock);
> --
> 2.7.4


Pavel Dovgalyuk