Re: [PATCH 4/4] replay: simple auto-snapshot mode for record

[Nicholas Piggin]

On Fri Aug 18, 2023 at 2:36 PM AEST, Pavel Dovgalyuk wrote:
> On 14.08.2023 19:31, Nicholas Piggin wrote:
> > record makes an initial snapshot when the machine is created, to enable
> > reverse-debugging. Often the issue being debugged appears near the end of
> > the trace, so it is important for performance to keep snapshots close to
> > the end.
> > 
> > This implements a periodic snapshot mode that keeps a rolling set of
> > recent snapshots.
> > 
> > Arguably this should be done by the debugger or a program that talks to
> > QMP, but for setting up simple scenarios and tests, it is convenient to
> > have this feature.

I'm looking at resurrecting this to help add a bit of testing...

[snip]

> > +static void replay_snapshot_timer_cb(void *opaque)
> > +{
> > +    Error *err = NULL;
> > +    char *name;
> > +
> > +    if (!replay_can_snapshot()) {
> > +        /* Try again soon */
> > +        timer_mod(replay_snapshot_timer,
> > +                  qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> > +                  replay_snapshot_periodic_delay / 10);
> > +        return;
> > +    }
> > +
> > +    name = g_strdup_printf("%s-%d", replay_snapshot, 
> > replay_snapshot_count);
> > +    if (!save_snapshot(name,
> > +                       true, NULL, false, NULL, &err)) {
> > +        error_report_err(err);
> > +        error_report("Could not create periodic snapshot "
> > +                     "for icount record, disabling");
> > +        g_free(name);
> > +        return;
> > +    }
> > +    g_free(name);
> > +    replay_snapshot_count++;
> > +
> > +    if (replay_snapshot_periodic_nr_keep >= 1 &&
> > +        replay_snapshot_count > replay_snapshot_periodic_nr_keep) {
> > +        int del_nr;
> > +
> > +        del_nr = replay_snapshot_count - replay_snapshot_periodic_nr_keep 
> > - 1;
> > +        name = g_strdup_printf("%s-%d", replay_snapshot, del_nr);
> > +        if (!delete_snapshot(name, false, NULL, &err)) {
> > +            error_report_err(err);
> > +            error_report("Could not delete periodic snapshot "
> > +                         "for icount record");
> > +        }
> > +        g_free(name);
> > +    }
> > +
> > +    timer_mod(replay_snapshot_timer,
> > +              qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> > +              replay_snapshot_periodic_delay);
>
> I'm not sure that realtime is not the best choice for such of a timer.
> Virtual machine may be stopped or slowed down for some reason.

My thinking was that, say if you snapshot every 10 seconds of real time
executed, then you should have an upper limit on the order of 10 seconds
to perform a reverse-debug operation (so long as you don't exceed your
nr_keep limit).

Is it worth worrying about complexity of slowdowns and vm pausing?
Maybe it could stop snapshotting on a host pause.

> > +}
> > +
> >   void replay_vmstate_init(void)
> >   {
> >       Error *err = NULL;
> > @@ -81,6 +128,16 @@ void replay_vmstate_init(void)
> >                   error_report("Could not create snapshot for icount 
> > record");
> >                   exit(1);
> >               }
> > +
> > +            if (replay_snapshot_mode == REPLAY_SNAPSHOT_MODE_PERIODIC) {
> > +                replay_snapshot_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
> > +                                                     
> > replay_snapshot_timer_cb,
> > +                                                     NULL);
> > +                timer_mod(replay_snapshot_timer,
> > +                          qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> > +                          replay_snapshot_periodic_delay);
> > +            }
> > +
>
> Please also delete placeholder comment for the snapshotting timer
> in replay_enable function.

Wil do.

> >           } else if (replay_mode == REPLAY_MODE_PLAY) {
> >               if (!load_snapshot(replay_snapshot, NULL, false, NULL, &err)) 
> > {
> >                   error_report_err(err);
> > diff --git a/replay/replay.c b/replay/replay.c
> > index e64f71209a..fa5930700d 100644
> > --- a/replay/replay.c
> > +++ b/replay/replay.c
> > @@ -29,6 +29,10 @@
> >   ReplayMode replay_mode = REPLAY_MODE_NONE;
> >   char *replay_snapshot;
> >   
> > +ReplaySnapshotMode replay_snapshot_mode;
> > +uint64_t replay_snapshot_periodic_delay;
> > +int replay_snapshot_periodic_nr_keep;
> > +
> >   /* Name of replay file  */
> >   static char *replay_filename;
> >   ReplayState replay_state;
> > @@ -313,6 +317,27 @@ void replay_configure(QemuOpts *opts)
> >       }
> >   
> >       replay_snapshot = g_strdup(qemu_opt_get(opts, "rrsnapshot"));
> > +    if (replay_snapshot && mode == REPLAY_MODE_RECORD) {
>
> Can such a snapshotting may be useful in replay mode?

Does snapshotting do anything in replay mode? I assume if we did
snapshotting based on the machine timer then we'd have to support
it here so the timer events get replayed properly, at least. But
I was trying to get by with minimum complexity :)

Thanks,
Nick