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

[David Hildenbrand]

[...]

> > > It might be OK if you just change the declaration; I mean '1' is pretty
> > > close to true? (I think...)
> > > Anyway, at least make the new one a bool.
> >
> > Agreed bool is better.  Can we rename it to something like "early_setup"?
> > "immutable" isn't clear on its most important attribute (on when it'll be
> > migrated).  Meanwhile I'd hope we can comment that explicitly.  I'd go with:
> >
> >    /*
> >     * This VMSD describes something that should be sent during setup phase
> >     * of migration.  It plays similar role as save_setup() for explicitly
> >     * registered vmstate entries, the only difference is the vmsd will be
> >     * sent right at the start of migration.
> >     */
> >    bool early_setup;
>
> Let me try some even better wording..
>
>      /*
>       * This VMSD describes something that should be sent during setup phase
>       * of migration.  It plays similar role as save_setup() for explicitly
>       * registered vmstate entries, so it can be seen as a way to describe
>       * save_setup() in vmsd structures.
>       *
>       * One SaveStateEntry should either have the save_setup() specified or
>       * the vmsd with early_setup set to true.  It should never have both
>       * things set.
>       */
>      bool early_setup;

Thanks, I'll use that.

> There's one tricky thing that we'll send QEMU_VM_SECTION_START for
> save_setup() entries but QEMU_VM_SECTION_FULL for vmsd early_setup
> entries.

I think that makes sense for now, though: we only transmit a VMSD and 
VMSDs are transmitted once and are not iterable.

In comparison, for iterable things we expect a

QEMU_VM_SECTION_START
0..X QEMU_VM_SECTION_PART
QEMU_VM_SECTION_END


I assume you're thinking about "mixing" save_state() with an early vmsd 
in a SaveStateEntry. I don't think something like that would currently 
work (I'm pretty sure the core would have a hard time figuring out if to 
restore a vmsd or whether to send the input to load_state()?), neither 
can it be configured: we wither have se->ops or se->vmsd.

> David, do you think we can slightly modify your new version of
> vmstate_save() so as to pass in the section_type?  I think it'll be even
> cleaner to send QEMU_VM_SECTION_START for the early vmsds too.  I assume
> this shouldn't affect your goal and anything else.

I'd prefer to not go down that path for now. QEMU_VM_SECTION_START 
without QEMU_VM_SECTION_PART and QEMU_VM_SECTION_END feels pretty 
incomplete and wrong to me.

If we want to do that in the future, we should conditionally send 
QEMU_VM_SECTION_START only if we have se->ops I assume?

> > > > > >        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.
> > >
> > > OK, but consider whether it might be better just to have a separate
> > > QTAILQ_FOREACH look in savevm_state_setup that first does all the
> > > immutables, and then all the setups.
> >
> > After patch 1 the order may not matter iiuc, because each call to the
> > immutable vmsds calls the new vmstate_save() which will always send
> > QEMU_VM_SECTION_FULL and footers along the vmsd.

Agreed. I'll leave it like that for now.

Thanks!

--
Thanks,

David / dhildenb