Re: [Qemu-devel] [RFC PATCH v8 19/21] replay: initialization and deinitialization

[Paolo Bonzini]

On 22/01/2015 09:53, Pavel Dovgalyuk wrote:
> This patch introduces the functions for enabling the record/replay and for
> freeing the resources when simulator closes.
> 
> Signed-off-by: Pavel Dovgalyuk <address@hidden>
> ---
>  block.c                  |    2 -
>  exec.c                   |    1 
>  replay/replay-internal.c |    1 
>  replay/replay-internal.h |    4 +
>  replay/replay.c          |  140 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  replay/replay.h          |   13 ++++
>  stubs/replay.c           |    1 
>  vl.c                     |    5 ++
>  8 files changed, 165 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 7f6fa8b..9923a80 100644
> --- a/block.c
> +++ b/block.c
> @@ -2010,7 +2010,7 @@ void bdrv_drain_all(void)
>              aio_context_release(aio_context);
>          }
>      }
> -    if (replay_mode == REPLAY_MODE_PLAY) {
> +    if (replay_mode == REPLAY_MODE_PLAY && replay_checkpoints) {
>          /* Skip checkpoints from the log */
>          while (replay_checkpoint(CHECKPOINT_BDRV_DRAIN)) {
>              /* Nothing */
> diff --git a/exec.c b/exec.c
> index 410371d..f70f03d 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -783,6 +783,7 @@ void cpu_abort(CPUState *cpu, const char *fmt, ...)
>      }
>      va_end(ap2);
>      va_end(ap);
> +    replay_finish();
>  #if defined(CONFIG_USER_ONLY)
>      {
>          struct sigaction act;
> diff --git a/replay/replay-internal.c b/replay/replay-internal.c
> index 49b37a6..d608b71 100755
> --- a/replay/replay-internal.c
> +++ b/replay/replay-internal.c
> @@ -33,6 +33,7 @@ void replay_put_byte(uint8_t byte)
>  
>  void replay_put_event(uint8_t event)
>  {
> +    assert(event <= EVENT_END);

Please move (way) earlier in the series.

>      replay_put_byte(event);
>  }
>  
> diff --git a/replay/replay-internal.h b/replay/replay-internal.h
> index 4d242aa..1e5d037 100755
> --- a/replay/replay-internal.h
> +++ b/replay/replay-internal.h
> @@ -34,7 +34,9 @@ enum ReplayEvents {
>      EVENT_CLOCK,
>      /* for checkpoint event */
>      /* some of grteater codes are reserved for checkpoints */
> -    EVENT_CHECKPOINT = EVENT_CLOCK + REPLAY_CLOCK_COUNT
> +    EVENT_CHECKPOINT = EVENT_CLOCK + REPLAY_CLOCK_COUNT,
> +    /* end of log event */
> +    EVENT_END = EVENT_CHECKPOINT + CHECKPOINT_COUNT

Please add EVENT_END in the beginning of the series.  If you do:

    EVENT_CLOCK,
    EVENT_CLOCK_LAST = EVENT_CLOCK + REPLAY_CLOCK_COUNT - 1,
    EVENT_CHECKPOINT,
    EVENT_CHECKPOINT_LAST = EVENT_CHECKPOINT + CHECKPOINT_COUNT - 1,
    EVENT_END,

the patches look quite nice.

>  };
>  
>  /* Asynchronous events IDs */
> diff --git a/replay/replay.c b/replay/replay.c
> index 7c4a801..fa738c2 100755
> --- a/replay/replay.c
> +++ b/replay/replay.c
> @@ -15,9 +15,18 @@
>  #include "qemu/timer.h"
>  #include "sysemu/sysemu.h"
>  
> +/* Current version of the replay mechanism.
> +   Increase it when file format changes. */
> +#define REPLAY_VERSION              0xe02002

Any meaning?  Perhaps make it 'QRR\0' or something identifiable?

> +/* Size of replay log header */
> +#define HEADER_SIZE                 (sizeof(uint32_t) + sizeof(uint64_t))
> +
>  ReplayMode replay_mode = REPLAY_MODE_NONE;
>  
> +/* Name of replay file  */
> +static char *replay_filename;
>  ReplayState replay_state;
> +bool replay_checkpoints;

Why is replay_checkpoints only defined and used in this patch?  Is it a
duplicate of replay_events_enabled?

>  bool skip_async_events(int stop_event)
>  {
> @@ -167,6 +176,10 @@ void replay_shutdown_request(void)
>  bool replay_checkpoint(ReplayCheckpoint checkpoint)
>  {
>      bool res = false;
> +    if (!replay_checkpoints) {
> +        return true;
> +    }
> +
>      replay_save_instructions();
>  
>      if (replay_file) {
> @@ -199,3 +212,130 @@ bool replay_checkpoint(ReplayCheckpoint checkpoint)
>  
>      return true;
>  }
> +
> +static void replay_enable(const char *fname, int mode)
> +{
> +    const char *fmode = NULL;
> +    if (replay_file) {
> +        fprintf(stderr,
> +                "Replay: some record/replay operation is already started\n");

Would this have to be an assertion?

> +        return;
> +    }
> +
> +    switch (mode) {
> +    case REPLAY_MODE_RECORD:
> +        fmode = "wb";
> +        break;
> +    case REPLAY_MODE_PLAY:
> +        fmode = "rb";
> +        break;
> +    default:
> +        fprintf(stderr, "Replay: internal error: invalid replay mode\n");
> +        exit(1);
> +    }
> +
> +    atexit(replay_finish);
> +
> +    replay_mutex_init();
> +
> +    replay_file = fopen(fname, fmode);
> +    if (replay_file == NULL) {
> +        fprintf(stderr, "Replay: open %s: %s\n", fname, strerror(errno));
> +        exit(1);
> +    }
> +
> +    replay_filename = g_strdup(fname);
> +
> +    replay_mode = mode;
> +    replay_has_unread_data = 0;
> +    replay_data_kind = -1;
> +    replay_state.instructions_count = 0;
> +    replay_state.current_step = 0;
> +
> +    /* skip file header for RECORD and check it for PLAY */
> +    if (replay_mode == REPLAY_MODE_RECORD) {
> +        fseek(replay_file, HEADER_SIZE, SEEK_SET);

Why can't you write the header here?

> +    } else if (replay_mode == REPLAY_MODE_PLAY) {
> +        unsigned int version = replay_get_dword();
> +        uint64_t offset = replay_get_qword();
> +        if (version != REPLAY_VERSION) {
> +            fprintf(stderr, "Replay: invalid input log file version\n");
> +            exit(1);
> +        }
> +        /* go to the beginning */
> +        fseek(replay_file, 12, SEEK_SET);
> +    }
> +
> +    replay_init_events();
> +}
> +
> +void replay_configure(QemuOpts *opts)
> +{
> +    const char *fname;
> +    const char *rr;
> +    ReplayMode mode = REPLAY_MODE_NONE;
> +
> +    rr = qemu_opt_get(opts, "rr");
> +    if (!rr) {
> +        /* Just enabling icount */
> +        return;
> +    } else if (!strcmp(rr, "record")) {
> +        mode = REPLAY_MODE_RECORD;
> +    } else if (!strcmp(rr, "replay")) {
> +        mode = REPLAY_MODE_PLAY;
> +    } else {
> +        fprintf(stderr, "Invalid icount rr option: %s\n", rr);
> +        exit(1);
> +    }
> +
> +    fname = qemu_opt_get(opts, "rrfname");
> +    if (!fname) {
> +        fprintf(stderr, "File name not specified for replay\n");
> +        exit(1);
> +    }
> +
> +    replay_enable(fname, mode);
> +}
> +
> +void replay_init_timer(void)
> +{
> +    if (replay_mode == REPLAY_MODE_NONE) {
> +        return;
> +    }
> +
> +    replay_checkpoints = true;
> +    replay_enable_events();

Move if to replay_enable_events() and drop replay_init_timer() altogether?

> +}
> +
> +void replay_finish(void)
> +{
> +    if (replay_mode == REPLAY_MODE_NONE) {
> +        return;
> +    }
> +
> +    replay_save_instructions();
> +
> +    /* finalize the file */
> +    if (replay_file) {
> +        if (replay_mode == REPLAY_MODE_RECORD) {
> +            uint64_t offset = 0;
> +            /* write end event */
> +            replay_put_event(EVENT_END);
> +
> +            /* write header */
> +            fseek(replay_file, 0, SEEK_SET);
> +            replay_put_dword(REPLAY_VERSION);
> +            replay_put_qword(offset);

Why is this done here?  You're writing a constant zero.

> +        }
> +
> +        fclose(replay_file);
> +        replay_file = NULL;
> +    }
> +    if (replay_filename) {
> +        g_free(replay_filename);
> +        replay_filename = NULL;
> +    }
> +
> +    replay_mutex_destroy();
> +    replay_finish_events();
> +}
> diff --git a/replay/replay.h b/replay/replay.h
> index 3110d46..231b8ec 100755
> --- a/replay/replay.h
> +++ b/replay/replay.h
> @@ -17,6 +17,8 @@
>  #include <time.h>
>  #include "qapi-types.h"
>  
> +struct QemuOpts;

#include instead?

>  /* replay clock kinds */
>  enum ReplayClockKind {
>      /* rdtsc */
> @@ -45,6 +47,17 @@ enum ReplayCheckpoint {
>  typedef enum ReplayCheckpoint ReplayCheckpoint;
>  
>  extern ReplayMode replay_mode;
> +/*! Is set to true after finishing initialization */
> +extern bool replay_checkpoints;
> +
> +/* Replay process control functions */
> +
> +/*! Enables recording or saving event log with specified parameters */
> +void replay_configure(struct QemuOpts *opts);
> +/*! Initializes timers used for snapshotting and enables events recording */
> +void replay_init_timer(void);
> +/*! Closes replay log file and frees other resources. */
> +void replay_finish(void);
>  
>  /* Processing the instructions */
>  
> diff --git a/stubs/replay.c b/stubs/replay.c
> index 81eddae..7d87964 100755
> --- a/stubs/replay.c
> +++ b/stubs/replay.c
> @@ -2,6 +2,7 @@
>  #include "sysemu/sysemu.h"
>  
>  ReplayMode replay_mode;
> +bool replay_checkpoints;
>  
>  int64_t replay_save_clock(unsigned int kind, int64_t clock)
>  {
> diff --git a/vl.c b/vl.c
> index 86ba385..ae3e97e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4376,7 +4376,12 @@ int main(int argc, char **argv, char **envp)
>          }
>      }
>  
> +    replay_init_timer();

replay_enable_events()

>      main_loop();
> +    if (replay_mode != REPLAY_MODE_NONE) {
> +        replay_disable_events();

Move "if" to replay_disable_events.

Paolo

> +    }
>      bdrv_close_all();
>      pause_all_vcpus();
>      res_free();
>