| Message ID | 161347990483.1313189.8371838968343494161.stgit@pasha-ThinkPad-X280 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | replay: fix icount request when replaying clock access | expand |
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
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;
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(-)