Re: [PATCH v3 3/8] migration/savevm: Allow immutable device state to be migrated early (i.e., before RAM)

[David Hildenbrand]

On 12.01.23 18:56, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
> > For virtio-mem, we want to have the plugged/unplugged state of memory
> > blocks available before migrating any actual RAM content, and perform
> > sanity checks before touching anything on the destination. This
> > information is immutable on the migration source while migration is active,
> >
> > We want to use this information for proper preallocation support with
> > migration: currently, we don't preallocate memory on the migration target,
> > and especially with hugetlb, we can easily run out of hugetlb pages during
> > RAM migration and will crash (SIGBUS) instead of catching this gracefully
> > via preallocation.
> >
> > Migrating device state via a vmsd before we start iterating is currently
> > impossible: the only approach that would be possible is avoiding a vmsd
> > and migrating state manually during save_setup(), to be restored during
> > load_state().
> >
> > Let's allow for migrating device state via a vmsd early, during the
> > setup phase in qemu_savevm_state_setup(). To keep it simple, we
> > indicate applicable vmds's using an "immutable" flag.
> >
> > Note that only very selected devices (i.e., ones seriously messing with
> > RAM setup) are supposed to make use of such early state migration.
> >
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> >   include/migration/vmstate.h |  5 +++++
> >   migration/savevm.c          | 14 ++++++++++++++
> >   2 files changed, 19 insertions(+)
> >
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index ad24aa1934..dd06c3abad 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -179,6 +179,11 @@ struct VMStateField {
> >   struct VMStateDescription {
> >       const char *name;
> >       int unmigratable;
> > +    /*
> > +     * The state is immutable while migration is active and is saved
> > +     * during the setup phase, to be restored early on the destination.
> > +     */
> > +    int immutable;
>
> A bool would be nicer (as it would for unmigratable above)

Yes, I chose an int for consistency with "unmigratable". I can turn that 
into a bool.

I'd even include a cleanup patch for unmigratable if it wouldn't be ...

$ git grep "unmigratable \=" | wc -l
29

> >       int version_id;
> >       int minimum_version_id;
> >       MigrationPriority priority;
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index ff2b8d0064..536d6f662b 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1200,6 +1200,15 @@ void qemu_savevm_state_setup(QEMUFile *f)
> >   
> >       trace_savevm_state_setup();
> >       QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> > +        if (se->vmsd && se->vmsd->immutable) {
> > +            ret = vmstate_save(f, se, ms->vmdesc);
> > +            if (ret) {
> > +                qemu_file_set_error(f, ret);
> > +                break;
> > +            }
> > +            continue;
> > +        }
> > +
>
> Does this give you the ordering you want? i.e. there's no guarantee here
> that immutables come first?

Yes, for virtio-mem at least this is fine. There are no real ordering 
requirements in regard to save_setup().

I guess one could use vmstate priorities to affect the ordering, if 
required.

So for my use case this is good enough, any suggestions? Thanks.

--
Thanks,

David / dhildenb