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

[David Hildenbrand]

On 09.01.23 20:54, Peter Xu wrote:
> On Mon, Jan 09, 2023 at 03:34:48PM +0100, David Hildenbrand wrote:
> > On 05.01.23 18:15, Peter Xu wrote:
> > > On Thu, Jan 05, 2023 at 09:35:54AM +0100, David Hildenbrand wrote:
> > > > On 04.01.23 18:23, Peter Xu wrote:
> > > > > On Thu, Dec 22, 2022 at 12:02:10PM +0100, David Hildenbrand wrote:
> > > > > > Migrating device state before we start iterating is currently impossible.
> > > > > > Introduce and use qemu_savevm_state_start_precopy(), and use
> > > > > > a new special migration priority -- MIG_PRI_POST_SETUP -- to decide whether
> > > > > > state will be saved in qemu_savevm_state_start_precopy() or in
> > > > > > qemu_savevm_state_complete_precopy_*().
> > > > >
> > > > > Can something like this be done in qemu_savevm_state_setup()?
> > > >
> > > > Hi Peter,
> > >
> > > Hi, David,
> > >
> > > > Do you mean
> > > >
> > > > (a) Moving qemu_savevm_state_start_precopy() effectively into
> > > >       qemu_savevm_state_setup()
> > > >
> > > > (b) Using se->ops->save_setup()
> > >
> > > I meant (b).
> > >
> > > > I first tried going via (b), but decided to go the current way of using a
> > > > proper vmstate with properties (instead of e.g., filling the stream
> > > > manually), which also made vmdesc handling possible (and significantly
> > > > cleaner).
> > > >
> > > > Regarding (a), I decided to not move logic of
> > > > qemu_savevm_state_start_precopy() into qemu_savevm_state_setup(), because it
> > > > looked cleaner to save device state with the BQL held and for background
> > > > snapshots, the VM has been stopped. To decouple device state saving from the
> > > > setup path, just like we do it right now for all vmstates.
> > >
> > > Is BQL required or optional?  IIUC it's at least still not taken in the
> > > migration thread path, only in savevm path.
> > >
> > > > Having that said, for virtio-mem, it would still work because that state is
> > > > immutable once migration starts, but it felt cleaner to separate the setup()
> > > > phase from actual device state saving.
> > >
> > > I get the point.  My major concerns are:
> > >
> > >     (1) The new migration priority is changing the semantic of original,
> > >         making it over-complicated
> > >
> > >     (2) The new precopy-start routine added one more step to the migration
> > >         framework, while it's somehow overlapping (if not to say, mostly the
> > >         same as..) save_setup().
> > >
> > > For (1): the old priority was only deciding the order of save entries in
> > > the global list, nothing more than that.  Even if we want to have a
> > > precopy-start phase, I'd suggest we use something else and keep the
> > > migration priority simple.  Otherwise we really need serious documentation
> > > for MigrationPriority and if so I'd rather don't bother and not reuse the
> > > priority field.
> > >
> > > For (2), if you see there're a bunch of save_setup() that already does
> > > things like transferring static data besides the device states.  Besides
> > > the notorious ram_save_setup() there's also dirty_bitmap_save_setup() which
> > > also sends a bitmap during save_setup() and some others.  It looks clean to
> > > me to do it in the same way as we used to.
> > >
> > > Reusing vmstate_save() and vmsd structures are useful too which I totally
> > > agree.  So.. can we just call vmstate_save_state() in the save_setup() of
> > > the other new vmsd of virtio-mem?
> >
> >
> > I went halfway that way, by moving stuff into qemu_savevm_state_setup()
> > and avoiding using a new migration priority. Seems to work:
>
> The whole point of my suggestion is not moving things into
> qemu_savevm_state_setup(), but avoid introducing more complexity to the
> migration framework if unnecessary, so keep the generic framework as simple
> as possible.

IMHO, the current approach is actually quite simple and clean. But ...
> > I think we could go one step further and perform it from a save_setup() 
> > callback,
> > however, I'm not convinced that this gets particularly cleaner (vmdesc handling
> > eventually).
>
> What I wanted to suggest is exactly trying to avoid vmsd handling.  To be
> more explicit, I mean: besides vmstate_virtio_mem_device_early, virtio-mem
> can register with another new SaveVMHandlers with both save_setup and
> load_setup registered, then e.g. in its save_setup(), one simply calls:

... I can see if it can be made working that way and how the result looks. I 
know
that we use vmstate_save_state() from virtio code, but I don't remember using
it in save_setup() from QEMU_VM_SECTION_START and not QEMU_VM_SECTION_FULL.


There is this interesting bit in register_savevm_live(), which sets "se->is_ram = 
1".
qemu_save_device_state() will not include the state. As it's used by XEN, I 
don't
particularly care.


>    vmstate_save_state(f, &vmstate_virtio_mem_device_early, virtio_mem_dev,
>                       NULL);
>
> I'm not sure whether the JSONWriter* is required in this case, maybe not
> yet to at least make it work.

It was required when handling vmstates the current way to make
analyze-migration.py not bail out (which is a good thing because one can
actually inspect the migration content):

$ ./scripts/analyze-migration.py -f STATEFILE
{
    "ram (2)": {
        "section sizes": {
            "0000:00:03.0/mem0": "0x0000000f00000000",
            "pc.ram": "0x0000000100000000",
            "/rom@etc/acpi/tables": "0x0000000000020000",
            "pc.bios": "0x0000000000040000",
            "0000:00:02.0/e1000.rom": "0x0000000000040000",
            "pc.rom": "0x0000000000020000",
            "/rom@etc/table-loader": "0x0000000000001000",
            "/rom@etc/acpi/rsdp": "0x0000000000001000"
        }
    },
    "0000:00:03.0/virtio-mem-device-early (51)": {
        "tmp": "00 00 00 01 40 00 00 00 00 00 00 0f 00 00 00 00 00 00 00 00 00 20 00 
00 00 00 00 00",
        "size": "0x0000000000000000",
        "bitmap": "00 00 00 00 00 00 00 00 00 00
        ...
    },
    "timer (0)": {
...

> We'll need to impl the load part, but then IIUC we don't need to touch the
> migration framework at all, and we keep all similar things (like other
> devices I mentioned) to be inside save_setup().
>
> Would that work?

Let me play with it.

--
Thanks,

David / dhildenb