Re: [Qemu-devel] [PATCH v16 00/21] Deterministic replay core

["Pavel Dovgaluk" <Pavel.Dovgaluk@ispras.ru>]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> unfortunately I do have some more review comments; that can happen when
> going back to the code after a few months, and it's also a good thing
> because it means that the code _is_ actually getting cleaner.

Thanks for review.

> However, I am fairly sure that v17 is going to be the good one and will
> be in 2.5 if I get it by mid September when I'll be back from vacation.
> 
> In particular:
> 
> * patch 3 seems to be unnecessary (for now at least)

Done.

> * replay_next_event_is is modified in patch 8 ("cpu: replay instructions
> sequence") after it's introduced in patch 6 ("replay: introduce icount
> event").  Please fold the change in patch 6.

Done.

> * replay_add_event is not used; please remove it, rename
> replay_add_event_internal to replay_add_event, and add an assertion that
> the rr mode is the right one (e.g. RECORD only?)

Done.

> * a couple of comments say "grteater" instead of "greater"

Done.

> * the replay_save_clock and replay_read_clock stubs should abort

Done.

> * please inline replay_input_event into qemu_input_event_send and
> replay_input_sync_event into qemu_input_event_sync, so that the
> corresponding *_impl functions can be static; this also means moving the
> prototypes of replay_add_input_event and replay_add_input_sync_event to
> replay/replay.h (I acknowledge this might undo a request from a previous
> review of mine; I don't remember)

I can't. *_impl functions are called to process input events, when they are
waiting in the events queue.

> * most stubs are unnecessary (replay_get_current_step,
> replay_checkpoint, qemu_system_shutdown_request,
> qemu_input_event_send_impl, qemu_input_event_sync_impl)

replay_checkpiont is required by qemu-timer.c


> 
> * please squash this in "replay: checkpoints"

Ok.

> And a few questions.  The first three are the "if the answer is yes,
> please do this" kind to questions, the others can have more
> open/subjective answers:
> 
> * does it make sense to call replay_check_error from
> replay_finish_event, and remove the call from replay_read_next_clock?

No, read errors are checked just after file read operations.
replay_finish_event usually is not coupled with any reads.

> * should qemu_clock_use_for_deadline always return false in replay mode?
> The clocks are all deterministic, so it doesn't make sense to take them
> into account in the poll() deadline.

It could be host clock or virtual_rt. In both cases qemu_soonest_timeout
will write some clock reads into the events log. In play we should read
these clocks to avoid inconsistency.

> * now that qemu_clock_warp has to be called in main_loop_wait, should
> the icount_warp_timer have a dummy callback?  icount_warp_rt is then
> only called from qemu_clock_warp.  If so, this (adding the call to
> qemu_clock_warp in main_loop_wait, making the icount_warp_timer dummy,
> removing the now-unnecessary argument of icount_warp_rt) should be a
> separate patch before "replay: checkpoints"

It seems that icount_warp_rt may be called without timer.
I'll prepare a patch.

> * can you explain why both CHECKPOINT_INIT and CHECKPOINT_RESET are
> needed?  What events are typically found in each of them?

'Init' and 'reset' checkpoints is used to split initialization and
resetting functions. Some of them interact with log file (e.g., write clock
values). These saved events may be used by other function than should
be in normal execution. Checkpoints prevent reading more clock values
than needed by polling functions, called when initializing the hardware.

> * would it make sense to test "replay_mode != REPLAY_MODE_NONE &&
> !runstate_is_running()" in replay_checkpoint, for all checkpoints, like
> 
>   if (replay_mode != REPLAY_MODE_NONE && !runstate_is_running()) {
>       return false;
>   }

It could be useful in case when machine is stopped just before
checkpoint. I'll fix this.

> * do we need an event for suspend?

Maybe, but the difference probably won't exhibit itself on a diskless system.

Pavel Dovgalyuk