Re: [PATCH] replay: fix icount request when replaying clock access

[Paolo Bonzini <pbonzini@redhat.com>]

On 16/02/21 13:51, Pavel Dovgalyuk wrote:
> Record/replay provides REPLAY_CLOCK_LOCKED macro to access
> the clock when vm_clock_seqlock is locked. This macro is
> needed because replay internals operate icount. In locked case
> replay use icount_get_raw_locked for icount request, which prevents
> excess locking which leads to deadlock. But previously only
> record code used *_locked function and replay did not.
> Therefore sometimes clock access lead to deadlocks.
> This patch fixes clock access for replay too and uses *_locked
> icount access function.
> 
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> ---
>   include/sysemu/replay.h  |   14 ++++++++------
>   replay/replay-internal.c |   29 +++++++++++++++++++++++++----
>   replay/replay-time.c     |    4 ++--
>   replay/replay.c          |   23 +----------------------
>   stubs/replay-tools.c     |    2 +-
>   5 files changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
> index 56c0c17c30..0f3b0f7eac 100644
> --- a/include/sysemu/replay.h
> +++ b/include/sysemu/replay.h
> @@ -128,18 +128,20 @@ bool replay_has_interrupt(void);
>   int64_t replay_save_clock(ReplayClockKind kind, int64_t clock,
>                             int64_t raw_icount);
>   /*! Read the specified clock from the log or return cached data */
> -int64_t replay_read_clock(ReplayClockKind kind);
> +int64_t replay_read_clock(ReplayClockKind kind, int64_t raw_icount);
>   /*! Saves or reads the clock depending on the current replay mode. */
>   #define REPLAY_CLOCK(clock, value)                                      \
> -    (replay_mode == REPLAY_MODE_PLAY ? replay_read_clock((clock))       \
> +    (replay_mode == REPLAY_MODE_PLAY                                    \
> +        ? replay_read_clock((clock), icount_get_raw())                  \
>           : replay_mode == REPLAY_MODE_RECORD                             \
> -            ? replay_save_clock((clock), (value), icount_get_raw()) \
> -        : (value))
> +            ? replay_save_clock((clock), (value), icount_get_raw())     \
> +            : (value))
>   #define REPLAY_CLOCK_LOCKED(clock, value)                               \
> -    (replay_mode == REPLAY_MODE_PLAY ? replay_read_clock((clock))       \
> +    (replay_mode == REPLAY_MODE_PLAY                                    \
> +        ? replay_read_clock((clock), icount_get_raw_locked())           \
>           : replay_mode == REPLAY_MODE_RECORD                             \
>               ? replay_save_clock((clock), (value), icount_get_raw_locked()) \
> -        : (value))
> +            : (value))
>   
>   /* Processing data from random generators */
>   
> diff --git a/replay/replay-internal.c b/replay/replay-internal.c
> index 2e8a3e947a..77d0c82327 100644
> --- a/replay/replay-internal.c
> +++ b/replay/replay-internal.c
> @@ -247,10 +247,31 @@ void replay_advance_current_icount(uint64_t current_icount)
>       /* Time can only go forward */
>       assert(diff >= 0);
>   
> -    if (diff > 0) {
> -        replay_put_event(EVENT_INSTRUCTION);
> -        replay_put_dword(diff);
> -        replay_state.current_icount += diff;
> +    if (replay_mode == REPLAY_MODE_RECORD) {
> +        if (diff > 0) {
> +            replay_put_event(EVENT_INSTRUCTION);
> +            replay_put_dword(diff);
> +            replay_state.current_icount += diff;
> +        }
> +    } else if (replay_mode == REPLAY_MODE_PLAY) {
> +        if (diff > 0) {
> +            replay_state.instruction_count -= diff;
> +            replay_state.current_icount += diff;
> +            if (replay_state.instruction_count == 0) {
> +                assert(replay_state.data_kind == EVENT_INSTRUCTION);
> +                replay_finish_event();
> +                /* Wake up iothread. This is required because
> +                    timers will not expire until clock counters
> +                    will be read from the log. */
> +                qemu_notify_event();
> +            }
> +        }
> +        /* Execution reached the break step */
> +        if (replay_break_icount == replay_state.current_icount) {
> +            /* Cannot make callback directly from the vCPU thread */
> +            timer_mod_ns(replay_break_timer,
> +                qemu_clock_get_ns(QEMU_CLOCK_REALTIME));
> +        }
>       }
>   }
>   
> diff --git a/replay/replay-time.c b/replay/replay-time.c
> index 43357c9f24..00ebcb7a49 100644
> --- a/replay/replay-time.c
> +++ b/replay/replay-time.c
> @@ -46,12 +46,12 @@ void replay_read_next_clock(ReplayClockKind kind)
>   }
>   
>   /*! Reads next clock event from the input. */
> -int64_t replay_read_clock(ReplayClockKind kind)
> +int64_t replay_read_clock(ReplayClockKind kind, int64_t raw_icount)
>   {
>       int64_t ret;
>       g_assert(replay_file && replay_mutex_locked());
>   
> -    replay_account_executed_instructions();
> +    replay_advance_current_icount(raw_icount);
>   
>       if (replay_next_event_is(EVENT_CLOCK + kind)) {
>           replay_read_next_clock(kind);
> diff --git a/replay/replay.c b/replay/replay.c
> index d4c228ab28..c806fec69a 100644
> --- a/replay/replay.c
> +++ b/replay/replay.c
> @@ -94,28 +94,7 @@ void replay_account_executed_instructions(void)
>       if (replay_mode == REPLAY_MODE_PLAY) {
>           g_assert(replay_mutex_locked());
>           if (replay_state.instruction_count > 0) {
> -            int count = (int)(replay_get_current_icount()
> -                              - replay_state.current_icount);
> -
> -            /* Time can only go forward */
> -            assert(count >= 0);
> -
> -            replay_state.instruction_count -= count;
> -            replay_state.current_icount += count;
> -            if (replay_state.instruction_count == 0) {
> -                assert(replay_state.data_kind == EVENT_INSTRUCTION);
> -                replay_finish_event();
> -                /* Wake up iothread. This is required because
> -                   timers will not expire until clock counters
> -                   will be read from the log. */
> -                qemu_notify_event();
> -            }
> -            /* Execution reached the break step */
> -            if (replay_break_icount == replay_state.current_icount) {
> -                /* Cannot make callback directly from the vCPU thread */
> -                timer_mod_ns(replay_break_timer,
> -                    qemu_clock_get_ns(QEMU_CLOCK_REALTIME));
> -            }
> +            replay_advance_current_icount(replay_get_current_icount());
>           }
>       }
>   }
> diff --git a/stubs/replay-tools.c b/stubs/replay-tools.c
> index c06b360e22..43296b3d4e 100644
> --- a/stubs/replay-tools.c
> +++ b/stubs/replay-tools.c
> @@ -13,7 +13,7 @@ int64_t replay_save_clock(unsigned int kind, int64_t clock, int64_t raw_icount)
>       return 0;
>   }
>   
> -int64_t replay_read_clock(unsigned int kind)
> +int64_t replay_read_clock(unsigned int kind, int64_t raw_icount)
>   {
>       abort();
>       return 0;
> 

I can't honestly say I understand this very well, but it's all in 
replay_mode != REPLAY_MODE_NONE so queued, thanks.

Paolo