[v3,4/4] Optimize record/replay checkpointing for all clocks it applies to

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(-)

> 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

> …
> This is wrong at least for QEMU_CLOCK_HOST.
> …
> Reading the host clock here is not protected by the checkpoint.
> Therefore it may incur the inconsistency when replaying the execution.

That's why I didn't like idea of this patch and asked for any possible side
effects beforehand. So, here is one.
I have no idea how to fix it properly and would like to recall this
particular patch from these series (remember, it doesn't match series
goal). Lets leave this patch/issue for separate resolution, although I'm
not very interested in it.

On 19/10/2018 08:30, Artem Pisarenko wrote:
>> This is wrong at least for QEMU_CLOCK_HOST.
>> …
>> Reading the host clock here is not protected by the checkpoint.
>> Therefore it may incur the inconsistency when replaying the execution.
> 
> That's why I didn't like idea of this patch and asked for any possible
> side effects beforehand. So, here is one.
> I have no idea how to fix it properly and would like to recall this
> particular patch from these series (remember, it doesn't match series
> goal). Lets leave this patch/issue for separate resolution, although I'm
> not very interested in it.

Yup, I'll send v2 of the pull request with this patch removed.

Paolo