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

[Paolo Bonzini]

On 15/08/2015 12:03, Paolo Bonzini wrote:
> 
> 
> On 15/08/2015 11:57, Pavel Dovgalyuk wrote:
>> Hi, Paolo!
>>
>> Will you apply these patches to 2.5?
> 
> Yes, I'll put them in my next pull request.

Hi Pavel,

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.

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)

* 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.

* 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?)

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

* the replay_save_clock and replay_read_clock stubs should abort

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

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

* please squash this in "replay: checkpoints"

diff --git a/vl.c b/vl.c
index 5a509dc..3c69563 100644
--- a/vl.c
+++ b/vl.c
@@ -1662,8 +1662,11 @@ static void qemu_kill_report(void)
 static int qemu_reset_requested(void)
 {
     int r = reset_requested;
-    reset_requested = 0;
-    return r;
+    if (r && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) {
+        reset_requested = 0;
+        return r;
+    }
+    return false;
 }

 static int qemu_suspend_requested(void)
@@ -1862,9 +1865,7 @@ static bool main_loop_should_exit(void)
             return true;
         }
     }
-    if (qemu_reset_requested_get()
-        && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) {
-        qemu_reset_requested();
+    if (qemu_reset_requested()) {
         pause_all_vcpus();
         cpu_synchronize_all_states();
         qemu_system_reset(VMRESET_REPORT);


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?

* 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.

* 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"

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

* 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;
  }

?

* do we need an event for suspend?

Thanks,

Paolo